[edk2] [PATCH edk2-non-osi v2] Update D03/D05 MemoryInit.efi binary for bug 3419

[Ming Huang]

https://bugs.linaro.org/show_bug.cgi?id=3061
For fix this bug,the function PciIoPciRead of NonDiscoverablePciDeviceDxe 
should be modified also.

Code can also be found in github:
https://github.com/hisilicon/OpenPlatformPkg.git
branch: rp-osi-bug-v2

Ming Huang (1):
  Hisilicon D0x: Remove uncacheable attribute from memory resource HOB

 Platform/Hisilicon/D03/MemoryInitPei/MemoryInit.efi | Bin 90272 -> 90336 bytes
 Platform/Hisilicon/D05/MemoryInitPei/MemoryInit.efi | Bin 152576 -> 152480 
bytes
 2 files changed, 0 insertions(+), 0 deletions(-)

-- 
1.9.1

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

[edk2] [PATCH edk2-non-osi v2] Hisilicon D0x: Remove uncacheable attribute from memory resource HOB

[Ming Huang]

If uncacheable attribute is included in memory resource HOB,
GCD spaces will also have EFI_MEMORY_UC capability,
then NonCoherentPciIoAllocateBuffer of NonDiscoverablePciDeviceDxe
module will allocate DMA buffer of EFI_MEMORY_UC type, which will
cause alignment fault exception with BaseMemoryLibOptDxe.

This not only affects NonDiscoverablePciDeviceDxe, it removes the
UC attribute from all DRAM regions in the UEFI memory map,
which makes much more sense on ARM

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Liu Yi 
Signed-off-by: Heyi Guo 
---
 Platform/Hisilicon/D03/MemoryInitPei/MemoryInit.efi | Bin 90272 -> 90336 bytes
 Platform/Hisilicon/D05/MemoryInitPei/MemoryInit.efi | Bin 152576 -> 152480 
bytes
 2 files changed, 0 insertions(+), 0 deletions(-)

diff --git a/Platform/Hisilicon/D03/MemoryInitPei/MemoryInit.efi 
b/Platform/Hisilicon/D03/MemoryInitPei/MemoryInit.efi
index 354abcc..31e2903 100644
Binary files a/Platform/Hisilicon/D03/MemoryInitPei/MemoryInit.efi and 
b/Platform/Hisilicon/D03/MemoryInitPei/MemoryInit.efi differ
diff --git a/Platform/Hisilicon/D05/MemoryInitPei/MemoryInit.efi 
b/Platform/Hisilicon/D05/MemoryInitPei/MemoryInit.efi
index b94e0cb..eb71c44 100644
Binary files a/Platform/Hisilicon/D05/MemoryInitPei/MemoryInit.efi and 
b/Platform/Hisilicon/D05/MemoryInitPei/MemoryInit.efi differ
-- 
1.9.1

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

[edk2] [Patch][edk2-platforms/devel-MinnowBoard3-UDK2017] fTPM

[xianhu2x]

Boot failed after PTT is enabled with gcc-built image

Contributed-under: TianoCore Contribution Agreement 1.1

Signed-off-by: xianhu2x 
---
 .../Txe/Library/PeiDxePttPtpLib/PeiDxePttPtpLib.c  | 44 ++
 1 file changed, 29 insertions(+), 15 deletions(-)

