[edk2] [PATCH V2] SecurityPkg OpalPasswordDxe:Fix wrong BufferSize input to UnicodeSPrint

[Star Zeng]

Current code uses string length as BufferSize input to UnicodeSPrint,
it is wrong and makes the pop up string trimmed. The BufferSize input
to UnicodeSPrint should be the size, in bytes, of the output buffer.

This is to use sizeof (mPopUpString) as the BufferSize input to
UnicodeSPrint, it also updates array size of mPopUpString from 256 to
100 that is enough, otherwise the pop up string may be too long.

Cc: Jiewen Yao 
Cc: Eric Dong 
Cc: Chao Zhang 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.c | 15 +--
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.c 
b/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.c
index 1b55bbe4ecb8..6344deb86750 100644
--- a/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.c
+++ b/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.c
@@ -27,7 +27,7 @@ EFI_GUID mOpalDeviceNvmeGuid = OPAL_DEVICE_NVME_GUID;
 BOOLEAN mOpalEndOfDxe = FALSE;
 OPAL_REQUEST_VARIABLE   *mOpalRequestVariable = NULL;
 UINTN   mOpalRequestVariableSize = 0;
-CHAR16  mPopUpString[256];
+CHAR16  mPopUpString[100];
 
 typedef struct {
   UINT32   Address;
@@ -908,9 +908,9 @@ OpalDriverPopUpPasswordInput (
 }
 
 /**
-  Check if disk is locked, show popup window and ask for password if it is.
+  Get pop up string.
 
-  @param[in] DevThe device which need to be unlocked.
+  @param[in] DevThe OPAL device.
   @param[in] RequestString  Request string.
 
 **/
@@ -920,15 +920,10 @@ OpalGetPopUpString (
   IN CHAR16 *RequestString
   )
 {
-  UINTN StrLength;
-
-  StrLength = StrLen (RequestString) + 1 + MAX (StrLen (Dev->Name16), StrLen 
(L"Disk"));
-  ASSERT (StrLength < sizeof (mPopUpString) / sizeof (CHAR16));
-
   if (Dev->Name16 == NULL) {
-UnicodeSPrint (mPopUpString, StrLength + 1, L"%s Disk", RequestString);
+UnicodeSPrint (mPopUpString, sizeof (mPopUpString), L"%s Disk", 
RequestString);
   } else {
-UnicodeSPrint (mPopUpString, StrLength + 1, L"%s %s", RequestString, 
Dev->Name16);
+UnicodeSPrint (mPopUpString, sizeof (mPopUpString), L"%s %s", 
RequestString, Dev->Name16);
   }
 
   return mPopUpString;
-- 
2.7.0.windows.1

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

Re: [edk2] [PATCH v6 0/6] Add translation support to generic PciHostBridge

[Guo Heyi]

On Thu, Mar 15, 2018 at 01:27:59PM +0800, Ni, Ruiyu wrote:
> On 3/15/2018 12:00 PM, Heyi Guo wrote:
> >v6:
> >- Patch 1, 2: implement 3 comments from Laszlo.
> >- Patch 4: implement 3 comments from Ray.
> >
> >Patch v5 inherits the code from RFC v4; we don't restart the version number 
> >for
> >RFC to PATCH change.
> >
> >v5:
> >- Patch 4/6: Modify the code according to the comments from Ray.
> >- Patch 1/6 and 2/6 are totally new. They add initialization for all fields 
> >of
> >   PCI_ROOT_BRIDGE_APERTURE temporary variables in PciHostBridgeLib 
> > instances, so
> >   that they will not suffer from extension of PCI_ROOT_BRIDGE_APERTURE
> >   structure.
> >- Generate a separate patch (3/6) for PciHostBridgeLib.h change. Though it 
> >is a
> >   prerequisite for patch 4/6, it does not change the code in PciHostBridge
> >   driver and won't cause any build failure or functional issue.
> >
> >
> >v4:
> >- Modify the code according to the comments from Ray, Laszlo and Ard (Please 
> >see
> >   the notes of Patch 1/3)
> >- Ignore translation of bus in CreateRootBridge.
> >
> >
> >v3:
> >- Keep definition of Translation consistent in EDKII code: Translation = 
> >device
> >   address - host address.
> >- Patch 2/2 is split into 2 patches (2/3 and 3/3).
> >- Refine comments and commit messages to make the code easier to understand.
> >
> >
> >v2:
> >Changs are made according to the discussion on the mailing list, including:
> >
> >- PciRootBridgeIo->Configuration should return CPU view address, as well as
> >   PciIo->GetBarAttributes, and Translation Offset should be equal to PCI 
> > view
> >   address - CPU view address.
> >- Add translation offset to PCI_ROOT_BRIDGE_APERTURE structure definition.
> >- PciHostBridge driver internally used Base Address is still based on PCI 
> >view
> >   address, and translation offset = CPU view - PCI view, which follows the
> >   definition in ACPI, and not the same as that in UEFI spec.
> >
> >Cc: Ruiyu Ni 
> >Cc: Ard Biesheuvel 
> >Cc: Star Zeng 
> >Cc: Eric Dong 
> >Cc: Laszlo Ersek 
> >Cc: Michael D Kinney 
> >Cc: Maurice Ma 
> >Cc: Prince Agyeman 
> >Cc: Benjamin You 
> >Cc: Jordan Justen 
> >Cc: Anthony Perard 
> >Cc: Julien Grall 
> >
> >
> >Heyi Guo (6):
> >   CorebootPayloadPkg/PciHostBridgeLib: clear aperture vars for (re)init
> >   OvmfPkg/PciHostBridgeLib: clear PCI aperture vars for (re)init
> >   MdeModulePkg/PciHostBridgeLib.h: add address Translation
> >   MdeModulePkg/PciHostBridgeDxe: Add support for address translation
> >   MdeModulePkg/PciBus: convert host address to device address
> >   MdeModulePkg/PciBus: return CPU address for GetBarAttributes
> >
> >  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h  |  21 
> > +++
> >  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h|   3 +
> >  MdeModulePkg/Include/Library/PciHostBridgeLib.h|  19 
> > +++
> >  CorebootPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c |   7 +-
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c |  12 +-
> >  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c  | 129 
> > ---
> >  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c| 135 
> > ++--
> >  OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c|   4 +
> >  OvmfPkg/Library/PciHostBridgeLib/XenSupport.c  |   7 +-
> >  9 files changed, 306 insertions(+), 31 deletions(-)
> >
> Heyi,
> I have given the Reviewed-by for the whole patch series. So please just add
> my R-b if you want to send further version of patches.
> I only have a minor comment for #4 patch. I don't think you need to send
> another version of #4 patch. You can just change the code when committing.

Thanks, but I don't have commit privilege myself, so maybe a v7 is needed, and I
will add your R-b for the whole series :)

Regards,
Heyi

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

Re: [edk2] [PATCH v3 0/3] fix empty window period of HeapGuard before CpuArchProtocol

[Ni, Ruiyu]

On 3/15/2018 1:03 PM, Jian J Wang wrote:

v3:
Split the fixes into three patch files.



v2:
Fix a logic hole in bits operation on address on 64K boundary with
just 64-bit length (SetBits(), ClearBits(), GetBits()).


This patch series fills up an empty window period of HeapGuard feature
before CpuArchProtocol installed, and fix an issue observed on 32-bit
real platform with this HeapGuard fix.

Jian J Wang (3):
   MdeModulePkg/Core: allow HeapGuard even before CpuArchProtocol
 installed
   MdeModulePkg/PiSmmCore: fix bits operation error on a boundary
 condition
   MdeModulePkg/Core: fix bits operation error on a boundary condition

  MdeModulePkg/Core/Dxe/Gcd/Gcd.c   |  10 ++
  MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 148 --
  MdeModulePkg/Core/Dxe/Mem/HeapGuard.h |   8 ++
  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c |   5 +
  MdeModulePkg/Core/PiSmmCore/HeapGuard.c   |  16 +--
  5 files changed, 174 insertions(+), 13 deletions(-)


Reviewed-by: Ruiyu Ni 

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

Re: [edk2] [PATCH v6 0/6] Add translation support to generic PciHostBridge

[Ni, Ruiyu]

On 3/15/2018 12:00 PM, Heyi Guo wrote:

v6:
- Patch 1, 2: implement 3 comments from Laszlo.
- Patch 4: implement 3 comments from Ray.

Patch v5 inherits the code from RFC v4; we don't restart the version number for
RFC to PATCH change.

v5:
- Patch 4/6: Modify the code according to the comments from Ray.
- Patch 1/6 and 2/6 are totally new. They add initialization for all fields of
   PCI_ROOT_BRIDGE_APERTURE temporary variables in PciHostBridgeLib instances, 
so
   that they will not suffer from extension of PCI_ROOT_BRIDGE_APERTURE
   structure.
- Generate a separate patch (3/6) for PciHostBridgeLib.h change. Though it is a
   prerequisite for patch 4/6, it does not change the code in PciHostBridge
   driver and won't cause any build failure or functional issue.


v4:
- Modify the code according to the comments from Ray, Laszlo and Ard (Please see
   the notes of Patch 1/3)
- Ignore translation of bus in CreateRootBridge.


v3:
- Keep definition of Translation consistent in EDKII code: Translation = device
   address - host address.
- Patch 2/2 is split into 2 patches (2/3 and 3/3).
- Refine comments and commit messages to make the code easier to understand.


v2:
Changs are made according to the discussion on the mailing list, including:

- PciRootBridgeIo->Configuration should return CPU view address, as well as
   PciIo->GetBarAttributes, and Translation Offset should be equal to PCI view
   address - CPU view address.
- Add translation offset to PCI_ROOT_BRIDGE_APERTURE structure definition.
- PciHostBridge driver internally used Base Address is still based on PCI view
   address, and translation offset = CPU view - PCI view, which follows the
   definition in ACPI, and not the same as that in UEFI spec.

Cc: Ruiyu Ni 
Cc: Ard Biesheuvel 
Cc: Star Zeng 
Cc: Eric Dong 
Cc: Laszlo Ersek 
Cc: Michael D Kinney 
Cc: Maurice Ma 
Cc: Prince Agyeman 
Cc: Benjamin You 
Cc: Jordan Justen 
Cc: Anthony Perard 
Cc: Julien Grall 


Heyi Guo (6):
   CorebootPayloadPkg/PciHostBridgeLib: clear aperture vars for (re)init
   OvmfPkg/PciHostBridgeLib: clear PCI aperture vars for (re)init
   MdeModulePkg/PciHostBridgeLib.h: add address Translation
   MdeModulePkg/PciHostBridgeDxe: Add support for address translation
   MdeModulePkg/PciBus: convert host address to device address
   MdeModulePkg/PciBus: return CPU address for GetBarAttributes

  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h  |  21 +++
  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h|   3 +
  MdeModulePkg/Include/Library/PciHostBridgeLib.h|  19 +++
  CorebootPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c |   7 +-
  MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c |  12 +-
  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c  | 129 
---
  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c| 135 
++--
  OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c|   4 +
  OvmfPkg/Library/PciHostBridgeLib/XenSupport.c  |   7 +-
  9 files changed, 306 insertions(+), 31 deletions(-)


Heyi,
I have given the Reviewed-by for the whole patch series. So please just 
add my R-b if you want to send further version of patches.
I only have a minor comment for #4 patch. I don't think you need to send 
another version of #4 patch. You can just change the code when committing.

Thanks,
Ray

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

Re: [edk2] [PATCH v6 4/6] MdeModulePkg/PciHostBridgeDxe: Add support for address translation

[Ni, Ruiyu]

On 3/15/2018 12:00 PM, Heyi Guo wrote:

PCI address translation is necessary for some non-x86 platforms. On
such platforms, address value (denoted as "device address" or "address
in PCI view") set to PCI BAR registers in configuration space might be
different from the address which is used by CPU to access the
registers in memory BAR or IO BAR spaces (denoted as "host address" or
"address in CPU view"). The difference between the two addresses is
called "Address Translation Offset" or simply "translation", and can
be represented by "Address Translation Offset" in ACPI QWORD Address
Space Descriptor (Offset 0x1E). However UEFI and ACPI differs on the
definitions of QWORD Address Space Descriptor, and we will follow UEFI
definition on UEFI protocols, such as PCI root bridge IO protocol and
PCI IO protocol. In UEFI 2.7, "Address Translation Offset" is "Offset
to apply to the Starting address to convert it to a PCI address". This
means:

1. Translation = device address - host address.

2. PciRootBridgeIo->Configuration should return CPU view address, as
well as PciIo->GetBarAttributes.

Summary of addresses used in protocol interfaces and internal
implementations:

1. *Only* the following protocol interfaces assume Address is Device
Address:
(1). PciHostBridgeResourceAllocation.GetProposedResources()
  Otherwise PCI bus driver cannot set correct address into PCI
  BARs.
(2). PciRootBridgeIo.Mem.Read() and PciRootBridgeIo.Mem.Write()
(3). PciRootBridgeIo.CopyMem()
UEFI and PI spec have clear statements for all other protocol
interfaces about the address type.

2. Library interfaces and internal implementation:
(1). Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address.
  It is easy to check whether the address is below 4G or above 4G.
(2). Addresses in PCI_ROOT_BRIDGE_INSTANCE.ResAllocNode are host
  address, for they are allocated from GCD.
(3). Address passed to PciHostBridgeResourceConflict is host address,
  for it comes from PCI_ROOT_BRIDGE_INSTANCE.ResAllocNode.

RESTRICTION: to simplify the situation, we require the alignment of
Translation must be larger than any BAR alignment in the same root
bridge, so that resource allocation alignment can be applied to both
device address and host address.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo 
Signed-off-by: Yi Li 
Cc: Ruiyu Ni 
Cc: Ard Biesheuvel 
Cc: Star Zeng 
Cc: Eric Dong 
Cc: Laszlo Ersek 
Cc: Michael D Kinney 
---

Notes:
 v6:
 - Change ASSERT(FALSE) to ASSERT ((Translation & Alignment) == 0) and
   move ASSERT before if-check [Ray]
 - Print translation of bus and then assert [Ray]
 - Add function header for RootBridgeIoGetMemTranslationByAddress()
   [Ray]
 
 v5:

 - Add check for the alignment of Translation against the BAR alignment
   [Ray]
 - Keep coding style of comments consistent with the context [Ray]
 - Comment change for Base in PCI_RES_NODE [Ray]
 - Add macros of TO_HOST_ADDRESS and TO_DEVICE_ADDRESS for address type
   conversion (After that we can also simply the comments about the
   calculation [Ray]
 - Add check for bus translation offset in CreateRootBridge(), making
   sure it is zero, and unify code logic for all types of resource
   after that [Ray]
 - Use GetTranslationByResourceType() to simplify the code in
   RootBridgeIoConfiguration() (also fix a bug in previous patch
   version of missing a break after case TypePMem64) [Ray]
 - Commit message refinement [Ray]
 
 v4:

 - Add ASSERT (FALSE) to default branch in GetTranslationByResourceType
   [Laszlo]
 - Fix bug when passing BaseAddress to gDS->AllocateIoSpace and
   gDS->AllocateMemorySpace [Laszlo]
 - Add comment for applying alignment to both device address and host
   address, and add NOTE for the alignment requirement of Translation,
   as well as in commit message [Laszlo][Ray]
 - Improve indention for the code in CreateRootBridge [Laszlo]
 - Improve comment for Translation in PCI_ROOT_BRIDGE_APERTURE
   definition [Laszlo]
 - Ignore translation of bus in CreateRootBridge
 
 v4:

 - Add ASSERT (FALSE) to default branch in GetTranslationByResourceType
   [Laszlo]
 - Fix bug when passing BaseAddress to gDS->AllocateIoSpace and
   gDS->AllocateMemorySpace [Laszlo]
 - Add comment for applying alignment to both device address and host
   address, and add NOTE for the alignment requirement of Translation,
   as well as in commit message [Laszlo][Ray]
 - Improve indention for the code in CreateRootBridge [Laszlo]
 - Improve comment for Translation in PCI_ROOT_BRIDGE_APERTURE
   definition [Laszlo]
 - Ignore translation of bus in 

Re: [edk2] [PATCH] MdeModulePkg/Core: allow HeapGuard even before CpuArchProtocol installed

[Wang, Jian J]

Thanks catching this. New patch has sent out.

Regards,
Jian


> -Original Message-
> From: Ni, Ruiyu
> Sent: Thursday, March 15, 2018 11:47 AM
> To: Wang, Jian J ; edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Dong, Eric ;
> Zeng, Star 
> Subject: Re: [edk2] [PATCH] MdeModulePkg/Core: allow HeapGuard even
> before CpuArchProtocol installed
> 
> On 3/15/2018 10:27 AM, Jian J Wang wrote:
> >> v2 changes:
> >> Fix a logic hole in bits operation on address on 64K boundary with
> >> just 64-bit length (SetBits(), ClearBits(), GetBits()).
> >
> > Due to the fact that HeapGuard needs CpuArchProtocol to update page
> > attributes, the feature is normally enabled after CpuArchProtocol is
> > installed. Since there're some drivers are loaded before CpuArchProtocl,
> > they cannot make use HeapGuard feature to detect potential issues.
> >
> > This patch fixes above situation by updating the DXE core to skip the
> > NULL check against global gCpu in the IsMemoryTypeToGuard(), and adding
> > NULL check against gCpu in SetGuardPage() and UnsetGuardPage() to make
> > sure that they can be called but do nothing. This will allow HeapGuard to
> > record all guarded memory without setting the related Guard pages to not-
> > present.
> >
> > Once the CpuArchProtocol is installed, a protocol notify will be called
> > to complete the work of setting Guard pages to not-present.
> >
> > Please note that above changes will cause a #PF in GCD code during cleanup
> > of map entries, which is initiated by CpuDxe driver to update real mtrr
> > and paging attributes back to GCD. During that time, CpuDxe doesn't allow
> > GCD to update memory attributes and then any Guard page cannot be unset.
> > As a result, this will prevent Guarded memory from freeing during memory
> > map cleanup.
> >
> > The solution is to avoid allocating guarded memory as memory map entries
> > in GCD code. It's done by setting global mOnGuarding to TRUE before memory
> > allocation and setting it back to FALSE afterwards in GCD function
> > CoreAllocateGcdMapEntry().
> >
> > Cc: Star Zeng 
> > Cc: Eric Dong 
> > Cc: Jiewen Yao 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang 
> > ---
> >   MdeModulePkg/Core/Dxe/Gcd/Gcd.c   |  10 ++
> >   MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 148
> --
> >   MdeModulePkg/Core/Dxe/Mem/HeapGuard.h |   8 ++
> >   MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c |   5 +
> >   MdeModulePkg/Core/PiSmmCore/HeapGuard.c   |  16 +--
> >   5 files changed, 174 insertions(+), 13 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > index 8fbc3d282c..77f4adb4bc 100644
> > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > @@ -16,6 +16,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
> >
> >   #include "DxeMain.h"
> >   #include "Gcd.h"
> > +#include "Mem/HeapGuard.h"
> >
> >   #define MINIMUM_INITIAL_MEMORY_SIZE 0x1
> >
> > @@ -391,12 +392,21 @@ CoreAllocateGcdMapEntry (
> > IN OUT EFI_GCD_MAP_ENTRY  **BottomEntry
> > )
> >   {
> > +  //
> > +  // Set to mOnGuarding to TRUE before memory allocation. This will make
> sure
> > +  // that the entry memory is not "guarded" by HeapGuard. Otherwise it 
> > might
> > +  // cause problem when it's freed (if HeapGuard is enabled).
> > +  //
> > +  mOnGuarding = TRUE;
> > *TopEntry = AllocateZeroPool (sizeof (EFI_GCD_MAP_ENTRY));
> > +  mOnGuarding = FALSE;
> > if (*TopEntry == NULL) {
> >   return EFI_OUT_OF_RESOURCES;
> > }
> >
> > +  mOnGuarding = TRUE;
> > *BottomEntry = AllocateZeroPool (sizeof (EFI_GCD_MAP_ENTRY));
> > +  mOnGuarding = FALSE;
> > if (*BottomEntry == NULL) {
> >   CoreFreePool (*TopEntry);
> >   return EFI_OUT_OF_RESOURCES;
> > diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
> b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
> > index ac043b5d9b..fd6aeee8da 100644
> > --- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
> > +++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
> > @@ -70,7 +70,7 @@ SetBits (
> > StartBit  = (UINTN)GUARDED_HEAP_MAP_ENTRY_BIT_INDEX (Address);
> > EndBit= (StartBit + BitNumber - 1) % GUARDED_HEAP_MAP_ENTRY_BITS;
> >
> > -  if ((StartBit + BitNumber) > GUARDED_HEAP_MAP_ENTRY_BITS) {
> > +  if ((StartBit + BitNumber) >= GUARDED_HEAP_MAP_ENTRY_BITS) {
> >   Msbs= (GUARDED_HEAP_MAP_ENTRY_BITS - StartBit) %
> > GUARDED_HEAP_MAP_ENTRY_BITS;
> >   Lsbs= (EndBit + 1) % GUARDED_HEAP_MAP_ENTRY_BITS;
> > @@ -123,7 +123,7 @@ ClearBits (
> > StartBit  = (UINTN)GUARDED_HEAP_MAP_ENTRY_BIT_INDEX (Address);
> > EndBit= (StartBit + BitNumber - 1) % GUARDED_HEAP_MAP_ENTRY_BITS;
> >
> > -  if 

[edk2] [PATCH v3 0/3] fix empty window period of HeapGuard before CpuArchProtocol

[Jian J Wang]

> v3:
>Split the fixes into three patch files.

> v2:
>Fix a logic hole in bits operation on address on 64K boundary with
>just 64-bit length (SetBits(), ClearBits(), GetBits()).

This patch series fills up an empty window period of HeapGuard feature
before CpuArchProtocol installed, and fix an issue observed on 32-bit
real platform with this HeapGuard fix.

Jian J Wang (3):
  MdeModulePkg/Core: allow HeapGuard even before CpuArchProtocol
installed
  MdeModulePkg/PiSmmCore: fix bits operation error on a boundary
condition
  MdeModulePkg/Core: fix bits operation error on a boundary condition

 MdeModulePkg/Core/Dxe/Gcd/Gcd.c   |  10 ++
 MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 148 --
 MdeModulePkg/Core/Dxe/Mem/HeapGuard.h |   8 ++
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c |   5 +
 MdeModulePkg/Core/PiSmmCore/HeapGuard.c   |  16 +--
 5 files changed, 174 insertions(+), 13 deletions(-)

-- 
2.16.2.windows.1

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

[edk2] [PATCH v3 2/3] MdeModulePkg/PiSmmCore: fix bits operation error on a boundary condition

[Jian J Wang]

If given address is on 64K boundary and the requested bit number is 64,
all SetBits(), ClearBits() and GetBits() will encounter ASSERT problem
in trying to do a 64 bits of shift, which is not allowed by LShift() and
RShift(). This patch tries to fix this issue by turning bits operation
into whole integer operation in such situation.

Cc: Star Zeng 
Cc: Eric Dong 
Cc: Jiewen Yao 
Cc: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 MdeModulePkg/Core/PiSmmCore/HeapGuard.c | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c 
b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
index 923af93de2..f9657f9baa 100644
--- a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
+++ b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
@@ -73,7 +73,7 @@ SetBits (
   StartBit  = (UINTN)GUARDED_HEAP_MAP_ENTRY_BIT_INDEX (Address);
   EndBit= (StartBit + BitNumber - 1) % GUARDED_HEAP_MAP_ENTRY_BITS;
 
-  if ((StartBit + BitNumber) > GUARDED_HEAP_MAP_ENTRY_BITS) {
+  if ((StartBit + BitNumber) >= GUARDED_HEAP_MAP_ENTRY_BITS) {
 Msbs= (GUARDED_HEAP_MAP_ENTRY_BITS - StartBit) %
   GUARDED_HEAP_MAP_ENTRY_BITS;
 Lsbs= (EndBit + 1) % GUARDED_HEAP_MAP_ENTRY_BITS;
@@ -126,7 +126,7 @@ ClearBits (
   StartBit  = (UINTN)GUARDED_HEAP_MAP_ENTRY_BIT_INDEX (Address);
   EndBit= (StartBit + BitNumber - 1) % GUARDED_HEAP_MAP_ENTRY_BITS;
 
-  if ((StartBit + BitNumber) > GUARDED_HEAP_MAP_ENTRY_BITS) {
+  if ((StartBit + BitNumber) >= GUARDED_HEAP_MAP_ENTRY_BITS) {
 Msbs= (GUARDED_HEAP_MAP_ENTRY_BITS - StartBit) %
   GUARDED_HEAP_MAP_ENTRY_BITS;
 Lsbs= (EndBit + 1) % GUARDED_HEAP_MAP_ENTRY_BITS;
@@ -191,10 +191,14 @@ GetBits (
 Lsbs = 0;
   }
 
-  Result= RShiftU64 ((*BitMap), StartBit) & (LShiftU64 (1, Msbs) - 1);
-  if (Lsbs > 0) {
-BitMap  += 1;
-Result  |= LShiftU64 ((*BitMap) & (LShiftU64 (1, Lsbs) - 1), Msbs);
+  if (StartBit == 0 && BitNumber == GUARDED_HEAP_MAP_ENTRY_BITS) {
+Result = *BitMap;
+  } else {
+Result= RShiftU64((*BitMap), StartBit) & (LShiftU64(1, Msbs) - 1);
+if (Lsbs > 0) {
+  BitMap  += 1;
+  Result  |= LShiftU64 ((*BitMap) & (LShiftU64 (1, Lsbs) - 1), Msbs);
+}
   }
 
   return Result;
-- 
2.16.2.windows.1

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

[edk2] [PATCH v3 3/3] MdeModulePkg/Core: fix bits operation error on a boundary condition

[Jian J Wang]

If given address is on 64K boundary and the requested bit number is 64,
all SetBits(), ClearBits() and GetBits() will encounter ASSERT problem
in trying to do a 64 bits of shift, which is not allowed by LShift() and
RShift(). This patch tries to fix this issue by turning bits operation
into whole integer operation in such situation.

Cc: Star Zeng 
Cc: Eric Dong 
Cc: Jiewen Yao 
Cc: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c 
b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
index f6068c459c..fd6aeee8da 100644
--- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
+++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
@@ -70,7 +70,7 @@ SetBits (
   StartBit  = (UINTN)GUARDED_HEAP_MAP_ENTRY_BIT_INDEX (Address);
   EndBit= (StartBit + BitNumber - 1) % GUARDED_HEAP_MAP_ENTRY_BITS;
 
-  if ((StartBit + BitNumber) > GUARDED_HEAP_MAP_ENTRY_BITS) {
+  if ((StartBit + BitNumber) >= GUARDED_HEAP_MAP_ENTRY_BITS) {
 Msbs= (GUARDED_HEAP_MAP_ENTRY_BITS - StartBit) %
   GUARDED_HEAP_MAP_ENTRY_BITS;
 Lsbs= (EndBit + 1) % GUARDED_HEAP_MAP_ENTRY_BITS;
@@ -123,7 +123,7 @@ ClearBits (
   StartBit  = (UINTN)GUARDED_HEAP_MAP_ENTRY_BIT_INDEX (Address);
   EndBit= (StartBit + BitNumber - 1) % GUARDED_HEAP_MAP_ENTRY_BITS;
 
-  if ((StartBit + BitNumber) > GUARDED_HEAP_MAP_ENTRY_BITS) {
+  if ((StartBit + BitNumber) >= GUARDED_HEAP_MAP_ENTRY_BITS) {
 Msbs= (GUARDED_HEAP_MAP_ENTRY_BITS - StartBit) %
   GUARDED_HEAP_MAP_ENTRY_BITS;
 Lsbs= (EndBit + 1) % GUARDED_HEAP_MAP_ENTRY_BITS;
@@ -188,10 +188,14 @@ GetBits (
 Lsbs = 0;
   }
 
-  Result= RShiftU64 ((*BitMap), StartBit) & (LShiftU64 (1, Msbs) - 1);
-  if (Lsbs > 0) {
-BitMap  += 1;
-Result  |= LShiftU64 ((*BitMap) & (LShiftU64 (1, Lsbs) - 1), Msbs);
+  if (StartBit == 0 && BitNumber == GUARDED_HEAP_MAP_ENTRY_BITS) {
+Result = *BitMap;
+  } else {
+Result= RShiftU64((*BitMap), StartBit) & (LShiftU64(1, Msbs) - 1);
+if (Lsbs > 0) {
+  BitMap  += 1;
+  Result  |= LShiftU64 ((*BitMap) & (LShiftU64 (1, Lsbs) - 1), Msbs);
+}
   }
 
   return Result;
-- 
2.16.2.windows.1

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

Re: [edk2] [RFC] Remove X86 .asm and .S assembly files in EDK2

[Andrew Fish]

> On Mar 14, 2018, at 6:56 PM, Gao, Liming  wrote:
> 
> Mike:
>  Now, %pragma macho subsections_via_symbols can't be enabled. So, will nasm 
> generate the bigger image size than .S assembly on macho?

Yes. 

Thanks,

Andrew Fish

> If yes, I agree nasm is not same to .S. .S may be kept for a while. But, .asm 
> is not necessary. We can remove .asm first. 
> 
> Thanks
> Liming
>> -Original Message-
>> From: Kinney, Michael D
>> Sent: Wednesday, March 14, 2018 4:00 AM
>> To: Gao, Liming ; edk2-devel@lists.01.org; Kinney, 
>> Michael D 
>> Subject: RE: [RFC] Remove X86 .asm and .S assembly files in EDK2
>> 
>> Liming,
>> 
>> After further evaluation on use of NASM with macho
>> there are a couple issues that need to be resolved
>> before the .asm and .S files can be removed from
>> all components.
>> 
>> The details are at:
>> 
>> https://bugzilla.tianocore.org/show_bug.cgi?id=881
>> 
>> NASM issues to be resolved are:
>> 
>> https://bugzilla.nasm.us/show_bug.cgi?id=3392469
>> https://bugzilla.nasm.us/show_bug.cgi?id=3392470
>> 
>> The recommendation is to not switch to NASM only
>> for libraries until the dead code stripping feature
>> works as expected on macho.
>> 
>> We can consider moving to NASM only for NASM files
>> in a module if there are no NASM functions that would
>> be dead stripped.  The recommendation for these types
>> of NASM files is do not use the following pragma
>> until the NASM issues above are resolved.
>> 
>>  %pragma macho subsections_via_symbols
>> 
>> Best regards,
>> 
>> Mike
>> 
>>> -Original Message-
>>> From: edk2-devel [mailto:edk2-devel-
>>> boun...@lists.01.org] On Behalf Of Gao, Liming
>>> Sent: Wednesday, January 31, 2018 3:06 AM
>>> To: edk2-devel@lists.01.org
>>> Subject: [edk2] [RFC] Remove X86 .asm and .S assembly
>>> files in EDK2
>>> 
>>> Edk2 has used nasm assembly file for all tool chains.
>>> So, IA32 and X64 .asm and .S assembly files can be
>>> removed if their nasm files are ready. It can save the
>>> maintain effort and avoid the confuse.
>>> 
>>> 
>>> 
>>> If you have any comments on this change, please let me
>>> know.
>>> 
>>> Thanks
>>> Liming
>>> ___
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[edk2] [PATCH v6 4/6] MdeModulePkg/PciHostBridgeDxe: Add support for address translation

[Heyi Guo]

PCI address translation is necessary for some non-x86 platforms. On
such platforms, address value (denoted as "device address" or "address
in PCI view") set to PCI BAR registers in configuration space might be
different from the address which is used by CPU to access the
registers in memory BAR or IO BAR spaces (denoted as "host address" or
"address in CPU view"). The difference between the two addresses is
called "Address Translation Offset" or simply "translation", and can
be represented by "Address Translation Offset" in ACPI QWORD Address
Space Descriptor (Offset 0x1E). However UEFI and ACPI differs on the
definitions of QWORD Address Space Descriptor, and we will follow UEFI
definition on UEFI protocols, such as PCI root bridge IO protocol and
PCI IO protocol. In UEFI 2.7, "Address Translation Offset" is "Offset
to apply to the Starting address to convert it to a PCI address". This
means:

1. Translation = device address - host address.

2. PciRootBridgeIo->Configuration should return CPU view address, as
well as PciIo->GetBarAttributes.

Summary of addresses used in protocol interfaces and internal
implementations:

1. *Only* the following protocol interfaces assume Address is Device
   Address:
(1). PciHostBridgeResourceAllocation.GetProposedResources()
 Otherwise PCI bus driver cannot set correct address into PCI
 BARs.
(2). PciRootBridgeIo.Mem.Read() and PciRootBridgeIo.Mem.Write()
(3). PciRootBridgeIo.CopyMem()
UEFI and PI spec have clear statements for all other protocol
interfaces about the address type.

2. Library interfaces and internal implementation:
(1). Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address.
 It is easy to check whether the address is below 4G or above 4G.
(2). Addresses in PCI_ROOT_BRIDGE_INSTANCE.ResAllocNode are host
 address, for they are allocated from GCD.
(3). Address passed to PciHostBridgeResourceConflict is host address,
 for it comes from PCI_ROOT_BRIDGE_INSTANCE.ResAllocNode.

RESTRICTION: to simplify the situation, we require the alignment of
Translation must be larger than any BAR alignment in the same root
bridge, so that resource allocation alignment can be applied to both
device address and host address.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo 
Signed-off-by: Yi Li 
Cc: Ruiyu Ni 
Cc: Ard Biesheuvel 
Cc: Star Zeng 
Cc: Eric Dong 
Cc: Laszlo Ersek 
Cc: Michael D Kinney 
---

Notes:
v6:
- Change ASSERT(FALSE) to ASSERT ((Translation & Alignment) == 0) and
  move ASSERT before if-check [Ray]
- Print translation of bus and then assert [Ray]
- Add function header for RootBridgeIoGetMemTranslationByAddress()
  [Ray]

v5:
- Add check for the alignment of Translation against the BAR alignment
  [Ray]
- Keep coding style of comments consistent with the context [Ray]
- Comment change for Base in PCI_RES_NODE [Ray]
- Add macros of TO_HOST_ADDRESS and TO_DEVICE_ADDRESS for address type
  conversion (After that we can also simply the comments about the
  calculation [Ray]
- Add check for bus translation offset in CreateRootBridge(), making
  sure it is zero, and unify code logic for all types of resource
  after that [Ray]
- Use GetTranslationByResourceType() to simplify the code in
  RootBridgeIoConfiguration() (also fix a bug in previous patch
  version of missing a break after case TypePMem64) [Ray]
- Commit message refinement [Ray]

v4:
- Add ASSERT (FALSE) to default branch in GetTranslationByResourceType
  [Laszlo]
- Fix bug when passing BaseAddress to gDS->AllocateIoSpace and
  gDS->AllocateMemorySpace [Laszlo]
- Add comment for applying alignment to both device address and host
  address, and add NOTE for the alignment requirement of Translation,
  as well as in commit message [Laszlo][Ray]
- Improve indention for the code in CreateRootBridge [Laszlo]
- Improve comment for Translation in PCI_ROOT_BRIDGE_APERTURE
  definition [Laszlo]
- Ignore translation of bus in CreateRootBridge

v4:
- Add ASSERT (FALSE) to default branch in GetTranslationByResourceType
  [Laszlo]
- Fix bug when passing BaseAddress to gDS->AllocateIoSpace and
  gDS->AllocateMemorySpace [Laszlo]
- Add comment for applying alignment to both device address and host
  address, and add NOTE for the alignment requirement of Translation,
  as well as in commit message [Laszlo][Ray]
- Improve indention for the code in CreateRootBridge [Laszlo]
- Improve comment for Translation in PCI_ROOT_BRIDGE_APERTURE
  definition [Laszlo]
- Ignore translation of bus in CreateRootBridge

 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h   |  21 +++
 

[edk2] [PATCH v6 5/6] MdeModulePkg/PciBus: convert host address to device address

[Heyi Guo]

According to UEFI spec 2.7, PciRootBridgeIo->Configuration() should
return host address (CPU view ddress) rather than device address
(PCI view address), so in function GetMmioAddressTranslationOffset we
need to convert the range to device address before comparing.

And device address = host address + translation offset.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo 
Signed-off-by: Yi Li 
Cc: Ruiyu Ni 
Cc: Ard Biesheuvel 
Cc: Star Zeng 
Cc: Eric Dong 
Cc: Laszlo Ersek 
Cc: Michael D Kinney 
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
index 190f4b0dc7ed..fef3eceb7f62 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
@@ -1812,10 +1812,14 @@ GetMmioAddressTranslationOffset (
 return (UINT64) -1;
   }
 
+  // According to UEFI 2.7, EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL::Configuration()
+  // returns host address instead of device address, while 
AddrTranslationOffset
+  // is not zero, and device address = host address + AddrTranslationOffset, so
+  // we convert host address to device address for range compare.
   while (Configuration->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) {
 if ((Configuration->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) &&
-(Configuration->AddrRangeMin <= AddrRangeMin) &&
-(Configuration->AddrRangeMin + Configuration->AddrLen >= AddrRangeMin 
+ AddrLen)
+(Configuration->AddrRangeMin + Configuration->AddrTranslationOffset <= 
AddrRangeMin) &&
+(Configuration->AddrRangeMin + Configuration->AddrLen + 
Configuration->AddrTranslationOffset >= AddrRangeMin + AddrLen)
 ) {
   return Configuration->AddrTranslationOffset;
 }
-- 
2.7.4

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

[edk2] [PATCH v6 2/6] OvmfPkg/PciHostBridgeLib: clear PCI aperture vars for (re)init

[Heyi Guo]

Use ZeroMem() to initialize (or re-initialize) all fields in temporary
PCI_ROOT_BRIDGE_APERTURE variables to zero. This is not mandatory but
is helpful for future extension: when we add new fields to
PCI_ROOT_BRIDGE_APERTURE and the default value of these fields can
safely be zero, this code will not suffer from an additional
change.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo 
Signed-off-by: Yi Li 
Cc: Jordan Justen 
Cc: Anthony Perard 
Cc: Julien Grall 
Cc: Ruiyu Ni 
Cc: Laszlo Ersek 
Cc: Ard Biesheuvel 
---

Notes:
v6:
- Move ZeroMem() into the loop [Laszlo]
- Move ZeroMem() after checking PcdPciDisableBusEnumeration [Laszlo]
- Commit title change (However 74 characters will cause PatchCheck
  warning, so I tried to reduce the length to eliminate the warning) 
[laszlo]
- Minor changes in commit message

 OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 4 
 OvmfPkg/Library/PciHostBridgeLib/XenSupport.c   | 7 ++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c 
b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
index ff837035caff..65d0ef9252c5 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
+++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
@@ -221,6 +221,10 @@ PciHostBridgeGetRootBridges (
 return ScanForRootBridges (Count);
   }
 
+  ZeroMem (, sizeof (Io));
+  ZeroMem (, sizeof (Mem));
+  ZeroMem (, sizeof (MemAbove4G));
+
   Attributes = EFI_PCI_ATTRIBUTE_IDE_PRIMARY_IO |
 EFI_PCI_ATTRIBUTE_IDE_SECONDARY_IO |
 EFI_PCI_ATTRIBUTE_ISA_IO_16 |
diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c 
b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
index 31c63ae19e0a..920417950ad9 100644
--- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
+++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
@@ -202,8 +202,13 @@ ScanForRootBridges (
   for (PrimaryBus = 0; PrimaryBus <= PCI_MAX_BUS; PrimaryBus = SubBus + 1) {
 SubBus = PrimaryBus;
 Attributes = 0;
+
+ZeroMem (, sizeof (Io));
+ZeroMem (, sizeof (Mem));
+ZeroMem (, sizeof (MemAbove4G));
+ZeroMem (, sizeof (PMem));
+ZeroMem (, sizeof (PMemAbove4G));
 Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = 
MAX_UINT64;
-Io.Limit = Mem.Limit = MemAbove4G.Limit = PMem.Limit = PMemAbove4G.Limit = 
0;
 //
 // Scan all the PCI devices on the primary bus of the PCI root bridge
 //
-- 
2.7.4

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

[edk2] [PATCH v6 3/6] MdeModulePkg/PciHostBridgeLib.h: add address Translation

[Heyi Guo]

Add Translation field to PCI_ROOT_BRIDGE_APERTURE. Translation is used
to represent the difference between device address and host address,
if they are not the same on some platforms.

In UEFI 2.7, "Address Translation Offset" is "Offset to apply to the
Starting address to convert it to a PCI address".  This means:

  Translation = device address - host address

So we also use the above calculation for this Translation field to
keep consistent.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo 
Signed-off-by: Yi Li 
Cc: Ruiyu Ni 
Cc: Ard Biesheuvel 
Cc: Star Zeng 
Cc: Eric Dong 
Cc: Laszlo Ersek 
Cc: Michael D Kinney 
---
 MdeModulePkg/Include/Library/PciHostBridgeLib.h | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/MdeModulePkg/Include/Library/PciHostBridgeLib.h 
b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
index d42e9ecdd763..18963a0d3821 100644
--- a/MdeModulePkg/Include/Library/PciHostBridgeLib.h
+++ b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
@@ -20,8 +20,27 @@
 // (Base > Limit) indicates an aperture is not available.
 //
 typedef struct {
+  //
+  // Base and Limit are the device address instead of host address when
+  // Translation is not zero
+  //
   UINT64 Base;
   UINT64 Limit;
+  //
+  // According to UEFI 2.7, Device Address = Host Address + Translation,
+  // so Translation = Device Address - Host Address.
+  // On platforms where Translation is not zero, the subtraction is probably to
+  // be performed with UINT64 wrap-around semantics, for we may translate an
+  // above-4G host address into a below-4G device address for legacy PCIe 
device
+  // compatibility.
+  //
+  // NOTE: The alignment of Translation is required to be larger than any BAR
+  // alignment in the same root bridge, so that the same alignment can be
+  // applied to both device address and host address, which simplifies the
+  // situation and makes the current resource allocation code in generic PCI
+  // host bridge driver still work.
+  //
+  UINT64 Translation;
 } PCI_ROOT_BRIDGE_APERTURE;
 
 typedef struct {
-- 
2.7.4

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

[edk2] [PATCH v6 1/6] CorebootPayloadPkg/PciHostBridgeLib: clear aperture vars for (re)init

[Heyi Guo]

Use ZeroMem() to initialize (or re-initialize) all fields in temporary
PCI_ROOT_BRIDGE_APERTURE variables to zero. This is not mandatory but
helpful for future extension: when we add new fields to
PCI_ROOT_BRIDGE_APERTURE and the default value of these fields can
safely be zero, this code will not suffer from an additional change.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo 
Signed-off-by: Yi Li 
Cc: Maurice Ma 
Cc: Prince Agyeman 
Cc: Benjamin You 
Cc: Ruiyu Ni 
Cc: Laszlo Ersek 
Cc: Ard Biesheuvel 
---

Notes:
v6:
- Move ZeroMem() into the loop just as Laszlo commented on OvmfPkg
  [Laszlo]
- Minor changes in commit message

 CorebootPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/CorebootPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c 
b/CorebootPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c
index 6d94ff72c956..18dcbafdf0c6 100644
--- a/CorebootPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c
+++ b/CorebootPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c
@@ -328,8 +328,13 @@ ScanForRootBridges (
   for (PrimaryBus = 0; PrimaryBus <= PCI_MAX_BUS; PrimaryBus = SubBus + 1) {
 SubBus = PrimaryBus;
 Attributes = 0;
+
+ZeroMem (, sizeof (Io));
+ZeroMem (, sizeof (Mem));
+ZeroMem (, sizeof (MemAbove4G));
+ZeroMem (, sizeof (PMem));
+ZeroMem (, sizeof (PMemAbove4G));
 Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = 
MAX_UINT64;
-Io.Limit = Mem.Limit = MemAbove4G.Limit = PMem.Limit = PMemAbove4G.Limit = 
0;
 //
 // Scan all the PCI devices on the primary bus of the PCI root bridge
 //
-- 
2.7.4

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

[edk2] [PATCH v6 6/6] MdeModulePkg/PciBus: return CPU address for GetBarAttributes

[Heyi Guo]

According to UEFI spec 2.7, PciIo->GetBarAttributes should return host
address (CPU view ddress) rather than device address (PCI view
address), and
device address = host address + address translation offset,
so we subtract translation from device address before returning.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo 
Signed-off-by: Yi Li 
Cc: Ruiyu Ni 
Cc: Ard Biesheuvel 
Cc: Star Zeng 
Cc: Eric Dong 
Cc: Laszlo Ersek 
Cc: Michael D Kinney 
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
index fef3eceb7f62..62179eb44bbd 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
@@ -1972,6 +1972,10 @@ PciIoGetBarAttributes (
 return EFI_UNSUPPORTED;
   }
 }
+
+// According to UEFI spec 2.7, we need return host address for
+// PciIo->GetBarAttributes, and host address = device address - 
translation.
+Descriptor->AddrRangeMin -= Descriptor->AddrTranslationOffset;
   }
 
   return EFI_SUCCESS;
-- 
2.7.4

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

[edk2] [PATCH v6 0/6] Add translation support to generic PciHostBridge

[Heyi Guo]

v6:
- Patch 1, 2: implement 3 comments from Laszlo.
- Patch 4: implement 3 comments from Ray.

Patch v5 inherits the code from RFC v4; we don't restart the version number for
RFC to PATCH change.

v5:
- Patch 4/6: Modify the code according to the comments from Ray.
- Patch 1/6 and 2/6 are totally new. They add initialization for all fields of
  PCI_ROOT_BRIDGE_APERTURE temporary variables in PciHostBridgeLib instances, so
  that they will not suffer from extension of PCI_ROOT_BRIDGE_APERTURE
  structure.
- Generate a separate patch (3/6) for PciHostBridgeLib.h change. Though it is a
  prerequisite for patch 4/6, it does not change the code in PciHostBridge
  driver and won't cause any build failure or functional issue.


v4:
- Modify the code according to the comments from Ray, Laszlo and Ard (Please see
  the notes of Patch 1/3)
- Ignore translation of bus in CreateRootBridge.


v3:
- Keep definition of Translation consistent in EDKII code: Translation = device
  address - host address.
- Patch 2/2 is split into 2 patches (2/3 and 3/3).
- Refine comments and commit messages to make the code easier to understand.


v2:
Changs are made according to the discussion on the mailing list, including:

- PciRootBridgeIo->Configuration should return CPU view address, as well as
  PciIo->GetBarAttributes, and Translation Offset should be equal to PCI view
  address - CPU view address.
- Add translation offset to PCI_ROOT_BRIDGE_APERTURE structure definition.
- PciHostBridge driver internally used Base Address is still based on PCI view
  address, and translation offset = CPU view - PCI view, which follows the
  definition in ACPI, and not the same as that in UEFI spec.

Cc: Ruiyu Ni 
Cc: Ard Biesheuvel 
Cc: Star Zeng 
Cc: Eric Dong 
Cc: Laszlo Ersek 
Cc: Michael D Kinney 
Cc: Maurice Ma 
Cc: Prince Agyeman 
Cc: Benjamin You 
Cc: Jordan Justen 
Cc: Anthony Perard 
Cc: Julien Grall 


Heyi Guo (6):
  CorebootPayloadPkg/PciHostBridgeLib: clear aperture vars for (re)init
  OvmfPkg/PciHostBridgeLib: clear PCI aperture vars for (re)init
  MdeModulePkg/PciHostBridgeLib.h: add address Translation
  MdeModulePkg/PciHostBridgeDxe: Add support for address translation
  MdeModulePkg/PciBus: convert host address to device address
  MdeModulePkg/PciBus: return CPU address for GetBarAttributes

 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h  |  21 +++
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h|   3 +
 MdeModulePkg/Include/Library/PciHostBridgeLib.h|  19 +++
 CorebootPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c |   7 +-
 MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c |  12 +-
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c  | 129 
---
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c| 135 
++--
 OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c|   4 +
 OvmfPkg/Library/PciHostBridgeLib/XenSupport.c  |   7 +-
 9 files changed, 306 insertions(+), 31 deletions(-)

-- 
2.7.4

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

Re: [edk2] [PATCH] MdeModulePkg/Core: allow HeapGuard even before CpuArchProtocol installed

[Ni, Ruiyu]

On 3/15/2018 10:27 AM, Jian J Wang wrote:

v2 changes:
Fix a logic hole in bits operation on address on 64K boundary with
just 64-bit length (SetBits(), ClearBits(), GetBits()).


Due to the fact that HeapGuard needs CpuArchProtocol to update page
attributes, the feature is normally enabled after CpuArchProtocol is
installed. Since there're some drivers are loaded before CpuArchProtocl,
they cannot make use HeapGuard feature to detect potential issues.

This patch fixes above situation by updating the DXE core to skip the
NULL check against global gCpu in the IsMemoryTypeToGuard(), and adding
NULL check against gCpu in SetGuardPage() and UnsetGuardPage() to make
sure that they can be called but do nothing. This will allow HeapGuard to
record all guarded memory without setting the related Guard pages to not-
present.

Once the CpuArchProtocol is installed, a protocol notify will be called
to complete the work of setting Guard pages to not-present.

Please note that above changes will cause a #PF in GCD code during cleanup
of map entries, which is initiated by CpuDxe driver to update real mtrr
and paging attributes back to GCD. During that time, CpuDxe doesn't allow
GCD to update memory attributes and then any Guard page cannot be unset.
As a result, this will prevent Guarded memory from freeing during memory
map cleanup.

The solution is to avoid allocating guarded memory as memory map entries
in GCD code. It's done by setting global mOnGuarding to TRUE before memory
allocation and setting it back to FALSE afterwards in GCD function
CoreAllocateGcdMapEntry().

Cc: Star Zeng 
Cc: Eric Dong 
Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
  MdeModulePkg/Core/Dxe/Gcd/Gcd.c   |  10 ++
  MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 148 --
  MdeModulePkg/Core/Dxe/Mem/HeapGuard.h |   8 ++
  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c |   5 +
  MdeModulePkg/Core/PiSmmCore/HeapGuard.c   |  16 +--
  5 files changed, 174 insertions(+), 13 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index 8fbc3d282c..77f4adb4bc 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -16,6 +16,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
  
  #include "DxeMain.h"

  #include "Gcd.h"
+#include "Mem/HeapGuard.h"
  
  #define MINIMUM_INITIAL_MEMORY_SIZE 0x1
  
@@ -391,12 +392,21 @@ CoreAllocateGcdMapEntry (

IN OUT EFI_GCD_MAP_ENTRY  **BottomEntry
)
  {
+  //
+  // Set to mOnGuarding to TRUE before memory allocation. This will make sure
+  // that the entry memory is not "guarded" by HeapGuard. Otherwise it might
+  // cause problem when it's freed (if HeapGuard is enabled).
+  //
+  mOnGuarding = TRUE;
*TopEntry = AllocateZeroPool (sizeof (EFI_GCD_MAP_ENTRY));
+  mOnGuarding = FALSE;
if (*TopEntry == NULL) {
  return EFI_OUT_OF_RESOURCES;
}
  
+  mOnGuarding = TRUE;

*BottomEntry = AllocateZeroPool (sizeof (EFI_GCD_MAP_ENTRY));
+  mOnGuarding = FALSE;
if (*BottomEntry == NULL) {
  CoreFreePool (*TopEntry);
  return EFI_OUT_OF_RESOURCES;
diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c 
b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
index ac043b5d9b..fd6aeee8da 100644
--- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
+++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
@@ -70,7 +70,7 @@ SetBits (
StartBit  = (UINTN)GUARDED_HEAP_MAP_ENTRY_BIT_INDEX (Address);
EndBit= (StartBit + BitNumber - 1) % GUARDED_HEAP_MAP_ENTRY_BITS;
  
-  if ((StartBit + BitNumber) > GUARDED_HEAP_MAP_ENTRY_BITS) {

+  if ((StartBit + BitNumber) >= GUARDED_HEAP_MAP_ENTRY_BITS) {
  Msbs= (GUARDED_HEAP_MAP_ENTRY_BITS - StartBit) %
GUARDED_HEAP_MAP_ENTRY_BITS;
  Lsbs= (EndBit + 1) % GUARDED_HEAP_MAP_ENTRY_BITS;
@@ -123,7 +123,7 @@ ClearBits (
StartBit  = (UINTN)GUARDED_HEAP_MAP_ENTRY_BIT_INDEX (Address);
EndBit= (StartBit + BitNumber - 1) % GUARDED_HEAP_MAP_ENTRY_BITS;
  
-  if ((StartBit + BitNumber) > GUARDED_HEAP_MAP_ENTRY_BITS) {

+  if ((StartBit + BitNumber) >= GUARDED_HEAP_MAP_ENTRY_BITS) {
  Msbs= (GUARDED_HEAP_MAP_ENTRY_BITS - StartBit) %
GUARDED_HEAP_MAP_ENTRY_BITS;
  Lsbs= (EndBit + 1) % GUARDED_HEAP_MAP_ENTRY_BITS;
@@ -188,10 +188,14 @@ GetBits (
  Lsbs = 0;
}
  
-  Result= RShiftU64 ((*BitMap), StartBit) & (LShiftU64 (1, Msbs) - 1);

-  if (Lsbs > 0) {
-BitMap  += 1;
-Result  |= LShiftU64 ((*BitMap) & (LShiftU64 (1, Lsbs) - 1), Msbs);
+  if (StartBit == 0 && BitNumber == GUARDED_HEAP_MAP_ENTRY_BITS) {
+Result = *BitMap;
+  } else {
+Result= RShiftU64((*BitMap), StartBit) & (LShiftU64(1, Msbs) - 1);
+if (Lsbs > 0) {
+  BitMap  += 1;
+  Result  |= LShiftU64 ((*BitMap) & (LShiftU64 (1, 

Re: [edk2] [PATCH v5 4/6] MdeModulePkg/PciHostBridgeDxe: Add support for address translation

[Ni, Ruiyu]

On 3/15/2018 10:57 AM, Guo Heyi wrote:

Hi Ray,

Sorry I never tried Ecc tool before, for there is few documents about it. When I
tried running python BaseTools/Source/Python/Ecc/Ecc.py -t -s ,
it showed me with error message:

ImportError: No module named Common.LongFilePathOs

Then I found there was an executable named "Ecc" in
BaseTools/BinWrappers/PosixLike/, and change the place of "-t" and "-s" after it
prompted:

  : error 1003: Invalid value of option
 Target [-s] does NOT exist

However, it still failed with error:

RuntimeError: ANTLR version mismatch: The recognizer has been generated with API
V0, but this runtime does not support this.

I could not find the reason for this error, maybe it was caused by incompatible
version of python and antlr3 (mine is Ubuntu 16.04.3 LTS, python 2.7.12,
python-antlr3 3.5.2-1).

Finally I downloaded prebuilt Win32 BaseTools and ran Ecc.exe on Windows. It
success and produced a "Report.csv" with only a titel line. I guess this means
there is no error in the coding style, isn't it?

Please let me know if there is anything wrong with what I did.

Sorry I provided the wrong command line. It should be
"...Ecc.py -t  -s".

If you haven't add the function header comments as I pointed out.
You should get at least one ECC error.
Otherwise, the code change should be fine.

Thanks for the try.



Thanks and regards,

Heyi


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

Re: [edk2] [PATCH v5 4/6] MdeModulePkg/PciHostBridgeDxe: Add support for address translation

[Guo Heyi]

Hi Ray,

Sorry I never tried Ecc tool before, for there is few documents about it. When I
tried running python BaseTools/Source/Python/Ecc/Ecc.py -t -s ,
it showed me with error message:

ImportError: No module named Common.LongFilePathOs

Then I found there was an executable named "Ecc" in
BaseTools/BinWrappers/PosixLike/, and change the place of "-t" and "-s" after it
prompted:

 : error 1003: Invalid value of option
Target [-s] does NOT exist

However, it still failed with error:

RuntimeError: ANTLR version mismatch: The recognizer has been generated with API
V0, but this runtime does not support this.

I could not find the reason for this error, maybe it was caused by incompatible
version of python and antlr3 (mine is Ubuntu 16.04.3 LTS, python 2.7.12,
python-antlr3 3.5.2-1).

Finally I downloaded prebuilt Win32 BaseTools and ran Ecc.exe on Windows. It
success and produced a "Report.csv" with only a titel line. I guess this means
there is no error in the coding style, isn't it?

Please let me know if there is anything wrong with what I did.

Thanks and regards,

Heyi

On Wed, Mar 14, 2018 at 03:57:14PM +0800, Ni, Ruiyu wrote:
> On 3/7/2018 2:01 PM, Guo Heyi wrote:
> >Thanks. Please let me know if any further changes are needed.
> >
> >Regards,
> >
> >Heyi
> >
> >On Wed, Mar 07, 2018 at 12:30:59PM +0800, Ni, Ruiyu wrote:
> >>On 3/6/2018 10:44 AM, Guo Heyi wrote:
> >>>Hi Ray,
> >>>
> >>>Any comments for v5?
> >>
> >>Heyi,
> >>Some backward compatibility concerns were received from internal production
> >>teams. Current change will cause silent failure on old platforms because
> >>TranslationOffset might be random if uninitialized.
> >>I will solve the concern and then send out updates to you, hopefully by end
> >>of next week.
> >>
> >>>
> >>>Regards,
> >>>
> >>>Heyi
> >>>
> >>>On Thu, Mar 01, 2018 at 02:57:22PM +0800, Heyi Guo wrote:
> PCI address translation is necessary for some non-x86 platforms. On
> such platforms, address value (denoted as "device address" or "address
> in PCI view") set to PCI BAR registers in configuration space might be
> different from the address which is used by CPU to access the
> registers in memory BAR or IO BAR spaces (denoted as "host address" or
> "address in CPU view"). The difference between the two addresses is
> called "Address Translation Offset" or simply "translation", and can
> be represented by "Address Translation Offset" in ACPI QWORD Address
> Space Descriptor (Offset 0x1E). However UEFI and ACPI differs on the
> definitions of QWORD Address Space Descriptor, and we will follow UEFI
> definition on UEFI protocols, such as PCI root bridge IO protocol and
> PCI IO protocol. In UEFI 2.7, "Address Translation Offset" is "Offset
> to apply to the Starting address to convert it to a PCI address". This
> means:
> 
> 1. Translation = device address - host address.
> 
> 2. PciRootBridgeIo->Configuration should return CPU view address, as
> well as PciIo->GetBarAttributes.
> 
> Summary of addresses used in protocol interfaces and internal
> implementations:
> 
> 1. *Only* the following protocol interfaces assume Address is Device
> Address:
> (1). PciHostBridgeResourceAllocation.GetProposedResources()
>   Otherwise PCI bus driver cannot set correct address into PCI
>   BARs.
> (2). PciRootBridgeIo.Mem.Read() and PciRootBridgeIo.Mem.Write()
> (3). PciRootBridgeIo.CopyMem()
> UEFI and PI spec have clear statements for all other protocol
> interfaces about the address type.
> 
> 2. Library interfaces and internal implementation:
> (1). Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address.
>   It is easy to check whether the address is below 4G or above 4G.
> (2). Addresses in PCI_ROOT_BRIDGE_INSTANCE.ResAllocNode are host
>   address, for they are allocated from GCD.
> (3). Address passed to PciHostBridgeResourceConflict is host address,
>   for it comes from PCI_ROOT_BRIDGE_INSTANCE.ResAllocNode.
> 
> RESTRICTION: to simplify the situation, we require the alignment of
> Translation must be larger than any BAR alignment in the same root
> bridge, so that resource allocation alignment can be applied to both
> device address and host address.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Heyi Guo 
> Cc: Ruiyu Ni 
> Cc: Ard Biesheuvel 
> Cc: Star Zeng 
> Cc: Eric Dong 
> Cc: Laszlo Ersek 
> Cc: Michael D Kinney 
> ---
> 
> Notes:
>  v5:
>  - Add check for the alignment of Translation against the BAR 
>  alignment
>    [Ray]
>  - Keep coding style of comments consistent with the 

Re: [edk2] To save size, use NULL library for DebugLib and ReportStatusCodeLib

[Gao, Liming]

Here is NULL DebugLib and ReportStatusCodeLib instance. They are dummy 
implementation. They will not take the image size, and remove the debug message 
and ASSERT code from the image. So, they can help to save the image size.

MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull.inf

Thanks
Liming
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of david 
> moheban
> Sent: Thursday, March 15, 2018 7:15 AM 
> MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull.inf
> To: edk2-devel@lists.01.org
> Subject: [edk2] To save size, use NULL library for DebugLib and 
> ReportStatusCodeLib
> 
> What does that mean??
> 
> Sorry for asking beginner questions but was just wondering. Trying to
> shrink the size of the Duet image.
> 
> Thank you
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH] MdeModulePkg/Core: allow HeapGuard even before CpuArchProtocol installed

[Jian J Wang]

> v2 changes:
>Fix a logic hole in bits operation on address on 64K boundary with
>just 64-bit length (SetBits(), ClearBits(), GetBits()).

Due to the fact that HeapGuard needs CpuArchProtocol to update page
attributes, the feature is normally enabled after CpuArchProtocol is
installed. Since there're some drivers are loaded before CpuArchProtocl,
they cannot make use HeapGuard feature to detect potential issues.

This patch fixes above situation by updating the DXE core to skip the
NULL check against global gCpu in the IsMemoryTypeToGuard(), and adding
NULL check against gCpu in SetGuardPage() and UnsetGuardPage() to make
sure that they can be called but do nothing. This will allow HeapGuard to
record all guarded memory without setting the related Guard pages to not-
present.

Once the CpuArchProtocol is installed, a protocol notify will be called
to complete the work of setting Guard pages to not-present.

Please note that above changes will cause a #PF in GCD code during cleanup
of map entries, which is initiated by CpuDxe driver to update real mtrr
and paging attributes back to GCD. During that time, CpuDxe doesn't allow
GCD to update memory attributes and then any Guard page cannot be unset.
As a result, this will prevent Guarded memory from freeing during memory
map cleanup.

The solution is to avoid allocating guarded memory as memory map entries
in GCD code. It's done by setting global mOnGuarding to TRUE before memory
allocation and setting it back to FALSE afterwards in GCD function
CoreAllocateGcdMapEntry().

Cc: Star Zeng 
Cc: Eric Dong 
Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c   |  10 ++
 MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 148 --
 MdeModulePkg/Core/Dxe/Mem/HeapGuard.h |   8 ++
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c |   5 +
 MdeModulePkg/Core/PiSmmCore/HeapGuard.c   |  16 +--
 5 files changed, 174 insertions(+), 13 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index 8fbc3d282c..77f4adb4bc 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -16,6 +16,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 
 #include "DxeMain.h"
 #include "Gcd.h"
+#include "Mem/HeapGuard.h"
 
 #define MINIMUM_INITIAL_MEMORY_SIZE 0x1
 
@@ -391,12 +392,21 @@ CoreAllocateGcdMapEntry (
   IN OUT EFI_GCD_MAP_ENTRY  **BottomEntry
   )
 {
+  //
+  // Set to mOnGuarding to TRUE before memory allocation. This will make sure
+  // that the entry memory is not "guarded" by HeapGuard. Otherwise it might
+  // cause problem when it's freed (if HeapGuard is enabled).
+  //
+  mOnGuarding = TRUE;
   *TopEntry = AllocateZeroPool (sizeof (EFI_GCD_MAP_ENTRY));
+  mOnGuarding = FALSE;
   if (*TopEntry == NULL) {
 return EFI_OUT_OF_RESOURCES;
   }
 
+  mOnGuarding = TRUE;
   *BottomEntry = AllocateZeroPool (sizeof (EFI_GCD_MAP_ENTRY));
+  mOnGuarding = FALSE;
   if (*BottomEntry == NULL) {
 CoreFreePool (*TopEntry);
 return EFI_OUT_OF_RESOURCES;
diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c 
b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
index ac043b5d9b..fd6aeee8da 100644
--- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
+++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
@@ -70,7 +70,7 @@ SetBits (
   StartBit  = (UINTN)GUARDED_HEAP_MAP_ENTRY_BIT_INDEX (Address);
   EndBit= (StartBit + BitNumber - 1) % GUARDED_HEAP_MAP_ENTRY_BITS;
 
-  if ((StartBit + BitNumber) > GUARDED_HEAP_MAP_ENTRY_BITS) {
+  if ((StartBit + BitNumber) >= GUARDED_HEAP_MAP_ENTRY_BITS) {
 Msbs= (GUARDED_HEAP_MAP_ENTRY_BITS - StartBit) %
   GUARDED_HEAP_MAP_ENTRY_BITS;
 Lsbs= (EndBit + 1) % GUARDED_HEAP_MAP_ENTRY_BITS;
@@ -123,7 +123,7 @@ ClearBits (
   StartBit  = (UINTN)GUARDED_HEAP_MAP_ENTRY_BIT_INDEX (Address);
   EndBit= (StartBit + BitNumber - 1) % GUARDED_HEAP_MAP_ENTRY_BITS;
 
-  if ((StartBit + BitNumber) > GUARDED_HEAP_MAP_ENTRY_BITS) {
+  if ((StartBit + BitNumber) >= GUARDED_HEAP_MAP_ENTRY_BITS) {
 Msbs= (GUARDED_HEAP_MAP_ENTRY_BITS - StartBit) %
   GUARDED_HEAP_MAP_ENTRY_BITS;
 Lsbs= (EndBit + 1) % GUARDED_HEAP_MAP_ENTRY_BITS;
@@ -188,10 +188,14 @@ GetBits (
 Lsbs = 0;
   }
 
-  Result= RShiftU64 ((*BitMap), StartBit) & (LShiftU64 (1, Msbs) - 1);
-  if (Lsbs > 0) {
-BitMap  += 1;
-Result  |= LShiftU64 ((*BitMap) & (LShiftU64 (1, Lsbs) - 1), Msbs);
+  if (StartBit == 0 && BitNumber == GUARDED_HEAP_MAP_ENTRY_BITS) {
+Result = *BitMap;
+  } else {
+Result= RShiftU64((*BitMap), StartBit) & (LShiftU64(1, Msbs) - 1);
+if (Lsbs > 0) {
+  BitMap  += 1;
+  Result  |= LShiftU64 ((*BitMap) & (LShiftU64 (1, Lsbs) - 1), Msbs);
+}
   }
 
   return Result;
@@ -576,6 +580,10 @@ SetGuardPage (
   

Re: [edk2] [Patch 4/5] Vlv2TbltDevicePkg/PlatformBootManagerLib: Check PcdPkcs7CertBufferXdr

[Wei, David]

Reviewed-by: david wei  

Thanks,
David  Wei

Intel SSG/STO/UEFI BIOS 


-Original Message-
From: Kinney, Michael D 
Sent: Tuesday, March 13, 2018 3:30 AM
To: edk2-devel@lists.01.org
Cc: Kinney, Michael D ; Sean Brogan 
; Wei, David ; Guo, Mang 

Subject: [Patch 4/5] Vlv2TbltDevicePkg/PlatformBootManagerLib: Check 
PcdPkcs7CertBufferXdr

From: Michael D Kinney 

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

Evaluate both PcdPkcs7CertBuffer and PcdPkcs7CertBufferXdr for the use
of the test key.  If the test key is found in either PCD, then the warning
messages for the use of a test key must be presented.

Cc: Sean Brogan 
Cc: David Wei 
Cc: Mang Guo 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael D Kinney 
---
 .../Library/PlatformBdsLib/BdsPlatform.c   | 57 +-
 .../Library/PlatformBdsLib/PlatformBdsLib.inf  | 22 +
 2 files changed, 68 insertions(+), 11 deletions(-)

diff --git a/Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.c 
b/Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.c
index 7f91777ea1..4aac7a2487 100644
--- a/Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.c
+++ b/Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.c
@@ -1,6 +1,6 @@
 /** @file
 
-  Copyright (c) 2004  - 2016, Intel Corporation. All rights reserved.
+  Copyright (c) 2004  - 2018, 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 that accompanies this 
distribution.  
@@ -2417,6 +2417,10 @@ ShowProgressHotKey (
   UINTN TmpStrSize;
   VOID  *Buffer;
   UINTN Size;
+  VOID  *PublicKeyData;
+  UINTN PublicKeyDataLength;
+  UINT8 *PublicKeyDataXdr;
+  UINT8 *PublicKeyDataXdrEnd;
 
   if (TimeoutDefault == 0) {
 return EFI_TIMEOUT;
@@ -2484,6 +2488,57 @@ ShowProgressHotKey (
   }
   PcdSetBoolS(PcdTestKeyUsed, TRUE);
 }
+
+//
+// Make sure none of the keys in PcdPkcs7CertBufferXdr match the test key
+//
+PublicKeyDataXdr= PcdGetPtr (PcdPkcs7CertBufferXdr);
+PublicKeyDataXdrEnd = PublicKeyDataXdr + PcdGetSize 
(PcdPkcs7CertBufferXdr);
+
+ASSERT (PublicKeyDataXdr != NULL);
+ASSERT (PublicKeyDataXdr != PublicKeyDataXdrEnd);
+
+//
+// Try each key from PcdPkcs7CertBufferXdr
+//
+while (PublicKeyDataXdr < PublicKeyDataXdrEnd) {
+  if (PublicKeyDataXdr + sizeof (UINT32) > PublicKeyDataXdrEnd) {
+//
+// Key data extends beyond end of PCD
+//
+break;
+  }
+  //
+  // Read key length stored in big endian format
+  //
+  PublicKeyDataLength = SwapBytes32 (*(UINT32 *)(PublicKeyDataXdr));
+  //
+  // Point to the start of the key data
+  //
+  PublicKeyDataXdr += sizeof (UINT32);
+  if (PublicKeyDataXdr + PublicKeyDataLength > PublicKeyDataXdrEnd) {
+//
+// Key data extends beyond end of PCD
+//
+break;
+  }
+  PublicKeyData = PublicKeyDataXdr;
+
+  if ((Size == PublicKeyDataLength) &&
+  (CompareMem(Buffer, PublicKeyData, Size) == 0)) {
+TmpStr3 = L"WARNING: Capsule Test Key is used.\r\n";
+if (DebugAssertEnabled()) {
+  DEBUG ((DEBUG_INFO, "\n\nWARNING: Capsule Test Key is used.\r\n"));
+} else {
+  SerialPortWrite((UINT8 *)"\n\nWARNING: Capsule Test Key is used.", 
sizeof("\n\nWARNING: Capsule Test Key is used."));
+}
+PcdSetBoolS(PcdTestKeyUsed, TRUE);
+  }
+
+  PublicKeyDataXdr += PublicKeyDataLength;
+  PublicKeyDataXdr = (UINT8 *)ALIGN_POINTER (PublicKeyDataXdr, 
sizeof(UINT32));
+}
+
 FreePool(Buffer);
   }
 
diff --git a/Vlv2TbltDevicePkg/Library/PlatformBdsLib/PlatformBdsLib.inf 
b/Vlv2TbltDevicePkg/Library/PlatformBdsLib/PlatformBdsLib.inf
index 7512556bb7..9f84d7b2e0 100644
--- a/Vlv2TbltDevicePkg/Library/PlatformBdsLib/PlatformBdsLib.inf
+++ b/Vlv2TbltDevicePkg/Library/PlatformBdsLib/PlatformBdsLib.inf
@@ -1,16 +1,17 @@
 #/** @file
 # Component name for module PlatformBootManagerLib
 #
-# Copyright (c) 2008  - 2016, 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 that accompanies this 
distribution.  

-# The full text of 

Re: [edk2] [RFC] Remove X86 .asm and .S assembly files in EDK2

[Gao, Liming]

Mike:
  Now, %pragma macho subsections_via_symbols can't be enabled. So, will nasm 
generate the bigger image size than .S assembly on macho? If yes, I agree nasm 
is not same to .S. .S may be kept for a while. But, .asm is not necessary. We 
can remove .asm first. 

Thanks
Liming
> -Original Message-
> From: Kinney, Michael D
> Sent: Wednesday, March 14, 2018 4:00 AM
> To: Gao, Liming ; edk2-devel@lists.01.org; Kinney, 
> Michael D 
> Subject: RE: [RFC] Remove X86 .asm and .S assembly files in EDK2
> 
> Liming,
> 
> After further evaluation on use of NASM with macho
> there are a couple issues that need to be resolved
> before the .asm and .S files can be removed from
> all components.
> 
> The details are at:
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=881
> 
> NASM issues to be resolved are:
> 
> https://bugzilla.nasm.us/show_bug.cgi?id=3392469
> https://bugzilla.nasm.us/show_bug.cgi?id=3392470
> 
> The recommendation is to not switch to NASM only
> for libraries until the dead code stripping feature
> works as expected on macho.
> 
> We can consider moving to NASM only for NASM files
> in a module if there are no NASM functions that would
> be dead stripped.  The recommendation for these types
> of NASM files is do not use the following pragma
> until the NASM issues above are resolved.
> 
>   %pragma macho subsections_via_symbols
> 
> Best regards,
> 
> Mike
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-
> > boun...@lists.01.org] On Behalf Of Gao, Liming
> > Sent: Wednesday, January 31, 2018 3:06 AM
> > To: edk2-devel@lists.01.org
> > Subject: [edk2] [RFC] Remove X86 .asm and .S assembly
> > files in EDK2
> >
> > Edk2 has used nasm assembly file for all tool chains.
> > So, IA32 and X64 .asm and .S assembly files can be
> > removed if their nasm files are ready. It can save the
> > maintain effort and avoid the confuse.
> >
> >
> >
> > If you have any comments on this change, please let me
> > know.
> >
> > Thanks
> > Liming
> > ___
> > 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: allow HeapGuard even before CpuArchProtocol installed

[Wang, Jian J]

There's a bit operation error found in 32-bit platform. I'll send a v2 patch 
later.

Regards,
Jian


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jian J
> Wang
> Sent: Wednesday, March 14, 2018 4:31 PM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Dong, Eric ;
> Zeng, Star 
> Subject: [edk2] [PATCH] MdeModulePkg/Core: allow HeapGuard even before
> CpuArchProtocol installed
> 
> Due to the fact that HeapGuard needs CpuArchProtocol to update page
> attributes, the feature is normally enabled after CpuArchProtocol is
> installed. Since there're some drivers are loaded before CpuArchProtocl,
> they cannot make use HeapGuard feature to detect potential issues.
> 
> This patch fixes above situation by updating the DXE core to skip the
> NULL check against global gCpu in the IsMemoryTypeToGuard(), and adding
> NULL check against gCpu in SetGuardPage() and UnsetGuardPage() to make
> sure that they can be called but do nothing. This will allow HeapGuard to
> record all guarded memory without setting the related Guard pages to not-
> present.
> 
> Once the CpuArchProtocol is installed, a protocol notify will be called
> to complete the work of setting Guard pages to not-present.
> 
> Please note that above changes will cause a #PF in GCD code during cleanup
> of map entries, which is initiated by CpuDxe driver to update real mtrr
> and paging attributes back to GCD. During that time, CpuDxe doesn't allow
> GCD to update memory attributes and then any Guard page cannot be unset.
> As a result, this will prevent Guarded memory from freeing during memory
> map cleanup.
> 
> The solution is to avoid allocating guarded memory as memory map entries
> in GCD code. It's done by setting global mOnGuarding to TRUE before memory
> allocation and setting it back to FALSE afterwards in GCD function
> CoreAllocateGcdMapEntry().
> 
> Cc: Star Zeng 
> Cc: Eric Dong 
> Cc: Jiewen Yao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  MdeModulePkg/Core/Dxe/Gcd/Gcd.c   |  10 ++
>  MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 132
> +-
>  MdeModulePkg/Core/Dxe/Mem/HeapGuard.h |   8 ++
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c |   5 +
>  4 files changed, 154 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> index 8fbc3d282c..77f4adb4bc 100644
> --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> @@ -16,6 +16,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
> 
>  #include "DxeMain.h"
>  #include "Gcd.h"
> +#include "Mem/HeapGuard.h"
> 
>  #define MINIMUM_INITIAL_MEMORY_SIZE 0x1
> 
> @@ -391,12 +392,21 @@ CoreAllocateGcdMapEntry (
>IN OUT EFI_GCD_MAP_ENTRY  **BottomEntry
>)
>  {
> +  //
> +  // Set to mOnGuarding to TRUE before memory allocation. This will make sure
> +  // that the entry memory is not "guarded" by HeapGuard. Otherwise it might
> +  // cause problem when it's freed (if HeapGuard is enabled).
> +  //
> +  mOnGuarding = TRUE;
>*TopEntry = AllocateZeroPool (sizeof (EFI_GCD_MAP_ENTRY));
> +  mOnGuarding = FALSE;
>if (*TopEntry == NULL) {
>  return EFI_OUT_OF_RESOURCES;
>}
> 
> +  mOnGuarding = TRUE;
>*BottomEntry = AllocateZeroPool (sizeof (EFI_GCD_MAP_ENTRY));
> +  mOnGuarding = FALSE;
>if (*BottomEntry == NULL) {
>  CoreFreePool (*TopEntry);
>  return EFI_OUT_OF_RESOURCES;
> diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
> b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
> index 19245049c2..de2c468b83 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
> @@ -576,6 +576,10 @@ SetGuardPage (
>IN  EFI_PHYSICAL_ADDRESS  BaseAddress
>)
>  {
> +  if (gCpu == NULL) {
> +return;
> +  }
> +
>//
>// Set flag to make sure allocating memory without GUARD for page table
>// operation; otherwise infinite loops could be caused.
> @@ -606,6 +610,10 @@ UnsetGuardPage (
>  {
>UINT64  Attributes;
> 
> +  if (gCpu == NULL) {
> +return;
> +  }
> +
>//
>// Once the Guard page is unset, it will be freed back to memory pool. NX
>// memory protection must be restored for this page if NX is enabled for 
> free
> @@ -652,7 +660,7 @@ IsMemoryTypeToGuard (
>UINT64 ConfigBit;
>BOOLEAN InSmm;
> 
> -  if (gCpu == NULL || AllocateType == AllocateAddress) {
> +  if (AllocateType == AllocateAddress) {
>  return FALSE;
>}
> 
> @@ -1160,6 +1168,128 @@ CoreConvertPagesWithGuard (
>return CoreConvertPages (Start, NumberOfPages, NewType);
>  }
> 
> +/**
> +  Set all Guard pages which cannot be set before CPU Arch Protocol installed.
> +**/
> +VOID
> 

Re: [edk2] [PATCH v5 0/6] Add translation support to generic PciHostBridge

[Ni, Ruiyu]

On 3/1/2018 2:57 PM, Heyi Guo wrote:

Patch v5 inherits the code from RFC v4; we don't restart the version number for
RFC to PATCH change.

v5:
- Patch 4/6: Modify the code according to the comments from Ray.
- Patch 1/6 and 2/6 are totally new. They add initialization for all fields of
   PCI_ROOT_BRIDGE_APERTURE temporary variables in PciHostBridgeLib instances, 
so
   that they will not suffer from extension of PCI_ROOT_BRIDGE_APERTURE
   structure.
- Generate a separate patch (3/6) for PciHostBridgeLib.h change. Though it is a
   prerequisite for patch 4/6, it does not change the code in PciHostBridge
   driver and won't cause any build failure or functional issue.


v4:
- Modify the code according to the comments from Ray, Laszlo and Ard (Please see
   the notes of Patch 1/3)
- Ignore translation of bus in CreateRootBridge.


v3:
- Keep definition of Translation consistent in EDKII code: Translation = device
   address - host address.
- Patch 2/2 is split into 2 patches (2/3 and 3/3).
- Refine comments and commit messages to make the code easier to understand.


v2:
Changs are made according to the discussion on the mailing list, including:

- PciRootBridgeIo->Configuration should return CPU view address, as well as
   PciIo->GetBarAttributes, and Translation Offset should be equal to PCI view
   address - CPU view address.
- Add translation offset to PCI_ROOT_BRIDGE_APERTURE structure definition.
- PciHostBridge driver internally used Base Address is still based on PCI view
   address, and translation offset = CPU view - PCI view, which follows the
   definition in ACPI, and not the same as that in UEFI spec.

Cc: Ruiyu Ni 
Cc: Ard Biesheuvel 
Cc: Star Zeng 
Cc: Eric Dong 
Cc: Laszlo Ersek 
Cc: Michael D Kinney 
Cc: Maurice Ma 
Cc: Prince Agyeman 
Cc: Benjamin You 
Cc: Jordan Justen 
Cc: Anthony Perard 
Cc: Julien Grall 

Heyi Guo (6):
   CorebootPayloadPkg/PciHostBridgeLib: Init PCI aperture to 0
   OvmfPkg/PciHostBridgeLib: Init PCI aperture to 0
   MdeModulePkg/PciHostBridgeLib.h: add address Translation
   MdeModulePkg/PciHostBridgeDxe: Add support for address translation
   MdeModulePkg/PciBus: convert host address to device address
   MdeModulePkg/PciBus: return CPU address for GetBarAttributes

  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h  |  21 
  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h|   3 +
  MdeModulePkg/Include/Library/PciHostBridgeLib.h|  19 +++
  CorebootPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c |   5 +
  MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c |  12 +-
  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c  | 129 
+---
  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c| 118 
--
  OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c|   4 +
  OvmfPkg/Library/PciHostBridgeLib/XenSupport.c  |   5 +
  9 files changed, 288 insertions(+), 28 deletions(-)


Reviewed-by: Ruiyu Ni 

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

[edk2] [PATCH v1 2/2] BaseTools: AutoGen should use is None not == None

[Jaben Carsey]

change to the style we document as in use

Cc: Yonghong Zhu 
Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jaben Carsey 
---
 BaseTools/Source/Python/AutoGen/AutoGen.py | 138 ++--
 1 file changed, 69 insertions(+), 69 deletions(-)

diff --git a/BaseTools/Source/Python/AutoGen/AutoGen.py 
b/BaseTools/Source/Python/AutoGen/AutoGen.py
index 5b09e0008ddd..2bea1adc5193 100644
--- a/BaseTools/Source/Python/AutoGen/AutoGen.py
+++ b/BaseTools/Source/Python/AutoGen/AutoGen.py
@@ -766,7 +766,7 @@ class WorkspaceAutoGen(AutoGen):
 for Fv in Fdf.Profile.FvDict:
 _GuidDict = {}
 for FfsFile in Fdf.Profile.FvDict[Fv].FfsList:
-if FfsFile.InfFileName and FfsFile.NameGuid == None:
+if FfsFile.InfFileName and FfsFile.NameGuid is None:
 #
 # Get INF file GUID
 #
@@ -939,13 +939,13 @@ class WorkspaceAutoGen(AutoGen):
 
 ## Return the directory to store FV files
 def _GetFvDir(self):
-if self._FvDir == None:
+if self._FvDir is None:
 self._FvDir = path.join(self.BuildDir, 'FV')
 return self._FvDir
 
 ## Return the directory to store all intermediate and final files built
 def _GetBuildDir(self):
-if self._BuildDir == None:
+if self._BuildDir is None:
 return self.AutoGenObjectList[0].BuildDir
 
 ## Return the build output directory platform specifies
@@ -973,7 +973,7 @@ class WorkspaceAutoGen(AutoGen):
 #   @retval string  Makefile directory
 #
 def _GetMakeFileDir(self):
-if self._MakeFileDir == None:
+if self._MakeFileDir is None:
 self._MakeFileDir = self.BuildDir
 return self._MakeFileDir
 
@@ -982,7 +982,7 @@ class WorkspaceAutoGen(AutoGen):
 #   @retval string  Build command string
 #
 def _GetBuildCommand(self):
-if self._BuildCommand == None:
+if self._BuildCommand is None:
 # BuildCommand should be all the same. So just get one from 
platform AutoGen
 self._BuildCommand = self.AutoGenObjectList[0].BuildCommand
 return self._BuildCommand
@@ -1331,7 +1331,7 @@ class PlatformAutoGen(AutoGen):
 for SkuName in Pcd.SkuInfoList:
 Sku = Pcd.SkuInfoList[SkuName]
 SkuId = Sku.SkuId
-if SkuId == None or SkuId == '':
+if SkuId is None or SkuId == '':
 continue
 if len(Sku.VariableName) > 0:
 VariableGuidStructure = Sku.VariableGuidValue
@@ -1642,7 +1642,7 @@ class PlatformAutoGen(AutoGen):
 # if the offset of a VPD is *, then it need to be 
fixed up by third party tool.
 if not NeedProcessVpdMapFile and Sku.VpdOffset == "*":
 NeedProcessVpdMapFile = True
-if self.Platform.VpdToolGuid == None or 
self.Platform.VpdToolGuid == '':
+if self.Platform.VpdToolGuid is None or 
self.Platform.VpdToolGuid == '':
 EdkLogger.error("Build", FILE_NOT_FOUND, \
 "Fail to find third-party BPDG 
tool to process VPD PCDs. BPDG Guid tool need to be defined in tools_def.txt 
and VPD_TOOL_GUID need to be provided in DSC file.")
 
@@ -1654,7 +1654,7 @@ class PlatformAutoGen(AutoGen):
 for DscPcd in PlatformPcds:
 DscPcdEntry = self._PlatformPcds[DscPcd]
 if DscPcdEntry.Type in [TAB_PCDS_DYNAMIC_VPD, 
TAB_PCDS_DYNAMIC_EX_VPD]:
-if not (self.Platform.VpdToolGuid == None or 
self.Platform.VpdToolGuid == ''):
+if not (self.Platform.VpdToolGuid is None or 
self.Platform.VpdToolGuid == ''):
 FoundFlag = False
 for VpdPcd in VpdFile._VpdArray.keys():
 # This PCD has been referenced by module
@@ -1734,7 +1734,7 @@ class PlatformAutoGen(AutoGen):
 
 # if the offset of a VPD is *, then it need to 
be fixed up by third party tool.
 VpdSkuMap[DscPcd] = SkuValueMap
-if (self.Platform.FlashDefinition == None or 
self.Platform.FlashDefinition == '') and \
+if (self.Platform.FlashDefinition is None or 
self.Platform.FlashDefinition == '') and \
VpdFile.GetCount() != 0:
 EdkLogger.error("build", ATTRIBUTE_NOT_AVAILABLE, 
 "Fail to get FLASH_DEFINITION definition in 
DSC file %s which is required when DSC contains VPD PCD." % 
str(self.Platform.MetaFile))
@@ -1824,7 +1824,7 @@ class PlatformAutoGen(AutoGen):
 
 ## Return the platform build data object
 def _GetPlatform(self):
-   

[edk2] [PATCH v1 1/2] BaseTools: Autogen - modify to use standard parent/child class relationships

[Jaben Carsey]

use __new__ and __init__ to create/manage/initialize objects in standard flow.

Cc: Yonghong Zhu 
Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jaben Carsey 
---
 BaseTools/Source/Python/AutoGen/AutoGen.py | 93 
 1 file changed, 54 insertions(+), 39 deletions(-)

diff --git a/BaseTools/Source/Python/AutoGen/AutoGen.py 
b/BaseTools/Source/Python/AutoGen/AutoGen.py
index 439e360955a3..5b09e0008ddd 100644
--- a/BaseTools/Source/Python/AutoGen/AutoGen.py
+++ b/BaseTools/Source/Python/AutoGen/AutoGen.py
@@ -159,8 +159,8 @@ ${tail_comments}
 #   This class just implements the cache mechanism of AutoGen objects.
 #
 class AutoGen(object):
-# database to maintain the objects of xxxAutoGen
-_CACHE_ = {}# (BuildTarget, ToolChain) : {ARCH : {platform file: 
AutoGen object}}}
+# database to maintain the objects in each child class
+__ObjectCache = {}# (BuildTarget, ToolChain, ARCH, platform file): 
AutoGen object
 
 ## Factory method
 #
@@ -174,24 +174,19 @@ class AutoGen(object):
 #   @param  *args   The specific class related parameters
 #   @param  **kwargsThe specific class related dict parameters
 #
-def __new__(Class, Workspace, MetaFile, Target, Toolchain, Arch, *args, 
**kwargs):
+def __new__(cls, Workspace, MetaFile, Target, Toolchain, Arch, *args, 
**kwargs):
 # check if the object has been created
-Key = (Target, Toolchain)
-if Key not in Class._CACHE_ or Arch not in Class._CACHE_[Key] \
-   or MetaFile not in Class._CACHE_[Key][Arch]:
-AutoGenObject = super(AutoGen, Class).__new__(Class)
-# call real constructor
-if not AutoGenObject._Init(Workspace, MetaFile, Target, Toolchain, 
Arch, *args, **kwargs):
-return None
-if Key not in Class._CACHE_:
-Class._CACHE_[Key] = {}
-if Arch not in Class._CACHE_[Key]:
-Class._CACHE_[Key][Arch] = {}
-Class._CACHE_[Key][Arch][MetaFile] = AutoGenObject
-else:
-AutoGenObject = Class._CACHE_[Key][Arch][MetaFile]
+Key = (Target, Toolchain, Arch, MetaFile)
+try:
+# if it exists, just return it directly
+return cls.__ObjectCache[Key]
+except:
+# it didnt exist. create it, cache it, then return it
+cls.__ObjectCache[Key] = super(AutoGen, cls).__new__(cls)
+return cls.__ObjectCache[Key]
 
-return AutoGenObject
+def __init__ (self, Workspace, MetaFile, Target, Toolchain, Arch, *args, 
**kwargs):
+super(AutoGen, self).__init__(self, Workspace, MetaFile, Target, 
Toolchain, Arch, *args, **kwargs)
 
 ## hash() operator
 #
@@ -221,10 +216,16 @@ class AutoGen(object):
 # architecture. This class will generate top level makefile.
 #
 class WorkspaceAutoGen(AutoGen):
-## Real constructor of WorkspaceAutoGen
-#
-# This method behaves the same as __init__ except that it needs explicit 
invoke
-# (in super class's __new__ method)
+# call super().__init__ then call the worker function with different 
parameter count
+def __init__(self, Workspace, MetaFile, Target, Toolchain, Arch, *args, 
**kwargs):
+try:
+self._Init
+except:
+super(WorkspaceAutoGen, self).__init__(Workspace, MetaFile, 
Target, Toolchain, Arch, *args, **kwargs)
+self._InitWorker(Workspace, MetaFile, Target, Toolchain, Arch, 
*args, **kwargs)
+self._Init = True
+
+## Initialize WorkspaceAutoGen
 #
 #   @param  WorkspaceDirRoot directory of workspace
 #   @param  ActivePlatform  Meta-file of active platform
@@ -240,7 +241,7 @@ class WorkspaceAutoGen(AutoGen):
 #   @param  CapsCapsule list to be generated
 #   @param  SkuId   SKU id from command line
 #
-def _Init(self, WorkspaceDir, ActivePlatform, Target, Toolchain, ArchList, 
MetaFileDb,
+def _InitWorker(self, WorkspaceDir, ActivePlatform, Target, Toolchain, 
ArchList, MetaFileDb,
   BuildConfig, ToolDefinition, FlashDefinitionFile='', Fds=None, 
Fvs=None, Caps=None, SkuId='', UniFlag=None,
   Progress=None, BuildModule=None):
 if Fds is None:
@@ -,6 +1112,14 @@ class WorkspaceAutoGen(AutoGen):
 #  file in order to generate makefile for platform.
 #
 class PlatformAutoGen(AutoGen):
+# call super().__init__ then call the worker function with different 
parameter count
+def __init__(self, Workspace, MetaFile, Target, Toolchain, Arch, *args, 
**kwargs):
+try:
+self._Init
+except:
+super(PlatformAutoGen, self).__init__(self, Workspace, MetaFile, 
Target, Toolchain, Arch, *args, **kwargs)
+self._InitWorker(Workspace, MetaFile, 

[edk2] [PATCH v1 0/2] BaseTools: AutoGen code style compliance

[Jaben Carsey]

update the object factory and child classes to use standard functions
update the file to use is None instead of == None

Jaben Carsey (2):
  BaseTools: Autogen - modify to use standard parent/child class
relationships
  BaseTools: AutoGen should use is None not == None

 BaseTools/Source/Python/AutoGen/AutoGen.py | 231 +++-
 1 file changed, 123 insertions(+), 108 deletions(-)

-- 
2.16.2.windows.1

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

[edk2] To save size, use NULL library for DebugLib and ReportStatusCodeLib

[david moheban]

What does that mean??

Sorry for asking beginner questions but was just wondering. Trying to
shrink the size of the Duet image.

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

Re: [edk2] AARCH64:Use of EFI_MEMORY_XP

[Ard Biesheuvel]

On 14 March 2018 at 19:34, Evan Lloyd  wrote:
> Hi Ard.
> We still have a minor problem in that the spec disqualifies EFI_MEMORY_XP for 
> AARCH64.
> Do you have any thoughts on this?
> How should we proceed here?  I assume the specification statement was a 
> considered decision.
> Do we need to get it changed, or is EFI_MEMORY_XP unnecessary?
>

No, that is a spec bug

EFI_MEMORY_RO and EFI_MEMORY_XP are essential for things like the
memory attributes table, which prevents UEFI memory regions from being
an exploit walhalla consisting only of memory regions that are
writable and executable at the same time, which would defeat all the
hard work OS engineers are doing to tighten memory permissions in
privileged execution contexts.

In this particular case, having a read-write-execute framebuffer could
be a security hazard as well, so I'd prefer to strip the executable
permissions here.


>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>> Evan Lloyd
>> Sent: 08 January 2018 18:51
>> To: Ard Biesheuvel 
>> Cc: "matteo.carl...@arm.com"@arm.com;
>> "leif.lindh...@linaro.org"@arm.com; "n...@arm.com"@arm.com; edk2-
>> de...@lists.01.org; Arvind Chauhan ;
>> "ard.biesheu...@linaro.org"@arm.com; Thomas Abraham
>> 
>> Subject: Re: [edk2] [PATCH edk2-platforms v2 15/18] ARM/VExpressPkg:
>> New DP500/DP550/DP650 platform library.
>>
>>
>>
>> > -Original Message-
>> > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>> > Sent: 23 December 2017 16:07
>> > To: Evan Lloyd 
>> > Cc: edk2-devel@lists.01.org; Arvind Chauhan
>> ;
>> > Daniil Egranov ; Thomas Abraham
>> > ; "ard.biesheu...@linaro.org"@arm.com;
>> > "leif.lindh...@linaro.org"@arm.com;
>> > "matteo.carl...@arm.com"@arm.com; "n...@arm.com"@arm.com
>> > Subject: Re: [PATCH edk2-platforms v2 15/18] ARM/VExpressPkg: New
>> > DP500/DP550/DP650 platform library.
>> >
> ...
>> > > +  // Mark the VRAM as write-combining. The VRAM is inside the DRAM,
>> > > + which is  // cacheable, for ARM/AArch64 EFI_MEMORY_WC memory
>> is
>> > actually uncached.
>> > > +  Status = gDS->SetMemorySpaceAttributes (
>> > > +  *VramBaseAddress,
>> > > +  *VramSize,
>> > > +  EFI_MEMORY_WC
>> >
>> > Please add EFI_MEMORY_XP here
>> >
>>
>>  [[Evan Lloyd]] We can do that, happily.  However, in looking at this we
>> found that the UEFI spec has in "2.3.6 AArch64 Platforms", section "2.3.6.1
>> Memory types":
>> EFI_MEMORY_XP, ...   
>>   Not used
>> or defined
>>
>> Does that suggest we need a minor spec update?
>>
>> > > +  );
> ...
> IMPORTANT NOTICE: The contents of this email and any attachments are 
> confidential and may also be privileged. If you are not the intended 
> recipient, please notify the sender immediately and do not disclose the 
> contents to any other person, use it for any purpose, or store or copy the 
> information in any medium. Thank you.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] AARCH64:Use of EFI_MEMORY_XP

[Evan Lloyd]

Hi Ard.
We still have a minor problem in that the spec disqualifies EFI_MEMORY_XP for 
AARCH64.
Do you have any thoughts on this?
How should we proceed here?  I assume the specification statement was a 
considered decision.
Do we need to get it changed, or is EFI_MEMORY_XP unnecessary?

Regards,
Evan

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Evan Lloyd
> Sent: 08 January 2018 18:51
> To: Ard Biesheuvel 
> Cc: "matteo.carl...@arm.com"@arm.com;
> "leif.lindh...@linaro.org"@arm.com; "n...@arm.com"@arm.com; edk2-
> de...@lists.01.org; Arvind Chauhan ;
> "ard.biesheu...@linaro.org"@arm.com; Thomas Abraham
> 
> Subject: Re: [edk2] [PATCH edk2-platforms v2 15/18] ARM/VExpressPkg:
> New DP500/DP550/DP650 platform library.
>
>
>
> > -Original Message-
> > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> > Sent: 23 December 2017 16:07
> > To: Evan Lloyd 
> > Cc: edk2-devel@lists.01.org; Arvind Chauhan
> ;
> > Daniil Egranov ; Thomas Abraham
> > ; "ard.biesheu...@linaro.org"@arm.com;
> > "leif.lindh...@linaro.org"@arm.com;
> > "matteo.carl...@arm.com"@arm.com; "n...@arm.com"@arm.com
> > Subject: Re: [PATCH edk2-platforms v2 15/18] ARM/VExpressPkg: New
> > DP500/DP550/DP650 platform library.
> >
...
> > > +  // Mark the VRAM as write-combining. The VRAM is inside the DRAM,
> > > + which is  // cacheable, for ARM/AArch64 EFI_MEMORY_WC memory
> is
> > actually uncached.
> > > +  Status = gDS->SetMemorySpaceAttributes (
> > > +  *VramBaseAddress,
> > > +  *VramSize,
> > > +  EFI_MEMORY_WC
> >
> > Please add EFI_MEMORY_XP here
> >
>
>  [[Evan Lloyd]] We can do that, happily.  However, in looking at this we
> found that the UEFI spec has in "2.3.6 AArch64 Platforms", section "2.3.6.1
> Memory types":
> EFI_MEMORY_XP, ...
>  Not used
> or defined
>
> Does that suggest we need a minor spec update?
>
> > > +  );
...
IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH][platforms/devel-dynamictables] Branch to implement Dynamic Tables Framework

[Sami Mujawar]

This patch introduces a branch for implementing Dynamic Tables
Framework. The description is in the Readme.md file.

Please create a branch called 'devel-dynamictables' in edk2-platforms.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sami Mujawar 
Signed-off-by: Evan Lloyd 
---
 Readme.md | 149 
 1 file changed, 149 insertions(+)

diff --git a/Readme.md b/Readme.md
new file mode 100644
index 
..dfe247d3fef8936b2f4551cf9f7e735a28bcf1da
--- /dev/null
+++ b/Readme.md
@@ -0,0 +1,149 @@
+This branch will be used to develop the Configuration Manager,
+which is the platform specific component of the Dynamic Tables
+Framework.
+
+Dynamic Tables Framework
+
+
+To reduce the amount of effort required in porting firmware to new
+platforms, we propose this "Dynamic Tables" framework.  The aim is
+to provide an example implementation capable of generating the
+firmware tables from an external source.  This is potentially a
+management node, either local or remote, or, where suitable, a file
+that might be generated from the system construction.  This initial
+"proof of concept" release does not fully implement that - the
+configuration is held in local UEFI modules.
+
+Branch Owners
+-
+ Evan Lloyd  \
+ Sami Mujawar 
+
+Feature Summary
+---
+The dynamic tables framework is designed to generate standardised
+firmware tables that describe the hardware information at
+run-time. A goal of standardised firmware is to have a common
+firmware for a platform capable of booting both Windows and Linux
+operating systems.
+
+Traditionally the firmware tables are handcrafted using ACPI
+Source Language (ASL), Table Definition Language (TDL) and
+C-code. This approach can be error prone and involves time
+consuming debugging. In addition, it may be desirable to configure
+platform hardware at runtime such as: configuring the number of
+cores available for use by the OS, or turning SoC features ON or
+OFF.
+
+The dynamic tables framework simplifies this by providing a set
+of standard table generators, that are implemented as libraries.
+These generators query a platform specific component, the
+'Configuration Manager', to collate the information required
+for generating the tables at run-time.
+
+The framework also provides the ability to implement custom/OEM
+generators; thereby facilitating support for custom tables. The
+custom generators can also utilize the existing standard generators
+and override any functionality if needed.
+
+The framework currently implements a set of standard ACPI table
+generators for ARM architecture, that can generate Server Base Boot
+Requirement (SBBR) compliant tables. Although, the set of standard
+generators implement the functionality required for ARM architecture;
+the framework is extensible, and support for other architectures can
+be added easily.
+
+The framework currently supports the following table generators for ARM:
+* DBG2 - Debug Port Table 2
+* DSDT - Differentiated system description table. This is essentially
+ a RAW table generator.
+* FADT - Fixed ACPI Description Table
+* GTDT - Generic Timer Description Table
+* IORT - IO Remapping Table
+* MADT - Multiple APIC Description Table
+* MCFG - PCI Express memory mapped configuration space base address
+ Description Table
+* SPCR - Serial Port Console Redirection Table
+* SSDT - Secondary System Description Table. This is essentially
+ a RAW table generator.
+
+Roadmap
+---
+The current implementation of the Configuration Manager populates the
+platform information statically as a C structure. Further enhancements
+to introduce runtime loading of platform information from a platform
+information file is planned.
+
+Also support for generating SMBIOS tables is planned and will be added
+subsequently.
+
+Related Modules
+---
+
+### edk2-staging
+The *dynamictables* branch in the **edk2-staging** repository
+contains the Dynamic Tables Framework.
+
+### ACPICA iASL compiler
+The RAW table generator, used to process the DSDT/SSDT files depends on
+the iASL compiler to convert the DSDT/SSDT ASL files to a C array containing
+the hex AML code. The current implementation of the iASL compiler does not
+support generation of a C header file suitable for including from a C source
+file.
+
+Related Links
+--
+
+
+
+
+
+
+Supported Platforms
+---
+1. Juno
+2. FVP Models
+
+Build Instructions
+--
+1. Set path for the iASL compiler with support for generating a C header
+   file as output.
+
+2. Set PACKAGES_PATH to point to the locations of the following repositories:
+
+Example:
+
+> set PACKAGES_PATH=%CD%\edk2;%CD%\edk2-platforms;%CD%\edk2-non-osi
+
+  or
+
+> export 

[edk2] [staging/dynamictables PATCH] Branch to implement Dynamic Tables Framework

[Sami Mujawar]

This patch introduces a branch for implementing Dynamic Tables
Framework. The description is in the Readme.md file.

Please create a branch called 'dynamictables' in edk2-staging.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sami Mujawar 
Signed-off-by: Evan Lloyd 
---
 Readme.md | 149 
 1 file changed, 149 insertions(+)

diff --git a/Readme.md b/Readme.md
new file mode 100644
index 
..9779dae812aff7cc05cdfe1d4acd5b5c2d197483
--- /dev/null
+++ b/Readme.md
@@ -0,0 +1,149 @@
+This branch will be used to develop the Dynamic Tables Framework.
+
+Dynamic Tables Framework
+
+
+To reduce the amount of effort required in porting firmware to new
+platforms, we propose this "Dynamic Tables" framework.  The aim is
+to provide an example implementation capable of generating the
+firmware tables from an external source.  This is potentially a
+management node, either local or remote, or, where suitable, a file
+that might be generated from the system construction.  This initial
+"proof of concept" release does not fully implement that - the
+configuration is held in local UEFI modules.
+
+Branch Owners
+-
+ Evan Lloyd  \
+ Sami Mujawar 
+
+Feature Summary
+---
+The dynamic tables framework is designed to generate standardised
+firmware tables that describe the hardware information at
+run-time. A goal of standardised firmware is to have a common
+firmware for a platform capable of booting both Windows and Linux
+operating systems.
+
+Traditionally the firmware tables are handcrafted using ACPI
+Source Language (ASL), Table Definition Language (TDL) and
+C-code. This approach can be error prone and involves time
+consuming debugging. In addition, it may be desirable to configure
+platform hardware at runtime such as: configuring the number of
+cores available for use by the OS, or turning SoC features ON or
+OFF.
+
+The dynamic tables framework simplifies this by providing a set
+of standard table generators, that are implemented as libraries.
+These generators query a platform specific component, the
+'Configuration Manager', to collate the information required
+for generating the tables at run-time.
+
+The framework also provides the ability to implement custom/OEM
+generators; thereby facilitating support for custom tables. The
+custom generators can also utilize the existing standard generators
+and override any functionality if needed.
+
+The framework currently implements a set of standard ACPI table
+generators for ARM architecture, that can generate Server Base Boot
+Requirement (SBBR) compliant tables. Although, the set of standard
+generators implement the functionality required for ARM architecture;
+the framework is extensible, and support for other architectures can
+be added easily.
+
+The framework currently supports the following table generators for ARM:
+* DBG2 - Debug Port Table 2
+* DSDT - Differentiated system description table. This is essentially
+ a RAW table generator.
+* FADT - Fixed ACPI Description Table
+* GTDT - Generic Timer Description Table
+* IORT - IO Remapping Table
+* MADT - Multiple APIC Description Table
+* MCFG - PCI Express memory mapped configuration space base address
+ Description Table
+* SPCR - Serial Port Console Redirection Table
+* SSDT - Secondary System Description Table. This is essentially
+ a RAW table generator.
+
+Roadmap
+---
+The current implementation of the Configuration Manager populates the
+platform information statically as a C structure. Further enhancements
+to introduce runtime loading of platform information from a platform
+information file is planned.
+
+Also support for generating SMBIOS tables is planned and will be added
+subsequently.
+
+Related Modules
+---
+
+### edk2-platforms
+The *devel-dynamictables* branch in the **edk2-platform** repository contains
+the Configuration Manager implementation (the platform specific component)
+for Juno and Fixed Virtual Platform models.
+
+### ACPICA iASL compiler
+The RAW table generator, used to process the DSDT/SSDT files depends on
+the iASL compiler to convert the DSDT/SSDT ASL files to a C array containing
+the hex AML code. The current implementation of the iASL compiler does not
+support generation of a C header file suitable for including from a C source
+file.
+
+Related Links
+--
+
+
+
+
+
+Supported Platforms
+---
+1. Juno
+2. FVP Models
+
+Build Instructions
+--
+1. Set path for the iASL compiler with support for generating a C header
+   file as output.
+
+2. Set PACKAGES_PATH to point to the locations of the following repositories:
+
+Example:
+
+> set PACKAGES_PATH=%CD%\edk2;%CD%\edk2-platforms;%CD%\edk2-non-osi
+
+  or
+
+> export 

Re: [edk2] [Patch 0/5] Add multi-cert PcdPkcs7CertBufferXdr

[Steele, Kelly]

Reviewed-by: Kelly Steele 

Thanks,
Kelly

> -Original Message-
> From: Kinney, Michael D
> Sent: March 12, 2018 12:30
> To: edk2-devel@lists.01.org
> Cc: Sean Brogan ; Zhu, Yonghong
> ; Gao, Liming ; Zhang,
> Chao B ; Yao, Jiewen ;
> Steele, Kelly ; Wei, David ;
> Guo, Mang ; Kinney, Michael D
> 
> Subject: [Patch 0/5] Add multi-cert PcdPkcs7CertBufferXdr
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=890
> https://bugzilla.tianocore.org/show_bug.cgi?id=891
> 
> * Update BinToPcd to support multiple one or more -i INPUTFILE arguments
> * Update BinToPcd to support -x, --xdr flags to encode PCD using the
>   Variable-Length Opaque Data of RFC 4506 External Data Representation
>   Standard (XDR).
> * Add PcdPkcs7CertBufferXdr that supports one or more PKCS7 certificates
>   encoded using the Variable-Length Opaque Data format of RFC 4506
> External
>   Data Representation Standard (XDR).
> * Use both PcdPkcs7CertBuffer and PcdPkcs7CertBufferXdr to authenticate
>   capsules.
> * Evaluate both PcdPkcs7CertBuffer and PcdPkcs7CertBufferXdr for the use
>   of the test key.
> 
> Branch for review:
> https://github.com/mdkinney/edk2/tree/Bug_890_891_BinToPcdMultipleIn
> putFiles
> 
> Cc: Sean Brogan 
> Cc: Yonghong Zhu 
> Cc: Liming Gao 
> Cc: Chao Zhang 
> Cc: Jiewen Yao 
> Cc: Kelly Steele 
> Cc: David Wei 
> Cc: Mang Guo 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Michael D Kinney 
> 
> Kinney, Michael D (4):
>   BaseTools/BinToPcd: Add support for multiple binary input files
>   SecurityPkg: Add PcdPkcs7CertBufferXdr
>   SecurityPkg/EdkiiSystemCapsuleLib: Use PcdPkcs7CertBufferXdr
>   QuarkPlatformPkg/PlatformBootManagerLib: Check PcdPkcs7CertBufferXdr
> 
> Michael D Kinney (1):
>   Vlv2TbltDevicePkg/PlatformBootManagerLib: Check PcdPkcs7CertBufferXdr
> 
>  BaseTools/Scripts/BinToPcd.py  | 83 
> ++
>  .../PlatformBootManagerLib/PlatformBootManager.c   | 51 -
>  .../PlatformBootManagerLib.inf |  3 +-
>  SecurityPkg/SecurityPkg.dec|  8 +++
>  SecurityPkg/SecurityPkg.uni|  6 ++
>  .../EdkiiSystemCapsuleLib/EdkiiSystemCapsuleLib.c  | 77
> +---
>  .../EdkiiSystemCapsuleLib.inf  |  3 +-
>  .../Library/PlatformBdsLib/BdsPlatform.c   | 57 ++-
>  .../Library/PlatformBdsLib/PlatformBdsLib.inf  | 22 +++---
>  9 files changed, 258 insertions(+), 52 deletions(-)
> 
> --
> 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 1/1] ArmPkg/TimerDxe: Add ISB for timer compare value reload

[Ard Biesheuvel]

On 14 March 2018 at 07:45, Marc Zyngier  wrote:
> On Wed, 14 Mar 2018 00:25:09 +,
> Guo Heyi wrote:
>>
>> On Tue, Mar 13, 2018 at 09:33:33AM +, Marc Zyngier wrote:
>> > On 13/03/18 00:31, Heyi Guo wrote:
>> > > If timer interrupt is level sensitive, reloading timer compare
>> > > register has a side effect of clearing GIC pending status, so a "ISB"
>> > > is needed to make sure this instruction is executed before enabling
>> > > CPU IRQ, or else we may get spurious timer interrupts.
>> > >
>> > > Contributed-under: TianoCore Contribution Agreement 1.1
>> > > Signed-off-by: Heyi Guo 
>> > > Signed-off-by: Yi Li 
>> > > Cc: Leif Lindholm 
>> > > Cc: Ard Biesheuvel 
>> > > Cc: Marc Zyngier 
>> > > ---
>> > >
>> > > Notes:
>> > > v2:
>> > > - Use ISB instead of DSB [Marc]
>> > > - Update commit message accordingly.
>> > >
>> > >  ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 +
>> > >  1 file changed, 1 insertion(+)
>> > >
>> > > diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c 
>> > > b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
>> > > index 33d7c91f..32abee8726a0 100644
>> > > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
>> > > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
>> > > @@ -337,6 +337,7 @@ TimerInterruptHandler (
>> > >
>> > >  // Set next compare value
>> > >  ArmGenericTimerSetCompareVal (CompareValue);
>> > > +ArmInstructionSynchronizationBarrier ();
>> > >  ArmGenericTimerEnableTimer ();
>> > >}
>> >
>> > Sorry for being pedantic here, but it would make more sense if ISB was
>> > placed after the enabling of the timer. Otherwise, you only force the
>> > update of the comparator. But on the other hand, the timer was never
>> > disabled the first place, in which case you'd wonder why you're trying
>> > to enable it again.
>> Yes, I also had such question and hesitated at this place :)
>> >
>> > So either you leave the ISB here and remove the enable call, or move the
>> > ISB after the enable.
>>
>> If we are going to remove the enable call, is it better to be changed in a
>> separate patch? It seems not related with adding ISB, though it is only a
>> one-line change.
>
> I guess a separate patch doesn't hurt, but that's for Ard and Leif to
> decide.
>

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

Re: [edk2] [edk2 PATCH v2 1/1] Maintainers.txt: Add StandaloneMmPkg and maintainers

[Leif Lindholm]

On Thu, Feb 15, 2018 at 01:31:50PM +, achin.gu...@arm.com wrote:
> From: Achin Gupta 
> 
> This patch adds maintainers, reviewer and directory for the
> StandaloneMmPkg. This package will host an implementation of Standalone
> Management Mode as specified in the Platform Initialization (PI)
> Specification, Volume 4: Management Mode Core Interface.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Achin Gupta 

Reviewed-by: Leif Lindholm 

> ---
>  Maintainers.txt | 5 +
>  StandaloneMmPkg | 0
>  2 files changed, 5 insertions(+)
> 
> diff --git a/Maintainers.txt b/Maintainers.txt
> index 74f2538..be5d527 100644
> --- a/Maintainers.txt
> +++ b/Maintainers.txt
> @@ -259,3 +259,8 @@ M: Mang Guo 
>  Vlv2TbltDevicePkg
>  M: David Wei 
>  M: Mang Guo 
> +
> +StandaloneMmPkg
> +M: Achin Gupta 
> +M: Jiewen Yao 
> +R: Supreeth Venkatesh 
> diff --git a/StandaloneMmPkg b/StandaloneMmPkg
> new file mode 100644
> index 000..e69de29
> -- 
> 1.9.1
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [RFC v1 1/1] MdeModulePkg/Usb: Use Pcd for UsbBootIoBlocks

[Ming Huang]

Mike,

Evaluating block size is a good idea!
The block size of the target USB device is 2048 in my platform.
I modify the UsbMassBoot.c with evaluating block size and it works.
I will send the patch later.

Thanks,
Ming

On 2018/3/13 9:38, Kinney, Michael D wrote:

> Star,
>
> Maybe we should evaluate block size of the target USB
> device and limit the total transfer size to 64KB.
>
> This would continue to use 128 for 512B blocks and 
> 32 for 2K blocks.
>
> Mike
>
>> -Original Message-
>> From: Zeng, Star
>> Sent: Monday, March 12, 2018 5:44 PM
>> To: Kinney, Michael D ; Ming
>> Huang ; Ni, Ruiyu
>> ; leif.lindh...@linaro.org; linaro-
>> u...@lists.linaro.org; edk2-devel@lists.01.org;
>> graeme.greg...@linaro.org; Dong, Eric
>> 
>> Cc: huangmin...@huawei.com; wanghuiqi...@huawei.com;
>> ard.biesheu...@linaro.org; zhangjinso...@huawei.com;
>> Gao, Liming ; guoh...@huawei.com;
>> wai...@126.com; mengfanr...@huawei.com;
>> huangda...@hisilicon.com; Zeng, Star
>> 
>> Subject: RE: [edk2] [RFC v1 1/1] MdeModulePkg/Usb: Use
>> Pcd for UsbBootIoBlocks
>>
>> Mike,
>>
>> There was some discussion at
>> https://lists.01.org/pipermail/edk2-devel/2017-
>> January/006707.html.
>> Seeming, there is no detailed information about why it
>> was changed to 128 according to the commit log.
>>
>>
>> Thanks,
>> Star
>>
>> -Original Message-
>> From: Kinney, Michael D
>> Sent: Monday, March 12, 2018 11:47 PM
>> To: Ming Huang ; Ni, Ruiyu
>> ; leif.lindh...@linaro.org; linaro-
>> u...@lists.linaro.org; edk2-devel@lists.01.org;
>> graeme.greg...@linaro.org; Zeng, Star
>> ; Dong, Eric ;
>> Kinney, Michael D 
>> Cc: huangmin...@huawei.com; wanghuiqi...@huawei.com;
>> ard.biesheu...@linaro.org; zhangjinso...@huawei.com;
>> Gao, Liming ; guoh...@huawei.com;
>> wai...@126.com; mengfanr...@huawei.com;
>> huangda...@hisilicon.com
>> Subject: RE: [edk2] [RFC v1 1/1] MdeModulePkg/Usb: Use
>> Pcd for UsbBootIoBlocks
>>
>> Does anyone know wow was the original #define of 128
>> selected?
>>
>> If we are seeing compatibility issues with 128 and we
>> have greater compatibility with 64, what would be the
>> impact of just changing the #define to 64?
>>
>> Thanks,
>>
>> Mike
>>
>>> -Original Message-
>>> From: Ming Huang [mailto:ming.hu...@linaro.org]
>>> Sent: Sunday, March 11, 2018 11:01 PM
>>> To: Ni, Ruiyu ;
>>> leif.lindh...@linaro.org; linaro-
>> u...@lists.linaro.org;
>>> edk2-devel@lists.01.org; graeme.greg...@linaro.org;
>> Zeng, Star
>>> ; Dong, Eric
>> 
>>> Cc: huangmin...@huawei.com; wanghuiqi...@huawei.com;
>>> ard.biesheu...@linaro.org; zhangjinso...@huawei.com;
>> Kinney, Michael D
>>> ; Gao, Liming
>> ;
>>> guoh...@huawei.com; wai...@126.com;
>> mengfanr...@huawei.com;
>>> huangda...@hisilicon.com
>>> Subject: Re: [edk2] [RFC v1 1/1] MdeModulePkg/Usb: Use
>> Pcd for
>>> UsbBootIoBlocks
>>>
>>> Any comments about this patch?
>>>
>>>
>>> On 2018/3/2 14:36, Ming Huang wrote:
 On 2018/2/27 18:01, Ni, Ruiyu wrote:
> On 2/27/2018 5:25 PM, Ming Huang wrote:
>> On 2018/2/27 13:25, Ni, Ruiyu wrote:
>>> I don't prefer to add PCD, unless we cannot find:
>>> 1. spec content  to describe the max/min blocks
>> There is no spec about the max/min blocks in my
>>> mind. I had checked this in
>> several pdf document like
>> Universal Serial Bus Mass Storage Class
>>> Specification Overview,
>> Universal Serial Bus Mass Storage Specification
>> For
>>> Bootability,
>> Universal Serial Bus Mass Storage Class Bulk-Only
>>> Transport.
>>> 2. error handling when the blocks number is
>> bigger
>>> than HW expects.
>> Where should error handling add to?  Error handing
>>> can't add to HW (end-point device),
>> because HW is not in our control scope.
> I mean maybe spec describes an error status could
>> be
>>> returned from HW when using 128. So that we can use
>> 64, 32, and
>>> smaller value until HW is happy.
> I am curious how the other USB storage drivers
>> handle
>>> this.
> PCD is a static way. Dynamic way is more preferred.
>
 When using 128,  after waiting
>>> 5x5(USB_BOOT_COMMAND_RETRY=5,
>>> USB_BOOT_GENERAL_CMD_TIMEOUT=5) seconds,
 the UsbBootReadBlocks ->UsbBootExecCmdWithRetry
>> retrun
>>> TimeOut. I don't know why HW return Timeout.
 Booting time is to long if using Dynamic way to fix
>>> the issue.
 When using 64, It works and booting from HW succeed.
 May be using PCD is a simple and effective mean.

 Thanks
 Ming
>>> Thanks/Ray
>>>
 -Original Message-
 From: edk2-devel [mailto:edk2-devel-
>>> 

Re: [edk2] [PATCH edk2-platforms v2 06/18] ARM/VExpressPkg: Add and update debug ASSERTS

[Leif Lindholm]

On Wed, Mar 14, 2018 at 12:35:03PM +, Ard Biesheuvel wrote:
> >> I guess Leif and I are in disagreement here. In particular, I think
> >> his comment
> >>
> >> """
> >> ASSERT (FALSE)?  (You already know Status is an EFI_ERROR, and a
> >> console message saying ASSERT (Status) is not getting you out of
> >> looking at the source code to find out what happened.)
> >> """
> >>
> >> is misguided, given that ASSERT_EFI_ERROR (Status) will actually print
> >> the value of Status to the debug console.
> >
> > I don't have a strong enough opinion on this that it should be
> > listened to if you both agree.
> >
> > It's just the "if error, then assert if error" that breaks up the
> > parsing flow for me. Could it make sense to use ASSERT_RETURN_ERROR
> > instead?
> >
> > So
> >   if (EFI_ERROR (Status)) {
> > ASSERT_RETURN_ERROR (Status);
> >   }
> >
> 
> Do you mean using ASSERT_RETURN_ERROR() rather than ASSERT_EFI_ERROR()?

Yes.

> That doesn't make sense: the former is for RETURN_STATUS type
> variables and the latter for EFI_STATUS

Right, of course.
Makes the text look nicer though :)

Anyway, I'm OK with the original version.
Just wish we had a less redundant-looking way to describe this.

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

Re: [edk2] [PATCH edk2-platforms v2 06/18] ARM/VExpressPkg: Add and update debug ASSERTS

[Ard Biesheuvel]

On 14 March 2018 at 12:24, Leif Lindholm  wrote:
> On Thu, Jan 04, 2018 at 07:24:20PM +, Ard Biesheuvel wrote:
>> On 4 January 2018 at 18:55, Girish Pathak  wrote:
>> > Hi Ard,
>> >
>> >> -Original Message-
>> >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>> >> Ard Biesheuvel
>> >> Sent: 23 December 2017 14:12
>> >> To: Evan Lloyd 
>> >> Cc: "matteo.carl...@arm.com"@arm.com;
>> >> "leif.lindh...@linaro.org"@arm.com; "n...@arm.com"@arm.com; edk2-
>> >> de...@lists.01.org; Thomas Abraham ; Arvind
>> >> Chauhan ;
>> >> "ard.biesheu...@linaro.org"@arm.com
>> >> Subject: Re: [edk2] [PATCH edk2-platforms v2 06/18] ARM/VExpressPkg: Add
>> >> and update debug ASSERTS
>> >>
>> >> On 22 December 2017 at 19:08,   wrote:
>> >> > From: Girish Pathak 
>> >> >
>> >> > This change adds some debug assertions e.g to catch NULL pointer
>> >> > errors missing in PL11Lcd and HdLcd platform libraries.
>> >> >
>> >> > Contributed-under: TianoCore Contribution Agreement 1.1
>> >> > Signed-off-by: Girish Pathak 
>> >> > Signed-off-by: Evan Lloyd 
>> >> > ---
>> >> >
>> >> Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpr
>> >> ess.c   | 22 +-
>> >> >
>> >> >
>> >> Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArm
>> >> VEx
>> >> > press.c | 24 +++-
>> >> >  2 files changed, 44 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git
>> >> >
>> >> a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx
>> >> pres
>> >> > s.c
>> >> >
>> >> b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx
>> >> pres
>> >> > s.c index
>> >> >
>> >> 6afd764897f49c64490ce891682f99bb0f5d993b..a8fe8696da0653017ce9fa6e4a
>> >> 86
>> >> > caf283bc04c9 100644
>> >> > ---
>> >> >
>> >> a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx
>> >> pres
>> >> > s.c
>> >> > +++
>> >> b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx
>> >> > +++ press.c
>> >> > @@ -153,6 +153,9 @@ LcdPlatformGetVram (
>> >> >EFI_STATUS  Status;
>> >> >EFI_ALLOCATE_TYPE   AllocationType;
>> >> >
>> >> > +  ASSERT (VramBaseAddress != NULL);
>> >> > +  ASSERT (VramSize != NULL);
>> >> > +
>> >> >// Set the vram size
>> >> >*VramSize = LCD_VRAM_SIZE;
>> >> >
>> >> > @@ -171,6 +174,7 @@ LcdPlatformGetVram (
>> >> >VramBaseAddress
>> >> >);
>> >> >if (EFI_ERROR (Status)) {
>> >> > +ASSERT (FALSE);
>> >> >  return Status;
>> >> >}
>> >> >
>> >> > @@ -181,8 +185,8 @@ LcdPlatformGetVram (
>> >> >*VramSize,
>> >> >EFI_MEMORY_WC
>> >> >);
>> >> > -  ASSERT_EFI_ERROR (Status);
>> >> >if (EFI_ERROR (Status)) {
>> >> > +ASSERT (FALSE);
>> >>
>> >> As in the sibling patch against EDK2, this patch makes it more difficult 
>> >> to
>> >> figure out what went wrong when you hit the ASSERT.
>> >> ASSERT_EFI_ERROR prints the value of Status, ASSERT(FALSE) only prints
>> >> '0 != 1'
>> >>
>> >
>> > This change(and other similar changes) is in response to review comments 
>> > on patch v1
>> > https://lists.01.org/pipermail/edk2-devel/2017-October/015995.html
>> >
>> > with above reference, Can you please confirm if we should revert to the 
>> > patch v1 version ?
>> >
>>
>> I guess Leif and I are in disagreement here. In particular, I think
>> his comment
>>
>> """
>> ASSERT (FALSE)?  (You already know Status is an EFI_ERROR, and a
>> console message saying ASSERT (Status) is not getting you out of
>> looking at the source code to find out what happened.)
>> """
>>
>> is misguided, given that ASSERT_EFI_ERROR (Status) will actually print
>> the value of Status to the debug console.
>
> I don't have a strong enough opinion on this that it should be
> listened to if you both agree.
>
> It's just the "if error, then assert if error" that breaks up the
> parsing flow for me. Could it make sense to use ASSERT_RETURN_ERROR
> instead?
>
> So
>   if (EFI_ERROR (Status)) {
> ASSERT_RETURN_ERROR (Status);
>   }
>

Do you mean using ASSERT_RETURN_ERROR() rather than ASSERT_EFI_ERROR()?

That doesn't make sense: the former is for RETURN_STATUS type
variables and the latter for EFI_STATUS

>> However, the objections against putting function calls in ASSERT()s
>> are justified: ASSERT() should not have side effects if its condition
>> is met, and function calls may have side effects.
>>
>> I suppose we should wait for Leif to return on the 22nd before
>> proceeding with the review.
>> Apologies for the confusion, and for the delay.
>
> Apologies for the even more ridicilous delay.
> Err, I need to stop taking holidays or something.
>
> Don't worry, I won't have another one for a couple of weeks :]
> 

Re: [edk2] [PATCH edk2-platforms v2 06/18] ARM/VExpressPkg: Add and update debug ASSERTS

[Leif Lindholm]

On Thu, Jan 04, 2018 at 07:24:20PM +, Ard Biesheuvel wrote:
> On 4 January 2018 at 18:55, Girish Pathak  wrote:
> > Hi Ard,
> >
> >> -Original Message-
> >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> >> Ard Biesheuvel
> >> Sent: 23 December 2017 14:12
> >> To: Evan Lloyd 
> >> Cc: "matteo.carl...@arm.com"@arm.com;
> >> "leif.lindh...@linaro.org"@arm.com; "n...@arm.com"@arm.com; edk2-
> >> de...@lists.01.org; Thomas Abraham ; Arvind
> >> Chauhan ;
> >> "ard.biesheu...@linaro.org"@arm.com
> >> Subject: Re: [edk2] [PATCH edk2-platforms v2 06/18] ARM/VExpressPkg: Add
> >> and update debug ASSERTS
> >>
> >> On 22 December 2017 at 19:08,   wrote:
> >> > From: Girish Pathak 
> >> >
> >> > This change adds some debug assertions e.g to catch NULL pointer
> >> > errors missing in PL11Lcd and HdLcd platform libraries.
> >> >
> >> > Contributed-under: TianoCore Contribution Agreement 1.1
> >> > Signed-off-by: Girish Pathak 
> >> > Signed-off-by: Evan Lloyd 
> >> > ---
> >> >
> >> Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpr
> >> ess.c   | 22 +-
> >> >
> >> >
> >> Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArm
> >> VEx
> >> > press.c | 24 +++-
> >> >  2 files changed, 44 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git
> >> >
> >> a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx
> >> pres
> >> > s.c
> >> >
> >> b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx
> >> pres
> >> > s.c index
> >> >
> >> 6afd764897f49c64490ce891682f99bb0f5d993b..a8fe8696da0653017ce9fa6e4a
> >> 86
> >> > caf283bc04c9 100644
> >> > ---
> >> >
> >> a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx
> >> pres
> >> > s.c
> >> > +++
> >> b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx
> >> > +++ press.c
> >> > @@ -153,6 +153,9 @@ LcdPlatformGetVram (
> >> >EFI_STATUS  Status;
> >> >EFI_ALLOCATE_TYPE   AllocationType;
> >> >
> >> > +  ASSERT (VramBaseAddress != NULL);
> >> > +  ASSERT (VramSize != NULL);
> >> > +
> >> >// Set the vram size
> >> >*VramSize = LCD_VRAM_SIZE;
> >> >
> >> > @@ -171,6 +174,7 @@ LcdPlatformGetVram (
> >> >VramBaseAddress
> >> >);
> >> >if (EFI_ERROR (Status)) {
> >> > +ASSERT (FALSE);
> >> >  return Status;
> >> >}
> >> >
> >> > @@ -181,8 +185,8 @@ LcdPlatformGetVram (
> >> >*VramSize,
> >> >EFI_MEMORY_WC
> >> >);
> >> > -  ASSERT_EFI_ERROR (Status);
> >> >if (EFI_ERROR (Status)) {
> >> > +ASSERT (FALSE);
> >>
> >> As in the sibling patch against EDK2, this patch makes it more difficult to
> >> figure out what went wrong when you hit the ASSERT.
> >> ASSERT_EFI_ERROR prints the value of Status, ASSERT(FALSE) only prints
> >> '0 != 1'
> >>
> >
> > This change(and other similar changes) is in response to review comments on 
> > patch v1
> > https://lists.01.org/pipermail/edk2-devel/2017-October/015995.html
> >
> > with above reference, Can you please confirm if we should revert to the 
> > patch v1 version ?
> >
> 
> I guess Leif and I are in disagreement here. In particular, I think
> his comment
> 
> """
> ASSERT (FALSE)?  (You already know Status is an EFI_ERROR, and a
> console message saying ASSERT (Status) is not getting you out of
> looking at the source code to find out what happened.)
> """
> 
> is misguided, given that ASSERT_EFI_ERROR (Status) will actually print
> the value of Status to the debug console.

I don't have a strong enough opinion on this that it should be
listened to if you both agree.

It's just the "if error, then assert if error" that breaks up the
parsing flow for me. Could it make sense to use ASSERT_RETURN_ERROR
instead?

So
  if (EFI_ERROR (Status)) {
ASSERT_RETURN_ERROR (Status);
  }

> However, the objections against putting function calls in ASSERT()s
> are justified: ASSERT() should not have side effects if its condition
> is met, and function calls may have side effects.
> 
> I suppose we should wait for Leif to return on the 22nd before
> proceeding with the review.
> Apologies for the confusion, and for the delay.

Apologies for the even more ridicilous delay.
Err, I need to stop taking holidays or something.

Don't worry, I won't have another one for a couple of weeks :]
First Linaro Connect, then Plugfest.

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

Re: [edk2] [PATCH V2 0/6] SourceLevelDebugPkg DebugUsb3: Re-Support IOMMU

[Wu, Hao A]

The series is good to me.
Reviewed-by: Hao Wu 


Best Regards,
Hao Wu


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Star
> Zeng
> Sent: Wednesday, March 14, 2018 5:34 PM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu; Wu, Hao A; Zeng, Star
> Subject: [edk2] [PATCH V2 0/6] SourceLevelDebugPkg DebugUsb3: Re-Support
> IOMMU
> 
> The patch series is also at
> https://github.com/lzeng14/edk2 DebugCommUsb3AfterIOMMUV2 branch.
> 
> Based on the feedbacks from Ray and Hao.
> It is V2 of
> https://lists.01.org/pipermail/edk2-devel/2018-March/022586.html.
> It has no essential difference with V1 about the final code, but
> re-arranges the patches to revert old IOMMU support patches and
> then re-support IOMMU.
> 
> de8373fa07f87ca735139bb86c51e2c29fb1d956 could not handle two cases.
> 1. For the case that the USB3 debug port instance and DMA buffers are
> from PEI HOB with IOMMU enabled, it was to reallocate the DMA buffers
> by AllocateAddress with the memory type accessible by SMM environment.
> But reallocating the DMA buffers by AllocateAddress may fail.
> 
> 2. At S3 resume, after the code is transferred to PiSmmCpuDxeSmm from
> S3Resume2Pei, HOB is still needed to be used for DMA operation, but
> PiSmmCpuDxeSmm has no way to get the HOB at S3 resume.
> 
> The patch is to re-support IOMMU.
> For PEI, allocate granted DMA buffer from IOMMU PPI, register IOMMU PPI
> notification to reinitialize hardware with granted DMA buffer if IOMMU
> PPI is not present yet.
> For DXE, map DMA buffer by PciIo in PciIo notification for early DXE,
> and register DxeSmmReadyToLock notification to reinitialize hardware
> with granted DXE DMA buffer accessible by SMM environment for late DXE.
> 
> DebugAgentLib has been managing the instance as Handle in
> HOB/SystemTable. The Handle(instance) from DebugAgentLib can be used
> directly in DebugCommunicationLibUsb3. Then DebugCommunicationLibUsb3
> could get consistent Handle(instance) from DebugAgentLib.
> 
> Cc: Ruiyu Ni 
> Cc: Hao Wu 
> 
> Star Zeng (6):
>   Revert "DebugUsb3: Check mUsb3Instance before dereferencing it"
>   Revert "DebugUsb3: Fix GCC build failures"
>   Revert "DebugUsb3: Support IOMMU"
>   SourceLevelDebugPkg DebugUsb3: Re-Fix GCC build failures
>   SourceLevelDebugPkg DebugCommUsb3: Refine some formats/comments
>   SourceLevelDebugPkg DebugUsb3: Re-Support IOMMU
> 
>  .../DebugCommunicationLibUsb3Common.c  | 110 +-
>  .../DebugCommunicationLibUsb3Dxe.c | 375 
> -
>  .../DebugCommunicationLibUsb3Dxe.inf   |  11 +-
>  .../DebugCommunicationLibUsb3Internal.h|  60 +---
>  .../DebugCommunicationLibUsb3Pei.c |  50 ++-
>  .../DebugCommunicationLibUsb3Pei.inf   |   4 +-
>  6 files changed, 351 insertions(+), 259 deletions(-)
> 
> --
> 2.7.0.windows.1
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v5 4/6] MdeModulePkg/PciHostBridgeDxe: Add support for address translation

[Ard Biesheuvel]

On 14 March 2018 at 07:57, Ni, Ruiyu  wrote:
> On 3/7/2018 2:01 PM, Guo Heyi wrote:
>>
>> Thanks. Please let me know if any further changes are needed.
>>
>> Regards,
>>
>> Heyi
>>
>> On Wed, Mar 07, 2018 at 12:30:59PM +0800, Ni, Ruiyu wrote:
>>>
>>> On 3/6/2018 10:44 AM, Guo Heyi wrote:

 Hi Ray,

 Any comments for v5?
>>>
>>>
>>> Heyi,
>>> Some backward compatibility concerns were received from internal
>>> production
>>> teams. Current change will cause silent failure on old platforms because
>>> TranslationOffset might be random if uninitialized.
>>> I will solve the concern and then send out updates to you, hopefully by
>>> end
>>> of next week.
>>>

 Regards,

 Heyi

 On Thu, Mar 01, 2018 at 02:57:22PM +0800, Heyi Guo wrote:
>
> PCI address translation is necessary for some non-x86 platforms. On
> such platforms, address value (denoted as "device address" or "address
> in PCI view") set to PCI BAR registers in configuration space might be
> different from the address which is used by CPU to access the
> registers in memory BAR or IO BAR spaces (denoted as "host address" or
> "address in CPU view"). The difference between the two addresses is
> called "Address Translation Offset" or simply "translation", and can
> be represented by "Address Translation Offset" in ACPI QWORD Address
> Space Descriptor (Offset 0x1E). However UEFI and ACPI differs on the
> definitions of QWORD Address Space Descriptor, and we will follow UEFI
> definition on UEFI protocols, such as PCI root bridge IO protocol and
> PCI IO protocol. In UEFI 2.7, "Address Translation Offset" is "Offset
> to apply to the Starting address to convert it to a PCI address". This
> means:
>
> 1. Translation = device address - host address.
>
> 2. PciRootBridgeIo->Configuration should return CPU view address, as
> well as PciIo->GetBarAttributes.
>
> Summary of addresses used in protocol interfaces and internal
> implementations:
>
> 1. *Only* the following protocol interfaces assume Address is Device
> Address:
> (1). PciHostBridgeResourceAllocation.GetProposedResources()
>   Otherwise PCI bus driver cannot set correct address into PCI
>   BARs.
> (2). PciRootBridgeIo.Mem.Read() and PciRootBridgeIo.Mem.Write()
> (3). PciRootBridgeIo.CopyMem()
> UEFI and PI spec have clear statements for all other protocol
> interfaces about the address type.
>
> 2. Library interfaces and internal implementation:
> (1). Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address.
>   It is easy to check whether the address is below 4G or above 4G.
> (2). Addresses in PCI_ROOT_BRIDGE_INSTANCE.ResAllocNode are host
>   address, for they are allocated from GCD.
> (3). Address passed to PciHostBridgeResourceConflict is host address,
>   for it comes from PCI_ROOT_BRIDGE_INSTANCE.ResAllocNode.
>
> RESTRICTION: to simplify the situation, we require the alignment of
> Translation must be larger than any BAR alignment in the same root
> bridge, so that resource allocation alignment can be applied to both
> device address and host address.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Heyi Guo 
> Cc: Ruiyu Ni 
> Cc: Ard Biesheuvel 
> Cc: Star Zeng 
> Cc: Eric Dong 
> Cc: Laszlo Ersek 
> Cc: Michael D Kinney 
> ---
>
> Notes:
>  v5:
>  - Add check for the alignment of Translation against the BAR
> alignment
>[Ray]
>  - Keep coding style of comments consistent with the context [Ray]
>  - Comment change for Base in PCI_RES_NODE [Ray]
>  - Add macros of TO_HOST_ADDRESS and TO_DEVICE_ADDRESS for address
> type
>conversion (After that we can also simply the comments about the
>calculation [Ray]
>  - Add check for bus translation offset in CreateRootBridge(),
> making
>sure it is zero, and unify code logic for all types of resource
>after that [Ray]
>  - Use GetTranslationByResourceType() to simplify the code in
>RootBridgeIoConfiguration() (also fix a bug in previous patch
>version of missing a break after case TypePMem64) [Ray]
>  - Commit message refinement [Ray]
>  v4:
>  - Add ASSERT (FALSE) to default branch in
> GetTranslationByResourceType
>[Laszlo]
>  - Fix bug when passing BaseAddress to gDS->AllocateIoSpace and
>gDS->AllocateMemorySpace [Laszlo]
>  - Add comment for applying alignment to both device address and
> host
>address, and add 

Re: [edk2] [PATCH] edksetup.sh: Update help section regarding positional

[Leif Lindholm]

Hi Arvind,

On Sat, Mar 10, 2018 at 11:11:58AM -0500, Arvind Prasanna wrote:
> It is possible to source edksetup.sh from another script. If the
> calling/sourcing script has any positional parameters set, those are
> incorrectly accounted for in edksetup.sh while sourcing it resulting in
> the the help section always being shown. This patch updates the help
> section advising the user about these set positional parameters so they
> can be unset prior to sourcing edksetup.sh.

This is really just one of the unpleasantries of sourcing shell
scripts.

Since the current script could only ever work with bash anyway (and
not sh, dash, ...) I don't know that we could do much better - and
there's nothing about the problem that is specific to this script.

As an aside, since we _know_ this only works on bash, you can also
make use of the bash side effect that if you pass any arguments to the
sourced script, it gets its own copies of $#, $0, $1 and so on.

For ancient backwards compatility, the parameter BaseTools is ignored
if provided on the command line. So you could always just use
  edksetup.sh BaseTools

Alternatively, you can use
  edksetup.sh --reconfig
which also ensures you are always using the latest toolchain
templates.

Regards,

Leif

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Arvind Prasanna 
> ---
>  edksetup.sh | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/edksetup.sh b/edksetup.sh
> index 93d6525..a3d5560 100755
> --- a/edksetup.sh
> +++ b/edksetup.sh
> @@ -42,6 +42,8 @@ function HelpMsg()
>echo Please note: This script must be \'sourced\' so the environment can 
> be changed.
>echo ". $SCRIPTNAME"
>echo "source $SCRIPTNAME"
> +  echo "If this script is being sourced from another script, please ensure 
> that the"
> +  echo "sourcing/calling script has no set postional parameters."
>  }
>  
>  function SetWorkspace()
> -- 
> 2.7.4
> 
> ___
> 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 v5 1/6] CorebootPayloadPkg/PciHostBridgeLib: Init PCI aperture to 0

[Ard Biesheuvel]

On 1 March 2018 at 06:57, Heyi Guo  wrote:
> Use ZeroMem to initialize all fields in temporary
> PCI_ROOT_BRIDGE_APERTURE variables to zero. This is not mandatory but
> helpful for future extension: when we add new fields to
> PCI_ROOT_BRIDGE_APERTURE and the default value of these fields can
> safely be zero, this code will not suffer from an additional change.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Heyi Guo 
> Cc: Maurice Ma 
> Cc: Prince Agyeman 
> Cc: Benjamin You 
> Cc: Ruiyu Ni 
> Cc: Laszlo Ersek 
> Cc: Ard Biesheuvel 

Reviewed-by: Ard Biesheuvel 

Maurice, Prince, Benjamin: any comments?

> ---
>  CorebootPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c | 5 +
>  1 file changed, 5 insertions(+)
>
> diff --git 
> a/CorebootPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c 
> b/CorebootPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c
> index 6d94ff72c956..c61609b79cce 100644
> --- a/CorebootPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c
> +++ b/CorebootPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c
> @@ -319,6 +319,11 @@ ScanForRootBridges (
>
>*NumberOfRootBridges = 0;
>RootBridges = NULL;
> +  ZeroMem (, sizeof (Io));
> +  ZeroMem (, sizeof (Mem));
> +  ZeroMem (, sizeof (MemAbove4G));
> +  ZeroMem (, sizeof (PMem));
> +  ZeroMem (, sizeof (PMemAbove4G));
>
>//
>// After scanning all the PCI devices on the PCI root bridge's primary bus,
> --
> 2.7.4
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH] BaseTools/PosixLike: honor pre-set PYTHONPATH

[Laszlo Ersek]

Utilities written in Python may depend on external (preinstalled) Python
packages; for example, Ecc depends on "antlr_python_runtime-3.0.1". Such
packages need not be installed system-wide, as long as they are reachable
through PYTHONPATH. Therefore we shouldn't overwrite the user's PYTHONPATH
with "BaseTools/Source/Python"; instead, we should prepend the latter to
the former.

Cc: Liming Gao 
Cc: Yonghong Zhu 
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=896
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek 
---

Notes:
Repo:   https://github.com/lersek/edk2.git
Branch: pythonpath

 BaseTools/BinWrappers/PosixLike/BPDG  | 2 +-
 BaseTools/BinWrappers/PosixLike/Ecc   | 2 +-
 BaseTools/BinWrappers/PosixLike/GenDepex  | 2 +-
 BaseTools/BinWrappers/PosixLike/GenFds| 2 +-
 BaseTools/BinWrappers/PosixLike/GenPatchPcdTable  | 2 +-
 BaseTools/BinWrappers/PosixLike/PatchPcdValue | 2 +-
 BaseTools/BinWrappers/PosixLike/Pkcs7Sign | 2 +-
 BaseTools/BinWrappers/PosixLike/Rsa2048Sha256GenerateKeys | 2 +-
 BaseTools/BinWrappers/PosixLike/Rsa2048Sha256Sign | 2 +-
 BaseTools/BinWrappers/PosixLike/TargetTool| 2 +-
 BaseTools/BinWrappers/PosixLike/Trim  | 2 +-
 BaseTools/BinWrappers/PosixLike/UPT   | 2 +-
 BaseTools/BinWrappers/PosixLike/build | 2 +-
 13 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/BaseTools/BinWrappers/PosixLike/BPDG 
b/BaseTools/BinWrappers/PosixLike/BPDG
index 214d88fff1b1..01ae23ddeb4f 100755
--- a/BaseTools/BinWrappers/PosixLike/BPDG
+++ b/BaseTools/BinWrappers/PosixLike/BPDG
@@ -10,5 +10,5 @@ full_cmd=${BASH_SOURCE:-$0} # see 
http://mywiki.wooledge.org/BashFAQ/028 for a d
 dir=$(dirname "$full_cmd")
 cmd=${full_cmd##*/}
 
-export PYTHONPATH="$dir/../../Source/Python"
+export PYTHONPATH="$dir/../../Source/Python${PYTHONPATH:+:"$PYTHONPATH"}"
 exec "${python_exe:-python}" "$dir/../../Source/Python/$cmd/$cmd.py" "$@"
diff --git a/BaseTools/BinWrappers/PosixLike/Ecc 
b/BaseTools/BinWrappers/PosixLike/Ecc
index 214d88fff1b1..01ae23ddeb4f 100755
--- a/BaseTools/BinWrappers/PosixLike/Ecc
+++ b/BaseTools/BinWrappers/PosixLike/Ecc
@@ -10,5 +10,5 @@ full_cmd=${BASH_SOURCE:-$0} # see 
http://mywiki.wooledge.org/BashFAQ/028 for a d
 dir=$(dirname "$full_cmd")
 cmd=${full_cmd##*/}
 
-export PYTHONPATH="$dir/../../Source/Python"
+export PYTHONPATH="$dir/../../Source/Python${PYTHONPATH:+:"$PYTHONPATH"}"
 exec "${python_exe:-python}" "$dir/../../Source/Python/$cmd/$cmd.py" "$@"
diff --git a/BaseTools/BinWrappers/PosixLike/GenDepex 
b/BaseTools/BinWrappers/PosixLike/GenDepex
index bdb6722a1f84..dad174788bc3 100755
--- a/BaseTools/BinWrappers/PosixLike/GenDepex
+++ b/BaseTools/BinWrappers/PosixLike/GenDepex
@@ -10,5 +10,5 @@ full_cmd=${BASH_SOURCE:-$0} # see 
http://mywiki.wooledge.org/BashFAQ/028 for a d
 dir=$(dirname "$full_cmd")
 cmd=${full_cmd##*/}
 
-export PYTHONPATH="$dir/../../Source/Python"
+export PYTHONPATH="$dir/../../Source/Python${PYTHONPATH:+:"$PYTHONPATH"}"
 exec "${python_exe:-python}" "$dir/../../Source/Python/AutoGen/$cmd.py" "$@"
diff --git a/BaseTools/BinWrappers/PosixLike/GenFds 
b/BaseTools/BinWrappers/PosixLike/GenFds
index 214d88fff1b1..01ae23ddeb4f 100755
--- a/BaseTools/BinWrappers/PosixLike/GenFds
+++ b/BaseTools/BinWrappers/PosixLike/GenFds
@@ -10,5 +10,5 @@ full_cmd=${BASH_SOURCE:-$0} # see 
http://mywiki.wooledge.org/BashFAQ/028 for a d
 dir=$(dirname "$full_cmd")
 cmd=${full_cmd##*/}
 
-export PYTHONPATH="$dir/../../Source/Python"
+export PYTHONPATH="$dir/../../Source/Python${PYTHONPATH:+:"$PYTHONPATH"}"
 exec "${python_exe:-python}" "$dir/../../Source/Python/$cmd/$cmd.py" "$@"
diff --git a/BaseTools/BinWrappers/PosixLike/GenPatchPcdTable 
b/BaseTools/BinWrappers/PosixLike/GenPatchPcdTable
index 214d88fff1b1..01ae23ddeb4f 100755
--- a/BaseTools/BinWrappers/PosixLike/GenPatchPcdTable
+++ b/BaseTools/BinWrappers/PosixLike/GenPatchPcdTable
@@ -10,5 +10,5 @@ full_cmd=${BASH_SOURCE:-$0} # see 
http://mywiki.wooledge.org/BashFAQ/028 for a d
 dir=$(dirname "$full_cmd")
 cmd=${full_cmd##*/}
 
-export PYTHONPATH="$dir/../../Source/Python"
+export PYTHONPATH="$dir/../../Source/Python${PYTHONPATH:+:"$PYTHONPATH"}"
 exec "${python_exe:-python}" "$dir/../../Source/Python/$cmd/$cmd.py" "$@"
diff --git a/BaseTools/BinWrappers/PosixLike/PatchPcdValue 
b/BaseTools/BinWrappers/PosixLike/PatchPcdValue
index 214d88fff1b1..01ae23ddeb4f 100755
--- a/BaseTools/BinWrappers/PosixLike/PatchPcdValue
+++ b/BaseTools/BinWrappers/PosixLike/PatchPcdValue
@@ -10,5 +10,5 @@ full_cmd=${BASH_SOURCE:-$0} # see 
http://mywiki.wooledge.org/BashFAQ/028 for a d
 dir=$(dirname "$full_cmd")
 cmd=${full_cmd##*/}
 
-export PYTHONPATH="$dir/../../Source/Python"
+export 

Re: [edk2] [PATCH 0/6] OvmfPkg, ArmVirtQemu: leaner platform BDS policy for connecting devices

[Laszlo Ersek]

On 03/13/18 22:22, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: qemu_bootorder_connect
> 
> Adding tens or hundreds of bootable devices to a QEMU VM config slows
> the OVMF and ArmVirtQemu boots to a crawl, several people have reported
> in the past.
> 
> There are at least two reasons for this (high pflash traffic due to
> heavy nvvar massaging per device, and PCI config space access slowing
> down on QEMU as the number of regions increases). However, part of the
> pain is self-inflicted in our PlatformBootManagerLib instances: we
> connect all bootable devices (for maximum compatibility with the user's
> VM config) even if the user doesn't intend to boot off most of them.
> 
> It's oft repeated that the set of devices connected during boot is
> platform policy, so this series replaces the culprit
> EfiBootManagerConnectAll() calls with a bit smarter algorithm.

Thanks everyone for the super quick feedback!

Commit range 12957e56d26d..ff1d0fbfbaec.

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

Re: [edk2] [PATCH V2 0/6] SourceLevelDebugPkg DebugUsb3: Re-Support IOMMU

[Ni, Ruiyu]

On 3/14/2018 5:33 PM, Star Zeng wrote:

The patch series is also at
https://github.com/lzeng14/edk2 DebugCommUsb3AfterIOMMUV2 branch.

Based on the feedbacks from Ray and Hao.
It is V2 of
https://lists.01.org/pipermail/edk2-devel/2018-March/022586.html.
It has no essential difference with V1 about the final code, but
re-arranges the patches to revert old IOMMU support patches and
then re-support IOMMU.

de8373fa07f87ca735139bb86c51e2c29fb1d956 could not handle two cases.
1. For the case that the USB3 debug port instance and DMA buffers are
from PEI HOB with IOMMU enabled, it was to reallocate the DMA buffers
by AllocateAddress with the memory type accessible by SMM environment.
But reallocating the DMA buffers by AllocateAddress may fail.

2. At S3 resume, after the code is transferred to PiSmmCpuDxeSmm from
S3Resume2Pei, HOB is still needed to be used for DMA operation, but
PiSmmCpuDxeSmm has no way to get the HOB at S3 resume.

The patch is to re-support IOMMU.
For PEI, allocate granted DMA buffer from IOMMU PPI, register IOMMU PPI
notification to reinitialize hardware with granted DMA buffer if IOMMU
PPI is not present yet.
For DXE, map DMA buffer by PciIo in PciIo notification for early DXE,
and register DxeSmmReadyToLock notification to reinitialize hardware
with granted DXE DMA buffer accessible by SMM environment for late DXE.

DebugAgentLib has been managing the instance as Handle in
HOB/SystemTable. The Handle(instance) from DebugAgentLib can be used
directly in DebugCommunicationLibUsb3. Then DebugCommunicationLibUsb3
could get consistent Handle(instance) from DebugAgentLib.

Cc: Ruiyu Ni 
Cc: Hao Wu 

Star Zeng (6):
   Revert "DebugUsb3: Check mUsb3Instance before dereferencing it"
   Revert "DebugUsb3: Fix GCC build failures"
   Revert "DebugUsb3: Support IOMMU"
   SourceLevelDebugPkg DebugUsb3: Re-Fix GCC build failures
   SourceLevelDebugPkg DebugCommUsb3: Refine some formats/comments
   SourceLevelDebugPkg DebugUsb3: Re-Support IOMMU

  .../DebugCommunicationLibUsb3Common.c  | 110 +-
  .../DebugCommunicationLibUsb3Dxe.c | 375 -
  .../DebugCommunicationLibUsb3Dxe.inf   |  11 +-
  .../DebugCommunicationLibUsb3Internal.h|  60 +---
  .../DebugCommunicationLibUsb3Pei.c |  50 ++-
  .../DebugCommunicationLibUsb3Pei.inf   |   4 +-
  6 files changed, 351 insertions(+), 259 deletions(-)


Reviewed-by: Ruiyu Ni 

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

[edk2] [PATCH V2 6/6] SourceLevelDebugPkg DebugUsb3: Re-Support IOMMU

[Star Zeng]

de8373fa07f87ca735139bb86c51e2c29fb1d956 could not handle two cases.
1. For the case that the USB3 debug port instance and DMA buffers are
from PEI HOB with IOMMU enabled, it was to reallocate the DMA buffers
by AllocateAddress with the memory type accessible by SMM environment.
But reallocating the DMA buffers by AllocateAddress may fail.

2. At S3 resume, after the code is transferred to PiSmmCpuDxeSmm from
S3Resume2Pei, HOB is still needed to be used for DMA operation, but
PiSmmCpuDxeSmm has no way to get the HOB at S3 resume.

The patch is to re-support IOMMU.
For PEI, allocate granted DMA buffer from IOMMU PPI, register IOMMU PPI
notification to reinitialize hardware with granted DMA buffer if IOMMU
PPI is not present yet.
For DXE, map DMA buffer by PciIo in PciIo notification for early DXE,
and register DxeSmmReadyToLock notification to reinitialize hardware
with granted DXE DMA buffer accessible by SMM environment for late DXE.

DebugAgentLib has been managing the instance as Handle in
HOB/SystemTable. The Handle(instance) from DebugAgentLib can be used
directly in DebugCommunicationLibUsb3. Then DebugCommunicationLibUsb3
could get consistent Handle(instance) from DebugAgentLib.

Cc: Ruiyu Ni 
Cc: Hao Wu 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 .../DebugCommunicationLibUsb3Common.c  | 256 +++
 .../DebugCommunicationLibUsb3Dxe.c | 500 -
 .../DebugCommunicationLibUsb3Dxe.inf   |  17 +-
 .../DebugCommunicationLibUsb3Internal.h|  53 ++-
 .../DebugCommunicationLibUsb3Pei.c | 232 +-
 .../DebugCommunicationLibUsb3Pei.inf   |   8 +-
 6 files changed, 954 insertions(+), 112 deletions(-)

diff --git 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
index 37f49ad822f5..fb9ca84fc7bc 100644
--- 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
+++ 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
@@ -14,11 +14,6 @@
 
 #include "DebugCommunicationLibUsb3Internal.h"
 
-//
-// The global variable which can be used after memory is ready.
-//
-USB3_DEBUG_PORT_HANDLE mDebugCommunicationLibUsb3DebugPortHandle;
-
 UINT16   mString0Desc[] = {
   //  String Descriptor Type + Length
   ( USB_DESC_TYPE_STRING << 8 ) + STRING0_DESC_LEN,
@@ -151,6 +146,28 @@ XhcSetDebugRegBit (
 }
 
 /**
+  Clear one bit of the debug register while keeping other bits.
+
+  @param  Handle   Debug port handle.
+  @param  Offset   The offset of the debug register.
+  @param  Bit  The bit mask of the register to clear.
+
+**/
+VOID
+XhcClearDebugRegBit (
+  IN USB3_DEBUG_PORT_HANDLE   *Handle,
+  IN UINT32   Offset,
+  IN UINT32   Bit
+  )
+{
+  UINT32  Data;
+
+  Data  = XhcReadDebugReg (Handle, Offset);
+  Data  &= ~Bit;
+  XhcWriteDebugReg (Handle, Offset, Data);
+}
+
+/**
   Program and eanble XHCI MMIO base address.
 
   @return XHCI MMIO base address.
@@ -199,7 +216,7 @@ UpdateXhcResource (
   IN EFI_PHYSICAL_ADDRESS   XhciMmioBase
   )
 {
-  if ((Handle == NULL) || (Handle->XhciMmioBase == XhciMmioBase)) {
+  if (Handle == NULL) {
 return;
   }
 
@@ -236,6 +253,14 @@ CalculateUsbDebugPortMmioBase (
   EFI_PHYSICAL_ADDRESSCapabilityPointer;
   UINT8   CapLength;
 
+  if (Handle->Initialized != USB3DBG_UNINITIALIZED) {
+if (Handle->Initialized == USB3DBG_NO_DBG_CAB) {
+  return RETURN_UNSUPPORTED;
+} else {
+  return RETURN_SUCCESS;
+}
+  }
+
   VendorId = PciRead16 (PcdGet32(PcdUsbXhciPciAddress) + PCI_VENDOR_ID_OFFSET);
   DeviceId = PciRead16 (PcdGet32(PcdUsbXhciPciAddress) + PCI_DEVICE_ID_OFFSET);
   
@@ -288,6 +313,7 @@ CalculateUsbDebugPortMmioBase (
   Handle->DebugCapabilityBase   = CapabilityPointer;
   Handle->DebugCapabilityOffset = CapabilityPointer - Handle->XhciMmioBase;
   Handle->XhciOpRegister= Handle->XhciMmioBase + CapLength;
+  Handle->DebugSupport = TRUE;
   Handle->Initialized = USB3DBG_DBG_CAB;
   return RETURN_SUCCESS;
 
@@ -326,6 +352,9 @@ NeedReinitializeHardware(
   Dcctrl = XhcReadDebugReg (Handle, XHC_DC_DCCTRL);
   if ((Dcctrl & BIT0) == 0) {
 Result = TRUE;
+  } else if (!Handle->Ready) {
+Handle->Ready = TRUE;
+Handle->Initialized = USB3DBG_ENABLED;
   }
 
   return Result;
@@ -678,6 +707,13 @@ InitializeUsbDebugHardware (
   }
 
   //
+  // Clear DCE bit and LSE bit in DCCTRL
+  //
+  if ((XhcReadDebugReg (Handle, XHC_DC_DCCTRL) & (BIT1|BIT31)) == 
(BIT1|BIT31)) {
+XhcClearDebugRegBit (Handle, XHC_DC_DCCTRL, BIT1|BIT31);
+  }
+
+  //
   // Construct the buffer for read, poll and write.
   //
   Handle->UrbIn.Data 

[edk2] [PATCH V2 5/6] SourceLevelDebugPkg DebugCommUsb3: Refine some formats/comments

[Star Zeng]

Refine some formats/comments and remove some unused prototypes.

Cc: Ruiyu Ni 
Cc: Hao Wu 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 .../DebugCommunicationLibUsb3Common.c  | 16 
 .../DebugCommunicationLibUsb3Internal.h| 47 +++---
 2 files changed, 13 insertions(+), 50 deletions(-)

diff --git 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
index 3b0f81cf88fd..37f49ad822f5 100644
--- 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
+++ 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
@@ -85,15 +85,15 @@ XhcClearR32Bit(
   Write the data to the XHCI debug register.
 
   @param  Handle   Debug port handle.
-  @param  Offset   The offset of the runtime register.
+  @param  Offset   The offset of the debug register.
   @param  Data The data to write.
 
 **/
 VOID
 XhcWriteDebugReg (
   IN USB3_DEBUG_PORT_HANDLE  *Handle,
-  IN UINT32   Offset,
-  IN UINT32   Data
+  IN UINT32  Offset,
+  IN UINT32  Data
   )
 {
   EFI_PHYSICAL_ADDRESS  DebugCapabilityBase;
@@ -116,7 +116,7 @@ XhcWriteDebugReg (
 UINT32
 XhcReadDebugReg (
   IN  USB3_DEBUG_PORT_HANDLE *Handle,
-  IN  UINT32   Offset
+  IN  UINT32 Offset
   )
 {
   UINT32  Data;
@@ -129,16 +129,16 @@ XhcReadDebugReg (
 }
 
 /**
-  Set one bit of the runtime register while keeping other bits.
+  Set one bit of the debug register while keeping other bits.
 
   @param  Handle   Debug port handle.
-  @param  Offset   The offset of the runtime register.
+  @param  Offset   The offset of the debug register.
   @param  Bit  The bit mask of the register to set.
 
 **/
 VOID
 XhcSetDebugRegBit (
-  IN USB3_DEBUG_PORT_HANDLE *Handle,
+  IN USB3_DEBUG_PORT_HANDLE   *Handle,
   IN UINT32   Offset,
   IN UINT32   Bit
   )
@@ -216,7 +216,7 @@ UpdateXhcResource (
 
   @param  Handle Debug port handle.
 
-  @retval RETURN_UNSUPPORTED The usb host controller does not supported usb 
debug port capability.
+  @retval RETURN_UNSUPPORTED The usb host controller does not support usb 
debug port capability.
   @retval RETURN_SUCCESS Get bar and offset successfully.
 
 **/
diff --git 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Internal.h
 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Internal.h
index 356485c5f697..9da49f964401 100644
--- 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Internal.h
+++ 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Internal.h
@@ -1,7 +1,7 @@
 /** @file
   Debug Port Library implementation based on usb3 debug port.
 
-  Copyright (c) 2014 - 2015, Intel Corporation. All rights reserved.
+  Copyright (c) 2014 - 2018, 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
@@ -540,7 +540,7 @@ typedef struct _USB3_DEBUG_PORT_INSTANCE {
 UINT32
 XhcReadDebugReg (
   IN  USB3_DEBUG_PORT_HANDLE*Handle,
-  IN  UINT32  Offset
+  IN  UINT32Offset
   );
 
 /**
@@ -554,8 +554,8 @@ XhcReadDebugReg (
 VOID
 XhcSetDebugRegBit (
   IN USB3_DEBUG_PORT_HANDLE  *Handle,
-  IN UINT32   Offset,
-  IN UINT32   Bit
+  IN UINT32  Offset,
+  IN UINT32  Bit
   );
   
 /**
@@ -574,43 +574,6 @@ XhcWriteDebugReg (
   );
 
 /**
-  Discover the USB3 debug device.
-  
-  @param  HandleDebug port handle.
-  
-  @retval RETURN_SUCCESSThe serial device was initialized.
-  @retval RETURN_DEVICE_ERROR   The serial device could not be initialized.
-
-**/
-RETURN_STATUS
-DiscoverUsb3DebugPort(
-  USB3_DEBUG_PORT_HANDLE  *Handle
-  );
-  
-/**
-  Initialize the Serial Device hardware.
-  
-  @param  HandleDebug port handle.
-
-  @retval RETURN_SUCCESSThe serial device was initialized successfully.
-  @retval !RETURN_SUCCESS   Error.
-
-**/
-RETURN_STATUS
-InitializeUsb3DebugPort (
-  USB3_DEBUG_PORT_HANDLE  *Handle
-  );
-
-/**
-  Return XHCI MMIO base address.
-
-**/
-EFI_PHYSICAL_ADDRESS
-GetXhciBaseAddress (
-  VOID
-  );
-
-/**
   Verifies if the bit positions specified by a mask are set in a register.
 
   @param[in, out] RegisterUNITN register
@@ -728,4 +691,4 @@ XhcDataTransfer (
   IN UINTN   Timeout
   

[edk2] [PATCH V2 3/6] Revert "DebugUsb3: Support IOMMU"

[Star Zeng]

This reverts commit de8373fa07f87ca735139bb86c51e2c29fb1d956.

Cc: Ruiyu Ni 
Cc: Hao Wu 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 .../DebugCommunicationLibUsb3Common.c  | 222 ++-
 .../DebugCommunicationLibUsb3Dxe.c | 435 +
 .../DebugCommunicationLibUsb3Dxe.inf   |  12 +-
 .../DebugCommunicationLibUsb3Internal.h|  60 +--
 .../DebugCommunicationLibUsb3Pei.c | 234 +--
 .../DebugCommunicationLibUsb3Pei.inf   |   8 +-
 6 files changed, 129 insertions(+), 842 deletions(-)

diff --git 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
index 87fb0265489b..49bad6b5864d 100644
--- 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
+++ 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
@@ -14,6 +14,11 @@
 
 #include "DebugCommunicationLibUsb3Internal.h"
 
+//
+// The global variable which can be used after memory is ready.
+//
+USB3_DEBUG_PORT_HANDLE mDebugCommunicationLibUsb3DebugPortHandle;
+
 UINT16   mString0Desc[] = {
   //  String Descriptor Type + Length
   ( USB_DESC_TYPE_STRING << 8 ) + STRING0_DESC_LEN,
@@ -80,7 +85,7 @@ XhcClearR32Bit(
   Write the data to the XHCI debug register.
 
   @param  Handle   Debug port handle.
-  @param  Offset   The offset of the debug register.
+  @param  Offset   The offset of the runtime register.
   @param  Data The data to write.
 
 **/
@@ -124,16 +129,16 @@ XhcReadDebugReg (
 }
 
 /**
-  Set one bit of the debug register while keeping other bits.
+  Set one bit of the runtime register while keeping other bits.
 
   @param  Handle   Debug port handle.
-  @param  Offset   The offset of the debug register.
+  @param  Offset   The offset of the runtime register.
   @param  Bit  The bit mask of the register to set.
 
 **/
 VOID
 XhcSetDebugRegBit (
-  IN USB3_DEBUG_PORT_HANDLE   *Handle,
+  IN USB3_DEBUG_PORT_HANDLE *Handle,
   IN UINT32   Offset,
   IN UINT32   Bit
   )
@@ -146,28 +151,6 @@ XhcSetDebugRegBit (
 }
 
 /**
-  Clear one bit of the debug register while keeping other bits.
-
-  @param  Handle   Debug port handle.
-  @param  Offset   The offset of the debug register.
-  @param  Bit  The bit mask of the register to clear.
-
-**/
-VOID
-XhcClearDebugRegBit (
-  IN USB3_DEBUG_PORT_HANDLE   *Handle,
-  IN UINT32   Offset,
-  IN UINT32   Bit
-  )
-{
-  UINT32  Data;
-
-  Data  = XhcReadDebugReg (Handle, Offset);
-  Data  &= ~Bit;
-  XhcWriteDebugReg (Handle, Offset, Data);
-}
-
-/**
   Program and eanble XHCI MMIO base address.
 
   @return XHCI MMIO base address.
@@ -216,7 +199,7 @@ UpdateXhcResource (
   IN EFI_PHYSICAL_ADDRESS   XhciMmioBase
   )
 {
-  if (Handle == NULL) {
+  if ((Handle == NULL) || (Handle->XhciMmioBase == XhciMmioBase)) {
 return;
   }
 
@@ -233,7 +216,7 @@ UpdateXhcResource (
 
   @param  Handle Debug port handle.
 
-  @retval RETURN_UNSUPPORTED The usb host controller does not support usb 
debug port capability.
+  @retval RETURN_UNSUPPORTED The usb host controller does not supported usb 
debug port capability.
   @retval RETURN_SUCCESS Get bar and offset successfully.
 
 **/
@@ -253,14 +236,6 @@ CalculateUsbDebugPortMmioBase (
   EFI_PHYSICAL_ADDRESSCapabilityPointer;
   UINT8   CapLength;
 
-  if (Handle->Initialized != USB3DBG_UNINITIALIZED) {
-if (Handle->Initialized == USB3DBG_NO_DBG_CAB) {
-  return RETURN_UNSUPPORTED;
-} else {
-  return RETURN_SUCCESS;
-}
-  }
-
   VendorId = PciRead16 (PcdGet32(PcdUsbXhciPciAddress) + PCI_VENDOR_ID_OFFSET);
   DeviceId = PciRead16 (PcdGet32(PcdUsbXhciPciAddress) + PCI_DEVICE_ID_OFFSET);
   
@@ -313,7 +288,6 @@ CalculateUsbDebugPortMmioBase (
   Handle->DebugCapabilityBase   = CapabilityPointer;
   Handle->DebugCapabilityOffset = CapabilityPointer - Handle->XhciMmioBase;
   Handle->XhciOpRegister= Handle->XhciMmioBase + CapLength;
-  Handle->DebugSupport = TRUE;
   Handle->Initialized = USB3DBG_DBG_CAB;
   return RETURN_SUCCESS;
 
@@ -352,9 +326,6 @@ NeedReinitializeHardware(
   Dcctrl = XhcReadDebugReg (Handle, XHC_DC_DCCTRL);
   if ((Dcctrl & BIT0) == 0) {
 Result = TRUE;
-  } else if (!Handle->Ready) {
-Handle->Ready = TRUE;
-Handle->Initialized = USB3DBG_ENABLED;
   }
 
   return Result;
@@ -707,13 +678,6 @@ InitializeUsbDebugHardware (
   }
 
   //
-  // Clear DCE bit and LSE bit in DCCTRL
-  //
-  if ((XhcReadDebugReg (Handle, XHC_DC_DCCTRL) & (BIT1|BIT31)) == 
(BIT1|BIT31)) {
-XhcClearDebugRegBit (Handle, XHC_DC_DCCTRL, 

[edk2] [PATCH V2 4/6] SourceLevelDebugPkg DebugUsb3: Re-Fix GCC build failures

[Star Zeng]

Fix GCC build failures below.
variable 'EvtTrb' set but not used [-Werror=unused-but-set-variable]
variable 'Index' set but not used [-Werror=unused-but-set-variable]

The build failure could only be caught with -D SOURCE_DEBUG_USE_USB3
build flag.

ad6040ec9b5bbc462762331f9738b8e42c0b9c80 needs to be also reverted
when reverting IOMMU support patches, otherwise there will be conflict.
This patch is to re-do ad6040ec9b5bbc462762331f9738b8e42c0b9c80.

Cc: Ruiyu Ni 
Cc: Hao Wu 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 .../DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c   | 2 --
 .../DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c | 4 +---
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
index 49bad6b5864d..3b0f81cf88fd 100644
--- 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
+++ 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
@@ -852,7 +852,6 @@ DebugPortWriteBuffer (
   UINTN Sent;
   UINTN Total;
   EFI_PHYSICAL_ADDRESS  XhciMmioBase;
-  UINTN Index;
 
   if (NumberOfBytes == 0 || Buffer == NULL) {
 return 0;
@@ -895,7 +894,6 @@ DebugPortWriteBuffer (
   //
   DebugPortPollBuffer (Handle);
 
-  Index = 0;
   while ((Total < NumberOfBytes)) {
 if (NumberOfBytes - Total > USB3_DEBUG_PORT_WRITE_MAX_PACKET_SIZE) {
   Sent = USB3_DEBUG_PORT_WRITE_MAX_PACKET_SIZE;
diff --git 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c
 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c
index dbff49362407..1b6645bd1c6d 100644
--- 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c
+++ 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c
@@ -1,7 +1,7 @@
 /** @file
   Debug Port Library implementation based on usb3 debug port.
 
-  Copyright (c) 2014 - 2015, Intel Corporation. All rights reserved.
+  Copyright (c) 2014 - 2018, 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
@@ -145,11 +145,9 @@ XhcCheckNewEvent (
   )
 {
   EFI_STATUS  Status;
-  TRB_TEMPLATE*EvtTrb;
 
   ASSERT (EvtRing != NULL);
 
-  EvtTrb = (TRB_TEMPLATE *)(UINTN) EvtRing->EventRingDequeue;
   *NewEvtTrb = (TRB_TEMPLATE *)(UINTN) EvtRing->EventRingDequeue;
 
   if (EvtRing->EventRingDequeue == EvtRing->EventRingEnqueue) {
-- 
2.7.0.windows.1

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

[edk2] [PATCH V2 2/6] Revert "DebugUsb3: Fix GCC build failures"

[Star Zeng]

This reverts commit ad6040ec9b5bbc462762331f9738b8e42c0b9c80.

Cc: Ruiyu Ni 
Cc: Hao Wu 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 .../DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c   | 2 ++
 .../DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c | 4 +++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
index c577df7dea97..87fb0265489b 100644
--- 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
+++ 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
@@ -906,6 +906,7 @@ DebugPortWriteBuffer (
   USB3_DEBUG_PORT_HANDLE*UsbDebugPortHandle;
   UINTN Sent;
   UINTN Total;
+  UINTN Index;
 
   if (NumberOfBytes == 0 || Buffer == NULL) {
 return 0;
@@ -933,6 +934,7 @@ DebugPortWriteBuffer (
   //
   DebugPortPollBuffer (UsbDebugPortHandle);
 
+  Index = 0;
   while ((Total < NumberOfBytes)) {
 if (NumberOfBytes - Total > USB3_DEBUG_PORT_WRITE_MAX_PACKET_SIZE) {
   Sent = USB3_DEBUG_PORT_WRITE_MAX_PACKET_SIZE;
diff --git 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c
 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c
index 1b6645bd1c6d..dbff49362407 100644
--- 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c
+++ 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Transfer.c
@@ -1,7 +1,7 @@
 /** @file
   Debug Port Library implementation based on usb3 debug port.
 
-  Copyright (c) 2014 - 2018, Intel Corporation. All rights reserved.
+  Copyright (c) 2014 - 2015, 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
@@ -145,9 +145,11 @@ XhcCheckNewEvent (
   )
 {
   EFI_STATUS  Status;
+  TRB_TEMPLATE*EvtTrb;
 
   ASSERT (EvtRing != NULL);
 
+  EvtTrb = (TRB_TEMPLATE *)(UINTN) EvtRing->EventRingDequeue;
   *NewEvtTrb = (TRB_TEMPLATE *)(UINTN) EvtRing->EventRingDequeue;
 
   if (EvtRing->EventRingDequeue == EvtRing->EventRingEnqueue) {
-- 
2.7.0.windows.1

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

[edk2] [PATCH V2 0/6] SourceLevelDebugPkg DebugUsb3: Re-Support IOMMU

[Star Zeng]

The patch series is also at
https://github.com/lzeng14/edk2 DebugCommUsb3AfterIOMMUV2 branch.

Based on the feedbacks from Ray and Hao.
It is V2 of
https://lists.01.org/pipermail/edk2-devel/2018-March/022586.html.
It has no essential difference with V1 about the final code, but
re-arranges the patches to revert old IOMMU support patches and
then re-support IOMMU.

de8373fa07f87ca735139bb86c51e2c29fb1d956 could not handle two cases.
1. For the case that the USB3 debug port instance and DMA buffers are
from PEI HOB with IOMMU enabled, it was to reallocate the DMA buffers
by AllocateAddress with the memory type accessible by SMM environment.
But reallocating the DMA buffers by AllocateAddress may fail.

2. At S3 resume, after the code is transferred to PiSmmCpuDxeSmm from
S3Resume2Pei, HOB is still needed to be used for DMA operation, but
PiSmmCpuDxeSmm has no way to get the HOB at S3 resume.

The patch is to re-support IOMMU.
For PEI, allocate granted DMA buffer from IOMMU PPI, register IOMMU PPI
notification to reinitialize hardware with granted DMA buffer if IOMMU
PPI is not present yet.
For DXE, map DMA buffer by PciIo in PciIo notification for early DXE,
and register DxeSmmReadyToLock notification to reinitialize hardware
with granted DXE DMA buffer accessible by SMM environment for late DXE.

DebugAgentLib has been managing the instance as Handle in
HOB/SystemTable. The Handle(instance) from DebugAgentLib can be used
directly in DebugCommunicationLibUsb3. Then DebugCommunicationLibUsb3
could get consistent Handle(instance) from DebugAgentLib.

Cc: Ruiyu Ni 
Cc: Hao Wu 

Star Zeng (6):
  Revert "DebugUsb3: Check mUsb3Instance before dereferencing it"
  Revert "DebugUsb3: Fix GCC build failures"
  Revert "DebugUsb3: Support IOMMU"
  SourceLevelDebugPkg DebugUsb3: Re-Fix GCC build failures
  SourceLevelDebugPkg DebugCommUsb3: Refine some formats/comments
  SourceLevelDebugPkg DebugUsb3: Re-Support IOMMU

 .../DebugCommunicationLibUsb3Common.c  | 110 +-
 .../DebugCommunicationLibUsb3Dxe.c | 375 -
 .../DebugCommunicationLibUsb3Dxe.inf   |  11 +-
 .../DebugCommunicationLibUsb3Internal.h|  60 +---
 .../DebugCommunicationLibUsb3Pei.c |  50 ++-
 .../DebugCommunicationLibUsb3Pei.inf   |   4 +-
 6 files changed, 351 insertions(+), 259 deletions(-)

-- 
2.7.0.windows.1

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

[edk2] [PATCH V2 1/6] Revert "DebugUsb3: Check mUsb3Instance before dereferencing it"

[Star Zeng]

This reverts commit 6ef394ffe29bbc67038fc16ed540bfe6eed10e16.

Cc: Ruiyu Ni 
Cc: Hao Wu 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 .../Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Dxe.c| 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Dxe.c
 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Dxe.c
index 29cec56f39dc..1582b9a8d6de 100644
--- 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Dxe.c
+++ 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Dxe.c
@@ -443,7 +443,7 @@ DebugCommunicationUsb3DxeConstructor (
   }
 
 Done:
-  if ((mUsb3Instance != NULL) && mUsb3Instance->Ready && 
(mUsb3Instance->PciIoEvent == 0)) {
+  if (mUsb3Instance->Ready && (mUsb3Instance->PciIoEvent == 0)) {
 Status = Usb3NamedEventListen (
,
TPL_NOTIFY,
-- 
2.7.0.windows.1

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

Re: [edk2] [PATCH 0/6] OvmfPkg, ArmVirtQemu: leaner platform BDS policy for connecting devices

[Ard Biesheuvel]

On 13 March 2018 at 21:22, Laszlo Ersek  wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: qemu_bootorder_connect
>
> Adding tens or hundreds of bootable devices to a QEMU VM config slows
> the OVMF and ArmVirtQemu boots to a crawl, several people have reported
> in the past.
>
> There are at least two reasons for this (high pflash traffic due to
> heavy nvvar massaging per device, and PCI config space access slowing
> down on QEMU as the number of regions increases). However, part of the
> pain is self-inflicted in our PlatformBootManagerLib instances: we
> connect all bootable devices (for maximum compatibility with the user's
> VM config) even if the user doesn't intend to boot off most of them.
>
> It's oft repeated that the set of devices connected during boot is
> platform policy, so this series replaces the culprit
> EfiBootManagerConnectAll() calls with a bit smarter algorithm.
>
> I sought to keep the commit messages under control.
>

This is really nice. Most platforms I've worked with just connect
everything all the time, which is sloppy. I'm glad you fixed this for
*VMF

Tested-by: Ard Biesheuvel  # ArmVirtQemu
Reviewed-by: Ard Biesheuvel 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] ShellPkg/Dmpstore: Enhance display information for Auth3 variable.

[Zhang, Chao B]

Chen Chen:
  Please update license header. Others are good to me. 
  Reviewed-by: Chao Zhang


-Original Message-
From: Chen, Chen A 
Sent: Tuesday, March 13, 2018 3:37 PM
To: edk2-devel@lists.01.org
Cc: Chen, Chen A ; Ni, Ruiyu ; 
Zhang, Chao B 
Subject: [PATCH] ShellPkg/Dmpstore: Enhance display information for Auth3 
variable.

Add "EA" flag for dumping auth3 variable. When dumping Auth3 variable, it will 
not only displaying variable content but also in addition to metadata.
Give a warning message when dumping auth3 variable.

Cc: Ni Ruiyu 
Cc: Zhang Chao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: chenc2 
---
 ShellPkg/Library/UefiShellDebug1CommandsLib/DmpStore.c   | 9 +
 .../UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.uni| 1 +
 2 files changed, 10 insertions(+)

diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/DmpStore.c 
b/ShellPkg/Library/UefiShellDebug1CommandsLib/DmpStore.c
index 5791da9acc..adcec41992 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/DmpStore.c
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/DmpStore.c
@@ -69,6 +69,9 @@ GetAttrType (
   if ((Atts & EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS) != 0) {
 StrnCatGrow (, , L"+AT", 0);
   }
+  if ((Atts & EFI_VARIABLE_ENHANCED_AUTHENTICATED_ACCESS) != 0) {
+StrnCatGrow(, , L"+EA", 0);  }
 
   if (RetString == NULL) {
 RetString = StrnCatGrow(, , L"Invalid", 0); @@ -507,6 
+510,12 @@ CascadeProcessVariables (
 if (Type == DmpStoreDisplay) {
   if (!EFI_ERROR(Status) && (DataBuffer != NULL) && (FoundVarName != 
NULL)) {
 AttrString = GetAttrType(Atts);
+if (StrStr (AttrString, L"EA") != NULL) {
+ShellPrintHiiEx (
+  -1, -1, NULL, STRING_TOKEN (STR_DMPSTORE_VAR_EA_WARNING), 
gShellDebug1HiiHandle
+  );
+}
+
 if (StandardFormatOutput) {
   HexString = AllocatePool ((DataSize * 2 + 1) * sizeof (CHAR16));
   if (HexString != NULL) {
diff --git 
a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.uni 
b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.uni
index 011a7bfc2d..90ce69c932 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.uni
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Command
+++ sLib.uni
@@ -394,6 +394,7 @@
 #string STR_DMPSTORE_NO_VAR_FOUND_G#language en-US "%H%s%N: No matching 
variables found. Guid %g\r\n"
 #string STR_DMPSTORE_NO_VAR_FOUND_G_SFO #language en-US 
"VariableInfo,\"\",\"%g\",\"\",\"\",\"\"\r\n"
 #string STR_DMPSTORE_VAR_SFO   #language en-US 
"VariableInfo,\"%s\",\"%g\",\"0x%x\",\"0x%x\",\"%s\"\r\n"
+#string STR_DMPSTORE_VAR_EA_WARNING#language en-US "(Enhanced 
Authenticated Variable, Should be interpreted according to the metadata 
headers!)\r\n"
 
 #string STR_GET_HELP_COMP #language en-US ""
 ".TH comp 0 "Compare 2 files"\r\n"
--
2.13.2.windows.1

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

[edk2] [PATCH] MdeModulePkg/Core: allow HeapGuard even before CpuArchProtocol installed

[Jian J Wang]

Due to the fact that HeapGuard needs CpuArchProtocol to update page
attributes, the feature is normally enabled after CpuArchProtocol is
installed. Since there're some drivers are loaded before CpuArchProtocl,
they cannot make use HeapGuard feature to detect potential issues.

This patch fixes above situation by updating the DXE core to skip the
NULL check against global gCpu in the IsMemoryTypeToGuard(), and adding
NULL check against gCpu in SetGuardPage() and UnsetGuardPage() to make
sure that they can be called but do nothing. This will allow HeapGuard to
record all guarded memory without setting the related Guard pages to not-
present.

Once the CpuArchProtocol is installed, a protocol notify will be called
to complete the work of setting Guard pages to not-present.

Please note that above changes will cause a #PF in GCD code during cleanup
of map entries, which is initiated by CpuDxe driver to update real mtrr
and paging attributes back to GCD. During that time, CpuDxe doesn't allow
GCD to update memory attributes and then any Guard page cannot be unset.
As a result, this will prevent Guarded memory from freeing during memory
map cleanup.

The solution is to avoid allocating guarded memory as memory map entries
in GCD code. It's done by setting global mOnGuarding to TRUE before memory
allocation and setting it back to FALSE afterwards in GCD function
CoreAllocateGcdMapEntry().

Cc: Star Zeng 
Cc: Eric Dong 
Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c   |  10 ++
 MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 132 +-
 MdeModulePkg/Core/Dxe/Mem/HeapGuard.h |   8 ++
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c |   5 +
 4 files changed, 154 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index 8fbc3d282c..77f4adb4bc 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -16,6 +16,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 
 #include "DxeMain.h"
 #include "Gcd.h"
+#include "Mem/HeapGuard.h"
 
 #define MINIMUM_INITIAL_MEMORY_SIZE 0x1
 
@@ -391,12 +392,21 @@ CoreAllocateGcdMapEntry (
   IN OUT EFI_GCD_MAP_ENTRY  **BottomEntry
   )
 {
+  //
+  // Set to mOnGuarding to TRUE before memory allocation. This will make sure
+  // that the entry memory is not "guarded" by HeapGuard. Otherwise it might
+  // cause problem when it's freed (if HeapGuard is enabled).
+  //
+  mOnGuarding = TRUE;
   *TopEntry = AllocateZeroPool (sizeof (EFI_GCD_MAP_ENTRY));
+  mOnGuarding = FALSE;
   if (*TopEntry == NULL) {
 return EFI_OUT_OF_RESOURCES;
   }
 
+  mOnGuarding = TRUE;
   *BottomEntry = AllocateZeroPool (sizeof (EFI_GCD_MAP_ENTRY));
+  mOnGuarding = FALSE;
   if (*BottomEntry == NULL) {
 CoreFreePool (*TopEntry);
 return EFI_OUT_OF_RESOURCES;
diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c 
b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
index 19245049c2..de2c468b83 100644
--- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
+++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
@@ -576,6 +576,10 @@ SetGuardPage (
   IN  EFI_PHYSICAL_ADDRESS  BaseAddress
   )
 {
+  if (gCpu == NULL) {
+return;
+  }
+
   //
   // Set flag to make sure allocating memory without GUARD for page table
   // operation; otherwise infinite loops could be caused.
@@ -606,6 +610,10 @@ UnsetGuardPage (
 {
   UINT64  Attributes;
 
+  if (gCpu == NULL) {
+return;
+  }
+
   //
   // Once the Guard page is unset, it will be freed back to memory pool. NX
   // memory protection must be restored for this page if NX is enabled for free
@@ -652,7 +660,7 @@ IsMemoryTypeToGuard (
   UINT64 ConfigBit;
   BOOLEAN InSmm;
 
-  if (gCpu == NULL || AllocateType == AllocateAddress) {
+  if (AllocateType == AllocateAddress) {
 return FALSE;
   }
 
@@ -1160,6 +1168,128 @@ CoreConvertPagesWithGuard (
   return CoreConvertPages (Start, NumberOfPages, NewType);
 }
 
+/**
+  Set all Guard pages which cannot be set before CPU Arch Protocol installed.
+**/
+VOID
+SetAllGuardPages (
+  VOID
+  )
+{
+  UINTN Entries[GUARDED_HEAP_MAP_TABLE_DEPTH];
+  UINTN Shifts[GUARDED_HEAP_MAP_TABLE_DEPTH];
+  UINTN Indices[GUARDED_HEAP_MAP_TABLE_DEPTH];
+  UINT64Tables[GUARDED_HEAP_MAP_TABLE_DEPTH];
+  UINT64Addresses[GUARDED_HEAP_MAP_TABLE_DEPTH];
+  UINT64TableEntry;
+  UINT64Address;
+  UINT64GuardPage;
+  INTN  Level;
+  UINTN Index;
+  BOOLEAN   OnGuarding;
+
+  if (mGuardedMemoryMap == 0 ||
+  mMapLevel == 0 ||
+  mMapLevel > GUARDED_HEAP_MAP_TABLE_DEPTH) {
+return;
+  }
+
+  CopyMem (Entries, mLevelMask, sizeof (Entries));
+  CopyMem (Shifts, mLevelShift, sizeof (Shifts));
+
+  SetMem (Tables, sizeof(Tables), 0);
+  SetMem (Addresses, sizeof(Addresses), 0);
+  

Re: [edk2] EFI ExitBootServices() function crashes in QEMU

[Laszlo Ersek]

On 03/14/18 00:28, Anatol Pomozov wrote:
> Hello
> 
> I am implementing a simple UEFI bootloader. Most EFI functions that I
> tried work me - I can clear Console, print text, successfully read
> files from system partition, allocate memory pages.
> 
> But if I try to read memory map and then call ExitBootServices() QEMU
> crashes for me:
>   > qemu-system-x86_64: Trying to execute code outside RAM or ROM at
> 0x000b
> 
> I tried to enable QEMU cpu debugging and I see that at some point flow
> jumps to address 0 then executes "00" operations until it reaches
> 0xb (video memory?) that contains non-zero data and crashes.
> 
> Here is my bootloader code (this one is based on GNU-EFI but I also
> tried one based on xefi from Fuchsia).
> 
> 
> 
> EFI_STATUS efi_main(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SysTab) {
> InitializeLib(ImageHandle, SysTab);
> 
> UINTN mkey = 0, dsize = 0;
> UINT32 dversion = 0;
> UINTN msize = 32768;
> EFI_MEMORY_DESCRIPTOR *mmap = AllocatePool(msize);
> if (!mmap) {
> Print(L"AllocatePool failed\n");
> goto error;
> }
> 
> SysTab->BootServices->GetMemoryMap(, mmap, , , 
> );
> 
> EFI_STATUS st = SysTab->BootServices->ExitBootServices(ImageHandle, mkey);
> // QEMU crashes here ^^
> // qemu-system-x86_64: Trying to execute code outside RAM or ROM
> at 0x000b
> error:
> return EFI_SUCCESS;
> }
> 
> 
> 
> And here is my Makefile:
> 
> 
> 
> ARCH= x86_64
> 
> OBJS= bootloader.o
> TARGET  = bootloader.efi
> 
> EFIINC  = /usr/include/efi
> EFIINCS = -I$(EFIINC) -I$(EFIINC)/$(ARCH) -I$(EFIINC)/protocol
> EFILIB  = /usr/lib
> EFI_CRT_OBJS= $(EFILIB)/crt0-efi-$(ARCH).o
> EFI_LDS = $(EFILIB)/elf_$(ARCH)_efi.lds
> 
> CFLAGS  = $(EFIINCS) -fno-stack-protector -fPIC -fshort-wchar
> -mno-red-zone -Wall -std=c11
> 
> ifeq ($(ARCH),x86_64)
>   CFLAGS += -DHAVE_USE_MS_ABI
> endif
> 
> LDFLAGS = -nostdlib -znocombreloc -T $(EFI_LDS) -shared
> -Bsymbolic -L $(EFILIB) $(EFI_CRT_OBJS)
> 
> all: $(TARGET)
> 
> bootloader.so: $(OBJS)
>ld $(LDFLAGS) $(OBJS) -o $@ -lefi -lgnuefi
> 
> %.efi: %.so
>objcopy -j .text -j .sdata -j .data -j .dynamic -j .dynsym  -j .rel
> -j .rela -j .reloc --target=efi-app-$(ARCH) $^ $@
> 
> hda/EFI/BOOT/BOOTX64.EFI: bootloader.efi
>mkdir -p hda/EFI/BOOT/
>cp bootloader.efi hda/EFI/BOOT/BOOTX64.EFI
> 
> run: hda/EFI/BOOT/BOOTX64.EFI
>qemu-system-x86_64 --bios OVMF_CODE.fd -hda fat:rw:hda -net none
> 
> 
> 
> The sourcecode is simple and clear, I think the problem either in the
> Makefile (CFLAGS/LDFLAGS) or OVMF.
> 
> So my questions: in what cases ExitBootServices() can cause execution
> of NULL pointer? How to debug this issue with OVMF?
> 
> Or maybe you see something wrong with my CFLAGS/LDFLAGS?

I suggest asking this question on a gnu-efi development list or forum.

Alternatively, you could check how grub2-efi is built with gnu-efi in a
Linux distribution -- for example, Fedora's build logs are saved in
Koji:
https://kojipkgs.fedoraproject.org//packages/grub2/2.02/27.fc29/data/logs/x86_64/build.log

I think it very unlikely that the issue is with the ExitBootServices()
boot service itself. No UEFI OS can be booted without that service; if
it was broken in OVMF, we'd be inundated with bug reports.

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

Re: [edk2] [PATCH v5 4/6] MdeModulePkg/PciHostBridgeDxe: Add support for address translation

[Ni, Ruiyu]

On 3/7/2018 2:01 PM, Guo Heyi wrote:

Thanks. Please let me know if any further changes are needed.

Regards,

Heyi

On Wed, Mar 07, 2018 at 12:30:59PM +0800, Ni, Ruiyu wrote:

On 3/6/2018 10:44 AM, Guo Heyi wrote:

Hi Ray,

Any comments for v5?


Heyi,
Some backward compatibility concerns were received from internal production
teams. Current change will cause silent failure on old platforms because
TranslationOffset might be random if uninitialized.
I will solve the concern and then send out updates to you, hopefully by end
of next week.



Regards,

Heyi

On Thu, Mar 01, 2018 at 02:57:22PM +0800, Heyi Guo wrote:

PCI address translation is necessary for some non-x86 platforms. On
such platforms, address value (denoted as "device address" or "address
in PCI view") set to PCI BAR registers in configuration space might be
different from the address which is used by CPU to access the
registers in memory BAR or IO BAR spaces (denoted as "host address" or
"address in CPU view"). The difference between the two addresses is
called "Address Translation Offset" or simply "translation", and can
be represented by "Address Translation Offset" in ACPI QWORD Address
Space Descriptor (Offset 0x1E). However UEFI and ACPI differs on the
definitions of QWORD Address Space Descriptor, and we will follow UEFI
definition on UEFI protocols, such as PCI root bridge IO protocol and
PCI IO protocol. In UEFI 2.7, "Address Translation Offset" is "Offset
to apply to the Starting address to convert it to a PCI address". This
means:

1. Translation = device address - host address.

2. PciRootBridgeIo->Configuration should return CPU view address, as
well as PciIo->GetBarAttributes.

Summary of addresses used in protocol interfaces and internal
implementations:

1. *Only* the following protocol interfaces assume Address is Device
Address:
(1). PciHostBridgeResourceAllocation.GetProposedResources()
  Otherwise PCI bus driver cannot set correct address into PCI
  BARs.
(2). PciRootBridgeIo.Mem.Read() and PciRootBridgeIo.Mem.Write()
(3). PciRootBridgeIo.CopyMem()
UEFI and PI spec have clear statements for all other protocol
interfaces about the address type.

2. Library interfaces and internal implementation:
(1). Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address.
  It is easy to check whether the address is below 4G or above 4G.
(2). Addresses in PCI_ROOT_BRIDGE_INSTANCE.ResAllocNode are host
  address, for they are allocated from GCD.
(3). Address passed to PciHostBridgeResourceConflict is host address,
  for it comes from PCI_ROOT_BRIDGE_INSTANCE.ResAllocNode.

RESTRICTION: to simplify the situation, we require the alignment of
Translation must be larger than any BAR alignment in the same root
bridge, so that resource allocation alignment can be applied to both
device address and host address.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo 
Cc: Ruiyu Ni 
Cc: Ard Biesheuvel 
Cc: Star Zeng 
Cc: Eric Dong 
Cc: Laszlo Ersek 
Cc: Michael D Kinney 
---

Notes:
 v5:
 - Add check for the alignment of Translation against the BAR alignment
   [Ray]
 - Keep coding style of comments consistent with the context [Ray]
 - Comment change for Base in PCI_RES_NODE [Ray]
 - Add macros of TO_HOST_ADDRESS and TO_DEVICE_ADDRESS for address type
   conversion (After that we can also simply the comments about the
   calculation [Ray]
 - Add check for bus translation offset in CreateRootBridge(), making
   sure it is zero, and unify code logic for all types of resource
   after that [Ray]
 - Use GetTranslationByResourceType() to simplify the code in
   RootBridgeIoConfiguration() (also fix a bug in previous patch
   version of missing a break after case TypePMem64) [Ray]
 - Commit message refinement [Ray]
 v4:
 - Add ASSERT (FALSE) to default branch in GetTranslationByResourceType
   [Laszlo]
 - Fix bug when passing BaseAddress to gDS->AllocateIoSpace and
   gDS->AllocateMemorySpace [Laszlo]
 - Add comment for applying alignment to both device address and host
   address, and add NOTE for the alignment requirement of Translation,
   as well as in commit message [Laszlo][Ray]
 - Improve indention for the code in CreateRootBridge [Laszlo]
 - Improve comment for Translation in PCI_ROOT_BRIDGE_APERTURE
   definition [Laszlo]
 - Ignore translation of bus in CreateRootBridge
 v4:
 - Add ASSERT (FALSE) to default branch in GetTranslationByResourceType
   [Laszlo]
 - Fix bug when passing BaseAddress to gDS->AllocateIoSpace and
   gDS->AllocateMemorySpace [Laszlo]
 - Add comment for applying alignment to both device address and host
   address, and add NOTE for the alignment requirement of Translation,
   

[edk2] Intel Reference Board doesn't work after inserting INT 15h/86h into Flat32.asm

[Toan Le manh]

Hello everyone,

I have an Intel Reference Board. I was trying to force BIOS to wait 1
second in SEC phase.
I inserted these assemble code (BIOS wait function) into very first of
ProtectedModeEntryPoint in Flat32.asm file:

  MOV CX, 0FH
  MOV DX, 4240H
  MOV AH, 86H
  INT 15H

Then, the board stopped working. Status code showed "00".
Even when I re-flashed its original BIOS, it's still not worked.

Could anyone please help me to figure out what's happen here? And how to
fix it?
I  appreciate your helps.

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

Re: [edk2] [PATCH v2 1/1] ArmPkg/TimerDxe: Add ISB for timer compare value reload

[Marc Zyngier]

On Wed, 14 Mar 2018 00:25:09 +,
Guo Heyi wrote:
> 
> On Tue, Mar 13, 2018 at 09:33:33AM +, Marc Zyngier wrote:
> > On 13/03/18 00:31, Heyi Guo wrote:
> > > If timer interrupt is level sensitive, reloading timer compare
> > > register has a side effect of clearing GIC pending status, so a "ISB"
> > > is needed to make sure this instruction is executed before enabling
> > > CPU IRQ, or else we may get spurious timer interrupts.
> > > 
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Heyi Guo 
> > > Signed-off-by: Yi Li 
> > > Cc: Leif Lindholm 
> > > Cc: Ard Biesheuvel 
> > > Cc: Marc Zyngier 
> > > ---
> > > 
> > > Notes:
> > > v2:
> > > - Use ISB instead of DSB [Marc]
> > > - Update commit message accordingly.
> > > 
> > >  ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c 
> > > b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> > > index 33d7c91f..32abee8726a0 100644
> > > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> > > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> > > @@ -337,6 +337,7 @@ TimerInterruptHandler (
> > >  
> > >  // Set next compare value
> > >  ArmGenericTimerSetCompareVal (CompareValue);
> > > +ArmInstructionSynchronizationBarrier ();
> > >  ArmGenericTimerEnableTimer ();
> > >}
> > 
> > Sorry for being pedantic here, but it would make more sense if ISB was
> > placed after the enabling of the timer. Otherwise, you only force the
> > update of the comparator. But on the other hand, the timer was never
> > disabled the first place, in which case you'd wonder why you're trying
> > to enable it again.
> Yes, I also had such question and hesitated at this place :)
> > 
> > So either you leave the ISB here and remove the enable call, or move the
> > ISB after the enable.
> 
> If we are going to remove the enable call, is it better to be changed in a
> separate patch? It seems not related with adding ISB, though it is only a
> one-line change.

I guess a separate patch doesn't hurt, but that's for Ard and Leif to
decide.

Thanks,

M.

-- 
Jazz is not dead, it just smell funny.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel