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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 15 Oct 2021, at 16:18, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On 15/10/2021 16:06, Bertrand Marquis wrote:
>>> 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.
> 
> From my understanding, when the device is initially registered we would go 
> through the first branch because pdev->domain is not yet set.
> 
> The else would be taken only with subsequent call of PHYSDEVOP_manage_pci_add 
> & co.
> 
> For the device assignment, a different path would be taken. This would go 
> through the domctl XEN_DOMCTL_assign_device.
> 
> Therefore, I think Roger is right and the call belongs to the first branch. 
> Otherwise, we would miss out the vpci handlers in some cases.

Yes we only want this to be done once on the first call.

So in fact it should be done when pdev->domain is NULL in the first branch.

I will do this in v8.

Thanks a lot for the analysis, saying it makes things more clear :-)

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall