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

2017-11-21 Thread Zeng, Star 
Reviewed-by: Star Zeng  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 ; edk2-devel@lists.01.org
Cc: Zeng, Star ; Laszlo Ersek ; Ard 
Biesheuvel ; Kinney, Michael D 

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 ; edk2-devel@lists.01.org
> Cc: Zeng, Star ; Laszlo Ersek 
> ; Ard Biesheuvel 
> 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 ; Zeng, Star 
> > ; Laszlo Ersek ; Ard 
> > Biesheuvel
> 
> > 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 
> > Cc: Star Zeng 
> > Cc: Laszlo Ersek 
> > Cc: Ard Biesheuvel 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang 
> > ---
> >  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

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

2017-11-20 Thread Wang, Jian J 
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:ler...@redhat.com]
> Sent: Tuesday, November 21, 2017 4:23 AM
> To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen@intel.com>; Zeng, Star <star.z...@intel.com>;
> Ard Biesheuvel <ard.biesheu...@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@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

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

2017-11-20 Thread Laszlo Ersek 
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 
> Cc: Star Zeng 
> Cc: Laszlo Ersek 
> Cc: Ard Biesheuvel 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  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 

(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

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

2017-11-16 Thread Wang, Jian J 
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 ; edk2-devel@lists.01.org
> Cc: Zeng, Star ; Laszlo Ersek ; Ard
> Biesheuvel 
> 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 ; Zeng, Star ;
> > Laszlo Ersek ; Ard Biesheuvel
> 
> > 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 
> > Cc: Star Zeng 
> > Cc: Laszlo Ersek 
> > Cc: Ard Biesheuvel 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang 
> > ---
> >  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

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

2017-11-16 Thread Yao, Jiewen 
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 ; Zeng, Star ;
> Laszlo Ersek ; Ard Biesheuvel 
> 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 
> Cc: Star Zeng 
> Cc: Laszlo Ersek 
> Cc: Ard Biesheuvel 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  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

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

2017-11-16 Thread Ard Biesheuvel 
On 16 November 2017 at 09:48, Zeng, Star <star.z...@intel.com> wrote:
> As I remember UEFI 2.5 clarified this and added EFI_MEMORY_RO was because 
> EFI_MEMORY_WP had been typically used for cache even before UEFI 2.5.
>
> And I do not think this patch should filter out EFI_MEMORY_WP since this 
> patch is to filter out new paging bits caused by 
> 14dde9e903bb9a719ebb8f3381da72b19509bc36 [MdeModulePkg/Core: Fix out-of-sync 
> issue in GCD].
>

Ah ok. If the bit does not get copied from the GCD map then there is
no need to filter it here.

>
> Thanks,
> Star
> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Thursday, November 16, 2017 5:30 PM
> To: Zeng, Star <star.z...@intel.com>
> Cc: Wang, Jian J <jian.j.w...@intel.com>; Laszlo Ersek <ler...@redhat.com>; 
> edk2-devel@lists.01.org; Yao, Jiewen <jiewen....@intel.com>
> Subject: Re: [edk2] [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all 
> paging capabilities
>
> On 16 November 2017 at 09:28, Zeng, Star <star.z...@intel.com> wrote:
>> Ard,
>>
>> EFI_MEMORY_WP is for cache.
>>
>> UefiSpec.h
>> //
>> // Note: UEFI spec 2.5 and following: use EFI_MEMORY_RO as
>> write-protected physical memory // protection attribute. Also, EFI_MEMORY_WP 
>> means cacheability attribute.
>> //
>> #define EFI_MEMORY_WP   0x1000ULL
>>
>
> Yes, but that was a change in v2.5, before that it was a permission 
> attribute. So it should be filtered from the OS visible memory map as well.
>
>> Thanks,
>> Star
>> -Original Message-
>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>> Sent: Thursday, November 16, 2017 5:25 PM
>> To: Wang, Jian J <jian.j.w...@intel.com>
>> Cc: edk2-devel@lists.01.org; Yao, Jiewen <jiewen@intel.com>; Zeng,
>> Star <star.z...@intel.com>; Laszlo Ersek <ler...@redhat.com>
>> Subject: Re: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all
>> paging capabilities
>>
>> On 16 November 2017 at 07:26, Jian J Wang <jian.j.w...@intel.com> 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);
>>
>> Why is EFI_MEMORY_WP missing here?
>>
>>> +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
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2017-11-16 Thread Zeng, Star 
As I remember UEFI 2.5 clarified this and added EFI_MEMORY_RO was because 
EFI_MEMORY_WP had been typically used for cache even before UEFI 2.5.

And I do not think this patch should filter out EFI_MEMORY_WP since this patch 
is to filter out new paging bits caused by 
14dde9e903bb9a719ebb8f3381da72b19509bc36 [MdeModulePkg/Core: Fix out-of-sync 
issue in GCD].


Thanks,
Star
-Original Message-
From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] 
Sent: Thursday, November 16, 2017 5:30 PM
To: Zeng, Star <star.z...@intel.com>
Cc: Wang, Jian J <jian.j.w...@intel.com>; Laszlo Ersek <ler...@redhat.com>; 
edk2-devel@lists.01.org; Yao, Jiewen <jiewen@intel.com>
Subject: Re: [edk2] [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging 
capabilities

On 16 November 2017 at 09:28, Zeng, Star <star.z...@intel.com> wrote:
> Ard,
>
> EFI_MEMORY_WP is for cache.
>
> UefiSpec.h
> //
> // Note: UEFI spec 2.5 and following: use EFI_MEMORY_RO as 
> write-protected physical memory // protection attribute. Also, EFI_MEMORY_WP 
> means cacheability attribute.
> //
> #define EFI_MEMORY_WP   0x1000ULL
>

Yes, but that was a change in v2.5, before that it was a permission attribute. 
So it should be filtered from the OS visible memory map as well.

> Thanks,
> Star
> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Thursday, November 16, 2017 5:25 PM
> To: Wang, Jian J <jian.j.w...@intel.com>
> Cc: edk2-devel@lists.01.org; Yao, Jiewen <jiewen@intel.com>; Zeng, 
> Star <star.z...@intel.com>; Laszlo Ersek <ler...@redhat.com>
> Subject: Re: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all 
> paging capabilities
>
> On 16 November 2017 at 07:26, Jian J Wang <jian.j.w...@intel.com> 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);
>
> Why is EFI_MEMORY_WP missing here?
>
>> +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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2017-11-16 Thread Ard Biesheuvel 
On 16 November 2017 at 09:28, Zeng, Star  wrote:
> Ard,
>
> EFI_MEMORY_WP is for cache.
>
> UefiSpec.h
> //
> // Note: UEFI spec 2.5 and following: use EFI_MEMORY_RO as write-protected 
> physical memory
> // protection attribute. Also, EFI_MEMORY_WP means cacheability attribute.
> //
> #define EFI_MEMORY_WP   0x1000ULL
>

