Reviewed-by: Star Zeng <star.z...@intel.com> if the feedback from Jiewen (about
comments) and Laszlo (about MemoryMapStart) has been addressed, and the merging
will be done in a separated patch.
Thanks,
Star
-----Original Message-----
From: Wang, Jian J
Sent: Friday, November 17, 2017 10:49 AM
To: Yao, Jiewen <jiewen....@intel.com>; edk2-devel@lists.01.org
Cc: Zeng, Star <star.z...@intel.com>; Laszlo Ersek <ler...@redhat.com>; Ard
Biesheuvel <ard.biesheu...@linaro.org>; Kinney, Michael D
<michael.d.kin...@intel.com>
Subject: RE: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging
capabilities
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.w...@intel.com>; edk2-devel@lists.01.org
> Cc: Zeng, Star <star.z...@intel.com>; Laszlo Ersek
> <ler...@redhat.com>; Ard Biesheuvel <ard.biesheu...@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....@intel.com>; Zeng, Star
> > <star.z...@intel.com>; Laszlo Ersek <ler...@redhat.com>; Ard
> > Biesheuvel
> <ard.biesheu...@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....@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:
> > --
> > 2.14.1.windows.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
