devel@edk2.groups.io

Re: ArmVirt and Self-Updating Code

Ard Biesheuvel

#78097

On Thu, 22 Jul 2021 at 16:54, Bret Barkelew <Bret.Barkelew@...> wrote:

Expanding audience to the full dev list…

See below…

- Bret

From: Thomas Abraham
Sent: Wednesday, July 7, 2021 11:07 PM
To: Bret Barkelew; Ard Biesheuvel (TianoCore); Lindholm, Leif; Laszlo Ersek; Marvin Häuser; Sami Mujawar
Cc: nd
Subject: [EXTERNAL] RE: ArmVirt and Self-Updating Code

+ Sami

From: Bret Barkelew <Bret.Barkelew@...>
Sent: Thursday, July 8, 2021 11:05 AM
To: Thomas Abraham <thomas.abraham@...>; Ard Biesheuvel (TianoCore) <ardb+tianocore@...>; Lindholm, Leif <leif@...>; Laszlo Ersek <lersek@...>; Marvin Häuser <mhaeuser@...>
Subject: ArmVirt and Self-Updating Code

All,

Marvin asked me a question on the UEFI Talkbox Discord that’s a little beyond my ken…

“There is self-relocating code in ArmVirtPkg:

https://github.com/tianocore/edk2/blob/17143c4837393d42c484b42d1789b85b2cff1aaf/ArmVirtPkg/PrePi/PrePi.c#L133-L165

According to comments in the ASM, it seems like this is for Linux-based RAM boot (I saw further stuff for KVM, so it makes sense I guess?). It seems unfortunate it cannot be mapped into a known address range so that self-relocation is not necessary, but that's out of my scope to understand.
"Mapping" implies that the MMU is on, but this code boots with the MMU
off. Unlike x86, ARM does not define any physical address ranges that
are guaranteed to be backed by DRAM, so a portable image either needs
to be fully position independent, or carry the metadata it needs to
relocate itself as it is invoked.

“Now, StandaloneMmPkg has similar (self-)relocation code too: https://github.com/tianocore/edk2/blob/17143c4837393d42c484b42d1789b85b2cff1aaf/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c#L379-L386

Because I cannot find such elsewhere, I assume it must be for the same ARM virtualised environment as above.No.

The binary it applies the Relocations to is documented to be the Standalone MM core, but in fact SecCore is located:

https://github.com/tianocore/edk2/blob/17143c4837393d42c484b42d1789b85b2cff1aaf/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/SetPermissions.c#L131-L158

“This yields the following questions to me:

1) What even invokes Standalone MM on ARM? It is documented it is spawned during SEC, but I could not find any actual invocation.
It is not spawned by the normal world code that runs UEFI. It is a
secure world component that runs in a completely different execution
context (TrustZone). The code does run with the MMU enabled from the
start, but running from an a priori fixed offset was considered to be
a security hazard, so we added self relocation support.

The alternative would have been to add metadata to the StMmCore
component that can be interpreted by the secure world component that
loads it, but this would go beyond any existing specs, and make
portability more problematic.

2) Why does Standalone MM (self-)relocation locate SecCore? Should it not already have been relocated with the code from ArmPlatformPkg? Is Standalone MM embedded into ARM SecCore?
No and no. Standalone MM has nothing to do with the code that runs as
part of UEFI itself. ArmPlatformPkg is completely separate from
StandaloneMmPkg.

3) Why is SecCore the only module relocated? Are all others guaranteed to be "properly" loaded?
SecCore contains a PE/COFF loader, so all subsequent modules are
loaded normally. This is similar to the ArmVirtQemuKernel
self-relocating SEC module, which only relocates itself in this
manner, and relies on standard PE/COFF metadata for loading other
modules.

4) Is there maybe some high-level documented about the ARM boot flow? It seems to be significantly different from the x86 routes quite vastly.”
trustedfirmware.org may have some useful documentation.

Hoping that one of you could get me closer to an answer for him. Also happy to take this to the greater mailing list, but thought I’d avoid churn.

Thanks in advance!

- Bret

Re: [staging/edk2-redfish-client Tools PATCH 6/6] RedfishClientPkg/Redfish-Profile-Simulator: Add requirements

Nickle Wang

#78096

Reviewed-by: Nickle Wang <nickle.wang@...>

Thanks,

Nickle

Show quoted text

From: Chang, Abner (HPS SW/FW Technologist) <abner.chang@...>
Sent: Thursday, July 22, 2021 14:08
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Wang, Nickle (HPS SW) <nickle.wang@...>; Liming Gao <gaoliming@...>
Subject: [staging/edk2-redfish-client Tools PATCH 6/6] RedfishClientPkg/Redfish-Profile-Simulator: Add requirements

add requirements.txt for the required python module

Signed-off-by: Abner Chang <abner.chang@...>
Cc: Nickle Wang <nickle.wang@...>
Cc: Liming Gao <gaoliming@...>
---
.../Tools/Redfish-Profile-Simulator/requirements.txt | 2 ++
1 file changed, 2 insertions(+)
create mode 100644 RedfishClientPkg/Tools/Redfish-Profile-Simulator/requirements.txt

diff --git a/RedfishClientPkg/Tools/Redfish-Profile-Simulator/requirements.txt b/RedfishClientPkg/Tools/Redfish-Profile-Simulator/requirements.txt
new file mode 100644
index 0000000000..88807d87c2
--- /dev/null
+++ b/RedfishClientPkg/Tools/Redfish-Profile-Simulator/requirements.txt
@@ -0,0 +1,2 @@
+flask==1.1.1
+pyOpenSSL
--
2.17.1

Re: [staging/edk2-redfish-client Tools PATCH 5/6] RedfishClientPkg/Redfish-Profile-Simulator: Add ETAG on memory resource

Nickle Wang

#78095

Reviewed-by: Nickle Wang <nickle.wang@...>

Thanks,

Nickle

Show quoted text

From: Chang, Abner (HPS SW/FW Technologist) <abner.chang@...>
Sent: Thursday, July 22, 2021 14:08
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Wang, Nickle (HPS SW) <nickle.wang@...>; Liming Gao <gaoliming@...>
Subject: [staging/edk2-redfish-client Tools PATCH 5/6] RedfishClientPkg/Redfish-Profile-Simulator: Add ETAG on memory resource

Add ETAG support on Memory resource.

Signed-off-by: Abner Chang <abner.chang@...>
Cc: Nickle Wang <nickle.wang@...>
Cc: Liming Gao <gaoliming@...>
---
.../Redfish-Profile-Simulator/v1sim/systems.py | 13 +++++++++++++
1 file changed, 13 insertions(+)

diff --git a/RedfishClientPkg/Tools/Redfish-Profile-Simulator/v1sim/systems.py b/RedfishClientPkg/Tools/Redfish-Profile-Simulator/v1sim/systems.py
index 690101fb10..de4b839aeb 100644
--- a/RedfishClientPkg/Tools/Redfish-Profile-Simulator/v1sim/systems.py
+++ b/RedfishClientPkg/Tools/Redfish-Profile-Simulator/v1sim/systems.py
@@ -18,6 +18,7 @@ from .resource import RfResource, RfCollection
from .storage import RfSimpleStorageCollection, RfSmartStorage
import flask
import json
+import hashlib
from collections import OrderedDict

class RfSystemsCollection(RfCollection):
@@ -142,13 +143,25 @@ class RfMemoryCollection(RfCollection):
 self.res_data["Members"].append({"@odata.id":newMemoryUrl})

 post_data["@odata.id"] = newMemoryUrl
+
+ md5 = hashlib.md5()
+ md5.update(json.dumps(post_data).encode("utf-8"))
+ etag_str = 'W/"' + md5.hexdigest() + '"'
+ post_data["@odata.etag"] = etag_str
 self.elements[str(newMemoryIdx)] = post_data

 resp = flask.Response(json.dumps(post_data,indent=4))
 resp.headers["Location"] = newMemoryUrl
+ resp.headers["ETag"] = etag_str
+
 return 0, 200, None, resp

 def patch_memory(self, Idx, patch_data):
+ md5 = hashlib.md5()
+ md5.update(json.dumps(patch_data).encode("utf-8"))
+ etag_str = 'W/"' + md5.hexdigest() + '"'
+ patch_data["@odata.etag"] = etag_str
+
 self.elements[str(Idx)] = {**self.elements[str(Idx)], **patch_data}
 resp = flask.Response(json.dumps(self.elements[str(Idx)],indent=4))
 return 0, 200, None, resp
--
2.17.1

Re: [staging/edk2-redfish-client Tools PATCH 4/6] RedfishClientPkg/Redfish-Profile-Simulator: HTTP methods on Memory Collection

Nickle Wang

#78094

Reviewed-by: Nickle Wang <nickle.wang@...>

Thanks,

Nickle

Show quoted text

From: Chang, Abner (HPS SW/FW Technologist) <abner.chang@...>
Sent: Thursday, July 22, 2021 14:08
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Wang, Nickle (HPS SW) <nickle.wang@...>; Liming Gao <gaoliming@...>
Subject: [staging/edk2-redfish-client Tools PATCH 4/6] RedfishClientPkg/Redfish-Profile-Simulator: HTTP methods on Memory Collection

Add POST and PATCH methods on Memory collection and resource.

Signed-off-by: Abner Chang <abner.chang@...>
Cc: Nickle Wang <nickle.wang@...>
Cc: Liming Gao <gaoliming@...>
---
.../v1sim/redfishURIs.py | 25 +++++++++++
.../v1sim/systems.py | 43 +++++++++++++++++++
2 files changed, 68 insertions(+)

diff --git a/RedfishClientPkg/Tools/Redfish-Profile-Simulator/v1sim/redfishURIs.py b/RedfishClientPkg/Tools/Redfish-Profile-Simulator/v1sim/redfishURIs.py
index 3c912f7ce1..35d3794cc6 100644
--- a/RedfishClientPkg/Tools/Redfish-Profile-Simulator/v1sim/redfishURIs.py
+++ b/RedfishClientPkg/Tools/Redfish-Profile-Simulator/v1sim/redfishURIs.py
@@ -1,6 +1,7 @@
#
# Copyright Notice:
# Copyright (c) 2019, Intel Corporation. All rights reserved. 
+# (C) Copyright 2021 Hewlett Packard Enterprise Development LP 
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
# Copyright Notice:
@@ -308,6 +309,30 @@ def rfApi_SimpleServer(root, versions, host="127.0.0.1", port=5000, cert="", key
 else:
 return err_string, status_code

+ @app.route("/redfish/v1/Systems/<string:system_id>/Memory", methods=['POST'])
+ @auth.rfAuthRequired
+ def rf_computer_memory_post(system_id):
+ print ("in POST memory collection")
+ rdata = json.loads(request.data,object_pairs_hook=OrderedDict)
+ print("rdata:{}".format(rdata))
+ rc, status_code, err_string, resp = root.components['Systems'].get_element(system_id).components['Memory'].post_resource(rdata)
+ if rc == 0:
+ return resp, status_code
+ else:
+ return err_string, status_code
+
+ @app.route("/redfish/v1/Systems/<string:system_id>/Memory/<string:MemoryIdx>", methods=['PATCH'])
+ @auth.rfAuthRequired
+ def rf_computer_memory_patch(system_id, MemoryIdx):
+ print ("in PATCH memory[%s] resource" % MemoryIdx)
+ rdata = json.loads(request.data,object_pairs_hook=OrderedDict)
+ print("rdata:{}".format(rdata))
+ rc, status_code, err_string, resp = root.components['Systems'].get_element(system_id).components['Memory'].patch_memory(MemoryIdx, rdata)
+ if rc == 0:
+ return resp, status_code
+ else:
+ return err_string, status_code
+
 def resolve_path(service, path):
 parts = path.split('/')
 result = service
diff --git a/RedfishClientPkg/Tools/Redfish-Profile-Simulator/v1sim/systems.py b/RedfishClientPkg/Tools/Redfish-Profile-Simulator/v1sim/systems.py
index b8b3788054..690101fb10 100644
--- a/RedfishClientPkg/Tools/Redfish-Profile-Simulator/v1sim/systems.py
+++ b/RedfishClientPkg/Tools/Redfish-Profile-Simulator/v1sim/systems.py
@@ -2,6 +2,7 @@
# Copyright Notice:
#
# Copyright (c) 2019, Intel Corporation. All rights reserved. 
+# (C) Copyright 2021 Hewlett Packard Enterprise Development LP 
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
# Copyright Notice:
@@ -123,9 +124,50 @@ class RfSystemObj(RfResource):

# subclass Logs Collection
class RfMemoryCollection(RfCollection):
+ def final_init_processing(self, base_path, rel_path):
+ self.maxIdx = self.res_data["Members@..."]
+
 def element_type(self):
 return RfMemory

+ def post_resource(self, post_data):
+ print("Members@...:{}".format(self.res_data["Members@..."]))
+ print("Members:{}".format(self.res_data["Members"]))
+ print("post_data:{}".format(post_data))
+
+ self.res_data["Members@..."] = self.res_data["Members@..."] + 1
+ self.maxIdx = self.maxIdx + 1
+ newMemoryIdx = self.maxIdx
+ newMemoryUrl = self.res_data["@odata.id"] + "/" + str(newMemoryIdx)
+ self.res_data["Members"].append({"@odata.id":newMemoryUrl})
+
+ post_data["@odata.id"] = newMemoryUrl
+ self.elements[str(newMemoryIdx)] = post_data
+
+ resp = flask.Response(json.dumps(post_data,indent=4))
+ resp.headers["Location"] = newMemoryUrl
+ return 0, 200, None, resp
+
+ def patch_memory(self, Idx, patch_data):
+ self.elements[str(Idx)] = {**self.elements[str(Idx)], **patch_data}
+ resp = flask.Response(json.dumps(self.elements[str(Idx)],indent=4))
+ return 0, 200, None, resp
+
+ def get_memory(self, Idx):
+ return json.dumps(self.elements[Idx],indent=4)
+
+ def delete_memory(self, Idx):
+ print("in delete_memory")
+
+ resp = flask.Response(json.dumps(self.elements[Idx],indent=4))
+
+ self.elements.pop(Idx)
+ self.res_data["Members@..."] = self.res_data["Members@..."] - 1
+
+ newMemoryUrl = self.res_data["@odata.id"] + "/" + str(Idx)
+ self.res_data["Members"].remove({"@odata.id":newMemoryUrl})
+ return 0, 200, None, resp
+

class RfMemory(RfResource):
 pass
@@ -267,3 +309,4 @@ class RfBootOptionCollection(RfCollection):
 return 0, 200, None, resp

class RfBootOption(RfResource):
+ pass
--
2.17.1

Re: ArmVirt and Self-Updating Code

Bret Barkelew

#78093

Expanding audience to the full dev list…

See below…

- Bret

Show quoted text

From: Thomas Abraham
Sent: Wednesday, July 7, 2021 11:07 PM
To: Bret Barkelew; Ard Biesheuvel (TianoCore); Lindholm, Leif; Laszlo Ersek; Marvin Häuser; Sami Mujawar
Cc: nd
Subject: [EXTERNAL] RE: ArmVirt and Self-Updating Code

+ Sami

From: Bret Barkelew <Bret.Barkelew@...>
Sent: Thursday, July 8, 2021 11:05 AM
To: Thomas Abraham <thomas.abraham@...>; Ard Biesheuvel (TianoCore) <ardb+tianocore@...>; Lindholm, Leif <leif@...>; Laszlo Ersek <lersek@...>; Marvin Häuser <mhaeuser@...>
Subject: ArmVirt and Self-Updating Code

All,

Marvin asked me a question on the UEFI Talkbox Discord that’s a little beyond my ken…

“There is self-relocating code in ArmVirtPkg:

https://github.com/tianocore/edk2/blob/17143c4837393d42c484b42d1789b85b2cff1aaf/ArmVirtPkg/PrePi/PrePi.c#L133-L165

According to comments in the ASM, it seems like this is for Linux-based RAM boot (I saw further stuff for KVM, so it makes sense I guess?). It seems unfortunate it cannot be mapped into a known address range so that self-relocation is not necessary, but that's out of my scope to understand.

“Now, StandaloneMmPkg has similar (self-)relocation code too: https://github.com/tianocore/edk2/blob/17143c4837393d42c484b42d1789b85b2cff1aaf/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/StandaloneMmCoreEntryPoint.c#L379-L386

Because I cannot find such elsewhere, I assume it must be for the same ARM virtualised environment as above. The binary it applies the Relocations to is documented to be the Standalone MM core, but in fact SecCore is located:

https://github.com/tianocore/edk2/blob/17143c4837393d42c484b42d1789b85b2cff1aaf/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/SetPermissions.c#L131-L158

“This yields the following questions to me:

1) What even invokes Standalone MM on ARM? It is documented it is spawned during SEC, but I could not find any actual invocation.

