From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.120, mailfrom: zhichao.gao@intel.com) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by groups.io with SMTP; Mon, 19 Aug 2019 00:16:44 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 19 Aug 2019 00:16:44 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,403,1559545200"; d="scan'208";a="172041478" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga008.jf.intel.com with ESMTP; 19 Aug 2019 00:16:43 -0700 Received: from fmsmsx604.amr.corp.intel.com (10.18.126.84) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 19 Aug 2019 00:16:43 -0700 Received: from fmsmsx604.amr.corp.intel.com (10.18.126.84) by fmsmsx604.amr.corp.intel.com (10.18.126.84) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Mon, 19 Aug 2019 00:16:42 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx604.amr.corp.intel.com (10.18.126.84) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Mon, 19 Aug 2019 00:16:42 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.80]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.62]) with mapi id 14.03.0439.000; Mon, 19 Aug 2019 15:16:39 +0800 From: "Gao, Zhichao" To: "devel@edk2.groups.io" , "krzysztof.koch@arm.com" CC: "Carsey, Jaben" , "Ni, Ray" , Sami Mujawar , Matteo Carlini , nd Subject: Re: [edk2-devel] [PATCH v1 05/11] ShellPkg: acpiview: SLIT: Validate System Locality count Thread-Topic: [edk2-devel] [PATCH v1 05/11] ShellPkg: acpiview: SLIT: Validate System Locality count Thread-Index: AQHVU2sn8cTIjNKO9Eun9NSKtXUilqcBruBw///SsACAAIe5kA== Date: Mon, 19 Aug 2019 07:16:38 +0000 Message-ID: <3CE959C139B4C44DBEA1810E3AA6F9000B823A23@SHSMSX101.ccr.corp.intel.com> References: <20190815131121.52644-1-krzysztof.koch@arm.com> <20190815131121.52644-6-krzysztof.koch@arm.com> <3CE959C139B4C44DBEA1810E3AA6F9000B82386D@SHSMSX101.ccr.corp.intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: zhichao.gao@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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) befo= re 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 ; devel@edk2.groups.io > Cc: Carsey, Jaben ; Ni, Ray ; > Sami Mujawar ; Matteo Carlini > ; nd > Subject: Re: [edk2-devel] [PATCH v1 05/11] ShellPkg: acpiview: SLIT: Val= idate > System Locality count >=20 >=20 > Hi Zhichao, >=20 > Please find my comments inline marked as [Krzysztof]. >=20 > Kind regards, >=20 > Krzysztof >=20 >=20 > -----Original Message----- > From: Gao, Zhichao > Sent: Monday, August 19, 2019 2:19 > To: devel@edk2.groups.io; Krzysztof Koch > Cc: Carsey, Jaben ; Ni, Ray ; > Sami Mujawar ; Matteo Carlini > ; nd > Subject: RE: [edk2-devel] [PATCH v1 05/11] ShellPkg: acpiview: SLIT: Val= idate > System Locality count >=20 >=20 >=20 > > -----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 ; Ni, Ray > > ; Gao, Zhichao ; > > 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 ma= trix. > > > > 3. Remove (now) redundant 64x64 bit multiplication. >=20 > Why removing? This change is added to fixed the issue build error with I= A32 > 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. >=20 > Thanks, > Zhichao >=20 > [Krzysztof] If you look closely, in this patch I have removed the need t= o 64x64 > bit multiplication. As I explain in the commit message, the specificatio= n of the > SLIT table has an error. The System Locality Count is a 64-bit value whi= le the > ACPI table length field is 32-bit long. >=20 > 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. >=20 > Please let me know what you think. >=20 > > > > Signed-off-by: Krzysztof Koch > > --- > > > > 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[] =3D { > > /** > > 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 num= ber of > > + localities that can be represented in SLIT can be calculated as: > > + > > + MaxLocality =3D sqrt (MAX_UINT32 - sizeof > > > (EFI_ACPI_6_3_SYSTEM_LOCALITY_DISTANCE_INFORMATION_TABLE_HEAD > > ER)) > > + =3D 65535 > > + =3D 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 =3D %ld. " \ > > + L"MaxLocalityCountAllowed =3D %d.\n", > > + *SlitSystemLocalityCount, > > + MAX_UINT16 > > + ); > > + return; > > + } > > + > > + LocalityCount =3D (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 =3D %ld. AcpiTableLength =3D %d.\n"= , > > + *SlitSystemLocalityCount, > > + AcpiTableLength > > + ); > > + return; > > + } > > + > > LocalityPtr =3D Ptr + Offset; > > - LocalityCount =3D *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)' > > > > > > > > >=20 >=20 >=20