Re: [PATCH v2 1/3] PCI: simplify (and thus correct) pci_get_pdev{,_by_domain}()

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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.singh@xxxxxxx>
>
> 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 <jbeulich@xxxxxxxx>

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

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.

~Andrew