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

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

Thanks for the comments. V7 patch has added one more variable so that
MemoryMapStart will be kept intact.

V7 added code to merge memory map after filtering. I have verified that
the output keeps the same as v6 on both OVMF and our real platform
(default platform configuration). I think it may be not necessary to validate
all OS boot again. But if you want and have time, you can do it anyway.

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, November 21, 2017 4:23 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>;
> Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all
> paging capabilities
> 
> 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