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

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

[Pete Batard]

ACPI 6.3 added a 16-bit SPE overflow Interrupt field, replacing
2 of the 3 reserved bytes that are defined at the end of the
GICC structure for 6.0.

Add a new macro to initialise the new field.

Signed-off-by: Pete Batard <pete@akeo.ie>
---
 EmbeddedPkg/Include/Library/AcpiLib.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/EmbeddedPkg/Include/Library/AcpiLib.h b/EmbeddedPkg/Include/Library/AcpiLib.h
index 5c6e2075de79..57d7bd595584 100644
--- a/EmbeddedPkg/Include/Library/AcpiLib.h
+++ b/EmbeddedPkg/Include/Library/AcpiLib.h
@@ -64,6 +64,14 @@
     {EFI_ACPI_RESERVED_BYTE, EFI_ACPI_RESERVED_BYTE, EFI_ACPI_RESERVED_BYTE}         \
   }
 
+#define EFI_ACPI_6_3_GICC_STRUCTURE_INIT(GicId, AcpiCpuUid, Mpidr, Flags, PmuIrq,    \
+    GicBase, GicVBase, GicHBase, GsivId, GicRBase, Efficiency, SpeOvflIrq)           \
+  {                                                                                  \
+    EFI_ACPI_6_0_GIC, sizeof (EFI_ACPI_6_0_GIC_STRUCTURE), EFI_ACPI_RESERVED_WORD,   \
+    GicId, AcpiCpuUid, Flags, 0, PmuIrq, 0, GicBase, GicVBase, GicHBase,             \
+    GsivId, GicRBase, Mpidr, Efficiency, EFI_ACPI_RESERVED_BYTE, SpeOvflIrq          \
+  }
+
 #define EFI_ACPI_6_0_GIC_MSI_FRAME_INIT(GicMsiFrameId, PhysicalBaseAddress, Flags, SPICount, SPIBase) \
   { \
     EFI_ACPI_6_0_GIC_MSI_FRAME, sizeof (EFI_ACPI_6_0_GIC_MSI_FRAME_STRUCTURE), EFI_ACPI_RESERVED_WORD, \
-- 
2.21.0.windows.1

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

[Pete Batard]

Incidentally, this is not an [edk2-platform] patch, as the subject line 
from previous mail seemed to indicate, but an [edk2] patch.

/Pete

On 2020.03.27 13:01, Pete Batard wrote:
> ACPI 6.3 added a 16-bit SPE overflow Interrupt field, replacing
> 2 of the 3 reserved bytes that are defined at the end of the
> GICC structure for 6.0.
> 
> Add a new macro to initialise the new field.
> 
> Signed-off-by: Pete Batard <pete@akeo.ie>
> ---
>   EmbeddedPkg/Include/Library/AcpiLib.h | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/EmbeddedPkg/Include/Library/AcpiLib.h b/EmbeddedPkg/Include/Library/AcpiLib.h
> index 5c6e2075de79..57d7bd595584 100644
> --- a/EmbeddedPkg/Include/Library/AcpiLib.h
> +++ b/EmbeddedPkg/Include/Library/AcpiLib.h
> @@ -64,6 +64,14 @@
>       {EFI_ACPI_RESERVED_BYTE, EFI_ACPI_RESERVED_BYTE, EFI_ACPI_RESERVED_BYTE}         \
>     }
>   
> +#define EFI_ACPI_6_3_GICC_STRUCTURE_INIT(GicId, AcpiCpuUid, Mpidr, Flags, PmuIrq,    \
> +    GicBase, GicVBase, GicHBase, GsivId, GicRBase, Efficiency, SpeOvflIrq)           \
> +  {                                                                                  \
> +    EFI_ACPI_6_0_GIC, sizeof (EFI_ACPI_6_0_GIC_STRUCTURE), EFI_ACPI_RESERVED_WORD,   \
> +    GicId, AcpiCpuUid, Flags, 0, PmuIrq, 0, GicBase, GicVBase, GicHBase,             \
> +    GsivId, GicRBase, Mpidr, Efficiency, EFI_ACPI_RESERVED_BYTE, SpeOvflIrq          \
> +  }
> +
>   #define EFI_ACPI_6_0_GIC_MSI_FRAME_INIT(GicMsiFrameId, PhysicalBaseAddress, Flags, SPICount, SPIBase) \
>     { \
>       EFI_ACPI_6_0_GIC_MSI_FRAME, sizeof (EFI_ACPI_6_0_GIC_MSI_FRAME_STRUCTURE), EFI_ACPI_RESERVED_WORD, \
> 

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

[Ard Biesheuvel]

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?

>
> On 2020.03.27 13:01, Pete Batard wrote:
> > ACPI 6.3 added a 16-bit SPE overflow Interrupt field, replacing
> > 2 of the 3 reserved bytes that are defined at the end of the
> > GICC structure for 6.0.
> >
> > Add a new macro to initialise the new field.
> >
> > Signed-off-by: Pete Batard <pete@akeo.ie>
> > ---
> >   EmbeddedPkg/Include/Library/AcpiLib.h | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> >
> > diff --git a/EmbeddedPkg/Include/Library/AcpiLib.h b/EmbeddedPkg/Include/Library/AcpiLib.h
> > index 5c6e2075de79..57d7bd595584 100644
> > --- a/EmbeddedPkg/Include/Library/AcpiLib.h
> > +++ b/EmbeddedPkg/Include/Library/AcpiLib.h
> > @@ -64,6 +64,14 @@
> >       {EFI_ACPI_RESERVED_BYTE, EFI_ACPI_RESERVED_BYTE, EFI_ACPI_RESERVED_BYTE}         \
> >     }
> >
> > +#define EFI_ACPI_6_3_GICC_STRUCTURE_INIT(GicId, AcpiCpuUid, Mpidr, Flags, PmuIrq,    \
> > +    GicBase, GicVBase, GicHBase, GsivId, GicRBase, Efficiency, SpeOvflIrq)           \
> > +  {                                                                                  \
> > +    EFI_ACPI_6_0_GIC, sizeof (EFI_ACPI_6_0_GIC_STRUCTURE), EFI_ACPI_RESERVED_WORD,   \
> > +    GicId, AcpiCpuUid, Flags, 0, PmuIrq, 0, GicBase, GicVBase, GicHBase,             \
> > +    GsivId, GicRBase, Mpidr, Efficiency, EFI_ACPI_RESERVED_BYTE, SpeOvflIrq          \
> > +  }
> > +
> >   #define EFI_ACPI_6_0_GIC_MSI_FRAME_INIT(GicMsiFrameId, PhysicalBaseAddress, Flags, SPICount, SPIBase) \
> >     { \
> >       EFI_ACPI_6_0_GIC_MSI_FRAME, sizeof (EFI_ACPI_6_0_GIC_MSI_FRAME_STRUCTURE), EFI_ACPI_RESERVED_WORD, \
> >
>

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

[Pete Batard]

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.

Regards,

/Pete

> 
>>
>> On 2020.03.27 13:01, Pete Batard wrote:
>>> ACPI 6.3 added a 16-bit SPE overflow Interrupt field, replacing
>>> 2 of the 3 reserved bytes that are defined at the end of the
>>> GICC structure for 6.0.
>>>
>>> Add a new macro to initialise the new field.
>>>
>>> Signed-off-by: Pete Batard <pete@akeo.ie>
>>> ---
>>>    EmbeddedPkg/Include/Library/AcpiLib.h | 8 ++++++++
>>>    1 file changed, 8 insertions(+)
>>>
>>> diff --git a/EmbeddedPkg/Include/Library/AcpiLib.h b/EmbeddedPkg/Include/Library/AcpiLib.h
>>> index 5c6e2075de79..57d7bd595584 100644
>>> --- a/EmbeddedPkg/Include/Library/AcpiLib.h
>>> +++ b/EmbeddedPkg/Include/Library/AcpiLib.h
>>> @@ -64,6 +64,14 @@
>>>        {EFI_ACPI_RESERVED_BYTE, EFI_ACPI_RESERVED_BYTE, EFI_ACPI_RESERVED_BYTE}         \
>>>      }
>>>
>>> +#define EFI_ACPI_6_3_GICC_STRUCTURE_INIT(GicId, AcpiCpuUid, Mpidr, Flags, PmuIrq,    \
>>> +    GicBase, GicVBase, GicHBase, GsivId, GicRBase, Efficiency, SpeOvflIrq)           \
>>> +  {                                                                                  \
>>> +    EFI_ACPI_6_0_GIC, sizeof (EFI_ACPI_6_0_GIC_STRUCTURE), EFI_ACPI_RESERVED_WORD,   \
>>> +    GicId, AcpiCpuUid, Flags, 0, PmuIrq, 0, GicBase, GicVBase, GicHBase,             \
>>> +    GsivId, GicRBase, Mpidr, Efficiency, EFI_ACPI_RESERVED_BYTE, SpeOvflIrq          \
>>> +  }
>>> +
>>>    #define EFI_ACPI_6_0_GIC_MSI_FRAME_INIT(GicMsiFrameId, PhysicalBaseAddress, Flags, SPICount, SPIBase) \
>>>      { \
>>>        EFI_ACPI_6_0_GIC_MSI_FRAME, sizeof (EFI_ACPI_6_0_GIC_MSI_FRAME_STRUCTURE), EFI_ACPI_RESERVED_WORD, \
>>>
>>

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

[Ard Biesheuvel]

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? 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. Or is there another reason you want to update the
MADT to 6.3?

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

[Ard Biesheuvel]

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? 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. Or is there another reason you want to update the
> MADT to 6.3?

BTW, this patch sets the size of the GICC entry to 'sizeof
(EFI_ACPI_6_0_GIC_STRUCTURE)' so it is likely that the parser will
choke on it.

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

[Pete Batard]

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 (
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

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.

>> Or is there another reason you want to update the
>> MADT to 6.3?

I just want to fix the compilation error, as well as make sure that 
folks who need the ACPI 6.3 SPE init can get it without having to spend 
time waiting for an EDK2 patch to be applied.

> BTW, this patch sets the size of the GICC entry to 'sizeof
> (EFI_ACPI_6_0_GIC_STRUCTURE)' so it is likely that the parser will
> choke on it.

The structure size hasn't changed. There were 3 reserved bytes, and now 
there's one reserve byte and one 16-bit word for the SPE.

Since I actually need this patch as part of a platform update, I did 
validate compilation, so, no, the parser doesn't choke on it.

Regards,

/Pete

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

[Ard Biesheuvel]

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?


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

> >> Or is there another reason you want to update the
> >> MADT to 6.3?
>
> I just want to fix the compilation error, as well as make sure that
> folks who need the ACPI 6.3 SPE init can get it without having to spend
> time waiting for an EDK2 patch to be applied.
>
> > BTW, this patch sets the size of the GICC entry to 'sizeof
> > (EFI_ACPI_6_0_GIC_STRUCTURE)' so it is likely that the parser will
> > choke on it.
>
> The structure size hasn't changed. There were 3 reserved bytes, and now
> there's one reserve byte and one 16-bit word for the SPE.
>
> Since I actually need this patch as part of a platform update, I did
> validate compilation, so, no, the parser doesn't choke on it.
>

I didn't mean to imply that you are being sloppy. I'd just like to
understand what the point is of all of this, given that ACPI 6.3 is
not needed to describe the RPi4

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

[Pete Batard]

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.

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.

Regards,

/Pete

> 
>>>> Or is there another reason you want to update the
>>>> MADT to 6.3?
>>
>> I just want to fix the compilation error, as well as make sure that
>> folks who need the ACPI 6.3 SPE init can get it without having to spend
>> time waiting for an EDK2 patch to be applied.
>>
>>> BTW, this patch sets the size of the GICC entry to 'sizeof
>>> (EFI_ACPI_6_0_GIC_STRUCTURE)' so it is likely that the parser will
>>> choke on it.
>>
>> The structure size hasn't changed. There were 3 reserved bytes, and now
>> there's one reserve byte and one 16-bit word for the SPE.
>>
>> Since I actually need this patch as part of a platform update, I did
>> validate compilation, so, no, the parser doesn't choke on it.
>>
> 
> I didn't mean to imply that you are being sloppy. I'd just like to
> understand what the point is of all of this, given that ACPI 6.3 is
> not needed to describe the RPi4
> 

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

[Ard Biesheuvel]

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.

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

[Pete Batard]

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.

Regards,

/Pete

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

[Ard Biesheuvel]

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

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

[Pete Batard]

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

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

[Ard Biesheuvel]

On Mon, 30 Mar 2020 at 16:39, Pete Batard <pete@akeo.ie> wrote:
>
> On 2020.03.30 15:34, Ard Biesheuvel wrote:
>
...
> > 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.
>

Then they are cordially invited to argue their position, preferably
using arguments.

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

Sure. I'd prefer on-list to be honest, but if you feel it needs to be
off-list, that is also fine.

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

[Pete Batard]

On 2020.03.30 15:42, Ard Biesheuvel wrote:
> On Mon, 30 Mar 2020 at 16:39, Pete Batard <pete@akeo.ie> wrote:
>>
>> On 2020.03.30 15:34, Ard Biesheuvel wrote:
>>
> ...
>>> 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.
>>
> 
> Then they are cordially invited to argue their position, preferably
> using arguments.
> 
>> 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.
>>
> 
> Sure. I'd prefer on-list to be honest, but if you feel it needs to be
> off-list, that is also fine.
> 

I personally wouldn't mind on-list, but I'm not sure I want to catapult 
all the members of the ad-hoc group we gathered off-list into a sudden 
public discussion.

At any rate, provided that you happen to change your position, I'd 
expect other people who agree with your current view to chime in 
on-list, if they strongly disagree with a proposal that upgrades the 
platform to ACPI 6.3, and then we can discuss further.

Regards,

/Pete

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

[Ard Biesheuvel]

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

Actually, even though I *really* like the RPi4 port in terms of
functionality and polish, it is *not* a good example to follow to
build a new platform.

First of all, it is based on PrePi instead of PrePeiCore, and so it is
very difficult to add PEI phase drivers for, e.g., capsule update or
measured boot. But in general, the quirky hardware and firmware
arrangement make it a one-off design IMO, and new platforms should use
something like SynQuacer as a reference instead.

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

[Pete Batard]

On 2020.03.30 15:19, 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.
>>
> 
> Actually, even though I *really* like the RPi4 port in terms of
> functionality and polish, it is *not* a good example to follow to
> build a new platform.
> 
> First of all, it is based on PrePi instead of PrePeiCore, and so it is
> very difficult to add PEI phase drivers for, e.g., capsule update or
> measured boot. But in general, the quirky hardware and firmware
> arrangement make it a one-off design IMO, and new platforms should use
> something like SynQuacer as a reference instead.

I'll be the first to agree that are much better platforms to use as 
starting point. Unfortunately they don't come at the same price point 
and widespread availability, which is why I think there is value is 
trying to showcase what we can with the Pi's, despite their major 
annoying quirks.

Regards,

/Pete

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

[Ard Biesheuvel]

On Fri, 27 Mar 2020 at 14:01, Pete Batard <pete@akeo.ie> wrote:
>
> ACPI 6.3 added a 16-bit SPE overflow Interrupt field, replacing
> 2 of the 3 reserved bytes that are defined at the end of the
> GICC structure for 6.0.
>
> Add a new macro to initialise the new field.
>
> Signed-off-by: Pete Batard <pete@akeo.ie>
> ---
>  EmbeddedPkg/Include/Library/AcpiLib.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/EmbeddedPkg/Include/Library/AcpiLib.h b/EmbeddedPkg/Include/Library/AcpiLib.h
> index 5c6e2075de79..57d7bd595584 100644
> --- a/EmbeddedPkg/Include/Library/AcpiLib.h
> +++ b/EmbeddedPkg/Include/Library/AcpiLib.h
> @@ -64,6 +64,14 @@
>      {EFI_ACPI_RESERVED_BYTE, EFI_ACPI_RESERVED_BYTE, EFI_ACPI_RESERVED_BYTE}         \
>    }
>
> +#define EFI_ACPI_6_3_GICC_STRUCTURE_INIT(GicId, AcpiCpuUid, Mpidr, Flags, PmuIrq,    \
> +    GicBase, GicVBase, GicHBase, GsivId, GicRBase, Efficiency, SpeOvflIrq)           \
> +  {                                                                                  \
> +    EFI_ACPI_6_0_GIC, sizeof (EFI_ACPI_6_0_GIC_STRUCTURE), EFI_ACPI_RESERVED_WORD,   \
> +    GicId, AcpiCpuUid, Flags, 0, PmuIrq, 0, GicBase, GicVBase, GicHBase,             \
> +    GsivId, GicRBase, Mpidr, Efficiency, EFI_ACPI_RESERVED_BYTE, SpeOvflIrq          \
> +  }
> +
>  #define EFI_ACPI_6_0_GIC_MSI_FRAME_INIT(GicMsiFrameId, PhysicalBaseAddress, Flags, SPICount, SPIBase) \
>    { \
>      EFI_ACPI_6_0_GIC_MSI_FRAME, sizeof (EFI_ACPI_6_0_GIC_MSI_FRAME_STRUCTURE), EFI_ACPI_RESERVED_WORD, \

[with the sizeof() changed to refer to EFI_ACPI_6_3_GIC_STRUCTURE]

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Pushed to edk2/master