Date   

Re: [PATCH] ArmPkg: Invalidate Instruction Cache On MMU Enable

Ashish Singhal
 

Hello Ard,

When we had a discussion on this topic earlier, maybe a few weeks back, we thought device memory is being accessed in a speculative manner. Our latest debug where we focussed on MMU page tables at the time of error tells that the issue is actually DRAM mapping in page tables getting corrupt (as per DS-5) where descriptor for a page seems to be something garbage. What this causes is that a valid input address in DRAM may get translated to an address in a different region of DRAM or some address in device memory.

When I invalidate the instruction cache after enabling MMUs, this issue seems to be getting resolved. Again, I am not saying this is a fix but this is something that solves the issue while we are looking for suggestions from you for a proper fix.

I have tried to summarize the problem based on the latest findings a few emails down the trail if you want to have a look at that again.

Thanks
Ashish


From: Ard Biesheuvel <ardb@...>
Sent: Wednesday, February 23, 2022 3:54 PM
To: Ashish Singhal <ashishsingha@...>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Marc Zyngier <maz@...>; Sami Mujawar <sami.mujawar@...>; Ard Biesheuvel <ardb+tianocore@...>; Leif Lindholm <quic_llindhol@...>
Subject: Re: [edk2-devel] [PATCH] ArmPkg: Invalidate Instruction Cache On MMU Enable
 
External email: Use caution opening links or attachments


On Wed, 23 Feb 2022 at 19:14, Ashish Singhal <ashishsingha@...> wrote:
>
> Ard,
>
> During PrePi, I setup the initial memory map by calling into ArmConfigureMmu function with my memory table where device memory regions have attribute of ARM_MEMORY_REGION_ATTRIBUTE_DEVICE and DRAM regions have attribute of ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK.
>
> For device memory, XN bit is set by ArmMemoryAttributeToPageAttribute function. After PrePi, when I add a region of memory to device memory from a DXE driver, I call gDS->AddMemorySpace with EfiGcdMemoryTypeMemoryMappedIo and EFI_MEMORY_UC | EFI_MEMORY_RUNTIME followed by gDS->SetMemorySpaceAttributes with EFI_MEMORY_UC.
>
> Please let me know in case I have still not understood your question.
>

This all looks ok. But the real question is whether the address that
the speculative access targets is mapped using the XN attribute or
not, so you will need to find a way to check that.

So there are a couple of options:
- The XN attribute is set correctly, but the CPU is speculatively
fetching instructions anyway. This would imply a severe hardware bug,
and flushing the I-cache is unlikely to make a difference.
- The speculative access is not the result of an instruction fetch, in
which case I-cache maintenance is unlikely to help either.
- The XN bit is not being set correctly, and so the MMU code needs to be fixed.

Papering over this by adding I-cache maintenance doesn't seem the best
course of action tbh.

--
Ard.


回复: [edk2-devel] [PATCH v2 0/1] MdePkg/Include SMBIOS 3.5.0 changes

gaoliming
 

AbduL:
Sorry for the late response. But, I see your sent patch is empty. Can you
check it?

Please see this link https://edk2.groups.io/g/devel/message/85329

Thanks
Liming

-----邮件原件-----
发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Attar,
AbdulLateef (Abdul Lateef) via groups.io
发送时间: 2022年2月24日 10:02
收件人: devel@edk2.groups.io; Attar, AbdulLateef (Abdul Lateef)
<AbdulLateef.Attar@...>
抄送: Michael D Kinney <michael.d.kinney@...>; Liming Gao
<gaoliming@...>; Zhiguang Liu <zhiguang.liu@...>
主题: Re: [edk2-devel] [PATCH v2 0/1] MdePkg/Include SMBIOS 3.5.0
changes

Hi,
Gentle reminder, Please review the patch.
Thanks
AbduL

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Abdul
Lateef Attar via groups.io
Sent: Friday, January 7, 2022 7:26 PM
To: devel@edk2.groups.io
Cc: Michael D Kinney <michael.d.kinney@...>; Liming Gao
<gaoliming@...>; Zhiguang Liu <zhiguang.liu@...>
Subject: [edk2-devel] [PATCH v2 0/1] MdePkg/Include SMBIOS 3.5.0 changes

[CAUTION: External Email]

Rebase the patch and fix the uncrustify.

Reference:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithu
b.com%2Fabdattar%2Fedk2%2Ftree%2Fsmbios_3_5_0_v2&amp;data=04%7C
01%7CAbdulLateef.Attar%40amd.com%7C4ecac07fa9984365a0dc08d9d1e55
c29%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C63777160590
7726617%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV
2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=83AEIB%2
BmxsZrv9vXDy%2FcEYRQx30r2dZHqlaiFPnAT4I%3D&amp;reserved=0

Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <gaoliming@...>
Cc: Zhiguang Liu <zhiguang.liu@...>

Abdul Lateef Attar (1):
MdePkg/Include: Smbios Specification 3.5.0 changes

MdePkg/Include/IndustryStandard/SmBios.h | 144 +++++++++++++++++++-
1 file changed, 140 insertions(+), 4 deletions(-)

--
2.25.1










Re: [PATCH] MdeModulePkg/Usb/Keyboard.c: Don't request protocol before setting

Wu, Hao A
 

1 inline comment below:

-----Original Message-----
From: Sean Rhodes <sean@...>
Sent: Friday, February 18, 2022 3:30 PM
To: devel@edk2.groups.io
Cc: Dong, Guo <guo.dong@...>; Rhodes, Sean
<sean@...>; Wu, Hao A <hao.a.wu@...>; Ni, Ray
<ray.ni@...>; Matt DeVillier <matt.devillier@...>; Patrick
Rudolph <patrick.rudolph@...>
Subject: [PATCH] MdeModulePkg/Usb/Keyboard.c: Don't request protocol
before setting

No need to check the interface protocol then conditionally setting,
just set it to BOOT_PROTOCOL and check for error.

This is what Linux does for HID devices as some don't follow the USB spec.
One example is the Aspeed BMC HID keyboard device, which adds a massive
boot delay without this patch as it doesn't respond to 'GetProtocolRequest'.

Cc: Hao A Wu <hao.a.wu@...>
Cc: Ray Ni <ray.ni@...>
Signed-off-by: Matt DeVillier <matt.devillier@...>
Signed-off-by: Patrick Rudolph <patrick.rudolph@...>
Signed-off-by: Sean Rhodes <sean@...>
---
MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c | 17 +++++------------
.../Library/BrotliCustomDecompressLib/brotli | 2 +-
2 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
b/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
index 5a94a4dda7..73b5df2b64 100644
--- a/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
+++ b/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
@@ -854,22 +854,15 @@ InitUSBKeyboard (
}

}



