Re: [edk2-devel] [PATCH] MinPlatformPkg/SecFspWrapperPlatformSecLib: Change TopOfTemporaryRam type

["Zhang, Shenglei" <shenglei.zhang@intel.com>]

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 <shenglei.zhang@intel.com>; devel@edk2.groups.io
> Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Desimone, Nathaniel
> L <nathaniel.l.desimone@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: RE: [edk2-devel] [PATCH]
> MinPlatformPkg/SecFspWrapperPlatformSecLib: Change
> TopOfTemporaryRam type
> 
> 
> 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.
> 
> Thanks!
> Chasel
> 
> > -----Original Message-----
> > From: Zhang, Shenglei
> > Sent: Thursday, September 5, 2019 10:11 AM
> > To: Chiu, Chasel <chasel.chiu@intel.com>; devel@edk2.groups.io
> > Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Desimone,
> Nathaniel
> > L <nathaniel.l.desimone@intel.com>; Gao, Liming <liming.gao@intel.com>
> > 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 <shenglei.zhang@intel.com>
> > > Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Desimone,
> > > Nathaniel L <nathaniel.l.desimone@intel.com>; Gao, Liming
> > > <liming.gao@intel.com>
> > > 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 code?
> >
> > 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 arithmetically
> > 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 <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 <michael.a.kubacki@intel.com>; Chiu, Chasel
> > > > <chasel.chiu@intel.com>; Desimone, Nathaniel L
> > > > <nathaniel.l.desimone@intel.com>; Gao, Liming <liming.gao@intel.com>
> > > > 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 <michael.a.kubacki@intel.com>
> > > > Cc: Chasel Chiu <chasel.chiu@intel.com>
> > > > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> > > > Cc: Liming Gao <liming.gao@intel.com>
> > > > Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> > > > ---
> > > >  .../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 = (UINT32)(UINTN)TopOfTemporaryRamPpi -
> > > > sizeof(UINT32);
> > > >    TopOfTemporaryRam -= sizeof(UINT32) * 2;
> > > > -  Count             = *(UINT32 *) (UINTN) (TopOfTemporaryRam -
> > sizeof
> > > > (UINT32));
> > > > +  Count             = *(UINT32 *)((UINTN)TopOfTemporaryRam -
> > sizeof
> > > > (UINT32));
> > > >    Size              = Count * sizeof (UINT32);
> > > >
> > > >    Ticker = *(UINT64 *) (UINTN) (TopOfTemporaryRam - sizeof (UINT32)
> > > > -
> > > 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 = (UINT32)(UINTN)TopOfTemporaryRamPpi -
> > sizeof
> > > > (UINT32);
> > > >    TopOfTemporaryRam -= sizeof(UINT32) * 2;
> > > > -  Count             = *((UINT32 *)(UINTN) (TopOfTemporaryRam -
> > sizeof
> > > > (UINT32)));
> > > > +  Count             = *((UINT32 *)((UINTN)TopOfTemporaryRam -
> > sizeof
> > > > (UINT32)));
> > > >    Size              = Count * sizeof (IA32_HANDOFF_STATUS);
> > > >
> > > >    if ((*StructureSize) < (UINT64) Size) {
> > > > --
> > > > 2.18.0.windows.1
> > > >
> > > >
> > > >