2) Why does Standalone MM (self-)relocation locate SecCore? Should it not already have been relocated with the code from ArmPlatformPkg? Is Standalone MM embedded into ARM SecCore?

3) Why is SecCore the only module relocated? Are all others guaranteed to be "properly" loaded?

4) Is there maybe some high-level documented about the ARM boot flow? It seems to be significantly different from the x86 routes quite vastly.”

Hoping that one of you could get me closer to an answer for him. Also happy to take this to the greater mailing list, but thought I’d avoid churn.

Thanks in advance!

- Bret

Re: [PATCH v1 6/6] UefiPayloadPkg: LinuxBoot: use a text format for the configuration block.

Cheng-Chieh Huang <chengchieh@...>

#78092

Here is the documentation: https://github.com/osresearch/kexec-load/blob/main/README.md

Show quoted text

On Thu, Jul 22, 2021 at 1:09 AM Marvin Häuser <mhaeuser@...> wrote:

On 21.07.21 15:23, Cheng-Chieh Huang via groups.io wrote:
> From: Trammell Hudson <hudson@...>
>
> This adds a text command line for the UefiPayloadPkg that uses
> a textual magic number 'LnxBoot1' and a series of white-space
> separated key=value[,value...] pairs for the parameters.
>
> The v1 binary configuration structure is used if it is present instead
> for backwards compatability.
>
> Currently supported text command line arguments are are:
>
> * `serial=baud,base,width,type,hz,pci`
> (only `baud` needs to be specified, the rest have reasonable
> defaults)
>
> * `mem=start,end,type`
> Types are based on `/sys/firmware/memmaps/*/types`:
> 1 == "System RAM"
> 3 == "ACPI Tables"
> 4 == "ACPI Non-volatile Storage"
> 5 == "Reserved"
>
> * `ACPI20=base` pointer to RDSP (from `/sys/firmware/efi/systab`)
>
> * `SMBIOS=base` pointer to the SMBIOS table (also from the EFI table)
>
> Signed-off-by: Cheng-Chieh Huang <chengchieh@...>
> ---
> UefiPayloadPkg/Include/Linuxboot.h | 17 +-
> UefiPayloadPkg/Library/LbParseLib/LbParseLib.c | 252 +++++++++++++++++---
> 2 files changed, 230 insertions(+), 39 deletions(-)
>
> diff --git a/UefiPayloadPkg/Include/Linuxboot.h b/UefiPayloadPkg/Include/Linuxboot.h
> index 34ca18069983..56b39b5a09ff 100644
> --- a/UefiPayloadPkg/Include/Linuxboot.h
> +++ b/UefiPayloadPkg/Include/Linuxboot.h
> @@ -24,8 +24,7 @@ typedef struct MemoryMapEntryStruct {
> UINT32 Type;
> } MemoryMapEntry;
>
> -typedef struct UefiPayloadConfigStruct {
> - UINT64 Version;
> +typedef struct {
> UINT64 AcpiBase;
> UINT64 AcpiSize;
> UINT64 SmbiosBase;
> @@ -33,10 +32,22 @@ typedef struct UefiPayloadConfigStruct {
> SerialPortConfig SerialConfig;
> UINT32 NumMemoryMapEntries;
> MemoryMapEntry MemoryMapEntries[0];
> +} UefiPayloadConfigV1;
> +
> +typedef struct UefiPayloadConfigStruct {
> + UINT64 Version;
> + union {
> + UefiPayloadConfigV1 v1;
> + struct {
> + char cmdline[0]; // up to 64 KB
> + } v2;
> + } config;
> } UefiPayloadConfig;
> #pragma pack()
>
> -#define UEFI_PAYLOAD_CONFIG_VERSION 1
> +// magic version config is "LnxBoot1"
> +#define UEFI_PAYLOAD_CONFIG_VERSION1 1
> +#define UEFI_PAYLOAD_CONFIG_VERSION2 0x31746f6f42786e4cULL
>
> #define LINUXBOOT_MEM_RAM 1
> #define LINUXBOOT_MEM_DEFAULT 2
> diff --git a/UefiPayloadPkg/Library/LbParseLib/LbParseLib.c b/UefiPayloadPkg/Library/LbParseLib/LbParseLib.c
> index 34bfb6a1073f..5e68091cac91 100644
> --- a/UefiPayloadPkg/Library/LbParseLib/LbParseLib.c
> +++ b/UefiPayloadPkg/Library/LbParseLib/LbParseLib.c
> @@ -1,13 +1,12 @@
> /** @file
> - This library will parse the linuxboot table in memory and extract those required
> - information.
> + This library will parse the linuxboot table in memory and extract those
> +required information.
>
> Copyright (c) 2021, the u-root Authors. All rights reserved. 
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> **/
>
> -
> #include <IndustryStandard/Acpi.h>
> #include <IndustryStandard/SmBios.h>
> #include <Library/BaseLib.h>
> @@ -18,17 +17,42 @@
> #include <Library/PcdLib.h>
> #include <Linuxboot.h>
> #include <Uefi/UefiBaseType.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +//#include <string.h>
> +//#include <ctype.h>
> +
> +#define strncmp(a, b, n) AsciiStrnCmp((a), (b), (n))
> +
> +static uint64_t parse_int(const char* s, char** end) {
> + UINT64 x;
> +
> + if (s[0] == '0' && s[1] == 'x')
> + AsciiStrHexToUint64S(s, end, &x);
> + else
> + AsciiStrDecimalToUint64S(s, end, &x);
> +
> + return x;
> +}
> +
> +static int isspace(const char c) { return c == ' ' || c == '\t' || c == '\n'; }
>
> // Retrieve UefiPayloadConfig from Linuxboot's uefiboot
> -UefiPayloadConfig* GetUefiPayLoadConfig() {
> - UefiPayloadConfig* config =
> +const UefiPayloadConfig* GetUefiPayLoadConfig() {
> + const UefiPayloadConfig* config =
> (UefiPayloadConfig*)(UINTN)(PcdGet32(PcdPayloadFdMemBase) - SIZE_64KB);
> - if (config->Version != UEFI_PAYLOAD_CONFIG_VERSION) {
> - DEBUG((DEBUG_ERROR, "Expect payload config version: %d, but get %d\n",
> - UEFI_PAYLOAD_CONFIG_VERSION, config->Version));
> - CpuDeadLoop ();
> - }
> - return config;
> +
> + if (config->Version == UEFI_PAYLOAD_CONFIG_VERSION1 ||
> + config->Version == UEFI_PAYLOAD_CONFIG_VERSION2)
> + return config;
> +
> + DEBUG((DEBUG_ERROR,
> + "Expect payload config version %016lx or %016lx, but get %016lx\n",
> + UEFI_PAYLOAD_CONFIG_VERSION1, UEFI_PAYLOAD_CONFIG_VERSION2,
> + config->Version));
> + CpuDeadLoop();
> + while (1)
> + ;
> }
>
> // Align the address and add memory rang to MemInfoCallback
> @@ -54,6 +78,57 @@ void AddMemoryRange(IN BL_MEM_INFO_CALLBACK MemInfoCallback, IN UINTN start,
> MemInfoCallback(&MemoryMap, NULL);
> }
>
> +const char* cmdline_next(const char* cmdline, const char** option) {
> + // at the end of the string, we're done
> + if (!cmdline || *cmdline == '\0') return NULL;
> +
> + // skip any leading whitespace
> + while (isspace(*cmdline)) cmdline++;
> +
> + // if we've hit the end of the string, we're done
> + if (*cmdline == '\0') return NULL;
> +
> + *option = cmdline;
> +
> + // find the end of this option or the string
> + while (!isspace(*cmdline) && *cmdline != '\0') cmdline++;
> +
> + // cmdline points to the whitespace or end of string
> + return cmdline;
> +}
> +
> +int cmdline_ints(const char* option, uint64_t args[], int max) {
> + // skip any leading text up to an '='
> + const char* s = option;
> + while (1) {
> + const char c = *s++;
> + if (c == '=') break;
> +
> + if (c == '\0' || isspace(c)) {
> + s = option;
> + break;
> + }
> + }
> +
> + for (int i = 0; i < max; i++) {
> + char* end;
> + args[i] = parse_int(s, &end);
> +
> + // end of string or end of the option?
> + if (*end == '\0' || isspace(*end)) return i + 1;
> +
> + // not separator? signal an error if we have consumed any ints,
> + // otherwise return 0 saying that none were found
> + if (*end != ',') return i == 0 ? 0 : -1;
> +
> + // skip the , and get the next value
> + s = end + 1;
> + }
> +
> + // too many values!
> + return -1;
> +}
> +
> /**
> Acquire the memory information from the linuxboot table in memory.
>
> @@ -67,20 +142,50 @@ void AddMemoryRange(IN BL_MEM_INFO_CALLBACK MemInfoCallback, IN UINTN start,
> RETURN_STATUS
> EFIAPI
> ParseMemoryInfo(IN BL_MEM_INFO_CALLBACK MemInfoCallback, IN VOID* Params) {
> - UefiPayloadConfig* config;
> - int i;
> + const UefiPayloadConfig* config = GetUefiPayLoadConfig();
> + if (!config) {
> + DEBUG(
> + (DEBUG_ERROR, "ParseMemoryInfo: Could not find UEFI Payload config\n"));
> + return RETURN_SUCCESS;
> + }
> +
> + if (config->Version == UEFI_PAYLOAD_CONFIG_VERSION1) {
> + const UefiPayloadConfigV1* config1 = &config->config.v1;
> + DEBUG(
> + (DEBUG_INFO, "MemoryMap #entries: %d\n", config1->NumMemoryMapEntries));
> +
> + for (int i = 0; i < config1->NumMemoryMapEntries; i++) {
> + const MemoryMapEntry* entry = &config1->MemoryMapEntries[i];
> + DEBUG((DEBUG_INFO, "Start: 0x%lx End: 0x%lx Type:%d\n", entry->Start,
> + entry->End, entry->Type));
> + AddMemoryRange(MemInfoCallback, entry->Start, entry->End, entry->Type);
> + }
> + } else
>
> - config = GetUefiPayLoadConfig();
> + if (config->Version == UEFI_PAYLOAD_CONFIG_VERSION2) {
> + const char* cmdline = config->config.v2.cmdline;
> + const char* option;
> + uint64_t args[3];
>
> - DEBUG((DEBUG_INFO, "MemoryMap #entries: %d\n", config->NumMemoryMapEntries));
> + // look for the mem=start,end,type
> + while ((cmdline = cmdline_next(cmdline, &option))) {
> + if (strncmp(option, "mem=", 4) != 0) continue;
>
> - MemoryMapEntry* entry = &config->MemoryMapEntries[0];
> - for (i = 0; i < config->NumMemoryMapEntries; i++) {
> - DEBUG((DEBUG_INFO, "Start: 0x%lx End: 0x%lx Type:%d\n", entry->Start,
> - entry->End, entry->Type));
> - AddMemoryRange(MemInfoCallback, entry->Start, entry->End, entry->Type);
> - entry++;
> + if (cmdline_ints(option, args, 3) != 3) {
> + DEBUG((DEBUG_ERROR, "Parse error: '%a'\n", option));
> + continue;
> + }
> +
> + const uint64_t start = args[0];
> + const uint64_t end = args[1];
> + const uint64_t type = args[2];
> +
> + DEBUG(
> + (DEBUG_INFO, "Start: 0x%lx End: 0x%lx Type:%d\n", start, end, type));
> + AddMemoryRange(MemInfoCallback, start, end, type);
> + }
> }
> +
> return RETURN_SUCCESS;
> }
>
> @@ -96,14 +201,52 @@ ParseMemoryInfo(IN BL_MEM_INFO_CALLBACK MemInfoCallback, IN VOID* Params) {
> RETURN_STATUS
> EFIAPI
> ParseSystemTable(OUT SYSTEM_TABLE_INFO* SystemTableInfo) {
> - UefiPayloadConfig* config;
> + const UefiPayloadConfig* config = GetUefiPayLoadConfig();
> + if (!config) {
> + DEBUG((DEBUG_ERROR,
> + "ParseSystemTable: Could not find UEFI Payload config\n"));
> + return RETURN_SUCCESS;
> + }
>
> - config = GetUefiPayLoadConfig();
> - SystemTableInfo->AcpiTableBase = config->AcpiBase;
> - SystemTableInfo->AcpiTableSize = config->AcpiSize;
> + if (config->Version == UEFI_PAYLOAD_CONFIG_VERSION1) {
> + const UefiPayloadConfigV1* config1 = &config->config.v1;
> + SystemTableInfo->AcpiTableBase = config1->AcpiBase;
> + SystemTableInfo->AcpiTableSize = config1->AcpiSize;
>
> - SystemTableInfo->SmbiosTableBase = config->SmbiosBase;
> - SystemTableInfo->SmbiosTableSize = config->SmbiosSize;
> + SystemTableInfo->SmbiosTableBase = config1->SmbiosBase;
> + SystemTableInfo->SmbiosTableSize = config1->SmbiosSize;
> + } else
> +
> + if (config->Version == UEFI_PAYLOAD_CONFIG_VERSION2) {
> + const char* cmdline = config->config.v2.cmdline;
> + const char* option;
> + uint64_t args[2];
> +
> + // look for the acpi config
> + while ((cmdline = cmdline_next(cmdline, &option))) {
> + if (strncmp(option, "ACPI20=", 7) == 0) {
> + const int count = cmdline_ints(option, args, 2);
> + if (count < 0) {
> + DEBUG((DEBUG_ERROR, "Parse error: '%a'\n", option));
> + continue;
> + }
> +
> + if (count > 0) SystemTableInfo->AcpiTableBase = args[0];
> + if (count > 1) SystemTableInfo->AcpiTableSize = args[1];
> + }
> +
> + if (strncmp(option, "SMBIOS=", 7) == 0) {
> + const int count = cmdline_ints(option, args, 2);
> + if (count < 0) {
> + DEBUG((DEBUG_ERROR, "Parse error: '%a'\n", option));
> + continue;
> + }
> +
> + if (count > 0) SystemTableInfo->SmbiosTableBase = args[0];
> + if (count > 1) SystemTableInfo->SmbiosTableSize = args[1];
> + }

Sorry, from only a quick peak, can this not leave {Acpi,Smbios}TableBase
and {Acpi,Smbios}TableSize undefined entirely, while returning
RETURN_SUCCESS?
Even if not, it looks kind of odd a command-line argument can change the
base address without changing the size, is there a documentation of this
behaviour?
What is the structure supposed to carry if no such arguments are given
at all?

Thanks for your time!

Best regards,
Marvin

> + }
> + }
>
> return RETURN_SUCCESS;
> }
> @@ -120,15 +263,52 @@ ParseSystemTable(OUT SYSTEM_TABLE_INFO* SystemTableInfo) {
> RETURN_STATUS
> EFIAPI
> ParseSerialInfo(OUT SERIAL_PORT_INFO* SerialPortInfo) {
> - UefiPayloadConfig* config;
> - config = GetUefiPayLoadConfig();
> -
> - SerialPortInfo->BaseAddr = config->SerialConfig.BaseAddr;
> - SerialPortInfo->RegWidth = config->SerialConfig.RegWidth;
> - SerialPortInfo->Type = config->SerialConfig.Type;
> - SerialPortInfo->Baud = config->SerialConfig.Baud;
> - SerialPortInfo->InputHertz = config->SerialConfig.InputHertz;
> - SerialPortInfo->UartPciAddr = config->SerialConfig.UartPciAddr;
> + // fill in some reasonable defaults
> + SerialPortInfo->BaseAddr = 0x3f8;
> + SerialPortInfo->RegWidth = 1;
> + SerialPortInfo->Type = 1; // uefi.SerialPortTypeIO
> + SerialPortInfo->Baud = 115200;
> + SerialPortInfo->InputHertz = 1843200;
> + SerialPortInfo->UartPciAddr = 0;
> +
> + const UefiPayloadConfig* config = GetUefiPayLoadConfig();
> + if (!config) {
> + DEBUG((DEBUG_ERROR, "ParseSerialInfo: using default config\n"));
> + return RETURN_SUCCESS;
> + }
> +
> + if (config->Version == UEFI_PAYLOAD_CONFIG_VERSION1) {
> + const UefiPayloadConfigV1* config1 = &config->config.v1;
> + SerialPortInfo->BaseAddr = config1->SerialConfig.BaseAddr;
> + SerialPortInfo->RegWidth = config1->SerialConfig.RegWidth;
> + SerialPortInfo->Type = config1->SerialConfig.Type;
> + SerialPortInfo->Baud = config1->SerialConfig.Baud;
> + SerialPortInfo->InputHertz = config1->SerialConfig.InputHertz;
> + SerialPortInfo->UartPciAddr = config1->SerialConfig.UartPciAddr;
> + } else
> +
> + if (config->Version == UEFI_PAYLOAD_CONFIG_VERSION2) {
> + const char* cmdline = config->config.v2.cmdline;
> + const char* option;
> + uint64_t args[6] = {};
> +
> + while ((cmdline = cmdline_next(cmdline, &option))) {
> + if (strncmp(option, "serial=", 7) != 0) continue;
> +
> + const int count = cmdline_ints(option, args, 6);
> + if (count < 0) {
> + DEBUG((DEBUG_ERROR, "Parse error: %a\n", option));
> + continue;
> + }
> +
> + if (count > 0) SerialPortInfo->Baud = args[0];
> + if (count > 1) SerialPortInfo->BaseAddr = args[1];
> + if (count > 2) SerialPortInfo->RegWidth = args[2];
> + if (count > 3) SerialPortInfo->Type = args[3];
> + if (count > 4) SerialPortInfo->InputHertz = args[4];
> + if (count > 5) SerialPortInfo->UartPciAddr = args[5];
> + }
> + }
>
> return RETURN_SUCCESS;
> }

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

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

#78091

Cancelled: TianoCore Design Meeting - APAC/NAMO

This event has been cancelled.

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

Where:
Microsoft Teams

Organizer: Ray Ni ray.ni@...

Description:

TOPIC

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

Calendar

[PATCH 3/3] IntelSiliconPkg: Add IgdOpRegion30.h to support IGD OpRegion v3.0

Digant H Solanki

#78090

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

- There are many OpRegion fields obsoleted in MBOX1
- MBOX2 is re-purposed for Backlight related fields for dual LFP.
- Backlight related fields moved to MBOX2 from MBOX3
and some fields are obsoleted in MBOX3.

Signed-off-by: Digant H Solanki <digant.h.solanki@...>
Cc: Ray Ni <ray.ni@...>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@...>
Cc: Ashraf Ali S <ashraf.ali.s@...>
---
 Silicon/Intel/IntelSiliconPkg/Include/IndustryStandard/IgdOpRegion30.h | 101 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/Silicon/Intel/IntelSiliconPkg/Include/IndustryStandard/IgdOpRegion30.h b/Silicon/Intel/IntelSiliconPkg/Include/IndustryStandard/IgdOpRegion30.h
new file mode 100644
index 0000000000..c9948ab55f
--- /dev/null
+++ b/Silicon/Intel/IntelSiliconPkg/Include/IndustryStandard/IgdOpRegion30.h
@@ -0,0 +1,101 @@
+/** @file
+  IGD OpRegion definition from Intel Integrated Graphics Device OpRegion
+  Specification based on version 3.0.
+
+  Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+#ifndef _IGD_OPREGION_3_0_H_
+#define _IGD_OPREGION_3_0_H_
+
+#include "IgdOpRegion.h"
+
+#define IGD_OPREGION_HEADER_MBOX2_VER_3_0 BIT5
+
+#pragma pack(1)
+///
+/// OpRegion Mailbox 1 - Public ACPI Methods
+/// Offset 0x100, Size 0x100
+///
+typedef struct {
+  UINT32 DRDY;          ///< Offset 0x100 Driver Readiness
+  UINT32 CSTS;          ///< Offset 0x104 Status
+  UINT32 CEVT;          ///< Offset 0x108 Current Event
+  UINT8  RM11[0x14];    ///< Offset 0x10C Reserved Must be Zero
+  UINT32 DIDL[8];       ///< Offset 0x120 Supported Display Devices ID List
+  UINT32 CPDL[8];       ///< Offset 0x140 obsolete
+  UINT32 CADL[8];       ///< Offset 0x160 obsolete
+  UINT32 NADL[8];       ///< Offset 0x180 obsolete
+  UINT32 ASLP;          ///< Offset 0x1A0 ASL Sleep Time Out
+  UINT32 TIDX;          ///< Offset 0x1A4 obsolete
+  UINT32 CHPD;          ///< Offset 0x1A8 obsolete
+  UINT32 CLID;          ///< Offset 0x1AC Current Lid State Indicator
+  UINT32 CDCK;          ///< Offset 0x1B0 Current Docking State Indicator
+  UINT32 SXSW;          ///< Offset 0x1B4 obsolete
+  UINT32 EVTS;          ///< Offset 0x1B8 obsolete
+  UINT32 CNOT;          ///< Offset 0x1BC obsolete
+  UINT32 NRDY;          ///< Offset 0x1C0 Driver Status
+  UINT8  DID2[0x1C];    ///< Offset 0x1C4 Extended Supported Devices ID List (DOD)
+  UINT8  CPD2[0x1C];    ///< Offset 0x1E0 obsolete
+  UINT8  RM12[4];       ///< Offset 0x1FC - 0x1FF Reserved Must be zero
+} IGD_OPREGION_MBOX1_VER_3_0;
+
+///
+/// OpRegion Mailbox 2 - Backlight communication
+/// Offset 0x200, Size 0x100
+///
+typedef struct {
+  UINT32 BCL1;          ///< Offset 0x200 Backlight Brightness for LFP1
+  UINT32 BCL2;          ///< Offset 0x204 Backlight Brightness for LFP2
+  UINT32 CBL1;          ///< Offset 0x208 Current User Brightness Level for LFP1
+  UINT32 CBL2;          ///< Offset 0x20C Current User Brightness Level for LFP2
+  UINT32 BCM1[0x1E];    ///< Offset 0x210 Backlight Brightness Levels Duty Cycle Mapping Table for LFP1
+  UINT32 BCM2[0x1E];    ///< Offset 0x288 Backlight Brightness Levels Duty Cycle Mapping Table for LFP2
+} IGD_OPREGION_MBOX2_VER_3_0;
+
+///
+/// OpRegion Mailbox 3 - BIOS/Driver Notification - ASLE Support
+/// Offset 0x300, Size 0x100
+///
+typedef struct {
+  UINT32 ARDY;          ///< Offset 0x300 obsolete
+  UINT32 ASLC;          ///< Offset 0x304 obsolete
+  UINT32 TCHE;          ///< Offset 0x308 obsolete
+  UINT32 ALSI;          ///< Offset 0x30C obsolete
+  UINT32 BCLP;          ///< Offset 0x310 obsoleted in ver 3.0, moved to Mailbox 2.
+  UINT32 PFIT;          ///< Offset 0x314 obsolete
+  UINT32 CBLV;          ///< Offset 0x318 obsoleted in ver 3.0, moved to Mailbox 2.
+  UINT16 BCLM[0x14];    ///< Offset 0x31C obsoleted in ver 3.0, moved to Mailbox 2.
+  UINT32 CPFM;          ///< Offset 0x344 obsolete
+  UINT32 EPFM;          ///< Offset 0x348 obsolete
+  UINT8  PLUT[0x4A];    ///< Offset 0x34C obsolete
+  UINT32 PFMB;          ///< Offset 0x396 obsolete
+  UINT32 CCDV;          ///< Offset 0x39A obsolete
+  UINT32 PCFT;          ///< Offset 0x39E obsolete
+  UINT32 SROT;          ///< Offset 0x3A2 obsolete
+  UINT32 IUER;          ///< Offset 0x3A6 obsolete
+  UINT64 FDSS;          ///< Offset 0x3AA obsolete
+  UINT32 FDSP;          ///< Offset 0x3B2 obsolete
+  UINT32 STAT;          ///< Offset 0x3B6 obsolete
+  UINT64 RVDA;          ///< Offset 0x3BA Physical address of Raw VBT data. Added from Spec Version 0.90 to support VBT greater than 6KB.
+  UINT32 RVDS;          ///< Offset 0x3C2 Size of Raw VBT data. Added from Spec Version 0.90 to support VBT greater than 6KB.
+  UINT8  VRSR;          ///< Offset 0x3C6 Video RAM Self Refresh
+  UINT64 DLHP;          ///< Offset 0x3C7 Dual LFP Hinge Alignment Parameters
+  UINT8  RM32[0x31];    ///< Offset 0x3CF - 0x3FF  Reserved Must be zero.
+} IGD_OPREGION_MBOX3_VER_3_0;
+
+///
+/// IGD OpRegion Structure
+///
+typedef struct {
+  IGD_OPREGION_HEADER         Header; ///< OpRegion header (Offset 0x0, Size 0x100)
+  IGD_OPREGION_MBOX1_VER_3_0  MBox1;  ///< Mailbox 1: Public ACPI Methods (Offset 0x100, Size 0x100)
+  IGD_OPREGION_MBOX2_VER_3_0  MBox2;  ///< Mailbox 2: Backlight communication (Offset 0x200, Size 0x100)
+  IGD_OPREGION_MBOX3_VER_3_0  MBox3;  ///< Mailbox 3: BIOS to Driver Notification (Offset 0x300, Size 0x100)
+  IGD_OPREGION_MBOX4          MBox4;  ///< Mailbox 4: Video BIOS Table (VBT) (Offset 0x400, Size 0x1800)
+  IGD_OPREGION_MBOX5          MBox5;  ///< Mailbox 5: BIOS to Driver Notification Extension (Offset 0x1C00, Size 0x400)
+} IGD_OPREGION_STRUCTURE_VER_3_0;
+#pragma pack()
+
+#endif
-- 
2.30.2.windows.1

Re: [edk2-platform][PATCH v1 1/1] Platform/RaspberryPi/RPi4: Fix non-standard ACPI HIDs

Ard Biesheuvel

#78089

On Wed, 21 Jul 2021 at 22:39, Andrei Warkentin <awarkentin@...> wrote:

Reviewed-by: Andrei Warkentin <awarkentin@...>Pushed as d549e39ca1a9..194269223294

Thanks all,

________________________________
From: Ard Biesheuvel <ardb@...>
Sent: Tuesday, July 20, 2021 10:37 AM
To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm <leif@...>; Ard Biesheuvel <ardb+tianocore@...>; Pete Batard <pete@...>; Andrei Warkentin <awarkentin@...>; Mario Bălănică <mariobalanica02@...>
Subject: Re: [edk2-platform][PATCH v1 1/1] Platform/RaspberryPi/RPi4: Fix non-standard ACPI HIDs

On Mon, 19 Jul 2021 at 22:45, Samer El-Haj-Mahmoud
<Samer.El-Haj-Mahmoud@...> wrote:

Remove non-standard RPI ACPI _CIDs that are not needed.
This also fixes the FWTS failure reported in
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fpftf%2FRPi4%2Fissues%2F67&;data=04%7C01%7Cawarkentin%40vmware.com%7Cbfd9e47da54c40ef9c5408d94b51486a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637623634750321947%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=YO4V6pksRlNqfPlMwO0VPKNVcp4npeP%2BN%2BpigTFfZnM%3D&amp;reserved=0

The windows drivers at https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fraspberrypi%2Fwindows-drivers&;data=04%7C01%7Cawarkentin%40vmware.com%7Cbfd9e47da54c40ef9c5408d94b51486a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637623634750321947%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=PMkBX%2F6vHaAoUC5tkCA7aAldaYEKFgh9w3yFYsgNrPQ%3D&amp;reserved=0
are still able to match the ACPI objects using the HIDs which
are supported in the drivers, with these two recent changes needed:
469702898789e555c6947e50216a3f79e0ddeb9
and
5c5e2742b4c983b3001c473b168b0dae2fcba0c2

Cc: Leif Lindholm <leif@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Pete Batard <pete@...>
Cc: Andrei Warkentin <awarkentin@...>
Cc: Mario Bălănică <mariobalanica02@...>
Signed-off-by: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>
Tested-by: Mario Bălănică <mariobalanica02@...>Glad to see this getting cleaned up.

Anyone care to ack?


---
 Platform/RaspberryPi/AcpiTables/GpuDevs.asl | 26 +++++++++++---------
 Platform/RaspberryPi/AcpiTables/Sdhc.asl    |  4 +--
 Platform/RaspberryPi/AcpiTables/Uart.asl    |  2 +-
 3 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/Platform/RaspberryPi/AcpiTables/GpuDevs.asl b/Platform/RaspberryPi/AcpiTables/GpuDevs.asl
index 966a94cdb5b5..9750dc25c07c 100644
--- a/Platform/RaspberryPi/AcpiTables/GpuDevs.asl
+++ b/Platform/RaspberryPi/AcpiTables/GpuDevs.asl
@@ -13,7 +13,11 @@
 Device (USB0)
 {
   Name (_HID, "BCM2848")
-  Name (_CID, Package() { "DWC_OTG", "DWC2_OTG" })
+#if (RPI_MODEL == 3)
+  Name (_CID, "DWC_OTG")
+#elif (RPI_MODEL == 4)
+  Name (_CID, "BCM2848")
+#endif
   Name (_UID, 0x0)
   Name (_CCA, 0x0)
   Method (_STA)
@@ -36,7 +40,7 @@ Device (USB0)
 Device (GPU0)
 {
   Name (_HID, "BCM2850")
-  Name (_CID, "VC4")
+  Name (_CID, "BCM2850")
   Name (_UID, 0x0)
   Name (_CCA, 0x0)
   Method (_STA)
@@ -140,7 +144,7 @@ Device (GPU0)
 Device (RPIQ)
 {
   Name (_HID, "BCM2849")
-  Name (_CID, "RPIQ")
+  Name (_CID, "BCM2849")
   Name (_UID, 0)
   Name (_CCA, 0x0)
   Method (_STA)
@@ -164,7 +168,7 @@ Device (RPIQ)
 Device (VCIQ)
 {
   Name (_HID, "BCM2835")
-  Name (_CID, "VCIQ")
+  Name (_CID, "BCM2835")
   Name (_UID, 0)
   Name (_CCA, 0x0)
   Name (_DEP, Package() { \_SB.GDV0.RPIQ })
@@ -189,7 +193,7 @@ Device (VCIQ)
 Device (VCSM)
 {
   Name (_HID, "BCM2856")
-  Name (_CID, "VCSM")
+  Name (_CID, "BCM2856")
   Name (_UID, 0)
   Name (_CCA, 0x0)
   Name (_DEP, Package() { \_SB.GDV0.VCIQ })
@@ -203,7 +207,7 @@ Device (VCSM)
 Device (GPI0)
 {
   Name (_HID, "BCM2845")
-  Name (_CID, "BCMGPIO")
+  Name (_CID, "BCM2845")
   Name (_UID, 0x0)
   Name (_CCA, 0x0)
   Method (_STA)
@@ -230,7 +234,7 @@ Device (GPI0)
 Device (I2C1)
 {
   Name (_HID, "BCM2841")
-  Name (_CID, "BCMI2C")
+  Name (_CID, "BCM2841")
   Name (_UID, 0x1)
   Name (_CCA, 0x0)
   Method (_STA)
@@ -254,7 +258,7 @@ Device (I2C1)
 Device (I2C2)
 {
   Name (_HID, "BCM2841")
-  Name (_CID, "BCMI2C")
+  Name (_CID, "BCM2841")
   Name (_UID, 0x2)
   Name (_CCA, 0x0)
   Method (_STA)
@@ -278,7 +282,7 @@ Device (I2C2)
 Device (SPI0)
 {
   Name (_HID, "BCM2838")
-  Name (_CID, "BCMSPI0")
+  Name (_CID, "BCM2838")
   Name (_UID, 0x0)
   Name (_CCA, 0x0)
   Method (_STA)
@@ -304,7 +308,7 @@ Device (SPI0)
 Device (SPI1)
 {
   Name (_HID, "BCM2839")
-  Name (_CID, "BCMAUXSPI")
+  Name (_CID, "BCM2839")
   Name (_UID, 0x1)
   Name (_CCA, 0x0)
   Name (_DEP, Package() { \_SB.GDV0.RPIQ })
@@ -331,7 +335,7 @@ Device (SPI1)
 // Device (SPI2)
 // {
 //   Name (_HID, "BCM2839")
-//   Name (_CID, "BCMAUXSPI")
+//   Name (_CID, "BCM2839")
 //   Name (_UID, 0x2)
 //   Name (_CCA, 0x0)
 //   Name (_DEP, Package() { \_SB.GDV0.RPIQ })
diff --git a/Platform/RaspberryPi/AcpiTables/Sdhc.asl b/Platform/RaspberryPi/AcpiTables/Sdhc.asl
index 42776e33bbc6..85d5053a338c 100644
--- a/Platform/RaspberryPi/AcpiTables/Sdhc.asl
+++ b/Platform/RaspberryPi/AcpiTables/Sdhc.asl
@@ -23,7 +23,7 @@
 Device (SDC1)
 {
   Name (_HID, "BCM2847")
-  Name (_CID, "ARASAN")
+  Name (_CID, "BCM2847")
   Name (_UID, 0x0)
   Name (_CCA, 0x0)
   Name (_S1D, 0x1)
@@ -78,7 +78,7 @@ Device (SDC1)
 Device (SDC2)
 {
   Name (_HID, "BCM2855")
-  Name (_CID, "SDHST")
+  Name (_CID, "BCM2855")
   Name (_UID, 0x0)
   Name (_CCA, 0x0)
   Name (_S1D, 0x1)
diff --git a/Platform/RaspberryPi/AcpiTables/Uart.asl b/Platform/RaspberryPi/AcpiTables/Uart.asl
index 167f94e8892b..974f06d3bc3f 100644
--- a/Platform/RaspberryPi/AcpiTables/Uart.asl
+++ b/Platform/RaspberryPi/AcpiTables/Uart.asl
@@ -59,7 +59,7 @@ Device (URT0)
 Device (URTM)
 {
   Name (_HID, "BCM2836")
-  Name (_CID, "MINIUART")
+  Name (_CID, "BCM2836")
   Name (_UID, 0x0)
   Name (_CCA, 0x0)
   Method (_STA)
--
2.25.1

Re: RFC: EXT4 filesystem driver

Marvin Häuser <mhaeuser@...>

#78088

On 22.07.21 01:12, Pedro Falcato wrote:
EXT4 (fourth extended filesystem) is a filesystem developed for Linux
that has been in wide use (desktops, servers, smartphones) since 2008.

The Ext4Pkg implements the Simple File System Protocol for a partition
that is formatted with the EXT4 file system. This allows UEFI Drivers,
UEFI Applications, UEFI OS Loaders, and the UEFI Shell to access files
on an EXT4 partition and supports booting a UEFI OS Loader from an
EXT4 partition.
This project is one of the TianoCore Google Summer of Code projects.

Right now, Ext4Pkg only contains a single member, Ext4Dxe, which is a
UEFI driver that consumes Block I/O, Disk I/O and (optionally) Disk
I/O 2 Protocols, and produces the Simple File System protocol. It
allows mounting ext4 filesystems exclusively.

Brief overhead of EXT4:
Layout of an EXT2/3/4 filesystem:
(note: this driver has been developed using
https://www.kernel.org/doc/html/latest/filesystems/ext4/index.html as
documentation).

An ext2/3/4 filesystem (here on out referred to as simply an ext4 filesystem,
due to the similarities) is composed of various concepts:

1) Superblock
The superblock is the structure near (1024 bytes offset from the start)
the start of the partition, and describes the filesystem in general.
Here, we get to know the size of the filesystem's blocks, which features
it supports or not, whether it's been cleanly unmounted, how many blocks
we have, etc.

2) Block groups
EXT4 filesystems are divided into block groups, and each block group covers
s_blocks_per_group(8 * Block Size) blocks. Each block group has an
associated block group descriptor; these are present directly after the
superblock. Each block group descriptor contains the location of the
inode table, and the inode and block bitmaps (note these bitmaps are only
a block long, which gets us the 8 * Block Size formula covered previously).

3) Blocks
The ext4 filesystem is divided into blocks, of size s_log_block_size ^ 1024.
Blocks can be allocated using individual block groups's bitmaps. Note
that block 0 is invalid and its presence on extents/block tables means
it's part of a file hole, and that particular location must be read as
a block full of zeros.

4) Inodes
The ext4 filesystem divides files/directories into inodes (originally
index nodes). Each file/socket/symlink/directory/etc (here on out referred
to as a file, since there is no distinction under the ext4 filesystem) is
stored as a /nameless/ inode, that is stored in some block group's inode
table. Each inode has s_inode_size size (or GOOD_OLD_INODE_SIZE if it's
an old filesystem), and holds various metadata about the file. Since the
largest inode structure right now is ~160 bytes, the rest of the inode
contains inline extended attributes. Inodes' data is stored using either
data blocks (under ext2/3) or extents (under ext4).

5) Extents
Ext4 inodes store data in extents. These let N contiguous logical blocks
that are represented by N contiguous physical blocks be represented by a
single extent structure, which minimizes filesystem metadata bloat and
speeds up block mapping (particularly due to the fact that high-quality
ext4 implementations like linux's try /really/ hard to make the file
contiguous, so it's common to have files with almost 0 fragmentation).
Inodes that use extents store them in a tree, and the top of the tree
is stored on i_data. The tree's leaves always start with an
EXT4_EXTENT_HEADER and contain EXT4_EXTENT_INDEX on eh_depth != 0 and
EXT4_EXTENT on eh_depth = 0; these entries are always sorted by logical
block.

6) Directories
Ext4 directories are files that store name -> inode mappings for the
logical directory; this is where files get their names, which means ext4
inodes do not themselves have names, since they can be linked (present)
multiple times with different names. Directories can store entries in two
different ways:
1) Classical linear directories: They store entries as a mostly-linked
mostly-list of EXT4_DIR_ENTRY.
2) Hash tree directories: These are used for larger directories, with
hundreds of entries, and are designed in a backwards-compatible way.
These are not yet implemented in the Ext4Dxe driver.

7) Journal
Ext3/4 filesystems have a journal to help protect the filesystem against
system crashes. This is not yet implemented in Ext4Dxe but is described
in detail in the Linux kernel's documentation.

The EDK2 implementation of ext4 is based only on the public documentation
available at https://www.kernel.org/doc/html/latest/filesystems/ext4/index.html
and
the FreeBSD ext2fs driver (available at
https://github.com/freebsd/freebsd-src/tree/main/sys/fs/ext2fs,
BSD-2-Clause-FreeBSD licensed). It is licensed as
SPDX-License-Identifier: BSD-2-Clause-Patent.

After a brief discussion with the community, the proposed package
location is edk2-platform/Features/Ext4Pkg
(relevant discussion: https://edk2.groups.io/g/devel/topic/83060185).

I was the main contributor and I would like to maintain the package in
the future, if possible.While I personally don't like it's outside of the EDK II core, I kind of get it. However I would strongly suggest to choose a more general package name, like "LinuxFsPkg", or "NixFsPkg", or maybe even just "FileSystemPkg" (and move FAT over some day?). Imagine someone wants to import BTRFS next year, should it really be "BtrfsPkg"? I understand it follows the "FatPkg" convention, but I feel like people forget FatPkg was special as to its awkward license before Microsoft allowed a change a few years ago. Maintainers.txt already has the concept of different Reviewers per subfolder, maybe it could be extended a little to have a common EDK II contributor to officially maintain the package, but have you be a Maintainer or something like a Reviewer+ to your driver? Or you could maintain the entire package of course.

Current limitations:
1) The Ext4Dxe driver is, at the moment, read-only.
2) The Ext4Dxe driver at the moment cannot mount older (ext2/3)
filesystems. Ensuring compatibility with
those may not be a bad idea.

I intend to test the package using the UEFI SCTs present in edk2-test,
and implement any other needed unit tests myself using the already
available unit test framework. I also intend to (privately) fuzz the
UEFI driver with bad/unusual disk images, to improve the security and
reliability of the driver.

In the future, ext4 write support should be added so edk2 has a
fully-featured RW ext4 driver. There could also be a focus on
supporting the older ext4-like filesystems, as I mentioned in the
limitations, but that is open for discussion.I may be alone, but I strongly object. One of our projects (OpenCore) has a disgusting way of writing files because the FAT32 driver in Aptio IV firmwares may corrupt the filesystem when resizing files. To be honest, it may corrupt with other usages too and we never noticed, because basically we wrote the code to require the least amount of (complex) FS operations.

The issue with EDK II is, there is a lot of own code and a lot of users, but little testing. By that I do not mean that developers do not test their code, but that nobody sits down and performs all sorts of FS manipulations in all sorts of orders and closely observes the result for regression-testing. Users will not really test it either, as UEFI to them should just somehow boot to Windows. If at least the code was shared with a codebase that is known-trusted (e.g. the Linux kernel itself), that'd be much easier to trust, but realistically this is not going to happen.
My point is, if a company like AMI cannot guarantee writing does not damage the FS for a very simple FS, how do you plan to guarantee yours doesn't for a much more complex FS? I'd rather have only one simple FS type that supports writing for most use-cases (e.g. logging).

At the very least I would beg you to have a PCD to turn write support off - if it will be off by default, that'd be great of course. :)
Was there any discussion yet as to why write support is needed in the first place you could point me to?

Thanks for your work!

Best regards,
Marvin

The driver's handling of unclean unmounting through forced shutdown is unclear.
Is there a position in edk2 on how to handle such cases? I don't think
FAT32 has a "this filesystem is/was dirty" and even though it seems to
me that stopping a system from booting/opening the partition because
"we may find some tiny irregularities" is not the best course of
action, I can't find a clear answer.

The driver also had to add implementations of CRC32C and CRC16, and
after talking with my mentor we quickly reached the conclusion that
these may be good candidates for inclusion in MdePkg. We also
discussed moving the Ucs2 <-> Utf8 conversion library in RedfishPkg
(BaseUcs2Utf8Lib) into MdePkg as well. Any comments?

Feel free to ask any questions you may find relevant.

Best Regards,

Pedro Falcato

Re: [PATCH 1/1] MdePkg: add definition of LINUX_EFI_INITRD_MEDIA_GUID

Ard Biesheuvel

#78087

On Wed, 21 Jul 2021 at 20:04, Kinney, Michael D <michael.d.kinney@...> wrote:

Hi Jeff,

I see. I missed the file rename line in the git patch.

I think the description needs to be expanded to clearly describe the production and consumption of this device path with this GUID.

What component creates the UEFI handle with the Device Path Protocol and the LoadFile2 Protocol with the initrd image? Is it the platform FW or the OS Loader? If it is the platform FW, then how does the platform FW know which initrd image to publish if there are multiple Linux OSes installed?

This is really the firmware/loader's problem. In the ARM / RISC-V world, the distinction between firmware and pre-OS loader is not as clear cut, and for instance, U-boot in EFI mode can either boot the kernel directly (and expose the initrd via this method), or invoke GRUB as an EFI app using LoadImage/StartImage, in which case GRUB can load the kernel and/or initrd via whichever interface it desires.

It is therefore also the platform FW's problem to decide which initrd goes with which kernel - it is highly platform dependent whether a certain initrd is compatible with only a single kernel, or can be combined with any kernel (e.g., when the kernel has all drivers builtin, and the initrd only contains the user space)

What component locates the UEFI handle with the Device Path Protocol and the LoadFile2 Protocol with the initrd image? It is another stage of the OS Loader or the OS Kernel? Given that these handles are only available before ExitBootServices, I think this means that the component that locates the initrd image has to do so before ExitBootServices is called.

The consumer of the protocol is the EFI stub loader in Linux, i.e., the OS loader that is built into the OS kernel. It is the agent that calls ExitBootServices(), and it indeed consumes the protocol beforehand.

In summary, I think it should be sufficient to describe the consumer's expectations with respect to the API. I don't think it makes sense to be normative about how platform firmware or intermediate loaders keep track of which file to expose, as long as it complies with the consumer's requirements.

To: Kinney, Michael D <michael.d.kinney@...>; Ard Biesheuvel <ardb@...>
Cc: devel@edk2.groups.io; ardb+tianocore@...; Justen, Jordan L <jordan.l.justen@...>; gaoliming@...; Liu, Zhiguang <zhiguang.liu@...>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>
Subject: Re: [edk2-devel] [PATCH 1/1] MdePkg: add definition of LINUX_EFI_INITRD_MEDIA_GUID

Does this look good for text to add

"Linux distro boot generally relies on an initial ramdisk (initrd)
which is provided by the loader, and which contains additional kernel
modules (for storage and network, for instance), and the initial user
space startup code, i.e., the code which brings up the user space side
of the entire OS.

In order to provide a standard method to locate this file,

the GUID defined in this file is used to describe the device path

for a LoadFile2 Protocol instance that is responsible for loading the initrd file"

Also, the patch does have

{OvmfPkg => MdePkg}/Include/Guid/LinuxEfiInitrdMedia.h | 0
3 files changed, 5 insertions(+), 1 deletion(-)
rename {OvmfPkg => MdePkg}/Include/Guid/LinuxEfiInitrdMedia.h (100%)
[snip]
diff --git a/OvmfPkg/Include/Guid/LinuxEfiInitrdMedia.h b/MdePkg/Include/Guid/LinuxEfiInitrdMedia.h
similarity index 100%
rename from OvmfPkg/Include/Guid/LinuxEfiInitrdMedia.h
rename to MdePkg/Include/Guid/LinuxEfiInitrdMedia.h

Thanks,

Jeff

From: Kinney, Michael D <michael.d.kinney@...>
Sent: Wednesday, July 21, 2021 9:38 AM
To: Jeff Brasen <jbrasen@...>; Ard Biesheuvel <ardb@...>; Kinney, Michael D <michael.d.kinney@...>
Cc: devel@edk2.groups.io <devel@edk2.groups.io>; ardb+tianocore@... <ardb+tianocore@...>; Justen, Jordan L <jordan.l.justen@...>; gaoliming@... <gaoliming@...>; Liu, Zhiguang <zhiguang.liu@...>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>
Subject: RE: [edk2-devel] [PATCH 1/1] MdePkg: add definition of LINUX_EFI_INITRD_MEDIA_GUID

External email: Use caution opening links or attachments

Hi Ard,

If this device path node is considered as part of the standard interface between the Linux kernel and

firmware, then it does make sense for it to be in the MdePkg. We usually try to reference a public

specification in the include file that defines the interface.

In this case, since there is no public document, but it is part of the Linux kernel assumptions,

can the include file for the GUID provide pointers to the Linux kernel that uses the GUID and

describe how the GUID is produced by the FW and consumed by the Linux kernel?

I also see that this patch appears to be incomplete. There is an OvmfPkg/Include/Guid/LinuxEfiInitrdMedia.h

file in the OvmfPkg. Shouldn’t that file also be moved to the MdePkg as part of this patch?

Thanks,

Mike

From: Jeff Brasen <jbrasen@...>
Sent: Tuesday, July 20, 2021 9:59 AM
To: Ard Biesheuvel <ardb@...>; Kinney, Michael D <michael.d.kinney@...>
Cc: devel@edk2.groups.io; ardb+tianocore@...; Justen, Jordan L <jordan.l.justen@...>; gaoliming@...; Liu, Zhiguang <zhiguang.liu@...>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>
Subject: Re: [edk2-devel] [PATCH 1/1] MdePkg: add definition of LINUX_EFI_INITRD_MEDIA_GUID

In my opinion MdePkg is where this should be as it is meant to be used by multiple software entities (linux kernel, grub, edk2, coreboot w/ uefi binding) and probably should be documented in some spec (Although, I am not sure which one would make sense)

I am fine with MdeModulePkg as well though.

Thanks,

Jeff

From: Ard Biesheuvel <ardb@...>
Sent: Friday, July 16, 2021 9:56 AM
To: Kinney, Michael D <michael.d.kinney@...>
Cc: Jeff Brasen <jbrasen@...>; devel@edk2.groups.io <devel@edk2.groups.io>; ardb+tianocore@... <ardb+tianocore@...>; Justen, Jordan L <jordan.l.justen@...>; gaoliming@... <gaoliming@...>; Liu, Zhiguang <zhiguang.liu@...>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>
Subject: Re: [edk2-devel] [PATCH 1/1] MdePkg: add definition of LINUX_EFI_INITRD_MEDIA_GUID

External email: Use caution opening links or attachments

On Fri, 16 Jul 2021 at 17:00, Kinney, Michael D
<michael.d.kinney@...> wrote:
>
> Hi Ard,
>
> I see you were involved in the OS side changes.
>
> Can you explain what is required for the FW <-> OS interface with respect to Load File Protocol and this media device path node.
>
> What happens if this media device path node is not present? What breaks?
>
> Trying to figure out if this is a required interop feature (MdePkg candidate) or an EDK II specific extension (MdeModulePkg candidate).
>

Let me give some context first:

Linux distro boot generally relies on an initial ramdisk (initrd)
which is provided by the loader, and which contains additional kernel
modules (for storage and netwerk, for instance), and the initial user
space startup code, ie., the code which brings up the user space side
of the entire OS.

Before we introduced this media path, the only way for a EFI pre-OS
loader (such as GRUB) to provide this initrd was to copy it into DRAM
somewhere, and use a arch-specific method of passing the DRAM address
and size to the OS (x86 uses struct bootparam, whereas ARM uses device
tree). It also requires knowledge on the part of GRUB regarding which
parts of DRAM are suitable for holding an initrd image. For measured
boot scenarios, it may be an advantage not to have the initrd linger
in DRAM for longer that necessary, and we actually intend to measure
the initrd loaded via the new method right after it has been loaded
this way.

To avoid extending this to other architectures such as RISC-V, I
decided to introduce a special vendor media path for Linux initrd
images, which GRUB et al can implement, which provides the initrd
image when the OS loader that consumes it asks for it.

So for Linux on x86 or ARM, this is optional, given that support for
the old method is not going away any time soon. For RISC-V, I
suggested that only the new method be implemented, but I am not sure
what the status is there. Note that many embedded style systems don't
use GRUB, and may not use initrds to begin with. OTOH, U-Boot also
implements support for the Linux initrd vendor media path, and work is
ongoing to add measured boot support as well.

In any case, I don't have a strong preference where this should live,
as long as it is in a generic place where all architectures can use
it.

--
Ard.

Re: [PATCH v4 10/11] OvmfPkg: add BlobVerifierLibSevHashes

Dov Murik

#78086

Here's the diff from the v3 of this patch. It's supposed to catch
more cases of bad length fields overflowing the reserved MEMFD space or
the declared length of the table.



diff --git a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
index 797d63d18067..372ae6f469e7 100644
--- a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
+++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
@@ -94,7 +94,7 @@ VerifyBlob (
   )
 {
   CONST GUID *Guid;
-  INT32 Len;
+  INT32 Remaining;
   HASH_TABLE *Entry;

   if (mHashesTable == NULL || mHashesTableSize == 0) {
@@ -111,9 +111,13 @@ VerifyBlob (
     return EFI_ACCESS_DENIED;
   }

-  for (Entry = mHashesTable, Len = 0;
-       Len < (INT32)mHashesTableSize;
-       Len += Entry->Len,
+  //
+  // Remaining is INT32 to catch underflow in case Entry->Len has a
+  // very high UINT16 value
+  //
+  for (Entry = mHashesTable, Remaining = mHashesTableSize;
+       Remaining >= sizeof *Entry && Remaining >= Entry->Len;
+       Remaining -= Entry->Len,
        Entry = (HASH_TABLE *)((UINT8 *)Entry + Entry->Len)) {
     UINTN EntrySize;
     EFI_STATUS Status;
@@ -125,7 +129,7 @@ VerifyBlob (

     DEBUG ((DEBUG_INFO, "%a: Found GUID %g in table\n", __FUNCTION__,
Guid));

-    EntrySize = Entry->Len - sizeof (Entry->Guid) - sizeof (Entry->Len);
+    EntrySize = Entry->Len - sizeof Entry->Guid - sizeof Entry->Len;
     if (EntrySize != SHA256_DIGEST_SIZE) {
       DEBUG ((DEBUG_ERROR, "%a: Hash has the wrong size %d != %d\n",
         __FUNCTION__, EntrySize, SHA256_DIGEST_SIZE));
@@ -161,7 +165,8 @@ VerifyBlob (
   This function always returns success, even if the table can't be
found.  The
   subsequent VerifyBlob calls will fail if no table was found.

-  @retval RETURN_SUCCESS   The verifier tables were set up correctly
+  @retval RETURN_SUCCESS   The hashes table is set up correctly, or
there is no
+                           hashes table
 **/
 RETURN_STATUS
 EFIAPI
@@ -175,15 +180,9 @@ BlobVerifierLibSevHashesConstructor (
   mHashesTable = NULL;
   mHashesTableSize = 0;

-  if (Ptr == NULL || Size == 0) {
-    return RETURN_SUCCESS;
-  }
-
-  if (!CompareGuid (&Ptr->Guid, &SEV_HASH_TABLE_GUID)) {
-    return RETURN_SUCCESS;
-  }
-
-  if (Ptr->Len < (sizeof Ptr->Guid + sizeof Ptr->Len)) {
+  if (Ptr == NULL || Size < sizeof *Ptr ||
+      !CompareGuid (&Ptr->Guid, &SEV_HASH_TABLE_GUID) ||
+      Ptr->Len < sizeof *Ptr || Ptr->Len > Size) {
     return RETURN_SUCCESS;
   }

Show quoted text

On 22/07/2021 11:43, Dov Murik wrote:
Add an implementation for BlobVerifierLib that locates the SEV hashes
table and verifies that the calculated hashes of the kernel, initrd, and
cmdline blobs indeed match the expected hashes stated in the hashes
table.

If there's a missing hash or a hash mismatch then EFI_ACCESS_DENIED is
returned which will cause a failure to load a kernel image.

Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Ashish Kalra <ashish.kalra@...>
Cc: Brijesh Singh <brijesh.singh@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: James Bottomley <jejb@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Min Xu <min.m.xu@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
Co-developed-by: James Bottomley <jejb@...>
Signed-off-by: James Bottomley <jejb@...>
Signed-off-by: Dov Murik <dovmurik@...>
---
 OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf |  37 ++++
 OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c      | 199 ++++++++++++++++++++
 2 files changed, 236 insertions(+)

diff --git a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
new file mode 100644
index 000000000000..76ca0b8154ce
--- /dev/null
+++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
@@ -0,0 +1,37 @@
+## @file

+#

+#  Blob verifier library that uses SEV hashes table.  The hashes table holds the

+#  allowed hashes of the kernel, initrd, and cmdline blobs.

+#

+#  Copyright (C) 2021, IBM Corp

+#

+#  SPDX-License-Identifier: BSD-2-Clause-Patent

+#

+##

+

+[Defines]

+  INF_VERSION                    = 1.29

+  BASE_NAME                      = BlobVerifierLibSevHashes

+  FILE_GUID                      = 59e713b5-eff3-46a7-8d8b-46f4c004ad7b

+  MODULE_TYPE                    = BASE

+  VERSION_STRING                 = 1.0

+  LIBRARY_CLASS                  = BlobVerifierLib

+  CONSTRUCTOR                    = BlobVerifierLibSevHashesConstructor

+

+[Sources]

+  BlobVerifierSevHashes.c

+

+[Packages]

+  CryptoPkg/CryptoPkg.dec

+  MdePkg/MdePkg.dec

+  OvmfPkg/OvmfPkg.dec

+

+[LibraryClasses]

+  BaseCryptLib

+  BaseMemoryLib

+  DebugLib

+  PcdLib

+

+[FixedPcd]

+  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase

+  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize

diff --git a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
new file mode 100644
index 000000000000..372ae6f469e7
--- /dev/null
+++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
@@ -0,0 +1,199 @@
+/** @file

+

+  Blob verifier library that uses SEV hashes table.  The hashes table holds the

+  allowed hashes of the kernel, initrd, and cmdline blobs.

+

+  Copyright (C) 2021, IBM Corporation

+

+  SPDX-License-Identifier: BSD-2-Clause-Patent

+**/

+

+#include <Library/BaseCryptLib.h>

+#include <Library/BaseLib.h>

+#include <Library/BaseMemoryLib.h>

+#include <Library/DebugLib.h>

+#include <Library/BlobVerifierLib.h>

+

+/**

+  The SEV Hashes table must be in encrypted memory and has the table

+  and its entries described by

+

+  <GUID>|UINT16 <len>|<data>

+

+  With the whole table GUID being 9438d606-4f22-4cc9-b479-a793d411fd21

+

+  The current possible table entries are for the kernel, the initrd

+  and the cmdline:

+

+  4de79437-abd2-427f-b835-d5b172d2045b  kernel

+  44baf731-3a2f-4bd7-9af1-41e29169781d  initrd

+  97d02dd8-bd20-4c94-aa78-e7714d36ab2a  cmdline

+

+  The size of the entry is used to identify the hash, but the

+  expectation is that it will be 32 bytes of SHA-256.

+**/

+

+#define SEV_HASH_TABLE_GUID \

+  (GUID) { 0x9438d606, 0x4f22, 0x4cc9, { 0xb4, 0x79, 0xa7, 0x93, 0xd4, 0x11, 0xfd, 0x21 } }

+#define SEV_KERNEL_HASH_GUID \

+  (GUID) { 0x4de79437, 0xabd2, 0x427f, { 0xb8, 0x35, 0xd5, 0xb1, 0x72, 0xd2, 0x04, 0x5b } }

+#define SEV_INITRD_HASH_GUID \

+  (GUID) { 0x44baf731, 0x3a2f, 0x4bd7, { 0x9a, 0xf1, 0x41, 0xe2, 0x91, 0x69, 0x78, 0x1d } }

+#define SEV_CMDLINE_HASH_GUID \

+  (GUID) { 0x97d02dd8, 0xbd20, 0x4c94, { 0xaa, 0x78, 0xe7, 0x71, 0x4d, 0x36, 0xab, 0x2a } }

+

+STATIC CONST EFI_GUID mSevKernelHashGuid = SEV_KERNEL_HASH_GUID;

+STATIC CONST EFI_GUID mSevInitrdHashGuid = SEV_INITRD_HASH_GUID;

+STATIC CONST EFI_GUID mSevCmdlineHashGuid = SEV_CMDLINE_HASH_GUID;

+

+#pragma pack (1)

+typedef struct {

+  GUID   Guid;

+  UINT16 Len;

+  UINT8  Data[];

+} HASH_TABLE;

+#pragma pack ()

+

+STATIC HASH_TABLE *mHashesTable;

+STATIC UINT16 mHashesTableSize;

+

+STATIC

+CONST GUID*

+FindBlobEntryGuid (

+  IN  CONST CHAR16    *BlobName

+  )

+{

+  if (StrCmp (BlobName, L"kernel") == 0) {

+    return &mSevKernelHashGuid;

+  } else if (StrCmp (BlobName, L"initrd") == 0) {

+    return &mSevInitrdHashGuid;

+  } else if (StrCmp (BlobName, L"cmdline") == 0) {

+    return &mSevCmdlineHashGuid;

+  } else {

+    return NULL;

+  }

+}

+

+/**

+  Verify blob from an external source.

+

+  @param[in] BlobName           The name of the blob

+  @param[in] Buf                The data of the blob

+  @param[in] BufSize            The size of the blob in bytes

+

+  @retval EFI_SUCCESS           The blob was verified successfully.

+  @retval EFI_ACCESS_DENIED     The blob could not be verified, and therefore

+                                should be considered non-secure.

+**/

+EFI_STATUS

+EFIAPI

+VerifyBlob (

+  IN  CONST CHAR16    *BlobName,

+  IN  CONST VOID      *Buf,

+  IN  UINT32          BufSize

+  )

+{

+  CONST GUID *Guid;

+  INT32 Remaining;

+  HASH_TABLE *Entry;

+

+  if (mHashesTable == NULL || mHashesTableSize == 0) {

+    DEBUG ((DEBUG_ERROR,

+      "%a: Verifier called but no hashes table discoverd in MEMFD\n",

+      __FUNCTION__));

+    return EFI_ACCESS_DENIED;

+  }

+

+  Guid = FindBlobEntryGuid (BlobName);

+  if (Guid == NULL) {

+    DEBUG ((DEBUG_ERROR, "%a: Unknown blob name \"%s\"\n", __FUNCTION__,

+      BlobName));

+    return EFI_ACCESS_DENIED;

+  }

+

+  //

+  // Remaining is INT32 to catch underflow in case Entry->Len has a

+  // very high UINT16 value

+  //

+  for (Entry = mHashesTable, Remaining = mHashesTableSize;

+       Remaining >= sizeof *Entry && Remaining >= Entry->Len;

+       Remaining -= Entry->Len,

+       Entry = (HASH_TABLE *)((UINT8 *)Entry + Entry->Len)) {

+    UINTN EntrySize;

+    EFI_STATUS Status;

+    UINT8 Hash[SHA256_DIGEST_SIZE];

+

+    if (!CompareGuid (&Entry->Guid, Guid)) {

+      continue;

+    }

+

+    DEBUG ((DEBUG_INFO, "%a: Found GUID %g in table\n", __FUNCTION__, Guid));

+

+    EntrySize = Entry->Len - sizeof Entry->Guid - sizeof Entry->Len;

+    if (EntrySize != SHA256_DIGEST_SIZE) {

+      DEBUG ((DEBUG_ERROR, "%a: Hash has the wrong size %d != %d\n",

+        __FUNCTION__, EntrySize, SHA256_DIGEST_SIZE));

+      return EFI_ACCESS_DENIED;

+    }

+

+    //

+    // Calculate the buffer's hash and verify that it is identical to the

+    // expected hash table entry

+    //

+    Sha256HashAll (Buf, BufSize, Hash);

+

+    if (CompareMem (Entry->Data, Hash, EntrySize) == 0) {

+      Status = EFI_SUCCESS;

+      DEBUG ((DEBUG_INFO, "%a: Hash comparison succeeded for \"%s\"\n",

+        __FUNCTION__, BlobName));

+    } else {

+      Status = EFI_ACCESS_DENIED;

+      DEBUG ((DEBUG_ERROR, "%a: Hash comparison failed for \"%s\"\n",

+        __FUNCTION__, BlobName));

+    }

+    return Status;

+  }

+

+  DEBUG ((DEBUG_ERROR, "%a: Hash GUID %g not found in table\n", __FUNCTION__,

+    Guid));

+  return EFI_ACCESS_DENIED;

+}

+

+/**

+  Locate the SEV hashes table.

+

+  This function always returns success, even if the table can't be found.  The

+  subsequent VerifyBlob calls will fail if no table was found.

+

+  @retval RETURN_SUCCESS   The hashes table is set up correctly, or there is no

+                           hashes table

+**/

+RETURN_STATUS

+EFIAPI

+BlobVerifierLibSevHashesConstructor (

+  VOID

+  )

+{

+  HASH_TABLE *Ptr = (void *)(UINTN)FixedPcdGet64 (PcdQemuHashTableBase);

+  UINT32 Size = FixedPcdGet32 (PcdQemuHashTableSize);

+

+  mHashesTable = NULL;

+  mHashesTableSize = 0;

+

+  if (Ptr == NULL || Size < sizeof *Ptr ||

+      !CompareGuid (&Ptr->Guid, &SEV_HASH_TABLE_GUID) ||

+      Ptr->Len < sizeof *Ptr || Ptr->Len > Size) {

+    return RETURN_SUCCESS;

+  }

+

+  DEBUG ((DEBUG_INFO, "%a: Found injected hashes table in secure location\n",

+    __FUNCTION__));

+

+  mHashesTable = (HASH_TABLE *)Ptr->Data;

+  mHashesTableSize = Ptr->Len - sizeof Ptr->Guid - sizeof Ptr->Len;

+

+  DEBUG ((DEBUG_VERBOSE, "%a: mHashesTable=0x%p, Size=%u\n", __FUNCTION__,

+    mHashesTable, mHashesTableSize));

+

+  return RETURN_SUCCESS;

+}

[PATCH v4 02/11] OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds

Dov Murik

#78085

Newer kernels support efistub and therefore don't need all the legacy
stuff in X86QemuLoadImageLib, which are harder to secure.  Specifically
the verification of kernel/initrd/cmdline blobs will be added only to
the GenericQemuLoadImageLib implementation, so use that for SEV builds.

Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Ashish Kalra <ashish.kalra@...>
Cc: Brijesh Singh <brijesh.singh@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: James Bottomley <jejb@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Min Xu <min.m.xu@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3457
Signed-off-by: Dov Murik <dovmurik@...>
Reviewed-by: Brijesh Singh <brijesh.singh@...>
---
 OvmfPkg/AmdSev/AmdSevX64.dsc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index 1d487befae08..a2f1324c40a6 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -368,7 +368,7 @@ [LibraryClasses.common.DXE_DRIVER]
   PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf=0D
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf=0D
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf=
=0D
-  QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib=
.inf=0D
+  QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoad=
ImageLib.inf=0D
 !if $(TPM_ENABLE) =3D=3D TRUE=0D
   Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibTcg.i=
nf=0D
   Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.in=
f=0D
--=20
2.25.1

[PATCH v4 10/11] OvmfPkg: add BlobVerifierLibSevHashes

Dov Murik

#78084

Add an implementation for BlobVerifierLib that locates the SEV hashes
table and verifies that the calculated hashes of the kernel, initrd, and
cmdline blobs indeed match the expected hashes stated in the hashes
table.

If there's a missing hash or a hash mismatch then EFI_ACCESS_DENIED is
returned which will cause a failure to load a kernel image.

Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Ashish Kalra <ashish.kalra@...>
Cc: Brijesh Singh <brijesh.singh@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: James Bottomley <jejb@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Min Xu <min.m.xu@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3457
Co-developed-by: James Bottomley <jejb@...>
Signed-off-by: James Bottomley <jejb@...>
Signed-off-by: Dov Murik <dovmurik@...>
---
 OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf |  37 ++++
 OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c      | 199 +++++++=
+++++++++++++
 2 files changed, 236 insertions(+)

diff --git a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf b=
/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
new file mode 100644
index 000000000000..76ca0b8154ce
--- /dev/null
+++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
@@ -0,0 +1,37 @@
+## @file=0D
+#=0D
+#  Blob verifier library that uses SEV hashes table.  The hashes table hol=
ds the=0D
+#  allowed hashes of the kernel, initrd, and cmdline blobs.=0D
+#=0D
+#  Copyright (C) 2021, IBM Corp=0D
+#=0D
+#  SPDX-License-Identifier: BSD-2-Clause-Patent=0D
+#=0D
+##=0D
+=0D
+[Defines]=0D
+  INF_VERSION                    =3D 1.29=0D
+  BASE_NAME                      =3D BlobVerifierLibSevHashes=0D
+  FILE_GUID                      =3D 59e713b5-eff3-46a7-8d8b-46f4c004ad7b=
=0D
+  MODULE_TYPE                    =3D BASE=0D
+  VERSION_STRING                 =3D 1.0=0D
+  LIBRARY_CLASS                  =3D BlobVerifierLib=0D
+  CONSTRUCTOR                    =3D BlobVerifierLibSevHashesConstructor=0D
+=0D
+[Sources]=0D
+  BlobVerifierSevHashes.c=0D
+=0D
+[Packages]=0D
+  CryptoPkg/CryptoPkg.dec=0D
+  MdePkg/MdePkg.dec=0D
+  OvmfPkg/OvmfPkg.dec=0D
+=0D
+[LibraryClasses]=0D
+  BaseCryptLib=0D
+  BaseMemoryLib=0D
+  DebugLib=0D
+  PcdLib=0D
+=0D
+[FixedPcd]=0D
+  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase=0D
+  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize=0D
diff --git a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c b/Ovmf=
Pkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
new file mode 100644
index 000000000000..372ae6f469e7
--- /dev/null
+++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
@@ -0,0 +1,199 @@
+/** @file=0D
+=0D
+  Blob verifier library that uses SEV hashes table.  The hashes table hold=
s the=0D
+  allowed hashes of the kernel, initrd, and cmdline blobs.=0D
+=0D
+  Copyright (C) 2021, IBM Corporation=0D
+=0D
+  SPDX-License-Identifier: BSD-2-Clause-Patent=0D
+**/=0D
+=0D
+#include <Library/BaseCryptLib.h>=0D
+#include <Library/BaseLib.h>=0D
+#include <Library/BaseMemoryLib.h>=0D
+#include <Library/DebugLib.h>=0D
+#include <Library/BlobVerifierLib.h>=0D
+=0D
+/**=0D
+  The SEV Hashes table must be in encrypted memory and has the table=0D
+  and its entries described by=0D
+=0D
+  <GUID>|UINT16 <len>|<data>=0D
+=0D
+  With the whole table GUID being 9438d606-4f22-4cc9-b479-a793d411fd21=0D
+=0D
+  The current possible table entries are for the kernel, the initrd=0D
+  and the cmdline:=0D
+=0D
+  4de79437-abd2-427f-b835-d5b172d2045b  kernel=0D
+  44baf731-3a2f-4bd7-9af1-41e29169781d  initrd=0D
+  97d02dd8-bd20-4c94-aa78-e7714d36ab2a  cmdline=0D
+=0D
+  The size of the entry is used to identify the hash, but the=0D
+  expectation is that it will be 32 bytes of SHA-256.=0D
+**/=0D
+=0D
+#define SEV_HASH_TABLE_GUID \=0D
+  (GUID) { 0x9438d606, 0x4f22, 0x4cc9, { 0xb4, 0x79, 0xa7, 0x93, 0xd4, 0x1=
1, 0xfd, 0x21 } }=0D
+#define SEV_KERNEL_HASH_GUID \=0D
+  (GUID) { 0x4de79437, 0xabd2, 0x427f, { 0xb8, 0x35, 0xd5, 0xb1, 0x72, 0xd=
2, 0x04, 0x5b } }=0D
+#define SEV_INITRD_HASH_GUID \=0D
+  (GUID) { 0x44baf731, 0x3a2f, 0x4bd7, { 0x9a, 0xf1, 0x41, 0xe2, 0x91, 0x6=
9, 0x78, 0x1d } }=0D
+#define SEV_CMDLINE_HASH_GUID \=0D
+  (GUID) { 0x97d02dd8, 0xbd20, 0x4c94, { 0xaa, 0x78, 0xe7, 0x71, 0x4d, 0x3=
6, 0xab, 0x2a } }=0D
+=0D
+STATIC CONST EFI_GUID mSevKernelHashGuid =3D SEV_KERNEL_HASH_GUID;=0D
+STATIC CONST EFI_GUID mSevInitrdHashGuid =3D SEV_INITRD_HASH_GUID;=0D
+STATIC CONST EFI_GUID mSevCmdlineHashGuid =3D SEV_CMDLINE_HASH_GUID;=0D
+=0D
+#pragma pack (1)=0D
+typedef struct {=0D
+  GUID   Guid;=0D
+  UINT16 Len;=0D
+  UINT8  Data[];=0D
+} HASH_TABLE;=0D
+#pragma pack ()=0D
+=0D
+STATIC HASH_TABLE *mHashesTable;=0D
+STATIC UINT16 mHashesTableSize;=0D
+=0D
+STATIC=0D
+CONST GUID*=0D
+FindBlobEntryGuid (=0D
+  IN  CONST CHAR16    *BlobName=0D
+  )=0D
+{=0D
+  if (StrCmp (BlobName, L"kernel") =3D=3D 0) {=0D
+    return &mSevKernelHashGuid;=0D
+  } else if (StrCmp (BlobName, L"initrd") =3D=3D 0) {=0D
+    return &mSevInitrdHashGuid;=0D
+  } else if (StrCmp (BlobName, L"cmdline") =3D=3D 0) {=0D
+    return &mSevCmdlineHashGuid;=0D
+  } else {=0D
+    return NULL;=0D
+  }=0D
+}=0D
+=0D
+/**=0D
+  Verify blob from an external source.=0D
+=0D
+  @param[in] BlobName           The name of the blob=0D
+  @param[in] Buf                The data of the blob=0D
+  @param[in] BufSize            The size of the blob in bytes=0D
+=0D
+  @retval EFI_SUCCESS           The blob was verified successfully.=0D
+  @retval EFI_ACCESS_DENIED     The blob could not be verified, and theref=
ore=0D
+                                should be considered non-secure.=0D
+**/=0D
+EFI_STATUS=0D
+EFIAPI=0D
+VerifyBlob (=0D
+  IN  CONST CHAR16    *BlobName,=0D
+  IN  CONST VOID      *Buf,=0D
+  IN  UINT32          BufSize=0D
+  )=0D
+{=0D
+  CONST GUID *Guid;=0D
+  INT32 Remaining;=0D
+  HASH_TABLE *Entry;=0D
+=0D
+  if (mHashesTable =3D=3D NULL || mHashesTableSize =3D=3D 0) {=0D
+    DEBUG ((DEBUG_ERROR,=0D
+      "%a: Verifier called but no hashes table discoverd in MEMFD\n",=0D
+      __FUNCTION__));=0D
+    return EFI_ACCESS_DENIED;=0D
+  }=0D
+=0D
+  Guid =3D FindBlobEntryGuid (BlobName);=0D
+  if (Guid =3D=3D NULL) {=0D
+    DEBUG ((DEBUG_ERROR, "%a: Unknown blob name \"%s\"\n", __FUNCTION__,=0D
+      BlobName));=0D
+    return EFI_ACCESS_DENIED;=0D
+  }=0D
+=0D
+  //=0D
+  // Remaining is INT32 to catch underflow in case Entry->Len has a=0D
+  // very high UINT16 value=0D
+  //=0D
+  for (Entry =3D mHashesTable, Remaining =3D mHashesTableSize;=0D
+       Remaining >=3D sizeof *Entry && Remaining >=3D Entry->Len;=0D
+       Remaining -=3D Entry->Len,=0D
+       Entry =3D (HASH_TABLE *)((UINT8 *)Entry + Entry->Len)) {=0D
+    UINTN EntrySize;=0D
+    EFI_STATUS Status;=0D
+    UINT8 Hash[SHA256_DIGEST_SIZE];=0D
+=0D
+    if (!CompareGuid (&Entry->Guid, Guid)) {=0D
+      continue;=0D
+    }=0D
+=0D
+    DEBUG ((DEBUG_INFO, "%a: Found GUID %g in table\n", __FUNCTION__, Guid=
));=0D
+=0D
+    EntrySize =3D Entry->Len - sizeof Entry->Guid - sizeof Entry->Len;=0D
+    if (EntrySize !=3D SHA256_DIGEST_SIZE) {=0D
+      DEBUG ((DEBUG_ERROR, "%a: Hash has the wrong size %d !=3D %d\n",=0D
+        __FUNCTION__, EntrySize, SHA256_DIGEST_SIZE));=0D
+      return EFI_ACCESS_DENIED;=0D
+    }=0D
+=0D
+    //=0D
+    // Calculate the buffer's hash and verify that it is identical to the=
=0D
+    // expected hash table entry=0D
+    //=0D
+    Sha256HashAll (Buf, BufSize, Hash);=0D
+=0D
+    if (CompareMem (Entry->Data, Hash, EntrySize) =3D=3D 0) {=0D
+      Status =3D EFI_SUCCESS;=0D
+      DEBUG ((DEBUG_INFO, "%a: Hash comparison succeeded for \"%s\"\n",=0D
+        __FUNCTION__, BlobName));=0D
+    } else {=0D
+      Status =3D EFI_ACCESS_DENIED;=0D
+      DEBUG ((DEBUG_ERROR, "%a: Hash comparison failed for \"%s\"\n",=0D
+        __FUNCTION__, BlobName));=0D
+    }=0D
+    return Status;=0D
+  }=0D
+=0D
+  DEBUG ((DEBUG_ERROR, "%a: Hash GUID %g not found in table\n", __FUNCTION=
__,=0D
+    Guid));=0D
+  return EFI_ACCESS_DENIED;=0D
+}=0D
+=0D
+/**=0D
+  Locate the SEV hashes table.=0D
+=0D
+  This function always returns success, even if the table can't be found. =
 The=0D
+  subsequent VerifyBlob calls will fail if no table was found.=0D
+=0D
+  @retval RETURN_SUCCESS   The hashes table is set up correctly, or there =
is no=0D
+                           hashes table=0D
+**/=0D
+RETURN_STATUS=0D
+EFIAPI=0D
+BlobVerifierLibSevHashesConstructor (=0D
+  VOID=0D
+  )=0D
+{=0D
+  HASH_TABLE *Ptr =3D (void *)(UINTN)FixedPcdGet64 (PcdQemuHashTableBase);=
=0D
+  UINT32 Size =3D FixedPcdGet32 (PcdQemuHashTableSize);=0D
+=0D
+  mHashesTable =3D NULL;=0D
+  mHashesTableSize =3D 0;=0D
+=0D
+  if (Ptr =3D=3D NULL || Size < sizeof *Ptr ||=0D
+      !CompareGuid (&Ptr->Guid, &SEV_HASH_TABLE_GUID) ||=0D
+      Ptr->Len < sizeof *Ptr || Ptr->Len > Size) {=0D
+    return RETURN_SUCCESS;=0D
+  }=0D
+=0D
+  DEBUG ((DEBUG_INFO, "%a: Found injected hashes table in secure location\=
n",=0D
+    __FUNCTION__));=0D
+=0D
+  mHashesTable =3D (HASH_TABLE *)Ptr->Data;=0D
+  mHashesTableSize =3D Ptr->Len - sizeof Ptr->Guid - sizeof Ptr->Len;=0D
+=0D
+  DEBUG ((DEBUG_VERBOSE, "%a: mHashesTable=3D0x%p, Size=3D%u\n", __FUNCTIO=
N__,=0D
+    mHashesTable, mHashesTableSize));=0D
+=0D
+  return RETURN_SUCCESS;=0D
+}=0D
--=20
2.25.1

[PATCH v4 04/11] OvmfPkg: add library class BlobVerifierLib with null implementation

Dov Murik

#78083

BlobVerifierLib will be used to verify blobs fetching them from QEMU's
firmware config (fw_cfg) in platforms that enable such verification.

The null implementation BlobVerifierLibNull treats all blobs as valid.

Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Ashish Kalra <ashish.kalra@...>
Cc: Brijesh Singh <brijesh.singh@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: James Bottomley <jejb@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Min Xu <min.m.xu@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3457
Signed-off-by: Dov Murik <dovmurik@...>
Reviewed-by: Tom Lendacky <thomas.lendacky@...>
---
 OvmfPkg/OvmfPkg.dec                                     |  3 ++
 OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf | 24 +++++++++++++
 OvmfPkg/Include/Library/BlobVerifierLib.h               | 38 +++++++++++++=
+++++++
 OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c      | 33 +++++++++++++=
++++
 4 files changed, 98 insertions(+)

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 6ae733f6e39f..f82228d69cc2 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -23,6 +23,9 @@ [LibraryClasses]
   ##  @libraryclass  Access bhyve's firmware control interface.=0D
   BhyveFwCtlLib|Include/Library/BhyveFwCtlLib.h=0D
 =0D
+  ##  @libraryclass  Verify blobs read from the VMM=0D
+  BlobVerifierLib|Include/Library/BlobVerifierLib.h=0D
+=0D
   ##  @libraryclass  Loads and boots a Linux kernel image=0D
   #=0D
   LoadLinuxLib|Include/Library/LoadLinuxLib.h=0D
diff --git a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf b/Ovmf=
Pkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf
new file mode 100644
index 000000000000..850d398e65a4
--- /dev/null
+++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf
@@ -0,0 +1,24 @@
+## @file=0D
+#=0D
+#  Null implementation of the blob verifier library.=0D
+#=0D
+#  Copyright (C) 2021, IBM Corp=0D
+#=0D
+#  SPDX-License-Identifier: BSD-2-Clause-Patent=0D
+#=0D
+##=0D
+=0D
+[Defines]=0D
+  INF_VERSION                    =3D 1.29=0D
+  BASE_NAME                      =3D BlobVerifierLibNull=0D
+  FILE_GUID                      =3D b1b5533e-e01a-43bb-9e54-414f00ca036e=
=0D
+  MODULE_TYPE                    =3D BASE=0D
+  VERSION_STRING                 =3D 1.0=0D
+  LIBRARY_CLASS                  =3D BlobVerifierLib=0D
+=0D
+[Sources]=0D
+  BlobVerifierNull.c=0D
+=0D
+[Packages]=0D
+  MdePkg/MdePkg.dec=0D
+  OvmfPkg/OvmfPkg.dec=0D
diff --git a/OvmfPkg/Include/Library/BlobVerifierLib.h b/OvmfPkg/Include/Li=
brary/BlobVerifierLib.h
new file mode 100644
index 000000000000..db122684f76c
--- /dev/null
+++ b/OvmfPkg/Include/Library/BlobVerifierLib.h
@@ -0,0 +1,38 @@
+/** @file=0D
+=0D
+  Blob verification library=0D
+=0D
+  This library class allows verifiying whether blobs from external sources=
=0D
+  (such as QEMU's firmware config) are trusted.=0D
+=0D
+  Copyright (C) 2021, IBM Corporation=0D
+=0D
+  SPDX-License-Identifier: BSD-2-Clause-Patent=0D
+**/=0D
+=0D
+#ifndef BLOB_VERIFIER_LIB_H__=0D
+#define BLOB_VERIFIER_LIB_H__=0D
+=0D
+#include <Uefi/UefiBaseType.h>=0D
+#include <Base.h>=0D
+=0D
+/**=0D
+  Verify blob from an external source.=0D
+=0D
+  @param[in] BlobName           The name of the blob=0D
+  @param[in] Buf                The data of the blob=0D
+  @param[in] BufSize            The size of the blob in bytes=0D
+=0D
+  @retval EFI_SUCCESS           The blob was verified successfully.=0D
+  @retval EFI_ACCESS_DENIED     The blob could not be verified, and theref=
ore=0D
+                                should be considered non-secure.=0D
+**/=0D
+EFI_STATUS=0D
+EFIAPI=0D
+VerifyBlob (=0D
+  IN  CONST CHAR16    *BlobName,=0D
+  IN  CONST VOID      *Buf,=0D
+  IN  UINT32          BufSize=0D
+  );=0D
+=0D
+#endif=0D
diff --git a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c b/OvmfPkg/L=
ibrary/BlobVerifierLib/BlobVerifierNull.c
new file mode 100644
index 000000000000..975d4dd52f80
--- /dev/null
+++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c
@@ -0,0 +1,33 @@
+/** @file=0D
+=0D
+  Null implementation of the blob verifier library.=0D
+=0D
+  Copyright (C) 2021, IBM Corporation=0D
+=0D
+  SPDX-License-Identifier: BSD-2-Clause-Patent=0D
+**/=0D
+=0D
+#include <Library/BaseLib.h>=0D
+#include <Library/BlobVerifierLib.h>=0D
+=0D
+/**=0D
+  Verify blob from an external source.=0D
+=0D
+  @param[in] BlobName           The name of the blob=0D
+  @param[in] Buf                The data of the blob=0D
+  @param[in] BufSize            The size of the blob in bytes=0D
+=0D
+  @retval EFI_SUCCESS           The blob was verified successfully.=0D
+  @retval EFI_ACCESS_DENIED     The blob could not be verified, and theref=
ore=0D
+                                should be considered non-secure.=0D
+**/=0D
+EFI_STATUS=0D
+EFIAPI=0D
+VerifyBlob (=0D
+  IN  CONST CHAR16    *BlobName,=0D
+  IN  CONST VOID      *Buf,=0D
+  IN  UINT32          BufSize=0D
+  )=0D
+{=0D
+  return EFI_SUCCESS;=0D
+}=0D
--=20
2.25.1

[PATCH v4 11/11] OvmfPkg/AmdSev: Enforce hash verification of kernel blobs

Dov Murik

#78082

In the AmdSevX64 build, use BlobVerifierLibSevHashes to enforce
verification of hashes of the kernel/initrd/cmdline blobs fetched from
firmware config.

This allows for secure (measured) boot of SEV guests with QEMU's
-kernel/-initrd/-append switches (with the corresponding QEMU support
for injecting the hashes table into initial measured guest memory).

Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Ashish Kalra <ashish.kalra@...>
Cc: Brijesh Singh <brijesh.singh@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: James Bottomley <jejb@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Min Xu <min.m.xu@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3457
Signed-off-by: Dov Murik <dovmurik@...>
Reviewed-by: Tom Lendacky <thomas.lendacky@...>
---
 OvmfPkg/AmdSev/AmdSevX64.dsc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index b2cc96cc5a97..c01599ea354f 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -173,7 +173,7 @@ [LibraryClasses]
   LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf=0D
   CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/Customize=
dDisplayLib.inf=0D
   FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltL=
ib.inf=0D
-  BlobVerifierLib|OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf=
=0D
+  BlobVerifierLib|OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes=
.inf=0D
 =0D
 !if $(SOURCE_DEBUG_ENABLE) =3D=3D TRUE=0D
   PeCoffExtraActionLib|SourceLevelDebugPkg/Library/PeCoffExtraActionLibDeb=
ug/PeCoffExtraActionLibDebug.inf=0D
@@ -696,7 +696,7 @@ [Components]
   }=0D
   OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf {=0D
     <LibraryClasses>=0D
-      NULL|OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf=0D
+      NULL|OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf=0D
   }=0D
   OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf=0D
   OvmfPkg/Virtio10Dxe/Virtio10.inf=0D
--=20
2.25.1

[PATCH v4 03/11] OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg

Dov Murik

#78081

From: James Bottomley <jejb@...>

Support QEMU's -kernel option.

Create a QemuKernel.c for PlatformBootManagerLibGrub
which is an exact copy of the file
PlatformBootManagerLib/QemuKernel.c .

Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Ashish Kalra <ashish.kalra@...>
Cc: Brijesh Singh <brijesh.singh@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: James Bottomley <jejb@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Min Xu <min.m.xu@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3457
Signed-off-by: James Bottomley <jejb@...>
Reviewed-by: Brijesh Singh <brijesh.singh@...>
---
 OvmfPkg/AmdSev/AmdSevX64.dsc                                              =
          |  1 +
 OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf =
          |  2 ++
 OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h                  =
          | 11 +++++++++++
 OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c                  =
          |  5 +++++
 OvmfPkg/Library/{PlatformBootManagerLib =3D> PlatformBootManagerLibGrub}/Q=
emuKernel.c |  0
 5 files changed, 19 insertions(+)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index a2f1324c40a6..aefdcf881c99 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -281,6 +281,7 @@ [LibraryClasses.common.PEIM]
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuE=
xceptionHandlerLib.inf=0D
   MpInitLib|UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf=0D
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf=
=0D
+  QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoad=
ImageLib.inf=0D
   PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf=0D
   QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiLib.inf=0D
 =0D
diff --git a/OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManager=
LibGrub.inf b/OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManage=
rLibGrub.inf
index 9a806d17ec45..5f6f73d18470 100644
--- a/OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub=
.inf
+++ b/OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub=
.inf
@@ -23,6 +23,7 @@ [Defines]
 =0D
 [Sources]=0D
   BdsPlatform.c=0D
+  QemuKernel.c=0D
   PlatformData.c=0D
   BdsPlatform.h=0D
 =0D
@@ -46,6 +47,7 @@ [LibraryClasses]
   BootLogoLib=0D
   DevicePathLib=0D
   PciLib=0D
+  QemuLoadImageLib=0D
   UefiLib=0D
   PlatformBmPrintScLib=0D
   Tcg2PhysicalPresenceLib=0D
diff --git a/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h b/Ovm=
fPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h
index 748c63081920..f1d3a2906c00 100644
--- a/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h
+++ b/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h
@@ -172,4 +172,15 @@ PlatformInitializeConsole (
   IN PLATFORM_CONSOLE_CONNECT_ENTRY   *PlatformConsole=0D
   );=0D
 =0D
+/**=0D
+  Loads and boots UEFI Linux via the FwCfg interface.=0D
+=0D
+  @retval    EFI_NOT_FOUND - The Linux kernel was not found=0D
+=0D
+**/=0D
+EFI_STATUS=0D
+TryRunningQemuKernel (=0D
+  VOID=0D
+  );=0D
+=0D
 #endif // _PLATFORM_SPECIFIC_BDS_PLATFORM_H_=0D
diff --git a/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c b/Ovm=
fPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c
index 5c92d4fc2b09..7cceeea4879c 100644
--- a/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c
@@ -1315,6 +1315,11 @@ PlatformBootManagerAfterConsole (
   //=0D
   Tcg2PhysicalPresenceLibProcessRequest (NULL);=0D
 =0D
+  //=0D
+  // Process QEMU's -kernel command line option=0D
+  //=0D
+  TryRunningQemuKernel ();=0D
+=0D
   //=0D
   // Perform some platform specific connect sequence=0D
   //=0D
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c b/OvmfPkg/=
Library/PlatformBootManagerLibGrub/QemuKernel.c
similarity index 100%
copy from OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c
copy to OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c
--=20
2.25.1

[PATCH v4 06/11] ArmVirtPkg: add BlobVerifierLibNull to DSC

Dov Murik

#78080

This prepares the ground for calling VerifyBlob() in
QemuKernelLoaderFsDxe.

Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Leif Lindholm <leif@...>
Cc: Sami Mujawar <sami.mujawar@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Ashish Kalra <ashish.kalra@...>
Cc: Brijesh Singh <brijesh.singh@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: James Bottomley <jejb@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Min Xu <min.m.xu@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3457
Signed-off-by: Dov Murik <dovmurik@...>
---
 ArmVirtPkg/ArmVirtQemu.dsc       | 5 ++++-
 ArmVirtPkg/ArmVirtQemuKernel.dsc | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 7ef5e7297bc7..bf8bb1ec9578 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -440,7 +440,10 @@ [Components.common]
       NULL|MdeModulePkg/Library/BootManagerUiLib/BootManagerUiLib.inf=0D
       NULL|MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanc=
eManagerUiLib.inf=0D
   }=0D
-  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf=0D
+  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf {=0D
+    <LibraryClasses>=0D
+      NULL|OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf=0D
+  }=0D
 =0D
   #=0D
   # Networking stack=0D
diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKerne=
l.dsc
index a542fcb157e9..af34cb47a12d 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -376,7 +376,10 @@ [Components.common]
       NULL|MdeModulePkg/Library/BootManagerUiLib/BootManagerUiLib.inf=0D
       NULL|MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanc=
eManagerUiLib.inf=0D
   }=0D
-  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf=0D
+  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf {=0D
+    <LibraryClasses>=0D
+      NULL|OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf=0D
+  }=0D
 =0D
   #=0D
   # Networking stack=0D
--=20
2.25.1

[PATCH v4 09/11] OvmfPkg/AmdSev: reserve MEMFD space for for firmware config hashes

Dov Murik

#78079

From: James Bottomley <jejb@...>

Split the existing 4KB page reserved for SEV launch secrets into two
parts: first 3KB for SEV launch secrets and last 1KB for firmware
config hashes.

The area of the firmware config hashes will be attested (measured) by
the PSP and thus the untrusted VMM can't pass in different files from
what the guest owner allows.

Declare this in the Reset Vector table using GUID
7255371f-3a3b-4b04-927b-1da6efa8d454 and a uint32_t table of a base
and size value (similar to the structure used to declare the launch
secret block).

Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Ashish Kalra <ashish.kalra@...>
Cc: Brijesh Singh <brijesh.singh@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: James Bottomley <jejb@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Min Xu <min.m.xu@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3457
Co-developed-by: Dov Murik <dovmurik@...>
Signed-off-by: Dov Murik <dovmurik@...>
Signed-off-by: James Bottomley <jejb@...>
Reviewed-by: Tom Lendacky <thomas.lendacky@...>
Reviewed-by: Brijesh Singh <brijesh.singh@...>
---
 OvmfPkg/OvmfPkg.dec                          |  6 ++++++
 OvmfPkg/AmdSev/AmdSevX64.fdf                 |  5 ++++-
 OvmfPkg/ResetVector/ResetVector.inf          |  2 ++
 OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 20 ++++++++++++++++++++
 OvmfPkg/ResetVector/ResetVector.nasmb        |  2 ++
 5 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index f82228d69cc2..2ab27f0c73c2 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -324,6 +324,12 @@ [PcdsFixedAtBuild]
   gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|0x0|UINT32|0x42=0D
   gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize|0x0|UINT32|0x43=0D
 =0D
+  ## The base address and size of a hash table confirming allowed=0D
+  #  parameters to be passed in via the Qemu firmware configuration=0D
+  #  device=0D
+  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase|0x0|UINT32|0x47=0D
+  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize|0x0|UINT32|0x48=0D
+=0D
 [PcdsDynamic, PcdsDynamicEx]=0D
   gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2=0D
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x1=
0=0D
diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf
index 9977b0f00a18..0a89749700c3 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.fdf
+++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
@@ -59,9 +59,12 @@ [FD.MEMFD]
 0x00B000|0x001000=0D
 gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.P=
cdSevEsWorkAreaSize=0D
 =0D
-0x00C000|0x001000=0D
+0x00C000|0x000C00=0D
 gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGu=
id.PcdSevLaunchSecretSize=0D
 =0D
+0x00CC00|0x000400=0D
+gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase|gUefiOvmfPkgTokenSpaceGuid=
.PcdQemuHashTableSize=0D
+=0D
 0x00D000|0x001000=0D
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpace=
Guid.PcdOvmfSecGhcbBackupSize=0D
 =0D
diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/Rese=
tVector.inf
index dc38f68919cd..d028c92d8cfa 100644
--- a/OvmfPkg/ResetVector/ResetVector.inf
+++ b/OvmfPkg/ResetVector/ResetVector.inf
@@ -47,3 +47,5 @@ [Pcd]
 [FixedPcd]=0D
   gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase=0D
   gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize=0D
+  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase=0D
+  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize=0D
diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/ResetVe=
ctor/Ia16/ResetVectorVtf0.asm
index 9c0b5853a46f..7ec3c6e980c3 100644
--- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
@@ -47,7 +47,27 @@ TIMES (15 - ((guidedStructureEnd - guidedStructureStart =
+ 15) % 16)) DB 0
 ;=0D
 guidedStructureStart:=0D
 =0D
+; SEV Hash Table Block=0D
 ;=0D
+; This describes the guest ram area where the hypervisor should=0D
+; install a table describing the hashes of certain firmware configuration=
=0D
+; device files that would otherwise be passed in unchecked.  The current=0D
+; use is for the kernel, initrd and command line values, but others may be=
=0D
+; added.  The data format is:=0D
+;=0D
+; base physical address (32 bit word)=0D
+; table length (32 bit word)=0D
+;=0D
+; GUID (SEV FW config hash block): 7255371f-3a3b-4b04-927b-1da6efa8d454=0D
+;=0D
+sevFwHashBlockStart:=0D
+    DD      SEV_FW_HASH_BLOCK_BASE=0D
+    DD      SEV_FW_HASH_BLOCK_SIZE=0D
+    DW      sevFwHashBlockEnd - sevFwHashBlockStart=0D
+    DB      0x1f, 0x37, 0x55, 0x72, 0x3b, 0x3a, 0x04, 0x4b=0D
+    DB      0x92, 0x7b, 0x1d, 0xa6, 0xef, 0xa8, 0xd4, 0x54=0D
+sevFwHashBlockEnd:=0D
+=0D
 ; SEV Secret block=0D
 ;=0D
 ; This describes the guest ram area where the hypervisor should=0D
diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/Re=
setVector.nasmb
index 5fbacaed5f9d..8d0bab02f8cb 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -88,5 +88,7 @@
   %define SEV_ES_AP_RESET_IP  FixedPcdGet32 (PcdSevEsWorkAreaBase)=0D
   %define SEV_LAUNCH_SECRET_BASE  FixedPcdGet32 (PcdSevLaunchSecretBase)=0D
   %define SEV_LAUNCH_SECRET_SIZE  FixedPcdGet32 (PcdSevLaunchSecretSize)=0D
+  %define SEV_FW_HASH_BLOCK_BASE  FixedPcdGet32 (PcdQemuHashTableBase)=0D
+  %define SEV_FW_HASH_BLOCK_SIZE  FixedPcdGet32 (PcdQemuHashTableSize)=0D
 %include "Ia16/ResetVectorVtf0.asm"=0D
 =0D
--=20
2.25.1

[PATCH v4 08/11] OvmfPkg/AmdSev/SecretPei: build hob for full page

Dov Murik

#78078

Round up the size of the SEV launch secret area to a whole page, as
required by BuildMemoryAllocationHob.  This will allow the secret
area defined in the MEMFD to take less than a whole 4KB page.

Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Ashish Kalra <ashish.kalra@...>
Cc: Brijesh Singh <brijesh.singh@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: James Bottomley <jejb@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Min Xu <min.m.xu@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3457
Signed-off-by: Dov Murik <dovmurik@...>
Reviewed-by: Tom Lendacky <thomas.lendacky@...>
Reviewed-by: Brijesh Singh <brijesh.singh@...>
---
 OvmfPkg/AmdSev/SecretPei/SecretPei.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.c b/OvmfPkg/AmdSev/SecretPe=
i/SecretPei.c
index ad491515dd5d..db94c26b54d1 100644
--- a/OvmfPkg/AmdSev/SecretPei/SecretPei.c
+++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
@@ -4,6 +4,7 @@
   Copyright (C) 2020 James Bottomley, IBM Corporation.=0D
   SPDX-License-Identifier: BSD-2-Clause-Patent=0D
 **/=0D
+#include <Base.h>=0D
 #include <PiPei.h>=0D
 #include <Library/HobLib.h>=0D
 #include <Library/PcdLib.h>=0D
@@ -17,7 +18,7 @@ InitializeSecretPei (
 {=0D
   BuildMemoryAllocationHob (=0D
     PcdGet32 (PcdSevLaunchSecretBase),=0D
-    PcdGet32 (PcdSevLaunchSecretSize),=0D
+    ALIGN_VALUE (PcdGet32 (PcdSevLaunchSecretSize), EFI_PAGE_SIZE),=0D
     EfiBootServicesData=0D
     );=0D
 =0D
--=20
2.25.1