- UsbGetProtocolRequest (

- UsbKeyboardDevice->UsbIo,

- UsbKeyboardDevice->InterfaceDescriptor.InterfaceNumber,

- &Protocol

- );

//

// Set boot protocol for the USB Keyboard.

// This driver only supports boot protocol.

//

- if (Protocol != BOOT_PROTOCOL) {

- UsbSetProtocolRequest (

- UsbKeyboardDevice->UsbIo,

- UsbKeyboardDevice->InterfaceDescriptor.InterfaceNumber,

- BOOT_PROTOCOL

- );

- }

+ UsbSetProtocolRequest (

+ UsbKeyboardDevice->UsbIo,

+ UsbKeyboardDevice->InterfaceDescriptor.InterfaceNumber,

+ BOOT_PROTOCOL

+ );



UsbKeyboardDevice->CtrlOn = FALSE;

UsbKeyboardDevice->AltOn = FALSE;

diff --git a/MdeModulePkg/Library/BrotliCustomDecompressLib/brotli
b/MdeModulePkg/Library/BrotliCustomDecompressLib/brotli
index f4153a09f8..666c3280cc 160000
--- a/MdeModulePkg/Library/BrotliCustomDecompressLib/brotli
+++ b/MdeModulePkg/Library/BrotliCustomDecompressLib/brotli
@@ -1 +1 @@
-Subproject commit f4153a09f87cbb9c826d8fc12c74642bb2d879ea
+Subproject commit 666c3280cc11dc433c303d79a83d4ffbdd12cc8d

Why change in BrotliCustomDecompressLib is needed for this patch?

Best Regards,
Hao Wu


--
2.32.0


Re: [PATCH 1/2] MdeModulePkg/SdMmcPciHcDxe: Make timeout for SD card configurable

Wu, Hao A
 

3 inline comments below:

-----Original Message-----
From: Sean Rhodes <sean@...>
Sent: Friday, February 18, 2022 3:24 PM
To: devel@edk2.groups.io
Cc: Dong, Guo <guo.dong@...>; Matt DeVillier
<matt.devillier@...>; Wu, Hao A <hao.a.wu@...>; Ni, Ray
<ray.ni@...>; Wang, Jian J <jian.j.wang@...>; Gao, Liming
<gaoliming@...>; Rhodes, Sean <sean@...>
Subject: [PATCH 1/2] MdeModulePkg/SdMmcPciHcDxe: Make timeout for
SD card configurable

From: Matt DeVillier <matt.devillier@...>

The default 1s timeout can delay boot splash on some hardware with no
benefit.

Cc: Hao A Wu <hao.a.wu@...>
Cc: Ray Ni <ray.ni@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Liming Gao <gaoliming@...>
Signed-off-by: Matt DeVillier <matt.devillier@...>
Signed-off-by: Sean Rhodes <sean@...>
---
MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h | 3 ++-
MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf | 4 ++++
MdeModulePkg/Library/BrotliCustomDecompressLib/brotli | 2 +-
MdeModulePkg/MdeModulePkg.dec | 4 ++++
MdeModulePkg/MdeModulePkg.uni | 4 ++++
5 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
index 85e09cf114..b76c7cffa2 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
@@ -24,6 +24,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Library/MemoryAllocationLib.h>

#include <Library/UefiLib.h>

#include <Library/DevicePathLib.h>

+#include <Library/PcdLib.h>



#include <Protocol/DevicePath.h>

#include <Protocol/PciIo.h>

@@ -49,7 +50,7 @@ extern EDKII_SD_MMC_OVERRIDE *mOverride;
//

// Generic time out value, 1 microsecond as unit.

//

-#define SD_MMC_HC_GENERIC_TIMEOUT 1 * 1000 * 1000

+#define SD_MMC_HC_GENERIC_TIMEOUT (PcdGet32
(PcdSdMmcGenericTimeoutValue))



//

// SD/MMC async transfer timer interval, set by experience.

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
index 453ecde7fd..a9d05736d7 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
@@ -56,6 +56,7 @@
BaseLib

UefiDriverEntryPoint

DebugLib

+ PcdLib



[Protocols]

gEdkiiSdMmcOverrideProtocolGuid ## SOMETIMES_CONSUMES

@@ -68,3 +69,6 @@


[UserExtensions.TianoCore."ExtraFiles"]

SdMmcPciHcDxeExtra.uni

+

+[Pcd.IA32,Pcd.X64]

1. "[Pcd]" should be fine.



+ gEfiMdeModulePkgTokenSpaceGuid.PcdSdMmcGenericTimeoutValue
## CONSUMES

2. Could you please help to run the PatchCheck.py to check your patch before sending out:

py BaseTools/Scripts/PatchCheck.py -1
Checking git commit: HEAD
MdeModulePkg/SdMmcPciHcDxe: Make timeout for SD card configurable
The commit message format passed all checks.
Code format is not valid:
* Tab character used
File: MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf
Line: gEfiMdeModulePkgTokenSpaceGuid.PcdSdMmcGenericTimeoutValue ## CONSUMES



diff --git a/MdeModulePkg/Library/BrotliCustomDecompressLib/brotli
b/MdeModulePkg/Library/BrotliCustomDecompressLib/brotli
index f4153a09f8..666c3280cc 160000
--- a/MdeModulePkg/Library/BrotliCustomDecompressLib/brotli
+++ b/MdeModulePkg/Library/BrotliCustomDecompressLib/brotli
@@ -1 +1 @@
-Subproject commit f4153a09f87cbb9c826d8fc12c74642bb2d879ea
+Subproject commit 666c3280cc11dc433c303d79a83d4ffbdd12cc8d

3. Why change in BrotliCustomDecompressLib is needed for this patch?


Best Regards,
Hao Wu


diff --git a/MdeModulePkg/MdeModulePkg.dec
b/MdeModulePkg/MdeModulePkg.dec
index 463e889e9a..40601c9583 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1559,6 +1559,10 @@
# @Prompt Maximum permitted FwVol section nesting depth (exclusive).


gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth|
0x10|UINT32|0x00000030



+ ## Indicates the default timeout value for SD/MMC Host Controller
operations in microseconds.

+ # @Prompt SD/MMC Host Controller Operations Timeout (us).

+
gEfiMdeModulePkgTokenSpaceGuid.PcdSdMmcGenericTimeoutValue|1000
000|UINT32|0x00000031

+

