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"?
--- 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 )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
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
© 2016 - 2024 Red Hat, Inc.