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