Date   

Re: [PATCH 5/5] [edk2-platforms] Platform/DeveloperBox: Expand NvStorage sizes

Leif Lindholm
 

On Fri, Nov 05, 2021 at 18:24:01 +0900, Masami Hiramatsu wrote:
Expand NvStorage Variable size and FTW spare/working size
for the DeveloperBox platform.

Since the size of the NvStorage VariableSize is not enough
large, FWTS uefirttime test, which updates the NV
variables in runtime, failes. This expands the size to fix
this issue.
Does this change erase all existing variables?

If so, I think it is worth introducing this as a non-default build
option, in order to not wreck existing installations on a firmware
update.

I think it would also be worth considering whether to update
PcdLowestSupportedFirmwareVersion. PcdFirmwareRevision should
definitely be updated.

/
Leif

Signed-off-by: Masami Hiramatsu <masami.hiramatsu@...>
Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@...>
---
.../Socionext/DeveloperBox/DeveloperBox.dsc.inc | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
index 0a364bc457..3baf97ecc0 100644
--- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
+++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc.inc
@@ -280,11 +280,11 @@
gFip006DxeTokenSpaceGuid.PcdFip006DxeMemBaseAddress|0x08000000

gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0x08400000
- gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00010000
- gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x08410000
- gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00010000
- gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x08420000
- gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00010000
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00080000
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x08480000
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00080000
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase|0x08500000
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00080000

gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemId|"SNI "
gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemTableId|0x52434155514e5953 # SYNQUACR


Re: [PATCH 4/5] [edk2-platforms] Silicon/SynQuacer: add DBG2 ACPI table

Leif Lindholm
 

On Fri, Nov 05, 2021 at 18:23:53 +0900, Masami Hiramatsu wrote:
Add DBG2 table to ACPI tables. The COM1 uart port will be used
for OS debug, and it is 16550 compatible.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu@...>
---
.../Socionext/SynQuacer/AcpiTables/AcpiTables.inf | 1
Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc | 70 ++++++++++++++++++++
2 files changed, 71 insertions(+)
create mode 100644 Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc

diff --git a/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf b/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
index 886777a0fa..3023206330 100644
--- a/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
+++ b/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
@@ -22,6 +22,7 @@
Dsdt.asl
Fadt.aslc
Gtdt.aslc
+ Dbg2.aslc
Please move this before Dsdt.asl, to keep the list alphabetically sorted.

Iort.aslc
Madt.aslc
Mcfg.aslc
diff --git a/Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc b/Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc
new file mode 100644
index 0000000000..027b3b658b
--- /dev/null
+++ b/Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc
@@ -0,0 +1,70 @@
+/** @file
+* Debug Port Table (DBG2)
+*
+* Copyright (c) 2020,2021 Linaro Ltd. All rights reserved.
+*
+* SPDX-License-Identifier: BSD-2-Clause-Patent
+*
+**/
+#include <IndustryStandard/Acpi.h>
+#include <IndustryStandard/DebugPort2Table.h>
+#include <Library/AcpiLib.h>
+#include <Library/PcdLib.h>
+#include <Platform/MemoryMap.h>
+
+#include "AcpiTables.h"
+
+#pragma pack(1)
+
+#define SYNQUACER_UART1_STR { '\\', '_', 'S', 'B', '.', 'C', 'O', 'M', '1', 0x00 }
+#define SQ_GAS32(Address) { EFI_ACPI_5_0_SYSTEM_MEMORY, 32, 0, EFI_ACPI_5_0_BYTE, Address }
Use EFI_ACPI_6_3_ consistently?

/
Leif

+
+typedef struct {
+ EFI_ACPI_DBG2_DEBUG_DEVICE_INFORMATION_STRUCT Dbg2Device;
+ EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE BaseAddressRegister;
+ UINT32 AddressSize;
+ UINT8 NameSpaceString[10];
+} DBG2_DEBUG_DEVICE_INFORMATION;
+
+typedef struct {
+ EFI_ACPI_DEBUG_PORT_2_DESCRIPTION_TABLE Description;
+ DBG2_DEBUG_DEVICE_INFORMATION Dbg2DeviceInfo;
+} DBG2_TABLE;
+
+
+STATIC DBG2_TABLE Dbg2 = {
+ {
+ __ACPI_HEADER (
+ EFI_ACPI_6_3_DEBUG_PORT_2_TABLE_SIGNATURE,
+ DBG2_TABLE,
+ EFI_ACPI_DBG2_DEBUG_DEVICE_INFORMATION_STRUCT_REVISION
+ ),
+ OFFSET_OF (DBG2_TABLE, Dbg2DeviceInfo),
+ 1 /* NumberOfDebugPorts */
+ },
+ {
+ {
+ EFI_ACPI_DBG2_DEBUG_DEVICE_INFORMATION_STRUCT_REVISION,
+ sizeof (DBG2_DEBUG_DEVICE_INFORMATION),
+ 1, /* NumberofGenericAddressRegisters */
+ 10, /* NameSpaceStringLength */
+ OFFSET_OF (DBG2_DEBUG_DEVICE_INFORMATION, NameSpaceString),
+ 0, /* OemDataLength */
+ 0, /* OemDataOffset */
+ EFI_ACPI_DBG2_PORT_TYPE_SERIAL,
+ EFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_16550_WITH_GAS,
+ {EFI_ACPI_RESERVED_BYTE, EFI_ACPI_RESERVED_BYTE},
+ OFFSET_OF (DBG2_DEBUG_DEVICE_INFORMATION, BaseAddressRegister),
+ OFFSET_OF (DBG2_DEBUG_DEVICE_INFORMATION, AddressSize)
+ },
+ SQ_GAS32 (SYNQUACER_UART1_BASE), /* BaseAddressRegister */
+ SYNQUACER_UART1_SIZE, /* AddressSize */
+ SYNQUACER_UART1_STR, /* NameSpaceString */
+ }
+};
+
+#pragma pack()
+
+// Reference the table being generated to prevent the optimizer from removing
+// the data structure from the executable
+VOID* CONST ReferenceAcpiTable = &Dbg2;


Re: [PATCH 3/5] [edk2-platforms] Silicon/SynQuacerPlatformFlashAccessLib: Fix the number of erase blocks

Leif Lindholm
 

On Fri, Nov 05, 2021 at 18:23:45 +0900, Masami Hiramatsu wrote:
Fix the number of erase blocks by rounding up the result.
The erase blocks must include the last block covered by the
length bytes.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu@...>
Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@...>
Reviewed-by: Leif Lindholm <leif@...>

