Re: [PATCH v5 08/11] xen/arm: Enable the existing x86 virtual PCI support for ARM.

[Bertrand Marquis <Bertrand.Marquis@arm.com>]

Hi,

> On 14 Oct 2021, at 07:33, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 13.10.2021 21:27, Stefano Stabellini wrote:
>> On Wed, 13 Oct 2021, Jan Beulich wrote:
>>> On 13.10.2021 16:51, Oleksandr Andrushchenko wrote:
>>>> On 13.10.21 16:00, Jan Beulich wrote:
>>>>> On 13.10.2021 10:45, Roger Pau Monné wrote:
>>>>>> On Wed, Oct 06, 2021 at 06:40:34PM +0100, Rahul Singh wrote:
>>>>>>> --- /dev/null
>>>>>>> +++ b/xen/arch/arm/vpci.c
>>>>>>> @@ -0,0 +1,102 @@
>>>>>>> +/*
>>>>>>> + * 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 <asm/mmio.h>
>>>>>>> +
>>>>>>> +#define REGISTER_OFFSET(addr)  ( (addr) & 0x00000fff)
>>>>>>> +
>>>>>>> +/* Do some sanity checks. */
>>>>>>> +static bool vpci_mmio_access_allowed(unsigned int reg, unsigned int len)
>>>>>>> +{
>>>>>>> +    /* Check access size. */
>>>>>>> +    if ( len > 8 )
>>>>>>> +        return false;
>>>>>>> +
>>>>>>> +    /* Check that access is size aligned. */
>>>>>>> +    if ( (reg & (len - 1)) )
>>>>>>> +        return false;
>>>>>>> +
>>>>>>> +    return true;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>>>>>>> +                          register_t *r, void *p)
>>>>>>> +{
>>>>>>> +    unsigned int reg;
>>>>>>> +    pci_sbdf_t sbdf;
>>>>>>> +    unsigned long data = ~0UL;
>>>>>>> +    unsigned int size = 1U << info->dabt.size;
>>>>>>> +
>>>>>>> +    sbdf.sbdf = MMCFG_BDF(info->gpa);
>>>>>>> +    reg = REGISTER_OFFSET(info->gpa);
>>>>>>> +
>>>>>>> +    if ( !vpci_mmio_access_allowed(reg, size) )
>>>>>>> +        return 0;
>>>>>>> +
>>>>>>> +    data = vpci_read(sbdf, reg, min(4u, size));
>>>>>>> +    if ( size == 8 )
>>>>>>> +        data |= (uint64_t)vpci_read(sbdf, reg + 4, 4) << 32;
>>>>>>> +
>>>>>>> +    *r = data;
>>>>>>> +
>>>>>>> +    return 1;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info,
>>>>>>> +                           register_t r, void *p)
>>>>>>> +{
>>>>>>> +    unsigned int reg;
>>>>>>> +    pci_sbdf_t sbdf;
>>>>>>> +    unsigned long data = r;
>>>>>>> +    unsigned int size = 1U << info->dabt.size;
>>>>>>> +
>>>>>>> +    sbdf.sbdf = MMCFG_BDF(info->gpa);
>>>>>>> +    reg = REGISTER_OFFSET(info->gpa);
>>>>>>> +
>>>>>>> +    if ( !vpci_mmio_access_allowed(reg, size) )
>>>>>>> +        return 0;
>>>>>>> +
>>>>>>> +    vpci_write(sbdf, reg, min(4u, size), data);
>>>>>>> +    if ( size == 8 )
>>>>>>> +        vpci_write(sbdf, reg + 4, 4, data >> 32);
>>>>>> I think those two helpers (and vpci_mmio_access_allowed) are very
>>>>>> similar to the existing x86 ones (see vpci_mmcfg_{read,write}), up to
>>>>>> the point where I would consider moving the shared code to vpci.c as
>>>>>> vpci_ecam_{read,write} and call them from the arch specific trap
>>>>>> handlers.
>>>>> Except that please can we stick to mcfg or mmcfg instead of ecam
>>>>> in names, as that's how the thing has been named in Xen from its
>>>>> introduction? I've just grep-ed the code base (case insensitively)
>>>>> and found no mention of ECAM. There are only a few "became".
>>>> I do understand that this is historically that we do not have ECAM in Xen,
>>>> but PCI is not about Xen. Thus, I think it is also acceptable to use
>>>> a commonly known ECAM for the code that works with ECAM.
>>> 
>>> ACPI, afaik, also doesn't call this ECAM. That's where MCFG / MMCFG
>>> actually come from, I believe.
>> 
>> My understanding is that "MCFG" is the name of the ACPI table that
>> describes the PCI config space [1]. The underlying PCI standard for the
>> memory mapped layout of the PCI config space is called ECAM. Here, it
>> makes sense to call it ECAM as it is firmware independent.
>> 
>> [1] https://wiki.osdev.org/PCI_Express
> 
> Okay, I can accept this, but then I'd expect all existing uses of
> MCFG / MMCFG in the code which aren't directly ACPI-related to get
> replaced. Otherwise it's needlessly harder to identify that one
> piece of code relates to certain other pieces.

To sum up on this subject, here what I will do in version 6:
- move vpci_ecam_{read/write} to vpci.c and rename them
- same for access allowed and I will rename it to vpci_ecam_access_allowed

I will work on this and try to push v6 before the end of the day (also handling other remarks on this patch).

Cheers
Bertrand

> 
> Jan
>