Message ID | 80fb0709-5a0d-a3a5-b2ea-dca089714ac2@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | PCI: avoid bogus calls to get_pseg() | expand |
Hi Jan, > On 9 Aug 2022, at 4:50 pm, Jan Beulich <jbeulich@suse.com> wrote: > > When passed -1, the function (taking a u16) will look for segment > 0xffff, which might exist. If it exists, we may find (return) the wrong > device. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> I tested the patch on ARM and it works as expected. Reviewed-by: Rahul Singh <rahul.singh@arm.com> Tested-by: Rahul Singh <rahul.singh@arm.com> Regards, Rahul
On 09/08/2022 16:50, Jan Beulich wrote: > When passed -1, the function (taking a u16) will look for segment > 0xffff, which might exist. If it exists, we may find (return) the wrong > device. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > An alternative would be to declare that both functions cannot be called > with "wildcards" anymore. The last such use went away with f591755823a7 > ("IOMMU/PCI: don't let domain cleanup continue when device de-assignment > failed") afaict. The way wildcards were used before were always bogus IMO. I suggest we take this opportunity to remove the ability to re-introduce that anti-pattern. > Each time I look at this pair of functions I wonder why we have two > copies of almost the same code (with a curious difference of only one > having ASSERT(pcidevs_locked())). Any opinions on deleting either one, > subsuming its functionality into the other one by allowing the domain > pointer to be NULL to signify "any"? I'm in two minds about this. Because they are the same, they ought to be deduped. Except they're both insane and both want changing to a less silly datastructure, at which point they will be different. It is a total waste to do an O(n) loop over all PCI devices in the system checking for equality to single device (and in the domain case, assignment to the domain). The domain variant should loop over the pci devices in that domain, because it is guaranteed to be a shorter list than all devices. The global lookup probably wants to investigate a more efficient datastructure because I bet this is a hotpath. ~Andrew
On 10.08.2022 14:13, Andrew Cooper wrote: > On 09/08/2022 16:50, Jan Beulich wrote: >> When passed -1, the function (taking a u16) will look for segment >> 0xffff, which might exist. If it exists, we may find (return) the wrong >> device. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> An alternative would be to declare that both functions cannot be called >> with "wildcards" anymore. The last such use went away with f591755823a7 >> ("IOMMU/PCI: don't let domain cleanup continue when device de-assignment >> failed") afaict. > > The way wildcards were used before were always bogus IMO. > > I suggest we take this opportunity to remove the ability to re-introduce > that anti-pattern. Okay, will do that in v2. Rahul - this means there's no point anymore sending a v2 of your fix, as the bug will disappear as a side effect. I'll add you as the reporter of that bug. >> Each time I look at this pair of functions I wonder why we have two >> copies of almost the same code (with a curious difference of only one >> having ASSERT(pcidevs_locked())). Any opinions on deleting either one, >> subsuming its functionality into the other one by allowing the domain >> pointer to be NULL to signify "any"? > > I'm in two minds about this. Because they are the same, they ought to > be deduped. > > Except they're both insane and both want changing to a less silly > datastructure, at which point they will be different. > > It is a total waste to do an O(n) loop over all PCI devices in the > system checking for equality to single device (and in the domain case, > assignment to the domain). The domain variant should loop over the pci > devices in that domain, because it is guaranteed to be a shorter list > than all devices. With the "wildcard" support gone, that's going to be sensible, yes, and I'll switch to that. Except for hwdom, where I mean to stick to the per-segment all-devices lists, as on multi-segment systems these are very likely the shorter lists, while on single-segment systems the sole all-devices list likely isn't much longer than the list of hwdom's devices (the delta being all devices [intended to be] passed through plus all hidden devices). At this point I'm not sure whether it would be worth further special-casing the single-segment case, even if that's (on x86) the applicable one on the vast majority of systems. > The global lookup probably wants to investigate a more efficient > datastructure because I bet this is a hotpath. I don't think it's a hot path, but it can certainly be improved. Yet that's future work, nothing to be done right here. But I'm inferring you agree in this regard by saying "investigate". Jan
Hi Jan, > On 11 Aug 2022, at 8:02 am, Jan Beulich <jbeulich@suse.com> wrote: > > On 10.08.2022 14:13, Andrew Cooper wrote: >> On 09/08/2022 16:50, Jan Beulich wrote: >>> When passed -1, the function (taking a u16) will look for segment >>> 0xffff, which might exist. If it exists, we may find (return) the wrong >>> device. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> An alternative would be to declare that both functions cannot be called >>> with "wildcards" anymore. The last such use went away with f591755823a7 >>> ("IOMMU/PCI: don't let domain cleanup continue when device de-assignment >>> failed") afaict. >> >> The way wildcards were used before were always bogus IMO. >> >> I suggest we take this opportunity to remove the ability to re-introduce >> that anti-pattern. > > Okay, will do that in v2. Rahul - this means there's no point anymore > sending a v2 of your fix, as the bug will disappear as a side effect. > I'll add you as the reporter of that bug. Ok. I will test the patch once you sent it.. Regards, Rahul
--- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -578,20 +578,19 @@ int __init pci_ro_device(int seg, int bu struct pci_dev *pci_get_pdev(int seg, int bus, int devfn) { - struct pci_seg *pseg = get_pseg(seg); + struct pci_seg *pseg = NULL; struct pci_dev *pdev = NULL; ASSERT(pcidevs_locked()); ASSERT(seg != -1 || bus == -1); ASSERT(bus != -1 || devfn == -1); + if ( seg == -1 ) + radix_tree_gang_lookup(&pci_segments, (void **)&pseg, 0, 1); + else + pseg = get_pseg(seg); if ( !pseg ) - { - if ( seg == -1 ) - radix_tree_gang_lookup(&pci_segments, (void **)&pseg, 0, 1); - if ( !pseg ) - return NULL; - } + return NULL; do { list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list ) @@ -628,19 +627,18 @@ struct pci_dev *pci_get_real_pdev(int se struct pci_dev *pci_get_pdev_by_domain(const struct domain *d, int seg, int bus, int devfn) { - struct pci_seg *pseg = get_pseg(seg); + struct pci_seg *pseg = NULL; struct pci_dev *pdev = NULL; ASSERT(seg != -1 || bus == -1); ASSERT(bus != -1 || devfn == -1); + if ( seg == -1 ) + radix_tree_gang_lookup(&pci_segments, (void **)&pseg, 0, 1); + else + pseg = get_pseg(seg); if ( !pseg ) - { - if ( seg == -1 ) - radix_tree_gang_lookup(&pci_segments, (void **)&pseg, 0, 1); - if ( !pseg ) - return NULL; - } + return NULL; do { list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
When passed -1, the function (taking a u16) will look for segment 0xffff, which might exist. If it exists, we may find (return) the wrong device. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- An alternative would be to declare that both functions cannot be called with "wildcards" anymore. The last such use went away with f591755823a7 ("IOMMU/PCI: don't let domain cleanup continue when device de-assignment failed") afaict. Each time I look at this pair of functions I wonder why we have two copies of almost the same code (with a curious difference of only one having ASSERT(pcidevs_locked())). Any opinions on deleting either one, subsuming its functionality into the other one by allowing the domain pointer to be NULL to signify "any"?