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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jan,

> On 18 Oct 2021, at 08:47, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 15.10.2021 18:51, Bertrand Marquis wrote:
>> --- /dev/null
>> +++ b/xen/arch/arm/vpci.c
>> @@ -0,0 +1,77 @@
>> +/*
>> + * xen/arch/arm/vpci.c
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +#include <xen/sched.h>
>> +#include <xen/vpci.h>
>> +
>> +#include <asm/mmio.h>
>> +
>> +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>> +                          register_t *r, void *p)
>> +{
>> +    pci_sbdf_t sbdf;
>> +    /* data is needed to prevent a pointer cast on 32bit */
>> +    unsigned long data;
>> +
>> +    /* We ignore segment part and always handle segment 0 */
>> +    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa);
>> +
>> +    if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
>> +                        1U << info->dabt.size, &data) )
>> +    {
> 
> Here it is quite clear that the SBDF you pass into vpci_ecam_read() is
> the virtual one. The function then calls vpci_read(), which in turn
> will call vpci_read_hw() in a number of situations (first and foremost
> whenever pci_get_pdev_by_domain() returns NULL). That function as well
> as pci_get_pdev_by_domain() use the passed in SBDF as if it was a
> physical one; I'm unable to spot any translation. Yet I do recall
> seeing assignment of a virtual device and function number somewhere
> (perhaps another of the related series), so the model also doesn't
> look to assume 1:1 mapping of SBDF.

This question was answered by Oleksandr I think.

> 
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -756,6 +756,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>     if ( !pdev->domain )
>>     {
>>         pdev->domain = hardware_domain;
>> +#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);
>> +            pdev->domain = NULL;
>> +            goto out;
>> +        }
>> +#endif
>>         ret = iommu_add_device(pdev);
>>         if ( ret )
>>         {
> 
> Upon failure, vpci_add_handlers() will itself call vpci_remove_handlers().
> What about iommu_add_device() failure? The device will have ->domain
> zapped, but all vPCI handlers still in place. This aspect of insufficient
> error cleanup was pointed out already in review of v1.

Yes a call to vpci_remove_device should be made on the error path out if
iommu_add_device is failing. This should also be done in fact in 
pci_remove_device before cleanup the msi.
We will push a patch with a proposal for a fix for this.

> 
> Furthermore already in v1 I questioned why this would be Arm-specific: On
> x86 this code path is going to be taken for all devices Xen wasn't able
> to discover at boot (anything on segments we wouldn't consider config
> space access safe on without reassurance by Dom0 plus SR-IOV VFs, at the
> very least). Hence it is my understanding that something along these
> lines is actually also needed for PVH Dom0. I've just checked, and
> according to my mailbox that comment was actually left unresponded to.
> 
> Roger, am I missing anything here as to PVH Dom0 getting handlers put in
> place?

>From Roger answer I understood that it will be needed (in the future). 
When and if this is needed, the ifdef CONFIG_ARM can be removed
but this would change x86 code behaviour so I do not think it would
have been right to do that in this serie.

> 
> Of course as soon as the CONFIG_ARM conditionals were dropped, the
> __hwdom_init issue would become an "active" one.

We will push a proposal for a fix for that.
If I understand Roger right, vpci_add_handler will also be needed in runtime
on x86 in the future so maybe it would even be right to remove the flag 
altogether ?

Regards
Bertrand

> 
> Jan
>