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....@intel.com> > Cc: Star Zeng <star.z...@intel.com> > Cc: Laszlo Ersek <ler...@redhat.com> > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang <jian.j.w...@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 <ler...@redhat.com> (I hope that Star and/or Jiewen will also R-b this patch, possibly with the updated code comment.) Thanks, Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel