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

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

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

> Ideally, we should update our macros the minute the specs are released,
> regardless of whether we believe there's a use-case for it or not.
>

I disagree. We add what we need when we need it. The patch volume is
high enough as it is.