Re: [PATCH v1 2/3] xen/pci: modify pci_add_device to handle device add by Xen

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.08.2025 11:22, Mykyta Poturai wrote:
> From: Luca Fancellu <luca.fancellu@xxxxxxx>
> 
> Currently pci_add_device is called from hypercalls requested by Dom0
> to add pci devices and when the device has no domain associated with
> it, it is assumed that hardware_domain is the owner.
> 
> On the dom0less scenario, the enumeration is performed by the
> firmware and Xen at boot time might want to assign some pci devices
> to guests, so it has to firstly add the device and then assign it to
> the final guest.
> 
> Modify pci_add_device to have the owner domain passed as a parameter
> to the function, so that when it is called from the hypercall the
> owner would be the hardware domain, otherwise when Xen is calling it,
> it would be another domain since hw domain could not be there
> (dom0less guests without Dom0 use case).
> 
> In pci_add_device there is a call to xsm that doesn't consider the
> requester of the function to be Xen itself, so add a check to skip
> the call if the owner domain is dom_io, since it means the call is
> coming directly from Xen.

I don't understand this particular aspect: All call sites pass
hardware_domain. Checking against dom_io therefore looks unnecessary, or
at least premature. In Misra's terms it is "dead code" that you add.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -654,7 +654,7 @@ unsigned int pci_size_mem_bar(pci_sbdf_t sbdf, unsigned 
> int pos,
>      return is64bits ? 2 : 1;
>  }
>  
> -int pci_add_device(u16 seg, u8 bus, u8 devfn,
> +int pci_add_device(struct domain *d, u16 seg, u8 bus, u8 devfn,
>                     const struct pci_dev_info *info, nodeid_t node)

For a pci_*() function I don't think it's appropriate to have a domain (not a
PCI domain, aka segment, which would be fine) as first parameter.

While you touch this it might also be nice if you switched u<N> to uint<N>_t.

> --- a/xen/drivers/pci/physdev.c
> +++ b/xen/drivers/pci/physdev.c
> @@ -50,7 +50,8 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
> arg)
>          }
>  #endif
>  
> -        ret = pci_add_device(add.seg, add.bus, add.devfn, &pdev_info, node);
> +        ret = pci_add_device(hardware_domain, add.seg, add.bus, add.devfn,
> +                             &pdev_info, node);

Here as well as in the x86-specific counterparts: Why hardware_domain, i.e.
why not current->domain? Yes, what you do is in line with how the code has
behaved so far, but now that you want to change it, it needs to be sorted
what it is that's really wanted. This btw also plays into the XSM aspect
that you're fiddling with.

Also, what if you end up passing NULL here in the dom0less case?

Jan