[edk2] [patch V2] MdeModulePkg: Fix Memory Attributes table type issue

[Jiewen Yao]

According to the spec, each entry in the Memory
Attributes table shall have the same type as
the region it was carved out of in the UEFI memory map.
The current attribute uses RTData for PE Data, but
it should be RTCode.

This patch fixed the issue. It is validated with or
without PropertiesTable.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: "Yao, Jiewen" 
Cc: "Ard Biesheuvel" 
Cc: "Gao, Liming" 
---
 MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c | 108 +--
 1 file changed, 36 insertions(+), 72 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c 
b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
index 6e5ad03..66c5eb6 100644
--- a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
+++ b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
@@ -81,13 +81,10 @@ EFI_PROPERTIES_TABLE  mPropertiesTable = {
 EFI_LOCK   mPropertiesTableLock = EFI_INITIALIZE_LOCK_VARIABLE 
(TPL_NOTIFY);
 
 //
-// Temporary save for original memory map.
 // This is for MemoryAttributesTable only.
 //
 extern BOOLEAN mIsConstructingMemoryAttributesTable;
-EFI_MEMORY_DESCRIPTOR  *mMemoryMapOrg;
-UINTN  mMemoryMapOrgSize;
-UINTN  mDescriptorSize;
+BOOLEANmPropertiesTableEnable;
 
 //
 // Below functions are for MemoryMap
@@ -199,42 +196,6 @@ SortMemoryMap (
 }
 
 /**
-  Check if this memory entry spans across original memory map boundary.
-
-  @param PhysicalStart   The PhysicalStart of memory
-  @param NumberOfPages   The NumberOfPages of memory
-
-  @retval TRUE  This memory entry spans across original memory map boundary.
-  @retval FALSE This memory entry does not span cross original memory map 
boundary.
-**/
-STATIC
-BOOLEAN
-DoesEntrySpanAcrossBoundary (
-  IN UINT64  PhysicalStart,
-  IN UINT64  NumberOfPages
-  )
-{
-  EFI_MEMORY_DESCRIPTOR   *MemoryMapEntry;
-  EFI_MEMORY_DESCRIPTOR   *MemoryMapEnd;
-  UINT64  MemoryBlockLength;
-
-  MemoryMapEntry = mMemoryMapOrg;
-  MemoryMapEnd   = (EFI_MEMORY_DESCRIPTOR *) ((UINT8 *) mMemoryMapOrg + 
mMemoryMapOrgSize);
-  while (MemoryMapEntry < MemoryMapEnd) {
-MemoryBlockLength = (UINT64) (EfiPagesToSize 
(MemoryMapEntry->NumberOfPages));
-
-if ((MemoryMapEntry->PhysicalStart <= PhysicalStart) &&
-(MemoryMapEntry->PhysicalStart + MemoryBlockLength > PhysicalStart) &&
-(MemoryMapEntry->PhysicalStart + MemoryBlockLength < PhysicalStart + 
EfiPagesToSize (NumberOfPages))) {
-  return TRUE;
-}
-
-MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, mDescriptorSize);
-  }
-  return FALSE;
-}
-
-/**
   Merge continous memory map entries whose have same attributes.
 
   @param  MemoryMap  A pointer to the buffer in which firmware 
places
@@ -271,8 +232,7 @@ MergeMemoryMap (
   if (((UINTN)NextMemoryMapEntry < (UINTN)MemoryMapEnd) &&
   (MemoryMapEntry->Type == NextMemoryMapEntry->Type) &&
   (MemoryMapEntry->Attribute == NextMemoryMapEntry->Attribute) &&
-  ((MemoryMapEntry->PhysicalStart + MemoryBlockLength) == 
NextMemoryMapEntry->PhysicalStart) &&
-  (!DoesEntrySpanAcrossBoundary (MemoryMapEntry->PhysicalStart, 
MemoryMapEntry->NumberOfPages + NextMemoryMapEntry->NumberOfPages))) {
+  ((MemoryMapEntry->PhysicalStart + MemoryBlockLength) == 
NextMemoryMapEntry->PhysicalStart)) {
 MemoryMapEntry->NumberOfPages += NextMemoryMapEntry->NumberOfPages;
 if (NewMemoryMapEntry != MemoryMapEntry) {
   NewMemoryMapEntry->NumberOfPages += 
NextMemoryMapEntry->NumberOfPages;
@@ -430,7 +390,11 @@ SetNewRecord (
   //
   // DATA
   //
-  NewRecord->Type = EfiRuntimeServicesData;
+  if (mIsConstructingMemoryAttributesTable && !mPropertiesTableEnable) {
+NewRecord->Type = TempRecord.Type;
+  } else {
+NewRecord->Type = EfiRuntimeServicesData;
+  }
   NewRecord->PhysicalStart = TempRecord.PhysicalStart;
   NewRecord->VirtualStart  = 0;
   NewRecord->NumberOfPages = 
EfiSizeToPages(ImageRecordCodeSection->CodeSegmentBase - 
NewRecord->PhysicalStart);
@@ -443,7 +407,11 @@ SetNewRecord (
   //
   // CODE
   //
-  NewRecord->Type = EfiRuntimeServicesCode;
+  if (mIsConstructingMemoryAttributesTable && !mPropertiesTableEnable) {
+NewRecord->Type = TempRecord.Type;
+  } else {
+NewRecord->Type = EfiRuntimeServicesCode;
+  }
   NewRecord->PhysicalStart = ImageRecordCodeSection->CodeSegmentBase;
   NewRecord->VirtualStart  = 0;
   NewRecord->NumberOfPages = 
EfiSizeToPages(ImageRecordCodeSection->CodeSegmentSize);
@@ -467,7 +435,11 @@ SetNewRecord (
   // Final DATA
   //
   if (TempRecord.PhysicalStart < ImageEnd) {
-NewRecord->Type = EfiRuntimeServicesData;
+if (mIsConstructingMemoryAttributesTable && !mPropertiesTableEnable) {
+  NewRecord->Type = TempRecord.Type;
+} else {
+  NewReco

Re: [edk2] [patch] MdeModulePkg: Fix Memory Attributes table type issue

[Yao, Jiewen]

Hi Ard
I have reproduced it according to the configuration in your log. I really 
appreciate your timely help.
The root-cause is that there is another place assumes RuntimeServicesData is 
used for PE/DATA, and I miss fixing it.
Now the issue is resolved on my side and I will send out v2 patch soon.

BTW: The ARM64 log seems a little odd.
The image record shows the image is 0x40 pages.
InsertImageRecord - 0x00013877 - 0x0004
The memory record shows 0x50 pages allocated.
RT_Code00013877-0001387B 0050 800F
I am not sure why they are different. They are same in my log, that is why the 
corner case is not triggered on my platform.

Thank you
Yao Jiewen

From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
Sent: Saturday, February 20, 2016 12:09 AM
To: Yao, Jiewen
Cc: edk2-devel@lists.01.org; Gao, Liming
Subject: Re: [edk2] [patch] MdeModulePkg: Fix Memory Attributes table type issue

On 19 February 2016 at 16:21, Yao, Jiewen wrote:
> Hi Ard
> I tried but I cannot reproduce it.
> I build a stub memory map and a stub PE image layout and let code parse it 
> and I got result below.
> I cannot see the duplicated entry and missing entry.
>
> I reviewed code again – it is pretty much straight forward, I am not sure why 
> the RuntimeData entry is always overlapped by last RuntimeCode record.
>
> I need your help here.
>
>
> 1) Would you please enable VERBOSE for DxeCore and send a serial debug log to 
> me for analysis?
> You can use below PCD override for DxeMain.inf
>
> gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x80400040
>
>
> 2) Would you please run SHELL command: memmap? And send log to me as well? I 
> would like to know full memory map layout.
>

Please find attached.


> ==
> MemoryAttributesTable:
> Version - 0x0001
> NumberOfEntries - 0x0017
> DescriptorSize - 0x0028
> Entry (0xA61E020)
> Type - 0x5
> PhysicalStart - 0x00013C0E
> VirtualStart - 0x
> NumberOfPages - 0x0010
> Attribute - 0x4000
> Entry (0xA61E048)
> Type - 0x5
> PhysicalStart - 0x00013C0F
> VirtualStart - 0x
> NumberOfPages - 0x0010
> Attribute - 0x0002
> Entry (0xA61E070)
> Type - 0x5
> PhysicalStart - 0x00013C10
> VirtualStart - 0x
> NumberOfPages - 0x0030
> Attribute - 0x4000
> Entry (0xA61E098)
> Type - 0x5
> PhysicalStart - 0x00013C13
> VirtualStart - 0x
> NumberOfPages - 0x0010
> Attribute - 0x0002
> Entry (0xA61E0C0)
> Type - 0x5
> PhysicalStart - 0x00013C14
> VirtualStart - 0x
> NumberOfPages - 0x0020
> Attribute - 0x4000
> Entry (0xA61E0E8)
> Type - 0x6
> PhysicalStart - 0x00013C16
> VirtualStart - 0x
> NumberOfPages - 0x0020
> Attribute - 0x4000
> Entry (0xA61E110)
> Type - 0x5
> PhysicalStart - 0x00013C1A
> VirtualStart - 0x
> NumberOfPages - 0x0010
> Attribute - 0x4000
> Entry (0xA61E138)
> Type - 0x5
> PhysicalStart - 0x00013C1B
> VirtualStart - 0x
> NumberOfPages - 0x0010
> Attribute - 0x0002
> Entry (0xA61E160)
> Type - 0x5
> PhysicalStart - 0x00013C1C
> VirtualStart - 0x
> NumberOfPages - 0x0020
> Attribute - 0x4000
> Entry (0xA61E188)
> Type - 0x6
> PhysicalStart - 0x00013C1E
> VirtualStart - 0x
> NumberOfPages - 0x00A0
> Attribute - 0x4000
> Entry (0xA61E1B0)
> Type - 0x5
> PhysicalStart - 0x00013C28
> VirtualStart - 0x
> NumberOfPages - 0x0010
> Attribute - 0x4000
> Entry (0xA61E1D8)
> Type - 0x5
> PhysicalStart - 0x00013C29
> VirtualStart - 0x
> NumberOfPages - 0x0010
> Attribute - 0x0002
> Entry (0xA61E200)
> Type - 0x5
> PhysicalStart - 0x00013C2A
> VirtualStart - 0x
> NumberOfPages - 0x0030
> Attribute - 0x4000
> Entry (0xA61E228)
> Type - 0x6
> PhysicalStart - 0x00013C2D
> VirtualStart - 0x
> NumberOfPages - 0x0050
> Attribute - 0x4000
> Entry (0xA61E250)
> Type - 0x5
> PhysicalStart - 0x00013C32
> VirtualStart - 0x
> NumberOfPages - 0x0010
> Attribute - 0x4000
> Entry (0xA61E278)
> Type - 0x5
> PhysicalStart - 0x00013C33
> VirtualStart - 0x
> NumberOfPages - 0x0010
> Attribute - 0x0002
> Entry (0xA61E2A0)
> Type - 0x5
> PhysicalStart - 0x00013C34
> VirtualStart - 0x
> NumberOfPages - 0x0030
> Attribute - 0x4000
> Entry (0xA61E2C8)
> Type - 0x5
> PhysicalStart - 0x00013F65
> VirtualStart - 0x00

Re: [edk2] [PATCH v2 2/2] OvmfPkg: implement UEFI driver for Virtio RNG devices

[Laszlo Ersek]

On 02/19/16 19:32, Ard Biesheuvel wrote:
> This implements a UEFI driver model driver for Virtio devices of type
> VIRTIO_SUBSYSTEM_ENTROPY_SOURCE, and exposes them via instances of
> the EFI_RNG_PROTOCOL protocol, supporting the EFI_RNG_ALGORITHM_RAW
> algorithm only.
> 
> Cc: Jordan Justen 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> ---
>  OvmfPkg/VirtioRngDxe/VirtioRng.c  | 607 
>  OvmfPkg/VirtioRngDxe/VirtioRng.h  |  32 ++
>  OvmfPkg/VirtioRngDxe/VirtioRngDxe.inf |  45 ++
>  3 files changed, 684 insertions(+)

The "OvmfPkg/VirtioRngDxe/VirtioRngDxe.inf" file is okay; snipped.

Main source file:

> diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c 
> b/OvmfPkg/VirtioRngDxe/VirtioRng.c
> new file mode 100644
> index ..5a3605b661dc
> --- /dev/null
> +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c
> @@ -0,0 +1,607 @@
> +/** @file
> +
> +  This driver produces EFI_RNG_PROTOCOL instances for virtio-rng devices.
> +
> +  The implementation is based on OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +
> +  Copyright (C) 2012, Red Hat, Inc.
> +  Copyright (c) 2012 - 2014, Intel Corporation. All rights reserved.
> +
> +  This driver:
> +
> +  Copyright (C) 2016, Linaro Ltd.
> +
> +  This program and the accompanying materials are licensed and made available
> +  under the terms and conditions of the BSD License which accompanies this
> +  distribution. The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, 
> WITHOUT
> +  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "VirtioRng.h"
> +
> +/**
> +  Returns information about the random number generation implementation.
> +
> +  @param[in] This A pointer to the EFI_RNG_PROTOCOL 
> instance.
> +  @param[in,out] RNGAlgorithmListSize On input, the size in bytes of 
> RNGAlgorithmList.
> +  On output with a return code of 
> EFI_SUCCESS, the size
> +  in bytes of the data returned in 
> RNGAlgorithmList. On output
> +  with a return code of 
> EFI_BUFFER_TOO_SMALL,
> +  the size of RNGAlgorithmList required 
> to obtain the list.
> +  @param[out] RNGAlgorithmListA caller-allocated memory buffer 
> filled by the driver
> +  with one EFI_RNG_ALGORITHM element for 
> each supported
> +  RNG algorithm. The list must not 
> change across multiple
> +  calls to the same driver. The first 
> algorithm in the list
> +  is the default algorithm for the 
> driver.
> +
> +  @retval EFI_SUCCESS The RNG algorithm list was returned 
> successfully.
> +  @retval EFI_UNSUPPORTED The services is not supported by this 
> driver.
> +  @retval EFI_DEVICE_ERRORThe list of algorithms could not be 
> retrieved due to a
> +  hardware or firmware error.
> +  @retval EFI_INVALID_PARAMETER   One or more of the parameters are 
> incorrect.
> +  @retval EFI_BUFFER_TOO_SMALLThe buffer RNGAlgorithmList is too 
> small to hold the result.
> +
> +**/

This comment block is likely copied from EFI_RNG_GET_INFO, in
"MdePkg/Include/Protocol/Rng.h". I usually insist on new files under
OvmfPkg being no wider than 79 characters, so in all such cases I rewrap
these comment blocks. I can do this too should you be annoyed by it.

> +STATIC
> +EFI_STATUS
> +EFIAPI
> +VirtioRngGetInfo (
> +  IN  EFI_RNG_PROTOCOL*This,
> +  IN OUT  UINTN   *RNGAlgorithmListSize,
> +  OUT EFI_RNG_ALGORITHM   *RNGAlgorithmList
> +  )
> +{
> +  if (This == NULL || RNGAlgorithmListSize == NULL) {
> +return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (*RNGAlgorithmListSize < sizeof (EFI_RNG_ALGORITHM)) {
> +*RNGAlgorithmListSize = sizeof (EFI_RNG_ALGORITHM);
> +return EFI_BUFFER_TOO_SMALL;
> +  }
> +
> +  if (RNGAlgorithmList != NULL) {
> +return EFI_INVALID_PARAMETER;
> +  }

I don't understand this. I think the condition should be negated.

> +
> +  *RNGAlgorithmListSize = sizeof (EFI_RNG_ALGORITHM);
> +  CopyGuid (RNGAlgorithmList, &gEfiRngAlgorithmRaw);
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Produces and returns an RNG value using either the default or specified 
> RNG algorithm.
> +
> +  @param[in]  ThisA pointer to the EFI_RNG_PROTOCOL 
> instance.
> +  @param[in]  RNGAlgorithmA pointer to the EFI_RNG_ALGORITHM 
> that identifies the RNG
> +  algorithm to use. May be NULL in

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

[David Woodhouse]

On Fri, 2016-02-19 at 14:26 +0100, Laszlo Ersek wrote:
> 
> In file included from
> .../CryptoPkg/Library/OpensslLib/openssl/crypto/bn/bn_add.c:59:0:
> .../CryptoPkg/Library/OpensslLib/openssl/crypto/bn/bn_lcl.h:114:31:
> fatal error: internal/bn_conf.h: No such file or directory
>  # include "internal/bn_conf.h"
>    ^

Hm, that's a new autogenerated file. Screw it, please run
./process_files.sh in the OpensslLib directory and I believe that'll be
created for you. I'll work out the alternative build procedure with a
*clean* tree in the meantime...

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH v2 1/2] OvmfPkg: VirtioFlush(): return the number of bytes written by the host

[Ard Biesheuvel]

From: Laszlo Ersek 

VirtioLib provides an API for simple, synchronous (request/response-style)
virtio communication. The guest driver builds one descriptor chain, link
for link, with VirtioPrepare() and VirtioAppendDesc(), then submits the
chain, and awaits the processing, with VirtioFlush().

The descriptor chain is always built at the beginning of the descriptor
area, with the head descriptor having descriptor index 0.

In order to submit the descriptor chain to the host, the guest always
pushes a new "available element" to the Available Ring, in genuine
queue-like fashion, with the new element referencing the head descriptor
(which always has index 0, see above).

In turn, after processing, the host always pushes a new "used element" to
the Used Ring, in genuine queue-like fashion, with the new element
referencing the head descriptor of the chain that was just processed. The
same element also reports the number of bytes that the host wrote,
consecutively across the host-writeable buffers that were linked by the
descriptors.

(See "OvmfPkg/VirtioNetDxe/TechNotes.txt" for a diagram about the
descriptor area and the rings.)

Because at most one descriptor chain can be in flight with VirtioLib at
any time,

- the Available Ring and the Used Ring proceed in lock-step,

- and the head descriptor that the new "available" and "used" elements can
  ever reference has index 0.

Based on the above, we can modify VirtioFlush() to return the number of
bytes written by the host across the descriptor chain. The virtio-block
and virtio-scsi drivers don't care (they have other ways to parse the data
produced by the host), while the virtio-net driver doesn't use
VirtioFlush() at all (it employs VirtioLib only to set up its rings).

However, the virtio entropy device,  to be covered in the upcoming
patches, reports the amount of randomness produced by the host only
through this quantity.

Cc: Jordan Justen 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
Signed-off-by: Ard Biesheuvel 
---
 OvmfPkg/Include/Library/VirtioLib.h   | 11 +++--
 OvmfPkg/Library/VirtioLib/VirtioLib.c | 26 ++--
 OvmfPkg/VirtioBlkDxe/VirtioBlk.c  |  3 ++-
 OvmfPkg/VirtioScsiDxe/VirtioScsi.c|  2 +-
 4 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/OvmfPkg/Include/Library/VirtioLib.h 
b/OvmfPkg/Include/Library/VirtioLib.h
index 36527a523f28..decd4418af3d 100644
--- a/OvmfPkg/Include/Library/VirtioLib.h
+++ b/OvmfPkg/Include/Library/VirtioLib.h
@@ -2,7 +2,7 @@
 
   Declarations of utility functions used by virtio device drivers.
 
-  Copyright (C) 2012, Red Hat, Inc.
+  Copyright (C) 2012-2016, Red Hat, Inc.
 
   This program and the accompanying materials are licensed and made available
   under the terms and conditions of the BSD License which accompanies this
@@ -167,6 +167,12 @@ VirtioAppendDesc (
   Indices->HeadDescIdx identifies the head descriptor
   of the descriptor chain.
 
+  @param[out] UsedLen On success, the total number of bytes, consecutively
+  across the buffers linked by the descriptor chain,
+  that the host wrote. May be NULL if the caller
+  doesn't care, or can compute the same information
+  from device-specific request structures linked by the
+  descriptor chain.
 
   @return  Error code from VirtIo->SetQueueNotify() if it fails.
 
@@ -179,7 +185,8 @@ VirtioFlush (
   IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
   IN UINT16 VirtQueueId,
   IN OUT VRING  *Ring,
-  IN DESC_INDICES   *Indices
+  IN DESC_INDICES   *Indices,
+  OUTUINT32 *UsedLenOPTIONAL
   );
 
 #endif // _VIRTIO_LIB_H_
diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c 
b/OvmfPkg/Library/VirtioLib/VirtioLib.c
index 54cf225c9885..4b1d78b5a03e 100644
--- a/OvmfPkg/Library/VirtioLib/VirtioLib.c
+++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c
@@ -2,7 +2,7 @@
 
   Utility functions used by virtio device drivers.
 
-  Copyright (C) 2012, Red Hat, Inc.
+  Copyright (C) 2012-2016, Red Hat, Inc.
   Portion of Copyright (C) 2013, ARM Ltd.
 
   This program and the accompanying materials are licensed and made available
@@ -249,6 +249,12 @@ VirtioAppendDesc (
   Indices->HeadDescIdx identifies the head descriptor
   of the descriptor chain.
 
+  @param[out] UsedLen On success, the total number of bytes, consecutively
+  across the buffers linked by the descriptor chain,
+  that the host wrote. May be NULL if the caller
+  doesn't care, or can compute the same information
+  from device-specific request structures linked by the
+  descriptor chain.
 
   @return   

[edk2] [PATCH v2 0/2] UEFI driver model driver for VirtIO-RNG

[Ard Biesheuvel]

This implements a UEFI driver model driver for the VirtIO RNG device.

Changes since v1:
- validate args rather than ASSERT () their validity
- use a volatile intermediate buffer so the compiler does not optimize the
  access away
- include Laszlo's patch #1 which greatly simplifies the next item
- poke the virtio RNG device as long as we need to satisfy our full request,
  since a single invocation may return less data than we asked for

I have tested the driver with a rate limit of 19 bytes per 1000 ms, with a
file containing a predictable pattern. The RngTest output is below, it took
a minute or so for the full output to appear, but it looks like the repeated
polling of the device is working as expected.

while true; do echo -n 00112233445566778899AABBCCDDEEFF; done | \
  xxd -r -p | dd iflag=fullblock count=2 bs=1M of=/tmp/unrandom

and pass 

-object rng-random,filename=/tmp/unrandom,id=rng0 \
-device virtio-rng-pci,rng=rng0,max-bytes=19,period=1000 \
  
on the QEMU command line

Ard Biesheuvel (1):
  OvmfPkg: implement UEFI driver for Virtio RNG devices

Laszlo Ersek (1):
  OvmfPkg: VirtioFlush(): return the number of bytes written by the host

 OvmfPkg/Include/Library/VirtioLib.h   |  11 +-
 OvmfPkg/Library/VirtioLib/VirtioLib.c |  26 +-
 OvmfPkg/VirtioBlkDxe/VirtioBlk.c  |   3 +-
 OvmfPkg/VirtioRngDxe/VirtioRng.c  | 607 
 OvmfPkg/VirtioRngDxe/VirtioRng.h  |  32 ++
 OvmfPkg/VirtioRngDxe/VirtioRngDxe.inf |  45 ++
 OvmfPkg/VirtioScsiDxe/VirtioScsi.c|   2 +-
 7 files changed, 720 insertions(+), 6 deletions(-)
 create mode 100644 OvmfPkg/VirtioRngDxe/VirtioRng.c
 create mode 100644 OvmfPkg/VirtioRngDxe/VirtioRng.h
 create mode 100644 OvmfPkg/VirtioRngDxe/VirtioRngDxe.inf

-- 
2.5.0

UEFI RNG Protocol Testing :

 -- Locate UEFI RNG Protocol : [Pass]
 -- Call RNG->GetInfo() interface : 
 >> Supported RNG Algorithm (Count = 1) : 
  0) 0930---181F-B7380100
 -- Call RNG->GetRNG() interface : 
 >> RNG with default algorithm : [Pass]
 >> RNG with SP800-90-HMAC-256 : [Fail - Status = Unsupported]
 >> RNG with SP800-90-Hash-256 : [Fail - Status = Unsupported]
 >> RNG with SP800-90-CTR-256 : [Fail - Status = Unsupported]
 >> RNG with X9.31-3DES : [Fail - Status = Unsupported]
 >> RNG with X9.31-AES : [Fail - Status = Unsupported]
 >> RNG with RAW Entropy : [Pass]
 -- Random Number Generation Test with default RNG Algorithm (20 Rounds): 
  01) - 00
  02) - 1122
  03) - 334455
  04) - 66778899
  05) - AABBCCDDEE
  06) - FF0011223344
  07) - 5566778899AABB
  08) - CCDDEEFF00112233
  09) - 445566778899AABBCC
  10) - DDEEFF00112233445566
  11) - 778899AABBCCDDEEFF0011
  12) - 2233445566778899AABBCCDD
  13) - EEFF00112233445566778899AA
  14) - BBCCDDEEFF001122334455667788
  15) - 99AABBCCDDEEFF0011223344556677
  16) - 8899AABBCCDDEEFF0011223344556677
  17) - 8899AABBCCDDEEFF001122334455667788
  18) - 99AABBCCDDEEFF00112233445566778899AA
  19) - BBCCDDEEFF00112233445566778899AABBCCDD
  20) - EEFF00112233445566778899AABBCCDDEEFF0011
 -- RAW Entropy Generation Test (20 Rounds) : 
  01) - 2233445566778899AABBCCDDEEFF00112233445566778899AABBCCDDEEFF0011
  02) - 2233445566778899AABBCCDDEEFF00112233445566778899AABBCCDDEEFF0011
  03) - 2233445566778899AABBCCDDEEFF00112233445566778899AABBCCDDEEFF0011
  04) - 2233445566778899AABBCCDDEEFF00112233445566778899AABBCCDDEEFF0011
  05) - 2233445566778899AABBCCDDEEFF00112233445566778899AABBCCDDEEFF0011
  06) - 2233445566778899AABBCCDDEEFF00112233445566778899AABBCCDDEEFF0011
  07) - 2233445566778899AABBCCDDEEFF00112233445566778899AABBCCDDEEFF0011
  08) - 2233445566778899AABBCCDDEEFF00112233445566778899AABBCCDDEEFF0011
  09) - 2233445566778899AABBCCDDEEFF00112233445566778899AABBCCDDEEFF0011
  10) - 2233445566778899AABBCCDDEEFF00112233445566778899AABBCCDDEEFF0011
  11) - 2233445566778899AABBCCDDEEFF00112233445566778899AABBCCDDEEFF0011
  12) - 2233445566778899AABBCCDDEEFF00112233445566778899AABBCCDDEEFF0011
  13) - 2233445566778899AABBCCDDEEFF00112233445566778899AABBCCDDEEFF0011
  14) - 2233445566778899AABBCCDDEEFF00112233445566778899AABBCCDDEEFF0011
  15) - 2233445566778899AABBCCDDEEFF00112233445566778899AABBCCDDEEFF0011
  16) - 2233445566778899AABBCCDDEEFF00112233445566778899AABBCCDDEEFF0011
  17) - 2233445566778899AABBCCDDEEFF00112233445566778899AABBCCDDEEFF0011
  18) - 2233445566778899AABBCCDDEEFF00112233445566778899AABBCCDDEEFF0011
  19) - 2233445566778899AABBCCDDEEFF00112233445566778899AABBCCDDEEFF0011
  20) - 2233445566778899AABBCCDDEEFF00112233445566778899AABBCCDDEEFF0011
 -- Exit UEFI RNG Protocol Test (Status = Success).

