Re: [edk2] [PATCH v4 0/2] quirks handling for SDHCI controllers

[Meenakshi Aggarwal]

Hi,

I am using Mmc Driver implemented in " EmbeddedPkg/Universal/MmcDxe/" for my 
SD/MMC controller and my controller is not on PCI bus.

I am a bit confused if i should move to SD implementation available in 
'MdeModulePkg\Bus\Pci\SdMmcPciHcDxe".

Please suggest.


Thanks,
Meenakshi

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Ard Biesheuvel
> Sent: Tuesday, January 30, 2018 3:22 PM
> To: Zeng, Star 
> Cc: Ni, Ruiyu ; Tian, Feng ; Wu,
> Hao A ; edk2-devel@lists.01.org;
> leif.lindh...@linaro.org; Kinney, Michael D 
> Subject: Re: [edk2] [PATCH v4 0/2] quirks handling for SDHCI controllers
> 
> On 30 January 2018 at 01:24, Zeng, Star  wrote:
> > Reviewed-by: Star Zeng 
> >
> > Thanks Hao's investigation and Ard's contribution.
> >
> 
> Thanks all
> 
> Pushed as 864701886fc3..b23fc39cd3c3
> 
> >
> > Star
> > -Original Message-
> > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> > Sent: Monday, January 29, 2018 4:26 PM
> > To: Wu, Hao A ; Zeng, Star ;
> Ni, Ruiyu 
> > Cc: edk2-devel@lists.01.org; leif.lindh...@linaro.org; Kinney, Michael D
> ; Tian, Feng 
> > Subject: Re: [PATCH v4 0/2] quirks handling for SDHCI controllers
> >
> > On 29 January 2018 at 05:13, Wu, Hao A  wrote:
> >> One minor comment, please help to remove the line (around line 1067):
> >> @param[in] Capability The capability of the slot.
> >>
> >> within function description comment for SdMmcHcInitHost() in file:
> >> MdeModulePkg\Bus\Pci\SdMmcPciHcDxe\SdMmcPciHci.c
> >>
> >> Other than that, the series is good to me:
> >> Reviewed-by: Hao Wu 
> >>
> >
> > Thank you very much!
> >
> >> Really sorry for the delay.
> >>
> >
> > No worries. Star, Ray, any more comments from your side?
> >
> >
> >>
> >>> -Original Message-
> >>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> >>> Sent: Friday, December 08, 2017 6:43 AM
> >>> To: edk2-devel@lists.01.org
> >>> Cc: leif.lindh...@linaro.org; Kinney, Michael D; Zeng, Star; Tian,
> >>> Feng; Ni, Ruiyu; Wu, Hao A; Ard Biesheuvel
> >>> Subject: [PATCH v4 0/2] quirks handling for SDHCI controllers
> >>>
> >>> Many SDHCI implementations exist that are almost spec complicant, and
> >>> could be driven by the generic SD/MMC host controller driver except
> >>> for some minimal necessary init time tweaks.
> >>>
> >>> Adding such tweaks to the generic driver is undesirable. On the other
> >>> hand, forking the driver for every platform that has such a SDHCI
> >>> controller is problematic when it comes to upstreaming and ongoing
> >>> maintenance (which is arguably the point of upstreaming in the first
> >>> place).
> >>>
> >>> So these patches propose a workaround that is minimally invasive on
> >>> the
> >>> EDK2 side, but gives platforms a lot of leeway when it comes to
> >>> applying SDHCI quirks.
> >>>
> >>> Changes since v3:
> >>> - remove PassThru argument from protocol members: it is unclear
> whether the
> >>>   protocol is available when the override protocol is invoked, and my
> >>>   example use case does not need it
> >>> - replace incorrect HandleProtocol with LocateProtocol, given that the
> override
> >>>   protocol is now a singleton instance
> >>> - merge notifier calls into SdMmcHcReset() and SdMmcHcInitHost (), this
> >>>   required changing the prototype to take a
> SD_MMC_HC_PRIVATE_DATA*
> >>> argument
> >>>   and so the prototypes no longer belong in SdMmcPciHci.h and have
> >>> been moved
> >>>   to SdMmcPciHcDxe.h
> >>> - use VOID* type for capability not UINT64* since we don't know its
> >>> alignment
> >>>
> >>> Changes since v2:
> >>> - use a singleton instance of the SD/MMC protocol rather than one per
> >>>   controller; this is needed to support 'reconnect -r', as pointed out
> >>>   by Ray
> >>> - use EDKII prefixes for all types defined by the protocol
> >>> - replace 'hook' with 'notify', and tweak some other identifiers
> >>> - add missing function comment headers for factored out functions
> >>>
> >>> Changes since RFC/v1:
> >>> - add EFI_SD_MMC_PASS_THRU_PROTOCOL* member to override
> methods
> >>> - use UINT64* not VOID* to pass capability structure (which is always 64
> bits
> >>>   in size)
> >>>
> >>> Ard Biesheuvel (2):
> >>>   MdeModulePkg: introduce SD/MMC override protocol
> >>>   MdeModulePkg/SdMmcPciHcDxe: allow HC capabilities to be
> overridden
> >>>
> >>>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c   | 35
> ++-
> >>>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h   | 36
> 
> >>>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf |  2 +
> >>>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 95
> >>> +--
> >>>  

[edk2] [PATCH 1/2] MdeModulePkg/NullMemoryTest: Change prototype of ConvertToTestedMemory

[Ruiyu Ni]

The patch should not impact the functionality.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni 
Cc: Liming Gao 
---
 .../MemoryTest/NullMemoryTestDxe/NullMemoryTest.c  | 37 ++
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git 
a/MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTest.c 
b/MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTest.c
index 11af8ea77f..c66f3fd208 100644
--- a/MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTest.c
+++ b/MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTest.c
@@ -59,37 +59,40 @@ GenericMemoryTestEntryPoint (
 }
 
 /**
-  Convert the memory descriptor to tested.
+  Convert the memory range to tested.
 
-  @param Descriptor  Pointer to EFI_GCD_MEMORY_SPACE_DESCRIPTOR
+  @param BaseAddress  Base address of the memory range.
+  @param Length   Length of the memory range.
+  @param Capabilities Capabilities of the memory range.
 
-  @retval EFI_SUCCESS The memory descriptor is converted to tested.
+  @retval EFI_SUCCESS The memory range is converted to tested.
   @retval others  Error happens.
 **/
 EFI_STATUS
 ConvertToTestedMemory (
-  IN CONST EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Descriptor
+  IN UINT64   BaseAddress,
+  IN UINT64   Length,
+  IN UINT64   Capabilities
   )
 {
   EFI_STATUS Status;
   Status = gDS->RemoveMemorySpace (
-  Descriptor->BaseAddress,
-  Descriptor->Length
+  BaseAddress,
+  Length
   );
   if (!EFI_ERROR (Status)) {
 Status = gDS->AddMemorySpace (
-((Descriptor->Capabilities & EFI_MEMORY_MORE_RELIABLE) == 
EFI_MEMORY_MORE_RELIABLE) ?
+((Capabilities & EFI_MEMORY_MORE_RELIABLE) == 
EFI_MEMORY_MORE_RELIABLE) ?
 EfiGcdMemoryTypeMoreReliable : 
EfiGcdMemoryTypeSystemMemory,
-Descriptor->BaseAddress,
-Descriptor->Length,
-Descriptor->Capabilities &~
+BaseAddress,
+Length,
+Capabilities &~
 (EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED | 
EFI_MEMORY_TESTED | EFI_MEMORY_RUNTIME)
 );
   }
   return Status;
 }
 
-
 /**
   Initialize the generic memory test.
 
@@ -129,7 +132,11 @@ InitializeMemoryTest (
   //
   // For those reserved memory that have not been tested, simply promote 
to system memory.
   //
-  Status = ConvertToTestedMemory ([Index]);
+  Status = ConvertToTestedMemory (
+ MemorySpaceMap[Index].BaseAddress,
+ MemorySpaceMap[Index].Length,
+ MemorySpaceMap[Index].Capabilities
+ );
   ASSERT_EFI_ERROR (Status);
   mTestedSystemMemory += MemorySpaceMap[Index].Length;
   mTotalSystemMemory += MemorySpaceMap[Index].Length;
@@ -236,7 +243,11 @@ GenCompatibleRangeTest (
 
   Status = gDS->GetMemorySpaceDescriptor (StartAddress, );
   if (!EFI_ERROR (Status)) {
-Status = ConvertToTestedMemory ();
+Status = ConvertToTestedMemory (
+   Descriptor.BaseAddress,
+   Descriptor.Length,
+   Descriptor.Capabilities
+   );
   }
   return Status;
 }
-- 
2.16.1.windows.1

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

[edk2] [PATCH 0/2] Fix bug in CompatibleRangeTest

[Ruiyu Ni]

Ruiyu Ni (2):
  MdeModulePkg/NullMemoryTest: Change prototype of ConvertToTestedMemory
  MdeModulePkg/NullMemoryTest: Fix bug in CompatibleRangeTest

 .../MemoryTest/NullMemoryTestDxe/NullMemoryTest.c  | 82 +-
 1 file changed, 65 insertions(+), 17 deletions(-)

-- 
2.16.1.windows.1

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

[edk2] [PATCH 2/2] MdeModulePkg/NullMemoryTest: Fix bug in CompatibleRangeTest

[Ruiyu Ni]

CompatibleRangeTest() contains two bugs:
1. It doesn't reject the memory above 16MB
2. it cannot handle the case when the partial or whole range of
   requested memory is already tested.

The patch fixes the two bugs.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni 
Cc: Michael D Kinney 
Cc: Liming Gao 
---
 .../MemoryTest/NullMemoryTestDxe/NullMemoryTest.c  | 55 ++
 1 file changed, 46 insertions(+), 9 deletions(-)

diff --git 
a/MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTest.c 
b/MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTest.c
index c66f3fd208..a9bd101501 100644
--- a/MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTest.c
+++ b/MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTest.c
@@ -240,14 +240,51 @@ GenCompatibleRangeTest (
 {
   EFI_STATUS  Status;
   EFI_GCD_MEMORY_SPACE_DESCRIPTOR Descriptor;
-
-  Status = gDS->GetMemorySpaceDescriptor (StartAddress, );
-  if (!EFI_ERROR (Status)) {
-Status = ConvertToTestedMemory (
-   Descriptor.BaseAddress,
-   Descriptor.Length,
-   Descriptor.Capabilities
-   );
+  EFI_PHYSICAL_ADDRESSCurrentBase;
+  UINT64  CurrentLength;
+
+  //
+  // Check if the parameter is below 16MB
+  //
+  if (StartAddress + Length > SIZE_16MB) {
+return EFI_INVALID_PARAMETER;
   }
-  return Status;
+  CurrentBase = StartAddress;
+  do {
+//
+// Check the required memory range status; if the required memory range 
span
+// the different GCD memory descriptor, it may be cause different action.
+//
+Status = gDS->GetMemorySpaceDescriptor (
+CurrentBase,
+
+);
+if (EFI_ERROR (Status)) {
+  return Status;
+}
+
+if (Descriptor.GcdMemoryType == EfiGcdMemoryTypeReserved &&
+(Descriptor.Capabilities & (EFI_MEMORY_PRESENT | 
EFI_MEMORY_INITIALIZED | EFI_MEMORY_TESTED)) ==
+  (EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED)
+  ) {
+  CurrentLength = Descriptor.BaseAddress + Descriptor.Length - CurrentBase;
+  if (CurrentBase + CurrentLength > StartAddress + Length) {
+CurrentLength = StartAddress + Length - CurrentBase;
+  }
+  Status = ConvertToTestedMemory (
+ CurrentBase,
+ CurrentLength,
+ Descriptor.Capabilities
+ );
+  if (EFI_ERROR (Status)) {
+return Status;
+  }
+}
+CurrentBase = Descriptor.BaseAddress + Descriptor.Length;
+  } while (CurrentBase < StartAddress + Length);
+  //
+  // Here means the required range already be tested, so just return success.
+  //
+  return EFI_SUCCESS;
 }
+
-- 
2.16.1.windows.1

___
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,

Any comments for v5?

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

Re: [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop

[Gao, Liming]

Laszlo:
  I also suggest to check the generated ProcessLibraryConstructorList () 
function. It is in the driver build output AutoGen.c code. You can check what 
library function be called in this function. Then, further add debug message in 
the library function. I suspect some function does the wrong operation and 
corrupt the memory. 

Thanks
Liming
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
> Ersek
> Sent: Tuesday, March 6, 2018 2:23 AM
> To: Marc-André Lureau ; Andrew Fish 
> 
> Cc: edk2-devel@lists.01.org; Peter Jones ; Yao, Jiewen 
> ; QEMU
> ; Javier Martinez Canillas 
> Subject: Re: [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop
> 
> On 03/05/18 15:05, Marc-André Lureau wrote:
> > Hi
> >
> > On Fri, Feb 23, 2018 at 8:45 PM, Andrew Fish  wrote:
> >>
> >>
> >>> On Feb 23, 2018, at 5:23 AM, marcandre.lur...@redhat.com wrote:
> >>>
> >>> From: Marc-André Lureau 
> >>>
> >>> Without this hack, GetNextHob() loops infinitely with the next
> >>> patch. I don't understand the reason.
> >>>
> >>> The loop is triggered by the GetFirstGuidHob ()
> >>> call.
> >>>
> >>> CC: Laszlo Ersek 
> >>> CC: Stefan Berger 
> >>> Contributed-under: TianoCore Contribution Agreement 1.0
> >>> Signed-off-by: Marc-André Lureau 
> >>> ---
> >>> MdePkg/Library/PeiHobLib/HobLib.c | 4 
> >>> 1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/MdePkg/Library/PeiHobLib/HobLib.c 
> >>> b/MdePkg/Library/PeiHobLib/HobLib.c
> >>> index 5c0eeb992f..ed3c5fbd6d 100644
> >>> --- a/MdePkg/Library/PeiHobLib/HobLib.c
> >>> +++ b/MdePkg/Library/PeiHobLib/HobLib.c
> >>> @@ -89,6 +89,10 @@ GetNextHob (
> >>> if (Hob.Header->HobType == Type) {
> >>>   return Hob.Raw;
> >>> }
> >>> +if (GET_HOB_LENGTH (HobStart) == 0) {
> >>
> >> As Laszlo points out this error condition is likely memory
> >> corruption. Thus it would be better to check for all know illegal
> >> values?
> >>
> >> if (GET_HOB_LENGTH(HobStart) < sizeof (EFI_HOB_GENERIC_HEADER)
> >>
> >
> > Thanks, I have adjusted the check.
> >
> > With manual calls and printf (I don't know  a better way to debug ovmf
> > ;),
> 
> Well that's how I generally debug it too :)
> 
> > I try to locate the issue. It's somehow related to
> > RegisterForShadow(). The "corruption" seems to happen during the
> > second call. After the
> > PeiLoadImage(...,PEIM_STATE_REGISTER_FOR_SHADOW,..), right before
> > calling PeimEntryPoint(), a GetFirstGuidHob() succeed, but inside the
> > function, it fails (with the same arguments). Right after it succeeds
> > again... The PeimEntryPoint() is not the Tcg2Pei:PeimEntryMA(), I
> > suppose there is some kind of wrapping code, but I fail to find where.
> > Any idea?
> 
> This sounds helpful. I don't know what the problem is, but I can
> elaborate on your details a bit; perhaps someone else will have more
> ideas.
> 
> Apparently there is a PEI service called RegisterForShadow().
> ("Apparently", because I've never seen, let alone written, a PEIM
> calling this service.) The service is specified in the PI spec, which is
> quoted in the edk2 tree [MdePkg/Include/Pi/PiPeiCis.h]:
> 
> > /**
> >   Register a PEIM so that it will be shadowed and called again.
> >
> >   This service registers a file handle so that after memory is
> >   available, the PEIM will be re-loaded into permanent memory and
> >   re-initialized. The PEIM registered this way will always be
> >   initialized twice. The first time, this function call will
> >   return EFI_SUCCESS. The second time, this function call will
> >   return EFI_ALREADY_STARTED. Depending on the order in which
> >   PEIMs are dispatched, the PEIM making this call may be
> >   initialized after permanent memory is installed, even the first
> >   time.
> >
> >   @param  FileHandlePEIM's file handle. Must be the currently
> > executing PEIM.
> >
> >   @retval EFI_SUCCESS   The PEIM was successfully registered for
> > shadowing.
> >   @retval EFI_ALREADY_STARTED   The PEIM was previously
> > registered for shadowing.
> >   @retval EFI_NOT_FOUND The FileHandle does not refer to a
> > valid file handle.
> >
> > **/
> > typedef
> > EFI_STATUS
> > (EFIAPI *EFI_PEI_REGISTER_FOR_SHADOW)(
> >   IN  EFI_PEI_FILE_HANDLE FileHandle
> >   );
> 
> PEIMs generally "execute in place" (XIP), i.e. they run from flash, not
> RAM. In this status they use "temporary RAM" (e.g. CPU caches configured
> like RAM) for stack & heap; whatever HOBs they produce are stored in
> "temp RAM" as well. Then one of the PEIMs "discovers permanent RAM"
> (basically it 

Re: [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop

[Brian J. Johnson]

On 03/05/2018 12:22 PM, Laszlo Ersek wrote:

PEIMs generally "execute in place" (XIP), i.e. they run from flash, not
RAM. In this status they use "temporary RAM" (e.g. CPU caches configured
like RAM) for stack & heap; whatever HOBs they produce are stored in
"temp RAM" as well. Then one of the PEIMs "discovers permanent RAM"
(basically it programs the memory controller and publishes the RAM
ranges). In turn the PEI core "migrates" PEIMs from temporary to
permanent RAM, including the HOB list.

Before the temporary RAM migration (when still executing in place from
flash), PEIMs cannot have writeable global variables. For example,
dynamic PCDs are also maintained in a HOB (the PCD HOB).

A PEIM normally cannot (and shouldn't) tell whether it is dispatched
before or after permanent RAM is published. If needed, a PEIM can
advertise that it depends on permanent RAM (for example because it needs
a lot of heap memory) by including "gEfiPeiMemoryDiscoveredPpiGuid" in
its DEPEX.

Finally, it seems like a PEIM can also express, "I'm fine with being
dispatched from both flash (XIP) vs. permanent RAM, just the PEI core
tell me whichever it is". Apparently, if the PEIM is dispatched from
flash (before permanent RAM is available), its call to
RegisterForShadow() returns EFI_SUCCESS (and then its entry point
function will be invoked for a 2nd time, after the temp RAM migration).
And when a PEIM is dispatched from RAM (either for the very first time,
or for the second time, after being dispatched from flash first), the
same call returns EFI_ALREADY_STARTED.

Honestly, I'm unsure what this is good for (both in general, and
specifically for Tcg2Pei). Apparently, Tcg2Pei needs permanent RAM for
doing the measurements (which makes sense); I just wonder what exactly
it does so that it cannot simply specify
"gEfiPeiMemoryDiscoveredPpiGuid" in its DEPEX.


I haven't looked at this particular PEIM.  But one case where 
registering for shadowing is useful is for improving performance when 
running from permanent RAM, where writable global variables are 
available.  For instance, when running from flash, a ReportStatusCode 
PEIM may need to go through a slow process to locate an output hardware 
device on every PPI call.  This may involve traversing the HOB list, 
consulting other PPIs, even probing hardware addresses.  But once it's 
shadowed to RAM, it can locate the device once, then cache its address 
in a global.  Not to mention that the code itself is far, far faster 
when run from RAM vs. flash.  (That's probably a key difference between 
a real machine and a VM.)


Also, I've personally written a PEIM which saves a bunch of internal 
state in a HOB, since that's the main writable storage when running from 
flash.  That state includes pointers to other data (in flash.)  Once the 
data is all shadowed to RAM, it updates the HOB to point to the data in 
RAM.  That way it's a lot faster to access the data.


I also have other PEIMs which are constrained (via DEPEX) to run *only* 
from RAM, since they have larger memory requirements than can be 
satisfied from temporary cache-as-RAM.  That's certainly a valid 
technique as well.


RegisterForShadow() is a useful tool for making the most of the 
restricted PEI environment.  And having it standardized like this is, as 
Andrew said, a lot better than the hacks people had to use beforehand.


Thanks,
--
Brian J. Johnson
Enterprise X86 Lab

Hewlett Packard Enterprise

brian.john...@hpe.com

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

[edk2] [Patch] EmbeddedPkg: Correct the way of handling sections with a large size

[Ge Song]

Correct the way of handling EFI_SECTION_GUID_DEFINED type sections
with a large size

Cc: Leif Lindholm 
Cc: Ard Biesheuvel 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ge Song 
---
 
EmbeddedPkg/Library/PrePiExtractGuidedSectionLib/PrePiExtractGuidedSectionLib.c 
| 18 --
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git 
a/EmbeddedPkg/Library/PrePiExtractGuidedSectionLib/PrePiExtractGuidedSectionLib.c
 
b/EmbeddedPkg/Library/PrePiExtractGuidedSectionLib/PrePiExtractGuidedSectionLib.c
index 7b08de8ab9fe..8e7abe202836 100644
--- 
a/EmbeddedPkg/Library/PrePiExtractGuidedSectionLib/PrePiExtractGuidedSectionLib.c
+++ 
b/EmbeddedPkg/Library/PrePiExtractGuidedSectionLib/PrePiExtractGuidedSectionLib.c
@@ -123,6 +123,7 @@ ExtractGuidedSectionGetInfo (
 {
   PRE_PI_EXTRACT_GUIDED_SECTION_DATA  *SavedData;
   UINT32  Index;
+  EFI_GUID *SectionDefinitionGuid;
 
   if (InputSection == NULL) {
 return RETURN_INVALID_PARAMETER;
@@ -134,11 +135,17 @@ ExtractGuidedSectionGetInfo (
 
   SavedData = GetSavedData();
 
+  if (IS_SECTION2 (InputSection)) {
+SectionDefinitionGuid = &(((EFI_GUID_DEFINED_SECTION2 *) 
InputSection)->SectionDefinitionGuid);
+  } else {
+SectionDefinitionGuid = &(((EFI_GUID_DEFINED_SECTION *) 
InputSection)->SectionDefinitionGuid);
+  }
+
   //
   // Search the match registered GetInfo handler for the input guided section.
   //
   for (Index = 0; Index < SavedData->NumberOfExtractHandler; Index ++) {
-if (CompareGuid (>ExtractHandlerGuidTable[Index], 
&(((EFI_GUID_DEFINED_SECTION *) InputSection)->SectionDefinitionGuid))) {
+if (CompareGuid (>ExtractHandlerGuidTable[Index], 
SectionDefinitionGuid)) {
   break;
 }
   }
@@ -172,6 +179,7 @@ ExtractGuidedSectionDecode (
 {
   PRE_PI_EXTRACT_GUIDED_SECTION_DATA  *SavedData;
   UINT32  Index;
+  EFI_GUID *SectionDefinitionGuid;
 
   if (InputSection == NULL) {
 return RETURN_INVALID_PARAMETER;
@@ -182,11 +190,17 @@ ExtractGuidedSectionDecode (
 
   SavedData = GetSavedData();
 
+  if (IS_SECTION2 (InputSection)) {
+SectionDefinitionGuid = &(((EFI_GUID_DEFINED_SECTION2 *) 
InputSection)->SectionDefinitionGuid);
+  } else {
+SectionDefinitionGuid = &(((EFI_GUID_DEFINED_SECTION *) 
InputSection)->SectionDefinitionGuid);
+  }
+
   //
   // Search the match registered GetInfo handler for the input guided section.
   //
   for (Index = 0; Index < SavedData->NumberOfExtractHandler; Index ++) {
-if (CompareGuid (>ExtractHandlerGuidTable[Index], 
&(((EFI_GUID_DEFINED_SECTION *) InputSection)->SectionDefinitionGuid))) {
+if (CompareGuid (>ExtractHandlerGuidTable[Index], 
SectionDefinitionGuid)) {
   break;
 }
   }
-- 
2.11.0


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

Re: [edk2] [PATCH] IntelSiliconPkg VTdPmrPei: Return SUCCESS when Mapping == NULL in Unmap

[Yao, Jiewen]

Reviewed-by: jiewen@intel.com

> -Original Message-
> From: Zeng, Star
> Sent: Monday, March 5, 2018 9:34 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; Yao, Jiewen 
> Subject: [PATCH] IntelSiliconPkg VTdPmrPei: Return SUCCESS when Mapping ==
> NULL in Unmap
> 
> NULL is returned to Mapping when Operation is BusMasterCommonBuffer or
> EdkiiIoMmuOperationBusMasterCommonBuffer64 in PeiIoMmuMap().
> So Mapping == NULL is valid when calling PeiIoMmuUnmap().
> 
> 940dbd071e9f01717236af236740aa0da716805f wrongly changed
> EFI_SUCCESS
> to EFI_INVALID_PARAMETER when Mapping == NULL in PeiIoMmuUnmap().
> This patch is to correct it.
> 
> Cc: Jiewen Yao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng 
> ---
>  IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c
> b/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c
> index ea3091ef911c..6289834fcb38 100644
> --- a/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c
> +++ b/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c
> @@ -189,7 +189,7 @@ PeiIoMmuMap (
>if (Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
>Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
>  *DeviceAddress = (UINTN)HostAddress;
> -*Mapping = 0;
> +*Mapping = NULL;
>  return EFI_SUCCESS;
>}
> 
> @@ -266,7 +266,7 @@ PeiIoMmuUnmap (
>}
> 
>if (Mapping == NULL) {
> -return EFI_INVALID_PARAMETER;
> +return EFI_SUCCESS;
>}
> 
>MapInfo = Mapping;
> --
> 2.13.3.windows.1

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

Re: [edk2] [PATCH] IntelSiliconPkg VTdPmrPei: Add PcdVTdPeiDmaBufferSize(S3)

[Yao, Jiewen]

Reviewed-by: jiewen@intel.com

> -Original Message-
> From: Zeng, Star
> Sent: Monday, March 5, 2018 10:17 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; Yao, Jiewen 
> Subject: [PATCH] IntelSiliconPkg VTdPmrPei: Add PcdVTdPeiDmaBufferSize(S3)
> 
> Add PcdVTdPeiDmaBufferSize(S3) to replace the hard coded value
> TOTAL_DMA_BUFFER_SIZE and TOTAL_DMA_BUFFER_SIZE_S3 in
> IntelVTdPmrPei.
> 
> Cc: Jiewen Yao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng 
> ---
>  .../Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c|  7 ++-
>  .../Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.inf  |  4 +++-
>  IntelSiliconPkg/IntelSiliconPkg.dec| 18
> +-
>  3 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c
> b/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c
> index 6289834fcb38..9a0138b3b086 100644
> --- a/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c
> +++ b/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c
> @@ -29,9 +29,6 @@
> 
>  #include "IntelVTdPmrPei.h"
> 
> -#define  TOTAL_DMA_BUFFER_SIZESIZE_4MB
> -#define  TOTAL_DMA_BUFFER_SIZE_S3 SIZE_2MB
> -
>  EFI_GUID mVTdInfoGuid = {
>0x222f5e30, 0x5cd, 0x49c6, { 0x8a, 0xc, 0x36, 0xd6, 0x58, 0x41, 0xe0,
> 0x82 }
>  };
> @@ -798,9 +795,9 @@ IntelVTdPmrInitialize (
>PeiServicesGetBootMode ();
> 
>if (BootMode == BOOT_ON_S3_RESUME) {
> -DmaBufferInfo->DmaBufferSize = TOTAL_DMA_BUFFER_SIZE_S3;
> +DmaBufferInfo->DmaBufferSize = PcdGet32
> (PcdVTdPeiDmaBufferSizeS3);
>} else {
> -DmaBufferInfo->DmaBufferSize = TOTAL_DMA_BUFFER_SIZE;
> +DmaBufferInfo->DmaBufferSize = PcdGet32 (PcdVTdPeiDmaBufferSize);
>}
> 
>Status = PeiServicesNotifyPpi ();
> diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.inf
> b/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.inf
> index e6d0323acc50..5b688d5cbf9f 100644
> --- a/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.inf
> +++ b/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.inf
> @@ -4,7 +4,7 @@
>  # This driver initializes VTd engine based upon EDKII_VTD_INFO_PPI
>  # and provide DMA protection in PEI.
>  #
> -# Copyright (c) 2017, Intel Corporation. All rights reserved.
> +# Copyright (c) 2017 - 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
> @@ -54,6 +54,8 @@ [Ppis]
> 
>  [Pcd]
>gIntelSiliconPkgTokenSpaceGuid.PcdVTdPolicyPropertyMask   ##
> CONSUMES
> +  gIntelSiliconPkgTokenSpaceGuid.PcdVTdPeiDmaBufferSize ##
> CONSUMES
> +  gIntelSiliconPkgTokenSpaceGuid.PcdVTdPeiDmaBufferSizeS3   ##
> CONSUMES
> 
>  [Depex]
>gEfiPeiMasterBootModePpiGuid AND
> diff --git a/IntelSiliconPkg/IntelSiliconPkg.dec
> b/IntelSiliconPkg/IntelSiliconPkg.dec
> index a15d3dee392c..c0cf58fa6cb5 100644
> --- a/IntelSiliconPkg/IntelSiliconPkg.dec
> +++ b/IntelSiliconPkg/IntelSiliconPkg.dec
> @@ -3,7 +3,7 @@
>  #
>  # This package provides common open source Intel silicon modules.
>  #
> -# Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.
> +# Copyright (c) 2016 - 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.
>  # The full text of the license may be found at
> @@ -61,3 +61,19 @@ [PcdsFixedAtBuild, PcdsPatchableInModule,
> PcdsDynamic, PcdsDynamicEx]
># @Prompt The policy for VTd driver behavior.
> 
> gIntelSiliconPkgTokenSpaceGuid.PcdVTdPolicyPropertyMask|1|UINT8|0x0
> 002
> 
> +  ## Declares VTd PEI DMA buffer size.
> +  #  When this PCD value is referred by platform to calculate the required
> +  #  memory size for PEI (InstallPeiMemory), the PMR alignment requirement
> +  #  needs be considered to be added with this PCD value for alignment
> +  #  adjustment need by AllocateAlignedPages.
> +  # @Prompt The VTd PEI DMA buffer size.
> +
> gIntelSiliconPkgTokenSpaceGuid.PcdVTdPeiDmaBufferSize|0x0040|UINT3
> 2|0x0003
> +
> +  ## Declares VTd PEI DMA buffer size for S3.
> +  #  When this PCD value is referred by platform to calculate the required
> +  #  memory size for PEI S3 (InstallPeiMemory), the PMR alignment
> requirement
> +  #  needs be considered to be added with this PCD value for alignment
> +  #  adjustment need by AllocateAlignedPages.
> +  # @Prompt The VTd PEI DMA buffer size for S3.
> +
> gIntelSiliconPkgTokenSpaceGuid.PcdVTdPeiDmaBufferSizeS3|0x0020|UINT
> 32|0x0004
> +
> --
> 2.13.3.windows.1

Re: [edk2] [PATCH 0/3] BaseTools: let the C-language build utils compile with gcc-8

[Laszlo Ersek]

On 03/05/18 15:13, Gao, Liming wrote:
> Reviewed-by: Liming Gao 

Thank you, Liming; I pushed the series as commit range
20203d3f98d6..9de306701312.

Laszlo

>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Saturday, March 03, 2018 2:09 AM
>> To: edk2-devel-01 
>> Cc: Ard Biesheuvel ; Cole Robinson
>> ; Gao, Liming ; Paolo Bonzini
>> ; Zhu, Yonghong 
>> Subject: [PATCH 0/3] BaseTools: let the C-language build utils compile with
>> gcc-8
>>
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: basetools_gcc8
>>
>> Once these patches are applied to the build flags and the source code of
>> the build utilities themselves, OVMF builds fine with gcc-8, using the
>> GCC5 toolchain settings without any changes.
>>
>> Regression-tested with gcc-4.8 / x86_64.
>>
>> Cc: Ard Biesheuvel 
>> Cc: Cole Robinson 
>> Cc: Liming Gao 
>> Cc: Paolo Bonzini 
>> Cc: Yonghong Zhu 
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (3):
>>  BaseTools/header.makefile: add "-Wno-stringop-truncation"
>>  BaseTools/header.makefile: add "-Wno-restrict"
>>  BaseTools/GenVtf: silence false "stringop-overflow" warning with
>>memcpy()
>>
>> BaseTools/Source/C/Makefiles/header.makefile | 4 ++--
>> BaseTools/Source/C/GenVtf/GenVtf.c   | 4 ++--
>> 2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> --
>> 2.14.1.3.gb7cf6e02401b
> 
> ___
> 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 00/20] OvmfPkg: SEV: decrypt the initial SMRAM save state map for SMBASE relocation

[Laszlo Ersek]

On 03/05/18 15:44, Brijesh Singh wrote:
> On 03/05/2018 08:00 AM, Laszlo Ersek wrote:

>> QEMU exits with the following error for me:
>>
>> 2018-03-05T13:40:12.478835Z qemu-system-x86_64: sev_ram_block_added:
>> failed to register region (0x7f3df3e0+0x2)
>> 2018-03-05T13:40:12.489183Z qemu-system-x86_64: sev_ram_block_added:
>> failed to register region (0x7f3ffaa0+0x37c000)
>> 2018-03-05T13:40:12.497580Z qemu-system-x86_64: sev_ram_block_added:
>> failed to register region (0x7f3ffa80+0x2)
>> 2018-03-05T13:40:12.504485Z qemu-system-x86_64:
>> sev_launch_update_data: LAUNCH_UPDATE ret=-12 fw_error=0 ''
>> 2018-03-05T13:40:12.504493Z qemu-system-x86_64: failed to encrypt
>> pflash rom
>>
>> Here's my full QEMU command line (started by libvirt) -- this command
>> line does not restrict pflash access to guest code that runs in SMM,
>> and correspondingly, the OVMF build lacks SMM_REQUIRE:
>>
> 
> Are you launching guest as a normal users or root ? If you are launching
> guest as normal user then please make sure you have increased the 'max
> locked memory' limit. The register region function will try to pin the
> memory, while doing so we check the limit and if requested size is
> greater than ulimit then we fail.
> 
> 
> # ulimit -a
> core file size  (blocks, -c) unlimited
> data seg size   (kbytes, -d) unlimited
> scheduling priority (-e) 0
> file size   (blocks, -f) unlimited
> pending signals (-i) 966418
> max locked memory   (kbytes, -l) 1024
> max memory size (kbytes, -m) unlimited
> open files  (-n) 1024
> pipe size    (512 bytes, -p) 8
> POSIX message queues (bytes, -q) 819200
> real-time priority  (-r) 0
> stack size  (kbytes, -s) 8192
> cpu time   (seconds, -t) unlimited
> max user processes  (-u) 966418
> virtual memory  (kbytes, -v) unlimited
> file locks  (-x) unlimited

Good catch! Libvirtd starts the QEMU process with UID=qemu, but the
restriction doesn't come from there.

Instead, it seems like the systemd default for "max locked memory" is
64KB on RHEL-7 anyway. I raised it by setting

  DefaultLimitMEMLOCK=infinity

in "/etc/systemd/system.conf".

(The documentation is at:
- ,
-
.)

Following your other email, I've now also added
"iommu_platform=on,ats=on" to virtio-net-pci, not just virtio-scsi-pci.

This got a lot farther: the TianoCore splash screen was dispalyed, but
then it got stuck.

Looking more at the libvirt-generated command line, I figured maybe
"vhost" should be disabled for virtio-net (so that the device
implementation would run from QEMU userspace, not in the host kernel).
Thus, ultimately I added


  
  ^^^


documented at
.

With these settings, the guest boots & works fine for me! I tested the
SEV guest with 4 VCPUs, both with and without SMM. (I used the same
kernel in the guest as on the host -- you wrote CONFIG_AMD_MEM_ENCRYPT
for the guest, and the host requirements imply that.)

I'm attaching the full domain XML for reference.

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

Re: [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop

[Andrew Fish]

> On Mar 5, 2018, at 10:22 AM, Laszlo Ersek  wrote:
> 
> On 03/05/18 15:05, Marc-André Lureau wrote:
>> Hi
>> 
>> On Fri, Feb 23, 2018 at 8:45 PM, Andrew Fish  wrote:
>>> 
>>> 
 On Feb 23, 2018, at 5:23 AM, marcandre.lur...@redhat.com wrote:
 
 From: Marc-André Lureau 
 
 Without this hack, GetNextHob() loops infinitely with the next
 patch. I don't understand the reason.
 
 The loop is triggered by the GetFirstGuidHob ()
 call.
 
 CC: Laszlo Ersek 
 CC: Stefan Berger 
 Contributed-under: TianoCore Contribution Agreement 1.0
 Signed-off-by: Marc-André Lureau 
 ---
 MdePkg/Library/PeiHobLib/HobLib.c | 4 
 1 file changed, 4 insertions(+)
 
 diff --git a/MdePkg/Library/PeiHobLib/HobLib.c 
 b/MdePkg/Library/PeiHobLib/HobLib.c
 index 5c0eeb992f..ed3c5fbd6d 100644
 --- a/MdePkg/Library/PeiHobLib/HobLib.c
 +++ b/MdePkg/Library/PeiHobLib/HobLib.c
 @@ -89,6 +89,10 @@ GetNextHob (
if (Hob.Header->HobType == Type) {
  return Hob.Raw;
}
 +if (GET_HOB_LENGTH (HobStart) == 0) {
>>> 
>>> As Laszlo points out this error condition is likely memory
>>> corruption. Thus it would be better to check for all know illegal
>>> values?
>>> 
>>> if (GET_HOB_LENGTH(HobStart) < sizeof (EFI_HOB_GENERIC_HEADER)
>>> 
>> 
>> Thanks, I have adjusted the check.
>> 
>> With manual calls and printf (I don't know  a better way to debug ovmf
>> ;),
> 
> Well that's how I generally debug it too :)
> 
>> I try to locate the issue. It's somehow related to
>> RegisterForShadow(). The "corruption" seems to happen during the
>> second call. After the
>> PeiLoadImage(...,PEIM_STATE_REGISTER_FOR_SHADOW,..), right before
>> calling PeimEntryPoint(), a GetFirstGuidHob() succeed, but inside the
>> function, it fails (with the same arguments). Right after it succeeds
>> again... The PeimEntryPoint() is not the Tcg2Pei:PeimEntryMA(), I
>> suppose there is some kind of wrapping code, but I fail to find where.
>> Any idea?
> 
> This sounds helpful. I don't know what the problem is, but I can
> elaborate on your details a bit; perhaps someone else will have more
> ideas.
> 
> Apparently there is a PEI service called RegisterForShadow().
> ("Apparently", because I've never seen, let alone written, a PEIM
> calling this service.) The service is specified in the PI spec, which is
> quoted in the edk2 tree [MdePkg/Include/Pi/PiPeiCis.h]:
> 

In the "olden days" the PEI Core did not shadow PEIMs. There were a few PEIMs 
that had some hacky code to shadow themselves into memory. We thought of making 
it a library, but there was not a clean way to detect if the PEIM was shadowed. 
So we ended up adding the PEI Service. You could also use RegisterForShadow 
with a DEPEX of gEfiPeiMemoryDiscoveredPpiGuid to cause your PEIM to get 
shadowed even if the PEI Core did not support automatically shadowing PEIMs 
after memory showed up. 

Anyway the RegisterForShadow can cause the entry point for the PEIM to get 
called again, and if there is a bug handling that I imagine bad things can 
happen. 

There are plenty examples of RegisterForShadow usage in the edk2. Maybe looking 
at how other PEIMs are using it would be helpful. 

(master)>git grep ".RegisterForShadow"
SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c:852:Status = 
(**PeiServices).RegisterForShadow(FileHandle);
SecurityPkg/Tcg/TcgPei/TcgPei.c:776:Status = 
(**PeiServices).RegisterForShadow(FileHandle);
SecurityPkg/Tcg/TrEEPei/TrEEPei.c:616:Status = 
(**PeiServices).RegisterForShadow(FileHandle);
(master)>git grep "PeiServicesRegisterForShadow"
EmulatorPkg/Library/SecPeiServicesLib/PeiServicesLib.c:423:PeiServicesRegisterForShadow
 (
FatPkg/FatPei/FatLiteApi.c:258:  Status = PeiServicesRegisterForShadow 
(FileHandle);
IntelFrameworkModulePkg/Bus/Isa/IsaFloppyPei/FloppyPeim.c:1725:  Status = 
PeiServicesRegisterForShadow (FileHandle);
MdeModulePkg/Bus/Pci/EhciPei/EhcPeim.c:1199:  if (!EFI_ERROR 
(PeiServicesRegisterForShadow (FileHandle))) {
MdeModulePkg/Bus/Pci/IdeBusPei/AtapiPeim.c:44:  Status = 
PeiServicesRegisterForShadow (FileHandle);
MdeModulePkg/Bus/Pci/SdMmcPciHcPei/SdMmcPciHcPei.c:103:  if (!EFI_ERROR 
(PeiServicesRegisterForShadow (FileHandle))) {
MdeModulePkg/Bus/Pci/UfsPciHcPei/UfsPciHcPei.c:92:  if (!EFI_ERROR 
(PeiServicesRegisterForShadow (FileHandle))) {
MdeModulePkg/Bus/Pci/UhciPei/UhcPeim.c:121:  if (!EFI_ERROR 
(PeiServicesRegisterForShadow (FileHandle))) {
MdeModulePkg/Bus/Pci/XhciPei/XhcPeim.c:1462:  if (!EFI_ERROR 
(PeiServicesRegisterForShadow (FileHandle))) {
MdeModulePkg/Bus/Sd/EmmcBlockIoPei/EmmcBlockIoPei.c:693:  if (!EFI_ERROR 
(PeiServicesRegisterForShadow (FileHandle))) {
MdeModulePkg/Bus/Sd/SdBlockIoPei/SdBlockIoPei.c:540:  if (!EFI_ERROR 
(PeiServicesRegisterForShadow (FileHandle))) {

Re: [edk2] [PATCH 5/7] ovmf: link with Tcg2Dxe module

[Laszlo Ersek]

On 03/05/18 16:45, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Feb 26, 2018 at 10:50 AM, Laszlo Ersek  wrote:
>> On 02/23/18 14:23, marcandre.lur...@redhat.com wrote:

>>> +  SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf {
>>> +
>>> +  
>>> Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
>>
>> Can you please explain why Tpm2DeviceLib has to be resolved differently
>> for DXE_DRIVER modules in general (see above) and for "Tcg2Dxe.inf"
>> specifically?
>>
>> Hmmm... Looks like "Tpm2DeviceLibTcg2.inf" implements the APIs via the
>> TPM2 protocol. Whereas "Tcg2Dxe.inf" itself provides that protocol, so
>> obviously it cannot rely on the protocol; it has to access the device,
>> which is done with the help of "Tpm2DeviceLibRouterDxe.inf" and the
>> "Tpm2InstanceLibDTpm.inf" that is plugged in via NULL resolution below.
>> Is that about correct?
>>
>> If so, can you please document it in the commit message?
> 
> I followed the SecurityPkg.dsc, and tried some variants. This router
> pattern can be found in other places, unfortunately, I can't explain
> it. I can copy your explanation if that helps.

Yes, I think we should capture it in the commit message.

I'd also like to make another attempt at explaining it to you (and me as
well :) ).


* We have a library class called Tpm2DeviceLib -- this is basically the
set of APIs declared in "SecurityPkg/Include/Library/Tpm2DeviceLib.h".
Its leading comment says "This library abstract how to access TPM2
hardware device".

There are two *sets* of APIs in "Tpm2DeviceLib.h":

(a) functions that deal with the TPM2 device:
- Tpm2RequestUseTpm(),
- Tpm2SubmitCommand()

This set of APIs is supposed to be used by clients that *consume*
the TPM2 device abstraction.

(b) the function Tpm2RegisterTpm2DeviceLib(), which is supposed to be
used by *providers* of various TPM2 device abstractions.


* Then, we have two implementations (instances) of the Tpm2DeviceLib class:
(1) SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
(2) SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf

(1) The first library instance ("Tpm2DeviceLibTcg2.inf") implements the
APIs listed under (a), and it does not implement (b) -- see
EFI_UNSUPPORTED. In other words, this lib instance is strictly meant for
drivers that *consume* the TPM2 device abstraction. And, the (a) group
of APIs is implemented by forwarding the requests to the TCG2 protocol.

The idea here is that all the drivers that consume the TPM2 abstraction
do not have to be statically linked with a large TPM2 device library
instance; instead they are only linked (statically) with this "thin"
library instance, and all the actual work is delegated to whichever
driver that provides the singleton TCG2 protocol.

(2) The second library instance ("Tpm2DeviceLibRouterDxe.inf") is meant
for the driver that offers (produces) the TCG2 protocol. This lib
instance implements both (a) and (b) API groups.


* Here's how things fit together:

(i) The "SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf"
library instance (which has no lib class) is linked into "Tcg2Dxe.inf"
via NULL class resolution. This simply means that before the
"Tcg2Dxe.inf" entry point function is entered, the constructor function
of "Tpm2InstanceLibDTpm.inf" will be called.

(ii) This Tpm2InstanceLibDTpmConstructor() function calls API (b), and
registers its own actual TPM2 command implementation with the
"Tpm2DeviceLibRouter" library instance (also linked into the Tcg2Dxe
driver). This provides the back-end for the API set (a).

   TCG2 protocol provider (Tcg2Dxe.inf driver) launches
|
v
  NULL class: Tpm2InstanceLibDTpm instance construction
|
v
  Tpm2DeviceLib class: Tpm2DeviceLibRouter instance
 backend registration for API set (a)

(iii) The Tcg2Dxe driver exposes the TCG2 protocol.

(iv) A TPM2 consumer calls API set (a) via lib instance (1). Such calls
land in Tcg2Dxe, via the protocol.

(v) Tcg2Dxe serves the protocol request by forwarding it to API set (a)
from lib instance (2).

(vi) Those functions call the "backend" functions registered by
Tpm2DeviceLibDTpm in step (ii).

 TPM 2 consumer driver
  |
  v
Tpm2DeviceLib class: Tpm2DeviceLibTcg2 instance
  |
  v
   TCG2 protocol interface
  |
  v
TCG2 protocol provider: Tcg2Dxe.inf driver
  |
  v
Tpm2DeviceLib class: Tpm2DeviceLibRouter instance
  |
  v
   NULL class: Tpm2InstanceLibDTpm instance
  (via earlier registration)
  |
  v
 TPM2 chip (actual hardware)


* So that is the "router" pattern in edk2. Namely,

- Consumers of an abstraction use a thin library instance.

- The thin library instance calls a firmware-global (singleton) service,
  i.e. a PPI (in the PEI phase) or 

Re: [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop

[Laszlo Ersek]

On 03/05/18 15:05, Marc-André Lureau wrote:
> Hi
>
> On Fri, Feb 23, 2018 at 8:45 PM, Andrew Fish  wrote:
>>
>>
>>> On Feb 23, 2018, at 5:23 AM, marcandre.lur...@redhat.com wrote:
>>>
>>> From: Marc-André Lureau 
>>>
>>> Without this hack, GetNextHob() loops infinitely with the next
>>> patch. I don't understand the reason.
>>>
>>> The loop is triggered by the GetFirstGuidHob ()
>>> call.
>>>
>>> CC: Laszlo Ersek 
>>> CC: Stefan Berger 
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Marc-André Lureau 
>>> ---
>>> MdePkg/Library/PeiHobLib/HobLib.c | 4 
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/MdePkg/Library/PeiHobLib/HobLib.c 
>>> b/MdePkg/Library/PeiHobLib/HobLib.c
>>> index 5c0eeb992f..ed3c5fbd6d 100644
>>> --- a/MdePkg/Library/PeiHobLib/HobLib.c
>>> +++ b/MdePkg/Library/PeiHobLib/HobLib.c
>>> @@ -89,6 +89,10 @@ GetNextHob (
>>> if (Hob.Header->HobType == Type) {
>>>   return Hob.Raw;
>>> }
>>> +if (GET_HOB_LENGTH (HobStart) == 0) {
>>
>> As Laszlo points out this error condition is likely memory
>> corruption. Thus it would be better to check for all know illegal
>> values?
>>
>> if (GET_HOB_LENGTH(HobStart) < sizeof (EFI_HOB_GENERIC_HEADER)
>>
>
> Thanks, I have adjusted the check.
>
> With manual calls and printf (I don't know  a better way to debug ovmf
> ;),

Well that's how I generally debug it too :)

> I try to locate the issue. It's somehow related to
> RegisterForShadow(). The "corruption" seems to happen during the
> second call. After the
> PeiLoadImage(...,PEIM_STATE_REGISTER_FOR_SHADOW,..), right before
> calling PeimEntryPoint(), a GetFirstGuidHob() succeed, but inside the
> function, it fails (with the same arguments). Right after it succeeds
> again... The PeimEntryPoint() is not the Tcg2Pei:PeimEntryMA(), I
> suppose there is some kind of wrapping code, but I fail to find where.
> Any idea?

This sounds helpful. I don't know what the problem is, but I can
elaborate on your details a bit; perhaps someone else will have more
ideas.

Apparently there is a PEI service called RegisterForShadow().
("Apparently", because I've never seen, let alone written, a PEIM
calling this service.) The service is specified in the PI spec, which is
quoted in the edk2 tree [MdePkg/Include/Pi/PiPeiCis.h]:

> /**
>   Register a PEIM so that it will be shadowed and called again.
>
>   This service registers a file handle so that after memory is
>   available, the PEIM will be re-loaded into permanent memory and
>   re-initialized. The PEIM registered this way will always be
>   initialized twice. The first time, this function call will
>   return EFI_SUCCESS. The second time, this function call will
>   return EFI_ALREADY_STARTED. Depending on the order in which
>   PEIMs are dispatched, the PEIM making this call may be
>   initialized after permanent memory is installed, even the first
>   time.
>
>   @param  FileHandlePEIM's file handle. Must be the currently
> executing PEIM.
>
>   @retval EFI_SUCCESS   The PEIM was successfully registered for
> shadowing.
>   @retval EFI_ALREADY_STARTED   The PEIM was previously
> registered for shadowing.
>   @retval EFI_NOT_FOUND The FileHandle does not refer to a
> valid file handle.
>
> **/
> typedef
> EFI_STATUS
> (EFIAPI *EFI_PEI_REGISTER_FOR_SHADOW)(
>   IN  EFI_PEI_FILE_HANDLE FileHandle
>   );

PEIMs generally "execute in place" (XIP), i.e. they run from flash, not
RAM. In this status they use "temporary RAM" (e.g. CPU caches configured
like RAM) for stack & heap; whatever HOBs they produce are stored in
"temp RAM" as well. Then one of the PEIMs "discovers permanent RAM"
(basically it programs the memory controller and publishes the RAM
ranges). In turn the PEI core "migrates" PEIMs from temporary to
permanent RAM, including the HOB list.

Before the temporary RAM migration (when still executing in place from
flash), PEIMs cannot have writeable global variables. For example,
dynamic PCDs are also maintained in a HOB (the PCD HOB).

A PEIM normally cannot (and shouldn't) tell whether it is dispatched
before or after permanent RAM is published. If needed, a PEIM can
advertise that it depends on permanent RAM (for example because it needs
a lot of heap memory) by including "gEfiPeiMemoryDiscoveredPpiGuid" in
its DEPEX.

Finally, it seems like a PEIM can also express, "I'm fine with being
dispatched from both flash (XIP) vs. permanent RAM, just the PEI core
tell me whichever it is". Apparently, if the PEIM is dispatched from
flash (before permanent RAM is available), its call to
RegisterForShadow() returns EFI_SUCCESS (and then its entry point
function will be invoked for a 2nd time, after the temp RAM migration).
And when a PEIM 

Re: [edk2] [PATCH 5/7] ovmf: link with Tcg2Dxe module

[Marc-André Lureau]

Hi

On Mon, Feb 26, 2018 at 10:50 AM, Laszlo Ersek  wrote:
> On 02/23/18 14:23, marcandre.lur...@redhat.com wrote:
>> From: Marc-André Lureau 
>>
>> This module measures and log the boot environment. It also produces
>> the Tcg2 protocol, which allows for example to read the log from OS:
>>
>> [0.00] efi: EFI v2.70 by EDK II
>> [0.00] efi:  SMBIOS=0x3fa1f000  ACPI=0x3fbb6000  ACPI 2.0=0x3fbb6014 
>>  MEMATTR=0x3e7d4318  TPMEventLog=0x3db21018
>>
>> $ python chipsec_util.py tpm parse_log binary_bios_measurements
>>
>> [CHIPSEC] Version 1.3.5.dev2
>> [CHIPSEC] API mode: using OS native API (not using CHIPSEC kernel module)
>> [CHIPSEC] Executing command 'tpm' with args ['parse_log', 
>> '/tmp/binary_bios_measurements']
>>
>> PCR: 0type: EV_S_CRTM_VERSION size: 0x2   
>> digest: 1489f923c4dca729178b3e3233458550d8dddf29
>>   + version:
>> PCR: 0type: EV_EFI_PLATFORM_FIRMWARE_BLOB size: 0x10  
>> digest: fd39ced7c0d2a61f6830c78c7625f94826b05bcc
>>   + base: 0x82length: 0xe
>> PCR: 0type: EV_EFI_PLATFORM_FIRMWARE_BLOB size: 0x10  
>> digest: 39ebc6783b72bc1e73c7d5bcfeb5f54a3f105d4c
>>   + base: 0x90length: 0xa0
>> PCR: 7type: EV_EFI_VARIABLE_DRIVER_CONFIG size: 0x35  
>> digest: 57cd4dc19442475aa82743484f3b1caa88e142b8
>> PCR: 7type: EV_EFI_VARIABLE_DRIVER_CONFIG size: 0x24  
>> digest: 9b1387306ebb7ff8e795e7be77563666bbf4516e
>> PCR: 7type: EV_EFI_VARIABLE_DRIVER_CONFIG size: 0x26  
>> digest: 9afa86c507419b8570c62167cb9486d9fc809758
>> PCR: 7type: EV_EFI_VARIABLE_DRIVER_CONFIG size: 0x24  
>> digest: 5bf8faa078d40ffbd03317c93398b01229a0e1e0
>> PCR: 7type: EV_EFI_VARIABLE_DRIVER_CONFIG size: 0x26  
>> digest: 734424c9fe8fc71716c42096f4b74c88733b175e
>> PCR: 7type: EV_SEPARATOR  size: 0x4   
>> digest: 9069ca78e7450a285173431b3e52c5c25299e473
>> PCR: 1type: EV_EFI_VARIABLE_BOOT  size: 0x3e  
>> digest: 252f8ebb85340290b64f4b06a001742be8e5cab6
>> PCR: 1type: EV_EFI_VARIABLE_BOOT  size: 0x6e  
>> digest: 22a4f6ee9af6dba01d3528deb64b74b582fc182b
>> PCR: 1type: EV_EFI_VARIABLE_BOOT  size: 0x80  
>> digest: b7811d5bf30a7efd4e385c6179fe10d9290bb9e8
>> PCR: 1type: EV_EFI_VARIABLE_BOOT  size: 0x84  
>> digest: 425e502c24fc924e231e0a62327b6b7d1f704573
>> PCR: 1type: EV_EFI_VARIABLE_BOOT  size: 0x9a  
>> digest: 0b5d2c98ac5de6148a4a1490ff9d5df69039f04e
>> PCR: 1type: EV_EFI_VARIABLE_BOOT  size: 0xbd  
>> digest: 20bd5f402271d57a88ea314fe35c1705956b1f74
>> PCR: 1type: EV_EFI_VARIABLE_BOOT  size: 0x88  
>> digest: df5d6605cb8f4366d745a8464cfb26c1efdc305c
>> PCR: 4type: EV_EFI_ACTION size: 0x28  
>> digest: cd0fdb4531a6ec41be2753ba042637d6e5f7f256
>> PCR: 0type: EV_SEPARATOR  size: 0x4   
>> digest: 9069ca78e7450a285173431b3e52c5c25299e473
>> PCR: 1type: EV_SEPARATOR  size: 0x4   
>> digest: 9069ca78e7450a285173431b3e52c5c25299e473
>> PCR: 2type: EV_SEPARATOR  size: 0x4   
>> digest: 9069ca78e7450a285173431b3e52c5c25299e473
>> PCR: 3type: EV_SEPARATOR  size: 0x4   
>> digest: 9069ca78e7450a285173431b3e52c5c25299e473
>> PCR: 4type: EV_SEPARATOR  size: 0x4   
>> digest: 9069ca78e7450a285173431b3e52c5c25299e473
>> PCR: 5type: EV_SEPARATOR  size: 0x4   
>> digest: 9069ca78e7450a285173431b3e52c5c25299e473
>>
>> $ tpm2_pcrlist
>> sha1 :
>>   0  : 35bd1786b6909daad610d7598b1d620352d33b8a
>>   1  : ec0511e860206e0af13c31da2f9e943fb6ca353d
>>   2  : b2a83b0ebf2f8374299a5b2bdfc31ea955ad7236
>>   3  : b2a83b0ebf2f8374299a5b2bdfc31ea955ad7236
>>   4  : 45a323382bd933f08e7f0e256bc8249e4095b1ec
>>   5  : d16d7e629fd8d08ca256f9ad3a3a1587c9e6cc1b
>>   6  : b2a83b0ebf2f8374299a5b2bdfc31ea955ad7236
>>   7  : 518bd167271fbb64589c61e43d8c0165861431d8
>>   8  : 
>>   9  : 
>>   10 : 
>>   11 : 
>>   12 : 
>>   13 : 
>>   14 : 
>>   15 : 
>>   16 : 
>>   17 : 
>>   18 : 
>>   19 : 
>>   20 : 
>>   21 : 
>>   22 : 

Re: [edk2] [PATCH] ShellPkg/ConsoleLogger: Fix a typo in UpdateDisplayFromHistory()

[Carsey, Jaben]

Reviewed-by: Jaben Carsey 

> -Original Message-
> From: Wu, Hao A
> Sent: Friday, March 02, 2018 7:05 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A ; Carsey, Jaben
> ; Ni, Ruiyu 
> Subject: [PATCH] ShellPkg/ConsoleLogger: Fix a typo in
> UpdateDisplayFromHistory()
> Importance: High
> 
> Within function UpdateDisplayFromHistory():
> 
> When getting a character with different attribute with the current one,
> the statement to compare the character with a 'NULL' char should be:
> 
> *StringSegmentEnd != CHAR_NULL
> 
> rather than:
> 
> StringSegmentEnd != CHAR_NULL
> 
> This commit resolves this typo.
> 
> Cc: Jaben Carsey 
> Cc: Ruiyu Ni 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Hao Wu 
> ---
>  ShellPkg/Application/Shell/ConsoleLogger.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ShellPkg/Application/Shell/ConsoleLogger.c
> b/ShellPkg/Application/Shell/ConsoleLogger.c
> index bc96da1f1b..074e0cf046 100644
> --- a/ShellPkg/Application/Shell/ConsoleLogger.c
> +++ b/ShellPkg/Application/Shell/ConsoleLogger.c
> @@ -2,7 +2,7 @@
>Provides interface to shell console logger.
> 
>(C) Copyright 2013 Hewlett-Packard Development Company, L.P.
> -  Copyright (c) 2009 - 2015, Intel Corporation. All rights reserved.
> +  Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.
>(C) Copyright 2016 Hewlett-Packard Development Company, L.P.
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of the BSD
> License
> @@ -322,7 +322,7 @@ UpdateDisplayFromHistory(
>  //
>  StringSegmentEndChar = CHAR_NULL;
>  for ( StringSegmentEnd = StringSegment
> -; StringSegmentEnd != CHAR_NULL
> +; *StringSegmentEnd != CHAR_NULL
>  ; StringSegmentEnd++
>  , Column++
> ){
> --
> 2.12.0.windows.1

___
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

[Evan Lloyd]

In that case, would you be happy to take Girish's patches with the ASSERTs done 
your (our) way?
Leif can fulminate about it when he gets back, if he really feels that strongly.
I suspect, though, that Leif is capable of being reasonable if pressed (and 
offered beer at Plugfest).

Regards,
Evan

> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: 02 March 2018 19:07
> To: Evan Lloyd 
> Cc: leif.lindh...@linaro.org; Girish Pathak ;
> Matteo Carlini ; nd ; edk2-
> de...@lists.01.org
> Subject: Re: [edk2] [PATCH edk2-platforms v2 06/18] ARM/VExpressPkg:
> Add and update debug ASSERTS
> 
> On 28 February 2018 at 20:27, Evan Lloyd  wrote:
> > Hi Leif, Ard.
> > Can I get you two argue out the pros and cons of the "ASSERT(FALSE)"
> debate, please.
> 
> I can argue the cons if you like. For the pros, you'll have to wait for Leif 
> to
> return from holiday (in a week or two AFAIK)
> 
> > (see
> > https://lists.01.org/pipermail/edk2-devel/2018-January/019788.html)
> > For what it is worth, our (surprisingly unanimous) opinion is that, since
> the ASSERT is only there to help spot a problem, then the more information
> reported the better.  The only benefits of ASSERT(FALSE) would be a smaller
> debug image and minor efficiency improvement on the path to the crash.
> >
> > Regards,
> > Evan
> >
> >> -Original Message-
> >> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> >> Sent: 04 January 2018 19:55
> >> To: Evan Lloyd 
> >> Cc: Girish Pathak ; Matteo Carlini
> >> ; nd ; edk2-
> de...@lists.01.org;
> >> Thomas Abraham ; Arvind Chauhan
> >> ; leif.lindh...@linaro.org
> >> Subject: Re: [edk2] [PATCH edk2-platforms v2 06/18] ARM/VExpressPkg:
> >> Add and update debug ASSERTS
> >>
> >> On 4 January 2018 at 19:51, Evan Lloyd  wrote:
> >> >
> >> >
> >> >> -Original Message-
> >> >> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> >> >> Sent: 04 January 2018 19:24
> >> >> To: Girish Pathak 
> >> >> Cc: Evan Lloyd ; Matteo Carlini
> >> >> ; nd ; edk2-
> >> de...@lists.01.org;
> >> >> Thomas Abraham ; Arvind Chauhan
> >> >> ; leif.lindh...@linaro.org
> >> >> Subject: Re: [edk2] [PATCH edk2-platforms v2 06/18]
> ARM/VExpressPkg:
> >> >> Add and update debug ASSERTS
> >> >>
> >> >> 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/HdLcdArmVExp
> >> >> r
> >> >> >> ess.c   | 22 +-
> >> >> >> >
> >> >> >> >
> >> >> >>
> >> >>
> >>
> Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdAr
> >> >> m
> >> >> >> VEx
> >> >> >> > press.c | 24 +++-
> >> >> >> >  2 files changed, 44 insertions(+), 2 deletions(-)
> >> >> >> >
> >> >> >> > diff --git
> >> >> >> >
> >> >> >>
> >> >>
> >>
> a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVE
> >> >> x
> >> >> >> pres
> >> >> >> > s.c
> >> >> >> >
> >> >> >>
> >> >>
> >>
> b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVE
> >> >> x
> >> >> >> pres
> >> >> >> > s.c index
> >> >> >> >
> >> >> >>
> >> >>
> >>
> 6afd764897f49c64490ce891682f99bb0f5d993b..a8fe8696da0653017ce9fa
> >> >> 6e4a
> >> >> >> 86
> >> >> >> > caf283bc04c9 100644
> >> >> >> > ---
> >> >> >> >
> >> >> >>
> >> >>
> >>
> a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVE
> >> >> x
> >> >> >> 

Re: [edk2] [PATCH v1] MdePkg: SMMUv3 updates for IORT table definitions

[Ard Biesheuvel]

On 5 March 2018 at 14:53, Sami Mujawar  wrote:
> Updated the IORT SMMUv3 Node structure and flags to match the
> IO Remapping Table, Platform Design Document, Revision C dated
> 15 MAY 2017.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Sami Mujawar 
> Signed-off-by: Evan Lloyd 

Reviewed-by: Ard Biesheuvel 

> ---
>
> The changes can be seen at 
> https://github.com/samimujawar/edk2/tree/236_ioremapping_header_v1
>
> Notes:
> v1:
> - Updated SMMUv3 node structure and associated flags. [SAMI]
>
>  MdePkg/Include/IndustryStandard/IoRemappingTable.h | 11 ++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/MdePkg/Include/IndustryStandard/IoRemappingTable.h 
> b/MdePkg/Include/IndustryStandard/IoRemappingTable.h
> index 
> c113afdd27843111bc7ad6e1de1108260fad2bbc..2e5cb45d7e2ffd4a0559ef706b71874843e3fdbd
>  100644
> --- a/MdePkg/Include/IndustryStandard/IoRemappingTable.h
> +++ b/MdePkg/Include/IndustryStandard/IoRemappingTable.h
> @@ -4,6 +4,7 @@
>
> http://infocenter.arm.com/help/topic/com.arm.doc.den0049c/DEN0049C_IO_Remapping_Table.pdf
>
>Copyright (c) 2017, Linaro Limited. All rights reserved.
> +  Copyright (c) 2018, ARM Limited. All rights reserved.
>
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of the BSD 
> License
> @@ -53,6 +54,11 @@
>
>  #define EFI_ACPI_IORT_SMMUv3_FLAG_COHAC_OVERRIDEBIT0
>  #define EFI_ACPI_IORT_SMMUv3_FLAG_HTTU_OVERRIDE BIT1
> +#define EFI_ACPI_IORT_SMMUv3_FLAG_PROXIMITY_DOMAIN  BIT3
> +
> +#define EFI_ACPI_IORT_SMMUv3_MODEL_GENERIC  0x0
> +#define EFI_ACPI_IORT_SMMUv3_MODEL_HISILICON_HI161X 0x1
> +#define EFI_ACPI_IORT_SMMUv3_MODEL_CAVIUM_CN99XX0x2
>
>  #define EFI_ACPI_IORT_ROOT_COMPLEX_ATS_UNSUPPORTED  0x0
>  #define EFI_ACPI_IORT_ROOT_COMPLEX_ATS_SUPPORTED0x1
> @@ -165,7 +171,7 @@ typedef struct {
>  } EFI_ACPI_6_0_IO_REMAPPING_SMMU_NODE;
>
>  ///
> -/// Node type 4: SMMUv4 node
> +/// Node type 4: SMMUv3 node
>  ///
>  typedef struct {
>EFI_ACPI_6_0_IO_REMAPPING_NODE  Node;
> @@ -179,6 +185,9 @@ typedef struct {
>UINT32  Pri;
>UINT32  Gerr;
>UINT32  Sync;
> +  UINT8   ProximityDomain;
> +  UINT8   Reserved1[3];
> +  UINT32  DeviceIdMappingIndex;
>  } EFI_ACPI_6_0_IO_REMAPPING_SMMU3_NODE;
>
>  ///
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH v1] MdePkg: SMMUv3 updates for IORT table definitions

[Sami Mujawar]

Updated the IORT SMMUv3 Node structure and flags to match the
IO Remapping Table, Platform Design Document, Revision C dated
15 MAY 2017.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sami Mujawar 
Signed-off-by: Evan Lloyd 
---

The changes can be seen at 
https://github.com/samimujawar/edk2/tree/236_ioremapping_header_v1

Notes:
v1:
- Updated SMMUv3 node structure and associated flags. [SAMI]

 MdePkg/Include/IndustryStandard/IoRemappingTable.h | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Include/IndustryStandard/IoRemappingTable.h 
b/MdePkg/Include/IndustryStandard/IoRemappingTable.h
index 
c113afdd27843111bc7ad6e1de1108260fad2bbc..2e5cb45d7e2ffd4a0559ef706b71874843e3fdbd
 100644
--- a/MdePkg/Include/IndustryStandard/IoRemappingTable.h
+++ b/MdePkg/Include/IndustryStandard/IoRemappingTable.h
@@ -4,6 +4,7 @@
   
http://infocenter.arm.com/help/topic/com.arm.doc.den0049c/DEN0049C_IO_Remapping_Table.pdf
 
   Copyright (c) 2017, Linaro Limited. All rights reserved.
+  Copyright (c) 2018, ARM Limited. All rights reserved.
 
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD 
License
@@ -53,6 +54,11 @@
 
 #define EFI_ACPI_IORT_SMMUv3_FLAG_COHAC_OVERRIDEBIT0
 #define EFI_ACPI_IORT_SMMUv3_FLAG_HTTU_OVERRIDE BIT1
+#define EFI_ACPI_IORT_SMMUv3_FLAG_PROXIMITY_DOMAIN  BIT3
+
+#define EFI_ACPI_IORT_SMMUv3_MODEL_GENERIC  0x0
+#define EFI_ACPI_IORT_SMMUv3_MODEL_HISILICON_HI161X 0x1
+#define EFI_ACPI_IORT_SMMUv3_MODEL_CAVIUM_CN99XX0x2
 
 #define EFI_ACPI_IORT_ROOT_COMPLEX_ATS_UNSUPPORTED  0x0
 #define EFI_ACPI_IORT_ROOT_COMPLEX_ATS_SUPPORTED0x1
@@ -165,7 +171,7 @@ typedef struct {
 } EFI_ACPI_6_0_IO_REMAPPING_SMMU_NODE;
 
 ///
-/// Node type 4: SMMUv4 node
+/// Node type 4: SMMUv3 node
 ///
 typedef struct {
   EFI_ACPI_6_0_IO_REMAPPING_NODE  Node;
@@ -179,6 +185,9 @@ typedef struct {
   UINT32  Pri;
   UINT32  Gerr;
   UINT32  Sync;
+  UINT8   ProximityDomain;
+  UINT8   Reserved1[3];
+  UINT32  DeviceIdMappingIndex;
 } EFI_ACPI_6_0_IO_REMAPPING_SMMU3_NODE;
 
 ///
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


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

Re: [edk2] [PATCH 00/20] OvmfPkg: SEV: decrypt the initial SMRAM save state map for SMBASE relocation

[Brijesh Singh]

One more comment.


On 03/05/2018 08:44 AM, Brijesh Singh wrote: >> \
   -device 
scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=1 
\

   -netdev tap,fd=26,id=hostnet0,vhost=on,vhostfd=28 \
   -device 
virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:65:f7:fb,bus=pci.4,addr=0x0,rombar=0 
\


Please add iommu_platform=on,ats=on argument in virtio network. In order 
the SEV guest to work with virtio we need to use iommu_platform=on.




   -chardev pty,id=charserial0 \
   -device isa-serial,chardev=charserial0,id=serial0 \
   -device usb-tablet,id=input2,bus=usb.0,port=1 \
   -spice 
port=5900,addr=127.0.0.1,disable-ticketing,seamless-migration=on \

   -device cirrus-vga,id=video0,bus=pcie.0,addr=0x1 \
   -device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x0 \
   -global isa-debugcon.iobase=0x402 \
   -debugcon file:/tmp/from-brijesh.log \
   -fw_cfg name=opt/ovmf/PcdResizeXterm,string=y \
   -s \
   -object sev-guest,id=sev0,policy=0x0,cbitpos=47,reduced-phys-bits=5 \
   -machine memory-encryption=sev0 \
   -msg timestamp=on

Thanks,
Laszlo


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

Re: [edk2] [PATCH 00/20] OvmfPkg: SEV: decrypt the initial SMRAM save state map for SMBASE relocation

[Brijesh Singh]

Hi Laszlo,



On 03/05/2018 08:00 AM, Laszlo Ersek wrote:

On 03/02/18 14:17, Brijesh Singh wrote:

On 3/2/18 5:53 AM, Laszlo Ersek wrote:



Do you have (maybe updated) instructions for setting up the SEV host?
What are the latest bits that are expected to work together?



For host kernel:
- use recent kvm/master
- make sure following kernel config is enabled
   CONFIG_KVM_AMD_SEV
   CONFIG_CRYPTO_DEV_SP_PSP
   CONFIG_AMD_MEM_ENCRYPT
   CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT

For guest kernel:
  - you can use host kernel or anything >=4.15
     make sure you have following config enabled in kernel:
       CONFIG_AMD_MEM_ENCRYPT

For qemu:
- v10 patches from this branch
https://github.com/codomania/qemu/tree/v10


QEMU exits with the following error for me:

2018-03-05T13:40:12.478835Z qemu-system-x86_64: sev_ram_block_added: failed to 
register region (0x7f3df3e0+0x2)
2018-03-05T13:40:12.489183Z qemu-system-x86_64: sev_ram_block_added: failed to 
register region (0x7f3ffaa0+0x37c000)
2018-03-05T13:40:12.497580Z qemu-system-x86_64: sev_ram_block_added: failed to 
register region (0x7f3ffa80+0x2)
2018-03-05T13:40:12.504485Z qemu-system-x86_64: sev_launch_update_data: 
LAUNCH_UPDATE ret=-12 fw_error=0 ''
2018-03-05T13:40:12.504493Z qemu-system-x86_64: failed to encrypt pflash rom

Here's my full QEMU command line (started by libvirt) -- this command line does 
not restrict pflash access to guest code that runs in SMM, and correspondingly, 
the OVMF build lacks SMM_REQUIRE:



Are you launching guest as a normal users or root ? If you are launching 
guest as normal user then please make sure you have increased the 'max 
locked memory' limit. The register region function will try to pin the 
memory, while doing so we check the limit and if requested size is 
greater than ulimit then we fail.



# ulimit -a
core file size  (blocks, -c) unlimited
data seg size   (kbytes, -d) unlimited
scheduling priority (-e) 0
file size   (blocks, -f) unlimited
pending signals (-i) 966418
max locked memory   (kbytes, -l) 1024
max memory size (kbytes, -m) unlimited
open files  (-n) 1024
pipe size(512 bytes, -p) 8
POSIX message queues (bytes, -q) 819200
real-time priority  (-r) 0
stack size  (kbytes, -s) 8192
cpu time   (seconds, -t) unlimited
max user processes  (-u) 966418
virtual memory  (kbytes, -v) unlimited
file locks  (-x) unlimited

If QEMU command is still failing for you then can you please share your 
kernel dmesg. thanks




/opt/qemu-installed/bin/qemu-system-x86_64 \
   -name guest=from-brijesh,debug-threads=on \
   -S \
   -object 
secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-4-from-brijesh/master-key.aes
 \
   -machine pc-q35-2.10,accel=kvm,usb=off,smm=on,dump-guest-core=off \
   -cpu host \
   -drive 
file=/home/virt-images/OVMF_CODE.4m.fd,if=pflash,format=raw,unit=0,readonly=on \
   -drive 
file=/var/lib/libvirt/qemu/nvram/from-brijesh_VARS.fd,if=pflash,format=raw,unit=1
 \
   -m 8192 \
   -realtime mlock=off \
   -smp 1,sockets=1,cores=1,threads=1 \
   -uuid e2373f13-f481-4008-88d0-d61fa9da16fe \
   -no-user-config \
   -nodefaults \
   -chardev 
socket,id=charmonitor,path=/var/lib/libvirt/qemu/domain-4-from-brijesh/monitor.sock,server,nowait
 \
   -mon chardev=charmonitor,id=monitor,mode=control \
   -rtc base=utc \
   -no-shutdown \
   -boot strict=on \
   -device 
pcie-root-port,port=0x10,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,addr=0x2
 \
   -device pcie-root-port,port=0x11,chassis=2,id=pci.2,bus=pcie.0,addr=0x2.0x1 \
   -device pcie-root-port,port=0x12,chassis=3,id=pci.3,bus=pcie.0,addr=0x2.0x2 \
   -device pcie-root-port,port=0x13,chassis=4,id=pci.4,bus=pcie.0,addr=0x2.0x3 \
   -device nec-usb-xhci,id=usb,bus=pci.1,addr=0x0 \
   -device virtio-scsi-pci,iommu_platform=on,ats=on,id=scsi0,bus=pci.3,addr=0x0 
\
   -drive 
file=/var/lib/libvirt/images/rhel-7-server.qcow2,format=qcow2,if=none,id=drive-scsi0-0-0-0,cache=writeback,discard=unmap,werror=enospc
 \
   -device 
scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=1
 \
   -netdev tap,fd=26,id=hostnet0,vhost=on,vhostfd=28 \
   -device 
virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:65:f7:fb,bus=pci.4,addr=0x0,rombar=0
 \
   -chardev pty,id=charserial0 \
   -device isa-serial,chardev=charserial0,id=serial0 \
   -device usb-tablet,id=input2,bus=usb.0,port=1 \
   -spice port=5900,addr=127.0.0.1,disable-ticketing,seamless-migration=on \
   -device cirrus-vga,id=video0,bus=pcie.0,addr=0x1 \
   -device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x0 \
   -global isa-debugcon.iobase=0x402 \
   -debugcon file:/tmp/from-brijesh.log \
   -fw_cfg name=opt/ovmf/PcdResizeXterm,string=y \
   -s \
   -object sev-guest,id=sev0,policy=0x0,cbitpos=47,reduced-phys-bits=5 \
   -machine 

Re: [edk2] [Patch V3] DSC spec: Add flexible PCD value format into spec

[Gao, Liming]

Reviewed-by: Liming Gao 

>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Yonghong Zhu
>Sent: Monday, February 26, 2018 4:05 PM
>To: edk2-devel@lists.01.org
>Cc: Kinney, Michael D ; Shaw, Kevin W
>; Gao, Liming 
>Subject: [edk2] [Patch V3] DSC spec: Add flexible PCD value format into spec
>
>V3: Update the Pcd value format in [Components] section
>V2: update EBNF for Array format.
>
>Cc: Liming Gao 
>Cc: Michael Kinney 
>Cc: Kevin W Shaw 
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Yonghong Zhu 
>---
> 3_edk_ii_dsc_file_format/310_pcd_sections.md   | 160 +---
>-
> .../311_[components]_sections.md   |  23 +--
> .../33_platform_dsc_definition.md  |  78 +++---
> 3 files changed, 174 insertions(+), 87 deletions(-)
>
>diff --git a/3_edk_ii_dsc_file_format/310_pcd_sections.md
>b/3_edk_ii_dsc_file_format/310_pcd_sections.md
>index 2af42cc..18a243d 100644
>--- a/3_edk_ii_dsc_file_format/310_pcd_sections.md
>+++ b/3_edk_ii_dsc_file_format/310_pcd_sections.md
>@@ -98,13 +98,11 @@ is permissible to list multiple architectures in a single
>method section as in:
> It is permissible to list a PCD in a common architecture section and also list
> it in an architecturally modified section. In this case, the value in the
> architectural section overrides the value specified in the common section.
>
> The PCD values must match the datum type declared for a given PCD in the
>DEC
>-file. While a PCD of datum type `BOOLEAN` is permitted to have a `1` or a `0`
>-(instead of TRUE or FALSE) in the value field, a PCD of type UINT* cannot use
>-`TRUE` or `FALSE` for values.
>+file.
>
> PCDs with a data type of `VOID`* can optionally provide the maximum size of
>the
> value. If not provided, the maximum length will be calculated as the largest 
> of
> the size of the data in the DSC file, the size of the data in the INF file or
> the size of the data in the DEC file that declares the PCD.
>@@ -220,21 +218,24 @@ fields that are separated by the pipe character, "|".
> ::=  [ ]*
>::= 
> ::= 
>  ::= {} {} {}
>   ::=   [ ] 
>-  ::= if (pcddatumtype == "BOOLEAN"): {}
>{}
>-elif (pcddatumtype == "UINT8"): {}
>-{} elif (pcddatumtype == "UINT16"):
>-{} {} elif (pcddatumtype ==
>-"UINT32"): {} {} elif
>-(pcddatumtype == "UINT64"): {} 
>{}
>+  ::= if (pcddatumtype == "BOOLEAN"):
>+  {} {}
>+elif (pcddatumtype == "UINT8"):
>+  {} {}
>+elif (pcddatumtype == "UINT16"):
>+  {} {}
>+elif (pcddatumtype == "UINT32"):
>+  {} {}
>+elif (pcddatumtype == "UINT64"):
>+  {} {}
> else:
>- []
>+   []
>::=  "VOID*"  {} {}
>-   ::= {} {} {}
>-{} {}
>+   ::= {} {} {}
> ```
>
>  Parameters
>
> **_Expression_**
>@@ -325,21 +326,24 @@ of the DSC file.
> ::=  [ ]*
>::= 
> ::= 
>  ::= {} {}
>{}
>   ::=   [ ] 
>-  ::= if (pcddatumtype == "BOOLEAN"): {}
>{}
>-elif (pcddatumtype == "UINT8"): {}
>-{} elif (pcddatumtype == "UINT16"):
>-{} {} elif (pcddatumtype ==
>-"UINT32"): {} {} elif
>-(pcddatumtype == "UINT64"): {} 
>{}
>+  ::= if (pcddatumtype == "BOOLEAN"):
>+  {} {}
>+elif (pcddatumtype == "UINT8"):
>+  {} {}
>+elif (pcddatumtype == "UINT16"):
>+  {} {}
>+elif (pcddatumtype == "UINT32"):
>+  {} {}
>+elif (pcddatumtype == "UINT64"):
>+  {} {}
> else:
>- []
>+   []
>::=  {} {}
>-   ::= {} {} {}
>-{} {}
>+   ::= {} {} {}
> ```
>
>  Parameters
>
> **_Expression_**
>@@ -458,41 +462,58 @@ sections of the DSC file.
>   ::= "."  ["." ]
>  ::=  [ ]*
> ::= 
>  ::= 
>::=  [ ] 
>-   ::= if (pcddatumtype == "BOOLEAN"): {}
>{}
>- elif (pcddatumtype == "UINT8"): {}
>- {} elif (pcddatumtype == "UINT16"):
>- {} {} elif (pcddatumtype ==
>- "UINT32"): {} {} elif
>- (pcddatumtype == "UINT64"): {}
>- {} else:
>-  []
>+   ::= if (pcddatumtype == "BOOLEAN"):
>+ 

Re: [edk2] [Patch V2] Build spec: Add flexible PCD value format into spec

[Gao, Liming]

Reviewed-by: Liming Gao 

>-Original Message-
>From: Zhu, Yonghong
>Sent: Wednesday, February 07, 2018 8:36 PM
>To: edk2-devel@lists.01.org
>Cc: Gao, Liming ; Kinney, Michael D
>; Shaw, Kevin W 
>Subject: [Patch V2] Build spec: Add flexible PCD value format into spec
>
>V2: update EBNF for Array format.
>
>Cc: Liming Gao 
>Cc: Michael Kinney 
>Cc: Kevin W Shaw 
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Yonghong Zhu 
>---
> 7_build_environment/73_guided_tools.md |  8 +--
> .../82_auto-generation_process.md  | 18 +++---
> appendix_d_buildexe_command/d4_usage.md| 72
>+++---
> 3 files changed, 77 insertions(+), 21 deletions(-)
>
>diff --git a/7_build_environment/73_guided_tools.md
>b/7_build_environment/73_guided_tools.md
>index a8881d3..50119f1 100644
>--- a/7_build_environment/73_guided_tools.md
>+++ b/7_build_environment/73_guided_tools.md
>@@ -119,21 +119,21 @@ file required by the build system is provided in the
>appendix, VPD Tool.
> ```
>
>   If using automatic offset feature, the build tools byte-align numeric 
> values,
>   while `VOID*` PCD types will be aligned using the following rules:
>
>-  * ASCII strings, "string", will be byte aligned.
>-  * Unicode strings, L"string" will be two-byte aligned.
>+  * ASCII strings, "string" or 'string', will be byte aligned.
>+  * Unicode strings, L"string" or L'string' will be two-byte aligned.
>   * Byte arrays, {0x00, 0x01} will be 8-byte aligned.
>
>   If the developer manually assigns offset values in the DSC file, the 
> developer
>   must follow the same rules.
>
>   **
>   **Note:** If a developer manually sets the offset of a `VOID*` PCD with
>-  Unicode string, L"string", style to a value that is not 2-byte aligned, then
>-  an error is generated and the build halts.
>+  Unicode string, L"string"/L'string' style to a value that is not 2-byte 
>aligned,
>+  then an error is generated and the build halts.
>   **
>   **Note:** If a developer manually sets the offset of a `VOID*` PCD with
>byte
>   array {} style to a value that is not 8-byte aligned, then a warning is
>   generated, but the build will continue.
>   **
>diff --git a/8_pre-build_autogen_stage/82_auto-generation_process.md
>b/8_pre-build_autogen_stage/82_auto-generation_process.md
>index f610185..5a950d7 100644
>--- a/8_pre-build_autogen_stage/82_auto-generation_process.md
>+++ b/8_pre-build_autogen_stage/82_auto-generation_process.md
>@@ -875,41 +875,41 @@ A PCD value set on the command-line has the
>highest precedence. It overrides
> all instances of the PCD value specified in the DSC or FDF file. The following
> is the syntax to override the value of a PCD on the command line:
>
> `--pcd [.]=`
>
>-For `VOID*` type PCDs, `` supports the following syntax:
>+`` supports the following syntax:
>
>-* ASCII string value for a `VOID*` PCD
>+* ASCII string value for a PCD
>
>   `--pcd  [.]="String"`
>+  `--pcd  [.]='String'`
>
>-*  Unicode string value for a `VOID*` PCD
>+* Unicode string value for a PCD
>
>   `--pcd  [.]=L"String"`
>+  `--pcd  [.]=L'String'`
>
>-*  Byte array value for a `VOID*` PCD
>+* Byte array value for a PCD
>
>   `--pcd  [.]= H"{0x1, 0x2}"`
>
> **
> **Note:** The EDK II meta-data specs have changed to permit a PCD entry
>(or any
> other entry) to be listed only one time per section.
> **
>-**Caution:** Dynamic and DynamicEx `VOID*` VPD PCD array values must be
>hex byte
>-arrays. Using a Registry or C format GUID value in the value field of a 
>`VOID*`
>-VPD PCD is not permitted.
>-**
>
> If the maximum size of a `VOID*` PCD is not specified in the DSC file, then 
> the
> maximum size is calculated based on the largest size of 1) the string or array
> in the DSC file, 2) the string or array in the INF file and 3) the string or
> array in the DEC file. If the value is a quoted text string, the size of the
> string will be incremented by one to handle string termination. If the quoted
> string is preceded by L, as in `L"This is a string"`, then the size of the 
> string
> will be incremented by two to handle unicode string termination. If the value
>-is a byte array, then the size of the byte array is not modified.
>+is a byte array, then the size of the byte array is not modified. If the 
>value is
>+a single quoted string, as in 'string' or L'string', the size of the string 
>doesn't
>+need to include string null termination character.
>
> For example, if the string in the DSC file is `L"DSC Length"`, the INF file 
> has
> `L"Module Length"` and the DEC file declares the default as `L"Length"`, then
> the maximum size that will be allocated for this PCD will be 28 bytes (`
> L"Module Length"` 26 bytes, 2 bytes for null termination 

Re: [edk2] [Patch V2] Expression spec: update format to support flexible Pcd format

[Gao, Liming]

Reviewed-by: Liming Gao 

>-Original Message-
>From: Zhu, Yonghong
>Sent: Wednesday, February 07, 2018 8:36 PM
>To: edk2-devel@lists.01.org
>Cc: Gao, Liming ; Kinney, Michael D
>; Shaw, Kevin W 
>Subject: [Patch V2] Expression spec: update format to support flexible Pcd
>format
>
>V2: update EBNF for Array format.
>
>Cc: Liming Gao 
>Cc: Michael Kinney 
>Cc: Kevin W Shaw 
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Yonghong Zhu 
>---
> 2_expression_overview.md |  3 +++
> 3_expression_format.md   | 50
>+++-
> 2 files changed, 48 insertions(+), 5 deletions(-)
>
>diff --git a/2_expression_overview.md b/2_expression_overview.md
>index 34ceb97..c29a632 100644
>--- a/2_expression_overview.md
>+++ b/2_expression_overview.md
>@@ -100,5 +100,8 @@ directives section do not apply.
> 12. Logical operators require operands that are type scalar.
>
> 13. For the Conditional Operator, the first operand must be scalar, while the
> second and third operands must have the same type (i.e., both being 
> scalar,
> both being integers, or both being string literals).
>+
>+14. Array format like "{0x10, 0x20}" can't be a operand of any operator
>except
>+Relational and equality operators.
>diff --git a/3_expression_format.md b/3_expression_format.md
>index 50b94c4..1f90fc9 100644
>--- a/3_expression_format.md
>+++ b/3_expression_format.md
>@@ -93,10 +93,21 @@ GRACEFULLY._
>  ::= 
> ::=  []+
>   ::= (a-fA-F0-9)
>  ::= {"0x"} {"0X"}
>  ::= {} {}
>+ ::=  (\x0 - \xFF)
>+::=  (\x0 - \x)
>+::=  (\x0 - \x)
>+::=  (\x0 - \x)
>+  ::= (0-255)
>+::= (0-65535)
>+   ::= (0-4294967295)
>+   ::= (0-18446744073709551615)
>+   ::= "GUID("  ")"
>+   ::= {  }
>+{} {}
> Rhex2   ::= [] 
> Rhex4   ::= [] [] Rhex2
> Rhex8   ::= [] [] [] 
> [] Rhex4
>::= Rghex8 "-" Rghex4 "-" Rghex4 "-" Rghex4 "-" 
> Rghex12
> Rghex2  ::=  
>@@ -111,19 +122,42 @@ Rghex12 ::=   
>
> Rghex8
>::= "{" []* 
>  ::=   "{" []*   
>  ::=   
>  ::=   
>  ::=  []* "}" []* "}"
>- ::= {} {}
>+ ::= {} {}
> ::= "{" * "}"
>- ::= "{" *  [ ]* "}"
>+ ::= "{" * [] 
>+ [ [] ]* "}"
>+  ::= {} {} {}
>+::= "DEVICE_PATH("  ")"
>+ ::= A double quoted string that follow the device path
>+as string format defined in UEFI Specification 2.6
>+Section 9.6
>+ ::= "LABEL("  ")"
>+::= "OFFSET_OF("  ")"
>  ::= {} {"L" }
>+{} {"L" }
>   ::= 0x22
>+  ::= 0x27
>   ::=  []* 
>+   ::=  []* 
>  ::= {} {}
>-   ::= {0x20} {0x21} {(0x23 - 0x5B)} {(0x5D - 0x7E)}
>- ::= "\" {"n"} {"r"} {"t"} {"f"} {"b"} {"0"} {"\"}
>{}
>+   ::= {0x21} {(0x23 - 0x26)} {(0x28 - 0x5B)}
>+{(0x5D - 0x7E)} {}
>+ ::= "\" {"n"} {"r"} {"t"} {"f"} {"b"} {"0"} {"\"}
>+{} {}
>+::= {} {} {}
>{}
>+ ::= {} {} {}
>+::= {} {} {}
>+::= {} {} {}
>+::= {} {} {}
>+   ::= {} {} {}
>{}
>+  ::= "UINT8("  ")"
>+ ::= "UINT16("  ")"
>+ ::= "UINT32("  ")"
>+ ::= "UINT64("  ")"
>  ::= {} {}
>::=  "." 
>::= {} {
>}
>::= {} {}
> ::= {"+"} {"-"} {"~"}
>@@ -187,13 +221,19 @@
>gUefiCpuPkgTokenSpaceGuid.PcdCpuLocalApicBaseAddress
>| !gCrownBayTokenSpaceGuid.
>
> This is the value of the MACRO assigned in a DEFINE statement.
>
> **Expressions**
>
>-If the "|" character is used in an expression, the expression must be
>+If the "|" or "||"character is used in an expression, the expression must be
> encapsulated by parenthesis.
>
>+**OFFSET_OF()**
>+
>+LABEL() macro in byte arrays to tag the byte offset of a location in a byte
>+array. OFFSET_OF() macro in byte arrays that returns the byte offset of a
>+LABEL() declared in a byte array.
>+
> ## 3.2 Conditional Directive Expressions
>
> Conditional directive statements are defined in the EDK II Platform
>Description
> (DSC) File and Flash Definition (FDF) File. The following EBNF describes the
> format for expressions used in conditional directives. The 

Re: [edk2] [Patch V2] DEC spec: Add flexible PCD value format into spec

[Gao, Liming]

Reviewed-by: Liming Gao 

>-Original Message-
>From: Zhu, Yonghong
>Sent: Wednesday, February 07, 2018 8:36 PM
>To: edk2-devel@lists.01.org
>Cc: Gao, Liming ; Kinney, Michael D
>; Shaw, Kevin W 
>Subject: [Patch V2] DEC spec: Add flexible PCD value format into spec
>
>V2: update EBNF for Array format.
>
>Cc: Liming Gao 
>Cc: Michael Kinney 
>Cc: Kevin W Shaw 
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Yonghong Zhu 
>---
> 3_edk_ii_dec_file_format/310_pcd_sections.md   | 14 ++--
> .../32_package_declaration_dec_definitions.md  | 74
>++
> 2 files changed, 67 insertions(+), 21 deletions(-)
>
>diff --git a/3_edk_ii_dec_file_format/310_pcd_sections.md
>b/3_edk_ii_dec_file_format/310_pcd_sections.md
>index 488f17f..36c32ff 100644
>--- a/3_edk_ii_dec_file_format/310_pcd_sections.md
>+++ b/3_edk_ii_dec_file_format/310_pcd_sections.md
>@@ -113,24 +113,24 @@ PCDs listed in `PcdsFeatureFlag` sections must only
>be listed in
>   [ ]
>   [] 
> ::=  {} {} {}
>  ::= {} {} {} {}
>  ::=  
>- ::=   "BOOLEAN"
>+ ::=   "BOOLEAN"
>  ::= {} {}
> ::=  
>-::=   "UINT8"
>+::= {} {}  "UINT8"
>::=  
>-   ::=   "UINT16"
>+   ::= {} {}  "UINT16"
>::=  
>-   ::=   "UINT32"
>+   ::= {} {}  "UINT32"
>::=  
>-   ::=   "UINT64"
>+   ::= {} {}  "UINT64"
>   ::=  
>::=   "VOID*"
>-  ::= {} {}
>-   ::= 
>+  ::= {} {}
>+   ::= {} {}
>   ::=  {+} {} {+}
>   ::= "#"  "@Prompt   
>::= "#"  "@ValidRange"   
> ::= [ ] 
> ::= "#"  "@ValidList"   
>diff --git
>a/3_edk_ii_dec_file_format/32_package_declaration_dec_definitions.md
>b/3_edk_ii_dec_file_format/32_package_declaration_dec_definitions.md
>index 8e473f2..c701603 100644
>--- a/3_edk_ii_dec_file_format/32_package_declaration_dec_definitions.md
>+++
>b/3_edk_ii_dec_file_format/32_package_declaration_dec_definitions.md
>@@ -86,15 +86,16 @@ DEC file (for example, `` statements are
>not permitted).
>::= (0-9)
> ::= (a-zA-Z_)
>   ::=  *
>::=  # A valid C variable name.
>   ::= (0x21 - 0x7E)
>-  ::= [{0x21} {(0x23 - 0x5B)} {(0x5D - 0x7E)}
>-  {}]*
>+  ::= [{0x21} {(0x23 - 0x26)} {(0x28 - 0x5B)}
>+  {(0x5D - 0x7E)} {}]*
> ::= 0x22
>+::= 0x27
>   ::= "\" {"n"} {"t"} {"f"} {"r"} {"b"} {"0"} {"\"}
>-  {}
>+  {} {}
> ::= {} {}
>   ::= *
>  ::= +
>  ::= 0x09
>::= 0x20
>@@ -112,15 +113,16 @@ DEC file (for example, `` statements are
>not permitted).
>  ::= [ * * ]*
>  ::= 
>   ::= 
>   ::= {} {}
> ::=  * 
>- ::= ["L"] 
>+ ::=  * 
>+ ::= {} {}
> ::=  [{} {}]+ 
>::=  "#" [ ] +
>  ::= "#"   +
>-   ::= "L" 
>+   ::= "L" {} {}
> ::= (a-fA-F0-9)
>  ::= {"0x"} {"0X"} [] 
>::= {"0x"} {"0X"} +
>   ::= "0x" [0]*  
>::= ? ? ?
>@@ -157,11 +159,12 @@ DEC file (for example, `` statements are
>not permitted).
>   ::= {} {}
> ::= {"TRUE"} {"true"} {"True"} {"0x1"}
>   {"0x01"} {"1"}
>::= {"FALSE"} {"false"} {"False"} {"0x0"}
>   {"0x00"} {"0"}
>-::= {} {}
>+ ::= {} {}
>+::= {} {"{""}"}
>::= (A-Z)(A-Z0-9_)*
> ::= "$("  ")"
>  ::=  "." 
> ::= 
>  ::= 
>@@ -182,14 +185,49 @@ DEC file (for example, `` statements are
>not permitted).
>
> ::= (0-255)
>   ::= (0-65535)
>  ::= (0-4294967295)
>  ::= (0-18446744073709551615)
>- ::= {} {}
>-::= {} {}
>-::= {} {}
>-::= {} {}
>+::= {} {} {}
>+  {} {}
>+   ::= {} {} {}
>+  {} {}
>+   ::= {} {} {}
>+  {} {}
>+   ::= {} {} {}
>+  {} {}
>+ ::= {} {"{""}"}
>+::= {}
>+  {"{" [ ]*"}"}
>+::= {}
>+  {"{" [ ]*"}"}
>+::= {}
>+  {"{" [ ]*"}"}
>+   ::= {} {} {}
>+   ::= "{" {} {[] 
>+   [ [] ]* } "}"
>+::= {} {} {}
>+  ::= {}{} {}
>+ 

Re: [edk2] [Patch V2] INF spec: Add flexible PCD value format into spec

[Gao, Liming]

Reviewed-by: Liming Gao 

>-Original Message-
>From: Zhu, Yonghong
>Sent: Wednesday, February 07, 2018 8:36 PM
>To: edk2-devel@lists.01.org
>Cc: Gao, Liming ; Kinney, Michael D
>; Shaw, Kevin W 
>Subject: [Patch V2] INF spec: Add flexible PCD value format into spec
>
>V2: update EBNF for Array format.
>
>Cc: Liming Gao 
>Cc: Michael Kinney 
>Cc: Kevin W Shaw 
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Yonghong Zhu 
>---
> .../32_component_inf_definition.md | 75 +-
> 3_edk_ii_inf_file_format/38_pcd_sections.md| 39 ++-
> 2 files changed, 83 insertions(+), 31 deletions(-)
>
>diff --git a/3_edk_ii_inf_file_format/32_component_inf_definition.md
>b/3_edk_ii_inf_file_format/32_component_inf_definition.md
>index 72bcf2c..ef70927 100644
>--- a/3_edk_ii_inf_file_format/32_component_inf_definition.md
>+++ b/3_edk_ii_inf_file_format/32_component_inf_definition.md
>@@ -111,15 +111,16 @@ The following are common definitions used by
>multiple section types.
>   ::= (0-9)
>::= (a-zA-Z_)
>  ::=  *
>   ::=  # A valid C variable name.
>  ::= (0x21 - 0x7E)
>- ::= [{0x21} {(0x23 - 0x5B)} {(0x5D - 0x7E)}
>- {}]*
>+ ::= [{0x21} {(0x23 - 0x26)} {(0x28 - 0x5B)}
>+ {(0x5D - 0x7E)} {}]*
>::= 0x22
>+   ::= 0x27
>  ::= "\" {"n"} {"t"} {"f"} {"r"} {"b"} {"0"} {"\"}
>- {}
>+ {} {}
>::= {} {}
>  ::= *
> ::= +
> ::= 0x09
>   ::= 0x20
>@@ -137,15 +138,16 @@ The following are common definitions used by
>multiple section types.
> ::= [ * * ]*
> ::= 
>  ::= 
>  ::= {} {}
>::=  * 
>-::= ["L"] 
>+::=  * 
>+::= {} {}
>::=  [{} {}]+ 
>   ::=  "#" [] +
> ::= "#"  +
>-  ::= "L" 
>+  ::= "L" {} {}
>::= (a-fA-F0-9)
> ::= {"0x"} {"0X"}  
>   ::= {"0x"} {"0X"} *
>  ::= "0x"  
>   ::= ? ? ?
>@@ -181,14 +183,14 @@ The following are common definitions used by
>multiple section types.
>   ::= {} {}
>::= (\x1 - \x)
>::= (1-18446744073709551615)
>   ::= {} {}
> ::= {"TRUE"} {"true"} {"True"} {"0x1"} {"0x01"} {"1"}
>-  {}
>::= {"FALSE"} {"false"} {"False"} {"0x0"}
>   {"0x00"} {"0"}
>-::= {} {}
>+ ::= {} {}
>+::= {} {"{""}"}
>::= (A-Z)(A-Z0-9_)*
> ::= "$("  ")"
>  ::=  "." 
> ::= 
>  ::= 
>@@ -209,14 +211,49 @@ The following are common definitions used by
>multiple section types.
>
> ::= (0-255)
>   ::= (0-65535)
>  ::= (0-4294967295)
>  ::= (0-18446744073709551615)
>- ::= {} {}
>-::= {} {}
>-::= {} {}
>-::= {} {}
>+::= {} {} {}
>+  {} {}
>+   ::= {} {} {}
>+  {} {}
>+   ::= {} {} {}
>+  {} {}
>+   ::= {} {} {}
>+  {} {}
>+ ::= {} {"{""}"}
>+::= {}
>+  {"{" [ ]*"}"}
>+::= {}
>+  {"{" [ ]*"}"}
>+::= {}
>+  {"{" [ ]*"}"}
>+   ::= {} {} {}
>+   ::= "{" {} {[] 
>+   [ [] ]* } "}"
>+::= {} {} {}
>+  ::= {} {} {}
>+  {} {}
>+   ::= {} {} {}
>+  ::= {} {} {}
>+  ::= {} {} {}
>+  ::= {} {} {}
>+ ::= "GUID("  ")"
>+ ::= {  }
>+  {} {}
>+  ::= "DEVICE_PATH("  ")"
>+   ::= A double quoted string that follow the device path
>+  as string format defined in UEFI Specification 2.6
>+  Section 9.6
>+ ::= {} {} {}
>{}
>+::= "UINT8("  ")"
>+   ::= "UINT16("  ")"
>+   ::= "UINT32("  ")"
>+   ::= "UINT64("  ")"
>+   ::= "LABEL("  ")"
>+  ::= "OFFSET_OF("  ")"
>   ::= {"BASE"} {"SEC"} {"PEI_CORE"} {"PEIM"}
>   {"DXE_CORE"} {"DXE_DRIVER"} {"SMM_CORE"}
>   {"DXE_RUNTIME_DRIVER"} {"DXE_SAL_DRIVER"}
>   {"DXE_SMM_DRIVER"} {"UEFI_DRIVER"}
>   {"UEFI_APPLICATION"} {"USER_DEFINED"}
>@@ -233,10 +270,18 @@ The following are 

[edk2] [PATCH] IntelSiliconPkg VTdPmrPei: Add PcdVTdPeiDmaBufferSize(S3)

[Star Zeng]

Add PcdVTdPeiDmaBufferSize(S3) to replace the hard coded value
TOTAL_DMA_BUFFER_SIZE and TOTAL_DMA_BUFFER_SIZE_S3 in IntelVTdPmrPei.

Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 .../Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c|  7 ++-
 .../Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.inf  |  4 +++-
 IntelSiliconPkg/IntelSiliconPkg.dec| 18 +-
 3 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c 
b/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c
index 6289834fcb38..9a0138b3b086 100644
--- a/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c
+++ b/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c
@@ -29,9 +29,6 @@
 
 #include "IntelVTdPmrPei.h"
 
-#define  TOTAL_DMA_BUFFER_SIZESIZE_4MB
-#define  TOTAL_DMA_BUFFER_SIZE_S3 SIZE_2MB
-
 EFI_GUID mVTdInfoGuid = {
   0x222f5e30, 0x5cd, 0x49c6, { 0x8a, 0xc, 0x36, 0xd6, 0x58, 0x41, 0xe0, 0x82 }
 };
@@ -798,9 +795,9 @@ IntelVTdPmrInitialize (
   PeiServicesGetBootMode ();
 
   if (BootMode == BOOT_ON_S3_RESUME) {
-DmaBufferInfo->DmaBufferSize = TOTAL_DMA_BUFFER_SIZE_S3;
+DmaBufferInfo->DmaBufferSize = PcdGet32 (PcdVTdPeiDmaBufferSizeS3);
   } else {
-DmaBufferInfo->DmaBufferSize = TOTAL_DMA_BUFFER_SIZE;
+DmaBufferInfo->DmaBufferSize = PcdGet32 (PcdVTdPeiDmaBufferSize);
   }
 
   Status = PeiServicesNotifyPpi ();
diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.inf 
b/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.inf
index e6d0323acc50..5b688d5cbf9f 100644
--- a/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.inf
+++ b/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.inf
@@ -4,7 +4,7 @@
 # This driver initializes VTd engine based upon EDKII_VTD_INFO_PPI
 # and provide DMA protection in PEI.
 #
-# Copyright (c) 2017, Intel Corporation. All rights reserved.
+# Copyright (c) 2017 - 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
@@ -54,6 +54,8 @@ [Ppis]
 
 [Pcd]
   gIntelSiliconPkgTokenSpaceGuid.PcdVTdPolicyPropertyMask   ## CONSUMES
+  gIntelSiliconPkgTokenSpaceGuid.PcdVTdPeiDmaBufferSize ## CONSUMES
+  gIntelSiliconPkgTokenSpaceGuid.PcdVTdPeiDmaBufferSizeS3   ## CONSUMES
 
 [Depex]
   gEfiPeiMasterBootModePpiGuid AND
diff --git a/IntelSiliconPkg/IntelSiliconPkg.dec 
b/IntelSiliconPkg/IntelSiliconPkg.dec
index a15d3dee392c..c0cf58fa6cb5 100644
--- a/IntelSiliconPkg/IntelSiliconPkg.dec
+++ b/IntelSiliconPkg/IntelSiliconPkg.dec
@@ -3,7 +3,7 @@
 #
 # This package provides common open source Intel silicon modules.
 #
-# Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.
+# Copyright (c) 2016 - 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.
 # The full text of the license may be found at
@@ -61,3 +61,19 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, 
PcdsDynamicEx]
   # @Prompt The policy for VTd driver behavior.
   gIntelSiliconPkgTokenSpaceGuid.PcdVTdPolicyPropertyMask|1|UINT8|0x0002
 
+  ## Declares VTd PEI DMA buffer size.
+  #  When this PCD value is referred by platform to calculate the required
+  #  memory size for PEI (InstallPeiMemory), the PMR alignment requirement
+  #  needs be considered to be added with this PCD value for alignment
+  #  adjustment need by AllocateAlignedPages.
+  # @Prompt The VTd PEI DMA buffer size.
+  
gIntelSiliconPkgTokenSpaceGuid.PcdVTdPeiDmaBufferSize|0x0040|UINT32|0x0003
+
+  ## Declares VTd PEI DMA buffer size for S3.
+  #  When this PCD value is referred by platform to calculate the required
+  #  memory size for PEI S3 (InstallPeiMemory), the PMR alignment requirement
+  #  needs be considered to be added with this PCD value for alignment
+  #  adjustment need by AllocateAlignedPages.
+  # @Prompt The VTd PEI DMA buffer size for S3.
+  
gIntelSiliconPkgTokenSpaceGuid.PcdVTdPeiDmaBufferSizeS3|0x0020|UINT32|0x0004
+
-- 
2.13.3.windows.1

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

Re: [edk2] [PATCH 0/3] BaseTools: let the C-language build utils compile with gcc-8

[Gao, Liming]

Reviewed-by: Liming Gao 

>-Original Message-
>From: Laszlo Ersek [mailto:ler...@redhat.com]
>Sent: Saturday, March 03, 2018 2:09 AM
>To: edk2-devel-01 
>Cc: Ard Biesheuvel ; Cole Robinson
>; Gao, Liming ; Paolo Bonzini
>; Zhu, Yonghong 
>Subject: [PATCH 0/3] BaseTools: let the C-language build utils compile with
>gcc-8
>
>Repo:   https://github.com/lersek/edk2.git
>Branch: basetools_gcc8
>
>Once these patches are applied to the build flags and the source code of
>the build utilities themselves, OVMF builds fine with gcc-8, using the
>GCC5 toolchain settings without any changes.
>
>Regression-tested with gcc-4.8 / x86_64.
>
>Cc: Ard Biesheuvel 
>Cc: Cole Robinson 
>Cc: Liming Gao 
>Cc: Paolo Bonzini 
>Cc: Yonghong Zhu 
>
>Thanks
>Laszlo
>
>Laszlo Ersek (3):
>  BaseTools/header.makefile: add "-Wno-stringop-truncation"
>  BaseTools/header.makefile: add "-Wno-restrict"
>  BaseTools/GenVtf: silence false "stringop-overflow" warning with
>memcpy()
>
> BaseTools/Source/C/Makefiles/header.makefile | 4 ++--
> BaseTools/Source/C/GenVtf/GenVtf.c   | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
>--
>2.14.1.3.gb7cf6e02401b

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

Re: [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop

[Marc-André Lureau]

Hi

On Fri, Feb 23, 2018 at 8:45 PM, Andrew Fish  wrote:
>
>
>> On Feb 23, 2018, at 5:23 AM, marcandre.lur...@redhat.com wrote:
>>
>> From: Marc-André Lureau 
>>
>> Without this hack, GetNextHob() loops infinitely with the next patch.
>> I don't understand the reason.
>>
>> The loop is triggered by the GetFirstGuidHob () call.
>>
>> CC: Laszlo Ersek 
>> CC: Stefan Berger 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Marc-André Lureau 
>> ---
>> MdePkg/Library/PeiHobLib/HobLib.c | 4 
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/MdePkg/Library/PeiHobLib/HobLib.c 
>> b/MdePkg/Library/PeiHobLib/HobLib.c
>> index 5c0eeb992f..ed3c5fbd6d 100644
>> --- a/MdePkg/Library/PeiHobLib/HobLib.c
>> +++ b/MdePkg/Library/PeiHobLib/HobLib.c
>> @@ -89,6 +89,10 @@ GetNextHob (
>> if (Hob.Header->HobType == Type) {
>>   return Hob.Raw;
>> }
>> +if (GET_HOB_LENGTH (HobStart) == 0) {
>
> As Laszlo points out this error condition is likely memory corruption. Thus 
> it would be better to check for all know illegal values?
>
> if (GET_HOB_LENGTH(HobStart) < sizeof (EFI_HOB_GENERIC_HEADER)
>

Thanks, I have adjusted the check.

With manual calls and printf (I don't know  a better way to debug ovmf
;), I try to locate the issue. It's somehow related to
RegisterForShadow(). The "corruption" seems to happen during the
second call. After the
PeiLoadImage(...,PEIM_STATE_REGISTER_FOR_SHADOW,..), right before
calling PeimEntryPoint(), a GetFirstGuidHob() succeed, but inside the
function, it fails (with the same arguments). Right after it succeeds
again... The PeimEntryPoint() is not the Tcg2Pei:PeimEntryMA(), I
suppose there is some kind of wrapping code, but I fail to find where.
Any idea?

thanks for your help

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

Re: [edk2] [PATCH 00/20] OvmfPkg: SEV: decrypt the initial SMRAM save state map for SMBASE relocation

[Laszlo Ersek]

On 03/02/18 14:17, Brijesh Singh wrote:
> On 3/2/18 5:53 AM, Laszlo Ersek wrote:

>> Do you have (maybe updated) instructions for setting up the SEV host?
>> What are the latest bits that are expected to work together?

> For host kernel:
> - use recent kvm/master
> - make sure following kernel config is enabled
>   CONFIG_KVM_AMD_SEV
>   CONFIG_CRYPTO_DEV_SP_PSP
>   CONFIG_AMD_MEM_ENCRYPT
>   CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT
> 
> For guest kernel:
>  - you can use host kernel or anything >=4.15
>     make sure you have following config enabled in kernel:
>       CONFIG_AMD_MEM_ENCRYPT
> 
> For qemu:
> - v10 patches from this branch
> https://github.com/codomania/qemu/tree/v10

QEMU exits with the following error for me:

2018-03-05T13:40:12.478835Z qemu-system-x86_64: sev_ram_block_added: failed to 
register region (0x7f3df3e0+0x2)
2018-03-05T13:40:12.489183Z qemu-system-x86_64: sev_ram_block_added: failed to 
register region (0x7f3ffaa0+0x37c000)
2018-03-05T13:40:12.497580Z qemu-system-x86_64: sev_ram_block_added: failed to 
register region (0x7f3ffa80+0x2)
2018-03-05T13:40:12.504485Z qemu-system-x86_64: sev_launch_update_data: 
LAUNCH_UPDATE ret=-12 fw_error=0 ''
2018-03-05T13:40:12.504493Z qemu-system-x86_64: failed to encrypt pflash rom

Here's my full QEMU command line (started by libvirt) -- this command line does 
not restrict pflash access to guest code that runs in SMM, and correspondingly, 
the OVMF build lacks SMM_REQUIRE:

/opt/qemu-installed/bin/qemu-system-x86_64 \
  -name guest=from-brijesh,debug-threads=on \
  -S \
  -object 
secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-4-from-brijesh/master-key.aes
 \
  -machine pc-q35-2.10,accel=kvm,usb=off,smm=on,dump-guest-core=off \
  -cpu host \
  -drive 
file=/home/virt-images/OVMF_CODE.4m.fd,if=pflash,format=raw,unit=0,readonly=on \
  -drive 
file=/var/lib/libvirt/qemu/nvram/from-brijesh_VARS.fd,if=pflash,format=raw,unit=1
 \
  -m 8192 \
  -realtime mlock=off \
  -smp 1,sockets=1,cores=1,threads=1 \
  -uuid e2373f13-f481-4008-88d0-d61fa9da16fe \
  -no-user-config \
  -nodefaults \
  -chardev 
socket,id=charmonitor,path=/var/lib/libvirt/qemu/domain-4-from-brijesh/monitor.sock,server,nowait
 \
  -mon chardev=charmonitor,id=monitor,mode=control \
  -rtc base=utc \
  -no-shutdown \
  -boot strict=on \
  -device 
pcie-root-port,port=0x10,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,addr=0x2
 \
  -device pcie-root-port,port=0x11,chassis=2,id=pci.2,bus=pcie.0,addr=0x2.0x1 \
  -device pcie-root-port,port=0x12,chassis=3,id=pci.3,bus=pcie.0,addr=0x2.0x2 \
  -device pcie-root-port,port=0x13,chassis=4,id=pci.4,bus=pcie.0,addr=0x2.0x3 \
  -device nec-usb-xhci,id=usb,bus=pci.1,addr=0x0 \
  -device virtio-scsi-pci,iommu_platform=on,ats=on,id=scsi0,bus=pci.3,addr=0x0 \
  -drive 
file=/var/lib/libvirt/images/rhel-7-server.qcow2,format=qcow2,if=none,id=drive-scsi0-0-0-0,cache=writeback,discard=unmap,werror=enospc
 \
  -device 
scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=1
 \
  -netdev tap,fd=26,id=hostnet0,vhost=on,vhostfd=28 \
  -device 
virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:65:f7:fb,bus=pci.4,addr=0x0,rombar=0
 \
  -chardev pty,id=charserial0 \
  -device isa-serial,chardev=charserial0,id=serial0 \
  -device usb-tablet,id=input2,bus=usb.0,port=1 \
  -spice port=5900,addr=127.0.0.1,disable-ticketing,seamless-migration=on \
  -device cirrus-vga,id=video0,bus=pcie.0,addr=0x1 \
  -device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x0 \
  -global isa-debugcon.iobase=0x402 \
  -debugcon file:/tmp/from-brijesh.log \
  -fw_cfg name=opt/ovmf/PcdResizeXterm,string=y \
  -s \
  -object sev-guest,id=sev0,policy=0x0,cbitpos=47,reduced-phys-bits=5 \
  -machine memory-encryption=sev0 \
  -msg timestamp=on

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

[edk2] [PATCH] IntelSiliconPkg VTdPmrPei: Return SUCCESS when Mapping == NULL in Unmap

[Star Zeng]

NULL is returned to Mapping when Operation is BusMasterCommonBuffer or
EdkiiIoMmuOperationBusMasterCommonBuffer64 in PeiIoMmuMap().
So Mapping == NULL is valid when calling PeiIoMmuUnmap().

940dbd071e9f01717236af236740aa0da716805f wrongly changed EFI_SUCCESS
to EFI_INVALID_PARAMETER when Mapping == NULL in PeiIoMmuUnmap().
This patch is to correct it.

Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c 
b/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c
index ea3091ef911c..6289834fcb38 100644
--- a/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c
+++ b/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c
@@ -189,7 +189,7 @@ PeiIoMmuMap (
   if (Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
   Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
 *DeviceAddress = (UINTN)HostAddress;
-*Mapping = 0;
+*Mapping = NULL;
 return EFI_SUCCESS;
   }
 
@@ -266,7 +266,7 @@ PeiIoMmuUnmap (
   }
 
   if (Mapping == NULL) {
-return EFI_INVALID_PARAMETER;
+return EFI_SUCCESS;
   }
 
   MapInfo = Mapping;
-- 
2.13.3.windows.1

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

Re: [edk2] [PATCH 00/20] OvmfPkg: SEV: decrypt the initial SMRAM save state map for SMBASE relocation

[Laszlo Ersek]

On 03/02/18 14:17, Brijesh Singh wrote:
> 
> 
> On 3/2/18 5:53 AM, Laszlo Ersek wrote:
>> On 03/02/18 02:16, Brijesh Singh wrote:
>>>
>>> On 3/1/18 6:03 PM, Laszlo Ersek wrote:
 I also tried to test the series with SEV guests (again with Brijesh's v2
 2/2 patch applied on top). Unfortunately, I didn't get good results with
 or without SMM. Without SMM, the guest OS boots to a point, but then it
 gets stuck with the CPU spinning. With SMM, OVMF gets stuck in SMBASE
 relocation.
>>> To boot the SEV guest with SMM support we need this KVM patch, without
>>> this we will get either #UD or some undefined behavior.
>>>
>>> https://git.kernel.org/pub/scm/virt/kvm/kvm.git/commit/?id=7607b7174405aec7441ff6c970833c463114040a
>> Looks like a very recent commit. What tree (and at what commit) do you
>> recommend that I build a new host kernel?
> 
> Yes this is very recent commit and it was developed during SMM work. For
> host kernel we need at least 4.16.0-rc1 but since you are going to boot
> the SMM enabled BIOS hence I recommend using latest kvm/master
> 
> https://git.kernel.org/pub/scm/virt/kvm/kvm.git/
> 
>>
>>> It's strange that you are having trouble booting SEV guest without SMM
>>> support. It's possible that we might have some mismatch kernel kvm +
>>> qemu + ovmf patches.
>> Wait, the details matter: I wrote "the guest OS boots to a point". There
>> are no problems with the firmware, or the initial OS boot progress. The
>> issue happens fairly later (but certainly before I reach a login prompt
>> or similar). Maybe this is nothing new relative to last November; I
>> don't remember.
> 
> Ah, my best guess is that userspace program is getting wrong time using
> clock_gettime() and hence the bootscripts are waiting on some events
> forever .. IIRC, I was getting boot hang sometime back in Oct or Nov and
> debugging took me to the kvmclock support for SEV guest. I was doing
> everything right in my patches for kvmclock except the first hunk of the
> below patch. When kvmclock is available the clock_getttime() uses vdso
> and since kvmclock page is shared between HV and Guest hence we needed
> to ensure that userspace pgtable have proper C-bit when accessing this
> memory range.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v4.16-rc3=819aeee065e5d1b417ecd633897427c89f3253ec
> 
> All the SEV guest got accepted in 4.15, hence for guest kernel you can
> use Linux kernel >=4.15
> 
 Until then, Brijesh, can you please test this series? Thank you!
>>>
>>> Sure, I will try the series tomorrow morning. thank you so much for the
>>> cleanup and remaining SMM work.
>> Thanks!
>>
>> Do you have (maybe updated) instructions for setting up the SEV host?
>> What are the latest bits that are expected to work together?
> 
> AMDSEV page https://github.com/AMDESE/AMDSEV contains some instruction
> and scripts to boot the SEV guest but its still using the older version
> of kernel and qemu. Here is what you need to do:
> 
> For host kernel:
> - use recent kvm/master
> - make sure following kernel config is enabled
>   CONFIG_KVM_AMD_SEV
>   CONFIG_CRYPTO_DEV_SP_PSP
>   CONFIG_AMD_MEM_ENCRYPT
>   CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT
> 
> For guest kernel:
>  - you can use host kernel or anything >=4.15
>     make sure you have following config enabled in kernel:
>       CONFIG_AMD_MEM_ENCRYPT
> 
> For qemu:
> - v10 patches from this branch
> https://github.com/codomania/qemu/tree/v10

Great, I'll work on updating all the bits and I'll report back.

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

[edk2] 答复: OVMF package : Question about Qemu/Xen support

[Tiger Liu(BJ-RD)]

Hi, Laszlo:
Got it!

Thanks a lot!

Best wishes,
-邮件原件-
发件人: Laszlo Ersek [mailto:ler...@redhat.com]
发送时间: 2018年3月5日 17:15
收件人: Tiger Liu(BJ-RD) 
抄送: 'edk2-devel@lists.01.org' ; Anthony Perard 
; Julien Grall 
主题: Re: [edk2] OVMF package : Question about Qemu/Xen support

On 03/05/18 06:49, Tiger Liu(BJ-RD) wrote:
> Hi, experts:
> I have a question about Ovmf.
>
> Must Ovmf be run with qemu tool?
> Or It could be run with Xen without needed qemu software.
>
> It seems Xen began to support uefi boot from 4.4 version.

- From the Xen wiki:

https://wiki.xenproject.org/wiki/OVMF

> One thing to have in mind is Xen supports both its own QEMU fork
> called qemu-traditional and upstream QEMU called qemu-xen. OVMF only
> supports the latter. Xen 4.4 has upstream QEMU configured for all HVM
> guests by default, so it is fine to not specify which QEMU to use in
> guest config file. But if you have already configured qemu-traditional
> for your guest you would need to delete / comment out that line.

See also

https://wiki.xenproject.org/wiki/Xen_Project_Release_Features#Device_Models_and_Virtual_Firmware

- I'm also CC'ing the OVMF reviewers for Xen (from the "Maintainers.txt"
file).

Thanks
Laszlo


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

Re: [edk2] OVMF package : Question about Qemu/Xen support

[Laszlo Ersek]

On 03/05/18 06:49, Tiger Liu(BJ-RD) wrote:
> Hi, experts:
> I have a question about Ovmf.
> 
> Must Ovmf be run with qemu tool?
> Or It could be run with Xen without needed qemu software.
> 
> It seems Xen began to support uefi boot from 4.4 version.

- From the Xen wiki:

https://wiki.xenproject.org/wiki/OVMF

> One thing to have in mind is Xen supports both its own QEMU fork
> called qemu-traditional and upstream QEMU called qemu-xen. OVMF only
> supports the latter. Xen 4.4 has upstream QEMU configured for all HVM
> guests by default, so it is fine to not specify which QEMU to use in
> guest config file. But if you have already configured qemu-traditional
> for your guest you would need to delete / comment out that line.

See also

https://wiki.xenproject.org/wiki/Xen_Project_Release_Features#Device_Models_and_Virtual_Firmware

- I'm also CC'ing the OVMF reviewers for Xen (from the "Maintainers.txt"
file).

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

Re: [edk2] [PATCH v2] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in executable memory

[Laszlo Ersek]

On 03/03/18 10:03, Jian J Wang wrote:
>> v2 changes:
>> a. Reserve memory of mReservedApLoopFunc and mReservedTopOfApStack
>>separately to avoid making mReservedTopOfApStack executable.
> 
> if PcdDxeNxMemoryProtectionPolicy is enabled for EfiReservedMemoryType
> of memory, #PF will be triggered for each APs after ExitBootServices
> in SCRT test. The root cause is that AP wakeup code executed at that
> time is stored in memory of type EfiReservedMemoryType (referenced by
> global mReservedApLoopFunc), which is marked as non-executable.
> 
> This patch fixes this issue by setting memory of mReservedApLoopFunc to
> be executable immediately after allocation.
> 
> Cc: Ruiyu Ni 
> Cc: Eric Dong 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 38 
> +
>  1 file changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c 
> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index fd2317924f..e7ed21c6cd 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -388,9 +388,9 @@ InitMpGlobalData (
>// Allocating it in advance since memory services are not available in
>// Exit Boot Services callback function.
>//
> -  ApSafeBufferSize  = CpuMpData->AddressMap.RelocateApLoopFuncSize;
> -  ApSafeBufferSize += CpuMpData->CpuCount * AP_SAFE_STACK_SIZE;
> -
> +  ApSafeBufferSize  = EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (
> +CpuMpData->AddressMap.RelocateApLoopFuncSize
> +));
>Address = BASE_4GB - 1;
>Status  = gBS->AllocatePages (
> AllocateMaxAddress,
> @@ -399,9 +399,39 @@ InitMpGlobalData (
> 
> );
>ASSERT_EFI_ERROR (Status);
> +
>mReservedApLoopFunc = (VOID *) (UINTN) Address;
>ASSERT (mReservedApLoopFunc != NULL);
> -  mReservedTopOfApStack = (UINTN) Address + EFI_PAGES_TO_SIZE 
> (EFI_SIZE_TO_PAGES (ApSafeBufferSize));
> +
> +  //
> +  // Make sure that the buffer memory is executable if NX protection is 
> enabled
> +  // for EfiReservedMemoryType.
> +  // 
> +  // TODO: Check EFI_MEMORY_XP bit set or not once it's available in DXE GCD
> +  //   service.
> +  //
> +  Status = gDS->GetMemorySpaceDescriptor (Address, );
> +  if (!EFI_ERROR (Status)) {
> +gDS->SetMemorySpaceAttributes (
> +   Address,
> +   ApSafeBufferSize,
> +   MemDesc.Attributes & (~EFI_MEMORY_XP)
> +   );
> +  }
> +
> +  ApSafeBufferSize = EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (
> +   CpuMpData->CpuCount * AP_SAFE_STACK_SIZE
> +   ));
> +  Address = BASE_4GB - 1;
> +  Status  = gBS->AllocatePages (
> +   AllocateMaxAddress,
> +   EfiReservedMemoryType,
> +   EFI_SIZE_TO_PAGES (ApSafeBufferSize),
> +   
> +   );
> +  ASSERT_EFI_ERROR (Status);
> +
> +  mReservedTopOfApStack = (UINTN) Address + ApSafeBufferSize;
>ASSERT ((mReservedTopOfApStack & (UINTN)(CPU_STACK_ALIGNMENT - 1)) == 0);
>CopyMem (
>  mReservedApLoopFunc,
> 

I didn't verify whether or not the AP wakeup code relies on the stack
following directly the AP loop func. Since you must have tested this
patch successfully, I guess there is no such dependency (it would be
wrong anyway, IMO).

Furthermore, the comment is helpful -- we don't call PcdGet, but the
comment seems helpful in finding the PCD after all. (The commit message
does name the PCD, and we can run "git blame" on the comment.)

Reviewed-by: Laszlo Ersek 

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

Re: [edk2] How to handle pflash backed OVMF FW upgrade and live migration best?

[Laszlo Ersek]

On 03/02/18 13:53, Dr. David Alan Gilbert wrote:
> * Laszlo Ersek (ler...@redhat.com) wrote:
>> CC Dave

>> To give you the simplest example, binaries (and varstores) corresponding
>> to FD_SIZE_2MB and FD_SIZE_4MB are incompatible. If a domain is
>> originally defined on top of an FD_SIZE_2MB OVMF, then it likely cannot
>> be migrated to a host where the same OVMF pathname refers to an
>> FD_SIZE_4MB binary. If you have a mixed environment, then you need to
>> carry both binaries to all hosts (and if you backport fixes from
>> upstream edk2, you need to backport those to both binaries).
>>
>> In addition, assuming the domain is powered down for good (the QEMU
>> process terminates), and you update the domain XML from the FD_SIZE_2MB
>> OVMF binary to the FD_SIZE_4MB binary, you *also* have to
>> delete/recreate the domain's variable store file (losing all UEFI
>> variables the domain has accumulated until then). This is because the
>> FD_SIZE_4MB binary is incompatible with the varstore that was originally
>> created for the FD_SIZE_2MB binary (and vice versa).
> 
> I assume that gives a clear and obvious error message - right?

Unfortunately, no, it doesn't.

The constants that describe the varstore flash location are generated at
build time, and they are cooked into the OVMF binary. Varstore flash
presence is checked / verified, but the flash is not "searched for". In
addition, flash is a pretty early / low-level requirement, so no
display, no serial console etc. So the end result is, you may get a
semi-obscure error message on the QEMU debug port. :( The error message
might vary with what QEMU actually mapped under the addresses that the
OVMF binary expects to belong to the flash chip(s).

Edk2 doesn't really consider "mismatched flash" a condition that can be
handled gracefully; I guess it would be similar to soldering a bad flash
chip to the board and expecting a friendly error message on the PCI
display -- the boot will likely not get that far. ... I'm not defending
the OVMF situation, it is really raw for virt users, and it sucks. :(

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

Re: [edk2] [PATCH v5 2/6] OvmfPkg/PciHostBridgeLib: Init PCI aperture to 0

[Guo Heyi]

On Fri, Mar 02, 2018 at 11:19:31AM +0100, Laszlo Ersek wrote:
> On 03/01/18 11:48, Guo Heyi wrote:
> > On Thu, Mar 01, 2018 at 11:17:30AM +0100, Laszlo Ersek wrote:
> >> On 03/01/18 07:57, Heyi Guo wrote:
> 
> >>> diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c 
> >>> b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
> >>> index 31c63ae19e0a..aaf101dfcb0e 100644
> >>> --- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
> >>> +++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c
> >>> @@ -193,6 +193,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,
> >>>
> >>
> >> these ZeroMem() calls are not in the correct place. Please move them
> >> into the "PrimaryBus" loop just underneath. That loop works like
> >> this:
> >>
> >> For each primary bus:
> >>
> >>   (1) set all of the aperture variables to "nonexistent":
> >>
> >> Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = 
> >> MAX_UINT64;
> >> Io.Limit = Mem.Limit = MemAbove4G.Limit = PMem.Limit = 
> >> PMemAbove4G.Limit = 0;
> >>
> >>   (2) accumulate the BARs of the devices on the bus into the aperture
> >>   variables
> >>
> >>   (3) call InitRootBridge() with the aperture variables
> >>
> >>
> >> That is, the ZeroMem() calls that you are adding have to be part of
> >> step (1). So, please replace the assignments
> >>
> >> Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = 
> >> MAX_UINT64;
> >> Io.Limit = Mem.Limit = MemAbove4G.Limit = PMem.Limit = 
> >> PMemAbove4G.Limit = 0;
> >>
> >> with
> >>
> >> 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;
> >
> > Will it cause functional issue?
> >
> > My idea of making the change is like this:
> >
> > 1. ZeroMem() is used to initialize all fields of APERTURE to 0; it can
> >make it in the current place of the patch;
> >
> > 2. In the loop, some fields may be changed by the end of each
> >iteration, and it is the responsibility of the existing code to
> >re-initialize the changed fields to expected values explicitly. It
> >seems not necessary to re-initialize the other fields which will
> >not be changed.
> >
> > However, your advice may be better that merges the initialization code
> > together. I can make the change in the next version of patch.
> 
> Yes, if it's not a big problem for you, please implement my request.
> Going forward I wouldn't like to depend on such intricate details as
> described in your point (2). Namely, in any other C project, I would
> suggest that we write:
> 
>   for (PrimaryBus = 0; PrimaryBus <= PCI_MAX_BUS; PrimaryBus = SubBus + 1) {
> PCI_ROOT_BRIDGE_APERTURE Io = { .Base = MAX_UINT64 },
>   Mem = Io,
>   MemAbove4G = Io,
>   PMem = Io,
>   PMemAbove4G = Io;
> /* ... */
>   }
> 
> In other words, I would:
> - move the definition of the structs into the loop (sort of accepted,
>   but not really liked in edk2),
> - use real C initialization (forbidden in edk2),
> - use designated initializers for the first object, which clears the
>   unlisted fields (C99, forbidden in edk2),
> - initialize the rest of the structs from the first struct where I used
>   the designated initializer explicitly.
> 
> Moving the ZeroMem() into the loop is the closest approximation of this,
> for edk2.

OK, I can do that in the next version of patch.

Thanks,

Heyi

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