Yes, but that was a change in v2.5, before that it was a permission
attribute. So it should be filtered from the OS visible memory map as
well.

> Thanks,
> Star
> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Thursday, November 16, 2017 5:25 PM
> To: Wang, Jian J 
> Cc: edk2-devel@lists.01.org; Yao, Jiewen ; Zeng, Star 
> ; Laszlo Ersek 
> Subject: Re: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging 
> capabilities
>
> On 16 November 2017 at 07: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 
>> Cc: Star Zeng 
>> Cc: Laszlo Ersek 
>> Cc: Ard Biesheuvel 
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Jian J Wang 
>> ---
>>  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);
>
> Why is EFI_MEMORY_WP missing here?
>
>> +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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2017-11-16 Thread Zeng, Star 
Ard,

EFI_MEMORY_WP is for cache.

UefiSpec.h
//
// Note: UEFI spec 2.5 and following: use EFI_MEMORY_RO as write-protected 
physical memory
// protection attribute. Also, EFI_MEMORY_WP means cacheability attribute.
//
#define EFI_MEMORY_WP   0x1000ULL

Thanks,
Star
-Original Message-
From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] 
Sent: Thursday, November 16, 2017 5:25 PM
To: Wang, Jian J 
Cc: edk2-devel@lists.01.org; Yao, Jiewen ; Zeng, Star 
; Laszlo Ersek 
Subject: Re: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging 
capabilities

On 16 November 2017 at 07: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 
> Cc: Star Zeng 
> Cc: Laszlo Ersek 
> Cc: Ard Biesheuvel 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  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);

Why is EFI_MEMORY_WP missing here?

> +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

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

2017-11-16 Thread Ard Biesheuvel 
On 16 November 2017 at 07: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 
> Cc: Star Zeng 
> Cc: Laszlo Ersek 
> Cc: Ard Biesheuvel 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  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);

Why is EFI_MEMORY_WP missing here?

> +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