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

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

1) Sure. I'll update the wording.
2) I still think this is just a workaround . In the long run, I don't think the merge is a good idea. It looks like it will "fix" more issues, but actually it just "hide" them and would cause more and more workaround in the future. Anyway, if no one else has objections, I'll update the code.

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Friday, November 17, 2017 9:37 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard
> Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: RE: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging
> capabilities
> 
> HI
> I have 2 comments:
> 
> 1) I do not think we need mention: WORKAROUND.
> I suggest we just use "NOTE".
> 
> We have similar example before, see
> MdePkg\Library\BasePeCoffLib\BasePeCoff.c
>   //
>   // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic
> value
>   //       in the PE/COFF Header.  If the MachineType is Itanium(IA64) and the
>   //       Magic value in the OptionalHeader is
> EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
>   //       then override the returned value to
> EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
>   //
> 
> 2) I agree with Star. I think we should merge the final result.
> The suggestion before is: *Keep current UEFI memory map unchanged.*
> Changing it brings lots of risk without validating all UEFI OS.
> 
> 
> Thank you
> Yao Jiewen
> 
> 
> > -----Original Message-----
> > From: Wang, Jian J
> > Sent: Thursday, November 16, 2017 3:27 PM
> > To: edk2-devel@lists.01.org
> > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>;
> > Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> > Subject: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging
> capabilities
> >
> > 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:
> > --
> > 2.14.1.windows.1