Re: [PATCH v2 6/7] xen/pci: assign discovered devices to hwdom

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.10.2025 15:56, Mykyta Poturai wrote:
> From: Luca Fancellu <luca.fancellu@xxxxxxx>
> 
> Hook up existing PCI setup routines for hwdom into Arm iommu
> initialization sequence, only assign endpoint devices.
> 
> During scanned PCI device assignment, also permit access to the BAR
> ranges if hwdom is using vpci and hide host bridges from domains that
> use the fully emulated one.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>

From: and first S-o-b don't match, which either is wrong or needs an 
explanation.

> @@ -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 doubt i wants to be signed, and as per its use below rc also wants to be
unsigned. rc (generally standing for "return code") also may be a misleading
name for the variable. Elsewhere we use "ret".

> @@ -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 ( !has_vpci_bridge(hardware_domain) )
> +        return;

This (and the other one further down) is odd to see, as it'll expand to
!is_hardware_domain(hardware_domain) (with, for Arm, some 2nd check, yes).

> +    for ( i = 0; i < PCI_HEADER_NORMAL_NR_BARS; i += rc )

What about the ROM BAR?

> +    {
> +        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 )
> +        {
> +            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: Indentation.

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

Again.

Also this one may read better when you use PFN_DOWN() and PFN_UP(). Of course
I wonder what the result would be if there were non-page-aligned, multiple-of-
page-size BARs.

Jan