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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.10.2021 12:11, Bertrand Marquis wrote:
>> 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.

Yes; I continue to be sure though that I saw devfn allocation logic in
one of the many patches that are related here. And I'm relatively sure
that there no adjustment to logic here was made (but since it's hard
to pick the right patch out of the huge pile without knowing its title,
I can't reasonably go check).

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

I view this differently: This change {c,sh}ould have been broken out
into one changing x86 behavior first, which Arm then would simply
have adopted. I don't find it unusual for issues to be found in
existing code when making that fit for another architecture. As a
result ...

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

... I view these as going hand in hand: Removing the annotation
altogether is the way to go, yes, because on x86 the call will also
need to be made.

Ian, considering that PVH Dom0 is still experimental on x86, and
considering that the patch here was committed prematurely anyway,
would you be willing to release-ack a patch dropping the "#ifdef
CONFIG_ARM" alongside other necessary adjustments here, provided
maintainers have reached agreement?

Jan