---
.../SynQuacerPlatformFlashAccessLib.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c b/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c
index bded74dc4f..ad4021cf59 100644
--- a/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c
+++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c
@@ -283,7 +283,7 @@ PerformFlashWriteWithProgress (
DEBUG ((DEBUG_INFO, "%a: erasing 0x%llx bytes at address %llx (LBA 0x%lx)\n",
__FUNCTION__, Length, FlashAddress, Lba));

- Status = Fvb->EraseBlocks (Fvb, Lba, Length / BlockSize,
+ Status = Fvb->EraseBlocks (Fvb, Lba, (Length + BlockSize - 1) / BlockSize,
EFI_LBA_LIST_TERMINATOR);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "%a: Fvb->EraseBlocks () failed - %r\n",


Re: [PATCH 2/5] [edk2-platforms] Silicon/Socionext/SynQuacer: Fix GenericWatchdog interrupt number

Leif Lindholm
 

On Fri, Nov 05, 2021 at 18:23:36 +0900, Masami Hiramatsu wrote:
This fixes Socionext DeveloperBox GenericWatchdog interrupt
number to 93 instead of 94. Since the 93 is the default interrupt
number defined in ArmPkg/ArmPkg.dec, this doesn't redefine
gArmTokenSpaceGuid.PcdGenericWatchdogEl2IntrNum.
That is one thing this patch does.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu@...>
Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@...>
---
.../Socionext/SynQuacer/AcpiTables/AcpiTables.inf | 1 +
Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc | 6 +++---
2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf b/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
index 96efb2d38e..886777a0fa 100644
--- a/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
+++ b/Silicon/Socionext/SynQuacer/AcpiTables/AcpiTables.inf
@@ -50,6 +50,7 @@

gArmTokenSpaceGuid.PcdGenericWatchdogControlBase
gArmTokenSpaceGuid.PcdGenericWatchdogRefreshBase
+ gArmTokenSpaceGuid.PcdGenericWatchdogEl2IntrNum

gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemRevision
gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorRevision
diff --git a/Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc b/Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc
index c811fc5a0c..b045a49efa 100644
--- a/Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc
+++ b/Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc
@@ -74,9 +74,9 @@ EFI_ACPI_6_0_GENERIC_TIMER_DESCRIPTION_TABLES Gtdt = {
// UINT32 GTxCommonFlags
},
EFI_ACPI_6_0_SBSA_GENERIC_WATCHDOG_STRUCTURE_INIT (
- FixedPcdGet32 (PcdGenericWatchdogRefreshBase),
- FixedPcdGet32 (PcdGenericWatchdogControlBase),
- 94,
+ FixedPcdGet64 (PcdGenericWatchdogRefreshBase),
+ FixedPcdGet64 (PcdGenericWatchdogControlBase),
But it also changes these two FixedPcdGet32 calls to FixedPcdGet64.
That should be a separate patch.

/
Leif

+ FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
0),
};


Re: [PATCH 1/5] [RESEND][edk2-platforms] Silicon/SynQuacerI2cDxe: Wait for bus busy

Leif Lindholm
 

On Fri, Nov 05, 2021 at 18:23:28 +0900, Masami Hiramatsu wrote:
If an EFI application frequently repeats SetTime and GetTime,
the I2C bus can be busy and failed to start. To fix this issue,
add waiting loop for the bus busy status. (Usually, it is
enough to read 3 times for checking, but for safety this
sets 10 for timeout.)

This also clean up the code path a bit so that it is easy to
understand what should do on each combinations of BSR.BB and
BCR.MSS.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu@...>
Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@...>
---
.../Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c | 38 ++++++++++++++------
1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
index 31f6e3072f..380eba8059 100644
--- a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
+++ b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
@@ -16,6 +16,8 @@
//
#define WAIT_FOR_INTERRUPT_TIMEOUT 50000

+#define WAIT_FOR_BUS_BUSY_TIMEOUT 10
+
I think it would be more clear English to say that we are waiting
_for_ the bus to be *ready* - meaning that we are waiting _while_ the
bus is *busy*.

So I suggest
WAIT_FOR_BUS_BUSY_TIMEOUT ->
WAIT_FOR_BUS_READY_TIMEOUT

/**
Set the frequency for the I2C clock line.

@@ -152,6 +154,7 @@ SynQuacerI2cMasterStart (
IN EFI_I2C_OPERATION *Op
)
{
+ UINTN Timeout = WAIT_FOR_BUS_BUSY_TIMEOUT;
This indentation does not match the subsequent lines.

/
Leif

UINT8 Bsr;
UINT8 Bcr;

@@ -167,24 +170,35 @@ SynQuacerI2cMasterStart (
Bsr = MmioRead8 (I2c->MmioBase + F_I2C_REG_BSR);
Bcr = MmioRead8 (I2c->MmioBase + F_I2C_REG_BCR);

- if ((Bsr & F_I2C_BSR_BB) && !(Bcr & F_I2C_BCR_MSS)) {
- DEBUG ((DEBUG_INFO, "%a: bus is busy\n", __FUNCTION__));
- return EFI_ALREADY_STARTED;
- }
+ if (!(Bcr & F_I2C_BCR_MSS)) {

- if (Bsr & F_I2C_BSR_BB) { // Bus is busy
- DEBUG ((DEBUG_INFO, "%a: Continuous Start\n", __FUNCTION__));
- MmioWrite8 (I2c->MmioBase + F_I2C_REG_BCR, Bcr | F_I2C_BCR_SCC);
- } else {
- if (Bcr & F_I2C_BCR_MSS) {
- DEBUG ((DEBUG_WARN,
- "%a: is not in master mode\n", __FUNCTION__));
- return EFI_DEVICE_ERROR;
+ if (Bsr & F_I2C_BSR_BB) { // Bus is busy
+ do {
+ Bsr = MmioRead8 (I2c->MmioBase + F_I2C_REG_BSR);
+ } while (Timeout-- && (Bsr & F_I2C_BSR_BB));
+
+ if (Bsr & F_I2C_BSR_BB) {
+ DEBUG ((DEBUG_INFO, "%a: bus is busy\n", __FUNCTION__));
+ return EFI_ALREADY_STARTED;
+ }
}
+
DEBUG ((DEBUG_INFO, "%a: Start Condition\n", __FUNCTION__));
MmioWrite8 (I2c->MmioBase + F_I2C_REG_BCR,
Bcr | F_I2C_BCR_MSS | F_I2C_BCR_INTE | F_I2C_BCR_BEIE);
+
+ } else { // F_I2C_BCR_MSS is set
+
+ if (!(Bsr & F_I2C_BSR_BB)) {
+ DEBUG ((DEBUG_WARN,
+ "%a: is not in master mode\n", __FUNCTION__));
+ return EFI_DEVICE_ERROR;
+ }
+
+ DEBUG ((DEBUG_INFO, "%a: Continuous Start\n", __FUNCTION__));
+ MmioWrite8 (I2c->MmioBase + F_I2C_REG_BCR, Bcr | F_I2C_BCR_SCC);
}
+
return EFI_SUCCESS;
}


Re: [PATCH edk2-platforms 0/2] Socionext housekeeping

Leif Lindholm
 

On Fri, Nov 26, 2021 at 09:56:29 +0900, Masami Hiramatsu wrote:
Hi Leif,

Sorry for replying late.

2021年11月11日(木) 20:59 Leif Lindholm <leif@...>:

Masami - while looking at your set from last Friday, patch 1/5 fails
to apply since the file being modified has the wrong line endings in
the tree.
Oh, OK. is it CR/LF?


2/2 fixes that.
(Obviously, the patch looks nonsensical since SMTP strips the added CR
back out.)
Thanks for fixing!


1/2 adds you as a reviewer to the Socionext platforms. (This means
that you will be cc:d on any future patches to these, and that I
can use your Reviewed-by for 2/2.)

Does this sound good to you?
Yes, these look good to me.

Acked-by: Masami Hiramatsu <masami.hiramatsu@...>

for the series.
Thank you - pushed as:
7f86d9002533..3e73df157306

/
Leif

Thank you!


Leif Lindholm (2):
Maintainers.txt: add Masami as Socionext reviewer
Silicon/Socionext: fix line endings

Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c | 1162 ++++++++++----------
Maintainers.txt | 1 +
2 files changed, 582 insertions(+), 581 deletions(-)

--
2.30.2

--
Masami Hiramatsu


Re: [Patch V2 0/3] Remove git reset and optimize

Sean
 

Acked-by: Sean Brogan <sean.brogan@...>

On 11/23/2021 8:30 AM, Michael D Kinney wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2986
New in V2
----------
* Change temp directory path from Build/ecctemp to
Build/.pytool/Plugin/EccCheck to provide a unique
temp directory location for any .pytool Plugin.
* Set working directory when ECC runs to temp directory
to guarantee all temp files created by EccCheck are
cleaned up.
* Use temp directory for all operations to prevent any
changed to git state.
* Remove git reset operation that could corrupt staged
and local changes.
* Improve performance by removing redundant directory scans
* Improve performance and reduce log file sizes by using
--output option of git diff to a temp file instead of
using stdout.
Cc: Sean Brogan <sean.brogan@...>
Cc: Bret Barkelew <Bret.Barkelew@...>
Cc: Liming Gao <gaoliming@...>
Cc: Michael Kubacki <michael.kubacki@...>
Signed-off-by: Michael D Kinney <michael.d.kinney@...>
Michael D Kinney (3):
.pytool/Plugin/EccCheck: Remove RevertCode()
.pytool/Plugin/EccCheck: Remove temp directory on exception
.pytool/Plugin/EccCheck: Add performance optimizations
.pytool/Plugin/EccCheck/EccCheck.py | 242 +++++++++++++++++++---------
1 file changed, 169 insertions(+), 73 deletions(-)


Re: [Patch V2 1/1] .pytools/Plugin/LicenseCheck: Use temp directory for git diff output

Sean
 

Reviewed-by: Sean Brogan <sean.brogan@...>

On 11/23/2021 9:31 AM, Michael D Kinney wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3746
Use --output option in git diff command to remove code diffs
from build log on stdout when LicenseCheck plugin is run.
Instead, create a temp directory for the diff output file and
remove the temp directory after the diff output is processed.
Cc: Sean Brogan <sean.brogan@...>
Cc: Bret Barkelew <Bret.Barkelew@...>
Cc: Liming Gao <gaoliming@...>
Cc: Michael Kubacki <michael.kubacki@...>
Signed-off-by: Michael D Kinney <michael.d.kinney@...>
---
.pytool/Plugin/LicenseCheck/LicenseCheck.py | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/.pytool/Plugin/LicenseCheck/LicenseCheck.py b/.pytool/Plugin/LicenseCheck/LicenseCheck.py
index 5733f7bf4ec0..7b998daf6f6b 100644
--- a/.pytool/Plugin/LicenseCheck/LicenseCheck.py
+++ b/.pytool/Plugin/LicenseCheck/LicenseCheck.py
@@ -5,6 +5,7 @@
##
import os
+import shutil
import logging
import re
from io import StringIO
@@ -61,12 +62,19 @@ class LicenseCheck(ICiBuildPlugin):
# - Junit Logger
# - output_stream the StringIO output stream from this plugin via logging
def RunBuildPlugin(self, packagename, Edk2pathObj, pkgconfig, environment, PLM, PLMHelper, tc, output_stream=None):
- return_buffer = StringIO()
- params = "diff --unified=0 origin/master HEAD"
- RunCmd("git", params, outstream=return_buffer)
- p = return_buffer.getvalue().strip()
- patch = p.split("\n")
- return_buffer.close()
+ # Create temp directory
+ temp_path = os.path.join(Edk2pathObj.WorkspacePath, 'Build', '.pytool', 'Plugin', 'LicenseCheck')
+ if not os.path.exists(temp_path):
+ os.makedirs(temp_path)
+ # Output file to use for git diff operations
+ temp_diff_output = os.path.join (temp_path, 'diff.txt')
+ params = "diff --output={} --unified=0 origin/master HEAD".format(temp_diff_output)
+ RunCmd("git", params)
+ with open(temp_diff_output) as file:
+ patch = file.read().strip().split("\n")
+ # Delete temp directory
+ if os.path.exists(temp_path):
+ shutil.rmtree(temp_path)
ignore_files = []
if "IgnoreFiles" in pkgconfig:


Re: [Patch 1/1] .azurepipelines/templates: Update max pipeline job time to 2 hours

Sean
 

Reviewed-by: Sean Brogan <sean.brogan@...>

On 11/23/2021 4:44 PM, Michael D Kinney wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3750
Large patches that modify a large number of files(e.g uncrustify)
take longer to process through CI checks such as ECC. Increase
the max job time from 1 hour to 2 hours to accommodate larger patch
series.
Cc: Sean Brogan <sean.brogan@...>
Cc: Bret Barkelew <Bret.Barkelew@...>
Cc: Liming Gao <gaoliming@...>
Cc: Michael Kubacki <michael.kubacki@...>
Signed-off-by: Michael D Kinney <michael.d.kinney@...>
---
.azurepipelines/templates/pr-gate-build-job.yml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/.azurepipelines/templates/pr-gate-build-job.yml b/.azurepipelines/templates/pr-gate-build-job.yml
index d5b16c127f58..244cffdbfaba 100644
--- a/.azurepipelines/templates/pr-gate-build-job.yml
+++ b/.azurepipelines/templates/pr-gate-build-job.yml
@@ -17,7 +17,7 @@ parameters:
jobs:
- job: Build_${{ parameters.tool_chain_tag }}
-
+ timeoutInMinutes: 120
#Use matrix to speed up the build process
strategy:
matrix:


Re: [PATCH v13 00/32] Add AMD Secure Nested Paging (SEV-SNP) support

Brijesh Singh
 

Hi Ray,

Can you please ack the remaining patches so that it can be merged?

thanks

On 11/12/21 11:39 AM, Brijesh Singh wrote:
---------------------------------------------------------------------------
Hi Ray,

Thanks for your reviews and continuous support; I have updated a couple of patches
to address your comment. As I said in my previous reply, I will working on a
follow-up series to group some of those Sev specific variables in CpuData.

I hope that is okay with you.

thanks
----------------------------------------------------------------------------

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

SEV-SNP builds upon existing SEV and SEV-ES functionality while adding
new hardware-based memory protections. SEV-SNP adds strong memory integrity
protection to help prevent malicious hypervisor-based attacks like data
replay, memory re-mapping and more in order to create an isolated memory
encryption environment.

This series provides the basic building blocks to support booting the SEV-SNP
VMs, it does not cover all the security enhancement introduced by the SEV-SNP
such as interrupt protection.

Many of the integrity guarantees of SEV-SNP are enforced through a new
structure called the Reverse Map Table (RMP). Adding a new page to SEV-SNP
VM requires a 2-step process. First, the hypervisor assigns a page to the
guest using the new RMPUPDATE instruction. This transitions the page to
guest-invalid. Second, the guest validates the page using the new PVALIDATE
instruction. The SEV-SNP VMs can use the new "Page State Change Request NAE"
defined in the GHCB specification to ask hypervisor to add or remove page
from the RMP table.

Each page assigned to the SEV-SNP VM can either be validated or unvalidated,
as indicated by the Validated flag in the page's RMP entry. There are two
approaches that can be taken for the page validation: Pre-validation and
Lazy Validation.

Under pre-validation, the pages are validated prior to first use. And under
lazy validation, pages are validated when first accessed. An access to a
unvalidated page results in a #VC exception, at which time the exception
handler may validate the page. Lazy validation requires careful tracking of
the validated pages to avoid validating the same GPA more than once. The
recently introduced "Unaccepted" memory type can be used to communicate the
unvalidated memory ranges to the Guest OS.

At this time we only support the pre-validation. OVMF detects all the available
system RAM in the PEI phase. When SEV-SNP is enabled, the memory is validated
before it is made available to the EDK2 core.

Now that series contains all the basic support required to launch SEV-SNP
guest. We are still missing the Interrupt security feature provided by the
SNP. The feature will be added after the base support is accepted.

Additional resources
---------------------
SEV-SNP whitepaper
https://www.amd.com/system/files/TechDocs/SEV-SNP-strengthening-vm-isolation-with-integrity-protection-and-more.pdf

APM 2: https://www.amd.com/system/files/TechDocs/24593.pdf (section 15.36)

The complete source is available at
https://github.com/AMDESE/ovmf/tree/snp-v13

GHCB spec:
https://developer.amd.com/wp-content/resources/56421.pdf

SEV-SNP firmware specification:
https://www.amd.com/system/files/TechDocs/56860.pdf

Change since v12:
* MpLib: Add comment to clarify that SEV-SNP enabled implicitly means SEV and SEV-ES are active.
* MpLib: Move the extended topology initialization in AmdSev.c

Change since v11:
* rebase to the latest
* fix the UefiCpuPkg PCD definition patch header.

Change since v10:
* fix 'unresolved external symbol __allshl' link error when building I32 for VS2017.

Changes since v9:
* Move CCAttrs Pcd define in MdePkg
* Add comment to indicate that allocating the identity map PT is temporary until we get lazy validation

Changes since v8:
* drop the generic metadata and make it specific to SEV.

Changes since v7:
* Move SEV specific changes in MpLib in AmdSev file
* Update the GHCB register function to not restore the GHCB MSR because
we were already in the MSR protocol mode.
* Drop the SNP name from PcdSnpSecPreValidate.
* Add new section for GHCB memory in the OVMF metadata.

Change since v6:
* Drop the SNP boot block GUID and switch to using the Metadata guided structure
proposed by Min in TDX series.
* Exclude the GHCB page from the pre-validated region. It simplifies the reset
vector code where we do not need to unvalidate the GHCB page.
* Now that GHCB page is not validated so move the VMPL check from reset vector
code to the MemEncryptSevLib on the first page validation.
* Introduce the ConfidentialComputingGuestAttr PCD to communicate which
memory encryption is active so that MpInitLib can make use of it.
* Drop the SEVES specific PCD as the information can be communicated via
the ConfidentialComputingGuestAttr.
* Move the SNP specific AP creation function in AmdSev.c.
* Define the SNP Blob GUID in a new file.

Change since v5:
* When possible use the CPUID value from CPUID page
* Move the SEV specific functions from SecMain.c in AmdSev.c
* Rebase to the latest code
* Add the review feedback from Yao.

Change since v4:
* Use the correct MSR for the SEV_STATUS
* Add VMPL-0 check

Change since v3:
* ResetVector: move all SEV specific code in AmdSev.asm and add macros to keep
the code readable.
* Drop extending the EsWorkArea to contain SNP specific state.
* Drop the GhcbGpa library and call the VmgExit directly to register GHCB GPA.
* Install the CC blob config table from AmdSevDxe instead of extending the
AmdSev/SecretsDxe for it.
* Add the separate PCDs for the SNP Secrets.

Changes since v2:
* Add support for the AP creation.
* Use the module-scoping override to make AmdSevDxe use the IO port for PCI reads.
* Use the reserved memory type for CPUID and Secrets page.
*
Changes since v1:
* Drop the interval tree support to detect the pre-validated overlap region.
* Use an array to keep track of pre-validated regions.
* Add support to query the Hypervisor feature and verify that SNP feature is supported.
* Introduce MemEncryptSevClearMmioPageEncMask() to clear the C-bit from MMIO ranges.
* Pull the SevSecretDxe and SevSecretPei into OVMF package build.
* Extend the SevSecretDxe to expose confidential computing blob location through
EFI configuration table.

Brijesh Singh (28):
OvmfPkg/SecMain: move SEV specific routines in AmdSev.c
UefiCpuPkg/MpInitLib: move SEV specific routines in AmdSev.c
OvmfPkg/ResetVector: move clearing GHCB in SecMain
OvmfPkg/ResetVector: introduce SEV metadata descriptor for VMM use
OvmfPkg: reserve SNP secrets page
OvmfPkg: reserve CPUID page
OvmfPkg/ResetVector: pre-validate the data pages used in SEC phase
OvmfPkg/MemEncryptSevLib: add MemEncryptSevSnpEnabled()
OvmfPkg/SecMain: register GHCB gpa for the SEV-SNP guest
OvmfPkg/PlatformPei: register GHCB gpa for the SEV-SNP guest
OvmfPkg/AmdSevDxe: do not use extended PCI config space
OvmfPkg/MemEncryptSevLib: add support to validate system RAM
OvmfPkg/MemEncryptSevLib: add function to check the VMPL0
OvmfPkg/BaseMemEncryptSevLib: skip the pre-validated system RAM
OvmfPkg/MemEncryptSevLib: add support to validate > 4GB memory in PEI
phase
OvmfPkg/SecMain: validate the memory used for decompressing Fv
OvmfPkg/PlatformPei: validate the system RAM when SNP is active
MdePkg: Define ConfidentialComputingGuestAttr
OvmfPkg/PlatformPei: set PcdConfidentialComputingAttr when SEV is
active
UefiCpuPkg/MpInitLib: use PcdConfidentialComputingAttr to check SEV
status
UefiCpuPkg: add PcdGhcbHypervisorFeatures
OvmfPkg/PlatformPei: set the Hypervisor Features PCD
MdePkg/GHCB: increase the GHCB protocol max version
UefiCpuPkg/MpLib: add support to register GHCB GPA when SEV-SNP is
enabled
OvmfPkg/MemEncryptSevLib: change the page state in the RMP table
OvmfPkg/MemEncryptSevLib: skip page state change for Mmio address
OvmfPkg/PlatformPei: mark cpuid and secrets memory reserved in EFI map
OvmfPkg/AmdSev: expose the SNP reserved pages through configuration
table

Michael Roth (3):
OvmfPkg/ResetVector: use SEV-SNP-validated CPUID values
OvmfPkg/VmgExitLib: use SEV-SNP-validated CPUID values
UefiCpuPkg/MpInitLib: use BSP to do extended topology check

Tom Lendacky (1):
UefiCpuPkg/MpInitLib: Use SEV-SNP AP Creation NAE event to launch APs

MdePkg/MdePkg.dec | 4 +
OvmfPkg/OvmfPkg.dec | 19 +
UefiCpuPkg/UefiCpuPkg.dec | 5 +
OvmfPkg/AmdSev/AmdSevX64.dsc | 8 +-
OvmfPkg/Bhyve/BhyveX64.dsc | 5 +-
OvmfPkg/OvmfPkgIa32.dsc | 4 +
OvmfPkg/OvmfPkgIa32X64.dsc | 9 +-
OvmfPkg/OvmfPkgX64.dsc | 8 +-
OvmfPkg/OvmfXen.dsc | 5 +-
OvmfPkg/OvmfPkgX64.fdf | 6 +
OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 7 +
.../DxeMemEncryptSevLib.inf | 3 +
.../PeiMemEncryptSevLib.inf | 7 +
.../SecMemEncryptSevLib.inf | 3 +
OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf | 2 +
OvmfPkg/Library/VmgExitLib/VmgExitLib.inf | 3 +
OvmfPkg/PlatformPei/PlatformPei.inf | 7 +
OvmfPkg/ResetVector/ResetVector.inf | 5 +
OvmfPkg/Sec/SecMain.inf | 4 +
UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 6 +-
UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf | 6 +-
.../Include/ConfidentialComputingGuestAttr.h | 25 +
MdePkg/Include/Register/Amd/Ghcb.h | 2 +-
.../Guid/ConfidentialComputingSevSnpBlob.h | 33 ++
OvmfPkg/Include/Library/MemEncryptSevLib.h | 26 +
.../X64/SnpPageStateChange.h | 36 ++
.../BaseMemEncryptSevLib/X64/VirtualMemory.h | 24 +
OvmfPkg/PlatformPei/Platform.h | 5 +
OvmfPkg/Sec/AmdSev.h | 95 ++++
UefiCpuPkg/Library/MpInitLib/MpLib.h | 103 ++++
OvmfPkg/AmdSevDxe/AmdSevDxe.c | 23 +
.../DxeMemEncryptSevLibInternal.c | 27 ++
.../Ia32/MemEncryptSevLib.c | 17 +
.../PeiMemEncryptSevLibInternal.c | 27 ++
.../SecMemEncryptSevLibInternal.c | 19 +
.../X64/DxeSnpSystemRamValidate.c | 40 ++
.../X64/PeiDxeVirtualMemory.c | 167 ++++++-
.../X64/PeiSnpSystemRamValidate.c | 127 +++++
.../X64/SecSnpSystemRamValidate.c | 82 ++++
.../X64/SnpPageStateChangeInternal.c | 294 ++++++++++++
OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 444 ++++++++++++++++--
OvmfPkg/PlatformPei/AmdSev.c | 231 +++++++++
OvmfPkg/PlatformPei/MemDetect.c | 2 +
OvmfPkg/Sec/AmdSev.c | 298 ++++++++++++
OvmfPkg/Sec/SecMain.c | 158 +------
UefiCpuPkg/Library/MpInitLib/AmdSev.c | 260 ++++++++++
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 16 +-
UefiCpuPkg/Library/MpInitLib/Ia32/AmdSev.c | 70 +++
UefiCpuPkg/Library/MpInitLib/MpLib.c | 347 +++++---------
UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 4 +-
UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c | 261 ++++++++++
OvmfPkg/FvmainCompactScratchEnd.fdf.inc | 5 +
OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 14 +
OvmfPkg/ResetVector/Ia32/AmdSev.asm | 86 +++-
OvmfPkg/ResetVector/ResetVector.nasmb | 18 +
OvmfPkg/ResetVector/X64/OvmfSevMetadata.asm | 74 +++
UefiCpuPkg/Library/MpInitLib/MpEqu.inc | 2 +
UefiCpuPkg/Library/MpInitLib/X64/AmdSev.nasm | 200 ++++++++
UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 100 +---
59 files changed, 3360 insertions(+), 528 deletions(-)
create mode 100644 MdePkg/Include/ConfidentialComputingGuestAttr.h
create mode 100644 OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateChange.h
create mode 100644 OvmfPkg/Sec/AmdSev.h
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiSnpSystemRamValidate.c
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecSnpSystemRamValidate.c
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateChangeInternal.c
create mode 100644 OvmfPkg/Sec/AmdSev.c
create mode 100644 UefiCpuPkg/Library/MpInitLib/AmdSev.c
create mode 100644 UefiCpuPkg/Library/MpInitLib/Ia32/AmdSev.c
create mode 100644 UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c
create mode 100644 OvmfPkg/ResetVector/X64/OvmfSevMetadata.asm
create mode 100644 UefiCpuPkg/Library/MpInitLib/X64/AmdSev.nasm


EDK II Stable Tag release edk2-stable202111 completed and Hard Freeze extend to 2021-12-03

gaoliming
 

Hi, all

 

The tag edk2-stable202111 has been created. https://github.com/tianocore/edk2/releases/tag/edk2-stable202111

  git clone -b edk2-stable202111 https://github.com/tianocore/edk2.git

 

The tag edk2-stable202111 has been added into the main EDK II Wiki page.

  https://github.com/tianocore/tianocore.github.io/wiki/EDK-II

 

Next edk2 stable tag (edk2-stable202202) planning has been added into wiki page.

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning.

 

If you have ideas for features in the next stable tag, please enter a Bugzilla for evaluation. Please let me know if there are existing open Bugzilla entries that should be targeted at this next stable tag.

 

  But now, we are still in hard freeze period until Uncrustify patches are merged. The plan is to extend one week hard freeze to 2021-12-03. Thank you for your cooperation and patience.

 

Thanks

Liming


回复: 回复: [edk2-devel] [Patch edk2-stable202111] NetworkPkg: Fix invalid pointer for DNS response token on error

gaoliming
 

Leif:
Yes. Both commit message and BZ are updated.
https://github.com/tianocore/edk2/pull/2222 is created to merge this patch.

Thanks
Liming

-----邮件原件-----
发件人: Leif Lindholm <leif@...>
发送时间: 2021年11月25日 18:55
收件人: devel@edk2.groups.io; gaoliming@...
抄送: jiaxin.wu@...; 'Anbazhagan, Baraneedharan'
<anbazhagan@...>; maciej.rabeda@...; 'Fu, Siyuan'
<siyuan.fu@...>; 'Andrew Fish' <afish@...>; 'Michael D Kinney'
<michael.d.kinney@...>
主题: Re: 回复: [edk2-devel] [Patch edk2-stable202111] NetworkPkg: Fix
invalid pointer for DNS response token on error

Agreed, but the commit message (and the BZ) should state which commit
introduced this issue.

/
Leif

On Thu, Nov 25, 2021 at 09:52:58 +0800, gaoliming wrote:
I suggest to merge this fix for the stable tag 202111. This fix is low risk.




Thanks

Liming

发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Wu, Jiaxin
发送时间: 2021年11月25日 9:37
收件人: Anbazhagan, Baraneedharan <anbazhagan@...>;
devel@edk2.groups.io
抄送: maciej.rabeda@...; Fu, Siyuan <siyuan.fu@...>
主题: Re: [edk2-devel] NetworkPkg: Fix invalid pointer for DNS response
token on error



Reviewed-by: Wu Jiaxin <jiaxin.wu@...
<mailto:jiaxin.wu@...> >







From: Anbazhagan, Baraneedharan <anbazhagan@...
<mailto:anbazhagan@...> >
Sent: Thursday, November 25, 2021 9:28 AM
To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>
Cc: maciej.rabeda@...
<mailto:maciej.rabeda@...> ;
Wu, Jiaxin <jiaxin.wu@... <mailto:jiaxin.wu@...> >; Fu, Siyuan
<siyuan.fu@... <mailto:siyuan.fu@...> >
Subject: RE: NetworkPkg: Fix invalid pointer for DNS response token on
error



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



Token->RspData.H2AData is de-allocated on error but it is not

set to NULL. HTTP module attempts to free again and cause assert.



Signed-off-by: Baraneedharan Anbazhagan anbazhagan@...
<mailto:anbazhagan@...>

---

NetworkPkg/DnsDxe/DnsImpl.c | 2 ++

1 file changed, 2 insertions(+)



diff --git a/NetworkPkg/DnsDxe/DnsImpl.c
b/NetworkPkg/DnsDxe/DnsImpl.c

index 2edcb280ac..78a56f2b56 100644

--- a/NetworkPkg/DnsDxe/DnsImpl.c

+++ b/NetworkPkg/DnsDxe/DnsImpl.c

@@ -1700,6 +1700,7 @@ ON_EXIT:

}



FreePool (Dns4TokenEntry->Token->RspData.H2AData);

+ Dns4TokenEntry->Token->RspData.H2AData = NULL;

}

}

}

@@ -1731,6 +1732,7 @@ ON_EXIT:

}



FreePool (Dns6TokenEntry->Token->RspData.H2AData);

+ Dns6TokenEntry->Token->RspData.H2AData = NULL;

}

}

}

--

2.33.0.windows.2










Re: [PATCH V3 15/29] OvmfPkg: Update SecEntry.nasm to support Tdx

Yao, Jiewen
 

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
Hoffmann
Sent: Thursday, November 25, 2021 4:32 PM
To: Yao, Jiewen <jiewen.yao@...>
Cc: devel@edk2.groups.io; jejb@...; Xu, Min M
<min.m.xu@...>; Ard Biesheuvel <ardb+tianocore@...>; Justen,
Jordan L <jordan.l.justen@...>; Brijesh Singh <brijesh.singh@...>;
Erdem Aktas <erdemaktas@...>; Tom Lendacky
<thomas.lendacky@...>
Subject: Re: [edk2-devel] [PATCH V3 15/29] OvmfPkg: Update SecEntry.nasm to
support Tdx

Hi,

So, my judgement is by removing PEI, we can reduce the risk introduce
by PEI Core + PEI Arch PEIM*. Reducing code == Reducing Security Risk.
Yes, PEI Core goes away.

No, PEI Arch PEIM (aka OvmfPkg/PlatformPei) wouldn't go away, you would
only move the code to SEC or DXE phase, the platform initialization has
to happen somewhere.
[Jiewen] "Arch PEIM" refers to the PEIM required by the PEI Core.
The "platform initialization" is FeatureX. It is something you have to do, no mater you have PEI or non-PEI.


Moving code to DXE has its problems as outlines by James at length.
[Jiewen] I don't think this statement is right, with assumption that your interpretation for James' "exposure" is correct: "Exposure == External Interface", and assumption that we are discussing general design principle.

I give my reason below:

1) From architecture perspective.

Please refer to PI specification 1.7A (https://uefi.org/sites/default/files/resources/PI_Spec_1_7_A_final_May1.pdf)
Section 2.1 - Introduction.
"Philosophically, the PEI phase is intended to be the thinnest amount of code to achieve the ends
listed above. As such, any more sophisticated algorithms or processing should be deferred to the
DXE phase of execution."

In history, the first EFI 1.02 implementation does not have PEI.
PEI was added, just because the EFI need permanent RAM, while it is not TRUE for the platform that requires DRAM init.
If the DRAM init simple, then you don't PEI, because it can be done in SEC.
PEI was invented, because the memory init becomes more and more complicated and has more dependency. We want to have a better architecture to support that.

The *intention* of PEI is *only* to initialize the environment for the EFI. That is why it is called as "Pre-EFI-initialization".
This is very similar to the coreboot - ROM stage and RAM stage. (You can treat PEI as ROM stage and DXE as RAM stage)

PEI should only include memory init (biggest part) and related code. If you don't have a strong reason to put a feature to PEI, then you had better to put it to DXE, according to the architecture direction.

2) From security perspective.

I am not convinced that "exposure (external interface)" is a good reason to decide where the module should be.
Thinking of this, if you prefer to put a module to the PEI because PEI has *less* exposure, then PEI will have *more* exposure after that.
If you move half of DXE features to PEI, then PEI has same exposure as DXE. What benefit we can get?

In EDKII, the general security guideline is that module location should be based upon "privilege", following the security principle - "least privilege".
If a module does not requires high privilege, then it should be put to lower privilege location.
In one of my email, I listed Trust Region (TR) concept. I copy them here again.
(I added SEC/BDS/RT to them as well, because I miss that in the first email)

================
Second, in EDKII, we have similar concept - we call trust region (TR):
1) Recovery Trust Region (SEC, PEI)
2) Main Trust Region (DXE-before EndOfDxe)
3) MM Trust Region (SMM)
4) Boot Trust Region (DXE w/o CSM-after EndOfDxe, BDS)
5) Legacy Trust Region (DXE with CSM-after EndOfDxe, BDS)
6) OS Trust Region (OS Boot, RT)

We use term "transition" to describe the domain jump. We don't use term
"isolation".
We use "isolation" where two co-existed RT cannot tamper each other. For
example, MM trust region and Boot Trust Region are isolated.
Actually, the only isolation example we have in BIOS is x86 SMM or ARM
TrustZone.
================

For example, SMM has much smaller exposure (external interface) than DXE.
But SMM has much higher privilege than DXE by design. However, it does not make sense to move feature from DXE to SMM.
All direction we have is to remove module from SMM to DXE, because the privilege concern, regardless of the exposure.

The SEC and PEI are almost same from TR perspective, they are in recovery TR.
Recovery TR has a little high privilege than Main TR, because there are some security LOCK activities happened in Recovery TR.

For a specific case, PEI might be a better place. I agree. For example, flash lock. I will definitely vote to put it to PEI.
But in general, I don't see why moving feature from PEI to DXE becomes a problem, unless you can explain in detail what the "problem" is.



Moving code to SEC has its problems too. SEC is a much more restricted
environment. A direct consequence is that you have re-invented
multiprocessor job scheduling (using tdx mailbox) instead of using
standard mp service for parallel accept. I do not account that as
"reducing complexity".
[Jiewen] OK. Let me explain multiprocessor related topic.
I don't think we intent to "reduce" complexity in this area. I would say, it is same with or without PEI.

TDX (also SEV) has special requirement to *accept* memory, before use it. The accepting is time consuming process.
So the motivation is to use multiprocessor to accelerate the process.

We have at least three architecture places to accept the memory - SEC, PEI and DXE.

A) Accept in SEC
We need write multiprocessor code in SEC. This is already mandatory, even without accepting memory.
The TDX architecture already changes the reset vector flow - ALL processors jumps to the reset vector at same time.
Having multiprocessor code in SEC is unavoidable. We have to do it, to rendez-vous APs and initialize mailbox.
The code is written because of TDX architecture change, not because memory accepting. So I won't treat it as a burden to add additional feature to memory accept in SEC.
At same time, the benefic to accept memory in SEC is significant, you can have memory ready for use. I will explain later why that matters.

B) Accept in PEI
PEI has MP_PPI, that is TRUE. But the problem is: MP_PPI starts *later*.
MP_PPI starts *after* the memory discovery (https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/CpuMpPei/CpuMpPei.c#L706), which mean the process will be:
Step 1: TdxPeim accepts PEI required memory without MP_PPI
Step 2: PlatformPei installs PEI required memory.
Step 3: MP_PPI starts.
Step 4: TdxPeim accepts additional memory with MP_PPI

Now, you can see the benefit to accept PEI memory is not there.
NOTE: PEI memory is ~64M if GPAW is 48, it is ~98M if GPAW is 52, which is a big number.

That mean, with the solution you proposed (use PEI MP_PPI), we will have trouble on boot performance.

C) Accept in DXE
Almost same as PEI.
You have to accept some memory before launch MP_SERVICE. Same boot performance issue.


As such, we conclude that doing memory accept in SEC is the best choice for TDX architecture.
You may argue that we re-invented is MP work in SEC.
Right, but this is done because of TDX architecture.
It is NOT related to config-A (w/ PEI) or config-B (w/o PEI).



And I've not yet seen the other changes you
have done for pei-less tdvf ...
[Jiewen] That is because there is no many change. :-)

Here is just some early code.
1) Platform Init
In config-B, it is at https://github.com/tianocore/edk2-staging/blob/TDVF/OvmfPkg/Library/TdvfPlatformLibQemu/Platform.c
In config-A, it is at https://github.com/mxu9/edk2/blob/tdvf_wave2.v3/OvmfPkg/PlatformPei/Platform.c

2) DXE hand off
In config-B, it is at https://github.com/tianocore/edk2-staging/blob/TDVF/OvmfPkg/Library/TdxStartupLib/DxeLoad.c
In config-A, it is at https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c

The reset should be almost same in from SEC/PEI perspective.
I don't see a reason to carry whole PEI just to run platform init (~300 line of code).

For config-B, Min is working on it and doing clean up.
You are welcome to provide feedback after we send out patch for config-B.




take care,
Gerd





Re: [PATCH 0/5] Series short description

 

Hi Leif,

Oh, I missed that. I need to fix my mailing list filter...

Anyway, thanks for the feedback. Let me update if I need.

Regards,

2021年11月26日(金) 1:40 Leif Lindholm <leif@...>:


Hi Masami,

My feedback was
https://edk2.groups.io/g/devel/message/83641

Best Regards,

Leif

On Thu, Nov 25, 2021 at 20:19:51 +0900, Masami Hiramatsu wrote:
Hello Leif and Ard,

Could you give me any feedback on this series?

Thank you,

2021年11月5日(金) 18:23 Masami Hiramatsu <masami.hiramatsu@...>:

Hello Leif and Ard,

Here are a series of patches to fix some issues on the DeveloperBox.
Our team found those issues when we ran the SystemReady ES ACS tests[1].

[1] https://github.com/ARM-software/arm-systemready/tree/main/ES

The seires has 5 patches, [1/5] is a resend patch which I sent
before[2], others are new fixes. Actually, one another issue
still exists, which will be fixed soon.

[2] https://www.mail-archive.com/devel@edk2.groups.io/msg37170.html

Thank you,

---

Masami Hiramatsu (5):
[RESEND][edk2-platforms] Silicon/SynQuacerI2cDxe: Wait for bus busy
[edk2-platforms] Silicon/Socionext/SynQuacer: Fix GenericWatchdog interrupt number
[edk2-platforms] Silicon/SynQuacerPlatformFlashAccessLib: Fix the number of erase blocks
[edk2-platforms] Silicon/SynQuacer: add DBG2 ACPI table
[edk2-platforms] Platform/DeveloperBox: Expand NvStorage sizes


.../Socionext/DeveloperBox/DeveloperBox.dsc.inc | 10 +--
.../Socionext/SynQuacer/AcpiTables/AcpiTables.inf | 2 +
Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc | 70 ++++++++++++++++++++
Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc | 6 +-
.../Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c | 38 +++++++----
.../SynQuacerPlatformFlashAccessLib.c | 2 -
6 files changed, 107 insertions(+), 21 deletions(-)
create mode 100644 Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc

--
Masami Hiramatsu <masami.hiramatsu@...>


--
Masami Hiramatsu
--
Masami Hiramatsu


Re: [PATCH edk2-platforms 0/2] Socionext housekeeping

 

Hi Leif,

Sorry for replying late.

2021年11月11日(木) 20:59 Leif Lindholm <leif@...>:

Masami - while looking at your set from last Friday, patch 1/5 fails
to apply since the file being modified has the wrong line endings in
the tree.
Oh, OK. is it CR/LF?


2/2 fixes that.
(Obviously, the patch looks nonsensical since SMTP strips the added CR
back out.)
Thanks for fixing!


1/2 adds you as a reviewer to the Socionext platforms. (This means
that you will be cc:d on any future patches to these, and that I
can use your Reviewed-by for 2/2.)

Does this sound good to you?
Yes, these look good to me.

Acked-by: Masami Hiramatsu <masami.hiramatsu@...>

for the series.

Thank you!


Leif Lindholm (2):
Maintainers.txt: add Masami as Socionext reviewer
Silicon/Socionext: fix line endings

Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c | 1162 ++++++++++----------
Maintainers.txt | 1 +
2 files changed, 582 insertions(+), 581 deletions(-)

--
2.30.2

--
Masami Hiramatsu


Re: [PATCH 0/5] Series short description

Leif Lindholm
 

Hi Masami,

My feedback was
https://edk2.groups.io/g/devel/message/83641

Best Regards,

Leif

On Thu, Nov 25, 2021 at 20:19:51 +0900, Masami Hiramatsu wrote:
Hello Leif and Ard,

Could you give me any feedback on this series?

Thank you,

2021年11月5日(金) 18:23 Masami Hiramatsu <masami.hiramatsu@...>:

Hello Leif and Ard,

Here are a series of patches to fix some issues on the DeveloperBox.
Our team found those issues when we ran the SystemReady ES ACS tests[1].

[1] https://github.com/ARM-software/arm-systemready/tree/main/ES

The seires has 5 patches, [1/5] is a resend patch which I sent
before[2], others are new fixes. Actually, one another issue
still exists, which will be fixed soon.

[2] https://www.mail-archive.com/devel@edk2.groups.io/msg37170.html

Thank you,

---

Masami Hiramatsu (5):
[RESEND][edk2-platforms] Silicon/SynQuacerI2cDxe: Wait for bus busy
[edk2-platforms] Silicon/Socionext/SynQuacer: Fix GenericWatchdog interrupt number
[edk2-platforms] Silicon/SynQuacerPlatformFlashAccessLib: Fix the number of erase blocks
[edk2-platforms] Silicon/SynQuacer: add DBG2 ACPI table
[edk2-platforms] Platform/DeveloperBox: Expand NvStorage sizes


.../Socionext/DeveloperBox/DeveloperBox.dsc.inc | 10 +--
.../Socionext/SynQuacer/AcpiTables/AcpiTables.inf | 2 +
Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc | 70 ++++++++++++++++++++
Silicon/Socionext/SynQuacer/AcpiTables/Gtdt.aslc | 6 +-
.../Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c | 38 +++++++----
.../SynQuacerPlatformFlashAccessLib.c | 2 -
6 files changed, 107 insertions(+), 21 deletions(-)
create mode 100644 Silicon/Socionext/SynQuacer/AcpiTables/Dbg2.aslc

--
Masami Hiramatsu <masami.hiramatsu@...>


--
Masami Hiramatsu


Cancelled Event: TianoCore Design Meeting - APAC/NAMO - Friday, November 26, 2021 #cal-cancelled

devel@edk2.groups.io Calendar <noreply@...>
 

Cancelled: TianoCore Design Meeting - APAC/NAMO

This event has been cancelled.

When:
Friday, November 26, 2021
9:30am to 10:30am
(UTC+08:00) Asia/Shanghai

Where:
Microsoft Teams

Organizer: Ray Ni ray.ni@...

Description:

TOPIC

  1. NA

For more info, see here: https://www.tianocore.org/design-meeting/


Microsoft Teams meeting

Join on your computer or mobile app

Click here to join the meeting

Join with a video conferencing device

teams@...

Video Conference ID: 119 715 416 0

Alternate VTC dialing instructions

Learn More | Meeting options


Re: [PATCH v2 3/8] ArmPkg: Add Arm Firmware TRNG library

Sami Mujawar
 

Hi Leif,

Thank you for the feedback.

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar


On 24/11/2021 01:01 PM, Leif Lindholm wrote:
Hi Sami,

On Tue, Nov 16, 2021 at 11:32:55 +0000, Sami Mujawar wrote:
Bugzilla: 3668 (https://bugzilla.tianocore.org/show_bug.cgi?id=3668)

The Arm True Random Number Generator Firmware, Interface 1.0,
Platform Design Document
(https://developer.arm.com/documentation/den0098/latest/)
defines an interface between an Operating System (OS) executing
at EL1 and Firmware (FW) exposing a conditioned entropy source
that is provided by a TRNG back end.

The conditioned entropy, that is provided by the TRNG FW interface,
is commonly used to seed deterministic random number generators.

This patch adds a TrngLib library that implements the Arm TRNG
firmware interface.

Signed-off-by: Sami Mujawar <sami.mujawar@...>
---

Notes:
v2:
- MdePkg\Include\Library\TrngLib.h is base type [LIMING]
library. It can use RETURN_STATUS instead of
EFI_STATUS.
- Replaced EFI_STATUS with RETURN_STATUS. [SAMI]
- MdePkg\Include\Library\TrngLib.h API parameter [LIMING]
doesn't require CONST. CONST means the value
specified by the input pointer will not be
changed in API implementation.
- Removed the use of constant pointers in the [SAMI]
TRNG API.

ArmPkg/ArmPkg.dsc | 1 +
ArmPkg/Library/ArmFwTrngLib/ArmFwTrngDefs.h | 64 +++
ArmPkg/Library/ArmFwTrngLib/ArmFwTrngLib.c | 483 ++++++++++++++++++++
ArmPkg/Library/ArmFwTrngLib/ArmFwTrngLib.inf | 34 ++
4 files changed, 582 insertions(+)

diff --git a/ArmPkg/ArmPkg.dsc b/ArmPkg/ArmPkg.dsc
index 59fd8f295d4f614cc68ee1021e691f94e279ab81..23df68c5eb53df11de5d96bde4949f3c833c9b2c 100644
--- a/ArmPkg/ArmPkg.dsc
+++ b/ArmPkg/ArmPkg.dsc
@@ -156,6 +156,7 @@ [Components.common]
ArmPkg/Drivers/ArmCrashDumpDxe/ArmCrashDumpDxe.inf
ArmPkg/Drivers/ArmScmiDxe/ArmScmiDxe.inf
+ ArmPkg/Library/ArmFwTrngLib/ArmFwTrngLib.inf
ArmPkg/Universal/Smbios/ProcessorSubClassDxe/ProcessorSubClassDxe.inf
ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf
ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLibNull.inf
diff --git a/ArmPkg/Library/ArmFwTrngLib/ArmFwTrngDefs.h b/ArmPkg/Library/ArmFwTrngLib/ArmFwTrngDefs.h
new file mode 100644
index 0000000000000000000000000000000000000000..42236e743d972df0df205b1565496afeff5785f3
--- /dev/null
+++ b/ArmPkg/Library/ArmFwTrngLib/ArmFwTrngDefs.h
@@ -0,0 +1,64 @@
+/** @file
+ Arm Firmware TRNG definitions.
+
+ Copyright (c) 2021, Arm Limited. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+ @par Reference(s):
+ - [1] Arm True Random Number Generator Firmware, Interface 1.0,
+ Platform Design Document.
+ (https://developer.arm.com/documentation/den0098/latest/)
+
+ @par Glossary:
+ - TRNG - True Random Number Generator
+ - FID - Function ID
+**/
+
+#ifndef ARM_FW_TRNG_DEFS_H_
+#define ARM_FW_TRNG_DEFS_H_
+
+// Firmware TRNG interface Function IDs
+#define FID_TRNG_VERSION 0x84000050
+#define FID_TRNG_FEATURES 0x84000051
+#define FID_TRNG_GET_UUID 0x84000052
+#define FID_TRNG_RND_AARCH32 0x84000053
+#define FID_TRNG_RND_AARCH64 0xC4000053
Do these belong in ArmStdSmc.h?
[SAMI] I will fix this in the next version.

+
+// Firmware TRNG revision mask and shift
+#define TRNG_REV_MAJOR_MASK 0x7FFF
+#define TRNG_REV_MINOR_MASK 0xFFFF
+#define TRNG_REV_MAJOR_SHIFT 16
+#define TRNG_REV_MINOR_SHIFT 0
+
+// Firmware TRNG status codes
+#define TRNG_STATUS_SUCCESS (INT32)(0)
+#define TRNG_NOT_SUPPORTED (INT32)(-1)
+#define TRNG_INVALID_PARAMETER (INT32)(-2)
+#define TRNG_NO_ENTROPY (INT32)(-3)
And the rest of the stuff to here, really?
[SAMI] I will fix this in the next version.

+#if defined (MDE_CPU_ARM)
+/** FID to use on AArch32 platform to request entropy.
+*/
+#define FID_TRNG_RND FID_TRNG_RND_AARCH32
+
+/** Maximum bits of entropy supported on AArch32.
+*/
+#define MAX_ENTROPY_BITS 96
+#elif defined (MDE_CPU_AARCH64)
+/** FID to use on AArch64 platform to request entropy.
+*/
+#define FID_TRNG_RND FID_TRNG_RND_AARCH64
+
+/** Maximum bits of entropy supported on AArch64.
+*/
+#define MAX_ENTROPY_BITS 192
+#else
+#error "Firmware TRNG not supported. Unknown chipset."
+#endif
+
+/** Typedef for SMC or HVC arguments.
+*/
+typedef ARM_SMC_ARGS ARM_MONITOR_ARGS;
+
+#endif // ARM_FW_TRNG_DEFS_H_
diff --git a/ArmPkg/Library/ArmFwTrngLib/ArmFwTrngLib.c b/ArmPkg/Library/ArmFwTrngLib/ArmFwTrngLib.c
new file mode 100644
index 0000000000000000000000000000000000000000..314e7ffbc232ae90bbb77306f9c7113ce63012c8
--- /dev/null
+++ b/ArmPkg/Library/ArmFwTrngLib/ArmFwTrngLib.c
@@ -0,0 +1,483 @@
+/** @file
+ Arm Firmware TRNG interface library.
+
+ Copyright (c) 2021, Arm Limited. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+ @par Reference(s):
+ - [1] Arm True Random Number Generator Firmware, Interface 1.0,
+ Platform Design Document.
+ (https://developer.arm.com/documentation/den0098/latest/)
+ - [2] NIST Special Publication 800-90A Revision 1, June 2015, Recommendation
+ for Random Number Generation Using Deterministic Random Bit Generators.
+ (https://csrc.nist.gov/publications/detail/sp/800-90a/rev-1/final)
+ - [3] NIST Special Publication 800-90B, Recommendation for the Entropy
+ Sources Used for Random Bit Generation.
+ (https://csrc.nist.gov/publications/detail/sp/800-90b/final)
+ - [4] (Second Draft) NIST Special Publication 800-90C, Recommendation for
+ Random Bit Generator (RBG) Constructions.
+ (https://csrc.nist.gov/publications/detail/sp/800-90c/draft)
+
+ @par Glossary:
+ - TRNG - True Random Number Generator
+ - FID - Function ID
+**/
+
+#include <Base.h>
+#include <Library/ArmHvcLib.h>
+#include <Library/ArmLib.h>
+#include <Library/ArmSmcLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+
+#include "ArmFwTrngDefs.h"
+
+/** Convert TRNG status codes to EFI status codes.
+
+ @param [in] TrngStatus TRNG status code.
+
+ @retval RETURN_SUCCESS Success.
+ @retval RETURN_UNSUPPORTED Function not implemented.
+ @retval RETURN_INVALID_PARAMETER A parameter is invalid.
+ @retval RETURN_NOT_READY No Entropy available.
+**/
+STATIC
+RETURN_STATUS
+TrngStatusToEfiStatus (
+ IN INT32 TrngStatus
+ )
+{
+ switch (TrngStatus) {
+ case TRNG_NOT_SUPPORTED:
+ return RETURN_UNSUPPORTED;
+
+ case TRNG_INVALID_PARAMETER:
+ return RETURN_INVALID_PARAMETER;
+
+ case TRNG_NO_ENTROPY:
+ return RETURN_NOT_READY;
+
+ case TRNG_STATUS_SUCCESS:
+ default:
+ return RETURN_SUCCESS;
+ }
+}
+
+/** Invoke the monitor call using the appropriate conduit.
+ If PcdMonitorConduitHvc is TRUE use the HVC conduit else use SMC conduit.
+
+ @param [in, out] Args Arguments passed to and returned from the monitor.
+
+ @return VOID
+**/
+STATIC
+VOID
+ArmCallMonitor (
+ IN OUT ARM_MONITOR_ARGS *Args
+ )
+{
+ if (FeaturePcdGet (PcdMonitorConduitHvc)) {
+ ArmCallHvc ((ARM_HVC_ARGS*)Args);
+ } else {
+ ArmCallSmc ((ARM_SMC_ARGS*)Args);
+ }
+}
Should this be in (a potentially renamed) ArmSmcLib?
[SAMI] Looking at ArmSmcLib and ArmHvcLib libraries there is not much difference in the code other than the SMC/HVC call. Please let me know if I should submit a patch to unify these in ArmMonitorLib?
The ArmCall<Smc|Hvc> APIs would still remain the same but moved to ArmMonitorLib.

+
+/** Get the version of the TRNG backend.
+
+ A TRNG may be implemented by the system firmware, in which case this
+ function shall return the version of the TRNG backend.
+ The implementation must return NOT_SUPPORTED if a Back end is not present.
+
+ @param [out] MajorRevision Major revision.
+ @param [out] MinorRevision Minor revision.
+
+ @retval RETURN_SUCCESS The function completed successfully.
+ @retval RETURN_INVALID_PARAMETER Invalid parameter.
+ @retval RETURN_UNSUPPORTED Backend not present.
+**/
+RETURN_STATUS
+EFIAPI
+GetTrngVersion (
+ OUT UINT16 *MajorRevision,
+ OUT UINT16 *MinorRevision
+ )
+{
+ RETURN_STATUS Status;
+ ARM_MONITOR_ARGS Parameters;
+ INT32 Revision;
+
+ if ((MajorRevision == NULL) || (MinorRevision == NULL)) {
+ return RETURN_INVALID_PARAMETER;
+ }
+
+ ZeroMem (&Parameters, sizeof (Parameters));
+
+ /*
+ Cf. [1], 2.1 TRNG_VERSION
+ Function ID (W0) 0x8400_0050
+ Parameters
+ W1-W7 Reserved (MBZ)
+ Returns
+ Success (W0 > 0) W0[31] MBZ
+ W0[30:16] Major revision
+ W0[15:0] Minor revision
+ W1 - W3 Reserved (MBZ)
+ Error (W0 < 0)
+ NOT_SUPPORTED Function not implemented
+ */
I have a comment on the placement of API descriptions further down.

+ Parameters.Arg0 = FID_TRNG_VERSION;
+ ArmCallMonitor (&Parameters);
+
+ Revision = (INT32)Parameters.Arg0;
+ // Convert status codes to EFI status codes.
+ Status = TrngStatusToEfiStatus (Revision);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ *MinorRevision = (Revision & TRNG_REV_MINOR_MASK);
+ *MajorRevision = ((Revision >> TRNG_REV_MAJOR_SHIFT) & TRNG_REV_MAJOR_MASK);
+ return RETURN_SUCCESS;
+}
+
+#ifndef MDEPKG_NDEBUG
+/** Get the features supported by the TRNG backend.
+
+ The caller can determine if functions defined in the TRNG ABI are
+ present in the ABI implementation.
+
+ @param [in] FunctionId Function Id.
+ @param [out] Capability Function specific capability if present
+ otherwise Zero is returned.
+
+ @retval RETURN_SUCCESS The function completed successfully.
+ @retval RETURN_INVALID_PARAMETER Invalid parameter.
+ @retval RETURN_UNSUPPORTED Function not implemented.
+**/
+STATIC
+RETURN_STATUS
+EFIAPI
+GetTrngFeatures (
+ IN CONST UINT32 FunctionId,
+ OUT UINT32 *Capability OPTIONAL
+ )
+{
+ ARM_MONITOR_ARGS Parameters;
+
+ ZeroMem (&Parameters, sizeof (Parameters));
+
+ /*
+ Cf. [1], Section 2.2 TRNG_FEATURES
+ Function ID (W0) 0x8400_0051
+ Parameters
+ W1 trng_func_id
+ W2-W7 Reserved (MBZ)
+ Returns
+ Success (W0 >= 0)
+ SUCCESS Function is implemented.
+ > 0 Function is implemented and
+ has specific capabilities,
+ see function definition.
+ Error (W0 < 0)
+ NOT_SUPPORTED Function with FID=trng_func_id
+ is not implemented
+ */
I have a comment on the placement of API descriptions further down.

+ Parameters.Arg0 = FID_TRNG_FEATURES;
+ Parameters.Arg1 = FunctionId;
+ ArmCallMonitor (&Parameters);
+ if (Parameters.Arg0 < TRNG_STATUS_SUCCESS) {
+ return RETURN_UNSUPPORTED;
+ }
+
+ if (Capability != NULL) {
+ *Capability = Parameters.Arg0;
+ }
+
+ return RETURN_SUCCESS;
+}
+#endif //MDEPKG_NDEBUG
+
+/** Get the UUID of the TRNG backend.
+
+ A TRNG may be implemented by the system firmware, in which case this
+ function shall return the UUID of the TRNG backend.
+ Returning the TRNG UUID is optional and if not implemented, RETURN_UNSUPPORTED
+ shall be returned.
+
+ Note: The caller must not rely on the returned UUID as a trustworthy TRNG
+ Back end identity
+
+ @param [out] Guid UUID of the TRNG backend.
+
+ @retval RETURN_SUCCESS The function completed successfully.
+ @retval RETURN_INVALID_PARAMETER Invalid parameter.
+ @retval RETURN_UNSUPPORTED Function not implemented.
+**/
+RETURN_STATUS
+EFIAPI
+GetTrngUuid (
+ OUT GUID *Guid
+ )
+{
+ RETURN_STATUS Status;
+ ARM_MONITOR_ARGS Parameters;
+
+ ZeroMem (&Parameters, sizeof (Parameters));
+
+ /*
+ Cf. [1], Section 2.3 TRNG_GET_UUID
+ Function ID (W0) 0x8400_0052
+ Parameters
+ W1-W7 Reserved (MBZ)
+ Returns
+ Success (W0 != -1)
+ W0 UUID[31:0]
+ W1 UUID[63:32]
+ W2 UUID[95:64]
+ W3 UUID[127:96]
+ Error (W0 = -1)
+ W0 NOT_SUPPORTED
+ */
+ Parameters.Arg0 = FID_TRNG_GET_UUID;
+ ArmCallMonitor (&Parameters);
+
+ // Convert status codes to EFI status codes.
+ Status = TrngStatusToEfiStatus ((INT32)Parameters.Arg0);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ Guid->Data1 = (Parameters.Arg0 & MAX_UINT32);
+ Guid->Data2 = (Parameters.Arg1 & MAX_UINT16);
+ Guid->Data3 = ((Parameters.Arg1 >> 16) & MAX_UINT16);
+
+ Guid->Data4[0] = (Parameters.Arg2 & MAX_UINT8);
+ Guid->Data4[1] = ((Parameters.Arg2 >> 8) & MAX_UINT8);
+ Guid->Data4[2] = ((Parameters.Arg2 >> 16) & MAX_UINT8);
+ Guid->Data4[3] = ((Parameters.Arg2 >> 24) & MAX_UINT8);
+
+ Guid->Data4[4] = (Parameters.Arg3 & MAX_UINT8);
+ Guid->Data4[5] = ((Parameters.Arg3 >> 8) & MAX_UINT8);
+ Guid->Data4[6] = ((Parameters.Arg3 >> 16) & MAX_UINT8);
+ Guid->Data4[7] = ((Parameters.Arg3 >> 24) & MAX_UINT8);
+
+ DEBUG ((DEBUG_INFO, "FW-TRNG: UUID %g\n", Guid));
+
+ return RETURN_SUCCESS;
+}
+
+/** Returns maximum number of entropy bits that can be returned in a single
+ call.
+
+ @return Returns the maximum number of Entropy bits that can be returned
+ in a single call to GetEntropy().
+**/
+UINTN
+EFIAPI
+GetTrngMaxSupportedEntropyBits (
+ VOID
+ )
+{
+ return MAX_ENTROPY_BITS;
+}
+
+/** Returns N bits of conditioned entropy.
+
+ See [3] Section 2.3.1 GetEntropy: An Interface to the Entropy Source
+ GetEntropy
+ Input:
+ bits_of_entropy: the requested amount of entropy
+ Output:
+ entropy_bitstring: The string that provides the requested entropy.
+ status: A Boolean value that is TRUE if the request has been satisfied,
+ and is FALSE otherwise.
+
+ Note: In this implementation this function returns a status code instead
+ of a boolean value.
+ This is also compatible with the definition of Get_Entropy, see [2]
+ Section 7.4 Entropy Source Calls.
+ (status, entropy_bitstring) = Get_Entropy (
+ requested_entropy,
+ max_length
+ )
+
+ @param [in] EntropyBits Number of entropy bits requested.
+ @param [out] Buffer Buffer to return the entropy bits.
+ @param [in] BufferSize Size of the Buffer in bytes.
+
+ @retval RETURN_SUCCESS The function completed successfully.
+ @retval RETURN_INVALID_PARAMETER Invalid parameter.
+ @retval RETURN_UNSUPPORTED Function not implemented.
+ @retval RETURN_BAD_BUFFER_SIZE Buffer size is too small.
+ @retval RETURN_NOT_READY No Entropy available.
+**/
+RETURN_STATUS
+EFIAPI
+GetEntropy (
+ IN CONST UINTN EntropyBits,
+ OUT UINT8 *Buffer,
+ IN CONST UINTN BufferSize
+ )
+{
+ RETURN_STATUS Status;
+ ARM_MONITOR_ARGS Parameters;
+ UINTN EntropyBytes;
+ UINTN LastValidBits;
+ UINTN ArgSelector;
+ UINTN BytesToClear;
+
+ // [1] Section 2.4.3 Caller responsibilities.
+ // The caller cannot request more than MAX_BITS bits of conditioned
+ // entropy per call.
Comment is redundant, code is clearer without it.

+ if ((EntropyBits == 0) || (EntropyBits > MAX_ENTROPY_BITS)) {
+ return RETURN_INVALID_PARAMETER;
+ }
+
+ EntropyBytes = (EntropyBits + 7) >> 3;
+ if (EntropyBytes > BufferSize) {
Not for later: we're verifying the value of EntropyBytes here - if
there are more aspects of it that need verifying, that should also be
done here.

+ return RETURN_BAD_BUFFER_SIZE;
+ }
+
+ ZeroMem (Buffer, BufferSize);
+ ZeroMem (&Parameters, sizeof (Parameters));
+
+ /*
+ Cf. [1], Section 2.4 TRNG_RND
+ Function ID (W0) 0x8400_0053
+ 0xC400_0053
+ SMC32 Parameters
+ W1 N bits of entropy (1 6 N 6 96)
+ W2-W7 Reserved (MBZ)
+ SMC64 Parameters
+ X1 N bits of entropy (1 6 N 6 192)
+ X2-X7 Reserved (MBZ)
+ SMC32 Returns
+ Success (W0 = 0):
+ W0 MBZ
+ W1 Entropy[95:64]
+ W2 Entropy[63:32]
+ W3 Entropy[31:0]
+ Error (W0 < 0)
+ W0 NOT_SUPPORTED
+ NO_ENTROPY
+ INVALID_PARAMETERS
+ W1 - W3 Reserved (MBZ)
+ SMC64 Returns
+ Success (X0 = 0):
+ X0 MBZ
+ X1 Entropy[191:128]
+ X2 Entropy[127:64]
+ X3 Entropy[63:0]
+ Error (X0 < 0)
+ X0 NOT_SUPPORTED
+ NO_ENTROPY
+ INVALID_PARAMETERS
+ X1 - X3 Reserved (MBZ)
+ */
The above comment block completely wrecks the readability of the
function.

Would suggest putting it in the header file describing the monitor
call. For our SIP SVC calls, we've done this in the following form:

/*
* SMC call to retrieve number of CPUs present in the system.
* Input values:
* x0: NUVIA_SIP_GET_NUM_CPUS
* Return values:
* x0: SMC_OK
* x1: Number of CPUs present
*/
#define NUVIA_SIP_GET_NUM_CPUS SIP_FUNCTION_ID(0x20)

(Where SIP_FUNCTION_ID is one of a set of macros I should submit for
addition to ArmStdSmc.h)

+ Parameters.Arg0 = FID_TRNG_RND;
+ Parameters.Arg1 = EntropyBits;
+ ArmCallMonitor (&Parameters);
+
+ // Convert status codes to EFI status codes.
Function name already says this, comment redundant.

+ Status = TrngStatusToEfiStatus ((INT32)Parameters.Arg0);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
From here

+ // Extract Data
+ // ArgSelector = ((EntropyBytes + 3) >> 2); for AArch32
+ // ArgSelector = ((EntropyBytes + 7) >> 3); for AArch64
+ // ((sizeof (UINTN) >> 2) + 1) is 3 or 2 depending on size of UINTN
+ ArgSelector = ((EntropyBytes + (sizeof (UINTN) - 1)) >>
+ ((sizeof (UINTN) >> 2) + 1));
+
+ switch (ArgSelector) {
+ case 3:
+ CopyMem (&Buffer[(sizeof (UINTN) * 2)], &Parameters.Arg1, sizeof (UINTN));
+
+ case 2:
+ CopyMem (&Buffer[sizeof (UINTN)], &Parameters.Arg2, sizeof (UINTN));
+
+ case 1:
+ CopyMem (&Buffer[0], &Parameters.Arg3, sizeof (UINTN));
+ break;
+
+ default:
+ ASSERT (0);
+ return RETURN_INVALID_PARAMETER;
+ } // switch
to here ... I'm not convinced you yourself would be able to read or
explain this code a few months down the line.

Is there a strong reason for why Buffer cannot be a UINTN *?
[SAMI] The specification allows to request minimum 1 bit of entropy (although I don't think there would be a use case for this). Therefore, I selected UINT8.
However, I agree the logic is complex. I will simplify this code.

I think what this code is doing can equally be written as:

Buffer[0] = Parameters.Arg3;
if ((EntropyBytes / sizeof (UINTN)) > 1) {
Buffer[1] = Parameters.Arg2;
}
if ((EntropyBytes / sizeof (UINTN)) > 2) {
Buffer[2] = Parameters.Arg1;
}

+
+
+ // [1] Section 2.4.3 Caller responsibilities.
+ // The caller must ensure that only the value in Entropy[N-1:0] is consumed
+ // and that the remaining bits in Entropy[MAX_BITS-1:N] are ignored.
+ // Therefore, Clear the unused upper bytes.
This is source code, not the specification.

// Mask off any unused top bytes, in accordance with specification

is sufficient as a comment.
[SAMI] I will fix this and the other comments in the next revision.

/
Leif

+ BytesToClear = (sizeof (UINTN) * ArgSelector) - EntropyBytes;
+ if (BytesToClear != 0) {
+ ZeroMem (&Buffer[EntropyBytes], BytesToClear);
+ }
+
+ // Clear the unused MSB bits of the last byte.
+ LastValidBits = EntropyBits & 0x7;
+ if (LastValidBits != 0) {
+ Buffer[EntropyBytes - 1] &= (0xFF >> (8 - LastValidBits));
+ }
+
+ return Status;
+}
+
+/** The constructor checks that the FW-TRNG interface is supported
+ by the host firmware.
+
+ It will ASSERT() if FW-TRNG is not supported.
+ It will always return RETURN_SUCCESS.
+
+ @retval RETURN_SUCCESS The constructor always returns RETURN_SUCCESS.
+**/
+RETURN_STATUS
+EFIAPI
+ArmFwTrngLibConstructor (
+ VOID
+ )
+{
+ RETURN_STATUS Status;
+ UINT16 MajorRev;
+ UINT16 MinorRev;
+ GUID Guid;
+
+ Status = GetTrngVersion (&MajorRev, &MinorRev);
+ if (EFI_ERROR (Status)) {
+ return RETURN_SUCCESS;
+ }
+
+#ifndef MDEPKG_NDEBUG
+ // Check that the required features are present.
+ Status = GetTrngFeatures (FID_TRNG_RND, NULL);
+ if (EFI_ERROR (Status)) {
+ return RETURN_SUCCESS;
+ }
+
+ // Check if TRNG UUID is supported and if so trace the GUID.
+ Status = GetTrngFeatures (FID_TRNG_GET_UUID, NULL);
+ if (EFI_ERROR (Status)) {
+ return RETURN_SUCCESS;
+ }
+#endif
+
+ Status = GetTrngUuid (&Guid);
+ if (EFI_ERROR (Status)) {
+ return RETURN_SUCCESS;
+ }
+
+ DEBUG ((
+ DEBUG_INFO,
+ "FW-TRNG: Version %d.%d, GUID {%g}\n",
+ MajorRev,
+ MinorRev,
+ Guid
+ ));
+
+ return RETURN_SUCCESS;
+}
diff --git a/ArmPkg/Library/ArmFwTrngLib/ArmFwTrngLib.inf b/ArmPkg/Library/ArmFwTrngLib/ArmFwTrngLib.inf
new file mode 100644
index 0000000000000000000000000000000000000000..4b2c58251fbe8fbcb5af308736db014e8d954720
--- /dev/null
+++ b/ArmPkg/Library/ArmFwTrngLib/ArmFwTrngLib.inf
@@ -0,0 +1,34 @@
+## @file
+# Arm Firmware TRNG interface library.
+#
+# Copyright (c) 2021, Arm Limited. All rights reserved.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+
+[Defines]
+ INF_VERSION = 0x0001001B
+ BASE_NAME = ArmFwTrngLib
+ FILE_GUID = 10DE97C9-28E4-4C9B-A53E-8D7D1B0DD4E0
+ VERSION_STRING = 1.0
+ MODULE_TYPE = BASE
+ LIBRARY_CLASS = TrngLib
+ CONSTRUCTOR = ArmFwTrngLibConstructor
+
+[Sources]
+ ArmFwTrngDefs.h
+ ArmFwTrngLib.c
+
+[Packages]
+ ArmPkg/ArmPkg.dec
+ MdePkg/MdePkg.dec
+
+[LibraryClasses]
+ ArmSmcLib
+ ArmHvcLib
+ BaseLib
+ BaseMemoryLib
+
+[Pcd]
+ gArmTokenSpaceGuid.PcdMonitorConduitHvc
+
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'





Re: [PATCH] OvmfPkg/MemEncryptSevLib: check CPUID when read msr during PEI phase

qi zhou
 

Here I will add some debug infomation about the reboot issue. on the master branch, I added some logs, see below:

https://1drv.ms/u/s!As-Ec5SPH0fuimANnT5FPm8cmeM6
---
.../Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c
index e2fd109d12..8ab5849386 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c
@@ -44,6 +44,7 @@ InternalMemEncryptSevStatus (
ReadSevMsr = FALSE;

SevEsWorkArea = (SEC_SEV_ES_WORK_AREA *) FixedPcdGet32 (PcdSevEsWorkAreaBase);
+ DEBUG ((DEBUG_INFO, "SevEsWorkAera: %p, SevEsWorkArea->EncryptionMask: %lu\n", SevEsWorkArea, SevEsWorkArea ? SevEsWorkArea->EncryptionMask : 0));
if (SevEsWorkArea != NULL && SevEsWorkArea->EncryptionMask != 0) {
//
// The MSR has been read before, so it is safe to read it again and avoid
@@ -71,7 +72,9 @@ InternalMemEncryptSevStatus (
//
// Check MSR_0xC0010131 Bit 0 (Sev Enabled)
//
+ DEBUG ((DEBUG_INFO, "Begin read msr\n"));
Msr.Uint32 = AsmReadMsr32 (MSR_SEV_STATUS);
+ DEBUG ((DEBUG_INFO, "End read msr\n"));
if (Msr.Bits.SevBit) {
mSevStatus = TRUE;
}
--
2.25.1

Then rebuild the ovmf package, launch qemu with windows 10 guest os installed, repeat reboot win10 guest os 4-10 times, then there may got reboot fail
see this screenshot: https://1drv.ms/u/s!As-Ec5SPH0fuil9hsVV_ooZG0mcw?e=xgfJoL

From: qi zhou <atmgnd@...>
Sent: Thursday, November 25, 2021 21:12
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: brijesh.singh@... <brijesh.singh@...>; erdemaktas@... <erdemaktas@...>; jejb@... <jejb@...>; jiewen.yao@... <jiewen.yao@...>; min.m.xu@... <min.m.xu@...>; thomas.lendacky@... <thomas.lendacky@...>
Subject: [PATCH] OvmfPkg/MemEncryptSevLib: check CPUID when read msr during PEI phase
 
From 5b10265fa5c7b5ca728b4f18488089de6535ed28 Mon Sep 17 00:00:00 2001
From: Qi Zhou <atmgnd@...>
Date: Thu, 25 Nov 2021 20:25:55 +0800
Subject: [PATCH] OvmfPkg/MemEncryptSevLib: check CPUID when read msr during
 PEI phase

Tested on Intel Platform, It is like 'SEV-ES work area' can be modified by
os(Windows etc), and will not restored on reboot, the
SevEsWorkArea->EncryptionMask may have a random value after reboot. then it
may casue fail on reboot. The msr bits already cached by mSevStatusChecked,
there is no need to try cache again in PEI phase.

Signed-off-by: Qi Zhou <atmgnd@...>
---
 .../PeiMemEncryptSevLibInternal.c             | 55 +++++++------------
 1 file changed, 19 insertions(+), 36 deletions(-)

diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c
index e2fd109d12..0819f50669 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c
@@ -38,49 +38,32 @@ InternalMemEncryptSevStatus (
   UINT32                            RegEax;
   MSR_SEV_STATUS_REGISTER           Msr;
   CPUID_MEMORY_ENCRYPTION_INFO_EAX  Eax;
-  BOOLEAN                           ReadSevMsr;
-  SEC_SEV_ES_WORK_AREA              *SevEsWorkArea;
 
-  ReadSevMsr = FALSE;
-
-  SevEsWorkArea = (SEC_SEV_ES_WORK_AREA *) FixedPcdGet32 (PcdSevEsWorkAreaBase);
-  if (SevEsWorkArea != NULL && SevEsWorkArea->EncryptionMask != 0) {
-    //
-    // The MSR has been read before, so it is safe to read it again and avoid
-    // having to validate the CPUID information.
+  //
+  // Check if memory encryption leaf exist
+  //
+  AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL);
+  if (RegEax >= CPUID_MEMORY_ENCRYPTION_INFO) {
     //
-    ReadSevMsr = TRUE;
-  } else {
+    // CPUID Fn8000_001F[EAX] Bit 1 (Sev supported)
     //
-    // Check if memory encryption leaf exist
-    //
-    AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL);
-    if (RegEax >= CPUID_MEMORY_ENCRYPTION_INFO) {
+    AsmCpuid (CPUID_MEMORY_ENCRYPTION_INFO, &Eax.Uint32, NULL, NULL, NULL);
+
+    if (Eax.Bits.SevBit) {
       //
-      // CPUID Fn8000_001F[EAX] Bit 1 (Sev supported)
+      // Check MSR_0xC0010131 Bit 0 (Sev Enabled)
       //
-      AsmCpuid (CPUID_MEMORY_ENCRYPTION_INFO, &Eax.Uint32, NULL, NULL, NULL);
-
-      if (Eax.Bits.SevBit) {
-        ReadSevMsr = TRUE;
+      Msr.Uint32 = AsmReadMsr32 (MSR_SEV_STATUS);
+      if (Msr.Bits.SevBit) {
+        mSevStatus = TRUE;
       }
-    }
-  }
-
-  if (ReadSevMsr) {
-    //
-    // Check MSR_0xC0010131 Bit 0 (Sev Enabled)
-    //
-    Msr.Uint32 = AsmReadMsr32 (MSR_SEV_STATUS);
-    if (Msr.Bits.SevBit) {
-      mSevStatus = TRUE;
-    }
 
-    //
-    // Check MSR_0xC0010131 Bit 1 (Sev-Es Enabled)
-    //
-    if (Msr.Bits.SevEsBit) {
-      mSevEsStatus = TRUE;
+      //
+      // Check MSR_0xC0010131 Bit 1 (Sev-Es Enabled)
+      //
+      if (Msr.Bits.SevEsBit) {
+        mSevEsStatus = TRUE;
+      }
     }
   }
 
--
2.25.1


[PATCH] OvmfPkg/MemEncryptSevLib: check CPUID when read msr during PEI phase

qi zhou
 

From 5b10265fa5c7b5ca728b4f18488089de6535ed28 Mon Sep 17 00:00:00 2001
From: Qi Zhou <atmgnd@...>
Date: Thu, 25 Nov 2021 20:25:55 +0800
Subject: [PATCH] OvmfPkg/MemEncryptSevLib: check CPUID when read msr during
PEI phase

Tested on Intel Platform, It is like 'SEV-ES work area' can be modified by
os(Windows etc), and will not restored on reboot, the
SevEsWorkArea->EncryptionMask may have a random value after reboot. then it
may casue fail on reboot. The msr bits already cached by mSevStatusChecked,
there is no need to try cache again in PEI phase.

Signed-off-by: Qi Zhou <atmgnd@...>
---
.../PeiMemEncryptSevLibInternal.c | 55 +++++++------------
1 file changed, 19 insertions(+), 36 deletions(-)

diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c
index e2fd109d12..0819f50669 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c
@@ -38,49 +38,32 @@ InternalMemEncryptSevStatus (
UINT32 RegEax;
MSR_SEV_STATUS_REGISTER Msr;
CPUID_MEMORY_ENCRYPTION_INFO_EAX Eax;
- BOOLEAN ReadSevMsr;
- SEC_SEV_ES_WORK_AREA *SevEsWorkArea;

- ReadSevMsr = FALSE;
-
- SevEsWorkArea = (SEC_SEV_ES_WORK_AREA *) FixedPcdGet32 (PcdSevEsWorkAreaBase);
- if (SevEsWorkArea != NULL && SevEsWorkArea->EncryptionMask != 0) {
- //
- // The MSR has been read before, so it is safe to read it again and avoid
- // having to validate the CPUID information.
+ //
+ // Check if memory encryption leaf exist
+ //
+ AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL);
+ if (RegEax >= CPUID_MEMORY_ENCRYPTION_INFO) {
//
- ReadSevMsr = TRUE;
- } else {
+ // CPUID Fn8000_001F[EAX] Bit 1 (Sev supported)
//
- // Check if memory encryption leaf exist
- //
- AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL);
- if (RegEax >= CPUID_MEMORY_ENCRYPTION_INFO) {
+ AsmCpuid (CPUID_MEMORY_ENCRYPTION_INFO, &Eax.Uint32, NULL, NULL, NULL);
+
+ if (Eax.Bits.SevBit) {
//
- // CPUID Fn8000_001F[EAX] Bit 1 (Sev supported)
+ // Check MSR_0xC0010131 Bit 0 (Sev Enabled)
//
- AsmCpuid (CPUID_MEMORY_ENCRYPTION_INFO, &Eax.Uint32, NULL, NULL, NULL);
-
- if (Eax.Bits.SevBit) {
- ReadSevMsr = TRUE;
+ Msr.Uint32 = AsmReadMsr32 (MSR_SEV_STATUS);
+ if (Msr.Bits.SevBit) {
+ mSevStatus = TRUE;
}
- }
- }
-
- if (ReadSevMsr) {
- //
- // Check MSR_0xC0010131 Bit 0 (Sev Enabled)
- //
- Msr.Uint32 = AsmReadMsr32 (MSR_SEV_STATUS);
- if (Msr.Bits.SevBit) {
- mSevStatus = TRUE;
- }

- //
- // Check MSR_0xC0010131 Bit 1 (Sev-Es Enabled)
- //
- if (Msr.Bits.SevEsBit) {
- mSevEsStatus = TRUE;
+ //
+ // Check MSR_0xC0010131 Bit 1 (Sev-Es Enabled)
+ //
+ if (Msr.Bits.SevEsBit) {
+ mSevEsStatus = TRUE;
+ }
}
}

--
2.25.1

8201 - 8220 of 92219