diff --git 
a/Silicon/BroxtonSoC/BroxtonSiPkg/Txe/Library/PeiDxePttPtpLib/PeiDxePttPtpLib.c 
b/Silicon/BroxtonSoC/BroxtonSiPkg/Txe/Library/PeiDxePttPtpLib/PeiDxePttPtpLib.c
index 24e8c5bb6..89251fa19 100644
--- 
a/Silicon/BroxtonSoC/BroxtonSiPkg/Txe/Library/PeiDxePttPtpLib/PeiDxePttPtpLib.c
+++ 
b/Silicon/BroxtonSoC/BroxtonSiPkg/Txe/Library/PeiDxePttPtpLib/PeiDxePttPtpLib.c
@@ -228,6 +228,8 @@ PttHciSend(
 {
   UINT32  ControlStatus;
   UINT32  WaitTime;
+  UINTN   StartAddress;
+  UINT32  *ReturnBuffer;
 
   //
   // Make sure that previous command was in fact completed if not, must not
@@ -290,12 +292,15 @@ PttHciSend(
 DataLength += (4 - (DataLength % 4));
 DEBUG ((DEBUG_INFO, "to %d\n", DataLength));
   }
-
-  MmioWriteBuffer32 ((UINTN) R_PTT_HCI_BASE_ADDRESS +
+  ReturnBuffer = (UINT32 *)PttBuffer;
+  StartAddress = (UINTN) R_PTT_HCI_BASE_ADDRESS +
   ( TPM_LOCALITY_0 * TPM_LOCALITY_BUFFER_SIZE )+
-  0x80,
-  DataLength,
-  (UINT32 *) PttBuffer );
+  0x80;
+  while (DataLength != 0) {
+MmioWrite32 ((UINTN) StartAddress, *(ReturnBuffer++));
+StartAddress += sizeof (UINT32);
+DataLength-= sizeof (UINT32);
+  }
 
   //
   // Trigger Command processing by writing to start register
@@ -334,6 +339,9 @@ PttHciReceive (
   UINT16  Data16;
   UINT32  Data32;
   EFI_STATUS  Status;
+  UINT32  *ReturnBuffer;
+  UINTN   StartAddress;
+  UINT32  DataLength;
 
   Status = EFI_SUCCESS;
   DEBUG ((DEBUG_INFO, "PTT: PttHciReceive start\n"));
@@ -365,11 +373,14 @@ PttHciReceive (
   //
   // Read the response data header
   //
-  MmioReadBuffer32 ((UINTN) R_PTT_HCI_BASE_ADDRESS +
-  ( TPM_LOCALITY_0 * TPM_LOCALITY_BUFFER_SIZE )+
-  0x80,
-  PTT_HCI_RESPONSE_HEADER_SIZE,
- (UINT32 *) PttBuffer);
+  ReturnBuffer = (UINT32 *)PttBuffer;
+  StartAddress = (UINTN) R_PTT_HCI_BASE_ADDRESS + ( TPM_LOCALITY_0 * 
TPM_LOCALITY_BUFFER_SIZE ) + 0x80;
+  DataLength   = PTT_HCI_RESPONSE_HEADER_SIZE;
+  while (DataLength != 0) {
+*(ReturnBuffer ++) = MmioRead32 (StartAddress);
+StartAddress += sizeof (UINT32);
+DataLength -= sizeof (UINT32);
+  }
 
   //
   // Check the reponse data header (tag, parasize and returncode)
@@ -411,11 +422,14 @@ PttHciReceive (
   //
   // Read the entire response data header
   //
-  MmioReadBuffer32 ((UINTN) R_PTT_HCI_BASE_ADDRESS +
-  (TPM_LOCALITY_0 * TPM_LOCALITY_BUFFER_SIZE)+
-  0x80,
-  *RespSize,
- (UINT32 *) PttBuffer);
+  ReturnBuffer = (UINT32 *)PttBuffer;
+  StartAddress = (UINTN) R_PTT_HCI_BASE_ADDRESS + ( TPM_LOCALITY_0 * 
TPM_LOCALITY_BUFFER_SIZE ) + 0x80;
+  DataLength   = *RespSize;
+  while (DataLength != 0) {
+*(ReturnBuffer ++) = MmioRead32 (StartAddress);
+StartAddress += sizeof (UINT32);
+DataLength -= sizeof (UINT32);
+  }
 Exit:
 
   return Status;
-- 
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] MdeModulePkg: Update IP4 stack to support point-to-point link with 31-bit mask.

[Wu, Jiaxin]

Reviewed-by: Wu Jiaxin 



> -Original Message-
> From: Fu, Siyuan
> Sent: Wednesday, October 18, 2017 1:17 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Jiaxin ; Ye, Ting 
> Subject: [Patch] MdeModulePkg: Update IP4 stack to support point-to-point
> link with 31-bit mask.
> 
> This patch is to follow RFC3021 which allows to use 31-bit mask
> in point-to-point link.
> If a 31-bit subnet mask is assigned to a point-to-point link, it
> leaves the  with only 1 bit.  Consequently, only two
> possible addresses may result:
>   {, 0} and {, -1}
> These addresses have historically been associated with network and
> broadcast addresses (see Section 2.2).  In a point-to-point link with
> a 31-bit subnet mask, the two addresses above MUST be interpreted as
> host addresses.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Fu Siyuan 
> Cc: Wu Jiaxin 
> Cc: Ye Ting 
> ---
>  MdeModulePkg/Include/Library/NetLib.h |  6 --
>  MdeModulePkg/Library/DxeNetLib/DxeNetLib.c| 14 ++
>  MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Common.c | 20
> +---
>  3 files changed, 27 insertions(+), 13 deletions(-)
> 
> diff --git a/MdeModulePkg/Include/Library/NetLib.h
> b/MdeModulePkg/Include/Library/NetLib.h
> index 4cd4227..b9df46c 100644
> --- a/MdeModulePkg/Include/Library/NetLib.h
> +++ b/MdeModulePkg/Include/Library/NetLib.h
> @@ -414,8 +414,10 @@ NetGetIpClass (
> 
>ASSERT if NetMask is zero.
> 
> -  If all bits of the host address of IP are 0 or 1, IP is also not a valid 
> unicast
> address.
> -
> +  If all bits of the host address of IP are 0 or 1, IP is also not a valid 
> unicast
> address,
> +  except when the originator is one of the endpoints of a point-to-point link
> with a 31-bit
> +  mask (RFC3021).
> +
>@param[in]  IpThe IP to check against.
>@param[in]  NetMask   The mask of the IP.
> 
> diff --git a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> index 1ee77fe..b8544b8 100644
> --- a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> +++ b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> @@ -637,7 +637,9 @@ NetGetIpClass (
> 
>ASSERT if NetMask is zero.
> 
> -  If all bits of the host address of IP are 0 or 1, IP is also not a valid 
> unicast
> address.
> +  If all bits of the host address of IP are 0 or 1, IP is also not a valid 
> unicast
> address,
> +  except when the originator is one of the endpoints of a point-to-point link
> with a 31-bit
> +  mask (RFC3021).
> 
>@param[in]  IpThe IP to check against.
>@param[in]  NetMask   The mask of the IP.
> @@ -657,9 +659,13 @@ NetIp4IsUnicast (
>if (Ip == 0 || IP4_IS_LOCAL_BROADCAST (Ip)) {
>  return FALSE;
>}
> -
> -  if (((Ip &~NetMask) == ~NetMask) || ((Ip &~NetMask) == 0)) {
> -return FALSE;
> +
> +  if (NetGetMaskLength (NetMask) != 31) {
> +if (((Ip &~NetMask) == ~NetMask) || ((Ip &~NetMask) == 0)) {
> +  return FALSE;
> +}
> +  } else {
> +return TRUE;
>}
> 
>return TRUE;
> diff --git a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Common.c
> b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Common.c
> index 7c7d182..b8160c1 100644
> --- a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Common.c
> +++ b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Common.c
> @@ -314,16 +314,22 @@ Ip4StationAddressValid (
> 
>//
>// Station address can't be subnet broadcast/net broadcast address
> +  // RFC3021: In a point-to-point network with 31-bit mask, the  number> only has
> +  // 1 bit, only two possible addresses may result:
> +  //   {, 0} and {, -1}
> +  // The two address above must be interpreted as host addresses.
>//
> -  if ((Ip == (Ip & Netmask)) || (Ip == (Ip | ~Netmask))) {
> -return FALSE;
> -  }
> +  if (Len != 31) {
> +if ((Ip == (Ip & Netmask)) || (Ip == (Ip | ~Netmask))) {
> +  return FALSE;
> +}
> 
> -  NetBrdcastMask = gIp4AllMasks[MIN (Len, Type << 3)];
> +NetBrdcastMask = gIp4AllMasks[MIN (Len, Type << 3)];
> 
> -  if (Ip == (Ip | ~NetBrdcastMask)) {
> -return FALSE;
> +if (Ip == (Ip | ~NetBrdcastMask)) {
> +  return FALSE;
> +}
>}
> 
>return TRUE;
> -}
> \ No newline at end of file
> +}
> --
> 1.9.5.msysgit.1

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

[edk2] [PATCH v5 0/7] Implement heap guard feature

[Jian J Wang]

> Patch V5 changes:
> a. Remove EFI from comment for SMM memory attribute protocol.
> b. Change parameter modifier of GetAttribute from IN to OUT
> c. Add ASSERT to make sure static paging and heap guard are not enabled
>at the same time
> d. Remove lib and pcd no longer needed in PiSmmCore.inf
> e. Fix duplicate pcd token number in dec


> Pacth V4 changes:
> a. Change names of gEdkiiSmmMemoryAttributeProtocolGuid related
>definitions from EFI_ to EDKII_
> b. Coding style cleanup
> c. Split patches in a more reasonable order and groups

> Patch V3 changes:
> a. Add new protocol gEdkiiSmmMemoryAttributeProtocolGuid to do
>memory attributes update instead of doing it directly in SmmCore
> b. Fix GCC build error

> Patch V2 changes:
> a. Remove local variable initializer with memory copy from globals
> b. Change map table dump code to use DEBUG_PAGE|DEBUG_POOL level
>message 
> c. Fix malfunction in 32-bit boot mode
> d. Add comment for the use of mOnGuarding
> e. Change name of function InitializePageTableLib to 
>InitializePageTableGlobals
> f. Add code in 32-bit code to bypass setting page table to read-only
> g. Coding style clean-up
>

This feature makes use of paging mechanism to add a hidden (not present)
page just before and after the allocated memory block. If the code tries
to access memory outside of the allocated part, page fault exception will
be triggered.

This feature is disabled by default and is not recommended to enable it
in production build of BIOS.

This patch has passed following validations:

  a. Boot to shell (OVMF, Intel real platform)(32/64)
  b. Boot to Fedora 25 (64)

NT32 emulation platform was not validated with this feature enabled
due to the fact that it doesn't support paging which is needed for
this feature to work. But all are validated with feature is disabled.

Suggested-by: Ayellet Wolman 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 

Jian J Wang (7):
  MdeModulePkg/MdeModulePkg.dec,.uni: Add Protocol, PCDs and string
tokens
  MdeModulePkg/SmmMemoryAttribute.h: Add new protocol definitions
  UefiCpuPkg/CpuDxe: Reduce debug message
  MdeModulePkg/DxeIpl: Enable paging for heap guard
  MdeModulePkg/DxeCore: Implement heap guard feature for UEFI
  UefiCpuPkg/PiSmmCpuDxeSmm: Add SmmMemoryAttribute protocol
  MdeModulePkg/PiSmmCore: Implement heap guard feature for SMM mode

 MdeModulePkg/Core/Dxe/DxeMain.inf  |4 +
 MdeModulePkg/Core/Dxe/Mem/HeapGuard.c  | 1182 
 MdeModulePkg/Core/Dxe/Mem/HeapGuard.h  |  394 ++
 MdeModulePkg/Core/Dxe/Mem/Imem.h   |   38 +-
 MdeModulePkg/Core/Dxe/Mem/Page.c   |  130 +-
 MdeModulePkg/Core/Dxe/Mem/Pool.c   |  154 +-
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf|1 +
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c|   36 +-
 MdeModulePkg/Core/PiSmmCore/HeapGuard.c| 1467 
 MdeModulePkg/Core/PiSmmCore/HeapGuard.h|  398 ++
 MdeModulePkg/Core/PiSmmCore/Page.c |   52 +-
 MdeModulePkg/Core/PiSmmCore/PiSmmCore.c|7 +-
 MdeModulePkg/Core/PiSmmCore/PiSmmCore.h|   81 +-
 MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf  |6 +
 MdeModulePkg/Core/PiSmmCore/Pool.c |   81 +-
 MdeModulePkg/Include/Protocol/SmmMemoryAttribute.h |  136 ++
 MdeModulePkg/MdeModulePkg.dec  |   60 +
 MdeModulePkg/MdeModulePkg.uni  |   58 +
 UefiCpuPkg/CpuDxe/CpuPageTable.c   |5 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c   |   10 +
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c |   20 +
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |   98 ++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf   |2 +
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c |  163 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c|   15 +-
 25 files changed, 4499 insertions(+), 99 deletions(-)
 create mode 100644 MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
 create mode 100644 MdeModulePkg/Core/Dxe/Mem/HeapGuard.h
 create mode 100644 MdeModulePkg/Core/PiSmmCore/HeapGuard.c
 create mode 100644 MdeModulePkg/Core/PiSmmCore/HeapGuard.h
 create mode 100644 MdeModulePkg/Include/Protocol/SmmMemoryAttribute.h

-- 
2.14.1.windows.1

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

[edk2] [PATCH v5 2/7] MdeModulePkg/SmmMemoryAttribute.h: Add new protocol definitions

[Jian J Wang]

> v5
> a. Remove EFI wording in comment for SMM memory attribute protocol.
> b. Change parameter modifier from IN to OUT

The new protocol gEdkiiSmmMemoryAttributeProtocolGuid is intended for
PiSmmCore to be able to change memory page attributes for the sake of
heap guard feature.

This protocol provides three interfaces to get/set/clear page attribute.

struct _EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL {
  EDKII_SMM_GET_MEMORY_ATTRIBUTES   GetMemoryAttributes;
  EDKII_SMM_SET_MEMORY_ATTRIBUTES   SetMemoryAttributes;
  EDKII_SMM_CLEAR_MEMORY_ATTRIBUTES ClearMemoryAttributes;
};

Cc: Star Zeng 
Cc: Eric Dong 
Cc: Jiewen Yao 
Cc: Ruiyu Ni 
Suggested-by: Ayellet Wolman 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
Reviewed-by: Jiewen Yao 
Regression-tested-by: Laszlo Ersek 
---
 MdeModulePkg/Include/Protocol/SmmMemoryAttribute.h | 136 +
 1 file changed, 136 insertions(+)
 create mode 100644 MdeModulePkg/Include/Protocol/SmmMemoryAttribute.h

diff --git a/MdeModulePkg/Include/Protocol/SmmMemoryAttribute.h 
b/MdeModulePkg/Include/Protocol/SmmMemoryAttribute.h
new file mode 100644
index 00..a8c10c07b4
--- /dev/null
+++ b/MdeModulePkg/Include/Protocol/SmmMemoryAttribute.h
@@ -0,0 +1,136 @@
+/** @file
+  EFI SMM Memory Attribute Protocol provides retrieval and update service
+  for memory attributes in EFI SMM environment.
+
+  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
+  which accompanies this distribution.  The full text of the license may be 
found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#ifndef __SMM_MEMORYATTRIBUTE_H__
+#define __SMM_MEMORYATTRIBUTE_H__
+
+//{69B792EA-39CE-402D-A2A6-F721DE351DFE}
+#define EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL_GUID \
+  { \
+0x69b792ea, 0x39ce, 0x402d, { 0xa2, 0xa6, 0xf7, 0x21, 0xde, 0x35, 0x1d, 
0xfe } \
+  }
+
+typedef struct _EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL 
EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL;
+
+/**
+  This function set given attributes of the memory region specified by
+  BaseAddress and Length.
+
+  @param  This  The EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL instance.
+  @param  BaseAddress   The physical address that is the start address of
+a memory region.
+  @param  LengthThe size in bytes of the memory region.
+  @param  AttributesThe bit mask of attributes to set for the memory
+region.
+
+  @retval EFI_SUCCESS   The attributes were set for the memory region.
+  @retval EFI_INVALID_PARAMETER Length is zero.
+Attributes specified an illegal combination of
+attributes that cannot be set together.
+  @retval EFI_UNSUPPORTED   The processor does not support one or more
+bytes of the memory resource range specified
+by BaseAddress and Length.
+The bit mask of attributes is not support for
+the memory resource range specified by
+BaseAddress and Length.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EDKII_SMM_SET_MEMORY_ATTRIBUTES)(
+  IN  EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL *This,
+  IN  EFI_PHYSICAL_ADDRESSBaseAddress,
+  IN  UINT64  Length,
+  IN  UINT64  Attributes
+  );
+
+/**
+  This function clears given attributes of the memory region specified by
+  BaseAddress and Length.
+
+  @param  This  The EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL instance.
+  @param  BaseAddress   The physical address that is the start address of
+a memory region.
+  @param  LengthThe size in bytes of the memory region.
+  @param  AttributesThe bit mask of attributes to set for the memory
+region.
+
+  @retval EFI_SUCCESS   The attributes were set for the memory region.
+  @retval EFI_INVALID_PARAMETER Length is zero.
+Attributes specified an illegal combination of
+attributes that cannot be set together.
+  @retval EFI_UNSUPPORTED   The processor does not support one or more
+bytes of the memory resource range specified
+by BaseAddress and Length.
+The bit mask of attributes is not 

[edk2] [PATCH v5 6/7] UefiCpuPkg/PiSmmCpuDxeSmm: Add SmmMemoryAttribute protocol

[Jian J Wang]

> v5
> a. Change parameter modifier from IN to OUT.
> b. Add ASSERT to make sure static paging and heap guard are not enabled at
>the same time.

> v4
> a. According to Ray's feedback, change the definition name prefix from EFI_
>to EDKII_
> b. Coding style cleanup
> c. Add more comments for the use of PcdHeapGuardPropertyMask

> v3
> According to Jiewen's feedback, implement new protocol
> gEdkiiSmmMemoryAttributeProtocolGuid
> to change memory attributes.

> v2
> According to Eric's feedback:
> a. Enclose bit-or with parentheses
> b. Add code in 32-bit code to bypass setting page table to read-only

Heap guard makes use of paging mechanism to implement its functionality. But
there's no protocol or library available to change page attribute in SMM mode.
A new protocol gEdkiiSmmMemoryAttributeProtocolGuid is introduced to make it
happen. This protocol provide three interfaces

struct _EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL {
  EDKII_SMM_GET_MEMORY_ATTRIBUTES   GetMemoryAttributes;
  EDKII_SMM_SET_MEMORY_ATTRIBUTES   SetMemoryAttributes;
  EDKII_SMM_CLEAR_MEMORY_ATTRIBUTES ClearMemoryAttributes;
};

Since heap guard feature need to update page attributes. The page table
should not set to be read-only if heap guard feature is enabled for SMM
mode. Otherwise this feature cannot work.

Cc: Eric Dong 
Cc: Jiewen Yao 
Cc: Laszlo Ersek 
Cc: Ruiyu Ni 
Suggested-by: Ayellet Wolman 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
Reviewed-by: Jiewen Yao 
Regression-tested-by: Laszlo Ersek 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c   |  10 ++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c |  20 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h |  98 +
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf   |   2 +
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 163 +
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c|  15 +-
 6 files changed, 307 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
index 641a1d69a2..0396f2daaa 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
@@ -196,6 +196,16 @@ SetPageTableAttributes (
   BOOLEAN   IsSplitted;
   BOOLEAN   PageTableSplitted;
 
+  //
+  // Don't mark page table as read-only if heap guard is enabled.
+  //
+  //  BIT2: SMM page guard enabled
+  //  BIT3: SMM pool guard enabled
+  //
+  if ((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT3 | BIT2)) != 0) {
+return ;
+  }
+
   DEBUG ((DEBUG_INFO, "SetPageTableAttributes\n"));
 
   //
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index 282d2e6981..4b66a0dfd9 100755
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -76,6 +76,15 @@ EFI_SMM_CPU_PROTOCOL  mSmmCpu  = {
   SmmWriteSaveState
 };
 
+///
+/// SMM Memory Attribute Protocol instance
+///
+EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL  mSmmMemoryAttribute  = {
+  EdkiiSmmGetMemoryAttributes,
+  EdkiiSmmSetMemoryAttributes,
+  EdkiiSmmClearMemoryAttributes
+};
+
 EFI_CPU_INTERRUPT_HANDLER   mExternalVectorTable[EXCEPTION_VECTOR_NUMBER];
 
 //
@@ -893,6 +902,17 @@ PiCpuSmmEntry (
 );
   ASSERT_EFI_ERROR (Status);
 
+  //
+  // Install the SMM Memory Attribute Protocol into SMM protocol database
+  //
+  Status = gSmst->SmmInstallProtocolInterface (
+,
+,
+EFI_NATIVE_INTERFACE,
+
+);
+  ASSERT_EFI_ERROR (Status);
+
   //
   // Expose address of CPU Hot Plug Data structure if CPU hot plug is 
supported.
   //
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 1cf85c1481..ef32f17676 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -25,6 +25,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -1068,4 +1069,101 @@ TransferApToSafeState (
   IN UINTN  NumberToFinishAddress
   );
 
+/**
+  This function set given attributes of the memory region specified by
+  BaseAddress and Length.
+
+  @param  This  The EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL instance.
+  @param  BaseAddress   The physical address that is the start address of
+a memory region.
+  @param  LengthThe size in bytes of the memory region.
+  @param  AttributesThe bit mask of attributes to set for the memory
+region.
+
+  @retval EFI_SUCCESS   The 

[edk2] [PATCH v5 7/7] MdeModulePkg/PiSmmCore: Implement heap guard feature for SMM mode

[Jian J Wang]

> v5
> a. Remove lib and pcd no longer needed from PiSmmCore.inf

> v4
> a. According to Ray's feedback, change the name of new protocol definitions
 from EFI_ to EDKII_.
> b. Coding style cleanup

> v3
> According to Jiewen's feedback, use new added protocol
> gEdkiiSmmMemoryAttributeProtocolGuid
> to update memory attribute.

>
> v2
> According to Eric's feedback:
> a. Remove local variable initializer with memory copy from globals
> b. Change map table dump code to use DEBUG_PAGE|DEBUG_POOL level
>message
> c. Remove unnecessary debug code
> d. Change name of function InitializePageTableLib to
>InitializePageTableGlobals
>
> Other changes:
> e. Fix issues in 32-bit boot mode
> f. Coding style cleanup

This feature makes use of paging mechanism to add a hidden (not present)
page just before and after the allocated memory block. If the code tries
to access memory outside of the allocated part, page fault exception will
be triggered.

This feature is controlled by three PCDs:

gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask
gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPoolType
gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPageType

BIT2 and BIT3 of PcdHeapGuardPropertyMask can be used to enable or disable
memory guard for SMM page and pool respectively. PcdHeapGuardPoolType and/or
PcdHeapGuardPageType are used to enable or disable guard for specific type
of memory. For example, we can turn on guard only for EfiRuntimeServicesCode
and EfiRuntimeServicesData by setting the PCD with value 0x60.

Pool memory is not ususally integer multiple of one page, and is more likely
less than a page. There's no way to monitor the overflow at both top and
bottom of pool memory. BIT7 of PcdHeapGuardPropertyMask is used to control
how to position the head of pool memory so that it's easier to catch memory
overflow in memory growing direction or in decreasing direction.

Cc: Eric Dong 
Cc: Jiewen Yao 
Cc: Laszlo Ersek 
Cc: Ruiyu Ni 
Suggested-by: Ayellet Wolman 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
Reviewed-by: Jiewen Yao 
Regression-tested-by: Laszlo Ersek 
---
 MdeModulePkg/Core/PiSmmCore/HeapGuard.c   | 1467 +
 MdeModulePkg/Core/PiSmmCore/HeapGuard.h   |  398 
 MdeModulePkg/Core/PiSmmCore/Page.c|   52 +-
 MdeModulePkg/Core/PiSmmCore/PiSmmCore.c   |7 +-
 MdeModulePkg/Core/PiSmmCore/PiSmmCore.h   |   81 +-
 MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf |6 +
 MdeModulePkg/Core/PiSmmCore/Pool.c|   81 +-
 7 files changed, 2064 insertions(+), 28 deletions(-)
 create mode 100644 MdeModulePkg/Core/PiSmmCore/HeapGuard.c
 create mode 100644 MdeModulePkg/Core/PiSmmCore/HeapGuard.h

diff --git a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c 
b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
new file mode 100644
index 00..8dd109b619
--- /dev/null
+++ b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
@@ -0,0 +1,1467 @@
+/** @file
+  UEFI Heap Guard functions.
+
+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
+which accompanies this distribution.  The full text of the license may be 
found at
+http://opensource.org/licenses/bsd-license.php
+
+THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include "HeapGuard.h"
+
+//
+// Global to avoid infinite reentrance of memory allocation when updating
+// page table attributes, which may need allocating pages for new PDE/PTE.
+//
+GLOBAL_REMOVE_IF_UNREFERENCED BOOLEAN mOnGuarding = FALSE;
+
+//
+// Pointer to table tracking the Guarded memory with bitmap, in which  '1'
+// is used to indicate memory guarded. '0' might be free memory or Guard
+// page itself, depending on status of memory adjacent to it.
+//
+GLOBAL_REMOVE_IF_UNREFERENCED UINT64 mGuardedMemoryMap = 0;
+
+//
+// Current depth level of map table pointed by mGuardedMemoryMap.
+// mMapLevel must be initialized at least by 1. It will be automatically
+// updated according to the address of memory just tracked.
+//
+GLOBAL_REMOVE_IF_UNREFERENCED UINTN mMapLevel = 1;
+
+//
+// Shift and mask for each level of map table
+//
+GLOBAL_REMOVE_IF_UNREFERENCED UINTN mLevelShift[GUARDED_HEAP_MAP_TABLE_DEPTH]
+= GUARDED_HEAP_MAP_TABLE_DEPTH_SHIFTS;
+GLOBAL_REMOVE_IF_UNREFERENCED UINTN mLevelMask[GUARDED_HEAP_MAP_TABLE_DEPTH]
+= GUARDED_HEAP_MAP_TABLE_DEPTH_MASKS;
+
+//
+// SMM memory attribute protocol
+//
+EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL *mSmmMemoryAttribute = NULL;
+
+/**
+  Set corresponding bits in bitmap table to 1 according to the 

[edk2] [PATCH v5 5/7] MdeModulePkg/DxeCore: Implement heap guard feature for UEFI

[Jian J Wang]

> v4
> Coding style cleanup

> v3
> Fix build error with GCC toolchain

> v2
> According to Eric's feedback:
> a. Remove local variable initializer with memory copy from globals
> b. Add comment for the use of mOnGuarding
> c. Change map table dump code to use DEBUG_PAGE|DEBUG_POOL level
>message
>
> Other changes:
> d. Fix issues in 32-bit boot mode
> e. Remove prototype of empty functions
>

This feature makes use of paging mechanism to add a hidden (not present)
page just before and after the allocated memory block. If the code tries
to access memory outside of the allocated part, page fault exception will
be triggered.

This feature is controlled by three PCDs:

gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask
gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPoolType
gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPageType

BIT0 and BIT1 of PcdHeapGuardPropertyMask can be used to enable or disable
memory guard for page and pool respectively. PcdHeapGuardPoolType and/or
PcdHeapGuardPageType are used to enable or disable guard for specific type
of memory. For example, we can turn on guard only for EfiBootServicesData
and EfiRuntimeServicesData by setting the PCD with value 0x50.

Pool memory is not ususally integer multiple of one page, and is more likely
less than a page. There's no way to monitor the overflow at both top and
bottom of pool memory. BIT7 of PcdHeapGuardPropertyMask is used to control
how to position the head of pool memory so that it's easier to catch memory
overflow in memory growing direction or in decreasing direction.

Note1: Turning on heap guard, especially pool guard, will introduce too many
memory fragments. Windows 10 has a limitation in its boot loader, which
accepts at most 512 memory descriptors passed from BIOS. This will prevent
Windows 10 from booting if heap guard is enabled. The latest Linux
distribution with grub boot loader has no such issue. Normally it's not
recommended to enable this feature in production build of BIOS.

Note2: Don't enable this feature for NT32 emulation platform which doesn't
support paging.

Cc: Star Zeng 
Cc: Eric Dong 
Cc: Jiewen Yao 
Cc: Michael Kinney 
Suggested-by: Ayellet Wolman 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
Reviewed-by: Jiewen Yao 
Regression-tested-by: Laszlo Ersek 
---
 MdeModulePkg/Core/Dxe/DxeMain.inf |4 +
 MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 1182 +
 MdeModulePkg/Core/Dxe/Mem/HeapGuard.h |  394 +++
 MdeModulePkg/Core/Dxe/Mem/Imem.h  |   38 +-
 MdeModulePkg/Core/Dxe/Mem/Page.c  |  130 +++-
 MdeModulePkg/Core/Dxe/Mem/Pool.c  |  154 -
 6 files changed, 1838 insertions(+), 64 deletions(-)
 create mode 100644 MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
 create mode 100644 MdeModulePkg/Core/Dxe/Mem/HeapGuard.h

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf 
b/MdeModulePkg/Core/Dxe/DxeMain.inf
index 15f4b03d3c..f2155fcab1 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -56,6 +56,7 @@
   Mem/MemData.c
   Mem/Imem.h
   Mem/MemoryProfileRecord.c
+  Mem/HeapGuard.c
   FwVolBlock/FwVolBlock.c
   FwVolBlock/FwVolBlock.h
   FwVol/FwVolWrite.c
@@ -193,6 +194,9 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy   ## 
CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ## 
CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask## 
CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPageType   ## 
CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPoolType   ## 
CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask   ## 
CONSUMES
 
 # [Hob]
 # RESOURCE_DESCRIPTOR   ## CONSUMES
diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c 
b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
new file mode 100644
index 00..55e29f4ded
--- /dev/null
+++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
@@ -0,0 +1,1182 @@
+/** @file
+  UEFI Heap Guard functions.
+
+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
+which accompanies this distribution.  The full text of the license may be 
found at
+http://opensource.org/licenses/bsd-license.php
+
+THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include "DxeMain.h"
+#include "Imem.h"
+#include "HeapGuard.h"
+
+//
+// Global to avoid infinite reentrance of memory allocation when updating
+// page table attributes, which may need allocate pages for new PDE/PTE.
+//

[edk2] [PATCH v5 1/7] MdeModulePkg/MdeModulePkg.dec, .uni: Add Protocol, PCDs and string tokens

[Jian J Wang]

>v4:
> No content change but move this patch to be the first one

Cc: Star Zeng 
Cc: Eric Dong 
Cc: Jiewen Yao 
Suggested-by: Ayellet Wolman 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
Reviewed-by: Jiewen Yao 
Regression-tested-by: Laszlo Ersek 
---
 MdeModulePkg/MdeModulePkg.dec | 60 +++
 MdeModulePkg/MdeModulePkg.uni | 58 +
 2 files changed, 118 insertions(+)

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 6f46d595de..856d67aceb 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -559,6 +559,9 @@
   ## Include/Protocol/SmmEndofS3Resume.h
   gEdkiiSmmEndOfS3ResumeProtocolGuid = { 0x96f5296d, 0x05f7, 0x4f3c, {0x84, 
0x67, 0xe4, 0x56, 0x89, 0x0e, 0x0c, 0xb5 } }
 
+  ## Include/Protocol/SmmMemoryAttribute.h
+  gEdkiiSmmMemoryAttributeProtocolGuid = { 0x69b792ea, 0x39ce, 0x402d, { 0xa2, 
0xa6, 0xf7, 0x21, 0xde, 0x35, 0x1d, 0xfe } }
+
 #
 # [Error.gEfiMdeModulePkgTokenSpaceGuid]
 #   0x8001 | Invalid value provided.
@@ -889,6 +892,63 @@
   # @Prompt Init Value in Temp Stack
   
gEfiMdeModulePkgTokenSpaceGuid.PcdInitValueInTempStack|0x5AA55AA5|UINT32|0x30001051
 
+  ## Indicates which type allocation need guard page.
+  # Below is bit mask for this PCD: (Order is same as UEFI spec)
+  #  EfiReservedMemoryType 0x0001
+  #  EfiLoaderCode 0x0002
+  #  EfiLoaderData 0x0004
+  #  EfiBootServicesCode   0x0008
+  #  EfiBootServicesData   0x0010
+  #  EfiRuntimeServicesCode0x0020
+  #  EfiRuntimeServicesData0x0040
+  #  EfiConventionalMemory 0x0080
+  #  EfiUnusableMemory 0x0100
+  #  EfiACPIReclaimMemory  0x0200
+  #  EfiACPIMemoryNVS  0x0400
+  #  EfiMemoryMappedIO 0x0800
+  #  EfiMemoryMappedIOPortSpace0x1000
+  #  EfiPalCode0x2000
+  #  EfiPersistentMemory   0x4000
+  #  OEM Reserved  0x4000
+  #  OS Reserved   0x8000
+  # e.g. LoaderCode+LoaderData+BootServicesCode+BootServicesData are needed, 
0x1E should be used.
+  # @Prompt The memory type mask for Page Guard.
+  gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPageType|0x0|UINT64|0x30001052
+
+  ## Indicates which type allocation need guard page.
+  # Below is bit mask for this PCD: (Order is same as UEFI spec)
+  #  EfiReservedMemoryType 0x0001
+  #  EfiLoaderCode 0x0002
+  #  EfiLoaderData 0x0004
+  #  EfiBootServicesCode   0x0008
+  #  EfiBootServicesData   0x0010
+  #  EfiRuntimeServicesCode0x0020
+  #  EfiRuntimeServicesData0x0040
+  #  EfiConventionalMemory 0x0080
+  #  EfiUnusableMemory 0x0100
+  #  EfiACPIReclaimMemory  0x0200
+  #  EfiACPIMemoryNVS  0x0400
+  #  EfiMemoryMappedIO 0x0800
+  #  EfiMemoryMappedIOPortSpace0x1000
+  #  EfiPalCode0x2000
+  #  EfiPersistentMemory   0x4000
+  #  OEM Reserved  0x4000
+  #  OS Reserved   0x8000
+  # e.g. LoaderCode+LoaderData+BootServicesCode+BootServicesData are needed, 
0x1E should be used.
+  # @Prompt The memory type mask for Pool Guard.
+  gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPoolType|0x0|UINT64|0x30001053
+
+  ## This mask is to control Heap Guard behavior.
+  #   BIT0 - Enable UEFI page guard.
+  #   BIT1 - Enable UEFI pool guard.
+  #   BIT2 - Enable SMM page guard.
+  #   BIT3 - Enable SMM pool guard.
+  #   BIT7 - The direction of Guard Page for Pool Guard.
+  #  0 - The returned pool is adjacent to the bottom guard page.
+  #  1 - The returned pool is adjacent to the top guard page.
+  # @Prompt The Heap Guard feature mask
+  gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask|0x0|UINT8|0x30001054
+
 [PcdsFixedAtBuild, PcdsPatchableInModule]
   ## Dynamic type PCD can be registered callback function for Pcd setting 
action.
   #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of 
callback function
diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni
index ce9de4897a..588905a9a1 100644
--- 

[edk2] [PATCH v5 3/7] UefiCpuPkg/CpuDxe: Reduce debug message

[Jian J Wang]

Heap guard feature will frequently update page attributes. The debug message
in CpuDxe driver will slow down the boot performance noticeably. Changing the
debug level to DEBUG_POOL and DEBUG_PAGE to reduce the message output for
normal debug configuration.

Cc: Eric Dong 
Cc: Jiewen Yao 
Suggested-by: Ayellet Wolman 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
Reviewed-by: Jiewen Yao 
Regression-tested-by: Laszlo Ersek 
---
 UefiCpuPkg/CpuDxe/CpuPageTable.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index d312eb66f8..5270a1124f 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -442,8 +442,9 @@ ConvertPageEntryAttribute (
   *PageEntry = NewPageEntry;
   if (CurrentPageEntry != NewPageEntry) {
 *IsModified = TRUE;
-DEBUG ((DEBUG_INFO, "ConvertPageEntryAttribute 0x%lx", CurrentPageEntry));
-DEBUG ((DEBUG_INFO, "->0x%lx\n", NewPageEntry));
+DEBUG ((DEBUG_POOL | DEBUG_PAGE, "ConvertPageEntryAttribute 0x%lx",
+CurrentPageEntry));
+DEBUG ((DEBUG_POOL | DEBUG_PAGE, "->0x%lx\n", NewPageEntry));
   } else {
 *IsModified = FALSE;
   }
-- 
2.14.1.windows.1

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

[edk2] [PATCH v5 4/7] MdeModulePkg/DxeIpl: Enable paging for heap guard

[Jian J Wang]

Heap guard feature needs paging to work properly. 64-bit BIOS uses
PcdDxeIplBuildPageTables to control the page table setup. 32-bit BIOS
has to check heap guard feature to decide enabling paging or not.

Cc: Star Zeng 
Cc: Eric Dong 
Cc: Jiewen Yao 
Suggested-by: Ayellet Wolman 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
Reviewed-by: Jiewen Yao 
Regression-tested-by: Laszlo Ersek 
---
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf |  1 +
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c | 36 ++---
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf 
b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index 9d0e76a293..a1b8748432 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -116,6 +116,7 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable  ## 
SOMETIMES_CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask## 
CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask## 
CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask   ## 
CONSUMES
 
 [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack   ## 
SOMETIMES_CONSUMES
diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c 
b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
index 96f5718444..5649265367 100644
--- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
@@ -211,6 +211,37 @@ IsExecuteDisableBitAvailable (
   return Available;
 }
 
+/**
+  The function will check if page table should be setup or not.
+
+  @retval TRUE  Page table should be created.
+  @retval FALSE Page table should not be created.
+
+**/
+BOOLEAN
+ToBuildPageTable (
+  VOID
+  )
+{
+  if (!IsIa32PaeSupport ()) {
+return FALSE;
+  }
+
+  if (IsNullDetectionEnabled ()) {
+return TRUE;
+  }
+
+  if (PcdGet8 (PcdHeapGuardPropertyMask) != 0) {
+return TRUE;
+  }
+
+  if (PcdGetBool (PcdSetNxForStack) && IsExecuteDisableBitAvailable ()) {
+return TRUE;
+  }
+
+  return FALSE;
+}
+
 /**
Transfers control to DxeCore.
 
@@ -385,10 +416,7 @@ HandOffToDxeCore (
 TopOfStack = (EFI_PHYSICAL_ADDRESS) (UINTN) ALIGN_POINTER (TopOfStack, 
CPU_STACK_ALIGNMENT);
 
 PageTables = 0;
-BuildPageTablesIa32Pae = (BOOLEAN) (IsIa32PaeSupport () &&
-(IsNullDetectionEnabled () ||
- (PcdGetBool (PcdSetNxForStack) &&
-  IsExecuteDisableBitAvailable (;
+BuildPageTablesIa32Pae = ToBuildPageTable ();
 if (BuildPageTablesIa32Pae) {
   PageTables = Create4GPageTablesIa32Pae (BaseOfStack, STACK_SIZE);
   if (IsExecuteDisableBitAvailable ()) {
-- 
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 V2 0/5] Fix MSFT C4255 warning

[Gao, Liming]

Reviewed-by: Liming Gao 

>-Original Message-
>From: Song, BinX
>Sent: Friday, November 10, 2017 12:10 PM
>To: edk2-devel@lists.01.org
>Cc: Gao, Liming 
>Subject: [PATCH V2 0/5] Fix MSFT C4255 warning
>
>V2:
>Fix MSFT C4255 warning.
>V1:
>Enable MSFT C4255 warning.
>
>From MSDN:
>Compiler Warning (level 4) C4255
>function' : no function prototype given: converting '()' to '(void)'
>The compiler did not find an explicit list of arguments to a function.
>This warning is for the C compiler only.
>
>Cc: Liming Gao 
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Bell Song 
>---
>Bell Song (5):
>  IntelFrameworkModulePkg: Fix MSFT C4255 warning
>  MdeModulePkg: Fix MSFT C4255 warning
>  MdePkg: Fix MSFT C4255 warning
>  NetworkPkg: Fix MSFT C4255 warning
>  ShellPkg: Fix MSFT C4255 warning
>
> .../BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLi
>b.c | 1 +
> .../Library/LzmaCustomDecompressLib/F86GuidedSectionExtraction.c| 1
>+
> .../Library/LzmaCustomDecompressLib/GuidedSectionExtraction.c   | 1 +
> MdeModulePkg/Include/Library/PlatformVarCleanupLib.h| 1 +
> .../Library/BrotliCustomDecompressLib/GuidedSectionExtraction.c | 1 +
> .../Library/DxeSecurityManagementLib/DxeSecurityManagementLib.c | 2
>++
> .../Library/LzmaCustomDecompressLib/F86GuidedSectionExtraction.c| 1
>+
>
>MdeModulePkg/Library/LzmaCustomDecompressLib/GuidedSectionExtractio
>n.c  | 1 +
> MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c  | 1
>+
> MdeModulePkg/Universal/SetupBrowserDxe/Expression.c | 1 +
> MdePkg/Library/BaseLib/Ia32/CpuBreakpoint.c | 2 +-
> MdePkg/Library/BaseLib/X64/CpuBreakpoint.c  | 2 +-
> MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.c
>| 1 +
> NetworkPkg/IScsiDxe/IScsiDriver.c   | 1 +
> ShellPkg/Application/Shell/Shell.c  | 1 +
> ShellPkg/Application/Shell/ShellProtocol.c  | 2 ++
> ShellPkg/Library/UefiShellLib/UefiShellLib.c| 1 +
> 17 files changed, 19 insertions(+), 2 deletions(-)
>
>--
>2.10.2.windows.1

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

[edk2] [PATCH V2 3/5] MdePkg: Fix MSFT C4255 warning

[Song, BinX]

V2:
Fix MSFT C4255 warning
V1:
Enable MSFT C4255 warning.

>From MSDN:
Compiler Warning (level 4) C4255
function' : no function prototype given: converting '()' to '(void)'
The compiler did not find an explicit list of arguments to a function.
This warning is for the C compiler only.

Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Bell Song 
---
 MdePkg/Library/BaseLib/Ia32/CpuBreakpoint.c| 2 +-
 MdePkg/Library/BaseLib/X64/CpuBreakpoint.c | 2 +-
 MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.c | 1 +
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Library/BaseLib/Ia32/CpuBreakpoint.c 
b/MdePkg/Library/BaseLib/Ia32/CpuBreakpoint.c
index f5df7f2..4f8c92d 100644
--- a/MdePkg/Library/BaseLib/Ia32/CpuBreakpoint.c
+++ b/MdePkg/Library/BaseLib/Ia32/CpuBreakpoint.c
@@ -19,7 +19,7 @@
   Microsoft Visual Studio 7.1 Function Prototypes for I/O Intrinsics.
 **/
 
-void __debugbreak ();
+void __debugbreak (VOID);
 
 #pragma intrinsic(__debugbreak)
 
diff --git a/MdePkg/Library/BaseLib/X64/CpuBreakpoint.c 
b/MdePkg/Library/BaseLib/X64/CpuBreakpoint.c
index d654f84..7d0cf4c 100644
--- a/MdePkg/Library/BaseLib/X64/CpuBreakpoint.c
+++ b/MdePkg/Library/BaseLib/X64/CpuBreakpoint.c
@@ -17,7 +17,7 @@
   Microsoft Visual Studio 7.1 Function Prototypes for I/O Intrinsics.
 **/
 
-void __debugbreak ();
+void __debugbreak (VOID);
 
 #pragma intrinsic(__debugbreak)
 
diff --git 
a/MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.c 
b/MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.c
index 8b8e528..744201b 100644
--- a/MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.c
+++ b/MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.c
@@ -38,6 +38,7 @@ EXTRACT_GUIDED_SECTION_GET_INFO_HANDLER 
*mExtractGetInfoHandlerTable = NULL;
 RETURN_STATUS
 EFIAPI
 ReallocateExtractHandlerTable (
+  VOID
   )
 { 
   //
-- 
2.10.2.windows.1

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

[edk2] [PATCH V2 5/5] ShellPkg: Fix MSFT C4255 warning

[Song, BinX]

V2:
Fix MSFT C4255 warning
V1:
Enable MSFT C4255 warning.

>From MSDN:
Compiler Warning (level 4) C4255
function' : no function prototype given: converting '()' to '(void)'
The compiler did not find an explicit list of arguments to a function.
This warning is for the C compiler only.

Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Bell Song 
---
 ShellPkg/Application/Shell/Shell.c   | 1 +
 ShellPkg/Application/Shell/ShellProtocol.c   | 2 ++
 ShellPkg/Library/UefiShellLib/UefiShellLib.c | 1 +
 3 files changed, 4 insertions(+)

diff --git a/ShellPkg/Application/Shell/Shell.c 
b/ShellPkg/Application/Shell/Shell.c
index 656206f..2adc992 100644
--- a/ShellPkg/Application/Shell/Shell.c
+++ b/ShellPkg/Application/Shell/Shell.c
@@ -726,6 +726,7 @@ FreeResources:
 **/
 EFI_STATUS
 SetBuiltInAlias(
+  VOID
   )
 {
   EFI_STATUS  Status;
diff --git a/ShellPkg/Application/Shell/ShellProtocol.c 
b/ShellPkg/Application/Shell/ShellProtocol.c
index 5e34b8d..dc3deee 100644
--- a/ShellPkg/Application/Shell/ShellProtocol.c
+++ b/ShellPkg/Application/Shell/ShellProtocol.c
@@ -1679,6 +1679,7 @@ InternalShellExecute(
 STATIC
 BOOLEAN
 NestingEnabled(
+  VOID
 )
 {
   EFI_STATUS  Status;
@@ -3286,6 +3287,7 @@ EfiShellIsRootShell(
 **/
 CHAR16 *
 InternalEfiShellGetListAlias(
+  VOID
   )
 {
   
diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c 
b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
index 25d3e33..677791c 100644
--- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
+++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
@@ -369,6 +369,7 @@ ShellLibDestructor (
 EFI_STATUS
 EFIAPI
 ShellInitialize (
+  VOID
   )
 {
   EFI_STATUS Status;
-- 
2.10.2.windows.1

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

[edk2] [PATCH V2 2/5] MdeModulePkg: Fix MSFT C4255 warning

[Song, BinX]

V2:
Fix MSFT C4255 warning
V1:
Enable MSFT C4255 warning.

>From MSDN:
Compiler Warning (level 4) C4255
function' : no function prototype given: converting '()' to '(void)'
The compiler did not find an explicit list of arguments to a function.
This warning is for the C compiler only.

Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Bell Song 
---
 MdeModulePkg/Include/Library/PlatformVarCleanupLib.h| 1 +
 .../Library/BrotliCustomDecompressLib/GuidedSectionExtraction.c | 1 +
 .../Library/DxeSecurityManagementLib/DxeSecurityManagementLib.c | 2 ++
 .../Library/LzmaCustomDecompressLib/F86GuidedSectionExtraction.c| 1 +
 MdeModulePkg/Library/LzmaCustomDecompressLib/GuidedSectionExtraction.c  | 1 +
 MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c  | 1 +
 MdeModulePkg/Universal/SetupBrowserDxe/Expression.c | 1 +
 7 files changed, 8 insertions(+)

diff --git a/MdeModulePkg/Include/Library/PlatformVarCleanupLib.h 
b/MdeModulePkg/Include/Library/PlatformVarCleanupLib.h
index a4691f0..1d642f6 100644
--- a/MdeModulePkg/Include/Library/PlatformVarCleanupLib.h
+++ b/MdeModulePkg/Include/Library/PlatformVarCleanupLib.h
@@ -32,6 +32,7 @@ typedef enum {
 VAR_ERROR_FLAG
 EFIAPI
 GetLastBootVarErrorFlag (
+  VOID
   );
 
 /**
diff --git 
a/MdeModulePkg/Library/BrotliCustomDecompressLib/GuidedSectionExtraction.c 
b/MdeModulePkg/Library/BrotliCustomDecompressLib/GuidedSectionExtraction.c
index 427adb8..c623e1e 100644
--- a/MdeModulePkg/Library/BrotliCustomDecompressLib/GuidedSectionExtraction.c
+++ b/MdeModulePkg/Library/BrotliCustomDecompressLib/GuidedSectionExtraction.c
@@ -186,6 +186,7 @@ BrotliGuidedSectionExtraction (
 EFI_STATUS
 EFIAPI
 BrotliDecompressLibConstructor (
+  VOID
   )
 {
   return ExtractGuidedSectionRegisterHandlers (
diff --git 
a/MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.c 
b/MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.c
index b96d786..aa4d93b 100644
--- a/MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.c
+++ b/MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.c
@@ -61,6 +61,7 @@ SECURITY2_INFO *mSecurity2Table  = NULL;
 RETURN_STATUS
 EFIAPI
 ReallocateSecurityHandlerTable (
+  VOID
   )
 {
   //
@@ -301,6 +302,7 @@ ExecuteSecurityHandlers (
 RETURN_STATUS
 EFIAPI
 ReallocateSecurity2HandlerTable (
+  VOID
   )
 {
   //
diff --git 
a/MdeModulePkg/Library/LzmaCustomDecompressLib/F86GuidedSectionExtraction.c 
b/MdeModulePkg/Library/LzmaCustomDecompressLib/F86GuidedSectionExtraction.c
index ada9a80..b02dc51 100644
--- a/MdeModulePkg/Library/LzmaCustomDecompressLib/F86GuidedSectionExtraction.c
+++ b/MdeModulePkg/Library/LzmaCustomDecompressLib/F86GuidedSectionExtraction.c
@@ -207,6 +207,7 @@ LzmaArchGuidedSectionExtraction (
 EFI_STATUS
 EFIAPI
 LzmaArchDecompressLibConstructor (
+  VOID
   )
 {
   return ExtractGuidedSectionRegisterHandlers (
diff --git 
a/MdeModulePkg/Library/LzmaCustomDecompressLib/GuidedSectionExtraction.c 
b/MdeModulePkg/Library/LzmaCustomDecompressLib/GuidedSectionExtraction.c
index f19e0d2..7ef9fbb 100644
--- a/MdeModulePkg/Library/LzmaCustomDecompressLib/GuidedSectionExtraction.c
+++ b/MdeModulePkg/Library/LzmaCustomDecompressLib/GuidedSectionExtraction.c
@@ -190,6 +190,7 @@ LzmaGuidedSectionExtraction (
 EFI_STATUS
 EFIAPI
 LzmaDecompressLibConstructor (
+  VOID
   )
 {
   return ExtractGuidedSectionRegisterHandlers (
diff --git a/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c 
b/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c
index c5fd30e..450b4b3 100644
--- a/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c
+++ b/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c
@@ -1187,6 +1187,7 @@ Done:
 VAR_ERROR_FLAG
 EFIAPI
 GetLastBootVarErrorFlag (
+  VOID
   )
 {
   return mLastVarErrorFlag;
diff --git a/MdeModulePkg/Universal/SetupBrowserDxe/Expression.c 
b/MdeModulePkg/Universal/SetupBrowserDxe/Expression.c
index 901b35c..297741c 100644
--- a/MdeModulePkg/Universal/SetupBrowserDxe/Expression.c
+++ b/MdeModulePkg/Universal/SetupBrowserDxe/Expression.c
@@ -751,6 +751,7 @@ PopExpression (
 **/
 UINTN
 SaveExpressionEvaluationStackOffset (
+  VOID
   )
 {
   UINTN TempStackOffset;
-- 
2.10.2.windows.1

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

[edk2] [PATCH V2 4/5] NetworkPkg: Fix MSFT C4255 warning

[Song, BinX]

V2:
Fix MSFT C4255 warning
V1:
Enable MSFT C4255 warning.

>From MSDN:
Compiler Warning (level 4) C4255
function' : no function prototype given: converting '()' to '(void)'
The compiler did not find an explicit list of arguments to a function.
This warning is for the C compiler only.

Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Bell Song 
---
 NetworkPkg/IScsiDxe/IScsiDriver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/NetworkPkg/IScsiDxe/IScsiDriver.c 
b/NetworkPkg/IScsiDxe/IScsiDriver.c
index fbeef97..a0ece3a 100644
--- a/NetworkPkg/IScsiDxe/IScsiDriver.c
+++ b/NetworkPkg/IScsiDxe/IScsiDriver.c
@@ -87,6 +87,7 @@ IScsiIsDevicePathSupported (
 **/
 EFI_STATUS
 IScsiCheckAip (
+  VOID
   )
 {
   UINTNAipHandleCount;
-- 
2.10.2.windows.1

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

[edk2] [PATCH V2 1/5] IntelFrameworkModulePkg: Fix MSFT C4255 warning

[Song, BinX]

V2:
Fix MSFT C4255 warning
V1:
Enable MSFT C4255 warning.

>From MSDN:
Compiler Warning (level 4) C4255
function' : no function prototype given: converting '()' to '(void)'
The compiler did not find an explicit list of arguments to a function.
This warning is for the C compiler only.

Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Bell Song 
---
 .../BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.c  | 1 +
 .../Library/LzmaCustomDecompressLib/F86GuidedSectionExtraction.c | 1 +
 .../Library/LzmaCustomDecompressLib/GuidedSectionExtraction.c| 1 +
 3 files changed, 3 insertions(+)

diff --git 
a/IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.c
 
b/IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.c
index 5d64f02..cb009e7 100644
--- 
a/IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.c
+++ 
b/IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.c
@@ -1040,6 +1040,7 @@ TianoDecompress (
 RETURN_STATUS
 EFIAPI
 TianoDecompressLibConstructor (
+  VOID
 )
 {
   return ExtractGuidedSectionRegisterHandlers (
diff --git 
a/IntelFrameworkModulePkg/Library/LzmaCustomDecompressLib/F86GuidedSectionExtraction.c
 
b/IntelFrameworkModulePkg/Library/LzmaCustomDecompressLib/F86GuidedSectionExtraction.c
index ada9a80..b02dc51 100644
--- 
a/IntelFrameworkModulePkg/Library/LzmaCustomDecompressLib/F86GuidedSectionExtraction.c
+++ 
b/IntelFrameworkModulePkg/Library/LzmaCustomDecompressLib/F86GuidedSectionExtraction.c
@@ -207,6 +207,7 @@ LzmaArchGuidedSectionExtraction (
 EFI_STATUS
 EFIAPI
 LzmaArchDecompressLibConstructor (
+  VOID
   )
 {
   return ExtractGuidedSectionRegisterHandlers (
diff --git 
a/IntelFrameworkModulePkg/Library/LzmaCustomDecompressLib/GuidedSectionExtraction.c
 
b/IntelFrameworkModulePkg/Library/LzmaCustomDecompressLib/GuidedSectionExtraction.c
index f19e0d2..7ef9fbb 100644
--- 
a/IntelFrameworkModulePkg/Library/LzmaCustomDecompressLib/GuidedSectionExtraction.c
+++ 
b/IntelFrameworkModulePkg/Library/LzmaCustomDecompressLib/GuidedSectionExtraction.c
@@ -190,6 +190,7 @@ LzmaGuidedSectionExtraction (
 EFI_STATUS
 EFIAPI
 LzmaDecompressLibConstructor (
+  VOID
   )
 {
   return ExtractGuidedSectionRegisterHandlers (
-- 
2.10.2.windows.1

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

[edk2] [PATCH V2 0/5] Fix MSFT C4255 warning

[Song, BinX]

V2:
Fix MSFT C4255 warning.
V1:
Enable MSFT C4255 warning.

>From MSDN:
Compiler Warning (level 4) C4255
function' : no function prototype given: converting '()' to '(void)'
The compiler did not find an explicit list of arguments to a function.
This warning is for the C compiler only.

Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Bell Song 
---
Bell Song (5):
  IntelFrameworkModulePkg: Fix MSFT C4255 warning
  MdeModulePkg: Fix MSFT C4255 warning
  MdePkg: Fix MSFT C4255 warning
  NetworkPkg: Fix MSFT C4255 warning
  ShellPkg: Fix MSFT C4255 warning

 .../BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.c | 1 +
 .../Library/LzmaCustomDecompressLib/F86GuidedSectionExtraction.c| 1 +
 .../Library/LzmaCustomDecompressLib/GuidedSectionExtraction.c   | 1 +
 MdeModulePkg/Include/Library/PlatformVarCleanupLib.h| 1 +
 .../Library/BrotliCustomDecompressLib/GuidedSectionExtraction.c | 1 +
 .../Library/DxeSecurityManagementLib/DxeSecurityManagementLib.c | 2 ++
 .../Library/LzmaCustomDecompressLib/F86GuidedSectionExtraction.c| 1 +
 MdeModulePkg/Library/LzmaCustomDecompressLib/GuidedSectionExtraction.c  | 1 +
 MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c  | 1 +
 MdeModulePkg/Universal/SetupBrowserDxe/Expression.c | 1 +
 MdePkg/Library/BaseLib/Ia32/CpuBreakpoint.c | 2 +-
 MdePkg/Library/BaseLib/X64/CpuBreakpoint.c  | 2 +-
 MdePkg/Library/DxeExtractGuidedSectionLib/DxeExtractGuidedSectionLib.c  | 1 +
 NetworkPkg/IScsiDxe/IScsiDriver.c   | 1 +
 ShellPkg/Application/Shell/Shell.c  | 1 +
 ShellPkg/Application/Shell/ShellProtocol.c  | 2 ++
 ShellPkg/Library/UefiShellLib/UefiShellLib.c| 1 +
 17 files changed, 19 insertions(+), 2 deletions(-)

-- 
2.10.2.windows.1

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

Re: [edk2] [PATCH] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

[Wang, Jian J]

Sorry guys. I did update the title but v5 is still missing. A new one will be 
sent out.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jian J
> Wang
> Sent: Friday, November 10, 2017 8:42 AM
> To: edk2-devel@lists.01.org
> Cc: Laszlo Ersek ; Yao, Jiewen ;
> Dong, Eric 
> Subject: [edk2] [PATCH] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in
> memory map
> 
> > v5:
> >Coding style clean-up
> 
> > v4:
> > a. Remove DoUpdate and check attributes mismatch all the time to avoid
> >a logic hole
> > b. Add warning message if failed to update capability
> > c. Add local variable to hold new attributes to make code cleaner
> 
> > v3:
> > a. Add comment to explain more on updating memory capabilities
> > b. Fix logic hole in updating attributes
> > c. Instead of checking illegal memory space address and size, use return
> >status of gDS->SetMemorySpaceCapabilities() to skip memory block which
> >cannot be updated with new capabilities.
> 
> > v2
> > a. Fix an issue which will cause setting capability failure if size is 
> > smaller
> >than a page.
> 
> More than one entry of RT_CODE memory might cause boot problem for some
> old OSs. This patch will fix this issue to keep OS compatibility as much
> as possible.
> 
> More detailed information, please refer to
> https://bugzilla.tianocore.org/show_bug.cgi?id=753
> 
> Cc: Eric Dong 
> Cc: Jiewen Yao 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 69 +--
> -
>  1 file changed, 50 insertions(+), 19 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index d312eb66f8..61537838b7 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -789,8 +789,7 @@ RefreshGcdMemoryAttributesFromPaging (
>UINT64  BaseAddress;
>UINT64  PageStartAddress;
>UINT64  Attributes;
> -  UINT64  Capabilities;
> -  BOOLEAN DoUpdate;
> +  UINT64  NewAttributes;
>UINTN   Index;
> 
>//
> @@ -802,9 +801,8 @@ RefreshGcdMemoryAttributesFromPaging (
> 
>GetCurrentPagingContext ();
> 
> -  DoUpdate  = FALSE;
> -  Capabilities  = 0;
>Attributes= 0;
> +  NewAttributes = 0;
>BaseAddress   = 0;
>PageLength= 0;
> 
> @@ -813,6 +811,34 @@ RefreshGcdMemoryAttributesFromPaging (
>continue;
>  }
> 
> +//
> +// Sync the actual paging related capabilities back to GCD service first.
> +// As a side effect (good one), this can also help to avoid unnecessary
> +// memory map entries due to the different capabilities of the same type
> +// memory, such as multiple RT_CODE and RT_DATA entries in memory map,
> +// which could cause boot failure of some old Linux distro (before v4.3).
> +//
> +Status = gDS->SetMemorySpaceCapabilities (
> +MemorySpaceMap[Index].BaseAddress,
> +MemorySpaceMap[Index].Length,
> +MemorySpaceMap[Index].Capabilities |
> +EFI_MEMORY_PAGETYPE_MASK
> +);
> +if (EFI_ERROR (Status)) {
> +  //
> +  // If we cannot udpate the capabilities, we cannot update its
> +  // attributes either. So just simply skip current block of memory.
> +  //
> +  DEBUG ((
> +DEBUG_WARN,
> +"Failed to update capability: [%lu] %016lx - %016lx (%016lx -> 
> %016lx)\r\n",
> +(UINT64)Index, BaseAddress, BaseAddress + Length - 1,
> +MemorySpaceMap[Index].Capabilities,
> +MemorySpaceMap[Index].Capabilities | EFI_MEMORY_PAGETYPE_MASK
> +));
> +  continue;
> +}
> +
>  if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
>//
>// Current memory space starts at a new page. Resetting PageLength will
> @@ -826,7 +852,9 @@ RefreshGcdMemoryAttributesFromPaging (
>PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
>  }
> 
> -// Sync real page attributes to GCD
> +//
> +// Sync actual page attributes to GCD
> +//
>  BaseAddress   = MemorySpaceMap[Index].BaseAddress;
>  MemorySpaceLength = MemorySpaceMap[Index].Length;
>  while (MemorySpaceLength > 0) {
> @@ -842,23 +870,26 @@ RefreshGcdMemoryAttributesFromPaging (
>  PageStartAddress  = (*PageEntry) &
> (UINT64)PageAttributeToMask(PageAttribute);
>  PageLength= PageAttributeToLength (PageAttribute) - 
> (BaseAddress -
> PageStartAddress);
>  

[edk2] [PATCH v5] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

[Jian J Wang]

> v5:
>Coding style clean-up

> v4:
> a. Remove DoUpdate and check attributes mismatch all the time to avoid
>a logic hole
> b. Add warning message if failed to update capability
> c. Add local variable to hold new attributes to make code cleaner

> v3:
> a. Add comment to explain more on updating memory capabilities
> b. Fix logic hole in updating attributes
> c. Instead of checking illegal memory space address and size, use return
>status of gDS->SetMemorySpaceCapabilities() to skip memory block which
>cannot be updated with new capabilities.

> v2
> a. Fix an issue which will cause setting capability failure if size is smaller
>than a page.

More than one entry of RT_CODE memory might cause boot problem for some
old OSs. This patch will fix this issue to keep OS compatibility as much
as possible.

More detailed information, please refer to
https://bugzilla.tianocore.org/show_bug.cgi?id=753

Cc: Eric Dong 
Cc: Jiewen Yao 
Cc: Laszlo Ersek 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 UefiCpuPkg/CpuDxe/CpuPageTable.c | 69 +---
 1 file changed, 50 insertions(+), 19 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index d312eb66f8..61537838b7 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -789,8 +789,7 @@ RefreshGcdMemoryAttributesFromPaging (
   UINT64  BaseAddress;
   UINT64  PageStartAddress;
   UINT64  Attributes;
-  UINT64  Capabilities;
-  BOOLEAN DoUpdate;
+  UINT64  NewAttributes;
   UINTN   Index;
 
   //
@@ -802,9 +801,8 @@ RefreshGcdMemoryAttributesFromPaging (
 
   GetCurrentPagingContext ();
 
-  DoUpdate  = FALSE;
-  Capabilities  = 0;
   Attributes= 0;
+  NewAttributes = 0;
   BaseAddress   = 0;
   PageLength= 0;
 
@@ -813,6 +811,34 @@ RefreshGcdMemoryAttributesFromPaging (
   continue;
 }
 
+//
+// Sync the actual paging related capabilities back to GCD service first.
+// As a side effect (good one), this can also help to avoid unnecessary
+// memory map entries due to the different capabilities of the same type
+// memory, such as multiple RT_CODE and RT_DATA entries in memory map,
+// which could cause boot failure of some old Linux distro (before v4.3).
+//
+Status = gDS->SetMemorySpaceCapabilities (
+MemorySpaceMap[Index].BaseAddress,
+MemorySpaceMap[Index].Length,
+MemorySpaceMap[Index].Capabilities |
+EFI_MEMORY_PAGETYPE_MASK
+);
+if (EFI_ERROR (Status)) {
+  //
+  // If we cannot udpate the capabilities, we cannot update its
+  // attributes either. So just simply skip current block of memory.
+  //
+  DEBUG ((
+DEBUG_WARN,
+"Failed to update capability: [%lu] %016lx - %016lx (%016lx -> 
%016lx)\r\n",
+(UINT64)Index, BaseAddress, BaseAddress + Length - 1,
+MemorySpaceMap[Index].Capabilities,
+MemorySpaceMap[Index].Capabilities | EFI_MEMORY_PAGETYPE_MASK
+));
+  continue;
+}
+
 if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
   //
   // Current memory space starts at a new page. Resetting PageLength will
@@ -826,7 +852,9 @@ RefreshGcdMemoryAttributesFromPaging (
   PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
 }
 
-// Sync real page attributes to GCD
+//
+// Sync actual page attributes to GCD
+//
 BaseAddress   = MemorySpaceMap[Index].BaseAddress;
 MemorySpaceLength = MemorySpaceMap[Index].Length;
 while (MemorySpaceLength > 0) {
@@ -842,23 +870,26 @@ RefreshGcdMemoryAttributesFromPaging (
 PageStartAddress  = (*PageEntry) & 
(UINT64)PageAttributeToMask(PageAttribute);
 PageLength= PageAttributeToLength (PageAttribute) - 
(BaseAddress - PageStartAddress);
 Attributes= GetAttributesFromPageEntry (PageEntry);
-
-if (Attributes != (MemorySpaceMap[Index].Attributes & 
EFI_MEMORY_PAGETYPE_MASK)) {
-  DoUpdate = TRUE;
-  Attributes |= (MemorySpaceMap[Index].Attributes & 
~EFI_MEMORY_PAGETYPE_MASK);
-  Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
-} else {
-  DoUpdate = FALSE;
-}
   }
 
   Length = MIN (PageLength, MemorySpaceLength);
-  if (DoUpdate) {
-gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
-gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
-DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - 

Re: [edk2] [PATCH 3/4] UefiCpuPkg/MtrrLib: Update algorithm to calculate optimal settings

[Ni, Ruiyu]

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Thursday, November 9, 2017 9:16 PM
> To: Ni, Ruiyu ; Justen, Jordan L
> ; Jeff Fan 
> Cc: Kinney, Michael D ; edk2-devel@lists.01.org;
> Yao, Jiewen ; Dong, Eric ; Ard
> Biesheuvel 
> Subject: Re: [edk2] [PATCH 3/4] UefiCpuPkg/MtrrLib: Update algorithm to
> calculate optimal settings
> 
> On 11/09/17 08:11, Ni, Ruiyu wrote:
> > The old version is not just slow.
> > It cannot work in certain cases. That's why the new version was developed.
> >
> > The new version adds a new API which allows caller to pass in the
> > scratch buffer instead of using the stack. If a platform has limited
> > stack, it can use that API.
> 
> (1) OK, so let me summarize:
> 
> (1a) Ray and Jeff confirmed that the AP stacks should be affected in
> *neither* PiSmmCpuDxeSmm *nor* the MpInitLib client modules (CpuMpPei,
> CpuDxe).
> 
> (1b) There is a new MtrrLib class API that allows the client module to pass 
> in a
> preallocated scratch buffer.
> 
> (1a) sounds great.
> 
> (1b) is an option we may or may not want to exercise in OVMF. I have the
> patches ready for enlarging the temp SEC/PEI RAM (and as part of that, the
> temp stack), which is one alternative. However, because OVMF's PEI phase runs
> from RAM (and not flash), the other alternative is just to add a sufficiently 
> large
> static UINT8 array to PlatformPei, and pass that as scratch space to MtrrLib.
> 
> 
> (2) However: I don't know *how* the new API --
> MtrrSetMemoryAttributesInMtrrSettings() -- is supposed to be used. In
> OvmfPkg/PlatformPei, we have the following MtrrLib calls:
> 
> * OvmfPkg/PlatformPei/Xen.c:
> 
> - MtrrSetMemoryAttribute()
> 
> (in a loop, basically)
> 
> * OvmfPkg/PlatformPei/MemDetect.c:
> 
> - IsMtrrSupported()
> - MtrrGetAllMtrrs()
> - MtrrSetAllMtrrs()
> - MtrrSetMemoryAttribute()
> 
> Is my understanding correct that MtrrSetMemoryAttribute() is the only function
> that is affected?

1. yes. Only MtrrSetMemoryAttribute() call in OVMF is affected.

> 
> 
> (3) Is my understanding correct that
> MtrrSetMemoryAttributesInMtrrSettings() should be used like this:
> 
> (3a) start with MtrrGetAllMtrrs()
> 
> (3b) collect all *foreseeable* MtrrSetMemoryAttribute() calls into an
>  array of MTRR_MEMORY_RANGE elements
> 
> (3c) Perform a batch update on the output of (3a) by calling
>  MtrrSetMemoryAttributesInMtrrSettings(). For this, the array from
>  (3b), plus a caller-allocated scratch space, have to be passed in,.
> 
> (3d) Finally, call MtrrSetAllMtrrs().
> 
> Is this correct?

2. yes. In summary, there are three ways to call this new API. The first way is 
what
you described. The second way is a bit change to (3a), ZeroMem() is called 
instead of MtrrGetAllMtrrs() to initialize the MTRR. The third way is to 
call
this new API using NULL MtrrSetting, which cause the API itself to retrieve
the current MTRR settings from CPU, apply the new setting, write to CPU.
But the third way doesn't support batch setting.

> 
> I think we could use this. Jordan, which alternative do you prefer; larger 
> stack
> and unchanged code in PlatformPei, or same stack and updated code in
> PlatformPei?
> 
> 
> (4) Ray: would it be possible to expose SCRATCH_BUFFER_SIZE (with a new
> name MTRR_SCRATCH_BUFFER_SIZE) in the library class header? I see the new
> RETURN_BUFFER_TOO_SMALL status codes, and I don't really want to deal with
> them. The library class header should provide clients with a size macro that
> *guarantees* that RETURN_BUFFER_TOO_SMALL will not occur.
> 
> Practically speaking, I would use MTRR_SCRATCH_BUFFER_SIZE in the definition
> of the static UINT8 array in PlatformPei. (If Jordan prefers this alternative 
> to the
> larger temp stack.)

3. Not quite correct. Because even when pass in the scratch buffer whose size 
equal
to MTRR_SCRATCH_BUFFER_SIZE, the BUFFER_TOO_SMALL could be returned.
That's why the BUFFER_TOO_SMALL status is invented. It requires caller to 
re-supply
the enough scratch buffer for calculation.
As such, I do not think exposing SCRATCH_BUFFER_SIZE helps.
When implementing the code, I tried to find out the maximum scratch buffer 
size but
found that the maximum could be up to 256KB. I cannot use such large stack 
because
as Jordan said, MSVC will inject some code results in unresolved symbol in 
EDKII code.
And DxeIpl only allocates 128KB stack for whole DXE phase.


> 
> Thanks!
> Laszlo
> 
> >> -Original Message-
> >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> >> Of Jordan Justen
> >> Sent: Thursday, November 9, 2017 2:56 PM
> >> To: Ni, Ruiyu ; Laszlo Ersek 
> >> Cc: Kinney, Michael D ; edk2-
> >> 

[edk2] [PATCH] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

[Jian J Wang]

> v5:
>Coding style clean-up

> v4:
> a. Remove DoUpdate and check attributes mismatch all the time to avoid
>a logic hole
> b. Add warning message if failed to update capability
> c. Add local variable to hold new attributes to make code cleaner

> v3:
> a. Add comment to explain more on updating memory capabilities
> b. Fix logic hole in updating attributes
> c. Instead of checking illegal memory space address and size, use return
>status of gDS->SetMemorySpaceCapabilities() to skip memory block which
>cannot be updated with new capabilities.

> v2
> a. Fix an issue which will cause setting capability failure if size is smaller
>than a page.

More than one entry of RT_CODE memory might cause boot problem for some
old OSs. This patch will fix this issue to keep OS compatibility as much
as possible.

More detailed information, please refer to
https://bugzilla.tianocore.org/show_bug.cgi?id=753

Cc: Eric Dong 
Cc: Jiewen Yao 
Cc: Laszlo Ersek 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 UefiCpuPkg/CpuDxe/CpuPageTable.c | 69 +---
 1 file changed, 50 insertions(+), 19 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index d312eb66f8..61537838b7 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -789,8 +789,7 @@ RefreshGcdMemoryAttributesFromPaging (
   UINT64  BaseAddress;
   UINT64  PageStartAddress;
   UINT64  Attributes;
-  UINT64  Capabilities;
-  BOOLEAN DoUpdate;
+  UINT64  NewAttributes;
   UINTN   Index;
 
   //
@@ -802,9 +801,8 @@ RefreshGcdMemoryAttributesFromPaging (
 
   GetCurrentPagingContext ();
 
-  DoUpdate  = FALSE;
-  Capabilities  = 0;
   Attributes= 0;
+  NewAttributes = 0;
   BaseAddress   = 0;
   PageLength= 0;
 
@@ -813,6 +811,34 @@ RefreshGcdMemoryAttributesFromPaging (
   continue;
 }
 
+//
+// Sync the actual paging related capabilities back to GCD service first.
+// As a side effect (good one), this can also help to avoid unnecessary
+// memory map entries due to the different capabilities of the same type
+// memory, such as multiple RT_CODE and RT_DATA entries in memory map,
+// which could cause boot failure of some old Linux distro (before v4.3).
+//
+Status = gDS->SetMemorySpaceCapabilities (
+MemorySpaceMap[Index].BaseAddress,
+MemorySpaceMap[Index].Length,
+MemorySpaceMap[Index].Capabilities |
+EFI_MEMORY_PAGETYPE_MASK
+);
+if (EFI_ERROR (Status)) {
+  //
+  // If we cannot udpate the capabilities, we cannot update its
+  // attributes either. So just simply skip current block of memory.
+  //
+  DEBUG ((
+DEBUG_WARN,
+"Failed to update capability: [%lu] %016lx - %016lx (%016lx -> 
%016lx)\r\n",
+(UINT64)Index, BaseAddress, BaseAddress + Length - 1,
+MemorySpaceMap[Index].Capabilities,
+MemorySpaceMap[Index].Capabilities | EFI_MEMORY_PAGETYPE_MASK
+));
+  continue;
+}
+
 if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
   //
   // Current memory space starts at a new page. Resetting PageLength will
@@ -826,7 +852,9 @@ RefreshGcdMemoryAttributesFromPaging (
   PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
 }
 
-// Sync real page attributes to GCD
+//
+// Sync actual page attributes to GCD
+//
 BaseAddress   = MemorySpaceMap[Index].BaseAddress;
 MemorySpaceLength = MemorySpaceMap[Index].Length;
 while (MemorySpaceLength > 0) {
@@ -842,23 +870,26 @@ RefreshGcdMemoryAttributesFromPaging (
 PageStartAddress  = (*PageEntry) & 
(UINT64)PageAttributeToMask(PageAttribute);
 PageLength= PageAttributeToLength (PageAttribute) - 
(BaseAddress - PageStartAddress);
 Attributes= GetAttributesFromPageEntry (PageEntry);
-
-if (Attributes != (MemorySpaceMap[Index].Attributes & 
EFI_MEMORY_PAGETYPE_MASK)) {
-  DoUpdate = TRUE;
-  Attributes |= (MemorySpaceMap[Index].Attributes & 
~EFI_MEMORY_PAGETYPE_MASK);
-  Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
-} else {
-  DoUpdate = FALSE;
-}
   }
 
   Length = MIN (PageLength, MemorySpaceLength);
-  if (DoUpdate) {
-gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
-gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
-DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - 

Re: [edk2] [PATCH v3] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

[Wang, Jian J]

Thanks for catching them. There'll be v5 today:)

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Thursday, November 09, 2017 10:13 PM
> To: Wang, Jian J ; edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Dong, Eric 
> Subject: Re: [edk2] [PATCH v3] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
> 
> Hi Jian,
> 
> this is v4, but the subject says v3 :) If you post a new version, please
> make sure that it says "v5" in the subject.
> 
> The logic looks OK to me; I've got some comments on style:
> 
> On 11/09/17 02:39, Jian J Wang wrote:
> >> v4:
> >> a. Remove DoUpdate and check attributes mismatch all the time to avoid
> >>a logic hole
> >> b. Add warning message if failed to update capability
> >> c. Add local variable to hold new attributes to make code cleaner
> >
> >> v3:
> >> a. Add comment to explain more on updating memory capabilities
> >> b. Fix logic hole in updating attributes
> >> c. Instead of checking illegal memory space address and size, use return
> >>status of gDS->SetMemorySpaceCapabilities() to skip memory block which
> >>cannot be updated with new capabilities.
> >
> >> v2
> >> a. Fix an issue which will cause setting capability failure if size is 
> >> smaller
> >>than a page.
> >
> > More than one entry of RT_CODE memory might cause boot problem for
> some
> > old OSs. This patch will fix this issue to keep OS compatibility as much
> > as possible.
> >
> > More detailed information, please refer to
> > https://bugzilla.tianocore.org/show_bug.cgi?id=753
> >
> > Cc: Eric Dong 
> > Cc: Jiewen Yao 
> > Cc: Laszlo Ersek 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang 
> > ---
> >  UefiCpuPkg/CpuDxe/CpuPageTable.c | 69
> +---
> >  1 file changed, 50 insertions(+), 19 deletions(-)
> >
> > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > index d312eb66f8..a1d804caed 100644
> > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > @@ -789,8 +789,7 @@ RefreshGcdMemoryAttributesFromPaging (
> >UINT64  BaseAddress;
> >UINT64  PageStartAddress;
> >UINT64  Attributes;
> > -  UINT64  Capabilities;
> > -  BOOLEAN DoUpdate;
> > +  UINT64  NewAttributes;
> >UINTN   Index;
> >
> >//
> > @@ -802,9 +801,8 @@ RefreshGcdMemoryAttributesFromPaging (
> >
> >GetCurrentPagingContext ();
> >
> > -  DoUpdate  = FALSE;
> > -  Capabilities  = 0;
> >Attributes= 0;
> > +  NewAttributes = 0;
> >BaseAddress   = 0;
> >PageLength= 0;
> >
> > @@ -813,6 +811,34 @@ RefreshGcdMemoryAttributesFromPaging (
> >continue;
> >  }
> >
> > +//
> > +// Sync the actual paging related capabilities back to GCD service 
> > first.
> > +// As a side effect (good one), this can also help to avoid unnecessary
> > +// memory map entries due to the different capabilities of the same 
> > type
> > +// memory, such as multiple RT_CODE and RT_DATA entries in memory
> map,
> > +// which could cause boot failure of some old Linux distro (before 
> > v4.3).
> > +//
> > +Status = gDS->SetMemorySpaceCapabilities (
> > +MemorySpaceMap[Index].BaseAddress,
> > +MemorySpaceMap[Index].Length,
> > +MemorySpaceMap[Index].Capabilities |
> > +EFI_MEMORY_PAGETYPE_MASK
> > +);
> > +if (EFI_ERROR (Status)) {
> > +  //
> > +  // If we cannot udpate the capabilities, we cannot update its
> > +  // attributes either. So just simply skip current block of memory.
> > +  //
> > +  DEBUG ((
> > +DEBUG_WARN,
> > +"Failed to update capability: [%d] %016lx - %016lx (%016lx -
> > %016lx)\r\n",
> > +Index, BaseAddress, BaseAddress + Length - 1,
> 
> (1) I think you forgot about my note that Index (which is of type UINTN)
> should not be printed with "%d". Instead, (UINT64)Index should be
> printed with "%lu".
> 
> > +MemorySpaceMap[Index].Capabilities,
> > +MemorySpaceMap[Index].Capabilities | EFI_MEMORY_PAGETYPE_MASK
> > +));
> > +  continue;
> > +}
> > +
> >  if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
> >//
> >// Current memory space starts at a new page. Resetting PageLength 
> > will
> > @@ -826,7 +852,9 @@ RefreshGcdMemoryAttributesFromPaging (
> >PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
> >  }
> >
> > -// Sync real page attributes to GCD
> > +//
> > + 

Re: [edk2] [PATCH v2 1/3] EmbeddedPkg/RealTimeClockRuntimeDxe: move common functionality into core

[Ard Biesheuvel]

On 9 November 2017 at 16:26, Leif Lindholm  wrote:
> On Mon, Nov 06, 2017 at 06:20:36PM +, Ard Biesheuvel wrote:
>> RealTimeClockRuntimeDxe defers the hardware/platform specific handling
>> of reading/setting the hardware clock to RealTimeClockLib, but for
>> unknown reasons, it also defers common functionality such as input
>> validation and recording the timezone and DST settings (which are
>> informational only and not managed by hardware)
>>
>> This has led to a lot of duplication in implementations of RealTimeClockLib
>> as well as TimeBaseLib, to the point where each library implementation
>> has its own set of UEFI variables to record the timezone and DST settings.
>> This makes little sense, and so let's update RealTimeClockRuntimeDxe now
>> to allow future implementations to rely on the core driver to take care of
>> these things.
>>
>> Note that reading the timezone and DST settings occurs before calling into
>> the library, so we can phase out this behavior gradually from library
>> implementations in EDK2, edk2-platforms or out of tree.
>
> Great consolidation, couple of minor questions/comments below:
>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>  EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c | 171 
>> +++-
>>  EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf |  11 +-
>>  2 files changed, 171 insertions(+), 11 deletions(-)
>>
>> diff --git a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c 
>> b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
>> index f1e067c0b59e..6b7cc876fa33 100644
>> --- a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
>> +++ b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
>> @@ -1,10 +1,8 @@
>>  /** @file
>>Implement EFI RealTimeClock runtime services via RTC Lib.
>>
>> -  Currently this driver does not support runtime virtual calling.
>> -
>> -
>>Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
>> +  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
>>
>>This program and the accompanying materials
>>are licensed and made available under the terms and conditions of the BSD 
>> License
>> @@ -17,14 +15,116 @@
>>  **/
>>
>>  #include 
>> +#include 
>> +#include 
>>  #include 
>>  #include 
>> -#include 
>> +#include 
>>  #include 
>>
>>  EFI_HANDLE  mHandle = NULL;
>>
>> +//
>> +// These values can be set by SetTime () and need to be returned by GetTime 
>> ()
>> +// but cannot usually be kept by the RTC hardware, so we store them in a 
>> UEFI
>> +// variable instead.
>> +//
>> +typedef struct {
>> +  INT16   TimeZone;
>> +  UINT8   Daylight;
>> +} NON_VOLATILE_TIME_SETTINGS;
>> +
>> +STATIC CONST CHAR16 mTimeSettingsVariableName[] = L"RtcTimeSettings";
>> +STATIC NON_VOLATILE_TIME_SETTINGS mTimeSettings;
>> +
>> +STATIC
>> +BOOLEAN
>> +IsValidTimeZone (
>> +  IN  INT16  TimeZone
>> +  )
>> +{
>> +  return TimeZone == EFI_UNSPECIFIED_TIMEZONE ||
>> + (TimeZone >= -1440 && TimeZone <= 1440);
>> +}
>> +
>> +STATIC
>> +BOOLEAN
>> +IsValidDaylight (
>> +  IN  INT8  Daylight
>> +  )
>> +{
>> +  return Daylight == 0 ||
>> + Daylight == EFI_TIME_ADJUST_DAYLIGHT ||
>> + Daylight == (EFI_TIME_ADJUST_DAYLIGHT | EFI_TIME_IN_DAYLIGHT);
>> +}
>>
>> +STATIC
>> +BOOLEAN
>> +EFIAPI
>> +IsLeapYear (
>> +  IN EFI_TIME   *Time
>> +  )
>> +{
>> +  if (Time->Year % 4 == 0) {
>> +if (Time->Year % 100 == 0) {
>> +  if (Time->Year % 400 == 0) {
>> +return TRUE;
>> +  } else {
>> +return FALSE;
>> +  }
>> +} else {
>> +  return TRUE;
>> +}
>> +  } else {
>> +return FALSE;
>> +  }
>> +}
>> +
>> +STATIC CONST INTN mDayOfMonth[12] = {
>> +  31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31
>> +};
>> +
>> +STATIC
>> +BOOLEAN
>> +EFIAPI
>> +IsDayValid (
>> +  IN  EFI_TIME  *Time
>> +  )
>> +{
>> +  ASSERT (Time->Day >= 1);
>> +  ASSERT (Time->Day <= mDayOfMonth[Time->Month - 1]);
>> +  ASSERT (Time->Month != 2 || (IsLeapYear (Time) && Time->Day <= 29));
>
> Is this me getting confused by basic logic operations, or should the
> above be something like
>ASSERT (Time->Month != 2 || (Time->Day <= (28 + IsLeapYear (Time) ? 1 : 
> 0)));
> ?
>

Yeah that does like dodgy. This actuall originates from TimeBaseLib,
which I would like to phase out as well (or reduce to the
EfiToEpochSeconds routines)

I think this should be

ASSERT (Time->Month != 2 || IsLeapYear (Time) || Time->Day <= 28);


>> +
>> +  if (Time->Day < 1 ||
>> +  Time->Day > mDayOfMonth[Time->Month - 1] ||
>> +  (Time->Month == 2 && (!IsLeapYear (Time) && Time->Day > 28))) {

Here, we should probably drop the inner parens, which will make the
expression more obviously the negation of the one above.

>> +return FALSE;
>> +  }
>> +  return TRUE;
>> +}
>> +
>> +STATIC
>> +BOOLEAN
>> +EFIAPI
>> +IsTimeValid(
>> +  IN EFI_TIME *Time
>> +  )

Re: [edk2] [PATCH v2] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core

[Ard Biesheuvel]

On 7 November 2017 at 18:13, Ard Biesheuvel  wrote:
> On 7 November 2017 at 18:09, Laszlo Ersek  wrote:
>> On 11/05/17 17:29, Ard Biesheuvel wrote:
>>> On 5 November 2017 at 16:27, Ard Biesheuvel  
>>> wrote:
 On 5 November 2017 at 05:52, Leif Lindholm  
 wrote:
> On Fri, Nov 03, 2017 at 11:33:52AM +, Ard Biesheuvel wrote:
>> DEBUG builds of PEI code will print a diagnostic message regarding
>> the utilization of temporary RAM before switching to permanent RAM.
>> For example,
>>
>>   Total temporary memory:16352 bytes.
>> temporary memory stack ever used:   4820 bytes.
>> temporary memory heap used for HobList: 4720 bytes.
>>
>> Tracking stack utilization like this requires the stack to be seeded
>> with a known magic value, and this needs to occur before entering C
>> code, given that it uses the stack. Currently, only Nt32Pkg appears
>> to implement this feature, but it is useful nonetheless, so let's
>> wire it up for PrePeiCore as well.
>>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=748
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel 
>
> OK, this may sound completely unreasonable, but seeing those
> implementations overwrite callee-saved registers without saving them
> makes my brain unhappy. (Yes, I know.)
>
> Could they either:
> - Have a comment prepended establishing the implicit ABI of which
>   registers the caller cannot rely on reusing after return.
>   Preferably somewhat echoed at the call site.
> - Be rewritten to use only scratch registers?
>

 I think it is implied that the startup code does not adhere to the
 AAPCS. That code already uses r5 and r6 without stacking them, simply
 because we're in the middle of preparing the stack and other execution
 context, precisely so the C code we call into can rely on AAPCS
 guarantees.
>>>
>>>
>>> Ehm, hold on, what do you mean by 'call site'? This code just runs and
>>> jumps back to a local label. There are no functions calls here until
>>> the point where we call into C (with the exception of the lovely
>>> ArmPlatformPeiBootAction() we added so Juno can find out how much DRAM
>>> it can use)
>>
>> Please continue the discussion with Leif on this; from my side, I'm
>> happy with the patch (I've sort of deduced what the assembly code does,
>> also relying on your v1 notes).
>>
>> The only eyebrow-raising part was:
>>
>> +  MOV64 (x9, FixedPcdGet32 (PcdInitValueInTempStack) |\
>> + FixedPcdGet32 (PcdInitValueInTempStack) << 32)
>>
>> where we left-shift a constant that is "in theory" UINT32 by 32 binary
>> places, using the << operator. In C that would be undefined behavior,
>> but this is assembly, so what do I know? ¯\_(ツ)_/¯
>>
>> Acked-by: Laszlo Ersek 
>>
>
> Thanks. And you're right, this is not C so no need to worry about that.
>
>> (
>>
>> By the way, just to see if I remember correctly, isn't STP:
>>
>> +0:stp   x9, x9, [x8], #16
>>
>> the kind of instruction that modifies multiple operands at once, and so
>> if it faults, it cannot be virtualized well? (Because the syndrome
>> register or whatever does not tell the VMM the whole picture about the
>> fault?)
>>
>> Totally irrelevant here, I'm just curious.
>>
>
> STP == STore Pair, and so it stores the values in the registers to
> memory. The only register that gets modified here is x8, due to the
> post-increment.
>

... which actually doesn't mean it is not affected by the same issue.

The reason such instructions are more difficult to virtualize is that
it requires KVM to decode the instruction, rather than read the
syndrome registers that can tell it which register we intended to
read/write from. So it is in fact perfectly feasible to virtualize it,
but the KVM authors just haven't bothered yet.

> But its converse
>
> LDP  , , [], #
>
> is indeed such an instruction, given that it modifies three registers
> at once, and so the registers that encode the exception run out of
> space. Note that this only affects virtualized MMIO.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core

[Ard Biesheuvel]

On 8 November 2017 at 16:12, Leif Lindholm  wrote:
> On Sun, Nov 05, 2017 at 04:29:15PM +, Ard Biesheuvel wrote:
>> >> OK, this may sound completely unreasonable, but seeing those
>> >> implementations overwrite callee-saved registers without saving them
>> >> makes my brain unhappy. (Yes, I know.)
>> >>
>> >> Could they either:
>> >> - Have a comment prepended establishing the implicit ABI of which
>> >>   registers the caller cannot rely on reusing after return.
>> >>   Preferably somewhat echoed at the call site.
>> >> - Be rewritten to use only scratch registers?
>> >>
>> >
>> > I think it is implied that the startup code does not adhere to the
>> > AAPCS. That code already uses r5 and r6 without stacking them, simply
>> > because we're in the middle of preparing the stack and other execution
>> > context, precisely so the C code we call into can rely on AAPCS
>> > guarantees.
>>
>> Ehm, hold on, what do you mean by 'call site'? This code just runs and
>> jumps back to a local label. There are no functions calls here until
>> the point where we call into C (with the exception of the lovely
>> ArmPlatformPeiBootAction() we added so Juno can find out how much DRAM
>> it can use)
>
> Yeah, you're right, I was misreading the block as a subroutine.
>
> Seems the only register that must be preserved across jumps is r5/x5,
> and neither of these modifications touch those (or change that fact).
>
> Reviewed-by: Leif Lindholm 
>

Thanks.

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

Re: [edk2] [Patch] MdeModulePkg/Core/Dxe: Remove extra connects for UEFI Applications

[Kinney, Michael D]

Star,

Thanks for noticing the inconsistency.

I agree that Image->Type is a better choice.

Mike

> -Original Message-
> From: Zeng, Star
> Sent: Thursday, November 9, 2017 5:10 AM
> To: Kinney, Michael D ;
> edk2-devel@lists.01.org
> Cc: Dong, Eric ; Zeng, Star
> 
> Subject: RE: [Patch] MdeModulePkg/Core/Dxe: Remove
> extra connects for UEFI Applications
> 
> Reviewed-by: Star Zeng 
> 
> BTW: I see the code is using Image-
> >ImageContext.ImageType at some places and Image->Type
> at other place, it seems a little inconsistent.
> 
> Thanks,
> Star
> -Original Message-
> From: Kinney, Michael D
> Sent: Thursday, November 9, 2017 1:55 AM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; Dong, Eric
> 
> Subject: [Patch] MdeModulePkg/Core/Dxe: Remove extra
> connects for UEFI Applications
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=765
> 
> The UEFI Specification Boot Services chapter,
> StartImage() service,
> EFF 1.10 Extension requires extra calls to
> ConnectController()
> if a UEFI Driver produces handles. The DXE Core is
> performing these
> extra calls to ConnectController() without evaluating
> the ImageType.
> 
> A filter is added to not make extra calls to
> ConnectController()
> if the ImageType is
> EFI_IMAGE_SUBSYSTEM_EFI_APPLICATION.
> 
> Without this filter, extra calls to ConnectController()
> may be
> performed by UEFI Applications or a UEFI Shell
> Applications that
> also call ConnectController().
> 
> Cc: Star Zeng 
> Cc: Eric Dong 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Michael D Kinney
> 
> ---
>  MdeModulePkg/Core/Dxe/Image/Image.c | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
> b/MdeModulePkg/Core/Dxe/Image/Image.c
> index 4e22aa6dc7..c6b8ff44b9 100644
> --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> @@ -1703,9 +1703,17 @@ CoreStartImage (
>mCurrentImage = LastImage;
> 
>//
> -  // Go connect any handles that were created or
> modified while the image executed.
> +  // UEFI Specification - StartImage() - EFI 1.10
> Extension
> +  // To maintain compatibility with UEFI drivers that
> are written to the EFI
> +  // 1.02 Specification, StartImage() must monitor the
> handle database before
> +  // and after each image is started. If any handles
> are created or modified
> +  // when an image is started, then
> EFI_BOOT_SERVICES.ConnectController() must
> +  // be called with the Recursive parameter set to
> TRUE for each of the newly
> +  // created or modified handles before StartImage()
> returns.
>//
> -  CoreConnectHandlesByKey (HandleDatabaseKey);
> +  if (Image->ImageContext.ImageType !=
> EFI_IMAGE_SUBSYSTEM_EFI_APPLICATION) {
> +CoreConnectHandlesByKey (HandleDatabaseKey);
> +  }
> 
>//
>// Handle the image's returned ExitData
> --
> 2.14.2.windows.3

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

Re: [edk2] [PATCH v2 3/3] ArmPlatformPkg/PL031RealTimeClockLib: ignore DST setting when timezone is set

[Leif Lindholm]

On Mon, Nov 06, 2017 at 06:20:38PM +, Ard Biesheuvel wrote:
> According to the UEFI spec, the timezone setting which the platform needs
> to record in addition to the actual date and time already reflects the
> current DST setting. In other words, moving the clock from standard time
> to daylight saving time also involves adding or subtracting 60 minutes
> from the timezone setting, as well as flicking the EFI_TIME_IN_DAYLIGHT
> bit in the DST setting.
> 
> This means we need to disregard the DST setting if the timezone is
> specified, and only add or subtract the additional hour if we are on
> local time.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 

Assuming I understand the above correctly, and page 259/260 in 2.7a:
Reviewed-by: Leif Lindholm 

> ---
>  ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c | 12 
> 
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git 
> a/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c 
> b/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c
> index f1eb0deb3249..459dcc0a0519 100644
> --- a/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c
> +++ b/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c
> @@ -164,12 +164,10 @@ LibGetTime (
>}
>  
>// Adjust for the correct time zone
> +  // The timezone setting also reflects the DST setting of the clock
>if (Time->TimeZone != EFI_UNSPECIFIED_TIMEZONE) {
>  EpochSeconds += Time->TimeZone * SEC_PER_MIN;
> -  }
> -
> -  // Adjust for the correct period
> -  if ((Time->Daylight & EFI_TIME_IN_DAYLIGHT) == EFI_TIME_IN_DAYLIGHT) {
> +  } else if ((Time->Daylight & EFI_TIME_IN_DAYLIGHT) == 
> EFI_TIME_IN_DAYLIGHT) {
>  // Convert to adjusted time, i.e. spring forwards one hour
>  EpochSeconds += SEC_PER_HOUR;
>}
> @@ -229,12 +227,10 @@ LibSetTime (
>EpochSeconds = EfiTimeToEpoch (Time);
>  
>// Adjust for the correct time zone, i.e. convert to UTC time zone
> +  // The timezone setting also reflects the DST setting of the clock
>if (Time->TimeZone != EFI_UNSPECIFIED_TIMEZONE) {
>  EpochSeconds -= Time->TimeZone * SEC_PER_MIN;
> -  }
> -
> -  // Adjust for the correct period
> -  if ((Time->Daylight & EFI_TIME_IN_DAYLIGHT) == EFI_TIME_IN_DAYLIGHT) {
> +  } else if ((Time->Daylight & EFI_TIME_IN_DAYLIGHT) == 
> EFI_TIME_IN_DAYLIGHT) {
>  // Convert to un-adjusted time, i.e. fall back one hour
>  EpochSeconds -= SEC_PER_HOUR;
>}
> -- 
> 2.11.0
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 2/3] ArmPlatformPkg/PL031RealTimeClockLib: remove validation and DST handling

[Leif Lindholm]

On Mon, Nov 06, 2017 at 06:20:37PM +, Ard Biesheuvel wrote:
> This library, which is intended to encapsulate the hardware specifics
> of the ARM PL031 RTC, also implements its own input validation routines
> and record the timezone and DST settings in its own set of EFI variables.
> 
> This functionality has recently been added to the core driver, so let's
> remove it here.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 

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

Re: [edk2] [PATCH v2 1/3] EmbeddedPkg/RealTimeClockRuntimeDxe: move common functionality into core

[Leif Lindholm]

On Mon, Nov 06, 2017 at 06:20:36PM +, Ard Biesheuvel wrote:
> RealTimeClockRuntimeDxe defers the hardware/platform specific handling
> of reading/setting the hardware clock to RealTimeClockLib, but for
> unknown reasons, it also defers common functionality such as input
> validation and recording the timezone and DST settings (which are
> informational only and not managed by hardware)
> 
> This has led to a lot of duplication in implementations of RealTimeClockLib
> as well as TimeBaseLib, to the point where each library implementation
> has its own set of UEFI variables to record the timezone and DST settings.
> This makes little sense, and so let's update RealTimeClockRuntimeDxe now
> to allow future implementations to rely on the core driver to take care of
> these things.
> 
> Note that reading the timezone and DST settings occurs before calling into
> the library, so we can phase out this behavior gradually from library
> implementations in EDK2, edk2-platforms or out of tree.

Great consolidation, couple of minor questions/comments below:

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
>  EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c | 171 
> +++-
>  EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf |  11 +-
>  2 files changed, 171 insertions(+), 11 deletions(-)
> 
> diff --git a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c 
> b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> index f1e067c0b59e..6b7cc876fa33 100644
> --- a/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> +++ b/EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClock.c
> @@ -1,10 +1,8 @@
>  /** @file
>Implement EFI RealTimeClock runtime services via RTC Lib.
>  
> -  Currently this driver does not support runtime virtual calling.
> -
> -
>Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
> +  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
>  
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of the BSD 
> License
> @@ -17,14 +15,116 @@
>  **/
>  
>  #include 
> +#include 
> +#include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  
>  EFI_HANDLE  mHandle = NULL;
>  
> +//
> +// These values can be set by SetTime () and need to be returned by GetTime 
> ()
> +// but cannot usually be kept by the RTC hardware, so we store them in a UEFI
> +// variable instead.
> +//
> +typedef struct {
> +  INT16   TimeZone;
> +  UINT8   Daylight;
> +} NON_VOLATILE_TIME_SETTINGS;
> +
> +STATIC CONST CHAR16 mTimeSettingsVariableName[] = L"RtcTimeSettings";
> +STATIC NON_VOLATILE_TIME_SETTINGS mTimeSettings;
> +
> +STATIC
> +BOOLEAN
> +IsValidTimeZone (
> +  IN  INT16  TimeZone
> +  )
> +{
> +  return TimeZone == EFI_UNSPECIFIED_TIMEZONE ||
> + (TimeZone >= -1440 && TimeZone <= 1440);
> +}
> +
> +STATIC
> +BOOLEAN
> +IsValidDaylight (
> +  IN  INT8  Daylight
> +  )
> +{
> +  return Daylight == 0 ||
> + Daylight == EFI_TIME_ADJUST_DAYLIGHT ||
> + Daylight == (EFI_TIME_ADJUST_DAYLIGHT | EFI_TIME_IN_DAYLIGHT);
> +}
>  
> +STATIC
> +BOOLEAN
> +EFIAPI
> +IsLeapYear (
> +  IN EFI_TIME   *Time
> +  )
> +{
> +  if (Time->Year % 4 == 0) {
> +if (Time->Year % 100 == 0) {
> +  if (Time->Year % 400 == 0) {
> +return TRUE;
> +  } else {
> +return FALSE;
> +  }
> +} else {
> +  return TRUE;
> +}
> +  } else {
> +return FALSE;
> +  }
> +}
> +
> +STATIC CONST INTN mDayOfMonth[12] = {
> +  31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31
> +};
> +
> +STATIC
> +BOOLEAN
> +EFIAPI
> +IsDayValid (
> +  IN  EFI_TIME  *Time
> +  )
> +{
> +  ASSERT (Time->Day >= 1);
> +  ASSERT (Time->Day <= mDayOfMonth[Time->Month - 1]);
> +  ASSERT (Time->Month != 2 || (IsLeapYear (Time) && Time->Day <= 29));

Is this me getting confused by basic logic operations, or should the
above be something like
   ASSERT (Time->Month != 2 || (Time->Day <= (28 + IsLeapYear (Time) ? 1 : 0)));
?

> +
> +  if (Time->Day < 1 ||
> +  Time->Day > mDayOfMonth[Time->Month - 1] ||
> +  (Time->Month == 2 && (!IsLeapYear (Time) && Time->Day > 28))) {
> +return FALSE;
> +  }
> +  return TRUE;
> +}
> +
> +STATIC
> +BOOLEAN
> +EFIAPI
> +IsTimeValid(
> +  IN EFI_TIME *Time
> +  )
> +{
> +  // Check the input parameters are within the range specified by UEFI
> +  if (Time->Year   < 1900   ||
> +  Time->Year   >    ||
> +  Time->Month  < 1  ||
> +  Time->Month  > 12 ||
> +  !IsDayValid (Time)||
> +  Time->Hour   > 23 ||
> +  Time->Minute > 59 ||
> +  Time->Second > 59 ||
> +  !IsValidTimeZone (Time->TimeZone) ||
> +  !IsValidDaylight (Time->Daylight)) {
> +return FALSE;
> +  }
> +  return TRUE;
> +}
>  
>  /**
>

Re: [edk2] [platforms: PATCH 3/4] Platform/Marvell: Introduce MvFvbDxe variable support driver

[Leif Lindholm]

On Thu, Nov 09, 2017 at 04:48:05PM +0100, Marcin Wojtas wrote:
> 2017-11-09 16:40 GMT+01:00 Leif Lindholm :
> > On Thu, Nov 09, 2017 at 04:16:54PM +0100, Marcin Wojtas wrote:
> >> Hi Leif,
> >>
> >> > > +  {
> >> > > +MvFvbGetAttributes,   // GetAttributes
> >> > > +MvFvbSetAttributes,   // SetAttributes
> >> > > +MvFvbGetPhysicalAddress,  // GetPhysicalAddress
> >> > > +MvFvbGetBlockSize,// GetBlockSize
> >> > > +MvFvbRead,// Read
> >> > > +MvFvbWrite,   // Write
> >> > > +MvFvbEraseBlocks, // EraseBlocks
> >> > > +NULL, // ParentHandle
> >> > > +  }, //  FvbProtoccol;
> >> >
> >> > -c
> >>
> >> Thanks for thorough review. Everything seems clear, but what does "-c"
> >> above mean?
> >
> > You stole the misspelt "//  FvbProtocol" from
> > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c.
> >
> > This had the benefit of showing me what this file was based on, but
> > should probably be fixed :)
> >
> 
> Right, I could have mentioned it explicitly in the commit message
> (only the copyrights are left). Significant part of the issues you
> pointed had been inherited from the original driver - my eyes and the
> checkpatch missed them.

I assumed they had, but that driver predates my reviewing.

If you could (completely unrelatedly) mirror those changes to the
original, that would of course be highly appreciated.

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

Re: [edk2] [platforms: PATCH 3/4] Platform/Marvell: Introduce MvFvbDxe variable support driver

[Marcin Wojtas]

2017-11-09 16:40 GMT+01:00 Leif Lindholm :
> On Thu, Nov 09, 2017 at 04:16:54PM +0100, Marcin Wojtas wrote:
>> Hi Leif,
>>
>> > > +  {
>> > > +MvFvbGetAttributes,   // GetAttributes
>> > > +MvFvbSetAttributes,   // SetAttributes
>> > > +MvFvbGetPhysicalAddress,  // GetPhysicalAddress
>> > > +MvFvbGetBlockSize,// GetBlockSize
>> > > +MvFvbRead,// Read
>> > > +MvFvbWrite,   // Write
>> > > +MvFvbEraseBlocks, // EraseBlocks
>> > > +NULL, // ParentHandle
>> > > +  }, //  FvbProtoccol;
>> >
>> > -c
>> >
>>
>> Thanks for thorough review. Everything seems clear, but what does "-c"
>> above mean?
>
> You stole the misspelt "//  FvbProtocol" from
> ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c.
>
> This had the benefit of showing me what this file was based on, but
> should probably be fixed :)
>

Right, I could have mentioned it explicitly in the commit message
(only the copyrights are left). Significant part of the issues you
pointed had been inherited from the original driver - my eyes and the
checkpatch missed them.

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

Re: [edk2] [platforms: PATCH 3/4] Platform/Marvell: Introduce MvFvbDxe variable support driver

[Leif Lindholm]

On Thu, Nov 09, 2017 at 04:16:54PM +0100, Marcin Wojtas wrote:
> Hi Leif,
> 
> > > +  {
> > > +MvFvbGetAttributes,   // GetAttributes
> > > +MvFvbSetAttributes,   // SetAttributes
> > > +MvFvbGetPhysicalAddress,  // GetPhysicalAddress
> > > +MvFvbGetBlockSize,// GetBlockSize
> > > +MvFvbRead,// Read
> > > +MvFvbWrite,   // Write
> > > +MvFvbEraseBlocks, // EraseBlocks
> > > +NULL, // ParentHandle
> > > +  }, //  FvbProtoccol;
> >
> > -c
> >
> 
> Thanks for thorough review. Everything seems clear, but what does "-c"
> above mean?

You stole the misspelt "//  FvbProtocol" from
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c.

This had the benefit of showing me what this file was based on, but
should probably be fixed :)

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

Re: [edk2] [platforms: PATCH 3/4] Platform/Marvell: Introduce MvFvbDxe variable support driver

[Marcin Wojtas]

Hi Leif,

2017-11-09 16:02 GMT+01:00 Leif Lindholm :
>
> On Sun, Nov 05, 2017 at 11:55:38AM +0100, Marcin Wojtas wrote:
> > MvFvbDxe driver introduces non-volatile EFI variable support
> > for Armada platforms. It relies on memory-mapped SPI read access.
>
> > Implementation of EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL
> > is done with using existing Marvell SPI infrastructure
> > (SpiMasterProtocol and SpiFlashProtocol), thanks to which
> > this driver will be able to support various combinations of
> > flash devices and host controllers.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas 
> > ---
> >  Platform/Marvell/Drivers/Spi/Variables/MvFvbDxe.c   | 1049 
> > 
> >  Platform/Marvell/Drivers/Spi/Variables/MvFvbDxe.h   |  114 +++
> >  Platform/Marvell/Drivers/Spi/Variables/MvFvbDxe.inf |   91 ++
> >  Platform/Marvell/Marvell.dec|1 +
> >  4 files changed, 1255 insertions(+)
> >
> > diff --git a/Platform/Marvell/Drivers/Spi/Variables/MvFvbDxe.c 
> > b/Platform/Marvell/Drivers/Spi/Variables/MvFvbDxe.c
> > new file mode 100644
> > index 000..fcb2d70
> > --- /dev/null
> > +++ b/Platform/Marvell/Drivers/Spi/Variables/MvFvbDxe.c
> > @@ -0,0 +1,1049 @@
> > +/*++ @file  MvFvbDxe.c
> > +
> > + Copyright (c) 2011 - 2014, ARM Ltd. All rights reserved.
> > + Copyright (c) 2017 Marvell International Ltd.
> > +
> > + 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
> > + http://opensource.org/licenses/bsd-license.php
> > +
> > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> > IMPLIED.
> > +
> > + --*/
> > +
> > +#include 
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include 
> > +#include 
>
> S before V.
>
> > +
> > +#include "MvFvbDxe.h"
> > +
> > +STATIC EFI_EVENT mFvbVirtualAddrChangeEvent;
> > +STATIC FVB_DEVICE*mFvbDevice;
> > +
> > +STATIC CONST FVB_DEVICE mMvFvbFlashInstanceTemplate = {
> > +  {
> > +0,// SpiFlash Chip Select ... NEED TO BE FILLED
> > +0,// SpiFlash Maximum Frequency ... NEED TO BE FILLED
> > +0,// SpiFlash Transfer Mode ... NEED TO BE FILLED
> > +0,// SpiFlash Address Size ... NEED TO BE FILLED
> > +NULL, // SpiFlash detailed information ... NEED TO BE FILLED
> > +0,// HostRegisterBaseAddress ... NEED TO BE FILLED
> > +0,// CoreClock ... NEED TO BE FILLED
> > +  }, // SpiDevice
> > +
> > +  NULL, // SpiFlashProtocol ... NEED TO BE FILLED
> > +  NULL, // SpiMasterProtocol ... NEED TO BE FILLED
> > +  NULL, // Handle ... NEED TO BE FILLED
> > +
> > +  FVB_FLASH_SIGNATURE, // Signature
> > +
> > +  0, // DeviceBaseAddress ... NEED TO BE FILLED
> > +  0, // RegionBaseAddress ... NEED TO BE FILLED
> > +  SIZE_256KB, // Size
> > +  0, // FvbOffset ... NEED TO BE FILLED
> > +  0, // FvbSize ... NEED TO BE FILLED
> > +  0, // StartLba
> > +
> > +  {
> > +0, // MediaId ... NEED TO BE FILLED
> > +FALSE, // RemovableMedia
> > +TRUE,  // MediaPresent
> > +FALSE, // LogicalPartition
> > +FALSE, // ReadOnly
> > +FALSE, // WriteCaching;
> > +0, // BlockSize ... NEED TO BE FILLED
> > +4, // IoAlign
> > +0, // LastBlock ... NEED TO BE FILLED
> > +0, // LowestAlignedLba
> > +1, // LogicalBlocksPerPhysicalBlock
> > +  }, //Media;
> > +
> > +  {
> > +MvFvbGetAttributes,   // GetAttributes
> > +MvFvbSetAttributes,   // SetAttributes
> > +MvFvbGetPhysicalAddress,  // GetPhysicalAddress
> > +MvFvbGetBlockSize,// GetBlockSize
> > +MvFvbRead,// Read
> > +MvFvbWrite,   // Write
> > +MvFvbEraseBlocks, // EraseBlocks
> > +NULL, // ParentHandle
> > +  }, //  FvbProtoccol;
>
> -c
>

Thanks for thorough review. Everything seems clear, but what does "-c"
above mean?

Best regards,
Marcin

> > +
> > +  {
> > +{
> > +  {
> > +HARDWARE_DEVICE_PATH,
> > +HW_VENDOR_DP,
> > +{
> > +  (UINT8)sizeof (VENDOR_DEVICE_PATH),
> > +  (UINT8)((sizeof (VENDOR_DEVICE_PATH)) >> 8)
> > +}
> > +  },
> > +  { 0xfc0cb972, 0x21df, 0x44d2, { 0x92, 0xa5, 0x78, 0x98, 0x99, 0xcb, 
> > 0xf6, 0x61 } }
> > +},
> > +{
> > +  END_DEVICE_PATH_TYPE,
> > +  END_ENTIRE_DEVICE_PATH_SUBTYPE,
> > +  { sizeof (EFI_DEVICE_PATH_PROTOCOL), 0 }
> > +}
> > +  } // DevicePath
> > +};
> > +
> > +//
> > +// The Firmware Volume Block Protocol is the low-level interface
> > +// to a firmware volume. File-level access to a 

Re: [edk2] [platforms: PATCH 4/4] Marvell/Armada: Enable variables support

[Leif Lindholm]

On Sun, Nov 05, 2017 at 11:55:39AM +0100, Marcin Wojtas wrote:
> Wire up the non-volatile EFI variable store support, by switching from
> the emulation driver to the real one. Define default values for
> memory mapped SPI access, which must be configured by the early
> firmware. In order to ensure proper execution, configure initialization
> order with Depex entries.

(And drop Depex entries from here.)
If you do:
Reviewed-by: Leif Lindholm 

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas 
> ---
>  Platform/Marvell/Armada/Armada.dsc.inc  | 25 +++-
>  Platform/Marvell/Armada/Armada70x0.fdf  |  6 -
>  Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf |  5 +++-
>  Platform/Marvell/Drivers/Spi/MvSpiDxe.inf   |  5 +++-
>  Platform/Marvell/Marvell.dec|  3 +++
>  5 files changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/Platform/Marvell/Armada/Armada.dsc.inc 
> b/Platform/Marvell/Armada/Armada.dsc.inc
> index 2cd96e6..3e0 100644
> --- a/Platform/Marvell/Armada/Armada.dsc.inc
> +++ b/Platform/Marvell/Armada/Armada.dsc.inc
> @@ -371,6 +371,17 @@
># TRNG
>gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0xF276
>  
> +  #
> +  # Variable store - default values
> +  #
> +  gMarvellTokenSpaceGuid.PcdSpiMemoryBase|0xF900
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0xF93C
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x0001
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0xF93D
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x0001
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0xF93E
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x0001
> +
>  
> 
>  #
>  # Components Section - list of all EDK II Modules needed by this Platform
> @@ -428,7 +439,6 @@
>MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
>MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
>MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
> -  MdeModulePkg/Universal/Variable/EmuRuntimeDxe/EmuVariableRuntimeDxe.inf
>  
>EmbeddedPkg/EmbeddedMonotonicCounter/EmbeddedMonotonicCounter.inf
>MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf
> @@ -485,6 +495,19 @@
>
> NULL|MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanceManagerUiLib.inf
>}
>  
> +  #
> +  # Variable services
> +  #
> +  Platform/Marvell/Drivers/Spi/Variables/MvFvbDxe.inf
> +  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
> +  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
> +
> +  
> AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf
> +  NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> +  
> TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> +  VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
> +  }
> +
># UEFI application (Shell Embedded Boot Loader)
>ShellPkg/Application/Shell/Shell.inf {
>  
> diff --git a/Platform/Marvell/Armada/Armada70x0.fdf 
> b/Platform/Marvell/Armada/Armada70x0.fdf
> index ec2c368..ca92c60 100644
> --- a/Platform/Marvell/Armada/Armada70x0.fdf
> +++ b/Platform/Marvell/Armada/Armada70x0.fdf
> @@ -103,7 +103,6 @@ FvNameGuid = 5eda4200-2c5f-43cb-9da3-0baf74b1b30c
>INF MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
>INF MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
>INF MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
> -  INF MdeModulePkg/Universal/Variable/EmuRuntimeDxe/EmuVariableRuntimeDxe.inf
>INF EmbeddedPkg/EmbeddedMonotonicCounter/EmbeddedMonotonicCounter.inf
>INF MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf
>INF EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf
> @@ -115,6 +114,11 @@ FvNameGuid = 5eda4200-2c5f-43cb-9da3-0baf74b1b30c
>INF Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf
>INF Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf
>  
> +  # Variable services
> +  INF Platform/Marvell/Drivers/Spi/Variables/MvFvbDxe.inf
> +  INF MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
> +  INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> +
># Network support
>INF MdeModulePkg/Universal/Network/SnpDxe/SnpDxe.inf
>INF MdeModulePkg/Universal/Network/DpcDxe/DpcDxe.inf
> diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf 
> b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf
> index 200a00c..c6bbe5e 100644
> --- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf
> +++ 

Re: [edk2] [platforms: PATCH 3/4] Platform/Marvell: Introduce MvFvbDxe variable support driver

[Leif Lindholm]

On Sun, Nov 05, 2017 at 11:55:38AM +0100, Marcin Wojtas wrote:
> MvFvbDxe driver introduces non-volatile EFI variable support
> for Armada platforms. It relies on memory-mapped SPI read access.

> Implementation of EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL
> is done with using existing Marvell SPI infrastructure
> (SpiMasterProtocol and SpiFlashProtocol), thanks to which
> this driver will be able to support various combinations of
> flash devices and host controllers.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas 
> ---
>  Platform/Marvell/Drivers/Spi/Variables/MvFvbDxe.c   | 1049 
> 
>  Platform/Marvell/Drivers/Spi/Variables/MvFvbDxe.h   |  114 +++
>  Platform/Marvell/Drivers/Spi/Variables/MvFvbDxe.inf |   91 ++
>  Platform/Marvell/Marvell.dec|1 +
>  4 files changed, 1255 insertions(+)
> 
> diff --git a/Platform/Marvell/Drivers/Spi/Variables/MvFvbDxe.c 
> b/Platform/Marvell/Drivers/Spi/Variables/MvFvbDxe.c
> new file mode 100644
> index 000..fcb2d70
> --- /dev/null
> +++ b/Platform/Marvell/Drivers/Spi/Variables/MvFvbDxe.c
> @@ -0,0 +1,1049 @@
> +/*++ @file  MvFvbDxe.c
> +
> + Copyright (c) 2011 - 2014, ARM Ltd. All rights reserved.
> + Copyright (c) 2017 Marvell International Ltd.
> +
> + 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
> + http://opensource.org/licenses/bsd-license.php
> +
> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +
> + --*/
> +
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 

S before V.

> +
> +#include "MvFvbDxe.h"
> +
> +STATIC EFI_EVENT mFvbVirtualAddrChangeEvent;
> +STATIC FVB_DEVICE*mFvbDevice;
> +
> +STATIC CONST FVB_DEVICE mMvFvbFlashInstanceTemplate = {
> +  {
> +0,// SpiFlash Chip Select ... NEED TO BE FILLED
> +0,// SpiFlash Maximum Frequency ... NEED TO BE FILLED
> +0,// SpiFlash Transfer Mode ... NEED TO BE FILLED
> +0,// SpiFlash Address Size ... NEED TO BE FILLED
> +NULL, // SpiFlash detailed information ... NEED TO BE FILLED
> +0,// HostRegisterBaseAddress ... NEED TO BE FILLED
> +0,// CoreClock ... NEED TO BE FILLED
> +  }, // SpiDevice
> +
> +  NULL, // SpiFlashProtocol ... NEED TO BE FILLED
> +  NULL, // SpiMasterProtocol ... NEED TO BE FILLED
> +  NULL, // Handle ... NEED TO BE FILLED
> +
> +  FVB_FLASH_SIGNATURE, // Signature
> +
> +  0, // DeviceBaseAddress ... NEED TO BE FILLED
> +  0, // RegionBaseAddress ... NEED TO BE FILLED
> +  SIZE_256KB, // Size
> +  0, // FvbOffset ... NEED TO BE FILLED
> +  0, // FvbSize ... NEED TO BE FILLED
> +  0, // StartLba
> +
> +  {
> +0, // MediaId ... NEED TO BE FILLED
> +FALSE, // RemovableMedia
> +TRUE,  // MediaPresent
> +FALSE, // LogicalPartition
> +FALSE, // ReadOnly
> +FALSE, // WriteCaching;
> +0, // BlockSize ... NEED TO BE FILLED
> +4, // IoAlign
> +0, // LastBlock ... NEED TO BE FILLED
> +0, // LowestAlignedLba
> +1, // LogicalBlocksPerPhysicalBlock
> +  }, //Media;
> +
> +  {
> +MvFvbGetAttributes,   // GetAttributes
> +MvFvbSetAttributes,   // SetAttributes
> +MvFvbGetPhysicalAddress,  // GetPhysicalAddress
> +MvFvbGetBlockSize,// GetBlockSize
> +MvFvbRead,// Read
> +MvFvbWrite,   // Write
> +MvFvbEraseBlocks, // EraseBlocks
> +NULL, // ParentHandle
> +  }, //  FvbProtoccol;

-c

> +
> +  {
> +{
> +  {
> +HARDWARE_DEVICE_PATH,
> +HW_VENDOR_DP,
> +{
> +  (UINT8)sizeof (VENDOR_DEVICE_PATH),
> +  (UINT8)((sizeof (VENDOR_DEVICE_PATH)) >> 8)
> +}
> +  },
> +  { 0xfc0cb972, 0x21df, 0x44d2, { 0x92, 0xa5, 0x78, 0x98, 0x99, 0xcb, 
> 0xf6, 0x61 } }
> +},
> +{
> +  END_DEVICE_PATH_TYPE,
> +  END_ENTIRE_DEVICE_PATH_SUBTYPE,
> +  { sizeof (EFI_DEVICE_PATH_PROTOCOL), 0 }
> +}
> +  } // DevicePath
> +};
> +
> +//
> +// The Firmware Volume Block Protocol is the low-level interface
> +// to a firmware volume. File-level access to a firmware volume
> +// should not be done using the Firmware Volume Block Protocol.
> +// Normal access to a firmware volume must use the Firmware
> +// Volume Protocol. Typically, only the file system driver that
> +// produces the Firmware Volume Protocol will bind to the
> +// Firmware Volume Block Protocol.
> +//
> +
> +/**
> +  Initialises the FV Header and Variable Store Header
> +  to support variable operations.
> +
> +  @param[in]  Ptr - Location to initialise the 

Re: [edk2] [PATCH v3] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

[Laszlo Ersek]

Hi Jian,

this is v4, but the subject says v3 :) If you post a new version, please
make sure that it says "v5" in the subject.

The logic looks OK to me; I've got some comments on style:

On 11/09/17 02:39, Jian J Wang wrote:
>> v4:
>> a. Remove DoUpdate and check attributes mismatch all the time to avoid
>>a logic hole
>> b. Add warning message if failed to update capability
>> c. Add local variable to hold new attributes to make code cleaner
> 
>> v3:
>> a. Add comment to explain more on updating memory capabilities
>> b. Fix logic hole in updating attributes
>> c. Instead of checking illegal memory space address and size, use return
>>status of gDS->SetMemorySpaceCapabilities() to skip memory block which
>>cannot be updated with new capabilities.
> 
>> v2
>> a. Fix an issue which will cause setting capability failure if size is 
>> smaller
>>than a page.
> 
> More than one entry of RT_CODE memory might cause boot problem for some
> old OSs. This patch will fix this issue to keep OS compatibility as much
> as possible.
> 
> More detailed information, please refer to
> https://bugzilla.tianocore.org/show_bug.cgi?id=753
> 
> Cc: Eric Dong 
> Cc: Jiewen Yao 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 69 
> +---
>  1 file changed, 50 insertions(+), 19 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c 
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index d312eb66f8..a1d804caed 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -789,8 +789,7 @@ RefreshGcdMemoryAttributesFromPaging (
>UINT64  BaseAddress;
>UINT64  PageStartAddress;
>UINT64  Attributes;
> -  UINT64  Capabilities;
> -  BOOLEAN DoUpdate;
> +  UINT64  NewAttributes;
>UINTN   Index;
>  
>//
> @@ -802,9 +801,8 @@ RefreshGcdMemoryAttributesFromPaging (
>  
>GetCurrentPagingContext ();
>  
> -  DoUpdate  = FALSE;
> -  Capabilities  = 0;
>Attributes= 0;
> +  NewAttributes = 0;
>BaseAddress   = 0;
>PageLength= 0;
>  
> @@ -813,6 +811,34 @@ RefreshGcdMemoryAttributesFromPaging (
>continue;
>  }
>  
> +//
> +// Sync the actual paging related capabilities back to GCD service first.
> +// As a side effect (good one), this can also help to avoid unnecessary
> +// memory map entries due to the different capabilities of the same type
> +// memory, such as multiple RT_CODE and RT_DATA entries in memory map,
> +// which could cause boot failure of some old Linux distro (before v4.3).
> +//
> +Status = gDS->SetMemorySpaceCapabilities (
> +MemorySpaceMap[Index].BaseAddress,
> +MemorySpaceMap[Index].Length,
> +MemorySpaceMap[Index].Capabilities |
> +EFI_MEMORY_PAGETYPE_MASK
> +);
> +if (EFI_ERROR (Status)) {
> +  //
> +  // If we cannot udpate the capabilities, we cannot update its
> +  // attributes either. So just simply skip current block of memory.
> +  //
> +  DEBUG ((
> +DEBUG_WARN,
> +"Failed to update capability: [%d] %016lx - %016lx (%016lx -> 
> %016lx)\r\n",
> +Index, BaseAddress, BaseAddress + Length - 1,

(1) I think you forgot about my note that Index (which is of type UINTN)
should not be printed with "%d". Instead, (UINT64)Index should be
printed with "%lu".

> +MemorySpaceMap[Index].Capabilities,
> +MemorySpaceMap[Index].Capabilities | EFI_MEMORY_PAGETYPE_MASK
> +));
> +  continue;
> +}
> +
>  if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
>//
>// Current memory space starts at a new page. Resetting PageLength will
> @@ -826,7 +852,9 @@ RefreshGcdMemoryAttributesFromPaging (
>PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
>  }
>  
> -// Sync real page attributes to GCD
> +//
> +// Sync actual page attributes to GCD
> +//
>  BaseAddress   = MemorySpaceMap[Index].BaseAddress;
>  MemorySpaceLength = MemorySpaceMap[Index].Length;
>  while (MemorySpaceLength > 0) {
> @@ -842,23 +870,26 @@ RefreshGcdMemoryAttributesFromPaging (
>  PageStartAddress  = (*PageEntry) & 
> (UINT64)PageAttributeToMask(PageAttribute);
>  PageLength= PageAttributeToLength (PageAttribute) - 
> (BaseAddress - PageStartAddress);
>  Attributes= GetAttributesFromPageEntry (PageEntry);
> -
> -if (Attributes != (MemorySpaceMap[Index].Attributes & 
> EFI_MEMORY_PAGETYPE_MASK)) {
> -  

Re: [edk2] [platforms: PATCH 2/4] Marvell/Drivers: MvSpiDxe: Enable using driver in RT

[Marcin Wojtas]

Leif

2017-11-09 14:44 GMT+01:00 Leif Lindholm :
> On Sun, Nov 05, 2017 at 11:55:37AM +0100, Marcin Wojtas wrote:
>> This patch applies necessary modifications, which allow to use
>> MvSpiDxe driver in variable support as a runtime service.
>> Its type is modified to DXE_RUNTIME_DRIVER, as well as
>> a new callback is introduced as a part of the SpiMasterProtocol.
>> It is supposed to configure the memory space for mmio access to
>> the host controller registers. Moreover gBS locking usage in
>> MvSpiTransfer is limited to the firmware, as the runtime access
>> to the flash is protected within the OS.
>
> Break the commit message up a bit:
> ---
> This patch applies necessary modifications, which allow to use
> MvSpiDxe driver in variable support as a runtime service.
>
> Its type is modified to DXE_RUNTIME_DRIVER, as well as
> a new callback is introduced as a part of the SpiMasterProtocol.
> ---
>
> And then this needs rewording
> ---
> It is supposed to configure the memory space for mmio access to
> the host controller registers.
> ---
> (Say what it does, not what it should be doing.)
>
> ---
> Moreover gBS locking usage in MvSpiTransfer is limited to the
> firmware, as the runtime access to the flash is protected within the
> OS.
> ---
> And "is limited to the firmware". Just because it is used at runtime
> does not make it not firmware. I would say something like:
> "Apply locking in the driver only during boot services. Once at
> runtime, resource protection is handled by the operating system.".
>
> Also, "Its" -> "The driver's" and "It" -> "The driver".
>
> Other than that, can you move the Depex addition here from 4/4 please?
>
> /

Sure.

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

Re: [edk2] [platforms: PATCH 2/4] Marvell/Drivers: MvSpiDxe: Enable using driver in RT

[Leif Lindholm]

On Sun, Nov 05, 2017 at 11:55:37AM +0100, Marcin Wojtas wrote:
> This patch applies necessary modifications, which allow to use
> MvSpiDxe driver in variable support as a runtime service.
> Its type is modified to DXE_RUNTIME_DRIVER, as well as
> a new callback is introduced as a part of the SpiMasterProtocol.
> It is supposed to configure the memory space for mmio access to
> the host controller registers. Moreover gBS locking usage in
> MvSpiTransfer is limited to the firmware, as the runtime access
> to the flash is protected within the OS.

Break the commit message up a bit:
---
This patch applies necessary modifications, which allow to use
MvSpiDxe driver in variable support as a runtime service.

Its type is modified to DXE_RUNTIME_DRIVER, as well as
a new callback is introduced as a part of the SpiMasterProtocol.
---

And then this needs rewording
---
It is supposed to configure the memory space for mmio access to
the host controller registers.
---
(Say what it does, not what it should be doing.)

---
Moreover gBS locking usage in MvSpiTransfer is limited to the
firmware, as the runtime access to the flash is protected within the
OS.
---
And "is limited to the firmware". Just because it is used at runtime
does not make it not firmware. I would say something like:
"Apply locking in the driver only during boot services. Once at
runtime, resource protection is handled by the operating system.".

Also, "Its" -> "The driver's" and "It" -> "The driver".

Other than that, can you move the Depex addition here from 4/4 please?

/
Leif

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas 
> ---
>  Platform/Marvell/Drivers/Spi/MvSpiDxe.c   | 50 ++--
>  Platform/Marvell/Drivers/Spi/MvSpiDxe.h   |  2 +
>  Platform/Marvell/Drivers/Spi/MvSpiDxe.inf |  4 +-
>  Platform/Marvell/Include/Protocol/Spi.h   |  7 +++
>  4 files changed, 58 insertions(+), 5 deletions(-)
> 
> diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c 
> b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
> index c60a520..bab6cf4 100755
> --- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
> +++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
> @@ -211,7 +211,9 @@ MvSpiTransfer (
>  
>Length = 8 * DataByteCount;
>  
> -  EfiAcquireLock (>Lock);
> +  if (!EfiAtRuntime ()) {
> +EfiAcquireLock (>Lock);
> +  }
>  
>if (Flag & SPI_TRANSFER_BEGIN) {
>  SpiActivateCs (Slave);
> @@ -254,7 +256,9 @@ MvSpiTransfer (
>  SpiDeactivateCs (Slave);
>}
>  
> -  EfiReleaseLock (>Lock);
> +  if (!EfiAtRuntime ()) {
> +EfiReleaseLock (>Lock);
> +  }
>  
>return EFI_SUCCESS;
>  }
> @@ -338,6 +342,44 @@ MvSpiFreeSlave (
>return EFI_SUCCESS;
>  }
>  
> +EFI_STATUS
> +EFIAPI
> +MvSpiConfigRuntime (
> +  IN SPI_DEVICE *Slave
> +  )
> +{
> +  EFI_STATUS Status;
> +  UINTN AlignedAddress;
> +
> +  //
> +  // Host register base may be not aligned to the page size,
> +  // which is not accepted when setting memory space attributes.
> +  // Add one aligned page of memory space which covers the host
> +  // controller registers.
> +  //
> +  AlignedAddress = Slave->HostRegisterBaseAddress & ~(SIZE_4KB - 1);
> +
> +  Status = gDS->AddMemorySpace (EfiGcdMemoryTypeMemoryMappedIo,
> +  AlignedAddress,
> +  SIZE_4KB,
> +  EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
> +  if (EFI_ERROR (Status)) {
> +DEBUG ((DEBUG_ERROR, "%a: Failed to add memory space\n", __FUNCTION__));
> +return Status;
> +  }
> +
> +  Status = gDS->SetMemorySpaceAttributes (AlignedAddress,
> +  SIZE_4KB,
> +  EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
> +  if (EFI_ERROR (Status)) {
> +DEBUG ((DEBUG_ERROR, "%a: Failed to set memory attributes\n", 
> __FUNCTION__));
> +gDS->RemoveMemorySpace (AlignedAddress, SIZE_4KB);
> +return Status;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
>  STATIC
>  EFI_STATUS
>  SpiMasterInitProtocol (
> @@ -350,6 +392,7 @@ SpiMasterInitProtocol (
>SpiMasterProtocol->FreeDevice  = MvSpiFreeSlave;
>SpiMasterProtocol->Transfer= MvSpiTransfer;
>SpiMasterProtocol->ReadWrite   = MvSpiReadWrite;
> +  SpiMasterProtocol->ConfigRuntime = MvSpiConfigRuntime;
>  
>return EFI_SUCCESS;
>  }
> @@ -363,8 +406,7 @@ SpiMasterEntryPoint (
>  {
>EFI_STATUS  Status;
>  
> -  mSpiMasterInstance = AllocateZeroPool (sizeof (SPI_MASTER));
> -
> +  mSpiMasterInstance = AllocateRuntimeZeroPool (sizeof (SPI_MASTER));
>if (mSpiMasterInstance == NULL) {
>  return EFI_OUT_OF_RESOURCES;
>}
> diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h 
> b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
> index e7e280a..50cdc02 100644
> --- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
> +++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
> @@ -38,10 +38,12 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH 
> DAMAGE.
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
> 

Re: [edk2] [platforms: PATCH 1/4] Marvell/Drivers: MvSpiFlash: Enable using driver in RT

[Leif Lindholm]

On Sun, Nov 05, 2017 at 11:55:36AM +0100, Marcin Wojtas wrote:
> This patch applies necessary modifications, which allow to use
> MvSpiFlash driver in variable support as a runtime service.
> Its type is modified to DXE_RUNTIME_DRIVER, as well as
> an event is created, which converts the pointers to the
> SpiMasterProtocol and its routines.

Please also move the Depex addition here from 4/4.
The rest looks fine.

/
Leif

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas 
> ---
>  Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c   | 58 ++--
>  Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h   |  1 +
>  Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf |  6 +-
>  3 files changed, 60 insertions(+), 5 deletions(-)
> 
> diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c 
> b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
> index 456d9f9..6886d01 100755
> --- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
> +++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
> @@ -33,6 +33,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  
> ***/
>  #include "MvSpiFlash.h"
>  
> +STATIC EFI_EVENTmMvSpiFlashVirtualAddrChangeEvent;
>  MARVELL_SPI_MASTER_PROTOCOL *SpiMasterProtocol;
>  SPI_FLASH_INSTANCE  *mSpiFlashInstance;
>  
> @@ -503,6 +504,33 @@ MvSpiFlashInitProtocol (
>return EFI_SUCCESS;
>  }
>  
> +/**
> +  Fixup internal data so that EFI can be call in virtual mode.
> +  Call the passed in Child Notify event and convert any pointers in
> +  lib to virtual mode.
> +
> +  @param[in]Event   The Event that is being processed
> +  @param[in]Context Event Context
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +MvSpiFlashVirtualNotifyEvent (
> +  IN EFI_EVENTEvent,
> +  IN VOID *Context
> +  )
> +{
> +  //
> +  // Convert SpiMasterProtocol callbacks in MvSpiFlashErase and
> +  // MvSpiFlashWrite required by runtime variable support.
> +  //
> +  EfiConvertPointer (0x0, (VOID**)>ReadWrite);
> +  EfiConvertPointer (0x0, (VOID**)>Transfer);
> +  EfiConvertPointer (0x0, (VOID**));
> +
> +  return;
> +}
> +
>  EFI_STATUS
>  EFIAPI
>  MvSpiFlashEntryPoint (
> @@ -522,8 +550,7 @@ MvSpiFlashEntryPoint (
>  return EFI_DEVICE_ERROR;
>}
>  
> -  mSpiFlashInstance = AllocateZeroPool (sizeof (SPI_FLASH_INSTANCE));
> -
> +  mSpiFlashInstance = AllocateRuntimeZeroPool (sizeof (SPI_FLASH_INSTANCE));
>if (mSpiFlashInstance == NULL) {
>  DEBUG((DEBUG_ERROR, "SpiFlash: Cannot allocate memory\n"));
>  return EFI_OUT_OF_RESOURCES;
> @@ -540,10 +567,33 @@ MvSpiFlashEntryPoint (
>NULL
>);
>if (EFI_ERROR (Status)) {
> -FreePool (mSpiFlashInstance);
>  DEBUG((DEBUG_ERROR, "SpiFlash: Cannot install SPI flash protocol\n"));
> -return EFI_DEVICE_ERROR;
> +goto ErrorInstallProto;
> +  }
> +
> +  //
> +  // Register for the virtual address change event
> +  //
> +  Status = gBS->CreateEventEx (EVT_NOTIFY_SIGNAL,
> +  TPL_NOTIFY,
> +  MvSpiFlashVirtualNotifyEvent,
> +  NULL,
> +  ,
> +  );
> +  if (EFI_ERROR (Status)) {
> +DEBUG ((DEBUG_ERROR, "%a: Failed to register VA change event\n", 
> __FUNCTION__));
> +goto ErrorCreateEvent;
>}
>  
>return EFI_SUCCESS;
> +
> +ErrorCreateEvent:
> +  gBS->UninstallMultipleProtocolInterfaces (>Handle,
> +,
> +NULL);
> +
> +ErrorInstallProto:
> +  FreePool (mSpiFlashInstance);
> +
> +  return EFI_SUCCESS;
>  }
> diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h 
> b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
> index f09ff50..f69c562 100755
> --- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
> +++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
> @@ -42,6 +42,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf 
> b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf
> index 6587f69..200a00c 100644
> --- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf
> +++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf
> @@ -33,7 +33,7 @@
>INF_VERSION= 0x00010005
>BASE_NAME  = SpiFlashDxe
>FILE_GUID  = 49d7fb74-306d-42bd-94c8-c0c54b181dd7
> -  MODULE_TYPE= DXE_DRIVER
> +  MODULE_TYPE= DXE_RUNTIME_DRIVER
>VERSION_STRING = 1.0
>ENTRY_POINT= MvSpiFlashEntryPoint
>  
> @@ -54,6 +54,10 @@
>UefiLib
>DebugLib
>MemoryAllocationLib
> +  UefiRuntimeLib
> +
> +[Guids]
> +  gEfiEventVirtualAddressChangeGuid
>  
>  [Protocols]
>gMarvellSpiMasterProtocolGuid
> -- 
> 

Re: [edk2] [PATCH 3/4] UefiCpuPkg/MtrrLib: Update algorithm to calculate optimal settings

[Laszlo Ersek]

On 11/09/17 08:11, Ni, Ruiyu wrote:
> The old version is not just slow.
> It cannot work in certain cases. That's why the new version was developed.
> 
> The new version adds a new API which allows caller to pass in the scratch
> buffer instead of using the stack. If a platform has limited stack, it can use
> that API.

(1) OK, so let me summarize:

(1a) Ray and Jeff confirmed that the AP stacks should be affected in
*neither* PiSmmCpuDxeSmm *nor* the MpInitLib client modules (CpuMpPei,
CpuDxe).

(1b) There is a new MtrrLib class API that allows the client module to
pass in a preallocated scratch buffer.

(1a) sounds great.

(1b) is an option we may or may not want to exercise in OVMF. I have the
patches ready for enlarging the temp SEC/PEI RAM (and as part of that,
the temp stack), which is one alternative. However, because OVMF's PEI
phase runs from RAM (and not flash), the other alternative is just to
add a sufficiently large static UINT8 array to PlatformPei, and pass
that as scratch space to MtrrLib.


(2) However: I don't know *how* the new API --
MtrrSetMemoryAttributesInMtrrSettings() -- is supposed to be used. In
OvmfPkg/PlatformPei, we have the following MtrrLib calls:

* OvmfPkg/PlatformPei/Xen.c:

- MtrrSetMemoryAttribute()

(in a loop, basically)

* OvmfPkg/PlatformPei/MemDetect.c:

- IsMtrrSupported()
- MtrrGetAllMtrrs()
- MtrrSetAllMtrrs()
- MtrrSetMemoryAttribute()

Is my understanding correct that MtrrSetMemoryAttribute() is the only
function that is affected?


(3) Is my understanding correct that
MtrrSetMemoryAttributesInMtrrSettings() should be used like this:

(3a) start with MtrrGetAllMtrrs()

(3b) collect all *foreseeable* MtrrSetMemoryAttribute() calls into an
 array of MTRR_MEMORY_RANGE elements

(3c) Perform a batch update on the output of (3a) by calling
 MtrrSetMemoryAttributesInMtrrSettings(). For this, the array from
 (3b), plus a caller-allocated scratch space, have to be passed in,.

(3d) Finally, call MtrrSetAllMtrrs().

Is this correct?

I think we could use this. Jordan, which alternative do you prefer;
larger stack and unchanged code in PlatformPei, or same stack and
updated code in PlatformPei?


(4) Ray: would it be possible to expose SCRATCH_BUFFER_SIZE (with a new
name MTRR_SCRATCH_BUFFER_SIZE) in the library class header? I see the
new RETURN_BUFFER_TOO_SMALL status codes, and I don't really want to
deal with them. The library class header should provide clients with a
size macro that *guarantees* that RETURN_BUFFER_TOO_SMALL will not occur.

Practically speaking, I would use MTRR_SCRATCH_BUFFER_SIZE in the
definition of the static UINT8 array in PlatformPei. (If Jordan prefers
this alternative to the larger temp stack.)

Thanks!
Laszlo

>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>> Jordan Justen
>> Sent: Thursday, November 9, 2017 2:56 PM
>> To: Ni, Ruiyu ; Laszlo Ersek 
>> Cc: Kinney, Michael D ; edk2-
>> de...@lists.01.org; Yao, Jiewen ; Dong, Eric
>> ; Ard Biesheuvel 
>> Subject: Re: [edk2] [PATCH 3/4] UefiCpuPkg/MtrrLib: Update algorithm to
>> calculate optimal settings
>>
>> On 2017-11-08 19:04:35, Ni, Ruiyu wrote:
>>> Jordan, Laszlo,
>>>
>>> I didn't realize that a platform may have less than 4-page stack
>>> before memory is ready. If I was aware of that, I would change the
>>> default scratch buffer size to 2 page, which should be enough too.
>>
>> This does not sound much better. I'm saying that the BASE library should only
>> use at most a few hundred bytes of stack.
>>
>> Apparently the old algorithm did not use much memory, but perhaps was
>> slow? Can we put it back in place for the BASE version of the library?
>> Then, we can add a DXE specific version that uses a large buffer which it can
>> allocate, and potentially free.
>>
>> -Jordan
>> ___
>> 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] MdeModulePkg/Core/Dxe: Remove extra connects for UEFI Applications

[Zeng, Star]

Reviewed-by: Star Zeng 

BTW: I see the code is using Image->ImageContext.ImageType at some places and 
Image->Type at other place, it seems a little inconsistent.

Thanks,
Star
-Original Message-
From: Kinney, Michael D 
Sent: Thursday, November 9, 2017 1:55 AM
To: edk2-devel@lists.01.org
Cc: Zeng, Star ; Dong, Eric 
Subject: [Patch] MdeModulePkg/Core/Dxe: Remove extra connects for UEFI 
Applications

https://bugzilla.tianocore.org/show_bug.cgi?id=765

The UEFI Specification Boot Services chapter, StartImage() service,
EFF 1.10 Extension requires extra calls to ConnectController()
if a UEFI Driver produces handles. The DXE Core is performing these
extra calls to ConnectController() without evaluating the ImageType.

A filter is added to not make extra calls to ConnectController()
if the ImageType is EFI_IMAGE_SUBSYSTEM_EFI_APPLICATION.

Without this filter, extra calls to ConnectController() may be
performed by UEFI Applications or a UEFI Shell Applications that
also call ConnectController().

Cc: Star Zeng 
Cc: Eric Dong 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael D Kinney 
---
 MdeModulePkg/Core/Dxe/Image/Image.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c 
b/MdeModulePkg/Core/Dxe/Image/Image.c
index 4e22aa6dc7..c6b8ff44b9 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -1703,9 +1703,17 @@ CoreStartImage (
   mCurrentImage = LastImage;
 
   //
-  // Go connect any handles that were created or modified while the image 
executed.
+  // UEFI Specification - StartImage() - EFI 1.10 Extension
+  // To maintain compatibility with UEFI drivers that are written to the EFI
+  // 1.02 Specification, StartImage() must monitor the handle database before
+  // and after each image is started. If any handles are created or modified
+  // when an image is started, then EFI_BOOT_SERVICES.ConnectController() must
+  // be called with the Recursive parameter set to TRUE for each of the newly
+  // created or modified handles before StartImage() returns.
   //
-  CoreConnectHandlesByKey (HandleDatabaseKey);
+  if (Image->ImageContext.ImageType != EFI_IMAGE_SUBSYSTEM_EFI_APPLICATION) {
+CoreConnectHandlesByKey (HandleDatabaseKey);
+  }
 
   //
   // Handle the image's returned ExitData
-- 
2.14.2.windows.3

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

Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

[Laszlo Ersek]

On 11/09/17 02:48, Yao, Jiewen wrote:
> FED0 is HPET region. It is MMIO.

Right, we add it in OvmfPkg/PlatformPei/Platform.c:

//
// address   purpose   size
//     -
...
// 0xFED0HPET   1 KB
...
AddIoMemoryBaseSizeHob (0xFED0, SIZE_1KB);

Because this is MMIO and not system memory, I think the 1KB size is
valid, as far as a resource descriptor HOB is concerned.

This goes on to support my earlier suggestion to check the GCD memory
space entry more strictly, for its type.

Anyway, the latest v3 (v4?) approach can handle the entry just as well,
so I don't insist on modifying the entry type check. I was mainly
curious if we did something wrong in OVMF. I believe the above HOB is
valid though.

Thanks
Laszlo


>> -Original Message-
>> From: Wang, Jian J
>> Sent: Thursday, November 9, 2017 8:42 AM
>> To: Laszlo Ersek ; edk2-devel@lists.01.org
>> Cc: Yao, Jiewen ; Dong, Eric 
>> Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
>> RT_CODE in memory map
>>
>> Hi Laszlo,
>>
>> The memory range is FED0 - FED003FF. Actually I don't
>> know if it's for MMIO or not. I might be mess it with other things.
>>
>> Thanks,
>> Jian
>>> -Original Message-
>>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>>> Sent: Wednesday, November 08, 2017 10:17 PM
>>> To: Wang, Jian J ; edk2-devel@lists.01.org
>>> Cc: Yao, Jiewen ; Dong, Eric 
>>> Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
>>> RT_CODE in memory map
>>>
>>> On 11/08/17 01:10, Wang, Jian J wrote:
 Hi Laszlo,

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, November 08, 2017 1:14 AM
> To: Wang, Jian J ; edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Dong, Eric 
> Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
>
> sorry about the late response
>
> On 11/03/17 01:57, Jian J Wang wrote:
>>> v2
>>> a. Fix an issue which will cause setting capability failure if size is 
>>> smaller
>>>than a page.
>>
>> More than one entry of RT_CODE memory might cause boot problem for
> some
>> old OSs. This patch will fix this issue to keep OS compatibility as much
>> as possible.
>>
>> More detailed information, please refer to
>> https://bugzilla.tianocore.org/show_bug.cgi?id=753
>>
>> Cc: Eric Dong 
>> Cc: Jiewen Yao 
>> Cc: Laszlo Ersek 
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Jian J Wang 
>> ---
>>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++
>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>> index d312eb66f8..4a7827ebc9 100644
>> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
>> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>> @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
>>PageLength= 0;
>>
>>for (Index = 0; Index < NumberOfDescriptors; Index++) {
>> -if (MemorySpaceMap[Index].GcdMemoryType ==
> EfiGcdMemoryTypeNonExistent) {
>> +if (MemorySpaceMap[Index].GcdMemoryType ==
> EfiGcdMemoryTypeNonExistent
>> +|| (MemorySpaceMap[Index].BaseAddress &
>> EFI_PAGE_MASK) != 0
>> +|| (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0)
>> {
>>continue;
>>  }
>
> When exactly do the new conditions match?
>
> I thought the base addresses and the lengths in the GCD memory space
>> map
> are all page aligned. Is that not the case?
>
> If these conditions are just a sanity check (i.e. we never expect them
> to fire), then should we perpahs turn them into ASSERT()s?
>

 I found that there's a mmio entry in memory map on OVMF which has size
 less than a page. I didn't encounter this before. Maybe some recent changes
 in other part of EDKII caused this situation. So ASSERT is not enough.
>>>
>>> Can you describe the base address and size of this MMIO entry?
>>>
>>> I don't see how it is possible to add such an entry in the first place
>>> -- if you try to add it in PEI, via a HOB, then IIRC HobLib will
>>> ASSERT(). If you try to add it with gDS->AddMemorySpace() in DXE, then
>>> the call should fail.
>>>
>>> I believe it might be useful to investigate this entry more closely.
>>> Knowing the base address could help us.
>>>
>>> Thanks!
>>> Laszlo

[edk2] [Patch][edk2-platforms/devel-MinnowBoard3-UDK2017] Change BIOS version

[Guo, Mang]

Contributed-under: TianoCore Contribution Agreement 1.1

Signed-off-by: Guo Mang 
---
 Platform/BroxtonPlatformPkg/BiosId.env | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Platform/BroxtonPlatformPkg/BiosId.env 
b/Platform/BroxtonPlatformPkg/BiosId.env
index 97510e5..3993fb2 100644
--- a/Platform/BroxtonPlatformPkg/BiosId.env
+++ b/Platform/BroxtonPlatformPkg/BiosId.env
@@ -31,5 +31,5 @@ BOARD_ID  = APLKRVP
 BOARD_REV = 3
 BUILD_TYPE= D
 VERSION_MAJOR = 0067
-VERSION_MINOR = 03
+VERSION_MINOR = 04
 BOARD_EXT = X64
-- 
2.10.1.windows.1

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

[edk2] [Patch][edk2-platforms/devel-MinnowBoard3-UDK2017] Set MaxPkgCState

[Guo, Mang]

Contributed-under: TianoCore Contribution Agreement 1.1

Signed-off-by: Guo Mang 
---
 .../Board/BensonGlacier/BoardInitPostMem/BoardInit.c  | 11 +--
 .../Board/BensonGlacier/BoardInitPostMem/BoardInitMiscs.h |  7 +++
 .../Board/BensonGlacier/BoardInitPostMem/BoardInitPostMem.inf |  1 +
 .../Board/LeafHill/BoardInitPostMem/BoardInit.c   |  9 -
 .../Board/LeafHill/BoardInitPostMem/BoardInitMiscs.h  |  6 ++
 .../Board/LeafHill/BoardInitPostMem/BoardInitPostMem.inf  |  1 +
 .../Library/PeiFspPolicyInitLib/PeiFspCpuPolicyInitLib.c  |  4 ++--
 .../Library/PeiFspPolicyInitLib/PeiFspPolicyInitLib.inf   |  1 +
 .../Common/Library/PeiPolicyUpdateLib/PeiCpuPolicyUpdate.c|  2 +-
 .../Common/Library/PeiPolicyUpdateLib/PeiPolicyUpdateLib.inf  |  1 +
 .../Common/PlatformSettings/PlatformDxe/Platform.c|  8 +++-
 .../Common/PlatformSettings/PlatformDxe/PlatformDxe.inf   |  3 ++-
 .../Common/PlatformSettings/PlatformSetupDxe/CpuPower.vfi |  6 +++---
 Platform/BroxtonPlatformPkg/PlatformPkg.dec   |  6 --
 14 files changed, 49 insertions(+), 17 deletions(-)

diff --git 
a/Platform/BroxtonPlatformPkg/Board/BensonGlacier/BoardInitPostMem/BoardInit.c 
b/Platform/BroxtonPlatformPkg/Board/BensonGlacier/BoardInitPostMem/BoardInit.c
index 536c390..ab11aaa 100644
--- 
a/Platform/BroxtonPlatformPkg/Board/BensonGlacier/BoardInitPostMem/BoardInit.c
+++ 
b/Platform/BroxtonPlatformPkg/Board/BensonGlacier/BoardInitPostMem/BoardInit.c
@@ -50,6 +50,7 @@ BensonGlacierPostMemInitCallback (
   UINT8FabId;
   UINT8ResetType;
   UINTNBufferSize;
+  UINT8MaxPkgCState;
 
   Status = PeiServicesLocatePpi (
  ,
@@ -88,12 +89,18 @@ BensonGlacierPostMemInitCallback (
   //
   BufferSize = sizeof (EFI_GUID);
   PcdSetPtr(PcdBoardVbtFileGuid, , (UINT8 
*));
-  
+
   //
   // Set PcdSueCreek
   //
   PcdSetBool (PcdSueCreek, TRUE);
-
+
+  //
+  // Set PcdMaxPkgCState
+  //
+  MaxPkgCState = MAX_PKG_CSTATE_C2;
+  PcdSet8 (PcdMaxPkgCState, (UINT8) MaxPkgCState);
+
   //
   // Add init steps here
   //
diff --git 
a/Platform/BroxtonPlatformPkg/Board/BensonGlacier/BoardInitPostMem/BoardInitMiscs.h
 
b/Platform/BroxtonPlatformPkg/Board/BensonGlacier/BoardInitPostMem/BoardInitMiscs.h
index 2cf4810..2ac2859 100644
--- 
a/Platform/BroxtonPlatformPkg/Board/BensonGlacier/BoardInitPostMem/BoardInitMiscs.h
+++ 
b/Platform/BroxtonPlatformPkg/Board/BensonGlacier/BoardInitPostMem/BoardInitMiscs.h
@@ -94,6 +94,13 @@
 #define SUBSYSTEM_DEVICE_ID   0x1234
 #define SUBSYSTEM_SVID_SSID   (SUBSYSTEM_VENDOR_ID + (SUBSYSTEM_DEVICE_ID << 
16))
 
+//
+// MaxPkgCState identifier.
+//
+#define MAX_PKG_CSTATE_C0 0x00
+#define MAX_PKG_CSTATE_C1 0x01
+#define MAX_PKG_CSTATE_C2 0x02
+
 EFI_STATUS
 BensonGetPlatformInfoHob (
   IN CONST EFI_PEI_SERVICES **PeiServices,
diff --git 
a/Platform/BroxtonPlatformPkg/Board/BensonGlacier/BoardInitPostMem/BoardInitPostMem.inf
 
b/Platform/BroxtonPlatformPkg/Board/BensonGlacier/BoardInitPostMem/BoardInitPostMem.inf
index 5989d30..55ec5b7 100644
--- 
a/Platform/BroxtonPlatformPkg/Board/BensonGlacier/BoardInitPostMem/BoardInitPostMem.inf
+++ 
b/Platform/BroxtonPlatformPkg/Board/BensonGlacier/BoardInitPostMem/BoardInitPostMem.inf
@@ -64,6 +64,7 @@
   gPlatformModuleTokenSpaceGuid.PcdResetType
   gPlatformModuleTokenSpaceGuid.PcdBoardVbtFileGuid
   gPlatformModuleTokenSpaceGuid.PcdSueCreek
+  gPlatformModuleTokenSpaceGuid.PcdMaxPkgCState
 
 [Guids]
   gEfiPlatformInfoGuid
diff --git 
a/Platform/BroxtonPlatformPkg/Board/LeafHill/BoardInitPostMem/BoardInit.c 
b/Platform/BroxtonPlatformPkg/Board/LeafHill/BoardInitPostMem/BoardInit.c
index af53b8c..7221f84 100644
--- a/Platform/BroxtonPlatformPkg/Board/LeafHill/BoardInitPostMem/BoardInit.c
+++ b/Platform/BroxtonPlatformPkg/Board/LeafHill/BoardInitPostMem/BoardInit.c
@@ -58,6 +58,7 @@ LeafHillPostMemInitCallback (
   UINT8FabId;
   UINT8ResetType;
   UINTNBufferSize;
+  UINT8MaxPkgCState;
 
   Status = PeiServicesLocatePpi (
  ,
@@ -101,7 +102,13 @@ LeafHillPostMemInitCallback (
   // Set PcdSueCreek
   //
   PcdSetBool (PcdSueCreek, FALSE);
-
+
+  //
+  // Set PcdMaxPkgCState
+  //
+  MaxPkgCState = MAX_PKG_CSTATE_C2;
+  PcdSet8 (PcdMaxPkgCState, (UINT8) MaxPkgCState);
+
   //
   // Add init steps here
   //
diff --git 
a/Platform/BroxtonPlatformPkg/Board/LeafHill/BoardInitPostMem/BoardInitMiscs.h 
b/Platform/BroxtonPlatformPkg/Board/LeafHill/BoardInitPostMem/BoardInitMiscs.h
index 98100c2..c1ba128 100644
--- 
a/Platform/BroxtonPlatformPkg/Board/LeafHill/BoardInitPostMem/BoardInitMiscs.h
+++ 
b/Platform/BroxtonPlatformPkg/Board/LeafHill/BoardInitPostMem/BoardInitMiscs.h
@@ -86,6 +86,12 @@
 #define 

Re: [edk2] [PATCH edk2-non-osi v1] Hisilicon D0x: Remove uncacheable attribute from memory resource HOB

[Ard Biesheuvel]

On 9 November 2017 at 03:30, Ming Huang  wrote:
> If uncacheable attribute is included in memory resource HOB,
> GCD spaces will also have EFI_MEMORY_UC capability,
> then NonCoherentPciIoAllocateBuffer of NonDiscoverablePciDeviceDxe
> module will allocate DMA buffer of EFI_MEMORY_UC type, which will
> cause alignment fault exception with BaseMemoryLibOptDxe.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Liu Yi 
> Signed-off-by: Heyi Guo 

Acked-by: Ard Biesheuvel 

As for the commit log, this not only affects
NonDiscoverablePciDeviceDxe, it removes the UC attribute from all DRAM
regions in the UEFI memory map, which makes much more sense on ARM.

> ---
>  Platform/Hisilicon/D03/MemoryInitPei/MemoryInit.efi | Bin 90272 -> 90336 
> bytes
>  Platform/Hisilicon/D05/MemoryInitPei/MemoryInit.efi | Bin 152576 -> 152480 
> bytes
>  2 files changed, 0 insertions(+), 0 deletions(-)
>
> diff --git a/Platform/Hisilicon/D03/MemoryInitPei/MemoryInit.efi 
> b/Platform/Hisilicon/D03/MemoryInitPei/MemoryInit.efi
> index 354abcc..31e2903 100644
> Binary files a/Platform/Hisilicon/D03/MemoryInitPei/MemoryInit.efi and 
> b/Platform/Hisilicon/D03/MemoryInitPei/MemoryInit.efi differ
> diff --git a/Platform/Hisilicon/D05/MemoryInitPei/MemoryInit.efi 
> b/Platform/Hisilicon/D05/MemoryInitPei/MemoryInit.efi
> index b94e0cb..eb71c44 100644
> Binary files a/Platform/Hisilicon/D05/MemoryInitPei/MemoryInit.efi and 
> b/Platform/Hisilicon/D05/MemoryInitPei/MemoryInit.efi differ
> --
> 1.9.1
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] GRUB issue on device priority

[Haojian Zhuang]

On 2017/11/8 21:53, Leif Lindholm wrote:

On Tue, Nov 07, 2017 at 10:02:30PM +0800, Haojian Zhuang wrote:

Hi all,

It seems there's a device priority issue in GRUB.


GRUB is behaving as expected.


All block io handles are linked into the list in edk2, and GRUB could fetch
it. Then GRUB creates its own ascending on HD priority.
add_device():
   for (p = devices; *p; p = &((*p)->next)) {
 ret = grub_efi_compare_device_paths (grub_efi_find_last_device_path
((*p->device_path),
  grub_efi_find_last_device_path->device_path));
 if (ret == 0) {
   ret = grub_efi_compare_device_paths ((*p)->device_path,
d->device_path);
 }
 if (ret == 0) {
   return;
 } else if (ret > 0) {
   break;
 }
   }
   ...


UEFI guarantees no ordering of block devices. GRUB is a UEFI
application, so it cannot assume anything with regards to device
ordering.


In the HiKey platform, I prepared the same driver for both eMMC and
SD. So the device paths are in below.
SD: /HardwareVendor(0d51905b-b77e-452a-a2c0-eca0cc8d514a)[9: 00 e0 23 f7 00 00 
00 00 00 ]/UnknownMessaging(1a)/EndEntire
eMMC: /HardwareVendor(0d51905b-b77e-452a-a2c0-eca0cc8d514a)[9: 00 d0 23 f7 00 
00 00 00 00]/UnknownMessaging(1d)/Ctrl(0)/EndEntire

#define MSG_SD_DP   0x1A
#define MSG_EMMC_DP 0x1D

In the second level, the device paths are different.

And GRUB resort the sequence by ascending order (with above
code). So SD device always gets higher priority than eMMC device.

If we always use installer to install OS, it may not an issue. Since
installer could create grub.cfg by itself. But it imports another
issue on lacking of persistent variable storage. And we need to
deploy system without installer on embedded device.


Yes, this is the bit which is interesting, and why I requested you
post this problem to edk2-devel.

I believe that we need to have a sensible way to deal with embedded
platforms that do not have operating systems "installed" to them, but
rather written as images to flash devices.

So, can you clarify a few things for me?:
- Where is GRUB located in your (pre-SD) system?


In eMMC.

- Where is GRUB located in your SD system?

In SD.

- Are you looking for a way to support GRUB being located on either
   eMMC or SD? Or are you looking to always have GRUB loaded from eMMC?


GRUB could be located in either in eMMC or SD.

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

[edk2] Questions about SubmitResources()

[Tiger Liu]

Hi, experts:
I have a question about PciHostBridge’s SubmitResources() function.

For example:

1.  PciBus driver scanned a pci bus, found no pci devices needed resource, 
such as : MMIO ranges

Then, Mem32Bridge->Length:0x0, Mem32Bridge->Alignment:0Xf , and 
AddrSpaceGranularity = 0

Then call SubmitResources function, and in this function:

   ……

   /// It seems code here will cause system abnormal. (because 
AddrSpaceGranularity = 0)

  if (Ptr->AddrSpaceGranularity != 32 && Ptr->AddrSpaceGranularity != 64) {

DEBUG((EFI_D_INFO, "BadAddrSpaceGranularity:%d\n", 
Ptr->AddrSpaceGranularity));

return EFI_INVALID_PARAMETER;

  }

Thanks


保密声明：
本邮件含有保密或专有信息，仅供指定收件人使用。严禁对本邮件或其内容做任何未经授权的查阅、使用、复制或转发。
CONFIDENTIAL NOTE:
This email contains confidential or legally privileged information and is for 
the sole use of its intended recipient. Any unauthorized review, use, copying 
or forwarding of this email or the content of this email is strictly prohibited.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel