On 11.08.2022 15:11, Andrew Cooper wrote:
> On 11/08/2022 11:51, Jan Beulich wrote:
>> The last "wildcard" use of either function went away with f591755823a7
>> ("IOMMU/PCI: don't let domain cleanup continue when device de-assignment
>> failed"). Don't allow them to be called this way anymore. Besides
>> simplifying the code this also fixes two bugs:
>>
>> 1) When seg != -1, the outer loops should have been terminated after the
>> first iteration, or else a device with the same BDF but on another
>> segment could be found / returned.
>>
>> Reported-by: Rahul Singh <rahul.si...@arm.com>
>>
>> 2) When seg == -1 calling get_pseg() is bogus. The function (taking a
>> u16) would look for segment 0xffff, which might exist. If it exists,
>> we might then find / return a wrong device.
>>
>> In pci_get_pdev_by_domain() also switch from using the per-segment list
>> to using the per-domain one, with the exception of the hardware domain
>> (see the code comment there).
>>
>> While there also constify "pseg" and drop "pdev"'s already previously
>> unnecessary initializer.
>>
>> Signed-off-by: Jan Beulich <jbeul...@suse.com>
>
> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com>
Thanks.
> I'm not totally convinced that special casing hwdom is right, because
> quarantine devices are domio not hwdom. But I also can't identify a
> case where it's definitely wrong either.
>
>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -177,10 +177,10 @@ int pci_add_device(u16 seg, u8 bus, u8 d
>> int pci_remove_device(u16 seg, u8 bus, u8 devfn);
>> int pci_ro_device(int seg, int bus, int devfn);
>> int pci_hide_device(unsigned int seg, unsigned int bus, unsigned int devfn);
>> -struct pci_dev *pci_get_pdev(int seg, int bus, int devfn);
>> +struct pci_dev *pci_get_pdev(uint16_t seg, uint8_t bus, uint8_t devfn);
>
> I was going to make a request, but I can't quite get it to compile...
>
> Passing sbdf as 3 parameters is a waste, and it would be great if we
> could take this opportunity to improve.
>
> Sadly,
>
> -struct pci_dev *pci_get_pdev(uint16_t seg, uint8_t bus, uint8_t devfn);
> +struct pci_dev *pci_get_pdev(pci_sbdf_t sbdf);
> +
> +#define pci_get_pdev(...) \
> + ({ \
> + count_args(__VA_ARGS__) == 1 \
> + ? pci_get_pdev(__VA_ARGS__) \
> + : pci_get_pdev(PCI_SBDF(__VA_ARGS__)); \
> + })
>
> this doesn't quite compile as a transition plan, and I'm stuck for
> further ideas.
Just look at patch 2.
Jan
