Re: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities

[Laszlo Ersek <lersek@redhat.com>]

On 11/16/17 08:26, Jian J Wang wrote:
> Some OSs will treat EFI_MEMORY_DESCRIPTOR.Attribute as really
> set attributes and change memory paging attribute accordingly.
> But current EFI_MEMORY_DESCRIPTOR.Attribute is assigned by
> value from Capabilities in GCD memory map. This might cause
> boot problems. Clearing all paging related capabilities can
> workaround it. The code added in this patch is supposed to
> be removed once the usage of EFI_MEMORY_DESCRIPTOR.Attribute
> is clarified in UEFI spec and adopted by both EDK-II Core and
> all supported OSs.
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/Mem/Page.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index c9219cc068..783b576e35 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -1829,6 +1829,23 @@ CoreGetMemoryMap (
>    //
>    BufferSize = ((UINT8 *)MemoryMap - (UINT8 *)MemoryMapStart);
>  
> +  //
> +  // WORKAROUND: Some OSs will treat EFI_MEMORY_DESCRIPTOR.Attribute as really
> +  //             set attributes and change memory paging attribute accordingly.
> +  //             But current EFI_MEMORY_DESCRIPTOR.Attribute is assigned by
> +  //             value from Capabilities in GCD memory map. This might cause
> +  //             boot problems. Clearing all paging related capabilities can
> +  //             workaround it. Following code is supposed to be removed once
> +  //             the usage of EFI_MEMORY_DESCRIPTOR.Attribute is clarified in
> +  //             UEFI spec and adopted by both EDK-II Core and all supported
> +  //             OSs.
> +  //
> +  while (MemoryMapStart < MemoryMap) {
> +    MemoryMapStart->Attribute &= ~(UINT64)(EFI_MEMORY_RP | EFI_MEMORY_RO |
> +                                           EFI_MEMORY_XP);
> +    MemoryMapStart = NEXT_MEMORY_DESCRIPTOR(MemoryMapStart, Size);
> +  }
> +
>    Status = EFI_SUCCESS;
>  
>  Done:
> 

OK, let me check if I understand the discussion thus far, for this patch:

- Ard asked about EFI_MEMORY_WP, but clearing that bit is not necessary
because CpuDxe does not add it anyway, in the GCD memory space map.

- The code comment might be updated (according to Jiewen's suggestion)
before pushing the patch. I don't have any particular opinion about the
comment.

- If I understand correctly, Jiewen agrees with applying both patches in
this series.


I have one superficial comment on this patch: in the CoreGetMemoryMap()
function, we keep "MemoryMapStart" unchanged (after the initial
assignment), and keep incrementing "MemoryMap". At the location where
the new code is being added, "MemoryMap" points one past the last
descriptor, and "MemoryMapStart" still points to the first one.

In order to keep this property for possible future "scans" of the full
map, I would prefer keeping "MemoryMapStart" unchanged in this location,
and using an independent loop variable.

... On the other hand, we can easily restore "MemoryMapStart", should it
be necessary, with the help of "BufferSize". So, I guess the function
does not become more difficult to work with after this patch.

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

(I hope that Star and/or Jiewen will also R-b this patch, possibly with
the updated code comment.)

Thanks,
Laszlo