[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/MdeModulePkg.uni
b/MdeModulePkg/MdeModulePkg.uni
index 27889a7280..b070f15ff2 100644
--- a/MdeModulePkg/MdeModulePkg.uni
+++ b/MdeModulePkg/MdeModulePkg.uni
@@ -1173,6 +1173,10 @@
" TRUE - Capsule In Ram is
supported.<BR>"

" FALSE - Capsule In Ram is not
supported."



+#string
STR_gEfiMdeModulePkgTokenSpaceGuid_PcdSdMmcGenericTimeoutValue
_PROMPT #language en-US "SD/MMC Host Controller Operations Timeout
(us)."

+

+#string
STR_gEfiMdeModulePkgTokenSpaceGuid_PcdSdMmcGenericTimeoutValue
_HELP #language en-US "Indicates the default timeout value for SD/MMC
Host Controller operations in microseconds."

+

#string
STR_gEfiMdeModulePkgTokenSpaceGuid_PcdCodRelocationDevPath_PRO
MPT #language en-US "Capsule On Disk relocation device path."



#string
STR_gEfiMdeModulePkgTokenSpaceGuid_PcdCodRelocationDevPath_HELP
#language en-US "Full device path of platform specific device to store
Capsule On Disk temp relocation file.<BR>"

--
2.32.0


Re: [PATCH] ArmPkg: Invalidate Instruction Cache On MMU Enable

Marc Zyngier <maz@...>
 

On Wed, 23 Feb 2022 07:02:28 +0000,
Ard Biesheuvel <ardb@...> wrote:

(+ Marc)

On Tue, 22 Feb 2022 at 03:42, Ashish Singhal <ashishsingha@...> wrote:

Even with MMU turned off, instruction cache can speculate
and fetch instructions. This can cause a crash if region
being executed has been modified recently. With this patch,
we ensure that instruction cache is invalidated right after
MMU has been enabled and any potentially stale instruction
fetched earlier has been discarded.
Are you doing code patching while the MMU is off?

I don't follow this reasoning. Every time the UEFI image loader loads
an image into memory, it does so with MMU and caches enabled, and the
code regions are cleaned to the PoU before being invalidated in the
I-cache. So how could these regions be stale? The only code that needs
special care here is the little trampoline that executes with the MMU
off, but this is being taken care of explicitly, by cleaning the code
to the PoC before invoking it.

This is specially helpful when the memory attributes of a
region in MMU are being changed and some instructions
operating on the region are prefetched in the instruction
cache.
Huh, this is way too vague. How do you expect anyone to understand
your problem based on this?


This sounds like a TLB problem not a cache problem. For the sake of
posterity, could you include a more detailed description of the issue
in the commit log? IIRC, this is about speculative instruction fetches
hitting device memory locations?

As I mentioned before, the architecture does not permit speculative
instruction fetches for regions mapped with the XN attribute.

Is the issue occurring under virtualization? Is there a stage 2
mapping that lacks the XN attribute for the device memory region in
question?

Signed-off-by: Ashish Singhal <ashishsingha@...>
---
ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 4 +++-
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S | 2 ++
2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
index d3cc1e8671..9648245182 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
@@ -89,7 +89,9 @@ ASM_FUNC(ArmEnableMmu)
dsb nsh
isb
msr sctlr_el3, x0 // Write back
-4: isb
+4: ic iallu
+ dsb sy
Why DSB SY? Given that you are only invalidating on a single CPU,
DSB NSH should be enough.

+ isb
ret


diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
index 66ebca571e..56cc2dd73f 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
@@ -37,6 +37,8 @@

// re-enable the MMU
msr sctlr_el\el, x8
+ ic iallu
+ dsb sy
Same thing here.

M.

--
Without deviation from the norm, progress is not possible.


Re: [PATCH v2 0/1] MdePkg/Include SMBIOS 3.5.0 changes

Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@...>
 

Hi,
Gentle reminder, Please review the patch.
Thanks
AbduL

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Abdul Lateef Attar via groups.io
Sent: Friday, January 7, 2022 7:26 PM
To: devel@edk2.groups.io
Cc: Michael D Kinney <michael.d.kinney@...>; Liming Gao <gaoliming@...>; Zhiguang Liu <zhiguang.liu@...>
Subject: [edk2-devel] [PATCH v2 0/1] MdePkg/Include SMBIOS 3.5.0 changes

[CAUTION: External Email]

Rebase the patch and fix the uncrustify.

Reference: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fabdattar%2Fedk2%2Ftree%2Fsmbios_3_5_0_v2&;data=04%7C01%7CAbdulLateef.Attar%40amd.com%7C4ecac07fa9984365a0dc08d9d1e55c29%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637771605907726617%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=83AEIB%2BmxsZrv9vXDy%2FcEYRQx30r2dZHqlaiFPnAT4I%3D&amp;reserved=0

Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <gaoliming@...>
Cc: Zhiguang Liu <zhiguang.liu@...>

Abdul Lateef Attar (1):
MdePkg/Include: Smbios Specification 3.5.0 changes

MdePkg/Include/IndustryStandard/SmBios.h | 144 +++++++++++++++++++-
1 file changed, 140 insertions(+), 4 deletions(-)

--
2.25.1


Re: [PATCH 2/2] MdePkg/Include: Define new DEBUG_FILE to specify path.

Guomin Jiang
 

Hi Kinney, Liming, Zhiguang,

Can you give comments on this patch

Thanks
Guomin

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Guomin
Jiang
Sent: Friday, February 18, 2022 10:30 AM
To: devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@...>; Gao, Liming
<gaoliming@...>; Liu, Zhiguang <zhiguang.liu@...>
Subject: [edk2-devel] [PATCH 2/2] MdePkg/Include: Define new
DEBUG_FILE to specify path.

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

Use DEBUG_FILE to control ASSERT path

Motivation and Goal:
1. The path will occupy many size in DEBUG build when file path is long 2. We
hope can reduce the size but not impact the debug capability 3. If only use
filename, we can search the filename to locate file. It
can save many size meanwhile.

Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <gaoliming@...>
Cc: Zhiguang Liu <zhiguang.liu@...>
Signed-off-by: Guomin Jiang <guomin.jiang@...>
---
MdePkg/Include/Library/DebugLib.h | 39 ++++++++++++++++++++++-------
--
1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/MdePkg/Include/Library/DebugLib.h
b/MdePkg/Include/Library/DebugLib.h
index 8d3d08638d73..5469c6308422 100644
--- a/MdePkg/Include/Library/DebugLib.h
+++ b/MdePkg/Include/Library/DebugLib.h
@@ -8,7 +8,7 @@
of size reduction when compiler optimization is disabled. If
MDEPKG_NDEBUG is
defined, then debug and assert related macros wrapped by it are the NULL
implementations.

-Copyright (c) 2006 - 2020, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2022, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -85,6 +85,31 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #define
DEBUG_LINE_NUMBER __LINE__ #endif

+//
+// Source file.
+// Default is use the to compiler provided __FILE__ macro value. The
+__FILE__ // mapping can be overriden by predefining DEBUG_FILE // //
+Defining DEBUG_FILE to a fixed value is useful when comparing builds //
+across machine or configuration with different slash or path // file.
+//
+// Another benefit is we can customize the ASSERT path without
+depending on // compiler ability // // It's for all no matter VS, GCC,
+CLANG // #ifdef DEBUG_FILE #else #define DEBUG_FILE __FILE__ #endif
+
+// Blow override for keep clang behavior #if defined (__clang__) &&
+defined (__FILE_NAME__) #undef DEBUG_FILE #define DEBUG_FILE
+__FILE_NAME__ #endif
+
/**
Macro that converts a Boolean expression to a Null-terminated ASCII string.

@@ -337,17 +362,9 @@ UnitTestDebugAssert (
IN CONST CHAR8 *Description
);

- #if defined (__clang__) && defined (__FILE_NAME__) -#define
_ASSERT(Expression) UnitTestDebugAssert (__FILE_NAME__,
DEBUG_LINE_NUMBER, DEBUG_EXPRESSION_STRING (Expression))
- #else
-#define _ASSERT(Expression) UnitTestDebugAssert (__FILE__,
DEBUG_LINE_NUMBER, DEBUG_EXPRESSION_STRING (Expression))
- #endif
+#define _ASSERT(Expression) UnitTestDebugAssert (DEBUG_FILE,
+DEBUG_LINE_NUMBER, DEBUG_EXPRESSION_STRING (Expression))
#else
- #if defined (__clang__) && defined (__FILE_NAME__) -#define
_ASSERT(Expression) DebugAssert (__FILE_NAME__,
DEBUG_LINE_NUMBER, DEBUG_EXPRESSION_STRING (Expression))
- #else
-#define _ASSERT(Expression) DebugAssert (__FILE__,
DEBUG_LINE_NUMBER, DEBUG_EXPRESSION_STRING (Expression))
- #endif
+#define _ASSERT(Expression) DebugAssert (DEBUG_FILE,
+DEBUG_LINE_NUMBER, DEBUG_EXPRESSION_STRING (Expression))
#endif

/**
--
2.35.1.windows.2





Re: [PATCH V3 8/8] OvmfPkg: Introduce IntelTdxX64 for TDVF Config-B

Min Xu
 

Hi

+ #
+ # Defines for default states. These can be changed on the command line.
+ # -D FLAG=VALUE
+ #
+ DEFINE SECURE_BOOT_ENABLE = FALSE
Will TDX support secure boot? If not then this config switch can simply be
dropped (and the checks for SECURE_BOOT_ENABLE too).
Secure boot is supported by TDVF.

Thanks
Min


Re: [PATCH V3 7/8] OvmfPkg: Update DxeAcpiTimerLib to read HostBridgeDevId in PlatformInfoHob

Min Xu
 

On February 23, 2022 7:04 PM, Gerd Hoffmann wrote:

//
// Query Host Bridge DID to determine platform type
+ // Tdx guest stores the HostBridgePciDevId in a GuidHob.
+ // So we first check if this HOB exists
//
- HostBridgeDevId = PcdGet16 (PcdOvmfHostBridgePciDevId);
+ GuidHob = GetFirstGuidHob (&gUefiOvmfPkgTdxPlatformGuid); if
+ (GuidHob != NULL) {
+ PlatformInfo = (EFI_HOB_PLATFORM_INFO *)GET_GUID_HOB_DATA
(GuidHob);
+ HostBridgeDevId = PlatformInfo->HostBridgePciDevId; } else {
+ HostBridgeDevId = PcdGet16 (PcdOvmfHostBridgePciDevId); }
Another thing we can probably simplify when we use a PlatformInfoHob in
both PEi and PEI-less mode.
Agree.

+[Guids]
+ gUefiOvmfPkgTdxPlatformGuid ## CONSUMES
name this UefiOvmfPkgPlatformInfoGuid ?
Sure. Thanks for the suggestion. It will be renamed in the next version.

Thanks
Min


回复: [edk2-devel] Hard Feature Freeze starts now for edk2-stable202202

gaoliming
 

Hi, all

 We will create edk2-stable202202 tag on 2022-02-25 (tomorrow). If you have any patch to catch this stable tag, please raise your request in this mail thread today.

 

Thanks

Liming

发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 gaoliming
发送时间: 2022211 17:58
收件人: devel@edk2.groups.io; announce@edk2.groups.io
抄送: 'Michael D Kinney' <michael.d.kinney@...>; 'Andrew Fish' <afish@...>; quic_llindhol@...; 'Demeter, Miki' <miki.demeter@...>
主题: [edk2-devel] Hard Feature Freeze starts now for edk2-stable202202

 

Hi, all

  Today, we enter into Hard Feature Freeze phase until edk2-stable202202 tag is created at 2022-02-25. Tag edk2-stable202202-rc1 (c9b7c6e0cc7da76b74bcdd8c90cef956d5ae971c) has been created for evaluation. In this phase, there is no feature to be pushed. The critical bug fix is still allowed.

 

  If the patch is sent after Hard Feature Freeze, and plans to catch this stable tag, please add edk2-stable202202 key words in the patch title and BZ, and also cc to Tianocore Stewards, then Stewards can give the comments.

 

Below is edk2-stable202202 tag planning (https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning)

 

Date (00:00:00 UTC-8) Description

2021-11-26  Beginning of development

2022-02-07  Soft Feature Freeze

2022-02-11  Hard Feature Freeze

2022-02-25  Release

 

Thanks

Liming

 


Re: [PATCH V3 4/8] OvmfPkg: Add TdxStartupLib

Min Xu
 

On February 23, 2022 6:50 PM, Gerd Hoffmann wrote:

+EFI_STATUS
+EFIAPI
+InitializePlatform (
+ EFI_HOB_PLATFORM_INFO *PlatformInfoHob
+ )
+{
+ Pci64Base = 0;
+ Pci64Size = 0;
+
+ FirstNonAddress = PlatformGetFirstNonAddress (&Pci64Base,
&Pci64Size, 0x800000000);
+ PlatformInfoHob->PcdPciMmio64Base = Pci64Base;
+ PlatformInfoHob->PcdPciMmio64Size = Pci64Size;
I think here are opportunities to make the differences between PEI and PEI-
less boot even smaller, by:

(1) Allocate a PlatformInfoHob also when using PEI boot workflow.
(2) Switch PlatformInitLib functions like PlatformGetFirstNonAddress() to
receive a PlatformInfoHob pointer so they can update the HOB
directly.
(3) Add more platform info variables to PlatformInfoHob
(FirstNonAddress, PhysMemAddressWidth for example).

But I guess that kind of improvements can also be done incrementally after
getting this upstream. It's also easier to test that kind of changes when we
have both PEI and PEI-less variants present in the upstream repo. So I'm fine
with deferring these changes for now.
Agree. Thanks for your understanding.


Beside that: The name TdxStartupLib doesn't reflect reality any more, we
should give it a better name.
How about PeilessStartupLib?

Thanks
Min


Re: [PATCH 1/2] MdeModulePkg/GraphicsConsoleDxe: Check status to make sure no error

Guomin Jiang
 

Hi Liming, Jian, Zhichao, Ray,

Can you help review it?

Thanks
Guomin

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Guomin
Jiang
Sent: Tuesday, February 22, 2022 11:41 AM
To: devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@...>; Gao, Liming
<gaoliming@...>; Gao, Zhichao <zhichao.gao@...>; Ni,
Ray <ray.ni@...>
Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg/GraphicsConsoleDxe:
Check status to make sure no error

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

SetMode will fail in some case. for example, without XServer.
Should handle these case when SetMode fail.

If we don't handle it, it will Segmentation fault.

Cc: Jian J Wang <jian.j.wang@...>
Cc: Liming Gao <gaoliming@...>
Cc: Zhichao Gao <zhichao.gao@...>
Cc: Ray Ni <ray.ni@...>
Signed-off-by: Guomin Jiang <guomin.jiang@...>
---
.../Console/GraphicsConsoleDxe/GraphicsConsole.c | 15 ++-------------
1 file changed, 2 insertions(+), 13 deletions(-)

diff --git
a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole
.c
b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsol
e.c
index 1bdd1b8a6732..07436cbd15bf 100644
---
a/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsole
.c
+++
b/MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsol
e.c
@@ -1,7 +1,7 @@
/** @file
This is the main routine for initializing the Graphics Console support routines.

-Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2022, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -518,7 +518,7 @@ GraphicsConsoleControllerDriverStart (
}
}

- if (ModeNumber != Private->GraphicsOutput->Mode->Mode) {
+ if (EFI_ERROR (Status) || (ModeNumber != Private->GraphicsOutput-
Mode->Mode)) {
//
// Current graphics mode is not set or is not set to the mode which we
have found,
// set the new graphic mode.
@@ -531,17 +531,6 @@ GraphicsConsoleControllerDriverStart (
goto Error;
}
}
-
- //
- // Double confirm SetMode can success
- //
- Status = Private->GraphicsOutput->SetMode (Private->GraphicsOutput,
ModeNumber);
- if (EFI_ERROR (Status)) {
- //
- // The mode set operation failed
- //
- goto Error;
- }
} else if (FeaturePcdGet (PcdUgaConsumeSupport)) {
//
// At first try to set user-defined resolution
--
2.35.1.windows.2





Re: [PATCH 2/2] EmulatorPkg/EmuGopDxe: Set ModeInfo after Open successfully

Guomin Jiang
 

Hi Fish and Ray,

Can you help review it?

Thanks
Guomin

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Guomin
Jiang
Sent: Tuesday, February 22, 2022 11:41 AM
To: devel@edk2.groups.io
Cc: Andrew Fish <afish@...>; Ni, Ray <ray.ni@...>
Subject: [edk2-devel] [PATCH 2/2] EmulatorPkg/EmuGopDxe: Set ModeInfo
after Open successfully

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

WindowOpen will fail in some case. for example, without XServer.

Shouldn't set ModeInfo in this case to avoid the caller use it incorrectly

Cc: Andrew Fish <afish@...>
Cc: Ray Ni <ray.ni@...>
Signed-off-by: Guomin Jiang <guomin.jiang@...>
---
EmulatorPkg/EmuGopDxe/GopScreen.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/EmulatorPkg/EmuGopDxe/GopScreen.c
b/EmulatorPkg/EmuGopDxe/GopScreen.c
index 41f748bc6402..ec5ef795d6e5 100644
--- a/EmulatorPkg/EmuGopDxe/GopScreen.c
+++ b/EmulatorPkg/EmuGopDxe/GopScreen.c
@@ -108,10 +108,6 @@ EmuGopSetMode (
}

ModeData = &Private->ModeData[ModeNumber];
- This->Mode->Mode = ModeNumber;
- Private->GraphicsOutput.Mode->Info->HorizontalResolution = ModeData-
HorizontalResolution;
- Private->GraphicsOutput.Mode->Info->VerticalResolution = ModeData-
VerticalResolution;
- Private->GraphicsOutput.Mode->Info->PixelsPerScanLine = ModeData-
HorizontalResolution;
if (Private->HardwareNeedsStarting) {
Status = EmuGopStartWindow (
@@ -128,6 +124,11 @@ EmuGopSetMode (
Private->HardwareNeedsStarting = FALSE;
}

+ This->Mode->Mode = ModeNumber;
+ Private->GraphicsOutput.Mode->Info->HorizontalResolution = ModeData-
HorizontalResolution;
+ Private->GraphicsOutput.Mode->Info->VerticalResolution = ModeData-
VerticalResolution;
+ Private->GraphicsOutput.Mode->Info->PixelsPerScanLine = ModeData-
HorizontalResolution;
+
Status = Private->EmuGraphicsWindow->Size (
Private->EmuGraphicsWindow,
ModeData->HorizontalResolution,
--
2.35.1.windows.2





Re: [PATCH V6 34/42] OvmfPkg: Update PlatformPei to support Tdx guest

Min Xu
 

OnFebruary 23, 2022 6:14 PM, Gerd Hoffmann wrote:
- Pml4Entries = 1 << (mPhysMemAddressWidth - 39);
+ if (TdIsEnabled ()) {
+ Pml4Entries = 0x200;
+ } else {
+ Pml4Entries = 1 << (mPhysMemAddressWidth - 39);
+ }
With the PlatformAddressWidthInitialization() update in patch #33 it should
not be needed to special-case TDX here. You might need a check for 5-level
paging support (mPhysMemAddressWidth > 48) though, or just cap
Pml4Entries at 512 entries.
As we agree in current stage 5-level paging is not supported, I will cap Pml4Entries at 512 entries when mPhysMemAddressWidth > 48.

Thanks
Min


Re: [PATCH edk2-platforms 1/1] Maintainers.txt: update email for Leif Lindholm

Rebecca Cran
 

Reviewed-by: Rebecca Cran <quic_rcran@...>

On 2/7/22 05:21, Leif Lindholm wrote:
NUVIA inc. was acquired by Qualcomm in March 2021, but we continued
contributions under the existing IDs until the start of this year.
We are now switching to use Qualcomm Innovation Center email, so
update Maintainers.txt to reflect this.

Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Leif Lindholm <quic_llindhol@...>
Signed-off-by: Leif Lindholm <leif@...>
---
Maintainers.txt | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/Maintainers.txt b/Maintainers.txt
index 8dcc34ed0c2e..da6abc7344c2 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -68,7 +68,7 @@ F: */
EDK II Platforms maintainers
----------------------------
F: *
-M: Leif Lindholm <leif@...>
+M: Leif Lindholm <quic_llindhol@...>
M: Michael D Kinney <michael.d.kinney@...>
Responsible Disclosure, Reporting Security Issues
@@ -81,7 +81,7 @@ EDK II Platforms Packages:
96Boards
F: Platform/96Boards/
M: Ard Biesheuvel <ardb+tianocore@...>
-M: Leif Lindholm <leif@...>
+M: Leif Lindholm <quic_llindhol@...>
AMD Seattle
F: Platform/AMD/OverdriveBoard/
@@ -89,7 +89,7 @@ F: Platform/LeMaker/CelloBoard/
F: Platform/SoftIron/
F: Silicon/AMD/Styx/
M: Ard Biesheuvel <ardb+tianocore@...>
-M: Leif Lindholm <leif@...>
+M: Leif Lindholm <quic_llindhol@...>
Ampere Computing
F: Platform/Ampere
@@ -98,7 +98,7 @@ R: Nhi Pham <nhi@...>
R: Vu Nguyen <vunguyen@...>
R: Thang Nguyen <thang@...>
R: Chuong Tran <chuong@...>
-M: Leif Lindholm <leif@...>
+M: Leif Lindholm <quic_llindhol@...>
ARM
F: Platform/ARM/
@@ -111,12 +111,12 @@ BeagleBoard:
F: Platform/BeagleBoard/
F: Silicon/TexasInstruments/
R: Ard Biesheuvel <ardb+tianocore@...>
-M: Leif Lindholm <leif@...>
+M: Leif Lindholm <quic_llindhol@...>
Comcast
F: Platform/Comcast/
M: Ard Biesheuvel <ardb+tianocore@...>
-M: Leif Lindholm <leif@...>
+M: Leif Lindholm <quic_llindhol@...>
OptionRomPkg
F: Drivers/OptionRomPkg/
@@ -130,14 +130,14 @@ M: Ilias Apalodimas <ilias.apalodimas@...>
DisplayLink
F: Drivers/DisplayLink/
-M: Leif Lindholm <leif@...>
+M: Leif Lindholm <quic_llindhol@...>
M: Ard Biesheuvel <ardb+tianocore@...>
R: Andy Hayes <andy.hayes@...>
HiSilicon
F: Platform/Hisilicon/
F: Silicon/Hisilicon/
-M: Leif Lindholm <leif@...>
+M: Leif Lindholm <quic_llindhol@...>
R: Ard Biesheuvel <ardb+tianocore@...>
R: Wenyi Xie <xiewenyi2@...>
@@ -320,26 +320,26 @@ F: Platform/Marvell/
F: Platform/SolidRun/Armada80x0McBin/
F: Silicon/Marvell/
R: Marcin Wojtas <mw@...>
-M: Leif Lindholm <leif@...>
+M: Leif Lindholm <quic_llindhol@...>
Miscellaneous drivers
F: Silicon/Atmel/
F: Silicon/Openmoko/
F: Silicon/Synopsys/DesignWare/
R: Ard Biesheuvel <ardb+tianocore@...>
-M: Leif Lindholm <leif@...>
+M: Leif Lindholm <quic_llindhol@...>
NXP platforms and silicon
F: Platform/NXP/
F: Silicon/NXP/
-M: Leif Lindholm <leif@...>
+M: Leif Lindholm <quic_llindhol@...>
R: Meenakshi Aggarwal <meenakshi.aggarwal@...>
QEMU sbsa-ref platform
F: Platform/Qemu/SbsaQemu/
F: Silicon/Qemu/SbsaQemu/
M: Ard Biesheuvel <ardb+tianocore@...>
-M: Leif Lindholm <leif@...>
+M: Leif Lindholm <quic_llindhol@...>
R: Graeme Gregory <graeme@...>
R: Radoslaw Biernacki <rad@...>
@@ -347,7 +347,7 @@ Raspberry Pi platforms and silicon
F: Platform/RaspberryPi/
F: Silicon/Broadcom/
M: Ard Biesheuvel <ardb+tianocore@...>
-M: Leif Lindholm <leif@...>
+M: Leif Lindholm <quic_llindhol@...>
R: Jeremy Linton <jeremy.linton@...>
RPMB driver for OP-TEE
@@ -360,7 +360,7 @@ F: Platform/Socionext/
F: Silicon/NXP/Library/Pcf8563RealTimeClockLib/
F: Silicon/Socionext/
M: Ard Biesheuvel <ardb+tianocore@...>
-M: Leif Lindholm <leif@...>
+M: Leif Lindholm <quic_llindhol@...>
R: Masami Hiramatsu <masami.hiramatsu@...>
Silicon/RISC-V/ProcessorPkg
@@ -378,7 +378,7 @@ R: Daniel Schaefer <daniel.schaefer@...>
Phytium platforms and silicon
F: Platform/Phytium/
F: Silicon/silicon/
-M: Leif Lindholm <leif@...>
+M: Leif Lindholm <quic_llindhol@...>
R: Peng Xie <xiepeng@...>
R: Ling Jia <jialing@...>
R: Yiqi Shu <shuyiqi@...>


[PATCH] Maintainers.txt: Add new reviewer for UefiPayloadPkg

Guo Dong
 

From: Guo Dong <guo.dong@...>

Add Sean Rhodes as UefiPayload reviewer mainly focus on
UEFI payload for coreboot support.

Signed-off-by: Guo Dong <guo.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Benjamin You <benjamin.you@...>
Cc: Maurice Ma <maurice.ma@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Sean Rhodes <sean@...>
---
Maintainers.txt | 1 +
1 file changed, 1 insertion(+)

diff --git a/Maintainers.txt b/Maintainers.txt
index 924069187e..d26f91c02e 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -601,6 +601,7 @@ M: Guo Dong <guo.dong@...> [gdong1]
M: Ray Ni <ray.ni@...> [niruiyu]=0D
R: Maurice Ma <maurice.ma@...> [mauricema]=0D
R: Benjamin You <benjamin.you@...> [BenjaminYou]=0D
+R: Sean Rhodes <sean@...>=0D
S: Maintained=0D
=0D
UnitTestFrameworkPkg=0D
--=20
2.35.1.windows.2


Re: [PATCH 2/2] UefiPayloadPkg: Hook up RNG support

Ma, Maurice <maurice.ma@...>
 

Reviewed-by: Maurice Ma <maurice.ma@...>

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean
Rhodes
Sent: Wednesday, February 23, 2022 14:44
To: devel@edk2.groups.io
Cc: Dong, Guo <guo.dong@...>; Patrick Rudolph
<patrick.rudolph@...>; Yao, Jiewen <jiewen.yao@...>;
Wang, Jian J <jian.j.wang@...>; Ni, Ray <ray.ni@...>; Ma,
Maurice <maurice.ma@...>; You, Benjamin
<benjamin.you@...>
Subject: [edk2-devel] [PATCH 2/2] UefiPayloadPkg: Hook up RNG support

From: Patrick Rudolph <patrick.rudolph@...>

Hoop Up RNG from SecurityPkg.

Cc: Jiewen Yao <jiewen.yao@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Guo Dong <guo.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Maurice Ma <maurice.ma@...>
Cc: Benjamin You <benjamin.you@...>
Signed-off-by: Patrick Rudolph <patrick.rudolph@...>
---
UefiPayloadPkg/UefiPayloadPkg.dsc | 8 ++++++++
UefiPayloadPkg/UefiPayloadPkg.fdf | 4 ++++
2 files changed, 12 insertions(+)

diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc
b/UefiPayloadPkg/UefiPayloadPkg.dsc
index 1ce96a51c1..0d4b4da24f 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
@@ -703,6 +703,14 @@

MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.i
nf !endif + #+ # Random Number Generator+ #+
SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf {+
<LibraryClasses>+
RngLib|SecurityPkg/Library/BaseRngLib/BaseRngLib.inf+ }+ #------------------
------------ # Build the shell #------------------------------diff --git
a/UefiPayloadPkg/UefiPayloadPkg.fdf b/UefiPayloadPkg/UefiPayloadPkg.fdf
index c7b04978ad..6af1a8c8aa 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.fdf
+++ b/UefiPayloadPkg/UefiPayloadPkg.fdf
@@ -229,6 +229,10 @@ INF
MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouseDxe.inf
# INF MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf +#
Random Number Generator+#+INF
SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf+ # # UEFI
network modules #--
2.32.0



-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#86923): https://edk2.groups.io/g/devel/message/86923
Mute This Topic: https://groups.io/mt/89353219/1773972
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [maurice.ma@...]
-=-=-=-=-=-=


Unit Test and sanitizers

Andrew Fish
 

Just throwing out an idea for the edk2 unit tests. At least for clang you can turn on the sanitizer via a simple command line flag to the compiler. So seems it would make sense to turn on it for unit tests? I’m not sure if the Linux clang, and maybe even some versions of gcc support this too?  Not clear how it works on VC++ or other compilers.

Here is a stupid example from the Xcode clang on macOS of what I’m talking about. In the 1st case the write to NULL crashes the test app with a seg fault. With the sanitizers the buffer overflow and UB  is detected. So the sanitizer gives you better test coverage for free and makes it much easier to root cause the failure.

~/work/Compiler/sanitize>cat t.c                                                          
int
main(int argc, char **argv)
{
  char test[1] = { 0 };
  char *ptr = &test[0];

#ifndef SKIP_OVERFLOW
  ptr += 2;
  *ptr = 1;
#endif

  

  ptr = (char *)0;
  *ptr = 2;
  return 0;
}
~/work/Compiler/sanitize>clang -g t.c && ./a.out                                          
zsh: segmentation fault  ./a.out
~/work/Compiler/sanitize>clang -g -fsanitize=address -fsanitize=undefined  t.c && ./a.out                 
=================================================================
==5302==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ff7b066f7c2 at pc 0x00010f893db0 bp 0x7ff7b066f790 sp 0x7ff7b066f788
WRITE of size 1 at 0x7ff7b066f7c2 thread T0
    #0 0x10f893daf in main t.c:9
    #1 0x1129754fd in start dyldMain.cpp:879

Address 0x7ff7b066f7c2 is located in stack of thread T0 at offset 34 in frame
    #0 0x10f893baf in main t.c:3

  This frame has 1 object(s):
    [32, 33) 'test' (line 4) <== Memory access at offset 34 overflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow t.c:9 in main
Shadow bytes around the buggy address:
  0x1ffef60cdea0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1ffef60cdeb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1ffef60cdec0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1ffef60cded0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1ffef60cdee0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x1ffef60cdef0: 00 00 00 00 f1 f1 f1 f1[01]f3 f3 f3 00 00 00 00
  0x1ffef60cdf00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1ffef60cdf10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1ffef60cdf20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1ffef60cdf30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1ffef60cdf40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==5302==ABORTING
zsh: abort      ./a.out
~/work/Compiler/sanitize>clang -g -fsanitize=address -fsanitize=undefined -DSKIP_OVERFLOW  t.c && ./a.out 
t.c:13:3: runtime error: store to null pointer of type 'char'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior t.c:13:3 in 
AddressSanitizer:DEADLYSIGNAL
=================================================================
==5312==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x0001020cde3f bp 0x7ff7bde358c0 sp 0x7ff7bde357c0 T0)
==5312==The signal is caused by a WRITE memory access.
==5312==Hint: address points to the zero page.
    #0 0x1020cde3f in main t.c:13
    #1 0x108d414fd in start dyldMain.cpp:879

