Re: [PATCH v1 7/8] xen/pci: assign discovered devices to hwdom

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.09.2025 09:59, Mykyta Poturai wrote:
> @@ -133,6 +134,12 @@ void arch_iommu_domain_destroy(struct domain *d)
>  {
>  }
>  
> +static int iommu_add_hwdom_pci_device(u8 devfn, struct pci_dev *pdev)
> +{
> +    const struct domain_iommu *hd = dom_iommu(hardware_domain);
> +    return iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));

Nit (style): Blank line please between declaration(s) and statement(s). And
blank line please also ahead of the main return statement of a function.

> @@ -142,6 +149,8 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
>      if ( iommu_hwdom_reserved == 1 )
>          printk(XENLOG_WARNING "map-reserved dom0-iommu option is not 
> supported on ARM\n");
>      iommu_hwdom_reserved = 0;
> +
> +    setup_hwdom_pci_devices(d, iommu_add_hwdom_pci_device);
With this function being __hwdom_init, why is iommu_add_hwdom_pci_device()
not also given that attribute?

As to cf_check I don't know what the Arm policy is: My suggestion would be
to put that attribute wherever it is (potentially) needed.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -20,6 +20,7 @@
>  #include <xen/pci_ids.h>
>  #include <xen/list.h>
>  #include <xen/prefetch.h>
> +#include <xen/iocap.h>
>  #include <xen/iommu.h>
>  #include <xen/irq.h>
>  #include <xen/param.h>
> @@ -1040,6 +1041,12 @@ enum pdev_type pdev_type(u16 seg, u8 bus, u8 devfn)
>      return pos ? DEV_TYPE_PCIe_ENDPOINT : DEV_TYPE_PCI;
>  }
>  
> +static bool pdev_is_endpoint(struct pci_dev *pdev)

__hwdom_init? And parameter pointer-to-const?

> +{
> +    enum pdev_type type = pdev_type(pdev->seg, pdev->bus, pdev->devfn);
> +    return type == DEV_TYPE_PCIe_ENDPOINT || type == DEV_TYPE_PCI;
> +}
> +
>  /*
>   * find the upstream PCIe-to-PCI/PCIX bridge or PCI legacy bridge
>   * return 0: the device is integrated PCI device or PCIe
> @@ -1255,7 +1262,7 @@ static void __hwdom_init setup_one_hwdom_device(const 
> struct setup_hwdom *ctxt,
>                                                  struct pci_dev *pdev)
>  {
>      u8 devfn = pdev->devfn;
> -    int err;
> +    int err, i, rc;

i clearly wants to be of an unsigned type. And rc, afaics, can have its scope
limited to the loop body.

> @@ -1276,6 +1283,34 @@ static void __hwdom_init setup_one_hwdom_device(const 
> struct setup_hwdom *ctxt,
>      if ( err )
>          printk(XENLOG_ERR "setup of vPCI for d%d failed: %d\n",
>                 ctxt->d->domain_id, err);
> +
> +    if ( !hwdom_uses_vpci() )
> +        return;
> +
> +    for ( i = 0; i < PCI_HEADER_NORMAL_NR_BARS; i += rc )
> +    {
> +        uint64_t addr, size;
> +        uint8_t reg = PCI_BASE_ADDRESS_0 + i * 4;
> +
> +        if ( (pci_conf_read32(pdev->sbdf, reg) & PCI_BASE_ADDRESS_SPACE)
> +             == PCI_BASE_ADDRESS_SPACE_IO )

Nit (style): Operator placement again.

> +        {
> +            rc = 1;
> +            continue;
> +        }
> +
> +        rc = pci_size_mem_bar(pdev->sbdf, reg, &addr, &size,
> +                              (i == PCI_HEADER_NORMAL_NR_BARS - 1)
> +                                  ? PCI_BAR_LAST : 0);

Nit (style): Indentation again.

> +
> +        if ( !size )
> +            continue;
> +
> +        err = iomem_permit_access(hardware_domain, paddr_to_pfn(addr),
> +                             paddr_to_pfn(PAGE_ALIGN(addr + size - 1)));

And again.

> +        if ( err )
> +            break;
> +    }
>  }

This change supports my comment on the earlier patch, regarding the option
of doing here some of what needs doing, rather than by another round of
iterating all devices.

> @@ -1294,6 +1329,9 @@ static int __hwdom_init cf_check 
> _setup_hwdom_pci_devices(
>              if ( !pdev )
>                  continue;
>  
> +            if ( hwdom_uses_vpci() && !pdev_is_endpoint(pdev) )
> +                continue;
> +
>              if ( !pdev->domain )
>              {
>                  pdev->domain = ctxt->d;

This is (logically) wrong: On x86 PVH Dom0 uses vPCI but wants all devices
to be handed to it. _This_ may be a place where has_vpci_bridge() might
make sense to use (simply by its name, that is).

Jan