[PATCH] pci: fix pci_get_pdev() to always account for the segment

Roger Pau Monne posted 1 patch 10 months, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20230518105738.16695-1-roger.pau@citrix.com
xen/drivers/passthrough/pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] pci: fix pci_get_pdev() to always account for the segment
Posted by Roger Pau Monne 10 months, 2 weeks ago
When a domain parameter is provided to pci_get_pdev() the search
function would match against the bdf, without taking the segment into
account.

Fix this and also account for the passed segment.

Fixes: 8cf6e0738906 ('PCI: simplify (and thus correct) pci_get_pdev{,_by_domain}()')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
There's no mention in 8cf6e0738906 that avoiding the segment check is
fine, and hence I assume it's an oversight, as it should be possible
to have devices from multiple segments assigned to the same domain.
---
 xen/drivers/passthrough/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index b42acb8d7c09..07d1986d330a 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -552,7 +552,7 @@ struct pci_dev *pci_get_pdev(const struct domain *d, pci_sbdf_t sbdf)
     }
     else
         list_for_each_entry ( pdev, &d->pdev_list, domain_list )
-            if ( pdev->sbdf.bdf == sbdf.bdf )
+            if ( pdev->sbdf.sbdf == sbdf.sbdf )
                 return pdev;
 
     return NULL;
-- 
2.40.0


Re: [PATCH] pci: fix pci_get_pdev() to always account for the segment
Posted by Jan Beulich 10 months, 2 weeks ago
On 18.05.2023 12:57, Roger Pau Monne wrote:
> When a domain parameter is provided to pci_get_pdev() the search
> function would match against the bdf, without taking the segment into
> account.
> 
> Fix this and also account for the passed segment.
> 
> Fixes: 8cf6e0738906 ('PCI: simplify (and thus correct) pci_get_pdev{,_by_domain}()')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> ---
> There's no mention in 8cf6e0738906 that avoiding the segment check is
> fine, and hence I assume it's an oversight, as it should be possible
> to have devices from multiple segments assigned to the same domain.

I guess this was a lack of editing after copy-and-paste from the
loops iterating over pseg->alldevs_list. Thanks much for spotting!

Jan

Re: [PATCH] pci: fix pci_get_pdev() to always account for the segment
Posted by Andrew Cooper 10 months, 2 weeks ago
On 18/05/2023 11:57 am, Roger Pau Monne wrote:
> When a domain parameter is provided to pci_get_pdev() the search
> function would match against the bdf, without taking the segment into
> account.
>
> Fix this and also account for the passed segment.
>
> Fixes: 8cf6e0738906 ('PCI: simplify (and thus correct) pci_get_pdev{,_by_domain}()')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> There's no mention in 8cf6e0738906 that avoiding the segment check is
> fine, and hence I assume it's an oversight, as it should be possible
> to have devices from multiple segments assigned to the same domain.

Oh, absolutely - skipping the segment check is very much not fine.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>


Re: [PATCH] pci: fix pci_get_pdev() to always account for the segment
Posted by Andrew Cooper 10 months, 2 weeks ago
On 18/05/2023 1:42 pm, Andrew Cooper wrote:
> On 18/05/2023 11:57 am, Roger Pau Monne wrote:
>> When a domain parameter is provided to pci_get_pdev() the search
>> function would match against the bdf, without taking the segment into
>> account.
>>
>> Fix this and also account for the passed segment.
>>
>> Fixes: 8cf6e0738906 ('PCI: simplify (and thus correct) pci_get_pdev{,_by_domain}()')
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>> There's no mention in 8cf6e0738906 that avoiding the segment check is
>> fine, and hence I assume it's an oversight, as it should be possible
>> to have devices from multiple segments assigned to the same domain.
> Oh, absolutely - skipping the segment check is very much not fine.
>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>

Sorry, I should go on to say.  Xen has had code for segments for years
and years and years, but I've seen plenty of evidence of Xen not having
any kind of regular testing in multi-segment systems.

Sapphire Rapids is the first platform I'm aware of which is
multi-segment in its base configuration and is going to see routine
testing with Xen.

I don't expect this to be the final bugfix before multi-segment works
properly...

~Andrew

Re: [PATCH] pci: fix pci_get_pdev() to always account for the segment
Posted by Roger Pau Monné 10 months, 2 weeks ago
On Thu, May 18, 2023 at 01:58:34PM +0100, Andrew Cooper wrote:
> On 18/05/2023 1:42 pm, Andrew Cooper wrote:
> > On 18/05/2023 11:57 am, Roger Pau Monne wrote:
> >> When a domain parameter is provided to pci_get_pdev() the search
> >> function would match against the bdf, without taking the segment into
> >> account.
> >>
> >> Fix this and also account for the passed segment.
> >>
> >> Fixes: 8cf6e0738906 ('PCI: simplify (and thus correct) pci_get_pdev{,_by_domain}()')
> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >> ---
> >> There's no mention in 8cf6e0738906 that avoiding the segment check is
> >> fine, and hence I assume it's an oversight, as it should be possible
> >> to have devices from multiple segments assigned to the same domain.
> > Oh, absolutely - skipping the segment check is very much not fine.
> >
> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >
> 
> Sorry, I should go on to say.  Xen has had code for segments for years
> and years and years, but I've seen plenty of evidence of Xen not having
> any kind of regular testing in multi-segment systems.
> 
> Sapphire Rapids is the first platform I'm aware of which is
> multi-segment in its base configuration and is going to see routine
> testing with Xen.
> 
> I don't expect this to be the final bugfix before multi-segment works
> properly...

I just found this by code inspection while looking at something else,
it wasn't related to any testing on multi segments systems.  IOW:
don't take this fix as me having done any kind of testing on multi
segment systems.

Thanks, Roger.

Re: [PATCH] pci: fix pci_get_pdev() to always account for the segment
Posted by Rahul Singh 10 months, 2 weeks ago
Hi Roger,

> On 18 May 2023, at 11:57 am, Roger Pau Monne <roger.pau@citrix.com> wrote:
> 
> When a domain parameter is provided to pci_get_pdev() the search
> function would match against the bdf, without taking the segment into
> account.
> 
> Fix this and also account for the passed segment.
> 
> Fixes: 8cf6e0738906 ('PCI: simplify (and thus correct) pci_get_pdev{,_by_domain}()')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
 
I think the correct fixes tag is:
Fixes: a37f9ea7a651 ("PCI: fold pci_get_pdev{,_by_domain}()")

With that:
Reviewed-by: Rahul Singh <rahul.singh@arm.com>
 

Regards,
Rahul
Re: [PATCH] pci: fix pci_get_pdev() to always account for the segment
Posted by Roger Pau Monné 10 months, 2 weeks ago
On Thu, May 18, 2023 at 12:14:46PM +0000, Rahul Singh wrote:
> Hi Roger,
> 
> > On 18 May 2023, at 11:57 am, Roger Pau Monne <roger.pau@citrix.com> wrote:
> > 
> > When a domain parameter is provided to pci_get_pdev() the search
> > function would match against the bdf, without taking the segment into
> > account.
> > 
> > Fix this and also account for the passed segment.
> > 
> > Fixes: 8cf6e0738906 ('PCI: simplify (and thus correct) pci_get_pdev{,_by_domain}()')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>  
> I think the correct fixes tag is:
> Fixes: a37f9ea7a651 ("PCI: fold pci_get_pdev{,_by_domain}()")

I don't think so, a37f9ea7a651 just changed:

         list_for_each_entry ( pdev, &d->pdev_list, domain_list )
-            if ( pdev->bus == bus && pdev->devfn == devfn )
+            if ( pdev->sbdf.bdf == sbdf.bdf )
                 return pdev;

That code was already wrong, a37f9ea7a651 simply switched it to use
the sbdf struct field.

Thanks, Roger.