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: shenglei.zhang@intel.com) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by groups.io with SMTP; Wed, 04 Sep 2019 22:52:00 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Sep 2019 22:52:00 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,469,1559545200"; d="scan'208";a="173830451" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga007.jf.intel.com with ESMTP; 04 Sep 2019 22:51:59 -0700 Received: from fmsmsx119.amr.corp.intel.com (10.18.124.207) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 4 Sep 2019 22:51:59 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX119.amr.corp.intel.com (10.18.124.207) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 4 Sep 2019 22:51:59 -0700 Received: from shsmsx106.ccr.corp.intel.com ([169.254.10.86]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.92]) with mapi id 14.03.0439.000; Thu, 5 Sep 2019 13:51:57 +0800 From: "Zhang, Shenglei" To: "Chiu, Chasel" , "devel@edk2.groups.io" CC: "Kubacki, Michael A" , "Desimone, Nathaniel L" , "Gao, Liming" Subject: Re: [edk2-devel] [PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib: Change TopOfTemporaryRam type Thread-Topic: [edk2-devel] [PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib: Change TopOfTemporaryRam type Thread-Index: AQHVYYrWoiM71jMSU0ydmmayYdsKMacbb9XggADfRND//5IHgIAAtsxA Date: Thu, 5 Sep 2019 05:51:56 +0000 Message-ID: References: <20190902123452.27716-1-shenglei.zhang@intel.com> <3C3EFB470A303B4AB093197B6777CCEC5048145B@PGSMSX111.gar.corp.intel.com> <3C3EFB470A303B4AB093197B6777CCEC50481D6C@PGSMSX111.gar.corp.intel.com> In-Reply-To: <3C3EFB470A303B4AB093197B6777CCEC50481D6C@PGSMSX111.gar.corp.intel.com> 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: shenglei.zhang@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable It works after I apply the change according to your advice. And that makes the code more clean. Thanks for your advice. I'll sent out a v2 patch. Best Regards, Shenglei > -----Original Message----- > From: Chiu, Chasel > Sent: Thursday, September 5, 2019 10:54 AM > To: Zhang, Shenglei ; devel@edk2.groups.io > Cc: Kubacki, Michael A ; Desimone, Nathanie= l > L ; Gao, Liming > Subject: RE: [edk2-devel] [PATCH] > MinPlatformPkg/SecFspWrapperPlatformSecLib: Change > TopOfTemporaryRam type >=20 >=20 > Thanks for explanations. > So looks like the intention is to support X64 build scenarios, I think > TopOfTemporaryRam should be defined as UINTN from beginning so it can > hold 64bits address in X64 build. > Please evaluate if we could make these changes. >=20 > Thanks! > Chasel >=20 > > -----Original Message----- > > From: Zhang, Shenglei > > Sent: Thursday, September 5, 2019 10:11 AM > > To: Chiu, Chasel ; devel@edk2.groups.io > > Cc: Kubacki, Michael A ; Desimone, > Nathaniel > > L ; Gao, Liming > > Subject: RE: [edk2-devel] [PATCH] > > MinPlatformPkg/SecFspWrapperPlatformSecLib: Change > > TopOfTemporaryRam type > > > > Hi chasel, > > > > > -----Original Message----- > > > From: Chiu, Chasel > > > Sent: Wednesday, September 4, 2019 8:13 PM > > > To: devel@edk2.groups.io; Zhang, Shenglei > > > Cc: Kubacki, Michael A ; Desimone, > > > Nathaniel L ; Gao, Liming > > > > > > Subject: RE: [edk2-devel] [PATCH] > > > MinPlatformPkg/SecFspWrapperPlatformSecLib: Change > > TopOfTemporaryRam > > > type > > > > > > > > > Hi Shenglei, > > > > > > Would you please elaborate a little on how casting to UINTN can > > > resolve the overflow scenario and why 64bits OS will affect this cod= e? > > > > Actually casting to UINTN can't resolve the overflow. > > What I mean is the result of (TopOfTemporaryRam - sizeof (UINT32)) may > > overflow. > > While it's meaningless to cast an already overflowed result to another= type. > > So I update the code to cast the variable to UINT before it is arithme= tically > > operated. > > > > Under 64bits OS, the size of UINTN is 64-bit and the original > > "(TopOfTemporaryRam - sizeof (UINT32)) " > > is 32-bit. So the operation for casting will be performed. That's why = 64bits > > OS will affect this code. > > > > Thanks, > > Shenglei > > > > > > > > Thanks! > > > Chasel > > > > > > > -----Original Message----- > > > > From: devel@edk2.groups.io On Behalf Of > > > > Zhang, Shenglei > > > > Sent: Monday, September 2, 2019 8:35 PM > > > > To: devel@edk2.groups.io > > > > Cc: Kubacki, Michael A ; Chiu, Chasel > > > > ; Desimone, Nathaniel L > > > > ; Gao, Liming > > > > Subject: [edk2-devel] [PATCH] > > > MinPlatformPkg/SecFspWrapperPlatformSecLib: > > > > Change TopOfTemporaryRam type > > > > > > > > Cast TopOfTemporaryRam's from UINT32 to UINTN in the expression. > > > > The original code (TopOfTemporaryRam - sizeof (UINT32)) may cause > > > > overflow. As a result the operation under 64-bit OS environment, > > > (UINT)(...), > > > > may cast a overflowed 4-byte result to 8-byte one. > > > > > > > > Cc: Michael Kubacki > > > > Cc: Chasel Chiu > > > > Cc: Nate DeSimone > > > > Cc: Liming Gao > > > > Signed-off-by: Shenglei Zhang > > > > --- > > > > .../Library/SecFspWrapperPlatformSecLib/SecGetPerformance.c |= 2 > > +- > > > > .../SecFspWrapperPlatformSecLib/SecPlatformInformation.c |= 2 > > +- > > > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git > > > > > > > > a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf > > > or > > > > mSecLib/SecGetPerformance.c > > > > > > > > b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf > > > or > > > > mSecLib/SecGetPerformance.c > > > > index c4eeb2b1..0cc42f96 100644 > > > > --- > > > > > > > > a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf > > > or > > > > mSecLib/SecGetPerformance.c > > > > +++ > > > > > > b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlat > > > > +++ formSecLib/SecGetPerformance.c > > > > @@ -79,7 +79,7 @@ SecGetPerformance ( > > > > // > > > > TopOfTemporaryRam =3D (UINT32)(UINTN)TopOfTemporaryRamPpi - > > > > sizeof(UINT32); > > > > TopOfTemporaryRam -=3D sizeof(UINT32) * 2; > > > > - Count =3D *(UINT32 *) (UINTN) (TopOfTemporaryRam - > > sizeof > > > > (UINT32)); > > > > + Count =3D *(UINT32 *)((UINTN)TopOfTemporaryRam - > > sizeof > > > > (UINT32)); > > > > Size =3D Count * sizeof (UINT32); > > > > > > > > Ticker =3D *(UINT64 *) (UINTN) (TopOfTemporaryRam - sizeof (UIN= T32) > > > > - > > > Size > > > > - sizeof (UINT32) * 2); diff --git > > > > > > > > a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf > > > or > > > > mSecLib/SecPlatformInformation.c > > > > > > > > b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf > > > or > > > > mSecLib/SecPlatformInformation.c > > > > index 5b94ed2b..1bcee5f4 100644 > > > > --- > > > > > > > > a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf > > > or > > > > mSecLib/SecPlatformInformation.c > > > > +++ > > > > > > b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlat > > > > +++ formSecLib/SecPlatformInformation.c > > > > @@ -61,7 +61,7 @@ SecPlatformInformation ( > > > > // > > > > TopOfTemporaryRam =3D (UINT32)(UINTN)TopOfTemporaryRamPpi - > > sizeof > > > > (UINT32); > > > > TopOfTemporaryRam -=3D sizeof(UINT32) * 2; > > > > - Count =3D *((UINT32 *)(UINTN) (TopOfTemporaryRam - > > sizeof > > > > (UINT32))); > > > > + Count =3D *((UINT32 *)((UINTN)TopOfTemporaryRam - > > sizeof > > > > (UINT32))); > > > > Size =3D Count * sizeof (IA32_HANDOFF_STATUS); > > > > > > > > if ((*StructureSize) < (UINT64) Size) { > > > > -- > > > > 2.18.0.windows.1 > > > > > > > > > > > >=20