| Message ID | 20230518105738.16695-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pci: fix pci_get_pdev() to always account for the segment | expand |
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
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.
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>
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
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.
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
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;
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(-)