Re: [PATCH v7 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Oct 15, 2021 at 02:59:19PM +0100, Bertrand Marquis wrote:
> From: Rahul Singh <rahul.singh@xxxxxxx>
> 
> The existing VPCI support available for X86 is adapted for Arm.
> When the device is added to XEN via the hyper call
> “PHYSDEVOP_pci_device_add”, VPCI handler for the config space
> access is added to the Xen to emulate the PCI devices config space.
> 
> A MMIO trap handler for the PCI ECAM space is registered in XEN
> so that when guest is trying to access the PCI config space,XEN
> will trap the access and emulate read/write using the VPCI and
> not the real PCI hardware.
> 
> For Dom0less systems scan_pci_devices() would be used to discover the
> PCI device in XEN and VPCI handler will be added during XEN boots.
> 
> This patch is also doing some small fixes to fix compilation errors on
> arm32 of vpci and prevent 64bit accesses on 32bit:
> - use %zu instead of lu in header.c for print
> - prevent 64bit accesses in vpci_access_allowed
> - ifdef out using CONFIG_64BIT handling of len 8 in
> vpci_ecam_{read/write}
> 
> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>

The vpci bits looks fine to me, so:

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

I have one question however related to the placement of the vpci setup
call in pci_add_device.

> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 3aa8c3175f..082892c8a2 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -766,7 +766,21 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>          list_add(&pdev->domain_list, &hardware_domain->pdev_list);
>      }
>      else
> +    {
> +#ifdef CONFIG_ARM
> +        /*
> +         * On ARM PCI devices discovery will be done by Dom0. Add vpci 
> handler
> +         * when Dom0 inform XEN to add the PCI devices in XEN.
> +         */
> +        ret = vpci_add_handlers(pdev);
> +        if ( ret )
> +        {
> +            printk(XENLOG_ERR "Setup of vPCI failed: %d\n", ret);
> +            goto out;
> +        }

I'm likely lost here, but shouldn't this also be done for devices that
belong to the hardware domain and are assigned to it in the first
branch of this conditional?

Or else you will end up with devices assigned to the hardware domain
that don't have vPCI setup for them.

Thanks, Roger.