Hi Ray,
On 08/07/18 07:58, Ruiyu Ni wrote:
> 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...@intel.com>
> Cc: Star Zeng <star.z...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> ---
> .../Csm/LegacyBiosDxe/LegacyBios.c | 22
> +++++++++++++++++-----
> 1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
> b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
> index 8f14687b28..f86d00b53f 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
> @@ -83,10 +84,21 @@ AllocateLegacyMemory (
> //
> if (!EFI_ERROR (Status)) {
> *Result = (EFI_PHYSICAL_ADDRESS) (UINTN) MemPage;
> + 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 reach here the status = EFI_SUCCESS
> - //
> +
> return Status;
> }
>
>
thanks for the update. I think the commit message is now *great*.
I'm adding Jian (like Star did for v1) so he can comment on the general
idea.
I have another remark regarding the logic -- and I apologize for missing
it in v1 --: if the SetMemorySpaceAttributes() calls fails, we correctly
propagate the status to the caller; however, we leak the allocated
memory. If SetMemorySpaceAttributes() fails, we should release the
memory before we return the error.
Can you please CC Jian on v3 at once?
Thank you!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