[edk2] [PATCH v2 2/2] OvmfPkg: implement UEFI driver for Virtio RNG devices

[Ard Biesheuvel]

This implements a UEFI driver model driver for Virtio devices of type
VIRTIO_SUBSYSTEM_ENTROPY_SOURCE, and exposes them via instances of
the EFI_RNG_PROTOCOL protocol, supporting the EFI_RNG_ALGORITHM_RAW
algorithm only.

Cc: Jordan Justen 
Cc: Laszlo Ersek 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
 OvmfPkg/VirtioRngDxe/VirtioRng.c  | 607 
 OvmfPkg/VirtioRngDxe/VirtioRng.h  |  32 ++
 OvmfPkg/VirtioRngDxe/VirtioRngDxe.inf |  45 ++
 3 files changed, 684 insertions(+)

diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c b/OvmfPkg/VirtioRngDxe/VirtioRng.c
new file mode 100644
index ..5a3605b661dc
--- /dev/null
+++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c
@@ -0,0 +1,607 @@
+/** @file
+
+  This driver produces EFI_RNG_PROTOCOL instances for virtio-rng devices.
+
+  The implementation is based on OvmfPkg/VirtioScsiDxe/VirtioScsi.c
+
+  Copyright (C) 2012, Red Hat, Inc.
+  Copyright (c) 2012 - 2014, Intel Corporation. All rights reserved.
+
+  This driver:
+
+  Copyright (C) 2016, Linaro Ltd.
+
+  This program and the accompanying materials are licensed and made available
+  under the terms and conditions of the BSD License which accompanies this
+  distribution. The full text of the license may be found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "VirtioRng.h"
+
+/**
+  Returns information about the random number generation implementation.
+
+  @param[in] This A pointer to the EFI_RNG_PROTOCOL 
instance.
+  @param[in,out] RNGAlgorithmListSize On input, the size in bytes of 
RNGAlgorithmList.
+  On output with a return code of 
EFI_SUCCESS, the size
+  in bytes of the data returned in 
RNGAlgorithmList. On output
+  with a return code of 
EFI_BUFFER_TOO_SMALL,
+  the size of RNGAlgorithmList required to 
obtain the list.
+  @param[out] RNGAlgorithmListA caller-allocated memory buffer filled 
by the driver
+  with one EFI_RNG_ALGORITHM element for 
each supported
+  RNG algorithm. The list must not change 
across multiple
+  calls to the same driver. The first 
algorithm in the list
+  is the default algorithm for the driver.
+
+  @retval EFI_SUCCESS The RNG algorithm list was returned 
successfully.
+  @retval EFI_UNSUPPORTED The services is not supported by this 
driver.
+  @retval EFI_DEVICE_ERRORThe list of algorithms could not be 
retrieved due to a
+  hardware or firmware error.
+  @retval EFI_INVALID_PARAMETER   One or more of the parameters are 
incorrect.
+  @retval EFI_BUFFER_TOO_SMALLThe buffer RNGAlgorithmList is too small 
to hold the result.
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+VirtioRngGetInfo (
+  IN  EFI_RNG_PROTOCOL*This,
+  IN OUT  UINTN   *RNGAlgorithmListSize,
+  OUT EFI_RNG_ALGORITHM   *RNGAlgorithmList
+  )
+{
+  if (This == NULL || RNGAlgorithmListSize == NULL) {
+return EFI_INVALID_PARAMETER;
+  }
+
+  if (*RNGAlgorithmListSize < sizeof (EFI_RNG_ALGORITHM)) {
+*RNGAlgorithmListSize = sizeof (EFI_RNG_ALGORITHM);
+return EFI_BUFFER_TOO_SMALL;
+  }
+
+  if (RNGAlgorithmList != NULL) {
+return EFI_INVALID_PARAMETER;
+  }
+
+  *RNGAlgorithmListSize = sizeof (EFI_RNG_ALGORITHM);
+  CopyGuid (RNGAlgorithmList, &gEfiRngAlgorithmRaw);
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Produces and returns an RNG value using either the default or specified RNG 
algorithm.
+
+  @param[in]  ThisA pointer to the EFI_RNG_PROTOCOL 
instance.
+  @param[in]  RNGAlgorithmA pointer to the EFI_RNG_ALGORITHM that 
identifies the RNG
+  algorithm to use. May be NULL in which 
case the function will
+  use its default RNG algorithm.
+  @param[in]  RNGValueLength  The length in bytes of the memory buffer 
pointed to by
+  RNGValue. The driver shall return 
exactly this numbers of bytes.
+  @param[out] RNGValueA caller-allocated memory buffer filled 
by the driver with the
+  resulting RNG value.
+
+  @retval EFI_SUCCESS The RNG value was returned successfully.
+  @retval EFI_UNSUPPORTED The algorithm specified by RNGAlgorithm 
is not supported by
+  this driver.
+  @retval EFI_DEVICE_ERROR 

[edk2] [PATCH 1/1] OvmfPkg: VirtioFlush(): return the number of bytes written by the host

[Laszlo Ersek]

VirtioLib provides an API for simple, synchronous (request/response-style)
virtio communication. The guest driver builds one descriptor chain, link
for link, with VirtioPrepare() and VirtioAppendDesc(), then submits the
chain, and awaits the processing, with VirtioFlush().

The descriptor chain is always built at the beginning of the descriptor
area, with the head descriptor having descriptor index 0.

In order to submit the descriptor chain to the host, the guest always
pushes a new "available element" to the Available Ring, in genuine
queue-like fashion, with the new element referencing the head descriptor
(which always has index 0, see above).

In turn, after processing, the host always pushes a new "used element" to
the Used Ring, in genuine queue-like fashion, with the new element
referencing the head descriptor of the chain that was just processed. The
same element also reports the number of bytes that the host wrote,
consecutively across the host-writeable buffers that were linked by the
descriptors.

(See "OvmfPkg/VirtioNetDxe/TechNotes.txt" for a diagram about the
descriptor area and the rings.)

Because at most one descriptor chain can be in flight with VirtioLib at
any time,

- the Available Ring and the Used Ring proceed in lock-step,

- and the head descriptor that the new "available" and "used" elements can
  ever reference has index 0.

Based on the above, we can modify VirtioFlush() to return the number of
bytes written by the host across the descriptor chain. The virtio-block
and virtio-scsi drivers don't care (they have other ways to parse the data
produced by the host), while the virtio-net driver doesn't use
VirtioFlush() at all (it employs VirtioLib only to set up its rings).

However, the virtio entropy device,  to be covered in the upcoming
patches, reports the amount of randomness produced by the host only
through this quantity.

Cc: Ard Biesheuvel 
Cc: Jordan Justen 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
---
 OvmfPkg/Include/Library/VirtioLib.h   | 11 +++--
 OvmfPkg/Library/VirtioLib/VirtioLib.c | 26 ++--
 OvmfPkg/VirtioBlkDxe/VirtioBlk.c  |  3 ++-
 OvmfPkg/VirtioScsiDxe/VirtioScsi.c|  2 +-
 4 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/OvmfPkg/Include/Library/VirtioLib.h 
b/OvmfPkg/Include/Library/VirtioLib.h
index 36527a523f28..decd4418af3d 100644
--- a/OvmfPkg/Include/Library/VirtioLib.h
+++ b/OvmfPkg/Include/Library/VirtioLib.h
@@ -2,7 +2,7 @@
 
   Declarations of utility functions used by virtio device drivers.
 
-  Copyright (C) 2012, Red Hat, Inc.
+  Copyright (C) 2012-2016, Red Hat, Inc.
 
   This program and the accompanying materials are licensed and made available
   under the terms and conditions of the BSD License which accompanies this
@@ -167,6 +167,12 @@ VirtioAppendDesc (
   Indices->HeadDescIdx identifies the head descriptor
   of the descriptor chain.
 
+  @param[out] UsedLen On success, the total number of bytes, consecutively
+  across the buffers linked by the descriptor chain,
+  that the host wrote. May be NULL if the caller
+  doesn't care, or can compute the same information
+  from device-specific request structures linked by the
+  descriptor chain.
 
   @return  Error code from VirtIo->SetQueueNotify() if it fails.
 
@@ -179,7 +185,8 @@ VirtioFlush (
   IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
   IN UINT16 VirtQueueId,
   IN OUT VRING  *Ring,
-  IN DESC_INDICES   *Indices
+  IN DESC_INDICES   *Indices,
+  OUTUINT32 *UsedLenOPTIONAL
   );
 
 #endif // _VIRTIO_LIB_H_
diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c 
b/OvmfPkg/Library/VirtioLib/VirtioLib.c
index 54cf225c9885..4b1d78b5a03e 100644
--- a/OvmfPkg/Library/VirtioLib/VirtioLib.c
+++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c
@@ -2,7 +2,7 @@
 
   Utility functions used by virtio device drivers.
 
-  Copyright (C) 2012, Red Hat, Inc.
+  Copyright (C) 2012-2016, Red Hat, Inc.
   Portion of Copyright (C) 2013, ARM Ltd.
 
   This program and the accompanying materials are licensed and made available
@@ -249,6 +249,12 @@ VirtioAppendDesc (
   Indices->HeadDescIdx identifies the head descriptor
   of the descriptor chain.
 
+  @param[out] UsedLen On success, the total number of bytes, consecutively
+  across the buffers linked by the descriptor chain,
+  that the host wrote. May be NULL if the caller
+  doesn't care, or can compute the same information
+  from device-specific request structures linked by the
+  descriptor chain.
 
   @return  Error code from VirtIo->SetQu

[edk2] [PATCH 0/1] OvmfPkg: VirtioFlush(): return the number of bytes written by the host

[Laszlo Ersek]

Public branch:
.

Cc: Ard Biesheuvel 
Cc: Jordan Justen 

Laszlo Ersek (1):
  OvmfPkg: VirtioFlush(): return the number of bytes written by the host

 OvmfPkg/Include/Library/VirtioLib.h   | 11 +++--
 OvmfPkg/Library/VirtioLib/VirtioLib.c | 26 ++--
 OvmfPkg/VirtioBlkDxe/VirtioBlk.c  |  3 ++-
 OvmfPkg/VirtioScsiDxe/VirtioScsi.c|  2 +-
 4 files changed, 36 insertions(+), 6 deletions(-)

-- 
1.8.3.1

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

Re: [edk2] [PATCH] IntelFrameworkModulePkg/Bds: Correct the total RAM calculation

[Jeremy Linton]

On 02/18/2016 08:55 PM, Zeng, Star wrote:

Jeremy,


(trimming)

-  );
-TokenToUpdate = STRING_TOKEN (STR_FRONT_PAGE_MEMORY_SIZE);
-HiiSetString (gFrontPagePrivate.HiiHandle, TokenToUpdate,
NewString, NULL);
-FreePool (NewString);
-Find[4] = TRUE;
+if (Type19Record->StartingAddress != 0x ) {
+  InstalledMemory += Type19Record->EndingAddress -
+Type19Record->StartingAddress;


According to SMBIOS spec, the EndingAddress is Physical ending address
of the *last kilobyte*, and the unit is kilobyte. So the code should be
   InstalledMemory += RShiftU64((Type19Record->EndingAddress -
Type19Record->StartingAddress + 1), 10);


Zeng,
Thats a big opps, thanks for catching that!





+} else {
+  InstalledMemory += Type19Record->ExtendedEndingAddress -
+  Type19Record->ExtendedStartingAddress;


According to SMBIOS spec, the ExtendedEndingAddress is Physical ending
address, in bytes. So similar with above, the code should be
   InstalledMemory +=
RShiftU64((Type19Record->ExtendedEndingAddress -
Type19Record->ExtendedStartingAddress + 1), 20);


+}
}
-} while ( !(Find[0] && Find[1] && Find[2] && Find[3] && Find[4]));
+
+  Status = Smbios->GetNext (Smbios, &SmbiosHandle, NULL, &Record,
NULL);
+}
+
+// now update the total installed RAM size
+InstalledMemory >>= 20;


Remove this code line and comments since the installed memory has been
translated to XXX MB above. In fact, directly >= operation with 64 bits
operand at 32 bits build will involve intrinsic function of compiler
that may cause build failure (at lease for IA32), RShiftU64() could be
used.


Ok,




+ConvertMemorySizeToString ((UINT32)InstalledMemory, &NewString );
+TokenToUpdate = STRING_TOKEN (STR_FRONT_PAGE_MEMORY_SIZE);
+HiiSetString (gFrontPagePrivate.HiiHandle, TokenToUpdate,
NewString, NULL);
+FreePool (NewString);
}
+
return ;
  }




More generic comments below.
1. Could you remove the trailing white space for new added code lines?
2. Could you sync the change to
MdeModulePkg/Application/UiApp/FrontPage.c after this patch finalized?


Yes, I will correct these issues and re-post.

Thanks again for taking the time to look at it.



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

Re: [edk2] [patch] MdeModulePkg: Fix Memory Attributes table type issue

[Ard Biesheuvel]

On 19 February 2016 at 16:21, Yao, Jiewen  wrote:
> Hi Ard
> I tried but I cannot reproduce it.
> I build a stub memory map and a stub PE image layout and let code parse it 
> and I got result below.
> I cannot see the duplicated entry and missing entry.
>
> I reviewed code again – it is pretty much straight forward, I am not sure why 
> the RuntimeData entry is always overlapped by last RuntimeCode record.
>
> I need your help here.
>
>
> 1)  Would you please enable VERBOSE for DxeCore and send a serial debug 
> log to me for analysis?
> You can use below PCD override for DxeMain.inf
> 
>   gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x80400040
>
>
> 2)  Would you please run SHELL command: memmap? And send log to me as 
> well? I would like to know full memory map layout.
>

Please find attached.


> ==
> MemoryAttributesTable:
>   Version  - 0x0001
>   NumberOfEntries  - 0x0017
>   DescriptorSize   - 0x0028
> Entry (0xA61E020)
>   Type  - 0x5
>   PhysicalStart - 0x00013C0E
>   VirtualStart  - 0x
>   NumberOfPages - 0x0010
>   Attribute - 0x4000
> Entry (0xA61E048)
>   Type  - 0x5
>   PhysicalStart - 0x00013C0F
>   VirtualStart  - 0x
>   NumberOfPages - 0x0010
>   Attribute - 0x0002
> Entry (0xA61E070)
>   Type  - 0x5
>   PhysicalStart - 0x00013C10
>   VirtualStart  - 0x
>   NumberOfPages - 0x0030
>   Attribute - 0x4000
> Entry (0xA61E098)
>   Type  - 0x5
>   PhysicalStart - 0x00013C13
>   VirtualStart  - 0x
>   NumberOfPages - 0x0010
>   Attribute - 0x0002
> Entry (0xA61E0C0)
>   Type  - 0x5
>   PhysicalStart - 0x00013C14
>   VirtualStart  - 0x
>   NumberOfPages - 0x0020
>   Attribute - 0x4000
> Entry (0xA61E0E8)
>   Type  - 0x6
>   PhysicalStart - 0x00013C16
>   VirtualStart  - 0x
>   NumberOfPages - 0x0020
>   Attribute - 0x4000
> Entry (0xA61E110)
>   Type  - 0x5
>   PhysicalStart - 0x00013C1A
>   VirtualStart  - 0x
>   NumberOfPages - 0x0010
>   Attribute - 0x4000
> Entry (0xA61E138)
>   Type  - 0x5
>   PhysicalStart - 0x00013C1B
>   VirtualStart  - 0x
>   NumberOfPages - 0x0010
>   Attribute - 0x0002
> Entry (0xA61E160)
>   Type  - 0x5
>   PhysicalStart - 0x00013C1C
>   VirtualStart  - 0x
>   NumberOfPages - 0x0020
>   Attribute - 0x4000
> Entry (0xA61E188)
>   Type  - 0x6
>   PhysicalStart - 0x00013C1E
>   VirtualStart  - 0x
>   NumberOfPages - 0x00A0
>   Attribute - 0x4000
> Entry (0xA61E1B0)
>   Type  - 0x5
>   PhysicalStart - 0x00013C28
>   VirtualStart  - 0x
>   NumberOfPages - 0x0010
>   Attribute - 0x4000
> Entry (0xA61E1D8)
>   Type  - 0x5
>   PhysicalStart - 0x00013C29
>   VirtualStart  - 0x
>   NumberOfPages - 0x0010
>   Attribute - 0x0002
> Entry (0xA61E200)
>   Type  - 0x5
>   PhysicalStart - 0x00013C2A
>   VirtualStart  - 0x
>   NumberOfPages - 0x0030
>   Attribute - 0x4000
> Entry (0xA61E228)
>   Type  - 0x6
>   PhysicalStart - 0x00013C2D
>   VirtualStart  - 0x
>   NumberOfPages - 0x0050
>   Attribute - 0x4000
> Entry (0xA61E250)
>   Type  - 0x5
>   PhysicalStart - 0x00013C32
>   VirtualStart  - 0x
>   NumberOfPages - 0x0010
>   Attribute - 0x4000
> Entry (0xA61E278)
>   Type  - 0x5
>   PhysicalStart - 0x00013C33
>   VirtualStart  - 0x
>   NumberOfPages - 0x0010
>   Attribute - 0x0002
> Entry (0xA61E2A0)
>   Type  - 0x5
>   PhysicalStart - 0x00013C34
>   VirtualStart  - 0x
>   NumberOfPages - 0x0030
>   Attribute - 0x4000
> Entry (0xA61E2C8)
>   Type  - 0x5
>   PhysicalStart - 0x00013F65
>   VirtualStart  - 0x
>   NumberOfPages - 0x0010
>   Attribute - 0x4000
> Entry (0xA61E2F0)
>   Type   

Re: [edk2] [PATCH] OvmfPkg: implement UEFI driver for Virtio RNG devices

[Laszlo Ersek]

On 02/19/16 16:07, Ard Biesheuvel wrote:
> On 19 February 2016 at 16:00, Laszlo Ersek  wrote:

>> (3) I searched the patch for "volatile", and found none. Please, in the
>> VirtioRngGetRNG() member function, allocate an appropriately sized
>> array, for a (volatile UINT8 *) pointer. The virtio transfer should
>> occur into this "bounce buffer" (so its address should go into the
>> descriptor you put on the ring), and once the transfer is complete,
>> please copy it with an open-coded loop (not CopyMem(), which drops
>> pointer qualifiers) into the user's buffer. Then free the bounce
>> buffer.
>>
>> The performance impact of this will be minimal (the guest won't
>> consume huge amounts of entropy), and we must prevent the compiler
>> from assuming that the buffer has not changed.
>>
> 
> Could you please elaborate? How is that any different from casting the
> RNGValue argument to (volatile UINT8 *) ?
> Or having an automatic volatile UINT8 * on the stack?

Consider what the caller does. It allocates a buffer (maybe on the
stack, maybe on the heap), calls GetRNG() with the address passed in,
then accesses the buffer.

In turn what does GetRNG() do? Massages a few pointers, and reads /
writes a few IOports or MMIO locations. Assuming an LTO compiler, it can
determine that the actual buffer, read by the caller of GetRNG() in the
end, is never ever written to. That's bad.

This is why you have to write to the caller's buffer yourself, manually
-- so that the compiler can see the write. But, unless you copy that
data from a volatile buffer, the compiler can eliminate this copy as well.

Casting the RNGValue argument to (volatile UINT8 *) in the function
makes no difference, on top of your current patch, because (a) you don't
write to that buffer in any way the compiler can see, (b) the caller
will not use a volatile-qualified pointer to access the buffer.

Having an automatic (volatile UINT8 *) pointer variable on the stack, in
GetRNG(), is fine. You just have to allocate the pointed-to array with
AllocatePool() yourself, pass *this* address to virtio, and once virtio
is done, copy from this buffer (going through the volatile-qualified
pointer) to the user's buffer (going through the normal RNGValue pointer).

The point is, the compiler, looking at the end-to-end data flow, must
see that the data ultimately originates from a *(volatile UINT8 *)
access. It has absolutely no idea that the virtio stuff changes memory
invisibly to it.

(Processor and cache level consistentcy is not my worry -- that's QEMU's
and KVM's responsibility; I care about the compiler here.)


> 
> 
>> (4) According to the virtio specification (I'm looking at
>> "virtio-v1.0-csprd05.pdf", section 5.4, Entropy Device), the device
>> is allowed to return fewer than requested bytes of randomness in
>> the buffers enumerated by the descriptor chain.
>>
>> Looking at the QEMU-side implementation [hw/virtio/virtio-rng.c],
>> this is actually possible: consider the case when the guest
>> requests 16 bytes of randomness, but the chr_read() function in
>> QEMU is entered with size=1 bytes of randomness available.
>> iov_from_buf() will copy 1 byte, the retval will be len=1, and
>> virtqueue_push() --> virtqueue_fill() will produce a virtio "used
>> element" that reports the population of 1 byte in the guest buffer.
>>
>> Whereas the UEFI spec requires the GetRNG() member function to
>> return RNGValueLength bytes exactly.
>>
>> This means that in VirtioRngGetRNG(), we need a loop. But how do we
>> know the number of bytes actually produced? I will soon submit a
>> patch for VirtioLib that will expose this value. Please include
>> this patch of mine in your v2 series, and rebase your current patch
>> on top of it, using this new output value to implement the loop.
>>
> 
> OK
> 
>> (This kind of information, from this exact source, has not been
>> necessary until now -- none of the devices thus far required us to
>> rely on this info source. However, the RNG device does, and the
>> QEMU implementation means that we must not rely on the virtio
>> spec's general recommendation in 2.4.8.1, "Legacy Interface: The
>> Virtqueue Used Ring", where it says that "virtq_used_elem.len"
>> should be ignored for the legacy interface. We can't ignore it,
>> despite being a legacy guest in the virtio sense.)

Actually, I was a little bit incorrect here: in case of the virtio-net
device, we do need this kind of information. However, that device is
inherently asynchronous, so it doesn't use the transfer simplification
functions from VirtioLib, only the setup functions. Other than that, the
virtio-net device driver has its own full-ish virtio transport code,
which does handle "virtq_used_elem.len".

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

Re: [edk2] [patch] MdeModulePkg: Fix Memory Attributes table type issue

[Yao, Jiewen]

Hi Ard
I tried but I cannot reproduce it.
I build a stub memory map and a stub PE image layout and let code parse it and 
I got result below.
I cannot see the duplicated entry and missing entry.

I reviewed code again – it is pretty much straight forward, I am not sure why 
the RuntimeData entry is always overlapped by last RuntimeCode record.

I need your help here.


1)  Would you please enable VERBOSE for DxeCore and send a serial debug log 
to me for analysis?
You can use below PCD override for DxeMain.inf

  gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x80400040


2)  Would you please run SHELL command: memmap? And send log to me as well? 
I would like to know full memory map layout.

==
MemoryAttributesTable:
  Version  - 0x0001
  NumberOfEntries  - 0x0017
  DescriptorSize   - 0x0028
Entry (0xA61E020)
  Type  - 0x5
  PhysicalStart - 0x00013C0E
  VirtualStart  - 0x
  NumberOfPages - 0x0010
  Attribute - 0x4000
Entry (0xA61E048)
  Type  - 0x5
  PhysicalStart - 0x00013C0F
  VirtualStart  - 0x
  NumberOfPages - 0x0010
  Attribute - 0x0002
Entry (0xA61E070)
  Type  - 0x5
  PhysicalStart - 0x00013C10
  VirtualStart  - 0x
  NumberOfPages - 0x0030
  Attribute - 0x4000
Entry (0xA61E098)
  Type  - 0x5
  PhysicalStart - 0x00013C13
  VirtualStart  - 0x
  NumberOfPages - 0x0010
  Attribute - 0x0002
Entry (0xA61E0C0)
  Type  - 0x5
  PhysicalStart - 0x00013C14
  VirtualStart  - 0x
  NumberOfPages - 0x0020
  Attribute - 0x4000
Entry (0xA61E0E8)
  Type  - 0x6
  PhysicalStart - 0x00013C16
  VirtualStart  - 0x
  NumberOfPages - 0x0020
  Attribute - 0x4000
Entry (0xA61E110)
  Type  - 0x5
  PhysicalStart - 0x00013C1A
  VirtualStart  - 0x
  NumberOfPages - 0x0010
  Attribute - 0x4000
Entry (0xA61E138)
  Type  - 0x5
  PhysicalStart - 0x00013C1B
  VirtualStart  - 0x
  NumberOfPages - 0x0010
  Attribute - 0x0002
Entry (0xA61E160)
  Type  - 0x5
  PhysicalStart - 0x00013C1C
  VirtualStart  - 0x
  NumberOfPages - 0x0020
  Attribute - 0x4000
Entry (0xA61E188)
  Type  - 0x6
  PhysicalStart - 0x00013C1E
  VirtualStart  - 0x
  NumberOfPages - 0x00A0
  Attribute - 0x4000
Entry (0xA61E1B0)
  Type  - 0x5
  PhysicalStart - 0x00013C28
  VirtualStart  - 0x
  NumberOfPages - 0x0010
  Attribute - 0x4000
Entry (0xA61E1D8)
  Type  - 0x5
  PhysicalStart - 0x00013C29
  VirtualStart  - 0x
  NumberOfPages - 0x0010
  Attribute - 0x0002
Entry (0xA61E200)
  Type  - 0x5
  PhysicalStart - 0x00013C2A
  VirtualStart  - 0x
  NumberOfPages - 0x0030
  Attribute - 0x4000
Entry (0xA61E228)
  Type  - 0x6
  PhysicalStart - 0x00013C2D
  VirtualStart  - 0x
  NumberOfPages - 0x0050
  Attribute - 0x4000
Entry (0xA61E250)
  Type  - 0x5
  PhysicalStart - 0x00013C32
  VirtualStart  - 0x
  NumberOfPages - 0x0010
  Attribute - 0x4000
Entry (0xA61E278)
  Type  - 0x5
  PhysicalStart - 0x00013C33
  VirtualStart  - 0x
  NumberOfPages - 0x0010
  Attribute - 0x0002
Entry (0xA61E2A0)
  Type  - 0x5
  PhysicalStart - 0x00013C34
  VirtualStart  - 0x
  NumberOfPages - 0x0030
  Attribute - 0x4000
Entry (0xA61E2C8)
  Type  - 0x5
  PhysicalStart - 0x00013F65
  VirtualStart  - 0x
  NumberOfPages - 0x0010
  Attribute - 0x4000
Entry (0xA61E2F0)
  Type  - 0x5
  PhysicalStart - 0x00013F66
  VirtualStart  - 0x
  NumberOfPages - 0x0010
  Attribute - 0x0002
Entry (0xA61E318)
  Type  - 0x5
  PhysicalStart - 0x00013F67
  VirtualStart  - 0x
  NumberOfPages - 0x0030
  

Re: [edk2] [PATCH] OvmfPkg: implement UEFI driver for Virtio RNG devices

[Ard Biesheuvel]

On 19 February 2016 at 16:00, Laszlo Ersek  wrote:
> On 02/19/16 11:54, Ard Biesheuvel wrote:
>> This implements a UEFI driver model driver for Virtio devices of type
>> VIRTIO_SUBSYSTEM_ENTROPY_SOURCE, and exposes them via instances of
>> the EFI_RNG_PROTOCOL protocol, supporting the EFI_RNG_ALGORITHM_RAW
>> algorithm only.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>
>> 90% of this driver is boilerplate taken from the virtio-scsi driver.
>>
>> The purpose of this driver is to expose the EFI_RNG_PROTOCOL to the arm64
>> Linux kernel UEFI stub, which will use it to randomize the kernel address
>> space [1]
>>
>> This driver can be tested from the UEFI shell using the RngTest UEFI
>> application (SecurityPkg/Application/RngTest/RngTest.inf), example output
>> can be found after the patch
>>
>> [1] http://thread.gmane.org/gmane.linux.kernel.efi/7446
>>
>>  OvmfPkg/VirtioRngDxe/VirtioRng.c  | 593 
>>  OvmfPkg/VirtioRngDxe/VirtioRngDxe.inf |  45 ++
>>  2 files changed, 638 insertions(+)
>
> Before I try to review this driver in more detail, I'd like to ask you
> to submit a quick update as v2, with just these changes:
>
> (1) the changes you pointed out yourself, in your followup message
>

OK

> (2) If I recall correctly, it had been Jordan's request, near the
> beginning of the virtio drivers in OVMF, to split off the driver-
> internal declarations to a header file. Please do that here as
> well, for two reasons: for consistency with existing drivers, and
> so I can comapre the files more easily.
>

Sure. VirtioScsi had that as well, but I didn't see the need. I can change that.

> (3) I searched the patch for "volatile", and found none. Please, in the
> VirtioRngGetRNG() member function, allocate an appropriately sized
> array, for a (volatile UINT8 *) pointer. The virtio transfer should
> occur into this "bounce buffer" (so its address should go into the
> descriptor you put on the ring), and once the transfer is complete,
> please copy it with an open-coded loop (not CopyMem(), which drops
> pointer qualifiers) into the user's buffer. Then free the bounce
> buffer.
>
> The performance impact of this will be minimal (the guest won't
> consume huge amounts of entropy), and we must prevent the compiler
> from assuming that the buffer has not changed.
>

Could you please elaborate? How is that any different from casting the
RNGValue argument to (volatile UINT8 *) ?
Or having an automatic volatile UINT8 * on the stack?


> (4) According to the virtio specification (I'm looking at
> "virtio-v1.0-csprd05.pdf", section 5.4, Entropy Device), the device
> is allowed to return fewer than requested bytes of randomness in
> the buffers enumerated by the descriptor chain.
>
> Looking at the QEMU-side implementation [hw/virtio/virtio-rng.c],
> this is actually possible: consider the case when the guest
> requests 16 bytes of randomness, but the chr_read() function in
> QEMU is entered with size=1 bytes of randomness available.
> iov_from_buf() will copy 1 byte, the retval will be len=1, and
> virtqueue_push() --> virtqueue_fill() will produce a virtio "used
> element" that reports the population of 1 byte in the guest buffer.
>
> Whereas the UEFI spec requires the GetRNG() member function to
> return RNGValueLength bytes exactly.
>
> This means that in VirtioRngGetRNG(), we need a loop. But how do we
> know the number of bytes actually produced? I will soon submit a
> patch for VirtioLib that will expose this value. Please include
> this patch of mine in your v2 series, and rebase your current patch
> on top of it, using this new output value to implement the loop.
>

OK

> (This kind of information, from this exact source, has not been
> necessary until now -- none of the devices thus far required us to
> rely on this info source. However, the RNG device does, and the
> QEMU implementation means that we must not rely on the virtio
> spec's general recommendation in 2.4.8.1, "Legacy Interface: The
> Virtqueue Used Ring", where it says that "virtq_used_elem.len"
> should be ignored for the legacy interface. We can't ignore it,
> despite being a legacy guest in the virtio sense.)
>
> Thanks!
> Laszlo
>
>> diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c 
>> b/OvmfPkg/VirtioRngDxe/VirtioRng.c
>> new file mode 100644
>> index ..ab52f269fc3f
>> --- /dev/null
>> +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c
>> @@ -0,0 +1,593 @@
>> +/** @file
>> +
>> +  This driver produces EFI_RNG_PROTOCOL instances for virtio-rng devices.
>> +
>> +  The implementation is based on OvmfPkg/VirtioScsiDxe/VirtioScsi.c
>> +
>> +  Copyright (C) 2012, Red Hat, Inc.
>> +  Copyright (c) 2012 - 2014, Intel Corporation. All rights reserved.
>> +
>> +  This driver:
>> +
>> +  Copyright (C) 2016, Linaro Ltd.
>> +
>

Re: [edk2] [PATCH] OvmfPkg: implement UEFI driver for Virtio RNG devices

[Laszlo Ersek]

On 02/19/16 11:54, Ard Biesheuvel wrote:
> This implements a UEFI driver model driver for Virtio devices of type
> VIRTIO_SUBSYSTEM_ENTROPY_SOURCE, and exposes them via instances of
> the EFI_RNG_PROTOCOL protocol, supporting the EFI_RNG_ALGORITHM_RAW
> algorithm only.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> ---
> 
> 90% of this driver is boilerplate taken from the virtio-scsi driver.
> 
> The purpose of this driver is to expose the EFI_RNG_PROTOCOL to the arm64
> Linux kernel UEFI stub, which will use it to randomize the kernel address
> space [1]
> 
> This driver can be tested from the UEFI shell using the RngTest UEFI
> application (SecurityPkg/Application/RngTest/RngTest.inf), example output
> can be found after the patch
> 
> [1] http://thread.gmane.org/gmane.linux.kernel.efi/7446
> 
>  OvmfPkg/VirtioRngDxe/VirtioRng.c  | 593 
>  OvmfPkg/VirtioRngDxe/VirtioRngDxe.inf |  45 ++
>  2 files changed, 638 insertions(+)

Before I try to review this driver in more detail, I'd like to ask you
to submit a quick update as v2, with just these changes:

(1) the changes you pointed out yourself, in your followup message

(2) If I recall correctly, it had been Jordan's request, near the
beginning of the virtio drivers in OVMF, to split off the driver-
internal declarations to a header file. Please do that here as
well, for two reasons: for consistency with existing drivers, and
so I can comapre the files more easily.

(3) I searched the patch for "volatile", and found none. Please, in the
VirtioRngGetRNG() member function, allocate an appropriately sized
array, for a (volatile UINT8 *) pointer. The virtio transfer should
occur into this "bounce buffer" (so its address should go into the
descriptor you put on the ring), and once the transfer is complete,
please copy it with an open-coded loop (not CopyMem(), which drops
pointer qualifiers) into the user's buffer. Then free the bounce
buffer.

The performance impact of this will be minimal (the guest won't
consume huge amounts of entropy), and we must prevent the compiler
from assuming that the buffer has not changed.

(4) According to the virtio specification (I'm looking at
"virtio-v1.0-csprd05.pdf", section 5.4, Entropy Device), the device
is allowed to return fewer than requested bytes of randomness in
the buffers enumerated by the descriptor chain.

Looking at the QEMU-side implementation [hw/virtio/virtio-rng.c],
this is actually possible: consider the case when the guest
requests 16 bytes of randomness, but the chr_read() function in
QEMU is entered with size=1 bytes of randomness available.
iov_from_buf() will copy 1 byte, the retval will be len=1, and
virtqueue_push() --> virtqueue_fill() will produce a virtio "used
element" that reports the population of 1 byte in the guest buffer.

Whereas the UEFI spec requires the GetRNG() member function to
return RNGValueLength bytes exactly.

This means that in VirtioRngGetRNG(), we need a loop. But how do we
know the number of bytes actually produced? I will soon submit a
patch for VirtioLib that will expose this value. Please include
this patch of mine in your v2 series, and rebase your current patch
on top of it, using this new output value to implement the loop.

(This kind of information, from this exact source, has not been
necessary until now -- none of the devices thus far required us to
rely on this info source. However, the RNG device does, and the
QEMU implementation means that we must not rely on the virtio
spec's general recommendation in 2.4.8.1, "Legacy Interface: The
Virtqueue Used Ring", where it says that "virtq_used_elem.len"
should be ignored for the legacy interface. We can't ignore it,
despite being a legacy guest in the virtio sense.)

Thanks!
Laszlo

> diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c 
> b/OvmfPkg/VirtioRngDxe/VirtioRng.c
> new file mode 100644
> index ..ab52f269fc3f
> --- /dev/null
> +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c
> @@ -0,0 +1,593 @@
> +/** @file
> +
> +  This driver produces EFI_RNG_PROTOCOL instances for virtio-rng devices.
> +
> +  The implementation is based on OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +
> +  Copyright (C) 2012, Red Hat, Inc.
> +  Copyright (c) 2012 - 2014, Intel Corporation. All rights reserved.
> +
> +  This driver:
> +
> +  Copyright (C) 2016, Linaro Ltd.
> +
> +  This program and the accompanying materials are licensed and made available
> +  under the terms and conditions of the BSD License which accompanies this
> +  distribution. The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, 
> WITHOUT
> +  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#in

Re: [edk2] [PATCH] OvmfPkg: implement UEFI driver for Virtio RNG devices

[Ard Biesheuvel]

On 19 February 2016 at 14:49, Laszlo Ersek  wrote:
> On 02/19/16 11:54, Ard Biesheuvel wrote:
>> This implements a UEFI driver model driver for Virtio devices of type
>> VIRTIO_SUBSYSTEM_ENTROPY_SOURCE, and exposes them via instances of
>> the EFI_RNG_PROTOCOL protocol, supporting the EFI_RNG_ALGORITHM_RAW
>> algorithm only.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>
>> 90% of this driver is boilerplate taken from the virtio-scsi driver.
>>
>> The purpose of this driver is to expose the EFI_RNG_PROTOCOL to the arm64
>> Linux kernel UEFI stub, which will use it to randomize the kernel address
>> space [1]
>>
>> This driver can be tested from the UEFI shell using the RngTest UEFI
>> application (SecurityPkg/Application/RngTest/RngTest.inf), example output
>> can be found after the patch
>>
>> [1] http://thread.gmane.org/gmane.linux.kernel.efi/7446
>>
>>  OvmfPkg/VirtioRngDxe/VirtioRng.c  | 593 
>>  OvmfPkg/VirtioRngDxe/VirtioRngDxe.inf |  45 ++
>>  2 files changed, 638 insertions(+)
>
> Can you push this to your repo please?
>

Weblink:
https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/shortlog/refs/heads/virtio-rng

Git URL:
git://git.linaro.org/people/ard.biesheuvel/uefi-next.git virtio-rng



>> diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c 
>> b/OvmfPkg/VirtioRngDxe/VirtioRng.c
>> new file mode 100644
>> index ..ab52f269fc3f
>> --- /dev/null
>> +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c
>> @@ -0,0 +1,593 @@
>> +/** @file
>> +
>> +  This driver produces EFI_RNG_PROTOCOL instances for virtio-rng devices.
>> +
>> +  The implementation is based on OvmfPkg/VirtioScsiDxe/VirtioScsi.c
>> +
>> +  Copyright (C) 2012, Red Hat, Inc.
>> +  Copyright (c) 2012 - 2014, Intel Corporation. All rights reserved.
>> +
>> +  This driver:
>> +
>> +  Copyright (C) 2016, Linaro Ltd.
>> +
>> +  This program and the accompanying materials are licensed and made 
>> available
>> +  under the terms and conditions of the BSD License which accompanies this
>> +  distribution. The full text of the license may be found at
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, 
>> WITHOUT
>> +  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include 
>> +
>> +#define VIRTIO_RNG_SIG  SIGNATURE_32 ('V', 'R', 'N', 'G')
>> +
>> +typedef struct {
>> +  UINT32  Signature;
>> +  VIRTIO_DEVICE_PROTOCOL  *VirtIo;
>> +  EFI_EVENT   ExitBoot;
>> +  VRING   Ring;
>> +  EFI_RNG_PROTOCOLRng;
>> +} VIRTIO_RNG_DEV;
>> +
>> +#define VIRTIO_ENTROPY_SOURCE_FROM_RNG(RngPointer) \
>> +CR (RngPointer, VIRTIO_RNG_DEV, Rng, VIRTIO_RNG_SIG)
>> +
>> +
>> +/**
>> +  Returns information about the random number generation implementation.
>> +
>> +  @param[in] This A pointer to the EFI_RNG_PROTOCOL 
>> instance.
>> +  @param[in,out] RNGAlgorithmListSize On input, the size in bytes of 
>> RNGAlgorithmList.
>> +  On output with a return code of 
>> EFI_SUCCESS, the size
>> +  in bytes of the data returned in 
>> RNGAlgorithmList. On output
>> +  with a return code of 
>> EFI_BUFFER_TOO_SMALL,
>> +  the size of RNGAlgorithmList required 
>> to obtain the list.
>> +  @param[out] RNGAlgorithmListA caller-allocated memory buffer 
>> filled by the driver
>> +  with one EFI_RNG_ALGORITHM element 
>> for each supported
>> +  RNG algorithm. The list must not 
>> change across multiple
>> +  calls to the same driver. The first 
>> algorithm in the list
>> +  is the default algorithm for the 
>> driver.
>> +
>> +  @retval EFI_SUCCESS The RNG algorithm list was returned 
>> successfully.
>> +  @retval EFI_UNSUPPORTED The services is not supported by this 
>> driver.
>> +  @retval EFI_DEVICE_ERRORThe list of algorithms could not be 
>> retrieved due to a
>> +  hardware or firmware error.
>> +  @retval EFI_INVALID_PARAMETER   One or more of the parameters are 
>> incorrect.
>> +  @retval EFI_BUFFER_TOO_SMALLThe buffer RNGAlgorithmList is too 
>> small to hold the result.
>> +
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +EFIAPI
>> +VirtioRngGetInfo (
>> +  IN  EFI_RNG_PROTOCOL*This,
>> +  IN OUT  UINTN   *RNGAlgorithmListSize,
>> +  OUT EFI_RNG_ALGORITHM   *RNGAlgorithmList
>> +  )
>> +{
>> +  ASSER

Re: [edk2] [PATCH] OvmfPkg: implement UEFI driver for Virtio RNG devices

[Laszlo Ersek]

On 02/19/16 11:54, Ard Biesheuvel wrote:
> This implements a UEFI driver model driver for Virtio devices of type
> VIRTIO_SUBSYSTEM_ENTROPY_SOURCE, and exposes them via instances of
> the EFI_RNG_PROTOCOL protocol, supporting the EFI_RNG_ALGORITHM_RAW
> algorithm only.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> ---
> 
> 90% of this driver is boilerplate taken from the virtio-scsi driver.
> 
> The purpose of this driver is to expose the EFI_RNG_PROTOCOL to the arm64
> Linux kernel UEFI stub, which will use it to randomize the kernel address
> space [1]
> 
> This driver can be tested from the UEFI shell using the RngTest UEFI
> application (SecurityPkg/Application/RngTest/RngTest.inf), example output
> can be found after the patch
> 
> [1] http://thread.gmane.org/gmane.linux.kernel.efi/7446
> 
>  OvmfPkg/VirtioRngDxe/VirtioRng.c  | 593 
>  OvmfPkg/VirtioRngDxe/VirtioRngDxe.inf |  45 ++
>  2 files changed, 638 insertions(+)

Can you push this to your repo please?

Thanks
Laszlo

> diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c 
> b/OvmfPkg/VirtioRngDxe/VirtioRng.c
> new file mode 100644
> index ..ab52f269fc3f
> --- /dev/null
> +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c
> @@ -0,0 +1,593 @@
> +/** @file
> +
> +  This driver produces EFI_RNG_PROTOCOL instances for virtio-rng devices.
> +
> +  The implementation is based on OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +
> +  Copyright (C) 2012, Red Hat, Inc.
> +  Copyright (c) 2012 - 2014, Intel Corporation. All rights reserved.
> +
> +  This driver:
> +
> +  Copyright (C) 2016, Linaro Ltd.
> +
> +  This program and the accompanying materials are licensed and made available
> +  under the terms and conditions of the BSD License which accompanies this
> +  distribution. The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, 
> WITHOUT
> +  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#define VIRTIO_RNG_SIG  SIGNATURE_32 ('V', 'R', 'N', 'G')
> +
> +typedef struct {
> +  UINT32  Signature;
> +  VIRTIO_DEVICE_PROTOCOL  *VirtIo;
> +  EFI_EVENT   ExitBoot;
> +  VRING   Ring;
> +  EFI_RNG_PROTOCOLRng;
> +} VIRTIO_RNG_DEV;
> +
> +#define VIRTIO_ENTROPY_SOURCE_FROM_RNG(RngPointer) \
> +CR (RngPointer, VIRTIO_RNG_DEV, Rng, VIRTIO_RNG_SIG)
> +
> +
> +/**
> +  Returns information about the random number generation implementation.
> +
> +  @param[in] This A pointer to the EFI_RNG_PROTOCOL 
> instance.
> +  @param[in,out] RNGAlgorithmListSize On input, the size in bytes of 
> RNGAlgorithmList. 
> +  On output with a return code of 
> EFI_SUCCESS, the size
> +  in bytes of the data returned in 
> RNGAlgorithmList. On output
> +  with a return code of 
> EFI_BUFFER_TOO_SMALL,
> +  the size of RNGAlgorithmList required 
> to obtain the list.
> +  @param[out] RNGAlgorithmListA caller-allocated memory buffer 
> filled by the driver
> +  with one EFI_RNG_ALGORITHM element for 
> each supported
> +  RNG algorithm. The list must not 
> change across multiple
> +  calls to the same driver. The first 
> algorithm in the list
> +  is the default algorithm for the 
> driver.
> +
> +  @retval EFI_SUCCESS The RNG algorithm list was returned 
> successfully.
> +  @retval EFI_UNSUPPORTED The services is not supported by this 
> driver.
> +  @retval EFI_DEVICE_ERRORThe list of algorithms could not be 
> retrieved due to a
> +  hardware or firmware error.
> +  @retval EFI_INVALID_PARAMETER   One or more of the parameters are 
> incorrect.
> +  @retval EFI_BUFFER_TOO_SMALLThe buffer RNGAlgorithmList is too 
> small to hold the result.
> +
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +VirtioRngGetInfo (
> +  IN  EFI_RNG_PROTOCOL*This,
> +  IN OUT  UINTN   *RNGAlgorithmListSize,
> +  OUT EFI_RNG_ALGORITHM   *RNGAlgorithmList
> +  )
> +{
> +  ASSERT (This != NULL);
> +  ASSERT (RNGAlgorithmListSize != NULL);
> +
> +  if (*RNGAlgorithmListSize < sizeof (EFI_RNG_ALGORITHM)) {
> +*RNGAlgorithmListSize = sizeof (EFI_RNG_ALGORITHM);
> +return EFI_BUFFER_TOO_SMALL;
> +  }
> +
> +  ASSERT (RNGAlgorithmList != NULL);
> +
> +  *RNGAlgorithmListSize = sizeof (EFI_RNG_ALGORITHM);
> +  CopyGuid (

Re: [edk2] [PATCH v3 4/4] ArmVirtPkg/ArmVirt.dsc.inc: lift 4 GB limit for PEI accessible memory

[Laszlo Ersek]

On 02/19/16 14:15, Ard Biesheuvel wrote:
> Forcing allocations to be served from memory below 4 GB does not make
> sense on ARM or AARCH64, either because this limit is implicit (ARM),
> or because the architecture does not impose such a limit (AARCH64), in
> which case such allocations may even fail inadvertently simply because
> no memory at all exists below the 4 GB mark.
> 
> So set gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdPeiAllocMemLimit4GB
> to FALSE.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmVirtPkg/ArmVirt.dsc.inc | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index 49e4264ee8a4..8f83b0df4352 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -275,6 +275,8 @@ [PcdsFeatureFlag.common]
>  
>gEfiMdeModulePkgTokenSpaceGuid.PcdTurnOffUsbLegacySupport|TRUE
>  
> +  gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdPeiAllocMemLimit4GB|FALSE
> +
>  [PcdsFixedAtBuild.common]
>gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength|100
>gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength|100
> 

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

Re: [edk2] [PATCH v3 3/4] ArmVirtPkg/ArmVirtQemu: limit ACPI support to v5.0 and higher

[Laszlo Ersek]

On 02/19/16 14:15, Ard Biesheuvel wrote:
> The ACPI spec predates the AARCH64 architecture by 5 versions, so there
> is no point in supporting anything below v5.0. So set the PCD that
> controls the ACPI table generation to the appropriate value.
> 
> Note that the current consumers of this PCD only check whether bit 1
> is set or not (i.e., ACPI v1.0b), but this may change in the future,
> so let's choose a meaningful value right away.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmVirtPkg/ArmVirtQemu.dsc | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index e2641fd2c289..db6d277972d3 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -177,6 +177,10 @@ [PcdsFixedAtBuild.AARCH64]
># point only, for entry point versions >= 3.0.
>gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosEntryPointProvideMethod|0x2
>  
> +  # ACPI predates the AARCH64 by 5 versions, so we only target OSes that
> +  # support ACPI v5.0 or later
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions|0x20
> +
>  [PcdsDynamicDefault.common]
>## If TRUE, OvmfPkg/AcpiPlatformDxe will not wait for PCI
>#  enumeration to complete before installing ACPI tables.
> 

Okay, now moved to [PcdsFixedAtBuild.AARCH64].

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

Re: [edk2] [PATCH v3 2/4] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB if needed

[Laszlo Ersek]

On 02/19/16 14:15, Ard Biesheuvel wrote:
> The reserved region for storing performance data is allocated below 4 GB,
> so that its contents are accessible from PEI upon S3 resume. However, this
> is a X64 pecularity, and on AARCH64 systems, which does not have this
> restriction (nor does it have S3, btw), this limit may cause the allocation
> to fail (if the platform does not have memory below 4 GB) or needlessly
> fragment the memory map if it does succeed.
> 
> So make this behavior conditional, based on the new PcdPeiAllocMemLimit4GB
> PCD, which defaults to TRUE, but can be overridden by platforms if they
> prefer allocations of PEI accessible memory to be served from anywhere.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> ---
>  IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec |  6 ++
>  IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf |  7 ---
>  IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c | 13 +++--
>  3 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec 
> b/IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec
> index 8bbde8e2c9c8..005d356f5938 100644
> --- a/IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec
> +++ b/IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec
> @@ -161,6 +161,12 @@ [PcdsFeatureFlag]
># @Prompt Enable Boot Logo only
>
> gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdBootlogoOnlyEnable|FALSE|BOOLEAN|0x00010048
>  
> +  ## Indicates whether allocations for PEI accessible memory should be below 
> the 4 GB mark
> +  #   TRUE  - PEI accessible memory should be allocated below 4 GB.
> +  #   FALSE - PEI accessible memory may be allocated anywhere.
> +  # @Prompt Allocated PEI accessible memory below 4 GB
> +  
> gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdPeiAllocMemLimit4GB|TRUE|BOOLEAN|0x00010049
> +
>  [PcdsFixedAtBuild, PcdsPatchableInModule]
>## FFS filename to find the default BMP Logo file.
># @Prompt FFS Name of Boot Logo File
> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf 
> b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
> index 6afb8a09df9c..fa210836f410 100644
> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
> @@ -181,9 +181,10 @@ [Protocols]
>gEdkiiVariableLockProtocolGuid## SOMETIMES_CONSUMES
>  
>  [FeaturePcd]
> -  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLangDeprecate## CONSUMES
> -  gEfiMdePkgTokenSpaceGuid.PcdUgaConsumeSupport   ## CONSUMES
> -  gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdBootlogoOnlyEnable ## CONSUMES
> +  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLangDeprecate  ## 
> CONSUMES
> +  gEfiMdePkgTokenSpaceGuid.PcdUgaConsumeSupport ## 
> CONSUMES
> +  gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdBootlogoOnlyEnable   ## 
> CONSUMES
> +  gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdPeiAllocMemLimit4GB  ## 
> CONSUMES
>  
>  [Pcd]
>gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLangCodes  ## 
> SOMETIMES_CONSUMES
> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c 
> b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
> index ae7ad2153c51..3b524db94b3d 100644
> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
> @@ -474,14 +474,23 @@ BdsAllocateMemoryForPerformanceData (
>EFI_STATUSStatus;
>EFI_PHYSICAL_ADDRESS  AcpiLowMemoryBase;
>EDKII_VARIABLE_LOCK_PROTOCOL  *VariableLock;
> +  EFI_ALLOCATE_TYPE AllocType;
>  
> -  AcpiLowMemoryBase = 0x0ULL;
> +  //
> +  // The memory we allocate here must be accessible from PEI at resume.
> +  //
> +  if (FeaturePcdGet (PcdPeiAllocMemLimit4GB)) {
> +AcpiLowMemoryBase = 0x0ULL;
> +AllocType = AllocateMaxAddress;
> +  } else {
> +AllocType = AllocateAnyPages;
> +  }
>  
>//
>// Allocate a block of memory that will contain performance data to OS.
>//
>Status = gBS->AllocatePages (
> -  AllocateMaxAddress,
> +  AllocType,
>EfiReservedMemoryType,
>EFI_SIZE_TO_PAGES (PERF_DATA_MAX_LENGTH),
>&AcpiLowMemoryBase
> 

My review is of course not authoritative for this package, but still:

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

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

[Laszlo Ersek]

On 02/19/16 10:48, David Woodhouse wrote:
> On Fri, 2016-02-19 at 10:43 +0100, Laszlo Ersek wrote:
>>
>> I can test this for you, if you give me precise instructions.
>>
>> (I'm asking for instructions because CryptoPkg/Include/openssl/README is
>> deleted in one of the early patches.)
> 
> It moved from Patch-HOWTO.txt to OpenSSL-HOWTO.txt since there was no
> longer a patch involved. Or did I forget to add the new file?

No, the file is there alright.

So, I started executing my test plan. (If you hadn't snipped it, I could
now refer to it more easily. ;))

Using your master branch at c46b545e4193fc9fbe7807ff17883b44a1bf8305
("CryptoPkg: Abuse internal headers to make OpenSSL HEAD build work"),
and cloning/using your openssl repo at
f2bfbfd71e57e3802863ef5c2332fc6217c99f64 ("RT4175: Fix regression using
PKCS7_verify() with Authenticode"), I get the following build failure in
step (1b), building for IA32:

In file included from
.../CryptoPkg/Library/OpensslLib/openssl/crypto/bn/bn_add.c:59:0:
.../CryptoPkg/Library/OpensslLib/openssl/crypto/bn/bn_lcl.h:114:31:
fatal error: internal/bn_conf.h: No such file or directory
 # include "internal/bn_conf.h"
   ^

I get the same error for the Ia32X64, and the X64 builds as well.

I also tried with ArmVirtPkg/ArmVirtQemu, for AARCH64: same build error.

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

[edk2] [PATCH v3 4/4] ArmVirtPkg/ArmVirt.dsc.inc: lift 4 GB limit for PEI accessible memory

[Ard Biesheuvel]

Forcing allocations to be served from memory below 4 GB does not make
sense on ARM or AARCH64, either because this limit is implicit (ARM),
or because the architecture does not impose such a limit (AARCH64), in
which case such allocations may even fail inadvertently simply because
no memory at all exists below the 4 GB mark.

So set gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdPeiAllocMemLimit4GB
to FALSE.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
 ArmVirtPkg/ArmVirt.dsc.inc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 49e4264ee8a4..8f83b0df4352 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -275,6 +275,8 @@ [PcdsFeatureFlag.common]
 
   gEfiMdeModulePkgTokenSpaceGuid.PcdTurnOffUsbLegacySupport|TRUE
 
+  gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdPeiAllocMemLimit4GB|FALSE
+
 [PcdsFixedAtBuild.common]
   gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength|100
   gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength|100
-- 
2.5.0

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

[edk2] [PATCH v3 2/4] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB if needed

[Ard Biesheuvel]

The reserved region for storing performance data is allocated below 4 GB,
so that its contents are accessible from PEI upon S3 resume. However, this
is a X64 pecularity, and on AARCH64 systems, which does not have this
restriction (nor does it have S3, btw), this limit may cause the allocation
to fail (if the platform does not have memory below 4 GB) or needlessly
fragment the memory map if it does succeed.

So make this behavior conditional, based on the new PcdPeiAllocMemLimit4GB
PCD, which defaults to TRUE, but can be overridden by platforms if they
prefer allocations of PEI accessible memory to be served from anywhere.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
 IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec |  6 ++
 IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf |  7 ---
 IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c | 13 +++--
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec 
b/IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec
index 8bbde8e2c9c8..005d356f5938 100644
--- a/IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec
+++ b/IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec
@@ -161,6 +161,12 @@ [PcdsFeatureFlag]
   # @Prompt Enable Boot Logo only
   
gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdBootlogoOnlyEnable|FALSE|BOOLEAN|0x00010048
 
+  ## Indicates whether allocations for PEI accessible memory should be below 
the 4 GB mark
+  #   TRUE  - PEI accessible memory should be allocated below 4 GB.
+  #   FALSE - PEI accessible memory may be allocated anywhere.
+  # @Prompt Allocated PEI accessible memory below 4 GB
+  
gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdPeiAllocMemLimit4GB|TRUE|BOOLEAN|0x00010049
+
 [PcdsFixedAtBuild, PcdsPatchableInModule]
   ## FFS filename to find the default BMP Logo file.
   # @Prompt FFS Name of Boot Logo File
diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf 
b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
index 6afb8a09df9c..fa210836f410 100644
--- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
+++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
@@ -181,9 +181,10 @@ [Protocols]
   gEdkiiVariableLockProtocolGuid## SOMETIMES_CONSUMES
 
 [FeaturePcd]
-  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLangDeprecate## CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdUgaConsumeSupport   ## CONSUMES
-  gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdBootlogoOnlyEnable ## CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLangDeprecate  ## CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdUgaConsumeSupport ## CONSUMES
+  gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdBootlogoOnlyEnable   ## CONSUMES
+  gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdPeiAllocMemLimit4GB  ## CONSUMES
 
 [Pcd]
   gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLangCodes  ## 
SOMETIMES_CONSUMES
diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c 
b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
index ae7ad2153c51..3b524db94b3d 100644
--- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -474,14 +474,23 @@ BdsAllocateMemoryForPerformanceData (
   EFI_STATUSStatus;
   EFI_PHYSICAL_ADDRESS  AcpiLowMemoryBase;
   EDKII_VARIABLE_LOCK_PROTOCOL  *VariableLock;
+  EFI_ALLOCATE_TYPE AllocType;
 
-  AcpiLowMemoryBase = 0x0ULL;
+  //
+  // The memory we allocate here must be accessible from PEI at resume.
+  //
+  if (FeaturePcdGet (PcdPeiAllocMemLimit4GB)) {
+AcpiLowMemoryBase = 0x0ULL;
+AllocType = AllocateMaxAddress;
+  } else {
+AllocType = AllocateAnyPages;
+  }
 
   //
   // Allocate a block of memory that will contain performance data to OS.
   //
   Status = gBS->AllocatePages (
-  AllocateMaxAddress,
+  AllocType,
   EfiReservedMemoryType,
   EFI_SIZE_TO_PAGES (PERF_DATA_MAX_LENGTH),
   &AcpiLowMemoryBase
-- 
2.5.0

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

[edk2] [PATCH v3 3/4] ArmVirtPkg/ArmVirtQemu: limit ACPI support to v5.0 and higher

[Ard Biesheuvel]

The ACPI spec predates the AARCH64 architecture by 5 versions, so there
is no point in supporting anything below v5.0. So set the PCD that
controls the ACPI table generation to the appropriate value.

Note that the current consumers of this PCD only check whether bit 1
is set or not (i.e., ACPI v1.0b), but this may change in the future,
so let's choose a meaningful value right away.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
 ArmVirtPkg/ArmVirtQemu.dsc | 4 
 1 file changed, 4 insertions(+)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index e2641fd2c289..db6d277972d3 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -177,6 +177,10 @@ [PcdsFixedAtBuild.AARCH64]
   # point only, for entry point versions >= 3.0.
   gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosEntryPointProvideMethod|0x2
 
+  # ACPI predates the AARCH64 by 5 versions, so we only target OSes that
+  # support ACPI v5.0 or later
+  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions|0x20
+
 [PcdsDynamicDefault.common]
   ## If TRUE, OvmfPkg/AcpiPlatformDxe will not wait for PCI
   #  enumeration to complete before installing ACPI tables.
-- 
2.5.0

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

[edk2] [PATCH v3 0/4] lift AARCH64 4GB allocation restriction

[Ard Biesheuvel]

On AARCH64, there is no requirement to support OSes that can only access
ACPI tables in low memory (i.e., below 4 GB), and there are reasons to
prefer allocations higher up. First of all, since DMA capable devices may
only be able to access 32-bit addressable memory. Additionally, keeping
memory reservations close together makes it easier for the OS to map
system RAM efficiently (with as few levels of translation as possible)

So introduce a PCD that inhibits the generation of ACPI tables that require
them to be allocated in memory below 4 GB, and wire it up to the allocation
of performance data memory as well. (#1)

Secondly, introduce a PCD under IntelFrameworkModulePkg that controls the
behavior regarding allocations performed in DXE that need to be accessible
from PEI, and hence may need to be in the lowest 4 GB of memory. (#2)

Finally, set the PCDs for ArmVirt[Qemu]. (#3 and #4)

Changes since v2:
- folded suggestion and added R-b from Jiewen (#1)
- update patch #2 to use a dedicated PCD rather than reusing the ACPI table
  version on
- move setting of first PCD to AARCH64 specific section
- added patch to set the second PCD for ArmVirt.dsc.inc

Ard Biesheuvel (4):
  MdeModulePkg: AcpiTableDxe: make 4 GB table allocation limit optional
  IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB if needed
  ArmVirtPkg/ArmVirtQemu: limit ACPI support to v5.0 and higher
  ArmVirtPkg/ArmVirt.dsc.inc: lift 4 GB limit for PEI accessible memory

 ArmVirtPkg/ArmVirt.dsc.inc   |   2 +
 ArmVirtPkg/ArmVirtQemu.dsc   |   4 +
 IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec  |   6 +
 IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf  |   7 +-
 IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c  |  13 +-
 MdeModulePkg/MdeModulePkg.dec|  11 +
 MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c   |   3 +-
 MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf|   2 +
 MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 448 

 9 files changed, 304 insertions(+), 192 deletions(-)

-- 
2.5.0

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

[edk2] [PATCH v3 1/4] MdeModulePkg: AcpiTableDxe: make 4 GB table allocation limit optional

[Ard Biesheuvel]

AARCH64 systems never require compatibility with legacy ACPI OSes, and
may not have any 32-bit addressable system RAM. To support ACPI on these
systems, we need to be able to relax the 4 GB allocation restriction.

So add a PCD PcdAcpiExposedTableVersions containing a bitmask describing
which ACPI versions are targeted, and wire it up it up to the memory
allocation calls in AcpiTableDxe/AcpiTableProtocol.c. I.e., if ACPI v1.0b
is not among the supported versions, the memory allocations are not limited
to 4 GB, and only table types that carry 64-bit addresses are emitted.

Note that this will inhibit the publishing of any tables that carry only
32-bit addresses, i.e., RSDPv1, RSDTv1 and RSDTv3.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
Reviewed-by: "Yao, Jiewen" 
---
 MdeModulePkg/MdeModulePkg.dec|  11 +
 MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c   |   3 +-
 MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf|   2 +
 MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 448 

 4 files changed, 277 insertions(+), 187 deletions(-)

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 623d8ed68286..af7bcab3baf6 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -4,6 +4,7 @@
 # and libraries instances, which are used for those modules.
 #
 # Copyright (c) 2007 - 2016, Intel Corporation. All rights reserved.
+# Copyright (c) 2016, Linaro Ltd. All rights reserved.
 # This program and the accompanying materials are licensed and made available 
under
 # the terms and conditions of the BSD License that accompanies this 
distribution.
 # The full text of the license may be found at
@@ -1049,6 +1050,16 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
   # @Prompt Driver guid array of VFR drivers for VarCheckHiiBin generation.
   gEfiMdeModulePkgTokenSpaceGuid.PcdVarCheckVfrDriverGuidArray|{ 0x00, 0x00, 
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
0x00 }|VOID*|0x3000103A
 
+  ## Indicates which ACPI versions are targeted by the ACPI tables exposed to 
the OS
+  #  These values are aligned with the definitions in 
MdePkg/Include/Protocol/AcpiSystemDescriptionTable.h
+  #   BIT 1 - EFI_ACPI_TABLE_VERSION_1_0B.
+  #   BIT 2 - EFI_ACPI_TABLE_VERSION_2_0.
+  #   BIT 3 - EFI_ACPI_TABLE_VERSION_3_0.
+  #   BIT 4 - EFI_ACPI_TABLE_VERSION_4_0.
+  #   BIT 5 - EFI_ACPI_TABLE_VERSION_5_0.
+  # @Prompt Exposed ACPI table versions.
+  
gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions|0x3E|UINT32|0x0001004c
+
 [PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
   ## This PCD defines the Console output row. The default value is 25 
according to UEFI spec.
   #  This PCD could be set to 0 then console output would be at max column and 
max row.
diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c 
b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c
index 57fdc7844e93..db693900758a 100644
--- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c
+++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c
@@ -19,7 +19,7 @@
 
 GLOBAL_REMOVE_IF_UNREFERENCED
 EFI_ACPI_SDT_PROTOCOL  mAcpiSdtProtocolTemplate = {
-  EFI_ACPI_TABLE_VERSION_NONE | EFI_ACPI_TABLE_VERSION_1_0B | 
ACPI_TABLE_VERSION_GTE_2_0,
+  EFI_ACPI_TABLE_VERSION_NONE,
   GetAcpiTable2,
   RegisterNotify,
   Open,
@@ -1102,6 +1102,7 @@ SdtAcpiTableAcpiSdtConstructor (
 
   InitializeListHead (&AcpiTableInstance->NotifyList);
   CopyMem (&AcpiTableInstance->AcpiSdtProtocol, &mAcpiSdtProtocolTemplate, 
sizeof(mAcpiSdtProtocolTemplate));
+  AcpiTableInstance->AcpiSdtProtocol.AcpiVersion = 
(EFI_ACPI_TABLE_VERSION)PcdGet32 (PcdAcpiExposedTableVersions);
 
   //
   // Register event for ExitPmAuth, so that we can uninstall ACPI SDT protocol 
after ExitPmAuth.
diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf 
b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
index e9cd728dbfb6..0fb2c9cfb52e 100644
--- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
+++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
@@ -2,6 +2,7 @@
 #  ACPI Table Protocol Driver
 #
 #  Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.
+#  Copyright (c) 2016, Linaro Ltd. All rights reserved.
 #  This program and the accompanying materials are
 #  licensed and made available under the terms and conditions of the BSD 
License
 #  which accompanies this distribution.  The full text of the license may be 
found at
@@ -67,6 +68,7 @@ [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemRevision  ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorId## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorRevision  ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions## CONSUMES
 
 [Protocols]
   gEfiAcpiTableProtocolGuid ## PRODUC

Re: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB on ACPI 1.0

[Laszlo Ersek]

On 02/19/16 13:50, Ard Biesheuvel wrote:
> On 19 February 2016 at 13:48, Laszlo Ersek  wrote:
>> On 02/19/16 13:40, Ard Biesheuvel wrote:
>>> On 19 February 2016 at 13:33, Laszlo Ersek  wrote:
>>
 I think customizing this code for MDE_CPU_AARCH64, on the source code
 level, is not good.

 Here we are under IntelFrameworkModulePkg/Universal/BdsDxe/, which has
 two consequences:

 - it says Universal, so it shouldn't be specific to architecture on the
 source code level,
 - it says IntelFrameworkModulePkg, so you have a PCD namespace that
 doesn't clutter MdePkg's / MdeModulePkg's.

 Also, you mentioned earlier that memory layout is not specific to ISA,
 but platform, hence MDE_CPU_AARCH64 would not be a perfect match anyway.

 My vote is to introduce a new PCD that controls this allocation limit.
 After all, it can depend on a bunch of things:
 - Whether you have S3 or not
 - whether PEI can address all of the memory or just 4GB
 - whether your platform has RAM under 4GB
 - whether allocating under 4GB is detrimental to performance

 and so on. I think this is the textbook case for a new PCD.

>>>
>>> OK, fair enough. That still means a PCD with different default values
>>> for X64 and other archs, but that is apparently not something to care
>>> about.
>>
>> Sorry, I don't understand; perhaps I missed something earlier.
>>
>> Your patch changes AllocateMaxAddress into AllocateAnyPages,
>> conditionally, for the "AcpiLowMemoryBase" allocation. Why is it not
>> enough to enable this change (with the PCD) for AARCH64 only? Why must
>> X64 differ?
>>
> 
> The only downside is that AARCH64 platforms don't get the change 'for
> free' but have to set this new PCD. But thinking about it, that is not
> entirely unreasonable ...

Yes: they are the freaks with no RAM under 4GB! ;)

Laszlo

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

Re: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB on ACPI 1.0

[Ard Biesheuvel]

On 19 February 2016 at 13:48, Laszlo Ersek  wrote:
> On 02/19/16 13:40, Ard Biesheuvel wrote:
>> On 19 February 2016 at 13:33, Laszlo Ersek  wrote:
>
>>> I think customizing this code for MDE_CPU_AARCH64, on the source code
>>> level, is not good.
>>>
>>> Here we are under IntelFrameworkModulePkg/Universal/BdsDxe/, which has
>>> two consequences:
>>>
>>> - it says Universal, so it shouldn't be specific to architecture on the
>>> source code level,
>>> - it says IntelFrameworkModulePkg, so you have a PCD namespace that
>>> doesn't clutter MdePkg's / MdeModulePkg's.
>>>
>>> Also, you mentioned earlier that memory layout is not specific to ISA,
>>> but platform, hence MDE_CPU_AARCH64 would not be a perfect match anyway.
>>>
>>> My vote is to introduce a new PCD that controls this allocation limit.
>>> After all, it can depend on a bunch of things:
>>> - Whether you have S3 or not
>>> - whether PEI can address all of the memory or just 4GB
>>> - whether your platform has RAM under 4GB
>>> - whether allocating under 4GB is detrimental to performance
>>>
>>> and so on. I think this is the textbook case for a new PCD.
>>>
>>
>> OK, fair enough. That still means a PCD with different default values
>> for X64 and other archs, but that is apparently not something to care
>> about.
>
> Sorry, I don't understand; perhaps I missed something earlier.
>
> Your patch changes AllocateMaxAddress into AllocateAnyPages,
> conditionally, for the "AcpiLowMemoryBase" allocation. Why is it not
> enough to enable this change (with the PCD) for AARCH64 only? Why must
> X64 differ?
>

The only downside is that AARCH64 platforms don't get the change 'for
free' but have to set this new PCD. But thinking about it, that is not
entirely unreasonable ...
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB on ACPI 1.0

[Laszlo Ersek]

On 02/19/16 13:40, Ard Biesheuvel wrote:
> On 19 February 2016 at 13:33, Laszlo Ersek  wrote:

>> I think customizing this code for MDE_CPU_AARCH64, on the source code
>> level, is not good.
>>
>> Here we are under IntelFrameworkModulePkg/Universal/BdsDxe/, which has
>> two consequences:
>>
>> - it says Universal, so it shouldn't be specific to architecture on the
>> source code level,
>> - it says IntelFrameworkModulePkg, so you have a PCD namespace that
>> doesn't clutter MdePkg's / MdeModulePkg's.
>>
>> Also, you mentioned earlier that memory layout is not specific to ISA,
>> but platform, hence MDE_CPU_AARCH64 would not be a perfect match anyway.
>>
>> My vote is to introduce a new PCD that controls this allocation limit.
>> After all, it can depend on a bunch of things:
>> - Whether you have S3 or not
>> - whether PEI can address all of the memory or just 4GB
>> - whether your platform has RAM under 4GB
>> - whether allocating under 4GB is detrimental to performance
>>
>> and so on. I think this is the textbook case for a new PCD.
>>
> 
> OK, fair enough. That still means a PCD with different default values
> for X64 and other archs, but that is apparently not something to care
> about.

Sorry, I don't understand; perhaps I missed something earlier.

Your patch changes AllocateMaxAddress into AllocateAnyPages,
conditionally, for the "AcpiLowMemoryBase" allocation. Why is it not
enough to enable this change (with the PCD) for AARCH64 only? Why must
X64 differ?

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

Re: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB on ACPI 1.0

[Ard Biesheuvel]

On 19 February 2016 at 13:33, Laszlo Ersek  wrote:
> On 02/19/16 13:18, Ard Biesheuvel wrote:
>> On 19 February 2016 at 13:14, Laszlo Ersek  wrote:
>>> On 02/19/16 12:52, Ard Biesheuvel wrote:
 On 19 February 2016 at 10:56, Laszlo Ersek  wrote:
> On 02/19/16 09:53, Ard Biesheuvel wrote:
>> On 19 February 2016 at 09:45, Yao, Jiewen  wrote:
>>> I can explain the reason on allocating <4G. It is because this data 
>>> will be used in PEI phase in S3 resume.
>>>
>>> On most X86 platform, PEI phase is 32bit, and DXE phase is 32bit or 
>>> 64bit. So we have to limit the allocation <4G.
>>>
>>
>> OK, got it.
>>
>>> Using ACPI version PCD looks strange here. Maybe another PCD, like 
>>> PcdDxeIplSwitchToLongMode?
>>>
>>
>> But that does not tell us if we are running a 32-bit PEI, right? It
>> only tells us if a 32-bit PEI should load a 32-bit DXE core on a
>> 64-bit capable machine.
>
> 32->32 and 64->64 builds have PcdDxeIplSwitchToLongMode set to FALSE.
>
> Only 32->64 builds have PcdDxeIplSwitchToLongMode set to TRUE.
>
> (This is what we have in the three DSC files of OVMF.)
>
> So, in order to see in DXE if your PEI is 32-bit, you can do:
>
>   BOOLEAN PeiIs32Bit;
>
>   PeiIs32Bit = FALSE;
>   if (FeaturePcdGet (PcdDxeIplSwitchToLongMode)) {
> //
> // this implies a 32->64 switch
> //
> PeiIs32Bit = TRUE;
>   } else {
> //
> // otherwise, PEI and DXE have the same bitness,
> // so derive it from DXE's bitness
> //
> #if defined (MDE_CPU_IA32) || defined (MDE_CPU_ARM)
> PeiIs32Bit = TRUE;
> #endif
>   }
>
> Alternatively:
>
>   PeiIs32Bit = FeaturePcdGet (PcdDxeIplSwitchToLongMode) ||
>sizeof (UINTN) == sizeof (UINT32);
>
> I'll admit I'm not really sure about EBC. The above mostly treats EBC as
> nonexistent. :)
>

 Thanks for clarifying.

 So the only case where we have to take care to allocate below 4 GB is
 the 32->64 case, since in all other cases, the memory allocated in DXE
 will always be addressable in PEI.
>>>
>>> I don't think so. PEI is not obliged to be able to address all of the
>>> memory.
>>>
>>> In OVMF for example, even if PEI is 64-bit, the reset vector builds page
>>> tables only for the first 4GB (for the SEC and PEI phases), and only the
>>> DXE core identify-maps the full memory.
>>>
>>
>> OK, so that would imply that we simply need to check for
>> defined(MDE_CPU_X64) here? Or does the same apply to IPF?
>>
>> Since this is a bit of a can of worms, I am perfectly happy to only
>> drop the limit for MDE_CPU_AARCH64 instead, and make everybody's lives
>> easier ...
>
> Okay, so the new PCD idea has been raised several times. IIRC Jiewen
> mentioned that proliferation of PCDs is now frowned upon. On the other
> hand, in this thread, Jiewen mentioned the possibility of a new PCD:
>
> http://thread.gmane.org/gmane.comp.bios.edk2.devel/7817/focus=7871
>
> I think customizing this code for MDE_CPU_AARCH64, on the source code
> level, is not good.
>
> Here we are under IntelFrameworkModulePkg/Universal/BdsDxe/, which has
> two consequences:
>
> - it says Universal, so it shouldn't be specific to architecture on the
> source code level,
> - it says IntelFrameworkModulePkg, so you have a PCD namespace that
> doesn't clutter MdePkg's / MdeModulePkg's.
>
> Also, you mentioned earlier that memory layout is not specific to ISA,
> but platform, hence MDE_CPU_AARCH64 would not be a perfect match anyway.
>
> My vote is to introduce a new PCD that controls this allocation limit.
> After all, it can depend on a bunch of things:
> - Whether you have S3 or not
> - whether PEI can address all of the memory or just 4GB
> - whether your platform has RAM under 4GB
> - whether allocating under 4GB is detrimental to performance
>
> and so on. I think this is the textbook case for a new PCD.
>

OK, fair enough. That still means a PCD with different default values
for X64 and other archs, but that is apparently not something to care
about.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] Ax88772b: Fixing compilation error variable set but not used

[Shivamurthy Shastri]

On 19 February 2016 at 17:45, Laszlo Ersek  wrote:

> On 02/19/16 13:04, Shivamurthy Shastri wrote:
> > On 19 February 2016 at 16:30, Laszlo Ersek  wrote:
> >
> >> On 02/10/16 15:07, Shivamurthy Shastri wrote:
> >>> error: pNicDevice variable set but not used
> >>>
> >>> Contributed-under: TianoCore Contribution Agreement 1.0
> >>> Signed-off-by: Shivamurthy Shastri 
> >>> ---
> >>>  OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/SimpleNetwork.c | 2 --
> >>>  1 file changed, 2 deletions(-)
> >>>
> >>> diff --git
> a/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/SimpleNetwork.c
> >> b/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/SimpleNetwork.c
> >>> index 9eeb61f..c061a6b 100644
> >>> --- a/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/SimpleNetwork.c
> >>> +++ b/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/SimpleNetwork.c
> >>> @@ -700,10 +700,8 @@ SN_ReceiveFilters (
> >>>EFI_SIMPLE_NETWORK_MODE * pMode;
> >>>EFI_STATUS Status = EFI_SUCCESS;
> >>>EFI_TPL TplPrevious;
> >>> -  NIC_DEVICE * pNicDevice;
> >>>
> >>>TplPrevious = gBS->RaiseTPL(TPL_CALLBACK);
> >>> -  pNicDevice = DEV_FROM_SIMPLE_NETWORK ( pSimpleNetwork );
> >>>pMode = pSimpleNetwork->Mode;
> >>>
> >>>if (pSimpleNetwork == NULL) {
> >>>
> >>
> >> As I said in the other (similar) thread, the subject should identify the
> >> top level package. It should go:
> >>
> >> OptionRomPkg: Ax88772b: ...
> >>
> >> Thanks
> >> Laszlo
> >>
> >
> > ​I will send new patch for that.
>
> If there are no other changes required for your patch(es), then the
> maintainer who commits the patch(es) can fix up the subject(s) at commit
> time. No need to resend just for the subject change.
>
> Thanks
> Laszlo
>
>
​OK, Thanks for review.
I will add ​Ruiyu Ni
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB on ACPI 1.0

[Laszlo Ersek]

On 02/19/16 13:18, Ard Biesheuvel wrote:
> On 19 February 2016 at 13:14, Laszlo Ersek  wrote:
>> On 02/19/16 12:52, Ard Biesheuvel wrote:
>>> On 19 February 2016 at 10:56, Laszlo Ersek  wrote:
 On 02/19/16 09:53, Ard Biesheuvel wrote:
> On 19 February 2016 at 09:45, Yao, Jiewen  wrote:
>> I can explain the reason on allocating <4G. It is because this data will 
>> be used in PEI phase in S3 resume.
>>
>> On most X86 platform, PEI phase is 32bit, and DXE phase is 32bit or 
>> 64bit. So we have to limit the allocation <4G.
>>
>
> OK, got it.
>
>> Using ACPI version PCD looks strange here. Maybe another PCD, like 
>> PcdDxeIplSwitchToLongMode?
>>
>
> But that does not tell us if we are running a 32-bit PEI, right? It
> only tells us if a 32-bit PEI should load a 32-bit DXE core on a
> 64-bit capable machine.

 32->32 and 64->64 builds have PcdDxeIplSwitchToLongMode set to FALSE.

 Only 32->64 builds have PcdDxeIplSwitchToLongMode set to TRUE.

 (This is what we have in the three DSC files of OVMF.)

 So, in order to see in DXE if your PEI is 32-bit, you can do:

   BOOLEAN PeiIs32Bit;

   PeiIs32Bit = FALSE;
   if (FeaturePcdGet (PcdDxeIplSwitchToLongMode)) {
 //
 // this implies a 32->64 switch
 //
 PeiIs32Bit = TRUE;
   } else {
 //
 // otherwise, PEI and DXE have the same bitness,
 // so derive it from DXE's bitness
 //
 #if defined (MDE_CPU_IA32) || defined (MDE_CPU_ARM)
 PeiIs32Bit = TRUE;
 #endif
   }

 Alternatively:

   PeiIs32Bit = FeaturePcdGet (PcdDxeIplSwitchToLongMode) ||
sizeof (UINTN) == sizeof (UINT32);

 I'll admit I'm not really sure about EBC. The above mostly treats EBC as
 nonexistent. :)

>>>
>>> Thanks for clarifying.
>>>
>>> So the only case where we have to take care to allocate below 4 GB is
>>> the 32->64 case, since in all other cases, the memory allocated in DXE
>>> will always be addressable in PEI.
>>
>> I don't think so. PEI is not obliged to be able to address all of the
>> memory.
>>
>> In OVMF for example, even if PEI is 64-bit, the reset vector builds page
>> tables only for the first 4GB (for the SEC and PEI phases), and only the
>> DXE core identify-maps the full memory.
>>
> 
> OK, so that would imply that we simply need to check for
> defined(MDE_CPU_X64) here? Or does the same apply to IPF?
> 
> Since this is a bit of a can of worms, I am perfectly happy to only
> drop the limit for MDE_CPU_AARCH64 instead, and make everybody's lives
> easier ...

Okay, so the new PCD idea has been raised several times. IIRC Jiewen
mentioned that proliferation of PCDs is now frowned upon. On the other
hand, in this thread, Jiewen mentioned the possibility of a new PCD:

http://thread.gmane.org/gmane.comp.bios.edk2.devel/7817/focus=7871

I think customizing this code for MDE_CPU_AARCH64, on the source code
level, is not good.

Here we are under IntelFrameworkModulePkg/Universal/BdsDxe/, which has
two consequences:

- it says Universal, so it shouldn't be specific to architecture on the
source code level,
- it says IntelFrameworkModulePkg, so you have a PCD namespace that
doesn't clutter MdePkg's / MdeModulePkg's.

Also, you mentioned earlier that memory layout is not specific to ISA,
but platform, hence MDE_CPU_AARCH64 would not be a perfect match anyway.

My vote is to introduce a new PCD that controls this allocation limit.
After all, it can depend on a bunch of things:
- Whether you have S3 or not
- whether PEI can address all of the memory or just 4GB
- whether your platform has RAM under 4GB
- whether allocating under 4GB is detrimental to performance

and so on. I think this is the textbook case for a new PCD.

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

Re: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB on ACPI 1.0

[Ard Biesheuvel]

On 19 February 2016 at 13:14, Laszlo Ersek  wrote:
> On 02/19/16 12:52, Ard Biesheuvel wrote:
>> On 19 February 2016 at 10:56, Laszlo Ersek  wrote:
>>> On 02/19/16 09:53, Ard Biesheuvel wrote:
 On 19 February 2016 at 09:45, Yao, Jiewen  wrote:
> I can explain the reason on allocating <4G. It is because this data will 
> be used in PEI phase in S3 resume.
>
> On most X86 platform, PEI phase is 32bit, and DXE phase is 32bit or 
> 64bit. So we have to limit the allocation <4G.
>

 OK, got it.

> Using ACPI version PCD looks strange here. Maybe another PCD, like 
> PcdDxeIplSwitchToLongMode?
>

 But that does not tell us if we are running a 32-bit PEI, right? It
 only tells us if a 32-bit PEI should load a 32-bit DXE core on a
 64-bit capable machine.
>>>
>>> 32->32 and 64->64 builds have PcdDxeIplSwitchToLongMode set to FALSE.
>>>
>>> Only 32->64 builds have PcdDxeIplSwitchToLongMode set to TRUE.
>>>
>>> (This is what we have in the three DSC files of OVMF.)
>>>
>>> So, in order to see in DXE if your PEI is 32-bit, you can do:
>>>
>>>   BOOLEAN PeiIs32Bit;
>>>
>>>   PeiIs32Bit = FALSE;
>>>   if (FeaturePcdGet (PcdDxeIplSwitchToLongMode)) {
>>> //
>>> // this implies a 32->64 switch
>>> //
>>> PeiIs32Bit = TRUE;
>>>   } else {
>>> //
>>> // otherwise, PEI and DXE have the same bitness,
>>> // so derive it from DXE's bitness
>>> //
>>> #if defined (MDE_CPU_IA32) || defined (MDE_CPU_ARM)
>>> PeiIs32Bit = TRUE;
>>> #endif
>>>   }
>>>
>>> Alternatively:
>>>
>>>   PeiIs32Bit = FeaturePcdGet (PcdDxeIplSwitchToLongMode) ||
>>>sizeof (UINTN) == sizeof (UINT32);
>>>
>>> I'll admit I'm not really sure about EBC. The above mostly treats EBC as
>>> nonexistent. :)
>>>
>>
>> Thanks for clarifying.
>>
>> So the only case where we have to take care to allocate below 4 GB is
>> the 32->64 case, since in all other cases, the memory allocated in DXE
>> will always be addressable in PEI.
>
> I don't think so. PEI is not obliged to be able to address all of the
> memory.
>
> In OVMF for example, even if PEI is 64-bit, the reset vector builds page
> tables only for the first 4GB (for the SEC and PEI phases), and only the
> DXE core identify-maps the full memory.
>

OK, so that would imply that we simply need to check for
defined(MDE_CPU_X64) here? Or does the same apply to IPF?

Since this is a bit of a can of worms, I am perfectly happy to only
drop the limit for MDE_CPU_AARCH64 instead, and make everybody's lives
easier ...
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] Ax88772b: Fixing compilation error variable set but not used

[Laszlo Ersek]

On 02/19/16 13:04, Shivamurthy Shastri wrote:
> On 19 February 2016 at 16:30, Laszlo Ersek  wrote:
> 
>> On 02/10/16 15:07, Shivamurthy Shastri wrote:
>>> error: pNicDevice variable set but not used
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Shivamurthy Shastri 
>>> ---
>>>  OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/SimpleNetwork.c | 2 --
>>>  1 file changed, 2 deletions(-)
>>>
>>> diff --git a/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/SimpleNetwork.c
>> b/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/SimpleNetwork.c
>>> index 9eeb61f..c061a6b 100644
>>> --- a/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/SimpleNetwork.c
>>> +++ b/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/SimpleNetwork.c
>>> @@ -700,10 +700,8 @@ SN_ReceiveFilters (
>>>EFI_SIMPLE_NETWORK_MODE * pMode;
>>>EFI_STATUS Status = EFI_SUCCESS;
>>>EFI_TPL TplPrevious;
>>> -  NIC_DEVICE * pNicDevice;
>>>
>>>TplPrevious = gBS->RaiseTPL(TPL_CALLBACK);
>>> -  pNicDevice = DEV_FROM_SIMPLE_NETWORK ( pSimpleNetwork );
>>>pMode = pSimpleNetwork->Mode;
>>>
>>>if (pSimpleNetwork == NULL) {
>>>
>>
>> As I said in the other (similar) thread, the subject should identify the
>> top level package. It should go:
>>
>> OptionRomPkg: Ax88772b: ...
>>
>> Thanks
>> Laszlo
>>
> 
> ​I will send new patch for that.

If there are no other changes required for your patch(es), then the
maintainer who commits the patch(es) can fix up the subject(s) at commit
time. No need to resend just for the subject change.

Thanks
Laszlo

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

Re: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB on ACPI 1.0

[Laszlo Ersek]

On 02/19/16 12:52, Ard Biesheuvel wrote:
> On 19 February 2016 at 10:56, Laszlo Ersek  wrote:
>> On 02/19/16 09:53, Ard Biesheuvel wrote:
>>> On 19 February 2016 at 09:45, Yao, Jiewen  wrote:
 I can explain the reason on allocating <4G. It is because this data will 
 be used in PEI phase in S3 resume.

 On most X86 platform, PEI phase is 32bit, and DXE phase is 32bit or 64bit. 
 So we have to limit the allocation <4G.

>>>
>>> OK, got it.
>>>
 Using ACPI version PCD looks strange here. Maybe another PCD, like 
 PcdDxeIplSwitchToLongMode?

>>>
>>> But that does not tell us if we are running a 32-bit PEI, right? It
>>> only tells us if a 32-bit PEI should load a 32-bit DXE core on a
>>> 64-bit capable machine.
>>
>> 32->32 and 64->64 builds have PcdDxeIplSwitchToLongMode set to FALSE.
>>
>> Only 32->64 builds have PcdDxeIplSwitchToLongMode set to TRUE.
>>
>> (This is what we have in the three DSC files of OVMF.)
>>
>> So, in order to see in DXE if your PEI is 32-bit, you can do:
>>
>>   BOOLEAN PeiIs32Bit;
>>
>>   PeiIs32Bit = FALSE;
>>   if (FeaturePcdGet (PcdDxeIplSwitchToLongMode)) {
>> //
>> // this implies a 32->64 switch
>> //
>> PeiIs32Bit = TRUE;
>>   } else {
>> //
>> // otherwise, PEI and DXE have the same bitness,
>> // so derive it from DXE's bitness
>> //
>> #if defined (MDE_CPU_IA32) || defined (MDE_CPU_ARM)
>> PeiIs32Bit = TRUE;
>> #endif
>>   }
>>
>> Alternatively:
>>
>>   PeiIs32Bit = FeaturePcdGet (PcdDxeIplSwitchToLongMode) ||
>>sizeof (UINTN) == sizeof (UINT32);
>>
>> I'll admit I'm not really sure about EBC. The above mostly treats EBC as
>> nonexistent. :)
>>
> 
> Thanks for clarifying.
> 
> So the only case where we have to take care to allocate below 4 GB is
> the 32->64 case, since in all other cases, the memory allocated in DXE
> will always be addressable in PEI.

I don't think so. PEI is not obliged to be able to address all of the
memory.

In OVMF for example, even if PEI is 64-bit, the reset vector builds page
tables only for the first 4GB (for the SEC and PEI phases), and only the
DXE core identify-maps the full memory.

Thanks
Laszlo

> 
> So I will take Jiewen's suggestion, and only limit the memory
> allocation if PcdDxeIplSwitchToLongMode is TRUE. But I will still need
> the MDE_CPU_X64 ifdef, since PcdDxeIplSwitchToLongMode is only defined
> for IA32 and X64, and since it defaults to TRUE, defining it for other
> archs would mean we have to change the default value as well.
> 
> Alternatively, I could propose this:
> 
> [PcdsFeatureFlag.IA32, PcdsFeatureFlag.X64]
>   
> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode|TRUE|BOOLEAN|0x0001003b
> 
> [PcdsFeatureFlag.ARM, PcdsFeatureFlag.AARCH64, PcdsFeatureFlag.IPF]
>  
> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode|FALSE|BOOLEAN|0x0001003b
> 
> and only test the PCD, but I don't think that is an improvement.
> 

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

Re: [edk2] [PATCH] Ax88772b: Fixing compilation error variable set but not used

[Shivamurthy Shastri]

On 19 February 2016 at 16:30, Laszlo Ersek  wrote:

> On 02/10/16 15:07, Shivamurthy Shastri wrote:
> > error: pNicDevice variable set but not used
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Shivamurthy Shastri 
> > ---
> >  OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/SimpleNetwork.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/SimpleNetwork.c
> b/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/SimpleNetwork.c
> > index 9eeb61f..c061a6b 100644
> > --- a/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/SimpleNetwork.c
> > +++ b/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/SimpleNetwork.c
> > @@ -700,10 +700,8 @@ SN_ReceiveFilters (
> >EFI_SIMPLE_NETWORK_MODE * pMode;
> >EFI_STATUS Status = EFI_SUCCESS;
> >EFI_TPL TplPrevious;
> > -  NIC_DEVICE * pNicDevice;
> >
> >TplPrevious = gBS->RaiseTPL(TPL_CALLBACK);
> > -  pNicDevice = DEV_FROM_SIMPLE_NETWORK ( pSimpleNetwork );
> >pMode = pSimpleNetwork->Mode;
> >
> >if (pSimpleNetwork == NULL) {
> >
>
> As I said in the other (similar) thread, the subject should identify the
> top level package. It should go:
>
> OptionRomPkg: Ax88772b: ...
>
> Thanks
> Laszlo
>

​I will send new patch for that.

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

Re: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB on ACPI 1.0

[Yao, Jiewen]

Thanks. I think it is good idea to change default value to FALSE to non-x86 
platform.

Thank you
Yao Jiewen

From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
Sent: Friday, February 19, 2016 7:52 PM
To: Laszlo Ersek
Cc: Yao, Jiewen; edk2-devel@lists.01.org; Tian, Feng; Zeng, Star; 
leif.lindh...@linaro.org; graeme.greg...@linaro.org; Fan, Jeff; 
mark.rutl...@arm.com; Gao, Liming; samer.el-haj-mahm...@hpe.com
Subject: Re: [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate 
below 4 GB on ACPI 1.0

On 19 February 2016 at 10:56, Laszlo Ersek wrote:
> On 02/19/16 09:53, Ard Biesheuvel wrote:
>> On 19 February 2016 at 09:45, Yao, Jiewen wrote:
>>> I can explain the reason on allocating
>>>
>>> On most X86 platform, PEI phase is 32bit, and DXE phase is 32bit or 64bit. 
>>> So we have to limit the allocation <4G.
>>>
>>
>> OK, got it.
>>
>>> Using ACPI version PCD looks strange here. Maybe another PCD, like 
>>> PcdDxeIplSwitchToLongMode?
>>>
>>
>> But that does not tell us if we are running a 32-bit PEI, right? It
>> only tells us if a 32-bit PEI should load a 32-bit DXE core on a
>> 64-bit capable machine.
>
> 32->32 and 64->64 builds have PcdDxeIplSwitchToLongMode set to FALSE.
>
> Only 32->64 builds have PcdDxeIplSwitchToLongMode set to TRUE.
>
> (This is what we have in the three DSC files of OVMF.)
>
> So, in order to see in DXE if your PEI is 32-bit, you can do:
>
> BOOLEAN PeiIs32Bit;
>
> PeiIs32Bit = FALSE;
> if (FeaturePcdGet (PcdDxeIplSwitchToLongMode)) {
> //
> // this implies a 32->64 switch
> //
> PeiIs32Bit = TRUE;
> } else {
> //
> // otherwise, PEI and DXE have the same bitness,
> // so derive it from DXE's bitness
> //
> #if defined (MDE_CPU_IA32) || defined (MDE_CPU_ARM)
> PeiIs32Bit = TRUE;
> #endif
> }
>
> Alternatively:
>
> PeiIs32Bit = FeaturePcdGet (PcdDxeIplSwitchToLongMode) ||
> sizeof (UINTN) == sizeof (UINT32);
>
> I'll admit I'm not really sure about EBC. The above mostly treats EBC
> as nonexistent. :)
>

Thanks for clarifying.

So the only case where we have to take care to allocate below 4 GB is the 
32->64 case, since in all other cases, the memory allocated in DXE will always 
be addressable in PEI.

So I will take Jiewen's suggestion, and only limit the memory allocation if 
PcdDxeIplSwitchToLongMode is TRUE. But I will still need the MDE_CPU_X64 ifdef, 
since PcdDxeIplSwitchToLongMode is only defined for IA32 and X64, and since it 
defaults to TRUE, defining it for other archs would mean we have to change the 
default value as well.

Alternatively, I could propose this:

[PcdsFeatureFlag.IA32, PcdsFeatureFlag.X64]
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode|TRUE|BOOLEAN|0x0001003b

[PcdsFeatureFlag.ARM, PcdsFeatureFlag.AARCH64, PcdsFeatureFlag.IPF] 
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode|FALSE|BOOLEAN|0x0001003b

and only test the PCD, but I don't think that is an improvement.

--
Ard.





>>
>>> -Original Message-
>>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>>> Sent: Friday, February 19, 2016 4:03 AM
>>> To: edk2-devel@lists.01.org; Tian, Feng; 
>>> Zeng, Star; leif.lindh...@linaro.org; 
>>> graeme.greg...@linaro.org; 
>>> ler...@redhat.com; Fan, Jeff; Yao, Jiewen
>>> Cc: mark.rutl...@arm.com; Gao, Liming; 
>>> samer.el-haj-mahm...@hpe.com; Ard 
>>> Biesheuvel
>>> Subject: [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate 
>>> below 4 GB on ACPI 1.0
>>>
>>> It is not entirely clear from the code why, but the reserved region for 
>>> storing performance data is allocated below 4 GB, and the variable to hold 
>>> the address of the allocation is called 'AcpiLowMemoryBase'
>>> (which is the only mention of ACPI in the entire file).
>>>
>>> Let's make this allocation dependent on PcdAcpiExposedTableVersions as 
>>> well, since systems that can deal with ACPI table in high memory are also 
>>> just as likely to be able to deal with performance data in high memory. 
>>> This prevents memory fragmentation on systems that don't care about the 4 
>>> GB limit.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel
>>> ---
>>> IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf | 1 + 
>>> IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c | 11 ++-
>>> 2 files changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf 
>>> b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
>>> index 6afb8a09df9c..38172780fb49 100644
>>> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
>>> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
>>> @@ -215,6 +215,7 @@ [Pcd]
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoHorizontalResolution ## CONSUMES
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoVerticalResolut

Re: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB on ACPI 1.0

[Ard Biesheuvel]

On 19 February 2016 at 10:56, Laszlo Ersek  wrote:
> On 02/19/16 09:53, Ard Biesheuvel wrote:
>> On 19 February 2016 at 09:45, Yao, Jiewen  wrote:
>>> I can explain the reason on allocating <4G. It is because this data will be 
>>> used in PEI phase in S3 resume.
>>>
>>> On most X86 platform, PEI phase is 32bit, and DXE phase is 32bit or 64bit. 
>>> So we have to limit the allocation <4G.
>>>
>>
>> OK, got it.
>>
>>> Using ACPI version PCD looks strange here. Maybe another PCD, like 
>>> PcdDxeIplSwitchToLongMode?
>>>
>>
>> But that does not tell us if we are running a 32-bit PEI, right? It
>> only tells us if a 32-bit PEI should load a 32-bit DXE core on a
>> 64-bit capable machine.
>
> 32->32 and 64->64 builds have PcdDxeIplSwitchToLongMode set to FALSE.
>
> Only 32->64 builds have PcdDxeIplSwitchToLongMode set to TRUE.
>
> (This is what we have in the three DSC files of OVMF.)
>
> So, in order to see in DXE if your PEI is 32-bit, you can do:
>
>   BOOLEAN PeiIs32Bit;
>
>   PeiIs32Bit = FALSE;
>   if (FeaturePcdGet (PcdDxeIplSwitchToLongMode)) {
> //
> // this implies a 32->64 switch
> //
> PeiIs32Bit = TRUE;
>   } else {
> //
> // otherwise, PEI and DXE have the same bitness,
> // so derive it from DXE's bitness
> //
> #if defined (MDE_CPU_IA32) || defined (MDE_CPU_ARM)
> PeiIs32Bit = TRUE;
> #endif
>   }
>
> Alternatively:
>
>   PeiIs32Bit = FeaturePcdGet (PcdDxeIplSwitchToLongMode) ||
>sizeof (UINTN) == sizeof (UINT32);
>
> I'll admit I'm not really sure about EBC. The above mostly treats EBC as
> nonexistent. :)
>

Thanks for clarifying.

So the only case where we have to take care to allocate below 4 GB is
the 32->64 case, since in all other cases, the memory allocated in DXE
will always be addressable in PEI.

So I will take Jiewen's suggestion, and only limit the memory
allocation if PcdDxeIplSwitchToLongMode is TRUE. But I will still need
the MDE_CPU_X64 ifdef, since PcdDxeIplSwitchToLongMode is only defined
for IA32 and X64, and since it defaults to TRUE, defining it for other
archs would mean we have to change the default value as well.

Alternatively, I could propose this:

[PcdsFeatureFlag.IA32, PcdsFeatureFlag.X64]
  
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode|TRUE|BOOLEAN|0x0001003b

[PcdsFeatureFlag.ARM, PcdsFeatureFlag.AARCH64, PcdsFeatureFlag.IPF]
 
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode|FALSE|BOOLEAN|0x0001003b

and only test the PCD, but I don't think that is an improvement.

-- 
Ard.





>>
>>> -Original Message-
>>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>>> Sent: Friday, February 19, 2016 4:03 AM
>>> To: edk2-devel@lists.01.org; Tian, Feng; Zeng, Star; 
>>> leif.lindh...@linaro.org; graeme.greg...@linaro.org; ler...@redhat.com; 
>>> Fan, Jeff; Yao, Jiewen
>>> Cc: mark.rutl...@arm.com; Gao, Liming; samer.el-haj-mahm...@hpe.com; Ard 
>>> Biesheuvel
>>> Subject: [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate 
>>> below 4 GB on ACPI 1.0
>>>
>>> It is not entirely clear from the code why, but the reserved region for 
>>> storing performance data is allocated below 4 GB, and the variable to hold 
>>> the address of the allocation is called 'AcpiLowMemoryBase'
>>> (which is the only mention of ACPI in the entire file).
>>>
>>> Let's make this allocation dependent on PcdAcpiExposedTableVersions as 
>>> well, since systems that can deal with ACPI table in high memory are also 
>>> just as likely to be able to deal with performance data in high memory. 
>>> This prevents memory fragmentation on systems that don't care about the 4 
>>> GB limit.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel 
>>> ---
>>>  IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf |  1 +  
>>> IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c | 11 ++-
>>>  2 files changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf 
>>> b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
>>> index 6afb8a09df9c..38172780fb49 100644
>>> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
>>> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
>>> @@ -215,6 +215,7 @@ [Pcd]
>>>gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoHorizontalResolution 
>>>## CONSUMES
>>>gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoVerticalResolution   
>>>## CONSUMES
>>>gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable   
>>>## CONSUMES
>>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions   
>>>## CONSUMES
>>>
>>>  [Depex]
>>>TRUE
>>> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c 
>>> b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
>>> index ae7ad2153c51..bf5bd6524a90 100644
>>> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
>>> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntr

Re: [edk2] [PATCH] OvmfPkg: implement UEFI driver for Virtio RNG devices

[Ard Biesheuvel]

On 19 February 2016 at 11:54, Ard Biesheuvel  wrote:
> This implements a UEFI driver model driver for Virtio devices of type
> VIRTIO_SUBSYSTEM_ENTROPY_SOURCE, and exposes them via instances of
> the EFI_RNG_PROTOCOL protocol, supporting the EFI_RNG_ALGORITHM_RAW
> algorithm only.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> ---
>
> 90% of this driver is boilerplate taken from the virtio-scsi driver.
>
> The purpose of this driver is to expose the EFI_RNG_PROTOCOL to the arm64
> Linux kernel UEFI stub, which will use it to randomize the kernel address
> space [1]
>
> This driver can be tested from the UEFI shell using the RngTest UEFI
> application (SecurityPkg/Application/RngTest/RngTest.inf), example output
> can be found after the patch
>
> [1] http://thread.gmane.org/gmane.linux.kernel.efi/7446
>
>  OvmfPkg/VirtioRngDxe/VirtioRng.c  | 593 
>  OvmfPkg/VirtioRngDxe/VirtioRngDxe.inf |  45 ++
>  2 files changed, 638 insertions(+)
>
> diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c 
> b/OvmfPkg/VirtioRngDxe/VirtioRng.c
> new file mode 100644
> index ..ab52f269fc3f
> --- /dev/null
> +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c
> @@ -0,0 +1,593 @@
> +/** @file
> +
> +  This driver produces EFI_RNG_PROTOCOL instances for virtio-rng devices.
> +
> +  The implementation is based on OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +
> +  Copyright (C) 2012, Red Hat, Inc.
> +  Copyright (c) 2012 - 2014, Intel Corporation. All rights reserved.
> +
> +  This driver:
> +
> +  Copyright (C) 2016, Linaro Ltd.
> +
> +  This program and the accompanying materials are licensed and made available
> +  under the terms and conditions of the BSD License which accompanies this
> +  distribution. The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, 
> WITHOUT
> +  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#define VIRTIO_RNG_SIG  SIGNATURE_32 ('V', 'R', 'N', 'G')
> +
> +typedef struct {
> +  UINT32  Signature;
> +  VIRTIO_DEVICE_PROTOCOL  *VirtIo;
> +  EFI_EVENT   ExitBoot;
> +  VRING   Ring;
> +  EFI_RNG_PROTOCOLRng;
> +} VIRTIO_RNG_DEV;
> +
> +#define VIRTIO_ENTROPY_SOURCE_FROM_RNG(RngPointer) \
> +CR (RngPointer, VIRTIO_RNG_DEV, Rng, VIRTIO_RNG_SIG)
> +
> +
> +/**
> +  Returns information about the random number generation implementation.
> +
> +  @param[in] This A pointer to the EFI_RNG_PROTOCOL 
> instance.
> +  @param[in,out] RNGAlgorithmListSize On input, the size in bytes of 
> RNGAlgorithmList.
> +  On output with a return code of 
> EFI_SUCCESS, the size
> +  in bytes of the data returned in 
> RNGAlgorithmList. On output
> +  with a return code of 
> EFI_BUFFER_TOO_SMALL,
> +  the size of RNGAlgorithmList required 
> to obtain the list.
> +  @param[out] RNGAlgorithmListA caller-allocated memory buffer 
> filled by the driver
> +  with one EFI_RNG_ALGORITHM element for 
> each supported
> +  RNG algorithm. The list must not 
> change across multiple
> +  calls to the same driver. The first 
> algorithm in the list
> +  is the default algorithm for the 
> driver.
> +
> +  @retval EFI_SUCCESS The RNG algorithm list was returned 
> successfully.
> +  @retval EFI_UNSUPPORTED The services is not supported by this 
> driver.
> +  @retval EFI_DEVICE_ERRORThe list of algorithms could not be 
> retrieved due to a
> +  hardware or firmware error.
> +  @retval EFI_INVALID_PARAMETER   One or more of the parameters are 
> incorrect.
> +  @retval EFI_BUFFER_TOO_SMALLThe buffer RNGAlgorithmList is too 
> small to hold the result.
> +
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +VirtioRngGetInfo (
> +  IN  EFI_RNG_PROTOCOL*This,
> +  IN OUT  UINTN   *RNGAlgorithmListSize,
> +  OUT EFI_RNG_ALGORITHM   *RNGAlgorithmList
> +  )
> +{
> +  ASSERT (This != NULL);
> +  ASSERT (RNGAlgorithmListSize != NULL);
> +

Self-review: I will get rid of these asserts and return
EFI_INVALID_PARAMETER instead.

> +  if (*RNGAlgorithmListSize < sizeof (EFI_RNG_ALGORITHM)) {
> +*RNGAlgorithmListSize = sizeof (EFI_RNG_ALGORITHM);
> +return EFI_BUFFER_TOO_SMALL;
> +  }
> +
> +  ASSERT (RNGAlgorithmList != NULL);
> +

.. and this one

> +  *RNGAlgor

Re: [edk2] [Patch 2/2] MdeModuelPkg/PciBus: Return AddrTranslationOffset in GetBarAttributes

[Laszlo Ersek]

On 02/18/16 22:40, Benjamin Herrenschmidt wrote:
> On Thu, 2016-02-18 at 18:02 +0100, Laszlo Ersek wrote:
>>
>> with Ray's change in the generic PCI bus driver, *must* all PciIo-based
>> device drivers be updated? Is this a (bug-)compatible change, or does it
>> regress existing drivers (perhaps by triggering their hidden bugs)?
> 
> I wouldn't expect existing drivers to break no, as I don't expect them
> to look at the AddressTranslation field and it's 0 anyway on existing
> platforms.
> 
> Of course they have to be updated to work on non-1:1 platforms.
> 
>> If this PciIo change has potential to expose bugs or invalid silent
>> assumptions in other drivers, then I think the patch should not be
>> committed in isolation. It should be part of a series that addresses
>> said assumptions in all dependent drivers that are present in the open
>> source edk2 tree.
> 
> I am not aware of such bugs or silent assumptions. I had the
> opportunity to look at a couple of vendor drivers as well and they also
> don't seem to make such problematic assumptions either.
> 
>> I believe, for QemuVideoDxe, running on
>> - qemu-system-(i386|x86_64), i440fx and q35 machine types, or
>> - qemu-system-(arm|aarch64), "virt" machine type,
>>
>> the translation offset between CPU and device MMIO is, in practice,
>> zero, so I guess in practice QemuVideoDxe won't break.
>>
>> But, it would be nice to see confirmation (plus in the long term,
>> QemuVideoDxe should be updated nonetheless)
> 
> I agree but overall I think this is pretty safe.

After reviewing Ray's v2 posting, I agree they are safe -- in particular
because the PCI root bridge drivers I worried about always store
AddrTranslationOffset=0 on output, in the Configuration() function.

(Well, they do so for the time being, anyway.)

Thanks
Laszlo

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

Re: [edk2] [PATCH] Ax88772b: Fixing compilation error variable set but not used

[Laszlo Ersek]

On 02/10/16 15:07, Shivamurthy Shastri wrote:
> error: pNicDevice variable set but not used
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Shivamurthy Shastri 
> ---
>  OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/SimpleNetwork.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/SimpleNetwork.c 
> b/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/SimpleNetwork.c
> index 9eeb61f..c061a6b 100644
> --- a/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/SimpleNetwork.c
> +++ b/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/SimpleNetwork.c
> @@ -700,10 +700,8 @@ SN_ReceiveFilters (
>EFI_SIMPLE_NETWORK_MODE * pMode;
>EFI_STATUS Status = EFI_SUCCESS;   
>EFI_TPL TplPrevious; 
> -  NIC_DEVICE * pNicDevice;
>  
>TplPrevious = gBS->RaiseTPL(TPL_CALLBACK);
> -  pNicDevice = DEV_FROM_SIMPLE_NETWORK ( pSimpleNetwork );
>pMode = pSimpleNetwork->Mode;
>  
>if (pSimpleNetwork == NULL) {
> 

As I said in the other (similar) thread, the subject should identify the
top level package. It should go:

OptionRomPkg: Ax88772b: ...

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

Re: [edk2] BaseTools build number after transition to git

[Laszlo Ersek]

On 02/18/16 22:10, Felix Poludov wrote:
> Eric,
> 
> Using git commit hash or a truncated version of hash, as proposed by Laszlo, 
> is mostly fine.
> The only inconvenience of git hashes is that they cannot be compared.
> Unlike SVN revision, it's impossible to tell which commit is newer, by  just 
> looking at git hash.
> I suggest using git hash plus commit date/time.
> Something similar to the output of the following command:
> git show -s --format="%H %ci" 

This is a great idea; this pattern (maybe shortened a bit) is used
elsewhere too.

Thanks
Laszlo

> 
> 
> -Original Message-
> From: Bjorge, Erik C [mailto:erik.c.bjo...@intel.com] 
> Sent: Friday, February 12, 2016 6:41 PM
> To: Felix Poludov; edk2-devel@lists.01.org
> Subject: RE: BaseTools build number after transition to git
> 
> Sorry for the late reply.  Currently I am planning to use the SHA1 as the 
> version.  This seems like a reasonable approach.
> 
> Thanks,
> -Erik
> 
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>> Felix Poludov
>> Sent: Wednesday, February 10, 2016 11:09 AM
>> To: edk2-devel@lists.01.org
>> Subject: [edk2] BaseTools build number after transition to git
>>
>> Most of the EDKII tools support --version switch that returns tool
>> version and build number.
>> We've been using SVN revision ID as a build number for all the tools.
>> Have there been any discussions on what should be used as a build number
>> in the new (and hopefully merrier)  EDKII Git world?
>>
>> Felix
>>
>>
>> The information contained in this message may be confidential and
>> proprietary to American Megatrends, Inc.  This communication is intended
>> to be read only by the individual or entity to whom it is addressed or
>> by their designee. If the reader of this message is not the intended
>> recipient, you are on notice that any distribution of this message, in
>> any form, is strictly prohibited.  Please promptly notify the sender by
>> reply e-mail or by telephone at 770-246-8600, and then delete or destroy
>> all copies of the transmission.
>> ___
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
> 
> The information contained in this message may be confidential and proprietary 
> to American Megatrends, Inc.  This communication is intended to be read only 
> by the individual or entity to whom it is addressed or by their designee. If 
> the reader of this message is not the intended recipient, you are on notice 
> that any distribution of this message, in any form, is strictly prohibited.  
> Please promptly notify the sender by reply e-mail or by telephone at 
> 770-246-8600, and then delete or destroy all copies of the transmission.
> ___
> 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 V2 2/2] MdeModuelPkg/PciBus: Return AddrTranslationOffset in GetBarAttributes

[Laszlo Ersek]

On 02/19/16 03:18, Ruiyu Ni wrote:
> Some platform doesn't use CPU(HOST)/Device 1:1 mapping for PCI Bus.
> But PCI IO doesn't have interface to tell caller (device driver)
> whether the address returned by GetBarAttributes() is HOST address
> or device address.
> UEFI Spec 2.6 addresses this issue by clarifying the address returned
> is HOST address and caller can use AddrTranslationOffset to calculate
> the device address.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni 
> Cc: Benjamin Herrenschmidt 
> Cc: Andrew Fish 
> Cc: Jeff Fan 
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c | 62 
> ++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c 
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> index 50ed866..55c9ce2 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> @@ -1744,6 +1744,53 @@ PciIoAttributes (
>  }
>  
>  /**
> +  Retrieve the AddrTranslationOffset from RootBridgeIo for the
> +  specified range.
> +
> +  @param RootBridgeIoRoot Bridge IO instance.
> +  @param AddrRangeMinThe base address of the MMIO.
> +  @param AddrLen The length of the MMIO.
> +
> +  @retval The AddrTranslationOffset from RootBridgeIo for the 
> +  specified range, or (UINT64) -1 if the range is not
> +  found in RootBridgeIo.
> +**/
> +UINT64
> +GetMmioAddressTranslationOffset (
> +  EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL   *RootBridgeIo,
> +  UINT64AddrRangeMin,
> +  UINT64AddrLen
> +  )
> +{
> +  EFI_STATUSStatus;
> +  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Configuration;
> +
> +  Status = RootBridgeIo->Configuration (
> +   RootBridgeIo,
> +   (VOID **) &Configuration
> +   );
> +  if (EFI_ERROR (Status)) {
> +return (UINT64) -1;
> +  }
> +
> +  while (Configuration->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) {
> +if ((Configuration->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) &&
> +(Configuration->AddrRangeMin <= AddrRangeMin) &&
> +(Configuration->AddrRangeMin + Configuration->AddrLen >= 
> AddrRangeMin + AddrLen)
> +) {
> +  return Configuration->AddrTranslationOffset;
> +}
> +Configuration++;
> +  }
> +
> +  //
> +  // The resource occupied by BAR should be in the range reported by 
> RootBridge.
> +  //
> +  ASSERT (Configuration->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR);

I don't understand this assertion. If we reach this spot, then the while
loop terminated, which implies

  Configuration->Desc != ACPI_ADDRESS_SPACE_DESCRIPTOR

So it seems like this ASSERT() will always fail.

What am I missing?

According to the comment, I think the intent is probably to enforce that
the loop always finds a match, and hence never completes. If that's the
case, can't you just add:

  ASSERT (FALSE);

? I think that would me much clearer.

> +  return (UINT64) -1;
> +}
> +
> +/**
>Gets the attributes that this PCI controller supports setting on a BAR 
> using
>SetBarAttributes(), and retrieves the list of resource descriptors for a 
> BAR.
>  
> @@ -1863,6 +1910,21 @@ PciIoGetBarAttributes (
>  End   = (EFI_ACPI_END_TAG_DESCRIPTOR *) (Descriptor + 1);
>  End->Desc = ACPI_END_TAG_DESCRIPTOR;
>  End->Checksum = 0;
> +
> +//
> +// Get the Address Translation Offset
> +//
> +if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
> +  Descriptor->AddrTranslationOffset = GetMmioAddressTranslationOffset (
> +PciIoDevice->PciRootBridgeIo,
> +Descriptor->AddrRangeMin,
> +Descriptor->AddrLen
> +);
> +  if (Descriptor->AddrTranslationOffset == (UINT64) -1) {
> +FreePool (Descriptor);
> +return EFI_UNSUPPORTED;
> +  }
> +}
>}
>  
>return EFI_SUCCESS;
> 

Otherwise the patch seems good to me.

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

[edk2] [PATCH] OvmfPkg: implement UEFI driver for Virtio RNG devices

[Ard Biesheuvel]

This implements a UEFI driver model driver for Virtio devices of type
VIRTIO_SUBSYSTEM_ENTROPY_SOURCE, and exposes them via instances of
the EFI_RNG_PROTOCOL protocol, supporting the EFI_RNG_ALGORITHM_RAW
algorithm only.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---

90% of this driver is boilerplate taken from the virtio-scsi driver.

The purpose of this driver is to expose the EFI_RNG_PROTOCOL to the arm64
Linux kernel UEFI stub, which will use it to randomize the kernel address
space [1]

This driver can be tested from the UEFI shell using the RngTest UEFI
application (SecurityPkg/Application/RngTest/RngTest.inf), example output
can be found after the patch

[1] http://thread.gmane.org/gmane.linux.kernel.efi/7446

 OvmfPkg/VirtioRngDxe/VirtioRng.c  | 593 
 OvmfPkg/VirtioRngDxe/VirtioRngDxe.inf |  45 ++
 2 files changed, 638 insertions(+)

diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c b/OvmfPkg/VirtioRngDxe/VirtioRng.c
new file mode 100644
index ..ab52f269fc3f
--- /dev/null
+++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c
@@ -0,0 +1,593 @@
+/** @file
+
+  This driver produces EFI_RNG_PROTOCOL instances for virtio-rng devices.
+
+  The implementation is based on OvmfPkg/VirtioScsiDxe/VirtioScsi.c
+
+  Copyright (C) 2012, Red Hat, Inc.
+  Copyright (c) 2012 - 2014, Intel Corporation. All rights reserved.
+
+  This driver:
+
+  Copyright (C) 2016, Linaro Ltd.
+
+  This program and the accompanying materials are licensed and made available
+  under the terms and conditions of the BSD License which accompanies this
+  distribution. The full text of the license may be found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+#include 
+
+#define VIRTIO_RNG_SIG  SIGNATURE_32 ('V', 'R', 'N', 'G')
+
+typedef struct {
+  UINT32  Signature;
+  VIRTIO_DEVICE_PROTOCOL  *VirtIo;
+  EFI_EVENT   ExitBoot;
+  VRING   Ring;
+  EFI_RNG_PROTOCOLRng;
+} VIRTIO_RNG_DEV;
+
+#define VIRTIO_ENTROPY_SOURCE_FROM_RNG(RngPointer) \
+CR (RngPointer, VIRTIO_RNG_DEV, Rng, VIRTIO_RNG_SIG)
+
+
+/**
+  Returns information about the random number generation implementation.
+
+  @param[in] This A pointer to the EFI_RNG_PROTOCOL 
instance.
+  @param[in,out] RNGAlgorithmListSize On input, the size in bytes of 
RNGAlgorithmList. 
+  On output with a return code of 
EFI_SUCCESS, the size
+  in bytes of the data returned in 
RNGAlgorithmList. On output
+  with a return code of 
EFI_BUFFER_TOO_SMALL,
+  the size of RNGAlgorithmList required to 
obtain the list.
+  @param[out] RNGAlgorithmListA caller-allocated memory buffer filled 
by the driver
+  with one EFI_RNG_ALGORITHM element for 
each supported
+  RNG algorithm. The list must not change 
across multiple
+  calls to the same driver. The first 
algorithm in the list
+  is the default algorithm for the driver.
+
+  @retval EFI_SUCCESS The RNG algorithm list was returned 
successfully.
+  @retval EFI_UNSUPPORTED The services is not supported by this 
driver.
+  @retval EFI_DEVICE_ERRORThe list of algorithms could not be 
retrieved due to a
+  hardware or firmware error.
+  @retval EFI_INVALID_PARAMETER   One or more of the parameters are 
incorrect.
+  @retval EFI_BUFFER_TOO_SMALLThe buffer RNGAlgorithmList is too small 
to hold the result.
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+VirtioRngGetInfo (
+  IN  EFI_RNG_PROTOCOL*This,
+  IN OUT  UINTN   *RNGAlgorithmListSize,
+  OUT EFI_RNG_ALGORITHM   *RNGAlgorithmList
+  )
+{
+  ASSERT (This != NULL);
+  ASSERT (RNGAlgorithmListSize != NULL);
+
+  if (*RNGAlgorithmListSize < sizeof (EFI_RNG_ALGORITHM)) {
+*RNGAlgorithmListSize = sizeof (EFI_RNG_ALGORITHM);
+return EFI_BUFFER_TOO_SMALL;
+  }
+
+  ASSERT (RNGAlgorithmList != NULL);
+
+  *RNGAlgorithmListSize = sizeof (EFI_RNG_ALGORITHM);
+  CopyGuid (RNGAlgorithmList, &gEfiRngAlgorithmRaw);
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Produces and returns an RNG value using either the default or specified RNG 
algorithm.
+
+  @param[in]  ThisA pointer to the EFI_RNG_PROTOCOL 
instance.
+  @param[in]  RNGAlgorithmA pointer to the EFI_RNG_ALGORITHM that 
identifies the RNG
+  

Re: [edk2] [Patch V2 1/2] MdeModulePkg/PciBus: Refine to reduce code lines.

[Laszlo Ersek]

On 02/19/16 03:18, Ruiyu Ni wrote:
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni 
> Cc: Feng Tian 
> Cc: Jeff Fan 
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c | 70 
> --
>  1 file changed, 24 insertions(+), 46 deletions(-)

I think this patch is correct. However, it is very hard to read.

I'd like to propose the following two improvements:

(1) Please split this patch in two. In the first step, please do not
reorganize the control flow, *only* rename the variable.

In the second step (a separate patch), the control flow can be
reorganized.

Both of the patches should point out that they incur no observable
change.

(2) In the reorganized version, please add

  //
  // fall through
  //

comments. Although they are not required by the coding style, I've seen
them elsewhere in edk2, and they are quite helpful.

Thanks
Laszlo


> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c 
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> index 4160632..50ed866 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> @@ -1,7 +1,7 @@
>  /** @file
>EFI PCI IO protocol functions implementation for PCI Bus module.
>  
> -Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.
> +Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD 
> License
>  which accompanies this distribution.  The full text of the license may be 
> found at
> @@ -1774,9 +1774,8 @@ PciIoGetBarAttributes (
>OUT VOID   **Resources OPTIONAL
>)
>  {
> -  UINT8 *Configuration;
>PCI_IO_DEVICE *PciIoDevice;
> -  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *AddressSpace;
> +  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor;
>EFI_ACPI_END_TAG_DESCRIPTOR   *End;
>  
>PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This);
> @@ -1798,19 +1797,18 @@ PciIoGetBarAttributes (
>}
>  
>if (Resources != NULL) {
> -Configuration = AllocateZeroPool (sizeof 
> (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) + sizeof (EFI_ACPI_END_TAG_DESCRIPTOR));
> -if (Configuration == NULL) {
> +Descriptor = AllocateZeroPool (sizeof 
> (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) + sizeof (EFI_ACPI_END_TAG_DESCRIPTOR));
> +if (Descriptor == NULL) {
>return EFI_OUT_OF_RESOURCES;
>  }
>  
> -AddressSpace = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *) Configuration;
> +*Resources   = Descriptor;
>  
> -AddressSpace->Desc = ACPI_ADDRESS_SPACE_DESCRIPTOR;
> -AddressSpace->Len  = (UINT16) (sizeof 
> (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3);
> -
> -AddressSpace->AddrRangeMin = PciIoDevice->PciBar[BarIndex].BaseAddress;
> -AddressSpace->AddrLen  = PciIoDevice->PciBar[BarIndex].Length;
> -AddressSpace->AddrRangeMax = PciIoDevice->PciBar[BarIndex].Alignment;
> +Descriptor->Desc = ACPI_ADDRESS_SPACE_DESCRIPTOR;
> +Descriptor->Len  = (UINT16) (sizeof 
> (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3);
> +Descriptor->AddrRangeMin = PciIoDevice->PciBar[BarIndex].BaseAddress;
> +Descriptor->AddrLen  = PciIoDevice->PciBar[BarIndex].Length;
> +Descriptor->AddrRangeMax = PciIoDevice->PciBar[BarIndex].Alignment;
>  
>  switch (PciIoDevice->PciBar[BarIndex].BarType) {
>  case PciBarTypeIo16:
> @@ -1818,59 +1816,41 @@ PciIoGetBarAttributes (
>//
>// Io
>//
> -  AddressSpace->ResType = ACPI_ADDRESS_SPACE_TYPE_IO;
> -  break;
> -
> -case PciBarTypeMem32:
> -  //
> -  // Mem
> -  //
> -  AddressSpace->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
> -  //
> -  // 32 bit
> -  //
> -  AddressSpace->AddrSpaceGranularity = 32;
> +  Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_IO;
>break;
>  
>  case PciBarTypePMem32:
>//
> -  // Mem
> -  //
> -  AddressSpace->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
> -  //
>// prefechable
>//
> -  AddressSpace->SpecificFlag = 0x6;
> -  //
> -  // 32 bit
> -  //
> -  AddressSpace->AddrSpaceGranularity = 32;
> -  break;
> +  Descriptor->SpecificFlag = 
> EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
>  
> -case PciBarTypeMem64:
> +case PciBarTypeMem32:
>//
>// Mem
>//
> -  AddressSpace->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
> +  Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
>//
> -  // 64 bit
> +  // 32 bit
>//
> -  AddressSpace->AddrSpaceGranularity = 64;
> +  Descriptor->AddrSpaceGranularity = 32;
>break;
>  
>  case PciBarTypePMem64:
>//
> -  // Mem
> +  // prefechable
>//
> -  AddressSpace->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
> +  Descr

Re: [edk2] [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Enable/Restore XD in SMM

[Laszlo Ersek]

On 02/19/16 03:35, Michael Kinney wrote:
> If XD is supported, then SMM enables it.  The non-SMM execution
> environment can choose to enable or disable XD, so the state of
> XD must be detected in each SMI and be enabled/restored.
> 
> Cc: Jeff Fan 
> Cc: Jiewen Yao 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Michael Kinney 
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c  | 18 +++---
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h |  3 ++-
>  2 files changed, 17 insertions(+), 4 deletions(-)

I did the following (usual) regression tests for this patch:
- OVMF Ia32 build, with the QEMU & KVM settings documented in the
  OVMF README; boot, suspend-resume, variable access on VCPU#0
  and != VCPU#0; Fedlet (~32 bit Fedora) guest
- same with OVMF Ia32X64 build, 64-bit Fedora guest
- OVMF Ia32X64 build, debug/checked Windows 8.1 guest, suspend-resume.

Regression-tested-by: Laszlo Ersek 

Thanks
Laszlo

> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 79b7c90..185cb3d 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -1,7 +1,7 @@
>  /** @file
>  SMM MP service implementation
>  
> -Copyright (c) 2009 - 2015, Intel Corporation. All rights reserved.
> +Copyright (c) 2009 - 2016, Intel Corporation. All rights reserved.
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD 
> License
>  which accompanies this distribution.  The full text of the license may be 
> found at
> @@ -1019,6 +1019,7 @@ SmiRendezvous (
>BOOLEAN   BspInProgress;
>UINTN Index;
>UINTN Cr2;
> +  BOOLEAN   XdDisableFlag;
>  
>//
>// Save Cr2 because Page Fault exception in SMM may override its value
> @@ -1078,9 +1079,14 @@ SmiRendezvous (
>  }
>  
>  //
> -// Try to enable NX
> +// Try to enable XD
>  //
> +XdDisableFlag = FALSE;
>  if (mXdSupported) {
> +  if ((AsmReadMsr64 (MSR_IA32_MISC_ENABLE) & B_XD_DISABLE_BIT) != 0) {
> +XdDisableFlag = TRUE;
> +AsmMsrAnd64 (MSR_IA32_MISC_ENABLE, ~B_XD_DISABLE_BIT);
> +  }
>ActivateXd ();
>  }
>  
> @@ -1152,7 +1158,6 @@ SmiRendezvous (
>  // BSP Handler is always called with a ValidSmi == TRUE
>  //
>  BSPHandler (CpuIndex, mSmmMpSyncData->EffectiveSyncMode);
> -
>} else {
>  APHandler (CpuIndex, ValidSmi, mSmmMpSyncData->EffectiveSyncMode);
>}
> @@ -1165,6 +1170,13 @@ SmiRendezvous (
>  //
>  while (mSmmMpSyncData->AllCpusInSync) {
>CpuPause ();
> + }
> +
> +//
> +// Restore XD
> +//
> +if (XdDisableFlag) {
> +  AsmMsrOr64 (MSR_IA32_MISC_ENABLE, B_XD_DISABLE_BIT);
>  }
>}
>  
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h
> index 4548467..f91c104 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h
> @@ -1,7 +1,7 @@
>  /** @file
>  SMM profile header file.
>  
> -Copyright (c) 2012 - 2015, Intel Corporation. All rights reserved.
> +Copyright (c) 2012 - 2016, Intel Corporation. All rights reserved.
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD 
> License
>  which accompanies this distribution.  The full text of the license may be 
> found at
> @@ -21,6 +21,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
> EXPRESS OR IMPLIED.
>  /// MSR Register Index
>  ///
>  #define MSR_IA32_MISC_ENABLE  0x1A0
> +#define   B_XD_DISABLE_BITBIT34
>  
>  //
>  // External functions
> 

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

Re: [edk2] [patch] MdeModulePkg: Should not overwrite the BltX

[Laszlo Ersek]

On 02/19/16 06:34, Dandan Bi wrote:
> when has next line to draw, should not overwrite the BltX
> to 0, instead should keep the BltX value that pass into
> StringToImage function.
> 
> Cc: Liming Gao 
> Cc: Eric Dong 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Dandan Bi 
> ---
>  MdeModulePkg/Universal/HiiDatabaseDxe/Font.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Please modify the subject line:

  MdeModulePkg: HiiDatabaseDxe: HiiStringToImage() should not overwrite BltX

(74 characters)

Thanks
Laszlo

> diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/Font.c 
> b/MdeModulePkg/Universal/HiiDatabaseDxe/Font.c
> index 4b70b99..56b30ff 100644
> --- a/MdeModulePkg/Universal/HiiDatabaseDxe/Font.c
> +++ b/MdeModulePkg/Universal/HiiDatabaseDxe/Font.c
> @@ -1,10 +1,10 @@
>  /** @file
>  Implementation for EFI_HII_FONT_PROTOCOL.
>  
>  
> -Copyright (c) 2007 - 2015, Intel Corporation. All rights reserved.
> +Copyright (c) 2007 - 2016, Intel Corporation. All rights reserved.
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD 
> License
>  which accompanies this distribution.  The full text of the license may be 
> found at
>  http://opensource.org/licenses/bsd-license.php
>  
> @@ -2154,13 +2154,12 @@ HiiStringToImage (
>}
>  }
>  
>  NextLine:
>  //
> -// Recalculate the start point of X/Y axis to draw multi-lines with the 
> order of top-to-down
> +// Recalculate the start point of Y axis to draw multi-lines with the 
> order of top-to-down
>  //
> -BltX = 0;
>  BltY += RowInfo[RowIndex].LineHeight;
>  
>  RowIndex++;
>  Index = NextIndex;
>  
> 

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

Re: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB on ACPI 1.0

[Laszlo Ersek]

On 02/19/16 09:53, Ard Biesheuvel wrote:
> On 19 February 2016 at 09:45, Yao, Jiewen  wrote:
>> I can explain the reason on allocating <4G. It is because this data will be 
>> used in PEI phase in S3 resume.
>>
>> On most X86 platform, PEI phase is 32bit, and DXE phase is 32bit or 64bit. 
>> So we have to limit the allocation <4G.
>>
> 
> OK, got it.
> 
>> Using ACPI version PCD looks strange here. Maybe another PCD, like 
>> PcdDxeIplSwitchToLongMode?
>>
> 
> But that does not tell us if we are running a 32-bit PEI, right? It
> only tells us if a 32-bit PEI should load a 32-bit DXE core on a
> 64-bit capable machine.

32->32 and 64->64 builds have PcdDxeIplSwitchToLongMode set to FALSE.

Only 32->64 builds have PcdDxeIplSwitchToLongMode set to TRUE.

(This is what we have in the three DSC files of OVMF.)

So, in order to see in DXE if your PEI is 32-bit, you can do:

  BOOLEAN PeiIs32Bit;

  PeiIs32Bit = FALSE;
  if (FeaturePcdGet (PcdDxeIplSwitchToLongMode)) {
//
// this implies a 32->64 switch
//
PeiIs32Bit = TRUE;
  } else {
//
// otherwise, PEI and DXE have the same bitness,
// so derive it from DXE's bitness
//
#if defined (MDE_CPU_IA32) || defined (MDE_CPU_ARM)
PeiIs32Bit = TRUE;
#endif
  }

Alternatively:

  PeiIs32Bit = FeaturePcdGet (PcdDxeIplSwitchToLongMode) ||
   sizeof (UINTN) == sizeof (UINT32);

I'll admit I'm not really sure about EBC. The above mostly treats EBC as
nonexistent. :)

Thanks
Laszlo

> 
> 
>> -Original Message-
>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>> Sent: Friday, February 19, 2016 4:03 AM
>> To: edk2-devel@lists.01.org; Tian, Feng; Zeng, Star; 
>> leif.lindh...@linaro.org; graeme.greg...@linaro.org; ler...@redhat.com; Fan, 
>> Jeff; Yao, Jiewen
>> Cc: mark.rutl...@arm.com; Gao, Liming; samer.el-haj-mahm...@hpe.com; Ard 
>> Biesheuvel
>> Subject: [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 
>> 4 GB on ACPI 1.0
>>
>> It is not entirely clear from the code why, but the reserved region for 
>> storing performance data is allocated below 4 GB, and the variable to hold 
>> the address of the allocation is called 'AcpiLowMemoryBase'
>> (which is the only mention of ACPI in the entire file).
>>
>> Let's make this allocation dependent on PcdAcpiExposedTableVersions as well, 
>> since systems that can deal with ACPI table in high memory are also just as 
>> likely to be able to deal with performance data in high memory. This 
>> prevents memory fragmentation on systems that don't care about the 4 GB 
>> limit.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>  IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf |  1 +  
>> IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c | 11 ++-
>>  2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf 
>> b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
>> index 6afb8a09df9c..38172780fb49 100644
>> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
>> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
>> @@ -215,6 +215,7 @@ [Pcd]
>>gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoHorizontalResolution  
>>   ## CONSUMES
>>gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoVerticalResolution
>>   ## CONSUMES
>>gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable
>>   ## CONSUMES
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions
>>   ## CONSUMES
>>
>>  [Depex]
>>TRUE
>> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c 
>> b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
>> index ae7ad2153c51..bf5bd6524a90 100644
>> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
>> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
>> @@ -22,6 +22,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
>> EXPRESS OR IMPLIED.
>>  #include "Hotkey.h"
>>  #include "HwErrRecSupport.h"
>>
>> +#include 
>> +
>>  ///
>>  /// BDS arch protocol instance initial value.
>>  ///
>> @@ -474,14 +476,21 @@ BdsAllocateMemoryForPerformanceData (
>>EFI_STATUSStatus;
>>EFI_PHYSICAL_ADDRESS  AcpiLowMemoryBase;
>>EDKII_VARIABLE_LOCK_PROTOCOL  *VariableLock;
>> +  EFI_ALLOCATE_TYPE AcpiTableAllocType;
>>
>>AcpiLowMemoryBase = 0x0ULL;
>>
>> +  if ((PcdGet32 (PcdAcpiExposedTableVersions) & 
>> EFI_ACPI_TABLE_VERSION_1_0B) != 0) {
>> +AcpiTableAllocType = AllocateMaxAddress;  } else {
>> +AcpiTableAllocType = AllocateAnyPages;  }
>> +
>>//
>>// Allocate a block of memory that will contain performance data to OS.
>>//
>>Status = gBS->AllocatePages (
>> -  AllocateMaxAddress,
>> +  AcpiTableAllocType,
>>EfiReservedMemoryType,
>>EFI_SIZE_TO_PAGES (PERF_DATA_MAX_LENGTH),

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

[David Woodhouse]

On Fri, 2016-02-19 at 10:43 +0100, Laszlo Ersek wrote:
> 
> I can test this for you, if you give me precise instructions.
> 
> (I'm asking for instructions because CryptoPkg/Include/openssl/README is
> deleted in one of the early patches.)

It moved from Patch-HOWTO.txt to OpenSSL-HOWTO.txt since there was no
longer a patch involved. Or did I forget to add the new file?


 Introduction

  OpenSSL is a well-known open source implementation of SSL and TLS protocols.
The core library implements the basic cryptographic functions and provides 
various
utility functions. The OpenSSL library is widely used in variety of security
products development as base crypto provider. (See http://www.openssl.org/ for 
more
information on OpenSSL).
  UEFI (Unified Extensible Firmware Interface) is a specification detailing the
interfaces between OS and platform firmware. Several security features were
introduced (e.g. Authenticated Variable Service, Driver Signing, etc) from UEFI
2.2 (http://www.uefi.org/). These security features highly depend on the
cryptography. This HOWTO documents OpenSSL building under UEFI environment.



OpenSSL-Version

  EDKII supports building with the master branch of OpenSSL, which will
  eventually be released as OpenSSL 1.1.0. In additional, fixes for the
  following OpenSSL "Request Tracker" tickets are required:

   RT4175: Fix regression using PKCS7_verify() with Authenticode
   RT4309: Define PRIu64 for UEFI build

  A clone of the OpenSSL git repository with all the required fixes is
  available at git://git.infradead.org/users/dwmw2/openssl.git



  HOW to Install Openssl for UEFI Building


1. Clone the above-referenced git repository into the directory
  CryptoPkg/Library/OpensslLib/openssl/

2. Copy the opensslconf.h file from this directory into the newly-created
   CryptoPkg/Library/OpensslLib/openssl/include/openssl directory next to
   the other OpenSSL include files.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

[Laszlo Ersek]

On 02/19/16 09:59, David Woodhouse wrote:
> On Fri, 2016-02-19 at 07:59 +, Long, Qin wrote:
>> I agree those changes really make sense for better alignment, under
>> both 1.0.2xx and 1.1 HEAD. The final out-of-box support will be
>> wonderful. 
>> The updates (http://git.infradead.org/users/dwmw2/edk2.git) looks
>> good to me. And I will follow more validations, and start the next
>> integration step-by-step. 
> 
> Great, thanks. I should note that, as before, I haven't actually done
> any real *testing* of this lot. Only build testing under Linux. I think
> I did manage to create a Windows VM with a build environment for EDK2
> at one point; I should at least dust that off.

I can test this for you, if you give me precise instructions.

(I'm asking for instructions because CryptoPkg/Include/openssl/README is
deleted in one of the early patches.)

I'm thinking of the following tests, with OVMF:

(1a) Enroll keys, and confirm SB being active in the Fedora guest,
 using my current build.
(1b) Rebuild the firmware binary with your patches & instructions. Do
 not touch the VM's varstore.
(1c) Confirm SB is still active in the Fedora guest.
(1d) Confirm an unsigned UEFI shell binary from removable media cannot
 be launched.

(2a) Enroll keys, and confirm SB being active in the Fedora guest,
 using your build from the outset.
(2b) Confirm an unsigned UEFI shell binary from removable media cannot
 be launched.

Sound good?

Thanks
Laszlo

> 
>> Yeah, also will do more follow-ups about the remaining opens...
> 
> Like using the OpenSSL TS support instead of our own? That would be
> good. Likewise I think I remember a vague plan of making it possible to
> disable the OCSP code, since we don't use it?
> 
> Note that the OpenSSL 1.1 Beta 1 release is scheduled in "about a
> month"¹, and that is the feature/API freeze deadline. Anything we want
> added (other than bug fixes, of course), needs to be in by then.
> 
> In the fullness of time, I would *also* like to clean up the litter of
> include files we provide in CryptoPkg/Include purely for the benefit of
> OpenSSL — removing stuff from OpenSslSupport.h as we go. If we really
> want to stop OpenSSL from including/requiring those headers then that
> makes sense to do before Beta 1 too.
> 
> I might start by looking for things which can be considered "obviously"
> bugs — like including anything in netinet/ when configured with no-sock 
> for example. And I really don't see why it needs syslog.h for the UEFI
> build, or dirent.h for a no-stdio (which really means no file access)
> build.
> 

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

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

[David Woodhouse]

On Fri, 2016-02-19 at 09:19 +, Long, Qin wrote:
> 
> //nod
> Some old include / structure definitions were put there just to satisfy the 
> compiler in early phase, which looks like one rough style to make it work. 
> It's
> really the time to have one clean-up for our "out-of-the-box" integration.

Yeah, I might actually throw the whole lot away and start again, adding
only what's needed (and only then when I can't make OpenSSL *stop*
needing it)...

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch v2] UefiCpuPkg/PiSmmCpuDxeSmm: Add EFIAPI to CheckFeatureSupported()

[Laszlo Ersek]

On 02/19/16 02:53, Michael Kinney wrote:
> The function CheckFeatureSupported() is used as an EFI_AP_PROCEDURE
> in the MP Services Protocol service StartAllAPs().  Any function
> used as an EFI_AP_PROCEDURE must use EFIAPI calling convention.
> 
> Cc: Laszlo Ersek 
> Cc: Jeff Fan 
> Cc: Jiewen Yao 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Michael Kinney 
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 9 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h | 5 +++--
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> index ec4ec9b..9c4f387 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> @@ -1,7 +1,7 @@
>  /** @file
>  Enable SMM profile.
>  
> -Copyright (c) 2012 - 2015, Intel Corporation. All rights reserved.
> +Copyright (c) 2012 - 2016, Intel Corporation. All rights reserved.
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD 
> License
>  which accompanies this distribution.  The full text of the license may be 
> found at
> @@ -930,8 +930,9 @@ InitSmmProfileInternal (
>  
>  **/
>  VOID
> +EFIAPI
>  CheckFeatureSupported (
> -  VOID
> +  IN OUT VOID   *Buffer
>)
>  {
>UINT32 RegEax;
> @@ -1001,7 +1002,7 @@ CheckProcessorFeature (
>//
>// Check if XD and BTS are supported on all processors.
>//
> -  CheckFeatureSupported ();
> +  CheckFeatureSupported (NULL);
>  
>//
>//Check on other processors if BSP supports this
> @@ -1009,7 +1010,7 @@ CheckProcessorFeature (
>if (mXdSupported || mBtsSupported) {
>  MpServices->StartupAllAPs (
>MpServices,
> -  (EFI_AP_PROCEDURE) CheckFeatureSupported,
> +  CheckFeatureSupported,
>TRUE,
>NULL,
>0,
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h
> index 4548467..d65048e 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h
> @@ -1,7 +1,7 @@
>  /** @file
>  SMM profile header file.
>  
> -Copyright (c) 2012 - 2015, Intel Corporation. All rights reserved.
> +Copyright (c) 2012 - 2016, Intel Corporation. All rights reserved.
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD 
> License
>  which accompanies this distribution.  The full text of the license may be 
> found at
> @@ -97,8 +97,9 @@ PageFaultIdtHandlerSmmProfile (
>  
>  **/
>  VOID
> +EFIAPI
>  CheckFeatureSupported (
> -  VOID
> +  IN OUT VOID   *Buffer
>);
>  
>  /**
> 

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] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

[Long, Qin]

> -Original Message-
> From: David Woodhouse [mailto:dw...@infradead.org]
> Sent: Friday, February 19, 2016 5:00 PM
> To: Long, Qin; Laszlo Ersek
> Cc: Ye, Ting; edk2-de...@ml01.01.org
> Subject: Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version
> to 1.0.2f
> 
> On Fri, 2016-02-19 at 07:59 +, Long, Qin wrote:
> > I agree those changes really make sense for better alignment, under
> > both 1.0.2xx and 1.1 HEAD. The final out-of-box support will be
> > wonderful.
> > The updates (http://git.infradead.org/users/dwmw2/edk2.git) looks
> > good to me. And I will follow more validations, and start the next
> > integration step-by-step.
> 
> Great, thanks. I should note that, as before, I haven't actually done
> any real *testing* of this lot. Only build testing under Linux. I think
> I did manage to create a Windows VM with a build environment for EDK2
> at one point; I should at least dust that off.
> 
> > Yeah, also will do more follow-ups about the remaining opens...
> 
> Like using the OpenSSL TS support instead of our own? That would be
> good. Likewise I think I remember a vague plan of making it possible to
> disable the OCSP code, since we don't use it?
> 
> Note that the OpenSSL 1.1 Beta 1 release is scheduled in "about a
> month"¹, and that is the feature/API freeze deadline. Anything we want
> added (other than bug fixes, of course), needs to be in by then.

Right. Just re-pickup those tasks. So will hurry up. 

> 
> In the fullness of time, I would *also* like to clean up the litter of
> include files we provide in CryptoPkg/Include purely for the benefit of
> OpenSSL — removing stuff from OpenSslSupport.h as we go. If we really
> want to stop OpenSSL from including/requiring those headers then that
> makes sense to do before Beta 1 too.
> 
> I might start by looking for things which can be considered "obviously"
> bugs — like including anything in netinet/ when configured with no-sock
> for example. And I really don't see why it needs syslog.h for the UEFI
> build, or dirent.h for a no-stdio (which really means no file access)
> build.
> 

//nod
Some old include / structure definitions were put there just to satisfy the 
compiler in early phase, which looks like one rough style to make it work. It's
really the time to have one clean-up for our "out-of-the-box" integration. 

> --
> David WoodhouseOpen Source Technology Centre
> david.woodho...@intel.com  Intel Corporation
> 
> 
> ¹ https://www.mail-archive.com/openssl-dev@openssl.org/msg42723.html

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

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

[David Woodhouse]

On Fri, 2016-02-19 at 07:59 +, Long, Qin wrote:
> I agree those changes really make sense for better alignment, under
> both 1.0.2xx and 1.1 HEAD. The final out-of-box support will be
> wonderful. 
> The updates (http://git.infradead.org/users/dwmw2/edk2.git) looks
> good to me. And I will follow more validations, and start the next
> integration step-by-step. 

Great, thanks. I should note that, as before, I haven't actually done
any real *testing* of this lot. Only build testing under Linux. I think
I did manage to create a Windows VM with a build environment for EDK2
at one point; I should at least dust that off.

> Yeah, also will do more follow-ups about the remaining opens...

Like using the OpenSSL TS support instead of our own? That would be
good. Likewise I think I remember a vague plan of making it possible to
disable the OCSP code, since we don't use it?

Note that the OpenSSL 1.1 Beta 1 release is scheduled in "about a
month"¹, and that is the feature/API freeze deadline. Anything we want
added (other than bug fixes, of course), needs to be in by then.

In the fullness of time, I would *also* like to clean up the litter of
include files we provide in CryptoPkg/Include purely for the benefit of
OpenSSL — removing stuff from OpenSslSupport.h as we go. If we really
want to stop OpenSSL from including/requiring those headers then that
makes sense to do before Beta 1 too.

I might start by looking for things which can be considered "obviously"
bugs — like including anything in netinet/ when configured with no-sock 
for example. And I really don't see why it needs syslog.h for the UEFI
build, or dirent.h for a no-stdio (which really means no file access)
build.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation


¹ https://www.mail-archive.com/openssl-dev@openssl.org/msg42723.html



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB on ACPI 1.0

[Ard Biesheuvel]

On 19 February 2016 at 09:45, Yao, Jiewen  wrote:
> I can explain the reason on allocating <4G. It is because this data will be 
> used in PEI phase in S3 resume.
>
> On most X86 platform, PEI phase is 32bit, and DXE phase is 32bit or 64bit. So 
> we have to limit the allocation <4G.
>

OK, got it.

> Using ACPI version PCD looks strange here. Maybe another PCD, like 
> PcdDxeIplSwitchToLongMode?
>

But that does not tell us if we are running a 32-bit PEI, right? It
only tells us if a 32-bit PEI should load a 32-bit DXE core on a
64-bit capable machine.


> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Friday, February 19, 2016 4:03 AM
> To: edk2-devel@lists.01.org; Tian, Feng; Zeng, Star; 
> leif.lindh...@linaro.org; graeme.greg...@linaro.org; ler...@redhat.com; Fan, 
> Jeff; Yao, Jiewen
> Cc: mark.rutl...@arm.com; Gao, Liming; samer.el-haj-mahm...@hpe.com; Ard 
> Biesheuvel
> Subject: [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 
> 4 GB on ACPI 1.0
>
> It is not entirely clear from the code why, but the reserved region for 
> storing performance data is allocated below 4 GB, and the variable to hold 
> the address of the allocation is called 'AcpiLowMemoryBase'
> (which is the only mention of ACPI in the entire file).
>
> Let's make this allocation dependent on PcdAcpiExposedTableVersions as well, 
> since systems that can deal with ACPI table in high memory are also just as 
> likely to be able to deal with performance data in high memory. This prevents 
> memory fragmentation on systems that don't care about the 4 GB limit.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> ---
>  IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf |  1 +  
> IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c | 11 ++-
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf 
> b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
> index 6afb8a09df9c..38172780fb49 100644
> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
> @@ -215,6 +215,7 @@ [Pcd]
>gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoHorizontalResolution   
>  ## CONSUMES
>gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoVerticalResolution 
>  ## CONSUMES
>gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable 
>  ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions 
>  ## CONSUMES
>
>  [Depex]
>TRUE
> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c 
> b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
> index ae7ad2153c51..bf5bd6524a90 100644
> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
> @@ -22,6 +22,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
> EXPRESS OR IMPLIED.
>  #include "Hotkey.h"
>  #include "HwErrRecSupport.h"
>
> +#include 
> +
>  ///
>  /// BDS arch protocol instance initial value.
>  ///
> @@ -474,14 +476,21 @@ BdsAllocateMemoryForPerformanceData (
>EFI_STATUSStatus;
>EFI_PHYSICAL_ADDRESS  AcpiLowMemoryBase;
>EDKII_VARIABLE_LOCK_PROTOCOL  *VariableLock;
> +  EFI_ALLOCATE_TYPE AcpiTableAllocType;
>
>AcpiLowMemoryBase = 0x0ULL;
>
> +  if ((PcdGet32 (PcdAcpiExposedTableVersions) & EFI_ACPI_TABLE_VERSION_1_0B) 
> != 0) {
> +AcpiTableAllocType = AllocateMaxAddress;  } else {
> +AcpiTableAllocType = AllocateAnyPages;  }
> +
>//
>// Allocate a block of memory that will contain performance data to OS.
>//
>Status = gBS->AllocatePages (
> -  AllocateMaxAddress,
> +  AcpiTableAllocType,
>EfiReservedMemoryType,
>EFI_SIZE_TO_PAGES (PERF_DATA_MAX_LENGTH),
>&AcpiLowMemoryBase
> --
> 2.5.0
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB on ACPI 1.0

[Yao, Jiewen]

I can explain the reason on allocating <4G. It is because this data will be 
used in PEI phase in S3 resume.

On most X86 platform, PEI phase is 32bit, and DXE phase is 32bit or 64bit. So 
we have to limit the allocation <4G.

Using ACPI version PCD looks strange here. Maybe another PCD, like 
PcdDxeIplSwitchToLongMode?

Thank you
Yao Jiewen

-Original Message-
From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] 
Sent: Friday, February 19, 2016 4:03 AM
To: edk2-devel@lists.01.org; Tian, Feng; Zeng, Star; leif.lindh...@linaro.org; 
graeme.greg...@linaro.org; ler...@redhat.com; Fan, Jeff; Yao, Jiewen
Cc: mark.rutl...@arm.com; Gao, Liming; samer.el-haj-mahm...@hpe.com; Ard 
Biesheuvel
Subject: [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 
GB on ACPI 1.0

It is not entirely clear from the code why, but the reserved region for storing 
performance data is allocated below 4 GB, and the variable to hold the address 
of the allocation is called 'AcpiLowMemoryBase'
(which is the only mention of ACPI in the entire file).

Let's make this allocation dependent on PcdAcpiExposedTableVersions as well, 
since systems that can deal with ACPI table in high memory are also just as 
likely to be able to deal with performance data in high memory. This prevents 
memory fragmentation on systems that don't care about the 4 GB limit.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
 IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf |  1 +  
IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c | 11 ++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf 
b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
index 6afb8a09df9c..38172780fb49 100644
--- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
+++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
@@ -215,6 +215,7 @@ [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoHorizontalResolution
## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoVerticalResolution  
## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable  
## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions  
## CONSUMES
 
 [Depex]
   TRUE
diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c 
b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
index ae7ad2153c51..bf5bd6524a90 100644
--- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -22,6 +22,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include "Hotkey.h"
 #include "HwErrRecSupport.h"
 
+#include 
+
 ///
 /// BDS arch protocol instance initial value.
 ///
@@ -474,14 +476,21 @@ BdsAllocateMemoryForPerformanceData (
   EFI_STATUSStatus;
   EFI_PHYSICAL_ADDRESS  AcpiLowMemoryBase;
   EDKII_VARIABLE_LOCK_PROTOCOL  *VariableLock;
+  EFI_ALLOCATE_TYPE AcpiTableAllocType;
 
   AcpiLowMemoryBase = 0x0ULL;
 
+  if ((PcdGet32 (PcdAcpiExposedTableVersions) & EFI_ACPI_TABLE_VERSION_1_0B) 
!= 0) {
+AcpiTableAllocType = AllocateMaxAddress;  } else {
+AcpiTableAllocType = AllocateAnyPages;  }
+
   //
   // Allocate a block of memory that will contain performance data to OS.
   //
   Status = gBS->AllocatePages (
-  AllocateMaxAddress,
+  AcpiTableAllocType,
   EfiReservedMemoryType,
   EFI_SIZE_TO_PAGES (PERF_DATA_MAX_LENGTH),
   &AcpiLowMemoryBase
--
2.5.0

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

Re: [edk2] [patch] MdeModulePkg: Fix Memory Attributes table type issue

[Yao, Jiewen]

That is weird. I did validate on my platform.

Let me find a way to reproduce and fix it.

Thank you
Yao Jiewen


From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
Sent: Friday, February 19, 2016 4:35 PM
To: Yao, Jiewen
Cc: edk2-devel@lists.01.org; Gao, Liming
Subject: Re: [edk2] [patch] MdeModulePkg: Fix Memory Attributes table type issue

On 19 February 2016 at 09:12, Jiewen Yao wrote:
> According to the spec, each entry in the Memory Attributes table shall
> have the same type as the region it was carved out of in the UEFI
> memory map.
> The current attribute uses RTData for PE Data, but it should be
> RTCode.
>
> This patch fixed the issue. It is validated with or without
> PropertiesTable.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: "Yao, Jiewen"
> Cc: "Ard Biesheuvel"
> Cc: "Gao, Liming"

Thanks for the quick fix

Unfortunately, the table appears corrupted now:

UEFI memmap:
0x00013c0e-0x00013c15 [Runtime Code |RUN| | | | | ]

0x00013c16-0x00013c17 [Runtime Data |RUN| | | | | ]

0x00013c1a-0x00013c1d [Runtime Code |RUN| | | | | ]

0x00013c1e-0x00013c27 [Runtime Data |RUN| | | | | ]

0x00013c28-0x00013c2c [Runtime Code |RUN| | | | | ]

0x00013c2d-0x00013c31 [Runtime Data |RUN| | | | | ]

0x00013c32-0x00013c36 [Runtime Code |RUN| | | | | ]

0x00013f65-0x00013f6d [Runtime Code |RUN| | | | | ]

0x00013f6f-0x00013f80 [Runtime Data |RUN| | | | | ]

Memory Attributes table:
0x00013c0e-0x00013c0e [Runtime Code |RUN| |XP| | | ]
0x00013c0f-0x00013c0f [Runtime Code |RUN| | | | |RO]
0x00013c10-0x00013c12 [Runtime Code |RUN| |XP| | | ]
0x00013c13-0x00013c13 [Runtime Code |RUN| | | | |RO]
0x00013c14-0x00013c14 [Runtime Code |RUN| |XP| | | ]
0x00013c14-0x00013c14 [Runtime Code |RUN| |XP| | | ]
0x00013c1a-0x00013c1a [Runtime Code |RUN| |XP| | | ]
0x00013c1b-0x00013c1b [Runtime Code |RUN| | | | |RO]
0x00013c1c-0x00013c1c [Runtime Code |RUN| |XP| | | ]
0x00013c1c-0x00013c1c [Runtime Code |RUN| |XP| | | ]
0x00013c28-0x00013c28 [Runtime Code |RUN| |XP| | | ]
0x00013c29-0x00013c29 [Runtime Code |RUN| | | | |RO]
0x00013c2a-0x00013c2b [Runtime Code |RUN| |XP| | | ]
0x00013c2a-0x00013c2b [Runtime Code |RUN| |XP| | | ]
0x00013c32-0x00013c32 [Runtime Code |RUN| |XP| | | ]
0x00013c33-0x00013c33 [Runtime Code |RUN| | | | |RO]
0x00013c34-0x00013c35 [Runtime Code |RUN| |XP| | | ]
0x00013c34-0x00013c35 [Runtime Code |RUN| |XP| | | ]
0x00013f65-0x00013f65 [Runtime Code |RUN| |XP| | | ]
0x00013f66-0x00013f66 [Runtime Code |RUN| | | | |RO]
0x00013f67-0x00013f69 [Runtime Code |RUN| |XP| | | ]
0x00013f6a-0x00013f6a [Runtime Code |RUN| | | | |RO]
0x00013f6b-0x00013f6c [Runtime Code |RUN| |XP| | | ]
0x00013f6b-0x00013f6c [Runtime Code |RUN| |XP| | | ]
0x00013f6f-0x00013f80 [Runtime Data |RUN| |XP| | | ]

There are duplicate entries in the memattr table, and some others are missing

> ---
> MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c | 89
> ++--
> 1 file changed, 19 insertions(+), 70 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> index 6e5ad03..b052148 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> @@ -81,13 +81,10 @@ EFI_PROPERTIES_TABLE mPropertiesTable = {
> EFI_LOCK mPropertiesTableLock = EFI_INITIALIZE_LOCK_VARIABLE (TPL_NOTIFY);
>
> //
> -// Temporary save for original memory map.
> // This is for MemoryAttributesTable only.
> //
> extern BOOLEAN mIsConstructingMemoryAttributesTable;
> -EFI_MEMORY_DESCRIPTOR *mMemoryMapOrg;
> -UINTN mMemoryMapOrgSize;
> -UINTN mDescriptorSize;
> +BOOLEAN mPropertiesTableEnable;
>
> //
> // Below functions are for MemoryMap
> @@ -199,42 +196,6 @@ SortMemoryMap (
> }
>
> /**
> - Check if this memory entry spans across original memory map boundary.
> -
> - @param PhysicalStart The PhysicalStart of memory
> - @param NumberOfPages The NumberOfPages of memory
> -
> - @retval TRUE This memory entry spans across original memory map boundary.
> - @retval FALSE This memory entry does not span cross original memory map 
> boundary.
> -**/
> -STATIC
> -BOOLEAN
> -DoesEntrySpanAcrossBoundary (
> - IN UINT64 PhysicalStart,
> - IN UINT64 NumberOfPages
> - )
> -{
> - EFI_MEMORY_DESCRIPTOR *MemoryMapEntry;
> - EFI_MEMORY_DESCRIPTOR *MemoryMapEnd;
> - UINT64 MemoryBlockLength;
> -
> - MemoryMapEntry = mMemoryMapOrg;
> - MemoryMapEnd = (EFI_MEMORY_DESCRIPTOR *) ((UINT8 *) mMemoryMapOrg + 
> mMemoryMapOrgSize);
> - while (MemoryMapEntry < MemoryMapEnd) {
> - MemoryBlockLength = (UINT64) (EfiPagesToSize 
> (MemoryMapEntry->NumberOfPages));
> -
> - if ((MemoryMapEntry->PhysicalStart <= PhysicalStart) &&
> - (MemoryMapEntry->PhysicalStart + Memory

Re: [edk2] [patch] MdeModulePkg: Fix Memory Attributes table type issue

[Ard Biesheuvel]

On 19 February 2016 at 09:12, Jiewen Yao  wrote:
> According to the spec, each entry in the Memory
> Attributes table shall have the same type as
> the region it was carved out of in the UEFI memory map.
> The current attribute uses RTData for PE Data, but
> it should be RTCode.
>
> This patch fixed the issue. It is validated with or
> without PropertiesTable.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: "Yao, Jiewen" 
> Cc: "Ard Biesheuvel" 
> Cc: "Gao, Liming" 

Thanks for the quick fix

Unfortunately, the table appears corrupted now:

UEFI memmap:
  0x00013c0e-0x00013c15 [Runtime Code  |RUN|  |  |  |  |  ]

  0x00013c16-0x00013c17 [Runtime Data  |RUN|  |  |  |  |  ]

  0x00013c1a-0x00013c1d [Runtime Code  |RUN|  |  |  |  |  ]

  0x00013c1e-0x00013c27 [Runtime Data  |RUN|  |  |  |  |  ]

  0x00013c28-0x00013c2c [Runtime Code  |RUN|  |  |  |  |  ]

  0x00013c2d-0x00013c31 [Runtime Data  |RUN|  |  |  |  |  ]

  0x00013c32-0x00013c36 [Runtime Code  |RUN|  |  |  |  |  ]

  0x00013f65-0x00013f6d [Runtime Code  |RUN|  |  |  |  |  ]

  0x00013f6f-0x00013f80 [Runtime Data  |RUN|  |  |  |  |  ]

Memory Attributes table:
  0x00013c0e-0x00013c0e [Runtime Code  |RUN|  |XP|  |  |  ]
  0x00013c0f-0x00013c0f [Runtime Code  |RUN|  |  |  |  |RO]
  0x00013c10-0x00013c12 [Runtime Code  |RUN|  |XP|  |  |  ]
  0x00013c13-0x00013c13 [Runtime Code  |RUN|  |  |  |  |RO]
  0x00013c14-0x00013c14 [Runtime Code  |RUN|  |XP|  |  |  ]
  0x00013c14-0x00013c14 [Runtime Code  |RUN|  |XP|  |  |  ]
  0x00013c1a-0x00013c1a [Runtime Code  |RUN|  |XP|  |  |  ]
  0x00013c1b-0x00013c1b [Runtime Code  |RUN|  |  |  |  |RO]
  0x00013c1c-0x00013c1c [Runtime Code  |RUN|  |XP|  |  |  ]
  0x00013c1c-0x00013c1c [Runtime Code  |RUN|  |XP|  |  |  ]
  0x00013c28-0x00013c28 [Runtime Code  |RUN|  |XP|  |  |  ]
  0x00013c29-0x00013c29 [Runtime Code  |RUN|  |  |  |  |RO]
  0x00013c2a-0x00013c2b [Runtime Code  |RUN|  |XP|  |  |  ]
  0x00013c2a-0x00013c2b [Runtime Code  |RUN|  |XP|  |  |  ]
  0x00013c32-0x00013c32 [Runtime Code  |RUN|  |XP|  |  |  ]
  0x00013c33-0x00013c33 [Runtime Code  |RUN|  |  |  |  |RO]
  0x00013c34-0x00013c35 [Runtime Code  |RUN|  |XP|  |  |  ]
  0x00013c34-0x00013c35 [Runtime Code  |RUN|  |XP|  |  |  ]
  0x00013f65-0x00013f65 [Runtime Code  |RUN|  |XP|  |  |  ]
  0x00013f66-0x00013f66 [Runtime Code  |RUN|  |  |  |  |RO]
  0x00013f67-0x00013f69 [Runtime Code  |RUN|  |XP|  |  |  ]
  0x00013f6a-0x00013f6a [Runtime Code  |RUN|  |  |  |  |RO]
  0x00013f6b-0x00013f6c [Runtime Code  |RUN|  |XP|  |  |  ]
  0x00013f6b-0x00013f6c [Runtime Code  |RUN|  |XP|  |  |  ]
  0x00013f6f-0x00013f80 [Runtime Data  |RUN|  |XP|  |  |  ]

There are duplicate entries in the memattr table, and some others are missing

> ---
>  MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c | 89 
> ++--
>  1 file changed, 19 insertions(+), 70 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c 
> b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> index 6e5ad03..b052148 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> @@ -81,13 +81,10 @@ EFI_PROPERTIES_TABLE  mPropertiesTable = {
>  EFI_LOCK   mPropertiesTableLock = EFI_INITIALIZE_LOCK_VARIABLE 
> (TPL_NOTIFY);
>
>  //
> -// Temporary save for original memory map.
>  // This is for MemoryAttributesTable only.
>  //
>  extern BOOLEAN mIsConstructingMemoryAttributesTable;
> -EFI_MEMORY_DESCRIPTOR  *mMemoryMapOrg;
> -UINTN  mMemoryMapOrgSize;
> -UINTN  mDescriptorSize;
> +BOOLEANmPropertiesTableEnable;
>
>  //
>  // Below functions are for MemoryMap
> @@ -199,42 +196,6 @@ SortMemoryMap (
>  }
>
>  /**
> -  Check if this memory entry spans across original memory map boundary.
> -
> -  @param PhysicalStart   The PhysicalStart of memory
> -  @param NumberOfPages   The NumberOfPages of memory
> -
> -  @retval TRUE  This memory entry spans across original memory map boundary.
> -  @retval FALSE This memory entry does not span cross original memory map 
> boundary.
> -**/
> -STATIC
> -BOOLEAN
> -DoesEntrySpanAcrossBoundary (
> -  IN UINT64  PhysicalStart,
> -  IN UINT64  NumberOfPages
> -  )
> -{
> -  EFI_MEMORY_DESCRIPTOR   *MemoryMapEntry;
> -  EFI_MEMORY_DESCRIPTOR   *MemoryMapEnd;
> -  UINT64  MemoryBlockLength;
> -
> -  MemoryMapEntry = mMemoryMapOrg;
> -  MemoryMapEnd   = (EFI_MEMORY_DESCRIPTOR *) ((UINT8 *) mMemoryMapOrg + 
> mMemoryMapOrgSize);
> -  while (MemoryMapEntry < MemoryMapEnd) {
> -MemoryBlockLength = (UINT64) (EfiPagesToSize 
> (MemoryMapEntry->NumberOfPages));
> -
> -if ((Memor

Re: [edk2] [PATCH v2 1/3] MdeModulePkg: AcpiTableDxe: make 4 GB table allocation limit optional

[Yao, Jiewen]

Thanks. This update looks good to me. Reviewed-by: jiewen@intel.com

For version, I think we need update AcpiSdt.c as well?

==
EFI_ACPI_SDT_PROTOCOL  mAcpiSdtProtocolTemplate = {
  EFI_ACPI_TABLE_VERSION_NONE | EFI_ACPI_TABLE_VERSION_1_0B | 
ACPI_TABLE_VERSION_GTE_2_0,
==

I think adding EFI_ACPI_TABLE_VERSION_NONE might be a bug here. And we can use 
the PCD value for it to report correct version.

Thank you
Yao Jiewen

-Original Message-
From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] 
Sent: Friday, February 19, 2016 4:03 AM
To: edk2-devel@lists.01.org; Tian, Feng; Zeng, Star; leif.lindh...@linaro.org; 
graeme.greg...@linaro.org; ler...@redhat.com; Fan, Jeff; Yao, Jiewen
Cc: mark.rutl...@arm.com; Gao, Liming; samer.el-haj-mahm...@hpe.com; Ard 
Biesheuvel
Subject: [PATCH v2 1/3] MdeModulePkg: AcpiTableDxe: make 4 GB table allocation 
limit optional

AARCH64 systems never require compatibility with legacy ACPI OSes, and may not 
have any 32-bit addressable system RAM. To support ACPI on these systems, we 
need to be able to relax the 4 GB allocation restriction.
Since systems that do have 32-bit addressable RAM may prefer to reserve it for 
DMA buffer for 32-bit peripherals, and since it is generally preferred to keep 
firmware reserved regions close together (to reduce memory fragmentation 
affecting TLB footprint), we should not fall back to high allocations only if 
low allocations fail.

So add a PCD PcdAcpiExposedTableVersions containing a bitmask describing which 
ACPI versions are targeted, and wire it up it up to the memory allocation calls 
in AcpiTableDxe/AcpiTableProtocol.c. I.e., if ACPI v1.0b is not among the 
supported versions, the memory allocations are not limited to 4 GB, and only 
table types that carry 64-bit addresses are emitted.

Note that this will inhibit the publishing of any tables that carry only 32-bit 
addresses, i.e., RSDPv1, RSDTv1 and RSDTv3.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
 MdeModulePkg/MdeModulePkg.dec|  11 +
 MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf|   2 +
 MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 448 

 3 files changed, 275 insertions(+), 186 deletions(-)

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec 
index 5c5a9ee13138..febfc7d28046 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -4,6 +4,7 @@
 # and libraries instances, which are used for those modules.
 #
 # Copyright (c) 2007 - 2016, Intel Corporation. All rights reserved.
+# Copyright (c) 2016, Linaro Ltd. All rights reserved.
 # This program and the accompanying materials are licensed and made available 
under  # the terms and conditions of the BSD License that accompanies this 
distribution.
 # The full text of the license may be found at @@ -1049,6 +1050,16 @@ 
[PcdsFixedAtBuild, PcdsPatchableInModule]
   # @Prompt Driver guid array of VFR drivers for VarCheckHiiBin generation.
   gEfiMdeModulePkgTokenSpaceGuid.PcdVarCheckVfrDriverGuidArray|{ 0x00, 0x00, 
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
0x00 }|VOID*|0x3000103A
 
+  ## Indicates which ACPI versions are targeted by the ACPI tables 
+ exposed to the OS  #  These values are aligned with the definitions in 
MdePkg/Include/Protocol/AcpiSystemDescriptionTable.h
+  #   BIT 1 - EFI_ACPI_TABLE_VERSION_1_0B.
+  #   BIT 2 - EFI_ACPI_TABLE_VERSION_2_0.
+  #   BIT 3 - EFI_ACPI_TABLE_VERSION_3_0.
+  #   BIT 4 - EFI_ACPI_TABLE_VERSION_4_0.
+  #   BIT 5 - EFI_ACPI_TABLE_VERSION_5_0.
+  # @Prompt Exposed ACPI table versions.
+  
+ gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions|0x3E|UINT32
+ |0x0001004c
+
 [PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
   ## This PCD defines the Console output row. The default value is 25 
according to UEFI spec.
   #  This PCD could be set to 0 then console output would be at max column and 
max row.
diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf 
b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
index e9cd728dbfb6..0fb2c9cfb52e 100644
--- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
+++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
@@ -2,6 +2,7 @@
 #  ACPI Table Protocol Driver
 #
 #  Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.
+#  Copyright (c) 2016, Linaro Ltd. All rights reserved.
 #  This program and the accompanying materials are  #  licensed and made 
available under the terms and conditions of the BSD License  #  which 
accompanies this distribution.  The full text of the license may be found at @@ 
-67,6 +68,7 @@ [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemRevision  ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorId## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorRevision

[edk2] [patch] MdeModulePkg: Fix Memory Attributes table type issue

[Jiewen Yao]

According to the spec, each entry in the Memory
Attributes table shall have the same type as
the region it was carved out of in the UEFI memory map.
The current attribute uses RTData for PE Data, but
it should be RTCode.

This patch fixed the issue. It is validated with or
without PropertiesTable.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: "Yao, Jiewen" 
Cc: "Ard Biesheuvel" 
Cc: "Gao, Liming" 
---
 MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c | 89 ++--
 1 file changed, 19 insertions(+), 70 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c 
b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
index 6e5ad03..b052148 100644
--- a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
+++ b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
@@ -81,13 +81,10 @@ EFI_PROPERTIES_TABLE  mPropertiesTable = {
 EFI_LOCK   mPropertiesTableLock = EFI_INITIALIZE_LOCK_VARIABLE 
(TPL_NOTIFY);
 
 //
-// Temporary save for original memory map.
 // This is for MemoryAttributesTable only.
 //
 extern BOOLEAN mIsConstructingMemoryAttributesTable;
-EFI_MEMORY_DESCRIPTOR  *mMemoryMapOrg;
-UINTN  mMemoryMapOrgSize;
-UINTN  mDescriptorSize;
+BOOLEANmPropertiesTableEnable;
 
 //
 // Below functions are for MemoryMap
@@ -199,42 +196,6 @@ SortMemoryMap (
 }
 
 /**
-  Check if this memory entry spans across original memory map boundary.
-
-  @param PhysicalStart   The PhysicalStart of memory
-  @param NumberOfPages   The NumberOfPages of memory
-
-  @retval TRUE  This memory entry spans across original memory map boundary.
-  @retval FALSE This memory entry does not span cross original memory map 
boundary.
-**/
-STATIC
-BOOLEAN
-DoesEntrySpanAcrossBoundary (
-  IN UINT64  PhysicalStart,
-  IN UINT64  NumberOfPages
-  )
-{
-  EFI_MEMORY_DESCRIPTOR   *MemoryMapEntry;
-  EFI_MEMORY_DESCRIPTOR   *MemoryMapEnd;
-  UINT64  MemoryBlockLength;
-
-  MemoryMapEntry = mMemoryMapOrg;
-  MemoryMapEnd   = (EFI_MEMORY_DESCRIPTOR *) ((UINT8 *) mMemoryMapOrg + 
mMemoryMapOrgSize);
-  while (MemoryMapEntry < MemoryMapEnd) {
-MemoryBlockLength = (UINT64) (EfiPagesToSize 
(MemoryMapEntry->NumberOfPages));
-
-if ((MemoryMapEntry->PhysicalStart <= PhysicalStart) &&
-(MemoryMapEntry->PhysicalStart + MemoryBlockLength > PhysicalStart) &&
-(MemoryMapEntry->PhysicalStart + MemoryBlockLength < PhysicalStart + 
EfiPagesToSize (NumberOfPages))) {
-  return TRUE;
-}
-
-MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, mDescriptorSize);
-  }
-  return FALSE;
-}
-
-/**
   Merge continous memory map entries whose have same attributes.
 
   @param  MemoryMap  A pointer to the buffer in which firmware 
places
@@ -271,8 +232,7 @@ MergeMemoryMap (
   if (((UINTN)NextMemoryMapEntry < (UINTN)MemoryMapEnd) &&
   (MemoryMapEntry->Type == NextMemoryMapEntry->Type) &&
   (MemoryMapEntry->Attribute == NextMemoryMapEntry->Attribute) &&
-  ((MemoryMapEntry->PhysicalStart + MemoryBlockLength) == 
NextMemoryMapEntry->PhysicalStart) &&
-  (!DoesEntrySpanAcrossBoundary (MemoryMapEntry->PhysicalStart, 
MemoryMapEntry->NumberOfPages + NextMemoryMapEntry->NumberOfPages))) {
+  ((MemoryMapEntry->PhysicalStart + MemoryBlockLength) == 
NextMemoryMapEntry->PhysicalStart)) {
 MemoryMapEntry->NumberOfPages += NextMemoryMapEntry->NumberOfPages;
 if (NewMemoryMapEntry != MemoryMapEntry) {
   NewMemoryMapEntry->NumberOfPages += 
NextMemoryMapEntry->NumberOfPages;
@@ -430,7 +390,11 @@ SetNewRecord (
   //
   // DATA
   //
-  NewRecord->Type = EfiRuntimeServicesData;
+  if (mIsConstructingMemoryAttributesTable && !mPropertiesTableEnable) {
+NewRecord->Type = TempRecord.Type;
+  } else {
+NewRecord->Type = EfiRuntimeServicesData;
+  }
   NewRecord->PhysicalStart = TempRecord.PhysicalStart;
   NewRecord->VirtualStart  = 0;
   NewRecord->NumberOfPages = 
EfiSizeToPages(ImageRecordCodeSection->CodeSegmentBase - 
NewRecord->PhysicalStart);
@@ -443,7 +407,11 @@ SetNewRecord (
   //
   // CODE
   //
-  NewRecord->Type = EfiRuntimeServicesCode;
+  if (mIsConstructingMemoryAttributesTable && !mPropertiesTableEnable) {
+NewRecord->Type = TempRecord.Type;
+  } else {
+NewRecord->Type = EfiRuntimeServicesCode;
+  }
   NewRecord->PhysicalStart = ImageRecordCodeSection->CodeSegmentBase;
   NewRecord->VirtualStart  = 0;
   NewRecord->NumberOfPages = 
EfiSizeToPages(ImageRecordCodeSection->CodeSegmentSize);
@@ -467,7 +435,11 @@ SetNewRecord (
   // Final DATA
   //
   if (TempRecord.PhysicalStart < ImageEnd) {
-NewRecord->Type = EfiRuntimeServicesData;
+if (mIsConstructingMemoryAttributesTable && !mPropertiesTableEnable) {
+  NewRecord->Type = TempRecord.Type;
+} else {
+  NewReco