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

[Jan Beulich <jbeulich@suse.com>]

On 14.10.2021 11:03, Bertrand Marquis wrote:
> Hi Jan,
> 
>> 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.
> 
> If that is ok with I will:
> - move function from x86/hw/io.c to vpci.c renaming them to ECAM
> - rename MMCFG_{BDF/REG_OFFSET) to ECAM_{BDF/REG_OFFSET}
> 
> For the rest of the occurrences in x86 I will not modify any of them as some
> are related to ACPI so using (M)MCFG is right and for the others I am not 100%
> sure.
> 
> Do you agree ?

Probably not, but I don't want to put time into checking existing
names right now. Yet I'm getting the impression that I'm being
overruled here anyway, so there may be little point in investing any
time here in the first place.

Jan