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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.08.2025 14:28, 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 caller 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).
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>
> ---
> (cherry picked from commit f0c85d9043f7c9402e85b73361c8a13c683428ca from
>  the downstream branch poc/pci-passthrough from
>  https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git)
> 
> v1->v2:
> * remove dom_io check
> * fixup pci_add_device parameters
> * use current->domain instead of hardware_domain

What I'm missing (as per my v1 comment) is discussion of the hardware_domain
-> current->domain change, including the XSM aspect. Because of the XSM aspect,
please also Cc the XSM maintainer going forward (I'm adding him here as well).

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -654,8 +654,9 @@ 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,
> -                   const struct pci_dev_info *info, nodeid_t node)
> +int pci_add_device(uint16_t seg, uint8_t bus, uint8_t devfn,
> +                   const struct pci_dev_info *info, nodeid_t node,
> +                   struct domain *d)
>  {
>      struct pci_seg *pseg;
>      struct pci_dev *pdev;
> @@ -663,6 +664,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>      const char *type;
>      int ret;
>  
> +    if ( !d )
> +        return -EINVAL;

This should't be needed. Very remotely ASSERT(d) could be added here, but
we don't normally do so elsewhere.

> @@ -767,9 +771,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>      ret = 0;
>      if ( !pdev->domain )
>      {
> -        pdev->domain = hardware_domain;
> -        write_lock(&hardware_domain->pci_lock);
> -        list_add(&pdev->domain_list, &hardware_domain->pdev_list);
> +        pdev->domain = d;
> +        write_lock(&d->pci_lock);
> +        list_add(&pdev->domain_list, &pdev->domain->pdev_list);

Why pdev->domain instead of the shorter and more efficient d?

Jan