Re: [PATCH v3] IntelFrameworkModulePkg/Csm: Set CSM memory executable

["Wang, Jian J" <jian.j.wang@intel.com>]

Hi Ruiyu,

In function description comments, the @retval doesn't match the changes in this patch.

With those changes,
    Reviewed-by: Jian J Wang <jian.j.wang@intel.com>

Regards,
Jian

> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Wednesday, August 08, 2018 1:28 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> Wang, Jian J <jian.j.wang@intel.com>
> Subject: [PATCH v3] IntelFrameworkModulePkg/Csm: Set CSM memory
> executable
> 
> Commit b22a62be5cdc8fd19d87ec1ecfa5b28fb9be50ad
> * IntelFrameworkModule/LegacyBios:Use reserved memory for legacy data
> allocates reserved memory for holding legacy code/data.
> 
> But with PcdDxeNxMemoryProtectionPolicy set to certain value to
> forbid execution when code is in certain type of memory, it's
> possible that a platform forbids execution when code is in reserved
> memory. The patch calls GCD service to allow such case otherwise
> CPU exception may occur.
> 
> Code execution in BSCode area should be enabled by platform by
> default.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jian Wang <jian.j.wang@intel.com>
> ---
>  .../Csm/LegacyBiosDxe/LegacyBios.c                 | 28 ++++++++++++++++++----
>  1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
> b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
> index 8f14687b28..80efe40489 100644
> --- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
> +++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
> @@ -64,8 +64,9 @@ AllocateLegacyMemory (
>    OUT EFI_PHYSICAL_ADDRESS      *Result
>    )
>  {
> -  EFI_STATUS            Status;
> -  EFI_PHYSICAL_ADDRESS  MemPage;
> +  EFI_STATUS                      Status;
> +  EFI_PHYSICAL_ADDRESS            MemPage;
> +  EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc;
> 
>    //
>    // Allocate Pages of memory less <= StartPageAddress
> @@ -81,12 +82,29 @@ AllocateLegacyMemory (
>    // Do not ASSERT on Status error but let caller decide since some cases
>    // memory is already taken but that is ok.
>    //
> +  if (!EFI_ERROR (Status)) {
> +    if (MemoryType != EfiBootServicesCode) {
> +      //
> +      // Make sure that the buffer can be used to store code.
> +      //
> +      Status = gDS->GetMemorySpaceDescriptor (MemPage, &MemDesc);
> +      if (!EFI_ERROR (Status) && (MemDesc.Attributes & EFI_MEMORY_XP) != 0) {
> +        Status = gDS->SetMemorySpaceAttributes (
> +                        MemPage,
> +                        EFI_PAGES_TO_SIZE (Pages),
> +                        MemDesc.Attributes & (~EFI_MEMORY_XP)
> +                        );
> +      }
> +      if (EFI_ERROR (Status)) {
> +        gBS->FreePages (MemPage, Pages);
> +      }
> +    }
> +  }
> +
>    if (!EFI_ERROR (Status)) {
>      *Result = (EFI_PHYSICAL_ADDRESS) (UINTN) MemPage;
>    }
> -  //
> -  // If reach here the status = EFI_SUCCESS
> -  //
> +
>    return Status;
>  }
> 
> --
> 2.16.1.windows.1