Sorry for missing consider of the commit message #1 and #2. I got your point. SlitSystemLocalityCount's high 32 bit (actually high 48 bit) data is useless. On my opinion, this field should be 2 bytes length and the spec should be updated. Then our code can be updated to match the spec. For now, I think the checking (*SlitSystemLocalityCount > MAX_UINT16) before it is enough.
Thanks, Zhichao > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Krzysztof Koch > Sent: Monday, August 19, 2019 2:28 PM > To: Gao, Zhichao <zhichao....@intel.com>; devel@edk2.groups.io > 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 > > > 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 (#46024): https://edk2.groups.io/g/devel/message/46024 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] -=-=-=-=-=-=-=-=-=-=-=-
