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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Roger,

> On 15 Oct 2021, at 15:30, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> 
> 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>

Thanks

> 
> 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.

I might be wrong but when the hardware domain is declaring the devices they are 
added to him.
Then later when those device are assigned to a guest, they are removed from the 
hardware domain.

Regards
Bertrand

> 
> Thanks, Roger.