Re: [edk2] [PATCH] ShellPkg/pci: Report error when invalid value is specified for "-ec"

2017-02-10 Thread Ni, Ruiyu 
Jaben,
"-ec" should be followed by a ID.

The patch fixes when "-ec" is followed by string other than ID, no error is 
reported.

Regards,
Ray

>-Original Message-
>From: Carsey, Jaben
>Sent: Saturday, February 11, 2017 1:06 AM
>To: Ni, Ruiyu ; edk2-devel@lists.01.org
>Cc: Carsey, Jaben 
>Subject: RE: [PATCH] ShellPkg/pci: Report error when invalid value is 
>specified for "-ec"
>
>Reviewed-by: Jaben Carsey 
>
>Ray,
>
>What is the intended behavior if the user does -ec with no data after it?
>
>-Jaben
>
>
>> -Original Message-
>> From: Ni, Ruiyu
>> Sent: Friday, February 10, 2017 12:24 AM
>> To: edk2-devel@lists.01.org
>> Cc: Carsey, Jaben 
>> Subject: [PATCH] ShellPkg/pci: Report error when invalid value is specified
>> for "-ec"
>> Importance: High
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ruiyu Ni 
>> Cc: Jaben Carsey 
>> ---
>>  ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c   | 21
>> +++--
>>  .../UefiShellDebug1CommandsLib.uni  |  2 +-
>>  2 files changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
>> b/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
>> index fb7561f..37f15d6 100644
>> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
>> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
>> @@ -2726,6 +2726,7 @@ ShellCommandRunPci (
>>  Bus   = 0;
>>  Device= 0;
>>  Func  = 0;
>> +EnhancedDump  = 0x;
>>  if (ShellCommandLineGetFlag(Package, L"-i")) {
>>ExplainData = TRUE;
>>  }
>> @@ -2807,6 +2808,20 @@ ShellCommandRunPci (
>>}
>>  }
>>
>> +Temp = ShellCommandLineGetValue (Package, L"-ec");
>> +if (Temp != NULL) {
>> +  //
>> +  // Input converted to hexadecimal number.
>> +  //
>> +  if (!EFI_ERROR (ShellConvertStringToUint64 (Temp, , TRUE,
>> TRUE))) {
>> +EnhancedDump = (UINT16) RetVal;
>> +  } else {
>> +ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN
>> (STR_GEN_PARAM_INV_HEX), gShellDebug1HiiHandle, L"pci", Temp);
>> +ShellStatus = SHELL_INVALID_PARAMETER;
>> +goto Done;
>> +  }
>> +}
>> +
>>  //
>>  // Find the protocol interface who's in charge of current segment, and 
>> its
>>  // bus range covers the current bus
>> @@ -2883,12 +2898,6 @@ ShellCommandRunPci (
>>  // If "-i" appears in command line, interpret data in configuration 
>> space
>>  //
>>  if (ExplainData) {
>> -  EnhancedDump = 0x;
>> -  if (ShellCommandLineGetFlag(Package, L"-ec")) {
>> -Temp = ShellCommandLineGetValue(Package, L"-ec");
>> -ASSERT (Temp != NULL);
>> -EnhancedDump = (UINT16) ShellHexStrToUintn (Temp);
>> -  }
>>Status = PciExplainData (, Address, IoDev, EnhancedDump);
>>  }
>>}
>> diff --git
>> a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman
>> dsLib.uni
>> b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman
>> dsLib.uni
>> index 8ea4215..7c0ca98 100644
>> ---
>> a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman
>> dsLib.uni
>> +++
>> b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman
>> dsLib.uni
>> @@ -39,7 +39,7 @@
>>  #string STR_GEN_PCIRBIO_NF#language en-US "%H%s%N: Protocol -
>> PciRootBridgeIo not found.\r\n"
>>  #string STR_GEN_PCIRBIO_ER#language en-US "%H%s%N: Problem
>> accessing the data using Protocol - PciRootBridgeIo\r\n"
>>  #string STR_GEN_PARAM_INV #language en-US "%H%s%N: Invalid
>> argument - '%H%s%N'\r\n"
>> -#string STR_GEN_PARAM_INV_HEX #language en-US "%H%s%N: Invalid
>> parameter - '%H%s%N:'.  Must be hexadecimal.\r\n"
>> +#string STR_GEN_PARAM_INV_HEX #language en-US "%H%s%N: Invalid
>> parameter - '%H%s%N'. Must be hexadecimal.\r\n"
>>  #string STR_GEN_PARAM_CONFLICT#language en-US "%H%s%N: Flags
>> conflict with - '%H%s%N' and '%H%s%N'\r\n"
>>  #string STR_GEN_OUT_MEM   #language en-US "%H%s%N: Memory
>> allocation was not successful.\r\n"
>>  #string STR_GEN_MAP_PROTOCOL  #language en-US "%H%s%N: Mapped
>> device '%B%s%N' does not have protocol %B%s%N\r\n"
>> --
>> 2.9.0.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 1/4] ArmPkg/CpuDxe: Correct EFI_MEMORY_RO usage

2017-02-10 Thread Leif Lindholm 
On Fri, Feb 10, 2017 at 06:25:00PM +, Ard Biesheuvel wrote:
> On 10 February 2017 at 18:17, Leif Lindholm  wrote:
> > On Thu, Feb 09, 2017 at 05:38:08PM +, Ard Biesheuvel wrote:
> >> From: Jiewen Yao 
> >>
> >> Current Arm CpuDxe driver uses EFI_MEMORY_WP for write protection,
> >> according to UEFI spec, we should use EFI_MEMORY_RO for write protection.
> >> The EFI_MEMORY_WP is the cache attribute instead of memory attribute.
> >>
> >> Cc: Leif Lindholm 
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Jiewen Yao 
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Ard Biesheuvel 
> >
> > No objections to this patch, but I would have expected it to be 4/4,
> > if it caused issues requiring the other 3 to be created?
> >
> 
> Not quite: it is the feature itself that requires these fixes, and
> this patch actually makes sense as 1/4, since it removes uses of
> EFI_MEMORY_WP that are no longer appropriate. Implementing 2-4 with
> EFI_MEMORY_WP instead of EFI_MEMORY_RO and then changing it at the end
> would make no sense at all.

OK, so basically, the issue was already in the existing code?

In that case:
Reviewed-by: Leif Lindholm 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 1/4] ArmPkg/CpuDxe: Correct EFI_MEMORY_RO usage

2017-02-10 Thread Ard Biesheuvel 
On 10 February 2017 at 18:17, Leif Lindholm  wrote:
> On Thu, Feb 09, 2017 at 05:38:08PM +, Ard Biesheuvel wrote:
>> From: Jiewen Yao 
>>
>> Current Arm CpuDxe driver uses EFI_MEMORY_WP for write protection,
>> according to UEFI spec, we should use EFI_MEMORY_RO for write protection.
>> The EFI_MEMORY_WP is the cache attribute instead of memory attribute.
>>
>> Cc: Leif Lindholm 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Jiewen Yao 
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel 
>
> No objections to this patch, but I would have expected it to be 4/4,
> if it caused issues requiring the other 3 to be created?
>

Not quite: it is the feature itself that requires these fixes, and
this patch actually makes sense as 1/4, since it removes uses of
EFI_MEMORY_WP that are no longer appropriate. Implementing 2-4 with
EFI_MEMORY_WP instead of EFI_MEMORY_RO and then changing it at the end
would make no sense at all.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 4/4] ArmPkg/ArmMmuLib: AARCH64: add support for modifying only permissions

2017-02-10 Thread Ard Biesheuvel 
On 10 February 2017 at 18:16, Leif Lindholm  wrote:
> On Thu, Feb 09, 2017 at 05:38:11PM +, Ard Biesheuvel wrote:
>> Since the new DXE page protection for PE/COFF images may invoke
>> EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() with only permission
>> attributes set, add support for this in the AARCH64 MMU code.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 73 +++-
>>  1 file changed, 56 insertions(+), 17 deletions(-)
>>
>> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c 
>> b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> index 6aa970bc0514..764e54b5d747 100644
>> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> @@ -28,6 +28,10 @@
>>  // We use this index definition to define an invalid block entry
>>  #define TT_ATTR_INDX_INVALID((UINT32)~0)
>>
>> +#define EFI_MEMORY_CACHETYPE_MASK   (EFI_MEMORY_UC | EFI_MEMORY_WC | \
>> + EFI_MEMORY_WT | EFI_MEMORY_WB | \
>> + EFI_MEMORY_UCE)
>> +
>
> This is already duplicated between
>
> ArmPkg/Drivers/CpuDxe/CpuDxe.h
> and
> UefiCpuPkg/CpuDxe/CpuDxe.h
>
> Can we avoid adding more?
>

