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

["Chiu, Chasel" <chasel.chiu@intel.com>]

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
> > >
> > >
> > >