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

["Zeng, Star" <star.zeng@intel.com>]

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
>