Re: [Patch 1/2] MdeModulePkg SmmIpl: Fill Smram range for SMM driver when LMFA enable

["Gao, Liming" <liming.gao@intel.com>]

Agree. Thanks!

> -----Original Message-----
> From: Zeng, Star
> Sent: Friday, December 09, 2016 6:20 PM
> To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [Patch 1/2] MdeModulePkg SmmIpl: Fill Smram range for
> SMM driver when LMFA enable
> 
> Liming,
> 
> A small comment added inline.
> With that update, Reviewed-by: Star Zeng <star.zeng@intel.com> to this
> patch series.
> 
> On 2016/12/1 14:49, Liming Gao wrote:
> > Allocate the additional Smram range to describe the reserved smram for
> > SMM core and driver when LMFA feature is enabled.
> >
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Liming Gao <liming.gao@intel.com>
> > ---
> >  MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 39
> +++++++++++++++++++++++++++++-----
> >  1 file changed, 34 insertions(+), 5 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> > index 18bec84..fd7027d 100644
> > --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> > +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> > @@ -973,6 +973,10 @@ ExecuteSmmCoreFromSmram (
> >        // Since the memory range to load SMM CORE will be cut out in SMM
> core, so no need to allocate and free this range
> >        //
> >        PageCount = 0;
> > +      //
> > +      // Reserved Smram Region for SmmCore is not used, and remove it
> from SmramRangeCount.
> > +      //
> > +      gSmmCorePrivate->SmramRangeCount --;
> >      } else {
> >        DEBUG ((EFI_D_INFO, "LOADING MODULE FIXED ERROR: Loading
> module at fixed address at address failed\n"));
> >        //
> > @@ -1299,6 +1303,7 @@ GetFullSmramRanges (
> >    UINTN                             Index2;
> >    EFI_SMRAM_DESCRIPTOR              *FullSmramRanges;
> >    UINTN                             TempSmramRangeCount;
> > +  UINTN                             AdditionSmramRangeCount;
> >    EFI_SMRAM_DESCRIPTOR              *TempSmramRanges;
> >    UINTN                             SmramRangeCount;
> >    EFI_SMRAM_DESCRIPTOR              *SmramRanges;
> > @@ -1332,12 +1337,23 @@ GetFullSmramRanges (
> >      }
> >    }
> >
> > +  //
> > +  // Reserve one entry for SMM Core in the full SMRAM ranges.
> 
> You have moved comment "Reserve one entry for SMM Core in the full
> SMRAM
> ranges." to here.
> 
> > +  //
> > +  AdditionSmramRangeCount = 1;
> > +  if (PcdGet64(PcdLoadModuleAtFixAddressEnable) != 0) {
> > +    //
> > +    // Reserve two entries for all SMM drivers and SMM Core in the full
> SMRAM ranges.
> > +    //
> > +    AdditionSmramRangeCount = 2;
> > +  }
> > +
> >    if (SmramReservedCount == 0) {
> >      //
> >      // No reserved SMRAM entry from SMM Configuration Protocol.
> >      // Reserve one entry for SMM Core in the full SMRAM ranges.
> 
> Since you have move comment "Reserve one entry for SMM Core in the full
> SMRAM ranges." to the code above, how about remove it in this comments?
> 
> Thanks,
> Star
> 
> >      //
> > -    *FullSmramRangeCount = SmramRangeCount + 1;
> > +    *FullSmramRangeCount = SmramRangeCount +
> AdditionSmramRangeCount;
> >      Size = (*FullSmramRangeCount) * sizeof (EFI_SMRAM_DESCRIPTOR);
> >      FullSmramRanges = (EFI_SMRAM_DESCRIPTOR *) AllocateZeroPool
> (Size);
> >      ASSERT (FullSmramRanges != NULL);
> > @@ -1449,10 +1465,9 @@ GetFullSmramRanges (
> >    ASSERT (TempSmramRangeCount <= MaxCount);
> >
> >    //
> > -  // Sort the entries,
> > -  // and reserve one entry for SMM Core in the full SMRAM ranges.
> > +  // Sort the entries
> >    //
> > -  FullSmramRanges = AllocateZeroPool ((TempSmramRangeCount + 1) *
> sizeof (EFI_SMRAM_DESCRIPTOR));
> > +  FullSmramRanges = AllocateZeroPool ((TempSmramRangeCount +
> AdditionSmramRangeCount) * sizeof (EFI_SMRAM_DESCRIPTOR));
> >    ASSERT (FullSmramRanges != NULL);
> >    *FullSmramRangeCount = 0;
> >    do {
> > @@ -1472,7 +1487,7 @@ GetFullSmramRanges (
> >      TempSmramRanges[Index].PhysicalSize = 0;
> >    } while (*FullSmramRangeCount < TempSmramRangeCount);
> >    ASSERT (*FullSmramRangeCount == TempSmramRangeCount);
> > -  *FullSmramRangeCount += 1;
> > +  *FullSmramRangeCount += AdditionSmramRangeCount;
> >
> >    FreePool (SmramRanges);
> >    FreePool (SmramReservedRanges);
> > @@ -1510,6 +1525,7 @@ SmmIplEntry (
> >    EFI_LOAD_FIXED_ADDRESS_CONFIGURATION_TABLE
> *LMFAConfigurationTable;
> >    EFI_CPU_ARCH_PROTOCOL           *CpuArch;
> >    EFI_STATUS                      SetAttrStatus;
> > +  EFI_SMRAM_DESCRIPTOR            *SmramRangeSmmDriver;
> >
> >    //
> >    // Fill in the image handle of the SMM IPL so the SMM Core can use this as
> the
> > @@ -1619,6 +1635,19 @@ SmmIplEntry (
> >          //
> >          DEBUG ((EFI_D_INFO, "LOADING MODULE FIXED INFO: TSEG BASE
> is %x. \n", LMFAConfigurationTable->SmramBase));
> >        }
> > +
> > +      //
> > +      // Fill the Smram range for all SMM code
> > +      //
> > +      SmramRangeSmmDriver = &gSmmCorePrivate-
> >SmramRanges[gSmmCorePrivate->SmramRangeCount - 2];
> > +      SmramRangeSmmDriver->CpuStart      = mCurrentSmramRange-
> >CpuStart;
> > +      SmramRangeSmmDriver->PhysicalStart = mCurrentSmramRange-
> >PhysicalStart;
> > +      SmramRangeSmmDriver->RegionState   = mCurrentSmramRange-
> >RegionState | EFI_ALLOCATED;
> > +      SmramRangeSmmDriver->PhysicalSize  = SmmCodeSize;
> > +
> > +      mCurrentSmramRange->PhysicalSize  -= SmmCodeSize;
> > +      mCurrentSmramRange->CpuStart       = mCurrentSmramRange-
> >CpuStart + SmmCodeSize;
> > +      mCurrentSmramRange->PhysicalStart  = mCurrentSmramRange-
> >PhysicalStart + SmmCodeSize;
> >      }
> >      //
> >      // Load SMM Core into SMRAM and execute it from SMRAM
> >