Good point. Mind if I move it to ArmMmuLib.h? (and keep the UefiCpuPkg
one alone)

>>  STATIC
>>  UINT64
>>  ArmMemoryAttributeToPageAttribute (
>> @@ -101,25 +105,46 @@ PageAttributeToGcdAttribute (
>>return GcdAttributes;
>>  }
>>
>> -ARM_MEMORY_REGION_ATTRIBUTES
>> -GcdAttributeToArmAttribute (
>> +STATIC
>> +UINT64
>> +GcdAttributeToPageAttribute (
>>IN UINT64 GcdAttributes
>>)
>>  {
>> -  switch (GcdAttributes & 0xFF) {
>> +  UINT64 PageAttributes;
>> +
>> +  switch (GcdAttributes & EFI_MEMORY_CACHETYPE_MASK) {
>>case EFI_MEMORY_UC:
>> -return ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>> +PageAttributes = TT_ATTR_INDX_DEVICE_MEMORY;
>> +break;
>>case EFI_MEMORY_WC:
>> -return ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
>> +PageAttributes = TT_ATTR_INDX_MEMORY_NON_CACHEABLE;
>> +break;
>>case EFI_MEMORY_WT:
>> -return ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH;
>> +PageAttributes = TT_ATTR_INDX_MEMORY_WRITE_THROUGH | 
>> TT_SH_INNER_SHAREABLE;
>
> These TT_SH additions look like a bugfix - should they be a separate commit?
>

No, it's the diff that is confusing here: GcdAttributeToArmAttribute()
is removed completely, and replaced with
GcdAttributeToPageAttribute(). Due to the case labels, these line up,
but they are completely unrelated.

>> +break;
>>case EFI_MEMORY_WB:
>> -return ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
>> +PageAttributes = TT_ATTR_INDX_MEMORY_WRITE_BACK | TT_SH_INNER_SHAREABLE;
>> +break;
>>default:
>> -DEBUG ((EFI_D_ERROR, "GcdAttributeToArmAttribute: 0x%lX attributes is 
>> not supported.\n", GcdAttributes));
>> -ASSERT (0);
>> -return ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>> +PageAttributes = TT_ATTR_INDX_MASK;
>
> OK, so you're doing the same thing here as in the ARM code.
> This is a substantial change in behaviour (old behaviour: if unknown,
> set to DEVICE; new behaviour: if unknown, set "all types permitted").
> Am I missing something?
>

Again, completely different function

>> +break;
>>}
>> +
>> +  if ((GcdAttributes & EFI_MEMORY_XP) != 0 ||
>> +  (GcdAttributes & EFI_MEMORY_CACHETYPE_MASK) == EFI_MEMORY_UC) {
>> +if (ArmReadCurrentEL () == AARCH64_EL2) {
>> +  PageAttributes |= TT_XN_MASK;
>> +} else {
>> +  PageAttributes |= TT_UXN_MASK | TT_PXN_MASK;
>> +}
>> +  }
>> +
>> +  if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
>> +PageAttributes |= TT_AP_RO_RO;
>> +  }
>> +
>> +  return PageAttributes | TT_AF;
>>  }
>>
>>  #define MIN_T0SZ16
>> @@ -434,17 +459,31 @@ SetMemoryAttributes (
>>)
>>  {
>>RETURN_STATUSStatus;
>> -  ARM_MEMORY_REGION_DESCRIPTOR MemoryRegion;
>>UINT64  *TranslationTable;
>> -
>> -  MemoryRegion.PhysicalBase = BaseAddress;
>> -  MemoryRegion.VirtualBase = BaseAddress;
>> -  MemoryRegion.Length = Length;
>> -  MemoryRegion.Attributes = GcdAttributeToArmAttribute (Attributes);
>> +  UINT64   PageAttributes;
>> +  UINT64   PageAttributeMask;
>> +
>> +  PageAttributes = GcdAttributeToPageAttribute (Attributes);
>> +  PageAttributeMask = 0;
>> +
>> +  if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {
>> +//
>> +// No memory type was set in Attributes, so we are going to update the
>> +// permissions only.
>> +//
>> +PageAttributes &= TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK;
>> +PageAttributeMask = ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK |
>> +  TT_PXN_MASK | TT_XN_MASK);
>> +  }
>>
>>TranslationTable =

Re: [edk2] [PATCH 1/4] ArmPkg/CpuDxe: Correct EFI_MEMORY_RO usage

2017-02-10 Thread Leif Lindholm 
On Thu, Feb 09, 2017 at 05:38:08PM +, Ard Biesheuvel wrote:
> From: Jiewen Yao 
> 
> Current Arm CpuDxe driver uses EFI_MEMORY_WP for write protection,
> according to UEFI spec, we should use EFI_MEMORY_RO for write protection.
> The EFI_MEMORY_WP is the cache attribute instead of memory attribute.
> 
> Cc: Leif Lindholm 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao 
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 

No objections to this patch, but I would have expected it to be 4/4,
if it caused issues requiring the other 3 to be created?

/
Leif

> ---
>  ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c  |  3 ++-
>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c  | 14 ++
>  ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c |  5 +++--
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |  3 ++-
>  4 files changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c 
> b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> index d8bb41978066..15d5a8173233 100644
> --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> @@ -3,6 +3,7 @@
>  Copyright (c) 2009, Hewlett-Packard Company. All rights reserved.
>  Portions copyright (c) 2010, Apple Inc. All rights reserved.
>  Portions copyright (c) 2011-2013, ARM Ltd. All rights reserved.
> +Copyright (c) 2017, Intel Corporation. All rights reserved.
>  
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD 
> License
> @@ -224,7 +225,7 @@ EfiAttributeToArmAttribute (
>ArmAttributes |= TT_AF;
>  
>// Determine protection attributes
> -  if (EfiAttributes & EFI_MEMORY_WP) {
> +  if (EfiAttributes & EFI_MEMORY_RO) {
>  ArmAttributes |= TT_AP_RO_RO;
>}
>  
> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> index 14fc22d7a59f..6dcfba69e879 100644
> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> @@ -3,6 +3,7 @@
>  Copyright (c) 2009, Hewlett-Packard Company. All rights reserved.
>  Portions copyright (c) 2010, Apple Inc. All rights reserved.
>  Portions copyright (c) 2013, ARM Ltd. All rights reserved.
> +Copyright (c) 2017, Intel Corporation. All rights reserved.
>  
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD 
> License
> @@ -62,7 +63,7 @@ SectionToGcdAttributes (
>// determine protection attributes
>switch(SectionAttributes & TT_DESCRIPTOR_SECTION_AP_MASK) {
>  case TT_DESCRIPTOR_SECTION_AP_NO_NO: // no read, no write
> -  //*GcdAttributes |= EFI_MEMORY_WP | EFI_MEMORY_RP;
> +  //*GcdAttributes |= EFI_MEMORY_RO | EFI_MEMORY_RP;
>break;
>  
>  case TT_DESCRIPTOR_SECTION_AP_RW_NO:
> @@ -73,7 +74,7 @@ SectionToGcdAttributes (
>  // read only cases map to write-protect
>  case TT_DESCRIPTOR_SECTION_AP_RO_NO:
>  case TT_DESCRIPTOR_SECTION_AP_RO_RO:
> -  *GcdAttributes |= EFI_MEMORY_WP;
> +  *GcdAttributes |= EFI_MEMORY_RO;
>break;
>  
>  default:
> @@ -126,7 +127,7 @@ PageToGcdAttributes (
>// determine protection attributes
>switch(PageAttributes & TT_DESCRIPTOR_PAGE_AP_MASK) {
>  case TT_DESCRIPTOR_PAGE_AP_NO_NO: // no read, no write
> -  //*GcdAttributes |= EFI_MEMORY_WP | EFI_MEMORY_RP;
> +  //*GcdAttributes |= EFI_MEMORY_RO | EFI_MEMORY_RP;
>break;
>  
>  case TT_DESCRIPTOR_PAGE_AP_RW_NO:
> @@ -137,7 +138,7 @@ PageToGcdAttributes (
>  // read only cases map to write-protect
>  case TT_DESCRIPTOR_PAGE_AP_RO_NO:
>  case TT_DESCRIPTOR_PAGE_AP_RO_RO:
> -  *GcdAttributes |= EFI_MEMORY_WP;
> +  *GcdAttributes |= EFI_MEMORY_RO;
>break;
>  
>  default:
> @@ -730,9 +731,6 @@ EfiAttributeToArmAttribute (
>ArmAttributes = TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; 
> // TEX [2:0] = 001, C=1, B=1
>break;
>  
> -case EFI_MEMORY_WP:
> -case EFI_MEMORY_XP:
> -case EFI_MEMORY_RP:
>  case EFI_MEMORY_UCE:
>  default:
>// Cannot be implemented UEFI definition unclear for ARM
> @@ -743,7 +741,7 @@ EfiAttributeToArmAttribute (
>}
>  
>// Determine protection attributes
> -  if (EfiAttributes & EFI_MEMORY_WP) {
> +  if (EfiAttributes & EFI_MEMORY_RO) {
>  ArmAttributes |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
>} else {
>  ArmAttributes |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
> diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c 
> b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> index 723604d1df96..54d9b0163331 100644
> --- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> +++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> @@ -1,6 +1,7 @@
>  /** @file
>  *
>  *  Copyright (c) 2013, ARM Limited. All rights reserved.
> +*  Copyright (c) 2017, Intel

Re: [edk2] [PATCH 3/4] ArmPkg/CpuDxe: ARM: ignore page table updates that only change permissions

2017-02-10 Thread Leif Lindholm 
On Thu, Feb 09, 2017 at 05:38:10PM +, Ard Biesheuvel wrote:
> Currently, we have not implemented support on 32-bit ARM for managing
> permission bits in the page tables. Since the new DXE page protection
> for PE/COFF images may invoke EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes()
> with only permission attributes set, let's simply ignore those for now.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 

Reviewed-by: Leif Lindholm 

> ---
>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> index b6ba975b353a..89e429925ba9 100644
> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> @@ -680,6 +680,13 @@ SetMemoryAttributes (
>  {
>EFI_STATUSStatus;
>  
> +  //
> +  // Ignore invocations that only modify permission bits
> +  //
> +  if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {
> +return EFI_SUCCESS;
> +  }
> +
>if(((BaseAddress & 0xF) == 0) && ((Length & 0xF) == 0)) {
>  // Is the base and length a multiple of 1 MB?
>  DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU section 0x%x length 0x%x 
> to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes));
> -- 
> 2.7.4
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 2/4] ArmPkg/CpuDxe: translate invalid memory types in EfiAttributeToArmAttribute

2017-02-10 Thread Ard Biesheuvel 
On 10 February 2017 at 17:54, Leif Lindholm  wrote:
> On Thu, Feb 09, 2017 at 05:38:09PM +, Ard Biesheuvel wrote:
>> The single user of EfiAttributeToArmAttribute () is the protocol
>> method EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes(), which uses the
>> return value to compare against the ARM attributes of an existing mapping,
>> to infer whether it is actually necessary to change anything, or whether
>> the requested update is redundant. This saves some cache and TLB
>> maintenance on 32-bit ARM systems that use uncached translation tables.
>>
>> However, EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() may be invoked with
>> only permission bits set, in which case the implied requested action is to
>> update the permissions of the region without modifying the cacheability
>> attributes. This is currently not possible, because
>> EfiAttributeToArmAttribute () ASSERT()s [on AArch64] on Attributes arguments
>> that lack a cacheability bit.
>>
>> So let's simply return TT_ATTR_INDX_INVALID (AArch64) or
>> TT_DESCRIPTOR_SECTION_TYPE_FAULT (ARM) in these cases (or'ed with the
>> appropriate permission bits). This way, the return value is equally
>> suitable for checking whether the attributes need to be modified, but
>> in a way that accommodates the use without a cacheability bit set.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>  ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 4 +---
>>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 3 ---
>>  2 files changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c 
>> b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>> index 15d5a8173233..7688846e70cb 100644
>> --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>> +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>> @@ -216,9 +216,7 @@ EfiAttributeToArmAttribute (
>>  ArmAttributes = TT_ATTR_INDX_MEMORY_WRITE_BACK;
>>  break;
>>default:
>> -DEBUG ((EFI_D_ERROR, "EfiAttributeToArmAttribute: 0x%lX attributes is 
>> not supported.\n", EfiAttributes));
>> -ASSERT (0);
>> -ArmAttributes = TT_ATTR_INDX_DEVICE_MEMORY;
>> +ArmAttributes = TT_ATTR_INDX_MASK;
>
> Commit message says TT_ATTR_INDX_INVALID - which one is intended to be
> correct?
>

_MASK is the correct one. I will fix up the commit message when
applying (assuming there are no other concerns)

TT_ATTR_INDX_INVALID is named poorly, and cannnot be and'ed in a
meaningful way with other TT_ flags, so I stopped using it, but forgot
to update the log message
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 2/4] ArmPkg/CpuDxe: translate invalid memory types in EfiAttributeToArmAttribute

2017-02-10 Thread Leif Lindholm 
On Thu, Feb 09, 2017 at 05:38:09PM +, Ard Biesheuvel wrote:
> The single user of EfiAttributeToArmAttribute () is the protocol
> method EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes(), which uses the
> return value to compare against the ARM attributes of an existing mapping,
> to infer whether it is actually necessary to change anything, or whether
> the requested update is redundant. This saves some cache and TLB
> maintenance on 32-bit ARM systems that use uncached translation tables.
> 
> However, EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() may be invoked with
> only permission bits set, in which case the implied requested action is to
> update the permissions of the region without modifying the cacheability
> attributes. This is currently not possible, because
> EfiAttributeToArmAttribute () ASSERT()s [on AArch64] on Attributes arguments
> that lack a cacheability bit.
> 
> So let's simply return TT_ATTR_INDX_INVALID (AArch64) or
> TT_DESCRIPTOR_SECTION_TYPE_FAULT (ARM) in these cases (or'ed with the
> appropriate permission bits). This way, the return value is equally
> suitable for checking whether the attributes need to be modified, but
> in a way that accommodates the use without a cacheability bit set.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 4 +---
>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 3 ---
>  2 files changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c 
> b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> index 15d5a8173233..7688846e70cb 100644
> --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> @@ -216,9 +216,7 @@ EfiAttributeToArmAttribute (
>  ArmAttributes = TT_ATTR_INDX_MEMORY_WRITE_BACK;
>  break;
>default:
> -DEBUG ((EFI_D_ERROR, "EfiAttributeToArmAttribute: 0x%lX attributes is 
> not supported.\n", EfiAttributes));
> -ASSERT (0);
> -ArmAttributes = TT_ATTR_INDX_DEVICE_MEMORY;
> +ArmAttributes = TT_ATTR_INDX_MASK;

Commit message says TT_ATTR_INDX_INVALID - which one is intended to be
correct?

/
Leif

>}
>  
>// Set the access flag to match the block attributes
> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> index 6dcfba69e879..b6ba975b353a 100644
> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> @@ -733,10 +733,7 @@ EfiAttributeToArmAttribute (
>  
>  case EFI_MEMORY_UCE:
>  default:
> -  // Cannot be implemented UEFI definition unclear for ARM
> -  // Cause a page fault if these ranges are accessed.
>ArmAttributes = TT_DESCRIPTOR_SECTION_TYPE_FAULT;
> -  DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): Unsupported attribute %x 
> will page fault on access\n", EfiAttributes));
>break;
>}
>  
> -- 
> 2.7.4
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] ShellPkg/pci: Report error when invalid value is specified for "-ec"

2017-02-10 Thread Carsey, Jaben 
Reviewed-by: Jaben Carsey 

Ray,

What is the intended behavior if the user does -ec with no data after it?

-Jaben


> -Original Message-
> From: Ni, Ruiyu
> Sent: Friday, February 10, 2017 12:24 AM
> To: edk2-devel@lists.01.org
> Cc: Carsey, Jaben 
> Subject: [PATCH] ShellPkg/pci: Report error when invalid value is specified
> for "-ec"
> Importance: High
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni 
> Cc: Jaben Carsey 
> ---
>  ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c   | 21
> +++--
>  .../UefiShellDebug1CommandsLib.uni  |  2 +-
>  2 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> index fb7561f..37f15d6 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> @@ -2726,6 +2726,7 @@ ShellCommandRunPci (
>  Bus   = 0;
>  Device= 0;
>  Func  = 0;
> +EnhancedDump  = 0x;
>  if (ShellCommandLineGetFlag(Package, L"-i")) {
>ExplainData = TRUE;
>  }
> @@ -2807,6 +2808,20 @@ ShellCommandRunPci (
>}
>  }
> 
> +Temp = ShellCommandLineGetValue (Package, L"-ec");
> +if (Temp != NULL) {
> +  //
> +  // Input converted to hexadecimal number.
> +  //
> +  if (!EFI_ERROR (ShellConvertStringToUint64 (Temp, , TRUE,
> TRUE))) {
> +EnhancedDump = (UINT16) RetVal;
> +  } else {
> +ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN
> (STR_GEN_PARAM_INV_HEX), gShellDebug1HiiHandle, L"pci", Temp);
> +ShellStatus = SHELL_INVALID_PARAMETER;
> +goto Done;
> +  }
> +}
> +
>  //
>  // Find the protocol interface who's in charge of current segment, and 
> its
>  // bus range covers the current bus
> @@ -2883,12 +2898,6 @@ ShellCommandRunPci (
>  // If "-i" appears in command line, interpret data in configuration space
>  //
>  if (ExplainData) {
> -  EnhancedDump = 0x;
> -  if (ShellCommandLineGetFlag(Package, L"-ec")) {
> -Temp = ShellCommandLineGetValue(Package, L"-ec");
> -ASSERT (Temp != NULL);
> -EnhancedDump = (UINT16) ShellHexStrToUintn (Temp);
> -  }
>Status = PciExplainData (, Address, IoDev, EnhancedDump);
>  }
>}
> diff --git
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman
> dsLib.uni
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman
> dsLib.uni
> index 8ea4215..7c0ca98 100644
> ---
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman
> dsLib.uni
> +++
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman
> dsLib.uni
> @@ -39,7 +39,7 @@
>  #string STR_GEN_PCIRBIO_NF#language en-US "%H%s%N: Protocol -
> PciRootBridgeIo not found.\r\n"
>  #string STR_GEN_PCIRBIO_ER#language en-US "%H%s%N: Problem
> accessing the data using Protocol - PciRootBridgeIo\r\n"
>  #string STR_GEN_PARAM_INV #language en-US "%H%s%N: Invalid
> argument - '%H%s%N'\r\n"
> -#string STR_GEN_PARAM_INV_HEX #language en-US "%H%s%N: Invalid
> parameter - '%H%s%N:'.  Must be hexadecimal.\r\n"
> +#string STR_GEN_PARAM_INV_HEX #language en-US "%H%s%N: Invalid
> parameter - '%H%s%N'. Must be hexadecimal.\r\n"
>  #string STR_GEN_PARAM_CONFLICT#language en-US "%H%s%N: Flags
> conflict with - '%H%s%N' and '%H%s%N'\r\n"
>  #string STR_GEN_OUT_MEM   #language en-US "%H%s%N: Memory
> allocation was not successful.\r\n"
>  #string STR_GEN_MAP_PROTOCOL  #language en-US "%H%s%N: Mapped
> device '%B%s%N' does not have protocol %B%s%N\r\n"
> --
> 2.9.0.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH V3 0/4] DXE Memory Protection

2017-02-10 Thread Ard Biesheuvel 
On 10 February 2017 at 12:59, Yao, Jiewen  wrote:
> Thanks for the info.
>
>
>
> Mike/Vincent also mentioned that FW does not own page tables after
> ExitBootServices(), so the OS would have to relax NX protection of RT code
> pages across SVA.
>
> Or delay setting NX protections on RT code pages until after SVA.
>
>
>
> I agree with you that we can mark RT region to be RW.
>
> Here is the pseudo code I plan to put to CoreExitBootServices().
>
>
>
> ===
>
> VOID
>
> MemoryprotectionExitBootServicesCallback (
>
>   VOID
>
>   )
>
> {
>
>   EFI_RUNTIME_IMAGE_ENTRY   *RuntimeImage;
>
>   LIST_ENTRY*Link;
>
>
>
>   //
>
>   // We need remove the RT protection, because RT relocation need write code
> segment
>
>   // at SetVirtualAddressMap(). We cannot assume OS/Loader has taken over
> page table at that time.
>
>   //
>
>   // Firmware does not own page tables after ExitBootServices(), so the OS
> would
>
>   // have to relax protection of RT code pages across
> SetVirtualAddressMap(), or
>
>   // delay setting protections on RT code pages until after
> SetVirtualAddressMap().
>
>   // OS may set protection on RT based upon EFI_MEMORY_ATTRIBUTES_TABLE
> later.
>
>   //
>
>   if (mImageProtectionPolicy != 0) {
>
> for (Link = gRuntime->ImageHead.ForwardLink; Link !=
> >ImageHead; Link = Link->ForwardLink) {
>
>   RuntimeImage = BASE_CR (Link, EFI_RUNTIME_IMAGE_ENTRY, Link);
>
>   SetUefiImageMemoryAttributes ((UINT64)(UINTN)RuntimeImage->ImageBase,
> ALIGN_VALUE(RuntimeImage->ImageSize, EFI_PAGE_SIZE), 0);
>
> }
>
>   }
>
> }


This looks correct to me, and it is the only place where we can deal
with this situation.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH V3 0/4] DXE Memory Protection

2017-02-10 Thread Yao, Jiewen 
Thanks for the info.

Mike/Vincent also mentioned that FW does not own page tables after 
ExitBootServices(), so the OS would have to relax NX protection of RT code 
pages across SVA.
Or delay setting NX protections on RT code pages until after SVA.

I agree with you that we can mark RT region to be RW.
Here is the pseudo code I plan to put to CoreExitBootServices().

===
VOID
MemoryprotectionExitBootServicesCallback (
  VOID
  )
{
  EFI_RUNTIME_IMAGE_ENTRY   *RuntimeImage;
  LIST_ENTRY*Link;

  //
  // We need remove the RT protection, because RT relocation need write code 
segment
  // at SetVirtualAddressMap(). We cannot assume OS/Loader has taken over page 
table at that time.
  //
  // Firmware does not own page tables after ExitBootServices(), so the OS would
  // have to relax protection of RT code pages across SetVirtualAddressMap(), or
  // delay setting protections on RT code pages until after 
SetVirtualAddressMap().
  // OS may set protection on RT based upon EFI_MEMORY_ATTRIBUTES_TABLE later.
  //
  if (mImageProtectionPolicy != 0) {
for (Link = gRuntime->ImageHead.ForwardLink; Link != >ImageHead; 
Link = Link->ForwardLink) {
  RuntimeImage = BASE_CR (Link, EFI_RUNTIME_IMAGE_ENTRY, Link);
  SetUefiImageMemoryAttributes ((UINT64)(UINTN)RuntimeImage->ImageBase, 
ALIGN_VALUE(RuntimeImage->ImageSize, EFI_PAGE_SIZE), 0);
}
  }
}
===


From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
Sent: Friday, February 10, 2017 3:43 AM
To: Yao, Jiewen 
Cc: Tian, Feng ; edk2-devel@lists.01.org; Leif Lindholm 
; Kinney, Michael D ; 
Fan, Jeff ; Zeng, Star 
Subject: Re: [edk2] [PATCH V3 0/4] DXE Memory Protection


On 10 Feb 2017, at 11:32, Yao, Jiewen 
> wrote:
Good to learn that ARM/ARCH64 behavior. That is interesting.

Yes, if that is the case, we need figure out a way to make it work.

IMHO, “Undo the protection” directly is also risky.
Maybe the protection is used or setup by OS purposely before EBS (WoW. ☺). 
Unprotecting them in BIOS might break the OS expectation.


Would you please provide more info on ARM Linux? When the OSLoader or OS takes 
over the page table? After EBS?

On ARM, we call SetVirtualAddressMap directly after EBS, in the OS loader. The 
caches are disabled much later. It is the OS itself that reenables the MMU, and 
install the UEFI virtual mapping as per-process mapping, which is only live 
when a runtime services call is in progress.

Therefore, we use a low virtual mapping for runtime services, which may 
conflict with the 1:1 mapping, so we never map any uefi regions 1:1 under the 
os.

This implies that the virtual mapping we install is not yet live when we 
install it, and so the easiest way to do that is to install it immediately 
after ebs, when the firmware's 1:1 mapping is still live.




Thank you
Yao Jiewen

From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ard 
Biesheuvel
Sent: Thursday, February 9, 2017 10:42 PM
To: Yao, Jiewen >
Cc: Tian, Feng >; 
edk2-devel@lists.01.org; Leif Lindholm 
>; Kinney, Michael D 
>; Fan, Jeff 
>; Zeng, Star 
>
Subject: Re: [edk2] [PATCH V3 0/4] DXE Memory Protection



> On 10 Feb 2017, at 06:34, Ard Biesheuvel 
> > wrote:
>
>
>
>> On 10 Feb 2017, at 02:26, Yao, Jiewen 
>> > wrote:
>>
>> Very good question.
>>
>> 1)   Yes, I did test UEFI OS boot, which is mentioned in V1 summary:
>> ==
>> Tested OS: UEFI Win10, UEFI Ubuntu 16.04.
>> ==
>>
>> 2)   Star helps double confirm that OS already takes over the control of 
>> page table on SetVirtualAddressMap().
>> See below log on UEFI Win10.
>> ==
>> DXEIPL CR3 0x8814
>> RUNTIMEDXE CR3 0x1AB000
>> ==
>>
>
> Not on AArch64/ARM linux, and the spec does not mandate it, so we need to 
> deal with this imo
>

I think we should probably undo the protections for runtime drivers in EBS()


>> Thank you
>> Yao Jiewen
>>
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ard 
>> Biesheuvel
>> Sent: Thursday, February 9, 2017 8:48 AM
>> To: Yao, Jiewen >
>> Cc: Tian, Feng >; 
>> edk2-devel@lists.01.org; Leif Lindholm 
>> >; Kinney, Michael

Re: [edk2] [PATCH V3 0/4] DXE Memory Protection

2017-02-10 Thread Ard Biesheuvel 

> On 10 Feb 2017, at 11:32, Yao, Jiewen  wrote:
> 
> Good to learn that ARM/ARCH64 behavior. That is interesting.
>  
> Yes, if that is the case, we need figure out a way to make it work.
>  
> IMHO, “Undo the protection” directly is also risky.
> Maybe the protection is used or setup by OS purposely before EBS (WoW. J). 
> Unprotecting them in BIOS might break the OS expectation.
>  
>  
> Would you please provide more info on ARM Linux? When the OSLoader or OS 
> takes over the page table? After EBS?

On ARM, we call SetVirtualAddressMap directly after EBS, in the OS loader. The 
caches are disabled much later. It is the OS itself that reenables the MMU, and 
install the UEFI virtual mapping as per-process mapping, which is only live 
when a runtime services call is in progress.

Therefore, we use a low virtual mapping for runtime services, which may 
conflict with the 1:1 mapping, so we never map any uefi regions 1:1 under the 
os.

This implies that the virtual mapping we install is not yet live when we 
install it, and so the easiest way to do that is to install it immediately 
after ebs, when the firmware's 1:1 mapping is still live.



> Thank you
> Yao Jiewen
>  
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ard 
> Biesheuvel
> Sent: Thursday, February 9, 2017 10:42 PM
> To: Yao, Jiewen 
> Cc: Tian, Feng ; edk2-devel@lists.01.org; Leif Lindholm 
> ; Kinney, Michael D ; 
> Fan, Jeff ; Zeng, Star 
> Subject: Re: [edk2] [PATCH V3 0/4] DXE Memory Protection
>  
> 
> 
> > On 10 Feb 2017, at 06:34, Ard Biesheuvel  wrote:
> > 
> > 
> > 
> >> On 10 Feb 2017, at 02:26, Yao, Jiewen  wrote:
> >> 
> >> Very good question.
> >>  
> >> 1)   Yes, I did test UEFI OS boot, which is mentioned in V1 summary:
> >> ==
> >> Tested OS: UEFI Win10, UEFI Ubuntu 16.04.
> >> ==
> >>  
> >> 2)   Star helps double confirm that OS already takes over the control 
> >> of page table on SetVirtualAddressMap().
> >> See below log on UEFI Win10.
> >> ==
> >> DXEIPL CR3 0x8814
> >> RUNTIMEDXE CR3 0x1AB000
> >> ==
> >>  
> > 
> > Not on AArch64/ARM linux, and the spec does not mandate it, so we need to 
> > deal with this imo
> > 
> 
> I think we should probably undo the protections for runtime drivers in EBS()
> 
> 
> >> Thank you
> >> Yao Jiewen
> >>  
> >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ard 
> >> Biesheuvel
> >> Sent: Thursday, February 9, 2017 8:48 AM
> >> To: Yao, Jiewen 
> >> Cc: Tian, Feng ; edk2-devel@lists.01.org; Leif 
> >> Lindholm ; Kinney, Michael D 
> >> ; Fan, Jeff ; Zeng, Star 
> >> 
> >> Subject: Re: [edk2] [PATCH V3 0/4] DXE Memory Protection
> >>  
> >> On 9 February 2017 at 16:30, Ard Biesheuvel  
> >> wrote:
> >> > On 9 February 2017 at 16:29, Yao, Jiewen  wrote:
> >> >> Very good point.
> >> >>
> >> >> Can ARCH64 set 4K paging for 64K aligned runtime memory?
> >> >>
> >> >
> >> > UEFI always uses 4 KB, but the OS may use 64 KB, so to create the
> >> > virtual address map it needs the runtime regions to be 64 KB aligned.
> >> >
> >> >>
> >> >>
> >> >> If yes, how about we use
> >> >>
> >> >> “ImageRecord->ImageSize = ALIGN_VALUE(LoadedImage->ImageSize,
> >> >> EFI_PAGE_SIZE);”
> >> >>
> >> >
> >> 
> >> Another question: did you try SetVirtualAddressMap()? It looks like we
> >> need to lift read-only permissions to allow the runtime PE/COFF
> >> relocation to apply the fixups
> >> ___
> >> 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 V3 0/4] DXE Memory Protection

2017-02-10 Thread Yao, Jiewen 
Good to learn that ARM/ARCH64 behavior. That is interesting.

Yes, if that is the case, we need figure out a way to make it work.

IMHO, “Undo the protection” directly is also risky.
Maybe the protection is used or setup by OS purposely before EBS (WoW. ☺). 
Unprotecting them in BIOS might break the OS expectation.


Would you please provide more info on ARM Linux? When the OSLoader or OS takes 
over the page table? After EBS?

Thank you
Yao Jiewen

From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ard 
Biesheuvel
Sent: Thursday, February 9, 2017 10:42 PM
To: Yao, Jiewen 
Cc: Tian, Feng ; edk2-devel@lists.01.org; Leif Lindholm 
; Kinney, Michael D ; 
Fan, Jeff ; Zeng, Star 
Subject: Re: [edk2] [PATCH V3 0/4] DXE Memory Protection



> On 10 Feb 2017, at 06:34, Ard Biesheuvel 
> > wrote:
>
>
>
>> On 10 Feb 2017, at 02:26, Yao, Jiewen 
>> > wrote:
>>
>> Very good question.
>>
>> 1)   Yes, I did test UEFI OS boot, which is mentioned in V1 summary:
>> ==
>> Tested OS: UEFI Win10, UEFI Ubuntu 16.04.
>> ==
>>
>> 2)   Star helps double confirm that OS already takes over the control of 
>> page table on SetVirtualAddressMap().
>> See below log on UEFI Win10.
>> ==
>> DXEIPL CR3 0x8814
>> RUNTIMEDXE CR3 0x1AB000
>> ==
>>
>
> Not on AArch64/ARM linux, and the spec does not mandate it, so we need to 
> deal with this imo
>

I think we should probably undo the protections for runtime drivers in EBS()


>> Thank you
>> Yao Jiewen
>>
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ard 
>> Biesheuvel
>> Sent: Thursday, February 9, 2017 8:48 AM
>> To: Yao, Jiewen >
>> Cc: Tian, Feng >; 
>> edk2-devel@lists.01.org; Leif Lindholm 
>> >; Kinney, Michael 
>> D >; Fan, Jeff 
>> >; Zeng, Star 
>> >
>> Subject: Re: [edk2] [PATCH V3 0/4] DXE Memory Protection
>>
>> On 9 February 2017 at 16:30, Ard Biesheuvel 
>> > wrote:
>> > On 9 February 2017 at 16:29, Yao, Jiewen 
>> > > wrote:
>> >> Very good point.
>> >>
>> >> Can ARCH64 set 4K paging for 64K aligned runtime memory?
>> >>
>> >
>> > UEFI always uses 4 KB, but the OS may use 64 KB, so to create the
>> > virtual address map it needs the runtime regions to be 64 KB aligned.
>> >
>> >>
>> >>
>> >> If yes, how about we use
>> >>
>> >> “ImageRecord->ImageSize = ALIGN_VALUE(LoadedImage->ImageSize,
>> >> EFI_PAGE_SIZE);”
>> >>
>> >
>>
>> Another question: did you try SetVirtualAddressMap()? It looks like we
>> need to lift read-only permissions to allow the runtime PE/COFF
>> relocation to apply the fixups
>> ___
>> 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] CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule

2017-02-10 Thread Yao, Jiewen 
That is an interesting question.

A general rule for Capsule App is:

1)   If a capsule has CAPSULE_FLAGS_INITIATE_RESET, the CapsuleService will 
issue reset. No need to handler in CapsuleApp. (The free memory logic will not 
run)

2)   If a capsule has CAPSULE_FLAGS_PERSIST_ACROSS_RESET, the 
CapsuleService will not issue reset, because CapsuleService will let caller 
decide when to reset. So here CapsuleApp does reset. (The free memory logic 
will not run)

3)   If a capsule has no CAPSULE_FLAGS_PERSIST_ACROSS_RESET flag, the 
CapsuleService processes the image immediately. Then CapsuleApp can free the 
buffer finally, because it is already processed.

May I know where you think we have logic error?

Thank you
Yao Jiewen

From: wang xiaofeng [mailto:winggundu...@163.com]
Sent: Friday, February 10, 2017 1:27 AM
To: Yao, Jiewen ; edk2-devel@lists.01.org
Subject: CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule

Hi Jiewen,
   I think it might be a logic error for CapsuleApp  with 
CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule. If a capsule only have 
CAPSULE_FLAGS_PERSIST_ACROSS_RESET flag, my understanding it will not trigger 
reset automatially. User will trigger reset（S3) in OS or application like 
CapsuleApp.
 CapsuleApp will not trigger reset if CAPSULE_FLAGS_INITIATE_RESET is not 
set, the problem is it will free the capsule buffer also!

Done:
  for (Index = 0; Index < CapsuleNum; Index++) {
if (CapsuleBuffer[Index] != NULL) {
  FreePool (CapsuleBuffer[Index]);
}
  }

  CleanGatherList(BlockDescriptors, CapsuleNum);

   I trited to remove the above free buffer logic and capsuleApp can work with 
CAPSULE_FLAGS_INITIATE_RESET only attribute. The capsule data should be kept in 
this case





___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH V2] MdePkg ACPI: Incorrect definition name for ACPI IORT Table signature

2017-02-10 Thread Yao, Jiewen 
Reviewed-by: jiewen@intel.com

> -Original Message-
> From: Zeng, Star
> Sent: Thursday, February 9, 2017 11:41 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; Alexei Fedorov
> ; Yao, Jiewen ; Gao, Liming
> ; Kinney, Michael D 
> Subject: [PATCH V2] MdePkg ACPI: Incorrect definition name for ACPI IORT Table
> signature
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=363
> 
> The definition name for ACPI IO Remapping Table signature is incorrect
> in Acpi60.h and Acpi61.h as below:
> EFI_ACPI_6_0_INTERRUPT_SOURCE_OVERRIDE_SIGNATURE
> EFI_ACPI_6_1_INTERRUPT_SOURCE_OVERRIDE_SIGNATURE
> 
> The name should be changed to
> EFI_ACPI_6_0_IO_REMAPPING_TABLE_SIGNATURE
> EFI_ACPI_6_1_IO_REMAPPING_TABLE_SIGNATURE
> 
> The comments
> ///
> /// "IORT" Interrupt Source Override
> ///
> will be also changed to
> ///
> /// "IORT" I/O Remapping Table
> ///
> 
> V2: Minor commit log update.
> 
> Cc: Alexei Fedorov 
> Cc: Jiewen Yao 
> Cc: Liming Gao 
> Cc: Michael Kinney 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Star Zeng 
> ---
>  MdePkg/Include/IndustryStandard/Acpi60.h | 6 +++---
>  MdePkg/Include/IndustryStandard/Acpi61.h | 6 +++---
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/MdePkg/Include/IndustryStandard/Acpi60.h
> b/MdePkg/Include/IndustryStandard/Acpi60.h
> index ab798e9cf4c1..6183d8a9bd6c 100644
> --- a/MdePkg/Include/IndustryStandard/Acpi60.h
> +++ b/MdePkg/Include/IndustryStandard/Acpi60.h
> @@ -1,7 +1,7 @@
>  /** @file
>ACPI 6.0 definitions from the ACPI Specification Revision 6.0 Errata A 
> January,
> 2016.
> 
> -  Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.
> +  Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.
>(C) Copyright 2015-2016 Hewlett Packard Enterprise Development LP
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of the BSD
> License
> @@ -2252,9 +2252,9 @@ typedef struct {
>  #define EFI_ACPI_6_0_ISCSI_BOOT_FIRMWARE_TABLE_SIGNATURE
> SIGNATURE_32('i', 'B', 'F', 'T')
> 
>  ///
> -/// "IORT" Interrupt Source Override
> +/// "IORT" I/O Remapping Table
>  ///
> -#define EFI_ACPI_6_0_INTERRUPT_SOURCE_OVERRIDE_SIGNATURE
> SIGNATURE_32('I', 'O', 'R', 'T')
> +#define EFI_ACPI_6_0_IO_REMAPPING_TABLE_SIGNATURE
> SIGNATURE_32('I', 'O', 'R', 'T')
> 
>  ///
>  /// "IVRS" I/O Virtualization Reporting Structure
> diff --git a/MdePkg/Include/IndustryStandard/Acpi61.h
> b/MdePkg/Include/IndustryStandard/Acpi61.h
> index dc3153b7e7c8..0fac9a80694d 100644
> --- a/MdePkg/Include/IndustryStandard/Acpi61.h
> +++ b/MdePkg/Include/IndustryStandard/Acpi61.h
> @@ -1,7 +1,7 @@
>  /** @file
>ACPI 6.1 definitions from the ACPI Specification Revision 6.1 January, 
> 2016.
> 
> -  Copyright (c) 2016, Intel Corporation. All rights reserved.
> +  Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.
>   (C) Copyright 2016 Hewlett Packard Enterprise Development LP
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of the BSD
> License
> @@ -2284,9 +2284,9 @@ typedef struct {
>  #define EFI_ACPI_6_1_ISCSI_BOOT_FIRMWARE_TABLE_SIGNATURE
> SIGNATURE_32('i', 'B', 'F', 'T')
> 
>  ///
> -/// "IORT" Interrupt Source Override
> +/// "IORT" I/O Remapping Table
>  ///
> -#define EFI_ACPI_6_1_INTERRUPT_SOURCE_OVERRIDE_SIGNATURE
> SIGNATURE_32('I', 'O', 'R', 'T')
> +#define EFI_ACPI_6_1_IO_REMAPPING_TABLE_SIGNATURE
> SIGNATURE_32('I', 'O', 'R', 'T')
> 
>  ///
>  /// "IVRS" I/O Virtualization Reporting Structure
> --
> 2.7.0.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 3/3] UefiCpuPkg/Universal/Acpi/S3Resume2Pei: Add support for PCD PcdPteMemoryEncryptionAddressOrMask

2017-02-10 Thread Laszlo Ersek 
Leo,

On 02/10/17 05:27, Duran, Leo wrote:
> Hi Jeff,
> The new PCD is intended to be OR'ed with the address (upper bits).
> Leo.

if I understand correctly, you only answered Jeff's first question:

>> -Original Message-
>> From: Fan, Jeff [mailto:jeff@intel.com]
>> Sent: Thursday, February 09, 2017 8:23 PM
>> To: Duran, Leo ; edk2-de...@ml01.01.org
>> Cc: Tian, Feng ; Zeng, Star ;
>> Laszlo Ersek ; Singh, Brijesh 
>> Subject: RE: [PATCH 3/3] UefiCpuPkg/Universal/Acpi/S3Resume2Pei: Add
>> support for PCD PcdPteMemoryEncryptionAddressOrMask
>>
>> Hi Leo,
>>
>> I want to understand your usage model. What fields are you going to update
>> in below Page Table Entry by the new PCD?

This being it. But there's another at the bottom:

>>
>> typedef union {
>>   struct {
>> UINT64  Present:1;// 0 = Not present in memory, 1 = 
>> Present in
>> memory
>> UINT64  ReadWrite:1;  // 0 = Read-Only, 1= Read/Write
>> UINT64  UserSupervisor:1; // 0 = Supervisor, 1=User
>> UINT64  WriteThrough:1;   // 0 = Write-Back caching, 
>> 1=Write-Through
>> caching
>> UINT64  CacheDisabled:1;  // 0 = Cached, 1=Non-Cached
>> UINT64  Accessed:1;   // 0 = Not accessed, 1 = Accessed (set 
>> by CPU)
>> UINT64  Dirty:1;  // 0 = Not Dirty, 1 = written by 
>> processor on access
>> to page
>> UINT64  MustBe1:1;// Must be 1
>> UINT64  Global:1; // 0 = Not global page, 1 = global 
>> page TLB not
>> cleared on CR3 write
>> UINT64  Available:3;  // Available for use by system software
>> UINT64  PAT:1;//
>> UINT64  MustBeZero:8; // Must be zero;
>> UINT64  PageTableBaseAddress:31;  // Page Table Base Address
>> UINT64  AvabilableHigh:11;// Available for use by system software
>> UINT64  Nx:1; // 0 = Execute Code, 1 = No Code 
>> Execution
>>   } Bits;
>>   UINT64Uint64;
>> } PAGE_TABLE_ENTRY;
>>
>> I did not see any updating in SMM. Is it un-necessary? Is this feature 
>> working
>> on POST phase or OS runtime phase?

I think that updating SMM is definitely necessary.

Have you been testing SMM?

If not, QEMU-level instructions are in OvmfPkg/README (see "SMM support").

If you prefer libvirt to the raw QEMU command line (I certainly do),
then please see
.
(This article talks about an Intel KVM host, but I trust you can easily
translate the instructions / checks to AMD.)

(If you've been testing with SMM all the time, then I apologize; I must
have missed it.)

Thanks!
Laszlo

>>
>> Thanks!
>> Jeff
>>
>> -Original Message-
>> From: Leo Duran [mailto:leo.du...@amd.com]
>> Sent: Thursday, February 09, 2017 5:13 AM
>> To: edk2-de...@ml01.01.org
>> Cc: Leo Duran; Fan, Jeff; Tian, Feng; Zeng, Star; Laszlo Ersek; Brijesh Singh
>> Subject: [PATCH 3/3] UefiCpuPkg/Universal/Acpi/S3Resume2Pei: Add
>> support for PCD PcdPteMemoryEncryptionAddressOrMask
>>
>> This PCD holds the address mask for page table entries when memory
>> encryption is enabled on AMD processors supporting the Secure Encrypted
>> Virtualization (SEV) feature.
>>
>> The mask is applied when page tables are created (S3Resume.c).
>>
>> CC: Jeff Fan 
>> Cc: Feng Tian 
>> Cc: Star Zeng 
>> Cc: Laszlo Ersek 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Brijesh Singh 
>> Signed-off-by: Leo Duran 
>> ---
>>  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c   | 9 +
>>  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf | 2 ++
>>  2 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
>> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
>> index d306fba..ee1e2cd 100644
>> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
>> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
>> @@ -5,6 +5,7 @@
>>control is passed to OS waking up handler.
>>
>>Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.
>> +  Copyright (c) 2017, AMD Incorporated. All rights reserved.
>>
>>This program and the accompanying materials
>>are licensed and made available under the terms and conditions @@ -696,7
>> +697,7 @@ RestoreS3PageTables (
>>//
>>// Make a PML4 Entry
>>//
>> -  PageMapLevel4Entry->Uint64 =
>> (UINT64)(UINTN)PageDirectoryPointerEntry;
>> +  PageMapLevel4Entry->Uint64 =
>> + (UINT64)(UINTN)PageDirectoryPointerEntry | PcdGet64
>> + (PcdPteMemoryEncryptionAddressOrMask);
>>PageMapLevel4Entry->Bits.ReadWrite = 1;
>>

[edk2] CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule

2017-02-10 Thread wang xiaofeng 
Hi Jiewen, 
   I think it might be a logic error for CapsuleApp  with 
CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule. If a capsule only have 
CAPSULE_FLAGS_PERSIST_ACROSS_RESET flag, my understanding it will not trigger 
reset automatially. User will trigger reset（S3) in OS or application like 
CapsuleApp.
 CapsuleApp will not trigger reset if CAPSULE_FLAGS_INITIATE_RESET is not 
set, the problem is it will free the capsule buffer also!


Done:
  for (Index = 0; Index < CapsuleNum; Index++) {
if (CapsuleBuffer[Index] != NULL) {
  FreePool (CapsuleBuffer[Index]);
}
  }


  CleanGatherList(BlockDescriptors, CapsuleNum);
  
   I trited to remove the above free buffer logic and capsuleApp can work with 
CAPSULE_FLAGS_INITIATE_RESET only attribute. The capsule data should be kept in 
this case



___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [Patch] BaseTools: NULL pointer comparison to a char literal should be to NULL

2017-02-10 Thread Yonghong Zhu 
From: Nikolai SAOUKH 

GCC7 complaint -- error: ISO C++ forbids comparison between pointer and
integer.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Nikolai SAOUKH 
Reviewed-by: Yonghong Zhu 
---
 BaseTools/Source/C/VfrCompile/VfrUtilityLib.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/BaseTools/Source/C/VfrCompile/VfrUtilityLib.cpp 
b/BaseTools/Source/C/VfrCompile/VfrUtilityLib.cpp
index 3ca57ed..b81fc7b 100644
--- a/BaseTools/Source/C/VfrCompile/VfrUtilityLib.cpp
+++ b/BaseTools/Source/C/VfrCompile/VfrUtilityLib.cpp
@@ -3370,11 +3370,11 @@ CVfrStringDB::GetVarStoreNameFormStringId (
   EFI_STATUS  Status;
   CHAR8   LineBuf[EFI_IFR_MAX_LENGTH];
   UINT8   BlockType;
   EFI_HII_STRING_PACKAGE_HDR *PkgHeader;
   
-  if (mStringFileName == '\0' ) {
+  if (mStringFileName == NULL || *mStringFileName == '\0' ) {
 return NULL;
   }
 
   if ((pInFile = fopen (LongFilePath (mStringFileName), "rb")) == NULL) {
 return NULL;
-- 
2.6.1.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH] ShellPkg/pci: Report error when invalid value is specified for "-ec"

2017-02-10 Thread Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni 
Cc: Jaben Carsey 
---
 ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c   | 21 +++--
 .../UefiShellDebug1CommandsLib.uni  |  2 +-
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c 
b/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
index fb7561f..37f15d6 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
@@ -2726,6 +2726,7 @@ ShellCommandRunPci (
 Bus   = 0;
 Device= 0;
 Func  = 0;
+EnhancedDump  = 0x;
 if (ShellCommandLineGetFlag(Package, L"-i")) {
   ExplainData = TRUE;
 }
@@ -2807,6 +2808,20 @@ ShellCommandRunPci (
   }
 }
 
+Temp = ShellCommandLineGetValue (Package, L"-ec");
+if (Temp != NULL) {
+  //
+  // Input converted to hexadecimal number.
+  //
+  if (!EFI_ERROR (ShellConvertStringToUint64 (Temp, , TRUE, TRUE))) 
{
+EnhancedDump = (UINT16) RetVal;
+  } else {
+ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_PARAM_INV_HEX), 
gShellDebug1HiiHandle, L"pci", Temp);  
+ShellStatus = SHELL_INVALID_PARAMETER;
+goto Done;
+  }
+}
+
 //
 // Find the protocol interface who's in charge of current segment, and its
 // bus range covers the current bus
@@ -2883,12 +2898,6 @@ ShellCommandRunPci (
 // If "-i" appears in command line, interpret data in configuration space
 //
 if (ExplainData) {
-  EnhancedDump = 0x;
-  if (ShellCommandLineGetFlag(Package, L"-ec")) {
-Temp = ShellCommandLineGetValue(Package, L"-ec");
-ASSERT (Temp != NULL);
-EnhancedDump = (UINT16) ShellHexStrToUintn (Temp);
-  }
   Status = PciExplainData (, Address, IoDev, EnhancedDump);
 }
   }
diff --git 
a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.uni 
b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.uni
index 8ea4215..7c0ca98 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.uni
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.uni
@@ -39,7 +39,7 @@
 #string STR_GEN_PCIRBIO_NF#language en-US "%H%s%N: Protocol - 
PciRootBridgeIo not found.\r\n"
 #string STR_GEN_PCIRBIO_ER#language en-US "%H%s%N: Problem accessing 
the data using Protocol - PciRootBridgeIo\r\n"
 #string STR_GEN_PARAM_INV #language en-US "%H%s%N: Invalid argument - 
'%H%s%N'\r\n"
-#string STR_GEN_PARAM_INV_HEX #language en-US "%H%s%N: Invalid parameter - 
'%H%s%N:'.  Must be hexadecimal.\r\n"
+#string STR_GEN_PARAM_INV_HEX #language en-US "%H%s%N: Invalid parameter - 
'%H%s%N'. Must be hexadecimal.\r\n"
 #string STR_GEN_PARAM_CONFLICT#language en-US "%H%s%N: Flags conflict with 
- '%H%s%N' and '%H%s%N'\r\n"
 #string STR_GEN_OUT_MEM   #language en-US "%H%s%N: Memory allocation 
was not successful.\r\n"
 #string STR_GEN_MAP_PROTOCOL  #language en-US "%H%s%N: Mapped device 
'%B%s%N' does not have protocol %B%s%N\r\n"
-- 
2.9.0.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 2/3] MdeModulePkg/Universal/CapsulePei: Add support for PCD PcdPteMemoryEncryptionAddressOrMask

2017-02-10 Thread Zeng, Star 
Leo,

CapsuleX64 is a standalone module, PcdGet64  
(PcdPteMemoryEncryptionAddressOrMask) could not be used in X64Entry 
PageFaultHandler() as PcdPteMemoryEncryptionAddressOrMask may be configured to 
DYNAMIC type.
You can use similar logic with PAGE_FAULT_CONTEXT.Page1GSupport to transfer the 
PcdPteMemoryEncryptionAddressOrMask PCD value from CapsulePei to CapsuleX64.

Thanks,
Star
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Leo Duran
Sent: Thursday, February 9, 2017 5:13 AM
To: edk2-de...@ml01.01.org
Cc: Laszlo Ersek ; Tian, Feng ; Leo 
Duran ; Brijesh Singh ; Zeng, Star 

Subject: [edk2] [PATCH 2/3] MdeModulePkg/Universal/CapsulePei: Add support for 
PCD PcdPteMemoryEncryptionAddressOrMask

This PCD holds the address mask for page table entries when memory encryption 
is enabled on AMD processors supporting the Secure Encrypted Virtualization 
(SEV) feature.

The mask is applied when 4GB tables are created (UefiCapsule.c), and when the 
tables are expanded on-demand by page-faults above 4GB's (X64Entry.c).

Cc: Feng Tian 
Cc: Star Zeng 
Cc: Laszlo Ersek 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Brijesh Singh 
Signed-off-by: Leo Duran 
---
 MdeModulePkg/Universal/CapsulePei/CapsulePei.inf |  2 ++  
MdeModulePkg/Universal/CapsulePei/CapsuleX64.inf |  4   
MdeModulePkg/Universal/CapsulePei/UefiCapsule.c  |  9 +  
MdeModulePkg/Universal/CapsulePei/X64/X64Entry.c | 10 ++
 4 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf 
b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
index d2ca0d0..c54bc21 100644
--- a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
+++ b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
@@ -7,6 +7,7 @@
 #  buffer overflow, integer overflow.
 #
 # Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.
+# Copyright (c) 2017, AMD Incorporated. All rights reserved.
 #
 # This program and the accompanying materials  # are licensed and made 
available under the terms and conditions @@ -76,6 +77,7 @@ [Ppis.IA32]  
[Pcd.IA32]
   gEfiMdeModulePkgTokenSpaceGuid.PcdCapsuleCoalesceFile ## 
SOMETIMES_CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable  ## 
SOMETIMES_CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask## 
CONSUMES
 
 [FeaturePcd.IA32]
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode  ## CONSUMES diff 
--git a/MdeModulePkg/Universal/CapsulePei/CapsuleX64.inf 
b/MdeModulePkg/Universal/CapsulePei/CapsuleX64.inf
index 8318eaa..cf8543b 100644
--- a/MdeModulePkg/Universal/CapsulePei/CapsuleX64.inf
+++ b/MdeModulePkg/Universal/CapsulePei/CapsuleX64.inf
@@ -10,6 +10,7 @@
 #  buffer overflow, integer overflow.
 #
 # Copyright (c) 2011 - 2015, Intel Corporation. All rights reserved.
+# Copyright (c) 2017, AMD Incorporated. All rights reserved.
 #
 # This program and the accompanying materials  # are licensed and made 
available under the terms and conditions @@ -53,6 +54,9 @@ [LibraryClasses]
   CpuExceptionHandlerLib
   DebugAgentLib
 
+[Pcd]
+  gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask## 
CONSUMES
+
 [Depex]
   FALSE
 
diff --git a/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c 
b/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c
index 9ac9d22..77bc68c 100644
--- a/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c
+++ b/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c
@@ -2,6 +2,7 @@
   Capsule update PEIM for UEFI2.0
 
 Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.
+Copyright (c) 2017, AMD Incorporated. All rights reserved.
 
 This program and the accompanying materials  are licensed and made available 
under the terms and conditions @@ -187,7 +188,7 @@ Create4GPageTables (
 //
 // Make a PML4 Entry
 //
-PageMapLevel4Entry->Uint64 = (UINT64)(UINTN)PageDirectoryPointerEntry;
+PageMapLevel4Entry->Uint64 = 
+ (UINT64)(UINTN)PageDirectoryPointerEntry | PcdGet64 
+ (PcdPteMemoryEncryptionAddressOrMask);
 PageMapLevel4Entry->Bits.ReadWrite = 1;
 PageMapLevel4Entry->Bits.Present = 1;
 
@@ -198,7 +199,7 @@ Create4GPageTables (
 //
 // Fill in the Page Directory entries
 //
-PageDirectory1GEntry->Uint64 = (UINT64)PageAddress;
+PageDirectory1GEntry->Uint64 = (UINT64)PageAddress | PcdGet64 
+ (PcdPteMemoryEncryptionAddressOrMask);
 PageDirectory1GEntry->Bits.ReadWrite = 1;
 PageDirectory1GEntry->Bits.Present = 1;
 PageDirectory1GEntry->Bits.MustBe1 = 1; @@ -215,7 +216,7 @@ 
Create4GPageTables (
 //
 // Fill in a Page Directory Pointer Entries
 //
-PageDirectoryPointerEntry->Uint64 =

Re: [edk2] [PATCH 1/3] MdeModulePkg: Add PCD PcdPteMemoryEncryptionAddressOrMask

2017-02-10 Thread Zeng, Star 
Reviewed-by: Star Zeng 

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Leo Duran
Sent: Thursday, February 9, 2017 5:13 AM
To: edk2-de...@ml01.01.org
Cc: Laszlo Ersek ; Tian, Feng ; Brijesh 
Singh ; Zeng, Star ; Leo Duran 

Subject: [edk2] [PATCH 1/3] MdeModulePkg: Add PCD 
PcdPteMemoryEncryptionAddressOrMask

From: Brijesh Singh 

This PCD holds the address mask for page table entries when memory encryption 
is enabled on AMD processors supporting the Secure Encrypted Virtualization 
(SEV) feature.

Cc: Feng Tian 
Cc: Star Zeng 
Cc: Laszlo Ersek 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Leo Duran 
---
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf  |  5 -
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 18 ++
 MdeModulePkg/MdeModulePkg.dec|  8 
 3 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf 
b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index 2bc41be..d62bd9b 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -6,6 +6,8 @@
 #  needed to run the DXE Foundation.
 #
 #  Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.
+#  Copyright (c) 2017, AMD Incorporated. All rights reserved. #
 #  This program and the accompanying materials  #  are licensed and made 
available under the terms and conditions of the BSD License  #  which 
accompanies this distribution.  The full text of the license may be found at @@ 
-111,7 +113,8 @@ [FeaturePcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSupportUefiDecompress ## CONSUMES
 
 [Pcd.IA32,Pcd.X64]
-  gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable  ## 
SOMETIMES_CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable  ## 
SOMETIMES_CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask## 
CONSUMES
 
 [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack   ## 
SOMETIMES_CONSUMES
diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c 
b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
index 790f6ab..2c52389 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
@@ -16,6 +16,8 @@
 3) IA-32 Intel(R) Architecture Software Developer's Manual Volume 3:System 
Programmer's Guide, Intel
 
 Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.
+Copyright (c) 2017, AMD Incorporated. All rights reserved.
+
 This program and the accompanying materials  are licensed and made available 
under the terms and conditions of the BSD License  which accompanies this 
distribution.  The full text of the license may be found at @@ -71,14 +73,14 @@ 
Split2MPageTo4K (
   //
   // Fill in 2M page entry.
   //
-  *PageEntry2M = (UINT64) (UINTN) PageTableEntry | IA32_PG_P | IA32_PG_RW;
+  *PageEntry2M = (UINT64) (UINTN) PageTableEntry | PcdGet64 
+ (PcdPteMemoryEncryptionAddressOrMask) | IA32_PG_P | IA32_PG_RW;
 
   PhysicalAddress4K = PhysicalAddress;
   for (IndexOfPageTableEntries = 0; IndexOfPageTableEntries < 512; 
IndexOfPageTableEntries++, PageTableEntry++, PhysicalAddress4K += SIZE_4KB) {
 //
 // Fill in the Page Table entries
 //
-PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K;
+PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | PcdGet64 
+ (PcdPteMemoryEncryptionAddressOrMask);
 PageTableEntry->Bits.ReadWrite = 1;
 PageTableEntry->Bits.Present = 1;
 if ((PhysicalAddress4K >= StackBase) && (PhysicalAddress4K < StackBase + 
StackSize)) { @@ -116,7 +118,7 @@ Split1GPageTo2M (
   //
   // Fill in 1G page entry.
   //
-  *PageEntry1G = (UINT64) (UINTN) PageDirectoryEntry | IA32_PG_P | IA32_PG_RW;
+  *PageEntry1G = (UINT64) (UINTN) PageDirectoryEntry | PcdGet64 
+ (PcdPteMemoryEncryptionAddressOrMask) | IA32_PG_P | IA32_PG_RW;
 
   PhysicalAddress2M = PhysicalAddress;
   for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; 
IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M += 
SIZE_2MB) { @@ -129,7 +131,7 @@ Split1GPageTo2M (
   //
   // Fill in the Page Directory entries
   //
-  PageDirectoryEntry->Uint64 = (UINT64) PhysicalAddress2M;
+  PageDirectoryEntry->Uint64 = (UINT64) PhysicalAddress2M | 
+ PcdGet64 (PcdPteMemoryEncryptionAddressOrMask);
   PageDirectoryEntry->Bits.ReadWrite = 1;
   PageDirectoryEntry->Bits.Present = 1;
   PageDirectoryEntry->Bits.MustBe1 = 1; @@ -248,7 +250,7 @@ 
CreateIdentityMappingPageTables (
 //
 // Make a PML4 Entry
 //
-PageMapLevel4Entry->Uint64 =