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

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

The function description comments should be not related to this patch.
I am ok with or without updating the comments in this patch.
Reviewed-by: Star Zeng <star.zeng@intel.com>.

Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Wednesday, August 8, 2018 7:06 PM
To: Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>
Subject: Re: [PATCH v3] IntelFrameworkModulePkg/Csm: Set CSM memory executable

On 08/08/18 09:04, Wang, Jian J wrote:
> 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>

Same from my side,

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

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