==5312==Register values:
rax = 0x0000000000000000  rbx = 0x00007ff7bde35800  rcx = 0x00007ff7bde357c0  rdx = 0x00001ffef7bc6af8  
rdi = 0x00007ff7bde352f1  rsi = 0x0000000000000000  rbp = 0x00007ff7bde358c0  rsp = 0x00007ff7bde357c0  
 r8 = 0x0000000102580480   r9 = 0x00007ff7bde34a90  r10 = 0x0000000000000000  r11 = 0x0000000000000206  
r12 = 0x0000000108db43a0  r13 = 0x00007ff7bde35978  r14 = 0x00000001020cdc80  r15 = 0x0000000108da8010  
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV t.c:13 in main
==5312==ABORTING
zsh: abort      ./a.out

Thanks,

Andrew Fish


[`edk2-devel][PATCH] Maintainers.txt: Add new reviewer for UefiPayloadPkg

Guo Dong
 

From: Guo Dong <guo.dong@...>

Add Sean Rhodes as UefiPayload reviewer mainly focus on
UEFI payload for coreboot support.

Signed-off-by: Guo Dong <guo.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Benjamin You <benjamin.you@...>
Cc: Maurice Ma <maurice.ma@...>
Cc: Michael D Kinney <michael.d.kinney@...>
---
Maintainers.txt | 1 +
1 file changed, 1 insertion(+)

diff --git a/Maintainers.txt b/Maintainers.txt
index 924069187e..d26f91c02e 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -601,6 +601,7 @@ M: Guo Dong <guo.dong@...> [gdong1]
M: Ray Ni <ray.ni@...> [niruiyu]=0D
R: Maurice Ma <maurice.ma@...> [mauricema]=0D
R: Benjamin You <benjamin.you@...> [BenjaminYou]=0D
+R: Sean Rhodes <sean@...>=0D
S: Maintained=0D
=0D
UnitTestFrameworkPkg=0D
--=20
2.35.1.windows.2


Re: [PATCH] UefiPayloadPkg: Add build option for Above 4G Memory

Guo Dong
 

Reviewed-by: Guo Dong <guo.dong@...>

-----Original Message-----
From: Sean Rhodes <sean@...>
Sent: Wednesday, February 23, 2022 4:00 PM
To: devel@edk2.groups.io
Cc: Dong, Guo <guo.dong@...>; Rhodes, Sean <sean@...>; Ni, Ray <ray.ni@...>; Ma, Maurice <maurice.ma@...>; You, Benjamin <benjamin.you@...>
Subject: [PATCH] UefiPayloadPkg: Add build option for Above 4G Memory

When build option ABOVE_4G_MEMORY is set to true, nothing will change and EDKII will use all available memory.

Setting it to false will create memory type information HOB in payload entry, so that EDKII will reserve enough memory below 4G for EDKII modules. This option is useful for bootloaders that are not fully 64-bit aware such as Qubes R4.0.4 bootloader, Zorin and Proxmox.

Cc: Guo Dong <guo.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Maurice Ma <maurice.ma@...>
Cc: Benjamin You <benjamin.you@...>
Signed-off-by: Sean Rhodes <sean@...>
---
.../UefiPayloadEntry/UefiPayloadEntry.c | 39 +++++++++++++++++++
.../UefiPayloadEntry/UefiPayloadEntry.inf | 7 ++++
UefiPayloadPkg/UefiPayloadPkg.dec | 3 ++
UefiPayloadPkg/UefiPayloadPkg.dsc | 3 ++
4 files changed, 52 insertions(+)

diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
index 0fed1e3691..780348eadf 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
+++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
@@ -5,10 +5,44 @@
**/ +#include <Guid/MemoryTypeInformation.h> #include "UefiPayloadEntry.h" STATIC UINT32 mTopOfLowerUsableDram = 0; +EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = {+ { EfiACPIReclaimMemory, FixedPcdGet32 (PcdMemoryTypeEfiACPIReclaimMemory) },+ { EfiACPIMemoryNVS, FixedPcdGet32 (PcdMemoryTypeEfiACPIMemoryNVS) },+ { EfiReservedMemoryType, FixedPcdGet32 (PcdMemoryTypeEfiReservedMemoryType) },+ { EfiRuntimeServicesData, FixedPcdGet32 (PcdMemoryTypeEfiRuntimeServicesData) },+ { EfiRuntimeServicesCode, FixedPcdGet32 (PcdMemoryTypeEfiRuntimeServicesCode) },+ { EfiMaxMemoryType, 0 }+};++/**+ Function to reserve memory below 4GB for EDKII Modules.++ This causes the DXE to dispatch everything under 4GB and allows Operating+ System's that require EFI_LOADED_IMAGE to be under 4GB to start.+ e.g. Xen hypervisor used in Qubes.+**/+VOID+ForceModulesBelow4G (+ VOID+ )+{+ DEBUG ((DEBUG_INFO, "Building hob to restrict memory resorces to below 4G.\n"));++ //+ // Create Memory Type Information HOB+ //+ BuildGuidDataHob (+ &gEfiMemoryTypeInformationGuid,+ mDefaultMemoryTypeInformation,+ sizeof (mDefaultMemoryTypeInformation)+ );+}+ /** Callback function to build resource descriptor HOB @@ -438,6 +472,11 @@ _ModuleEntryPoint (
// Build other HOBs required by DXE BuildGenericHob (); + // Create a HOB to make resources for EDKII modules below 4G+ if (!FixedPcdGetBool (PcdDispatchModuleAbove4GMemory)) {+ ForceModulesBelow4G ();+ }+ // Load the DXE Core Status = LoadDxeCore (&DxeCoreEntryPoint); ASSERT_EFI_ERROR (Status);diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf
index 1847d6481a..c4e4339ede 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf
+++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf
@@ -86,8 +86,15 @@
gUefiPayloadPkgTokenSpaceGuid.PcdPayloadFdMemSize gUefiPayloadPkgTokenSpaceGuid.PcdBootloaderParameter gUefiPayloadPkgTokenSpaceGuid.PcdSystemMemoryUefiRegionSize+ gUefiPayloadPkgTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS+ gUefiPayloadPkgTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory+ gUefiPayloadPkgTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType+ gUefiPayloadPkgTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData+ gUefiPayloadPkgTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ## SOMETIMES_CONSUMES gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ## SOMETIMES_CONSUMES gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy ## SOMETIMES_CONSUMES + gUefiPayloadPkgTokenSpaceGuid.PcdDispatchModuleAbove4GMemory+diff --git a/UefiPayloadPkg/UefiPayloadPkg.dec b/UefiPayloadPkg/UefiPayloadPkg.dec
index 551f0a4915..e9204d1168 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.dec
+++ b/UefiPayloadPkg/UefiPayloadPkg.dec
@@ -83,6 +83,9 @@ gUefiPayloadPkgTokenSpaceGuid.PcdSystemMemoryUefiRegionSize|0x04000000|UINT32|0x
gUefiPayloadPkgTokenSpaceGuid.PcdPcdDriverFile|{ 0x57, 0x72, 0xcf, 0x80, 0xab, 0x87, 0xf9, 0x47, 0xa3, 0xfe, 0xD5, 0x0B, 0x76, 0xd8, 0x95, 0x41 }|VOID*|0x00000018 +# Above 4G Memory+gUefiPayloadPkgTokenSpaceGuid.PcdDispatchModuleAbove4GMemory|TRUE|BOOLEAN|0x00000019+ ## FFS filename to find the default variable initial data file. # @Prompt FFS Name of variable initial data file gUefiPayloadPkgTokenSpaceGuid.PcdNvsDataFile |{ 0x1a, 0xf1, 0xb1, 0xae, 0x42, 0xcc, 0xcf, 0x4e, 0xac, 0x60, 0xdb, 0xab, 0xf6, 0xca, 0x69, 0xe6 }|VOID*|0x00000025diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc b/UefiPayloadPkg/UefiPayloadPkg.dsc
index 1ce96a51c1..4fe81a61d6 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
@@ -33,6 +33,7 @@
DEFINE UNIVERSAL_PAYLOAD = FALSE DEFINE SECURITY_STUB_ENABLE = TRUE DEFINE SMM_SUPPORT = FALSE+ DEFINE ABOVE_4G_MEMORY = TRUE # # SBL: UEFI payload for Slim Bootloader # COREBOOT: UEFI payload for coreboot@@ -399,6 +400,8 @@
gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask | 0x1 !endif + gUefiPayloadPkgTokenSpaceGuid.PcdDispatchModuleAbove4GMemory|$(ABOVE_4G_MEMORY)+ [PcdsPatchableInModule.X64] gPcAtChipsetPkgTokenSpaceGuid.PcdRtcIndexRegister|$(RTC_INDEX_REGISTER) gPcAtChipsetPkgTokenSpaceGuid.PcdRtcTargetRegister|$(RTC_TARGET_REGISTER)--
2.32.0

4061 - 4080 of 90921