I think it would be easier to see/review these changes logically if the functional changes (1 and 3) were separate from the refactoring change (2).
Reviewed-by: Jaben Carsey <jaben.car...@intel.com> > -----Original Message----- > From: Krzysztof Koch [mailto:krzysztof.k...@arm.com] > Sent: Thursday, July 11, 2019 11:53 PM > To: devel@edk2.groups.io > Cc: Carsey, Jaben <jaben.car...@intel.com>; Ni, Ray <ray...@intel.com>; > Gao, Zhichao <zhichao....@intel.com>; sami.muja...@arm.com; > matteo.carl...@arm.com; n...@arm.com > Subject: [PATCH v1 01/11] ShellPkg: acpiview: FADT: Validate global pointers > before use > > 1. Check if the global pointer have been successfully updated before > they are later used to control the parsing logic in the FADT acpiview > parser. > > 2. Remove redundant forward function declarations by repositioning > blocks of code. > > 3. Allow silencing ACPI table content validation errors which do not > cause table parsing to fail. > > Signed-off-by: Krzysztof Koch <krzysztof.k...@arm.com> > --- > > Changes can be seen at: > https://github.com/KrzysztofKoch1/edk2/commit/49cc41430775fb93205e302 > 590a7d31f080c3952 > > Notes: > v1: > - improve the logic in the parser [Krzysztof] > > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c > | 131 ++++++++------------ > 1 file changed, 51 insertions(+), 80 deletions(-) > > diff --git > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser. > c > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser. > c > index > cee7ee0770433da96d6042d2f5d687903f4b5495..600d3b16d7b22b61c1a1fd21 > ecb93f16c7f8fa1a 100644 > --- > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser. > c > +++ > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser. > c > @@ -1,7 +1,7 @@ > /** @file > FADT table parser > > - Copyright (c) 2016 - 2018, ARM Limited. All rights reserved. > + Copyright (c) 2016 - 2019, ARM Limited. All rights reserved. > SPDX-License-Identifier: BSD-2-Clause-Patent > > @par Reference(s): > @@ -12,6 +12,7 @@ > #include <Library/UefiLib.h> > #include "AcpiParser.h" > #include "AcpiTableParser.h" > +#include "AcpiView.h" > > // Local variables > STATIC CONST UINT32* DsdtAddress; > @@ -46,7 +47,17 @@ EFIAPI > ValidateFirmwareCtrl ( > IN UINT8* Ptr, > IN VOID* Context > - ); > +) > +{ > +#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64) > + if (*(UINT32*)Ptr != 0) { > + IncrementErrorCount (); > + Print ( > + L"\nERROR: Firmware Control must be zero for ARM platforms." > + ); > + } > +#endif > +} > > /** > This function validates the X_Firmware Control Field. > @@ -61,7 +72,17 @@ EFIAPI > ValidateXFirmwareCtrl ( > IN UINT8* Ptr, > IN VOID* Context > - ); > +) > +{ > +#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64) > + if (*(UINT64*)Ptr != 0) { > + IncrementErrorCount (); > + Print ( > + L"\nERROR: X Firmware Control must be zero for ARM platforms." > + ); > + } > +#endif > +} > > /** > This function validates the flags. > @@ -76,7 +97,17 @@ EFIAPI > ValidateFlags ( > IN UINT8* Ptr, > IN VOID* Context > - ); > +) > +{ > +#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64) > + if (((*(UINT32*)Ptr) & HW_REDUCED_ACPI) == 0) { > + IncrementErrorCount (); > + Print ( > + L"\nERROR: HW_REDUCED_ACPI flag must be set for ARM platforms." > + ); > + } > +#endif > +} > > /** > An ACPI_PARSER array describing the ACPI FADT Table. > @@ -142,81 +173,6 @@ STATIC CONST ACPI_PARSER FadtParser[] = { > {L"Hypervisor VendorIdentity", 8, 268, L"%lx", NULL, NULL, NULL, NULL} > }; > > -/** > - This function validates the Firmware Control Field. > - > - @param [in] Ptr Pointer to the start of the field data. > - @param [in] Context Pointer to context specific information e.g. this > - could be a pointer to the ACPI table header. > -**/ > -STATIC > -VOID > -EFIAPI > -ValidateFirmwareCtrl ( > - IN UINT8* Ptr, > - IN VOID* Context > -) > -{ > -#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64) > - if (*(UINT32*)Ptr != 0) { > - IncrementErrorCount (); > - Print ( > - L"\nERROR: Firmware Control must be zero for ARM platforms." > - ); > - } > -#endif > -} > - > -/** > - This function validates the X_Firmware Control Field. > - > - @param [in] Ptr Pointer to the start of the field data. > - @param [in] Context Pointer to context specific information e.g. this > - could be a pointer to the ACPI table header. > -**/ > -STATIC > -VOID > -EFIAPI > -ValidateXFirmwareCtrl ( > - IN UINT8* Ptr, > - IN VOID* Context > -) > -{ > -#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64) > - if (*(UINT64*)Ptr != 0) { > - IncrementErrorCount (); > - Print ( > - L"\nERROR: X Firmware Control must be zero for ARM platforms." > - ); > - } > -#endif > -} > - > -/** > - This function validates the flags. > - > - @param [in] Ptr Pointer to the start of the field data. > - @param [in] Context Pointer to context specific information e.g. this > - could be a pointer to the ACPI table header. > -**/ > -STATIC > -VOID > -EFIAPI > -ValidateFlags ( > - IN UINT8* Ptr, > - IN VOID* Context > -) > -{ > -#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64) > - if (((*(UINT32*)Ptr) & HW_REDUCED_ACPI) == 0) { > - IncrementErrorCount (); > - Print ( > - L"\nERROR: HW_REDUCED_ACPI flag must be set for ARM platforms." > - ); > - } > -#endif > -} > - > /** > This function parses the ACPI FADT table. > This function parses the FADT table and optionally traces the ACPI table > fields. > @@ -248,12 +204,27 @@ ParseAcpiFadt ( > PARSER_PARAMS (FadtParser) > ); > > + // Check if the values used to control the parsing logic have been > + // successfully read. > + if ((DsdtAddress == NULL) || > + (FadtMinorRevision == NULL) || > + (X_DsdtAddress == NULL)) { > + IncrementErrorCount (); > + Print ( > + L"ERROR: Insufficient table length. AcpiTableLength = %d. " \ > + L"FADT parsing aborted.\n", > + AcpiTableLength > + ); > + return; > + } > + > if (Trace) { > Print (L"\nSummary:\n"); > PrintFieldName (2, L"FADT Version"); > Print (L"%d.%d\n", *AcpiHdrInfo.Revision, *FadtMinorRevision); > > - if (*GetAcpiXsdtHeaderInfo ()->OemTableId != > *AcpiHdrInfo.OemTableId) { > + if (GetConsistencyChecking () && > + (*GetAcpiXsdtHeaderInfo ()->OemTableId != > *AcpiHdrInfo.OemTableId)) { > IncrementErrorCount (); > Print (L"ERROR: OEM Table Id does not match with RSDT/XSDT.\n"); > } > -- > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#43659): https://edk2.groups.io/g/devel/message/43659 Mute This Topic: https://groups.io/mt/32439503/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
