Hi Zhichao,
Please find my comments inline marked as [Krzysztof]. Kind regards, Krzysztof -----Original Message----- From: Gao, Zhichao <zhichao....@intel.com> Sent: Monday, August 19, 2019 2:19 To: devel@edk2.groups.io; Krzysztof Koch <krzysztof.k...@arm.com> Cc: Carsey, Jaben <jaben.car...@intel.com>; Ni, Ray <ray...@intel.com>; Sami Mujawar <sami.muja...@arm.com>; Matteo Carlini <matteo.carl...@arm.com>; nd <n...@arm.com> Subject: RE: [edk2-devel] [PATCH v1 05/11] ShellPkg: acpiview: SLIT: Validate System Locality count > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Krzysztof Koch > Sent: Thursday, August 15, 2019 9:11 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: [edk2-devel] [PATCH v1 05/11] ShellPkg: acpiview: SLIT: > Validate System Locality count > > 1. Check if the 'Number of System Localities' provided can be > represented in the SLIT table. The table 'Length' field is a 32-bit > value while the 'Number of System Localities' field is 64-bit long. > > 2. Check if the SLIT matrix fits in the table buffer. If N is the SLIT > locality count, then the matrix used to represent the localities is > N*N bytes long. The ACPI table length must be big enough to fit the matrix. > > 3. Remove (now) redundant 64x64 bit multiplication. Why removing? This change is added to fixed the issue build error with IA32 multiplication of two 64 bits data. The change of #3 should be removed from the patch. Keeping the variable size as UINT64 wouldn't affect the result. Thanks, Zhichao [Krzysztof] If you look closely, in this patch I have removed the need to 64x64 bit multiplication. As I explain in the commit message, the specification of the SLIT table has an error. The System Locality Count is a 64-bit value while the ACPI table length field is 32-bit long. Consequently, after the right checks are implemented (in this patch), it is possible to operate on 32-bit values only. I believe that now MultU64x64() is no longer needed so I reverted back to the '*' multiplication operator. Please let me know what you think. > > Signed-off-by: Krzysztof Koch <krzysztof.k...@arm.com> > --- > > Notes: > v1: > - Validate the 'Number of System Localities' Field [Krzysztof] > - Remove redundant 64x64 bit multiplication [Krzysztof] > > > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c > | > 47 +++++++++++++++++--- > 1 file changed, 42 insertions(+), 5 deletions(-) > > diff --git > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser > .c > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser > .c > index > 17e2166a09d8615b714e0c51d4d93d293fcdf601..e4625ee8b13907893a9b6990 > ecb956baf91cc3b9 100644 > --- > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser > .c > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitPa > +++ rs > +++ er.c > @@ -30,7 +30,7 @@ STATIC CONST ACPI_PARSER SlitParser[] = { > /** > Macro to get the value of a System Locality **/ -#define > SLIT_ELEMENT(Ptr, i, j) *(Ptr + (MultU64x64 (i, LocalityCount)) + j) > +#define SLIT_ELEMENT(Ptr, i, j) *(Ptr + (i * LocalityCount) + j) > > /** > This function parses the ACPI SLIT table. > @@ -57,9 +57,9 @@ ParseAcpiSlit ( > ) > { > UINT32 Offset; > - UINT64 Count; > - UINT64 Index; > - UINT64 LocalityCount; > + UINT32 Count; > + UINT32 Index; > + UINT32 LocalityCount; > UINT8* LocalityPtr; > CHAR16 Buffer[80]; // Used for AsciiName param of ParseAcpi > > @@ -87,8 +87,45 @@ ParseAcpiSlit ( > return; > } > > + /* > + Despite the 'Number of System Localities' being a 64-bit field in SLIT, > + the maximum number of localities that can be represented in SLIT > + is > limited > + by the 'Length' field of the ACPI table. > + > + Since the ACPI table length field is 32-bit wide. The maximum number of > + localities that can be represented in SLIT can be calculated as: > + > + MaxLocality = sqrt (MAX_UINT32 - sizeof > (EFI_ACPI_6_3_SYSTEM_LOCALITY_DISTANCE_INFORMATION_TABLE_HEAD > ER)) > + = 65535 > + = MAX_UINT16 > + */ > + if (*SlitSystemLocalityCount > MAX_UINT16) { > + IncrementErrorCount (); > + Print ( > + L"ERROR: The Number of System Localities provided can't be > represented " \ > + L"in the SLIT table. SlitSystemLocalityCount = %ld. " \ > + L"MaxLocalityCountAllowed = %d.\n", > + *SlitSystemLocalityCount, > + MAX_UINT16 > + ); > + return; > + } > + > + LocalityCount = (UINT32)*SlitSystemLocalityCount; > + > + // Make sure system localities fit in the table buffer provided if > + (Offset + (LocalityCount * LocalityCount) > AcpiTableLength) { > + IncrementErrorCount (); > + Print ( > + L"ERROR: Invalid Number of System Localities. " \ > + L"SlitSystemLocalityCount = %ld. AcpiTableLength = %d.\n", > + *SlitSystemLocalityCount, > + AcpiTableLength > + ); > + return; > + } > + > LocalityPtr = Ptr + Offset; > - LocalityCount = *SlitSystemLocalityCount; > > // We only print the Localities if the count is less than 16 > // If the locality count is more than 16 then refer to the > -- > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#46023): https://edk2.groups.io/g/devel/message/46023 Mute This Topic: https://groups.io/mt/32886579/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
