Re: [edk2-devel] [PATCH v1 05/11] ShellPkg: acpiview: SLIT: Validate System Locality count

["Gao, Zhichao" <zhichao.gao@intel.com>]

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.gao@intel.com>; devel@edk2.groups.io
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini
> <Matteo.Carlini@arm.com>; nd <nd@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.gao@intel.com>
> Sent: Monday, August 19, 2019 2:19
> To: devel@edk2.groups.io; Krzysztof Koch <Krzysztof.Koch@arm.com>
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini
> <Matteo.Carlini@arm.com>; nd <nd@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.carsey@intel.com>; Ni, Ray
> > <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
> > Sami.Mujawar@arm.com; Matteo.Carlini@arm.com; nd@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.koch@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)'
> >
> >
> >
> >
> 
> 
>