Re: [PATCH 1/1] EmbeddedPkg/AcpiLib: add GICC table init macro for ACPI 6.3

["Pete Batard" <pete@akeo.ie>]

On 2020.03.30 15:34, Ard Biesheuvel wrote:
> On Mon, 30 Mar 2020 at 16:29, Pete Batard <pete@akeo.ie> wrote:
>>
>> On 2020.03.30 15:16, Ard Biesheuvel wrote:
>>> On Mon, 30 Mar 2020 at 16:07, Pete Batard <pete@akeo.ie> wrote:
>>>>
>>>> On 2020.03.30 15:01, Ard Biesheuvel wrote:
>>>>> On Mon, 30 Mar 2020 at 15:56, Pete Batard <pete@akeo.ie> wrote:
>>>>>>
>>>>>> On 2020.03.30 14:20, Ard Biesheuvel wrote:
>>>>>>> On Mon, 30 Mar 2020 at 15:12, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>>>>>>
>>>>>>>> On Mon, 30 Mar 2020 at 15:09, Pete Batard <pete@akeo.ie> wrote:
>>>>>>>>>
>>>>>>>>> On 2020.03.30 14:06, Ard Biesheuvel wrote:
>>>>>>>>>> On Fri, 27 Mar 2020 at 14:06, Pete Batard <pete@akeo.ie> wrote:
>>>>>>>>>>>
>>>>>>>>>>> Incidentally, this is not an [edk2-platform] patch, as the subject line
>>>>>>>>>>> from previous mail seemed to indicate, but an [edk2] patch.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Do we have a user for this?
>>>>>>>>>
>>>>>>>>> Yes we do. I have a pachset lined up that updates the Raspberry Pi ACPI
>>>>>>>>> to 6.3, that has a dependency on this.
>>>>>>>>>
>>>>>>>>
>>>>>>>> But does the RPi have SPE and the associated overflow interrupt?
>>>>>>
>>>>>> No, but it doesn't matter since the specs indicate that SPE values can
>>>>>> be set to zero if unused/non-applicable.
>>>>>>
>>>>>>>> ACPI
>>>>>>>> is designed to be backward compatible, so it is perfectly acceptable
>>>>>>>> to use the 6.2 macros in the context of a firmware implementation that
>>>>>>>> complies with 6.3.
>>>>>>
>>>>>> This is what happens if you try to use EFI_ACPI_6_0_GICC_STRUCTURE_INIT
>>>>>> in a 6.3 context:
>>>>>>
>>>>>> /usr/src/edk2/MdePkg/Include/IndustryStandard/Acpi10.h:297:33: error:
>>>>>> excess elements in scalar initializer [-Werror]
>>>>>>      #define EFI_ACPI_RESERVED_BYTE  0x00
>>>>>>                                      ^~~~
>>>>>> Building ...
>>>>>> /usr/src/edk2/MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf [AARCH64]
>>>>>> /usr/src/edk2/EmbeddedPkg/Include/Library/AcpiLib.h:64:30: note: in
>>>>>> expansion of macro ‘EFI_ACPI_RESERVED_BYTE’
>>>>>>          {EFI_ACPI_RESERVED_BYTE, EFI_ACPI_RESERVED_BYTE,
>>>>>> EFI_ACPI_RESERVED_BYTE}         \
>>>>>>                                   ^~~~~~~~~~~~~~~~~~~~~~
>>>>>> /usr/src/edk2-platforms/Platform/RaspberryPi/AcpiTables/Madt.aslc:64:5:
>>>>>> note: in expansion of macro ‘EFI_ACPI_6_0_GICC_STRUCTURE_INIT’
>>>>>>          EFI_ACPI_6_0_GICC_STRUCTURE_INIT (
>>>>>>          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>
>>>>>
>>>>> What do you mean exactly by 'in a 6.3 context': are you trying to
>>>>> statically initialize a 63 struct with the 60 macro?
>>>>
>>>> Yes. I am trying to upgrade all of our ACPI tables to 6.3, on account
>>>> that (part of a commit message from the edk2-platform I have lined up):
>>>>
>>>> -----------------------------------------------------------------------
>>>> Because of its widespread availability and low price, we expect the
>>>> Raspberry Pi source to be used by platform developers as a starting
>>>> point to create their own platform implementation.
>>>>
>>>> As such, it makes a lot of sense to want to use the most up to date
>>>> underlying standards, even if the pay off is limited in this case,
>>>> as it may help others benefit from the latest improvements and
>>>> features brought by modern ACPI.
>>>> -----------------------------------------------------------------------
>>>>
>>>>>> The only reason I'm sending an EDK2 patch, which I'd always rather avoid
>>>>>> so that edk2-platforms patches can be applied faster, is that I haven't
>>>>>> been able to find a way to make the existing 6.0 macros work in a 6.3
>>>>>> context, and I expect that this will be the case for others.
>>>>>>
>>>>>
>>>>> By why do we need the 6.3 context? If 6.0 can describe our platform
>>>>> fully, it is actually better for compatibility to stick with it rather
>>>>> than upgrade to 6.3.
>>>>
>>>> See above.
>>>>
>>>> I have to say that I'm a bit taken aback by the idea that, even though
>>>> we can anticipate that there will be a need for a 6.3 macro that does
>>>> initialise the SPE field, there seems to be strong reluctance to add
>>>> that macro before someone makes the case for it.
>>>>
>>>
>>> Well, the reason I asked was because I only want to merge changes that
>>> are actually need. If there is a valid need, I will happily merge this
>>> patch.
>>>
>>> But as it turns out, the reason is simply being able to claim that we
>>> implement the latest ACPI revision, which is actually a bad idea for
>>> platforms that don't actually rely on any of the new features, since
>>> it may prevent you from being able to run older OSes
>>
>> Then we have two very different visions for the platform, especially
>> when it comes to the Pi 4, where networking and SD card support is
>> pretty much NO_GO for any "older" OS (and troublesome enough as it is in
>> modern nones).
>>
>> This is actually the one place where I'd like to see an actual use-case
>> made for "older OSes incompatibility", especially when it comes to 6.0
>> MADT vs 6.3 MADT, where the only different is that 6.0 had 3 reserved
>> fields and 6.3 started to use 2 of those.
>>
>> For the record, the MADT blobs we got from Microsoft (for the Pi 3) were
>> 6.0, and we did downgrade them to 5.1 for convenience (rather than
>> compatibility issues), so I really fail to see the issue with bumping
>> MADT to 6.3.
>>
>> Instead, what I'm seeing is folks who need the SPE fields for their
>> platform and look at other implementations to see how to set them up
>> wasting time having to send a duplicate of the current to edk2 for very
>> dubious reasons.
>>
>>> I disagree. We add what we need when we need it. The patch volume is
>>> high enough as it is.
>>
>> Well, considering that different folks will have to send duplicate
>> patches, this doesn't entirely come as a surprise...
>>
>> Sorry, but I really can't see any good reason for this patch to be shot
>> down. You guys *will* have to process a similar patch sooner or later.
>>
> 
> Again, I am happy to merge this patch. The argument is about whether
> we need to upgrade RPi4 to 6.3
> 

Last time I checked, quite a few of the people who contributed code to 
the platform were of the opinion that we did.

I will re-send an e-mail to the group, off-list, so that you get an idea 
of what the current stance is on that topic, and then decide whether 
your want to uphold your current position.

Regards,

/Pete