Re: [edk2-devel] [PATCH edk2-platforms v1 00/11] Fix build breaks and update IORT revision macro

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Ard,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar


On 10/11/2021 11:33 AM, Ard Biesheuvel wrote:
> On Wed, 10 Nov 2021 at 12:01, Sami Mujawar <sami.mujawar@arm.com> wrote:
>> The first 4 patches in this series fix build breaks in upstream
>> edk2-platforms code for Socionext, Lemaker and SynQuacerEvalBoard.
>>
>> The IORT Rev E.b specification updates the IORT table revision
>> to 3. Following this the IORT table revision macro
>> EFI_ACPI_IO_REMAPPING_TABLE_REVISION which was at Rev 0 has
>> been renamed to EFI_ACPI_IO_REMAPPING_TABLE_REV0.
> I *really* don't like this. All other ACPI tables are set up in a
> forward compatible way, where an existing definition in code is
> guaranteed to comply with later versions of the spec, and can always
> claim to be the newest revision. This is the reason why
> xxx_TABLE_REVISION macros exist in the first place.
>
> So I suppose that means that the IORT was modified in a non-forward
> compatible fashion? Does that mean that newer firmware has to stick
> with the old revision in order to remain able to boot older OS
> versions?
[SAMI] I don't think the OS compatibility is broken by the IORT spec.

In edk2, the revision macro for IORT is defined differently when compared
to the other ACPI table version macros.

e.g. The IORT revision macro was defined as
"#define EFI_ACPI_IO_REMAPPING_TABLE_REVISION        0x0"
compare this to MADT table version macro which is
"#define EFI_ACPI_6_4_MULTIPLE_APIC_DESCRIPTION_TABLE_REVISION 0x05".

Note the EFI_ACPI_x_x prefix in MADT macro name. This allows existing tables
to work, as for ACPI 6.3 the version in Acpi63.h i.e.
EFI_ACPI_6_3_MULTIPLE_APIC_DESCRIPTION_TABLE_REVISION will be used.

Since IORT table version macro does not have any version information in the
macro name EFI_ACPI_IO_REMAPPING_TABLE_REV0 is introduced.

The IORT Rev E.b introduces updates the table vertion to 3 and therefore
EFI_ACPI_IO_REMAPPING_TABLE_REV3 has been introduced.
Ref: https://edk2.groups.io/g/devel/message/83547

This IORT Rev E.b patch series essentially retrofits the version tag 
missing in the
IORT table revision macro and this edk2-platform series is a 
corresponding adotption.
[/SAMI]
>
>
>> Therefore,
>> the last 7 patches in this series are added to reflect this
>> change for the following platforms:
>>   - FVP
>>   - Morello FVP
>>   - SGI
>>   - AMD/Styx
>>   - Socionext/SynQuacer
>>   - RaspberryPi
>>   - N1SDP
>>
>> Note: This patch series is dependent on the edk2 patch series:
>>    "[PATCH v3 0/8] IORT Rev E.b specification updates"
>>    (https://edk2.groups.io/g/devel/message/83545), which must
>> be merged in edk2 before this patch series can be integrated.
>>
>> The changes can be seen at:
>> https://github.com/samimujawar/edk2-platforms/tree/1527_iort_rev_eb_platforms_v1
>>
>> Sami Mujawar (11):
>>    Platform/Socionext: Fix build break for SynQuacerEvalBoard
>>    Platform/LeMaker: Fix build break for CelloBoard
>>    Platform/Socionext/SynQuacerEvalBoard: Add missing ArmSoftFloatLib
>>    Platform/Socionext/SynQuacerEvalBoard: Disable EbcDxe for AArch32
>>      builds
>>    Platform/ARM: FVP: Update for IORT revision macro renaming
>>    Platform/ARM: Morello: Update for IORT revision macro renaming
>>    Platform/ARM: SGI: Update for IORT revision macro renaming
>>    Silicon/AMD/Styx: Update for IORT revision macro renaming
>>    Silicon/Socionext/SynQuacer: Update for IORT revision macro renaming
>>    Platform/RaspberryPi: Update for IORT revision macro renaming
>>    Platform/ARM: N1SDP: Update for IORT revision macro renaming
>>
>>   Platform/ARM/Morello/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManagerFvp.c  |  2 +-
>>   Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c       |  2 +-
>>   Platform/ARM/SgiPkg/AcpiTables/Iort.aslc                                                     |  2 +-
>>   Platform/ARM/VExpressPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c |  2 +-
>>   Platform/LeMaker/CelloBoard/CelloBoard.dsc                                                   |  4 +++-
>>   Platform/RaspberryPi/AcpiTables/Iort.aslc                                                    |  2 +-
>>   Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc                                 | 11 +++++++++--
>>   Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.fdf                                 |  8 +++++++-
>>   Silicon/AMD/Styx/Drivers/AcpiPlatformDxe/Iort.aslc                                           |  2 +-
>>   Silicon/Socionext/SynQuacer/AcpiTables/Iort.aslc                                             |  2 +-
>>   10 files changed, 26 insertions(+), 11 deletions(-)
>>
>> --
>> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>>
>>
>>
>> 
>>
>>