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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.10.2021 10:03, Roger Pau Monné wrote:
> On Mon, Oct 18, 2021 at 09:47:06AM +0200, Jan Beulich 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.
>>
>>> --- 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.
>>
>> 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).
> 
> My original plans for SR-IOV VFs on PVH dom0 involved trapping
> accesses to the SR-IOV capability and detecting the creation of VFs
> without the need for dom0 to notify them to Xen. This would avoid dom0
> having to call PHYSDEVOP_pci_device_add for that case.
> 
> I might be confused, but I think we also spoke about other (non SR-IOV
> related) cases where PCI devices might appear after certain actions by
> dom0, so I think we need to keep the PHYSDEVOP_pci_device_add for PVH
> dom0.

Right, hotplugged ones, which I forgot to mention in my earlier reply.

>> 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?
> 
> No, I think we will need those, likewise for run-time reported MCFG
> regions.

Yes, this was what I referred to by "without reassurance by Dom0".
Thanks for confirming.

Jan