Date   

[RFC PATCH v4 02/27] OvmfPkg/ResetVector: add the macro to invoke MSR protocol based VMGEXIT

Brijesh Singh
 

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

The upcoming SEV-SNP support will need to make a few additional MSR
protocol based VMGEXIT's. Add a macro that wraps the common setup and
response validation logic in one place to keep the code readable.

While at it, define SEV_STATUS_MSR that will be used to get the SEV STATUS
MSR instead of open coding it.

Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Suggested-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/ResetVector/Ia32/AmdSev.asm | 69 +++++++++++++++++++----------
1 file changed, 45 insertions(+), 24 deletions(-)

diff --git a/OvmfPkg/ResetVector/Ia32/AmdSev.asm b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
index b32dd3b5d656..c3b4e16bf681 100644
--- a/OvmfPkg/ResetVector/Ia32/AmdSev.asm
+++ b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
@@ -35,6 +35,42 @@ BITS 32
%define GHCB_CPUID_REGISTER_SHIFT 30
%define CPUID_INSN_LEN 2

+%define SEV_STATUS_MSR 0xc0010130
+
+; Macro is used to issue the MSR protocol based VMGEXIT. The caller is
+; responsible to populate values in the EDX:EAX registers. After the vmmcall
+; returns, it verifies that the response code matches with the expected
+; code. If it does not match then terminate the guest. The result of request
+; is returned in the EDX:EAX.
+;
+; args 1:Request code, 2: Response code
+%macro VmgExit 2
+ ;
+ ; Add request code:
+ ; GHCB_MSR[11:0] = Request code
+ or eax, %1
+
+ mov ecx, SEV_STATUS_MSR
+ wrmsr
+
+ ; Issue VMGEXIT - NASM doesn't support the vmmcall instruction in 32-bit
+ ; mode, so work around this by temporarily switching to 64-bit mode.
+ ;
+BITS 64
+ rep vmmcall
+BITS 32
+
+ mov ecx, SEV_STATUS_MSR
+ rdmsr
+
+ ;
+ ; Verify the reponse code, if it does not match then request to terminate
+ ; GHCB_MSR[11:0] = Response code
+ mov ecx, eax
+ and ecx, 0xfff
+ cmp ecx, %2
+ jne SevEsUnexpectedRespTerminate
+%endmacro

; Check if Secure Encrypted Virtualization (SEV) features are enabled.
;
@@ -85,7 +121,7 @@ CheckSevFeatures:

; Check if SEV memory encryption is enabled
; MSR_0xC0010131 - Bit 0 (SEV enabled)
- mov ecx, 0xc0010131
+ mov ecx, SEV_STATUS_MSR
rdmsr
bt eax, 0
jnc NoSev
@@ -100,7 +136,7 @@ CheckSevFeatures:

; Check if SEV-ES is enabled
; MSR_0xC0010131 - Bit 1 (SEV-ES enabled)
- mov ecx, 0xc0010131
+ mov ecx, SEV_STATUS_MSR
rdmsr
bt eax, 1
jnc GetSevEncBit
@@ -197,10 +233,10 @@ SevEsIdtNotCpuid:
mov eax, 1
jmp SevEsIdtTerminate

-SevEsIdtNoCpuidResponse:
+SevEsUnexpectedRespTerminate:
;
; Use VMGEXIT to request termination.
- ; 2 - GHCB_CPUID_RESPONSE not received
+ ; 2 - Unexpected Response is received
;
mov eax, 2

@@ -216,7 +252,7 @@ SevEsIdtTerminate:
shl eax, 16
or eax, 0x1100
xor edx, edx
- mov ecx, 0xc0010130
+ mov ecx, SEV_STATUS_MSR
wrmsr
;
; Issue VMGEXIT - NASM doesn't support the vmmcall instruction in 32-bit
@@ -276,7 +312,7 @@ SevEsIdtVmmComm:
mov [esp + VC_CPUID_REQUEST_REGISTER], eax

; Save current GHCB MSR value
- mov ecx, 0xc0010130
+ mov ecx, SEV_STATUS_MSR
rdmsr
mov [esp + VC_GHCB_MSR_EAX], eax
mov [esp + VC_GHCB_MSR_EDX], edx
@@ -293,31 +329,16 @@ NextReg:
jge VmmDone

shl eax, GHCB_CPUID_REGISTER_SHIFT
- or eax, GHCB_CPUID_REQUEST
mov edx, [esp + VC_CPUID_FUNCTION]
- mov ecx, 0xc0010130
- wrmsr

- ;
- ; Issue VMGEXIT - NASM doesn't support the vmmcall instruction in 32-bit
- ; mode, so work around this by temporarily switching to 64-bit mode.
- ;
-BITS 64
- rep vmmcall
-BITS 32
+ VmgExit GHCB_CPUID_REQUEST, GHCB_CPUID_RESPONSE

;
- ; Read GHCB MSR
+ ; Response GHCB MSR
; GHCB_MSR[63:32] = CPUID register value
; GHCB_MSR[31:30] = CPUID register
; GHCB_MSR[11:0] = CPUID response protocol
;
- mov ecx, 0xc0010130
- rdmsr
- mov ecx, eax
- and ecx, 0xfff
- cmp ecx, GHCB_CPUID_RESPONSE
- jne SevEsIdtNoCpuidResponse

; Save returned value
shr eax, GHCB_CPUID_REGISTER_SHIFT
@@ -335,7 +356,7 @@ VmmDone:
;
mov eax, [esp + VC_GHCB_MSR_EAX]
mov edx, [esp + VC_GHCB_MSR_EDX]
- mov ecx, 0xc0010130
+ mov ecx, SEV_STATUS_MSR
wrmsr

mov eax, [esp + VC_CPUID_RESULT_EAX]
--
2.17.1


[RFC PATCH v4 04/27] OvmfPkg: reserve SNP secrets page

Brijesh Singh
 

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

During the SNP guest launch sequence, a special secrets page needs to be
inserted by the VMM. The PSP will populate the page; it will contain the
VM Platform Communication Key (VMPCKs) used by the guest to send and
receive secure messages to the PSP.

The purpose of the secrets page in the SEV-SNP is different from the one
used in SEV guests. In SEV, the secrets page contains the guest owner's
private data after the remote attestation.

Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/OvmfPkg.dec | 7 +++++++
OvmfPkg/OvmfPkgX64.fdf | 3 +++
2 files changed, 10 insertions(+)

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 6ae733f6e39f..106a368ec975 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -321,6 +321,13 @@ [PcdsFixedAtBuild]
gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|0x0|UINT32|0x42
gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize|0x0|UINT32|0x43

+ ## The base address and size of the SEV-SNP Secrets Area that contains
+ # the VM platform communication key used to send and recieve the
+ # messages to the PSP. If this is set in the .fdf, the platform
+ # is responsible to reserve this area from DXE phase overwrites.
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase|0|UINT32|0x47
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize|0|UINT32|0x48
+
[PcdsDynamic, PcdsDynamicEx]
gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index 5fa8c0895808..902c6a4e9ea1 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -88,6 +88,9 @@ [FD.MEMFD]
0x00C000|0x001000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize

+0x00D000|0x001000
+gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize
+
0x010000|0x010000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize

--
2.17.1


[RFC PATCH v4 03/27] OvmfPkg/ResetVector: add the macro to request guest termination

Brijesh Singh
 

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

The upcoming SEV-SNP support will need to make a few additional guest
termination requests depending on the failure type. Let's move the logic
to request the guest termination into a macro to keep the code readable.

Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Suggested-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/ResetVector/Ia32/AmdSev.asm | 87 +++++++++++++++--------------
1 file changed, 45 insertions(+), 42 deletions(-)

diff --git a/OvmfPkg/ResetVector/Ia32/AmdSev.asm b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
index c3b4e16bf681..7465f7086449 100644
--- a/OvmfPkg/ResetVector/Ia32/AmdSev.asm
+++ b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
@@ -37,6 +37,13 @@ BITS 32

%define SEV_STATUS_MSR 0xc0010130

+; The #VC was not for CPUID
+%define TERM_VC_NOT_CPUID 1
+
+; The unexpected response code
+%define TERM_UNEXPECTED_RESP_CODE 2
+
+
; Macro is used to issue the MSR protocol based VMGEXIT. The caller is
; responsible to populate values in the EDX:EAX registers. After the vmmcall
; returns, it verifies that the response code matches with the expected
@@ -72,6 +79,43 @@ BITS 32
jne SevEsUnexpectedRespTerminate
%endmacro

+; Macro to terminate the guest using the VMGEXIT.
+; arg 1: reason code
+%macro TerminateVmgExit 1
+ mov eax, %1
+ ;
+ ; Use VMGEXIT to request termination. At this point the reason code is
+ ; located in EAX, so shift it left 16 bits to the proper location.
+ ;
+ ; EAX[11:0] => 0x100 - request termination
+ ; EAX[15:12] => 0x1 - OVMF
+ ; EAX[23:16] => 0xXX - REASON CODE
+ ;
+ shl eax, 16
+ or eax, 0x1100
+ xor edx, edx
+ mov ecx, SEV_STATUS_MSR
+ wrmsr
+ ;
+ ; Issue VMGEXIT - NASM doesn't support the vmmcall instruction in 32-bit
+ ; mode, so work around this by temporarily switching to 64-bit mode.
+ ;
+BITS 64
+ rep vmmcall
+BITS 32
+
+ ;
+ ; We shouldn't come back from the VMGEXIT, but if we do, just loop.
+ ;
+%%TerminateHlt:
+ hlt
+ jmp %%TerminateHlt
+%endmacro
+
+; Terminate the guest due to unexpected response code.
+SevEsUnexpectedRespTerminate:
+ TerminateVmgExit TERM_UNEXPECTED_RESP_CODE
+
; Check if Secure Encrypted Virtualization (SEV) features are enabled.
;
; Register usage is tight in this routine, so multiple calls for the
@@ -226,48 +270,7 @@ SevEsDisabled:
;

SevEsIdtNotCpuid:
- ;
- ; Use VMGEXIT to request termination.
- ; 1 - #VC was not for CPUID
- ;
- mov eax, 1
- jmp SevEsIdtTerminate
-
-SevEsUnexpectedRespTerminate:
- ;
- ; Use VMGEXIT to request termination.
- ; 2 - Unexpected Response is received
- ;
- mov eax, 2
-
-SevEsIdtTerminate:
- ;
- ; Use VMGEXIT to request termination. At this point the reason code is
- ; located in EAX, so shift it left 16 bits to the proper location.
- ;
- ; EAX[11:0] => 0x100 - request termination
- ; EAX[15:12] => 0x1 - OVMF
- ; EAX[23:16] => 0xXX - REASON CODE
- ;
- shl eax, 16
- or eax, 0x1100
- xor edx, edx
- mov ecx, SEV_STATUS_MSR
- wrmsr
- ;
- ; Issue VMGEXIT - NASM doesn't support the vmmcall instruction in 32-bit
- ; mode, so work around this by temporarily switching to 64-bit mode.
- ;
-BITS 64
- rep vmmcall
-BITS 32
-
- ;
- ; We shouldn't come back from the VMGEXIT, but if we do, just loop.
- ;
-SevEsIdtHlt:
- hlt
- jmp SevEsIdtHlt
+ TerminateVmgExit TERM_VC_NOT_CPUID
iret

;
--
2.17.1


[RFC PATCH v4 00/27] Add AMD Secure Nested Paging (SEV-SNP) support

Brijesh Singh
 

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

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

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

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

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

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

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

This series does not implements the following SEV-SNP features yet:

* CPUID filtering
* Lazy validation
* Interrupt security

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

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

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

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

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

Brijesh Singh (26):
OvmfPkg/ResetVector: move SEV specific code in a separate file
OvmfPkg/ResetVector: add the macro to invoke MSR protocol based
VMGEXIT
OvmfPkg/ResetVector: add the macro to request guest termination
OvmfPkg: reserve SNP secrets page
OvmfPkg: reserve CPUID page for SEV-SNP
OvmfPkg/ResetVector: introduce SEV-SNP boot block GUID
OvmfPkg/ResetVector: pre-validate the data pages used in SEC phase
OvmfPkg/ResetVector: invalidate the GHCB page
UefiCpuPkg: Define the SEV-SNP specific dynamic PCDs
OvmfPkg/MemEncryptSevLib: add MemEncryptSevSnpEnabled()
OvmfPkg/SecMain: register GHCB gpa for the SEV-SNP guest
OvmfPkg/PlatformPei: register GHCB gpa for the SEV-SNP guest
OvmfPkg/AmdSevDxe: do not use extended PCI config space
OvmfPkg/MemEncryptSevLib: add support to validate system RAM
OvmfPkg/BaseMemEncryptSevLib: skip the pre-validated system RAM
OvmfPkg/MemEncryptSevLib: add support to validate > 4GB memory in PEI
phase
OvmfPkg/SecMain: pre-validate the memory used for decompressing Fv
OvmfPkg/PlatformPei: validate the system RAM when SNP is active
OvmfPkg/PlatformPei: set the SEV-SNP enabled PCD
OvmfPkg/PlatformPei: set the Hypervisor Features PCD
MdePkg/GHCB: increase the GHCB protocol max version
UefiCpuPkg/MpLib: add support to register GHCB GPA when SEV-SNP is
enabled
OvmfPkg/MemEncryptSevLib: change the page state in the RMP table
OvmfPkg/MemEncryptSevLib: skip page state change for Mmio address
OvmfPkg/PlatformPei: mark cpuid and secrets memory reserved in EFI map
OvmfPkg/AmdSev: expose the SNP reserved pages through configuration
table

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

OvmfPkg/OvmfPkg.dec | 24 +
UefiCpuPkg/UefiCpuPkg.dec | 11 +
OvmfPkg/AmdSev/AmdSevX64.dsc | 5 +-
OvmfPkg/Bhyve/BhyveX64.dsc | 5 +-
OvmfPkg/OvmfPkgIa32.dsc | 1 +
OvmfPkg/OvmfPkgIa32X64.dsc | 6 +-
OvmfPkg/OvmfPkgX64.dsc | 5 +-
OvmfPkg/OvmfXen.dsc | 5 +-
OvmfPkg/OvmfPkgX64.fdf | 14 +-
OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 7 +
.../DxeMemEncryptSevLib.inf | 3 +
.../PeiMemEncryptSevLib.inf | 7 +
.../SecMemEncryptSevLib.inf | 3 +
OvmfPkg/PlatformPei/PlatformPei.inf | 8 +
OvmfPkg/ResetVector/ResetVector.inf | 6 +
OvmfPkg/Sec/SecMain.inf | 3 +
UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 4 +
UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf | 4 +
MdePkg/Include/Register/Amd/Ghcb.h | 2 +-
.../Guid/ConfidentialComputingSecret.h | 18 +
OvmfPkg/Include/Library/MemEncryptSevLib.h | 26 ++
.../X64/SnpPageStateChange.h | 31 ++
.../BaseMemEncryptSevLib/X64/VirtualMemory.h | 19 +
UefiCpuPkg/Library/MpInitLib/MpLib.h | 19 +
OvmfPkg/AmdSevDxe/AmdSevDxe.c | 23 +
.../DxeMemEncryptSevLibInternal.c | 27 ++
.../Ia32/MemEncryptSevLib.c | 17 +
.../PeiMemEncryptSevLibInternal.c | 27 ++
.../SecMemEncryptSevLibInternal.c | 19 +
.../X64/DxeSnpSystemRamValidate.c | 40 ++
.../X64/PeiDxeVirtualMemory.c | 167 ++++++-
.../X64/PeiSnpSystemRamValidate.c | 126 ++++++
.../X64/SecSnpSystemRamValidate.c | 36 ++
.../X64/SnpPageStateChangeInternal.c | 295 +++++++++++++
OvmfPkg/PlatformPei/AmdSev.c | 192 ++++++++
OvmfPkg/PlatformPei/MemDetect.c | 21 +
OvmfPkg/Sec/SecMain.c | 111 +++++
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 11 +-
.../MpInitLib/Ia32/SevSnpRmpAdjustInternal.c | 31 ++
UefiCpuPkg/Library/MpInitLib/MpLib.c | 275 +++++++++++-
.../MpInitLib/X64/SevSnpRmpAdjustInternal.c | 44 ++
OvmfPkg/FvmainCompactScratchEnd.fdf.inc | 5 +
OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 27 ++
.../Ia32/{PageTables64.asm => AmdSev.asm} | 415 +++++++++---------
OvmfPkg/ResetVector/Ia32/PageTables64.asm | 404 +----------------
OvmfPkg/ResetVector/ResetVector.nasmb | 7 +
UefiCpuPkg/Library/MpInitLib/MpEqu.inc | 1 +
UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 51 +++
48 files changed, 1978 insertions(+), 630 deletions(-)
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateChange.h
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiSnpSystemRamValidate.c
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecSnpSystemRamValidate.c
create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateChangeInternal.c
create mode 100644 UefiCpuPkg/Library/MpInitLib/Ia32/SevSnpRmpAdjustInternal.c
create mode 100644 UefiCpuPkg/Library/MpInitLib/X64/SevSnpRmpAdjustInternal.c
copy OvmfPkg/ResetVector/Ia32/{PageTables64.asm => AmdSev.asm} (67%)

--
2.17.1


Re: [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER

Marvin Häuser
 

On 28.06.21 16:57, Laszlo Ersek wrote:
On 06/25/21 20:47, Kun Qin wrote:
Hi Mike,

Thanks for the information. I can update the patch and proposed spec
change to use flexible array in v-next if there is no other concerns.

After switching to flexible array, OFFSET_OF (Data) should lead to the
same result as sizeof.
This may be true on specific compilers, but it is *not* guaranteed by
the C language standard.
Sorry, for completeness sake... :)

I don't think it really depends on the compiler (but can vary per ABI), but it's a conceptual issue with alignment requirements. Assuming natural alignment and the usual stuff, for "struct s { uint64_t a; uint32_t b; uint8_t c[]; }" the alignment requirement is 8 Bytes, where there are 4 Bytes of padding after "b" (as "c" may as well be empty). "c" however is guaranteed to start after b in the same fashion as if it was declared with the maximum amount of elements that fit the memory. So if we take 4 elements for "c", and note that "c" has an alignment requirement of 1 Byte, c[0 .. 3] will alias the padding after "b". For "sizeof" this means that the padding is included, whereas for "offsetof" it is not, yielding "sizeof(struct s) == offsetof(struct s, c) + 4". That is what I meant by "wasted space" earlier, but this could possibly be made nicer with macros as necessary.

As for this specific struct, the values should be identical as it is padded.

Best regards,
Marvin


Quoting C99 6.7.2.1 "Structure and union specifiers", paragraph 16:

"In most situations, the flexible array member is ignored. In
particular, the size of the structure is as if the flexible array member
were omitted except that it may have more trailing padding than the
omission would imply."

Quoting footnotes 17 and 19,

(17) [...]
struct s { int n; double d[]; };
[...]

(19) [...]
the expressions:
[...]
sizeof (struct s) >= offsetof(struct s, d)

are always equal to 1.

Thanks
Laszlo



While sizeof would be a preferred way to move
forward.

Regards,
Kun

On 06/24/2021 08:25, Kinney, Michael D wrote:
Hello,

Flexible array members are supported and should be used.  The old style
of adding an array of size [1] at the end of a structure was used at a
time
flexible array members were not supported by all compilers (late 1990's).
The workarounds used to handle the array of size [1] are very
confusing when
reading the C  code and the fact that sizeof() does not produce the
expected
result make it even worse.

If we use flexible array members in this proposed change then there is
no need to use OFFSET_OF().  Correct?

Mike


-----Original Message-----
From: Marvin Häuser <mhaeuser@posteo.de>
Sent: Thursday, June 24, 2021 1:00 AM
To: Kun Qin <kuqin12@gmail.com>; Laszlo Ersek <lersek@redhat.com>;
devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
<hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray
<ray.ni@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang
<zhiguang.liu@intel.com>; Andrew Fish <afish@apple.com>; Leif
Lindholm <leif@nuviainc.com>; Bret Barkelew
<Bret.Barkelew@microsoft.com>; michael.kubacki@microsoft.com
Subject: Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI
Specification: Update EFI_MM_COMMUNICATE_HEADER

Hey Kun,

Why would you rely on undefined behaviours? The OFFSET_OF macro is
well-defined for GCC and Clang as it's implemented by an intrinsic, and
while the expression for the MSVC compiler is undefined behaviour as per
the C standard, it is well-defined for MSVC due to their own
implementation being identical. From my standpoint, all supported
compilers will yield well-defined behaviour even this way. OFFSET_OF on
flexible arrays is not UB in any case to my knowledge.

However, the same way as your new suggestion, you can replace OFFSET_OF
with sizeof. While this *can* lead to wasted space with certain
structure layouts (e.g. when the flexible array overlays padding bytes),
this is not the case here, and otherwise just loses you a few bytes. I
think this comes down to preference.

The pattern you mentioned arguably is less nice syntax when used
(involves address calculation and casting), but the biggest problem here
is alignment constraints. For packed structures, you lose the ability of
automatic unaligned accesses (irrelevant here because the structure is
manually padded anyway). For non-packed structures, you still need to
ensure the alignment requirement of the trailing array data is met
manually. With flexible array members, the compiler takes care of both
cases automatically.

Best regards,
Marvin

On 24.06.21 02:24, Kun Qin wrote:
Hi Marvin,

I would prefer not to rely on undefined behaviors from different
compilers. Instead of using flexible arrays, is it better to remove
the `Data` field, pack the structure and follow
"VARIABLE_LOCK_ON_VAR_STATE_POLICY" pattern?

In that case, OFFSET_OF will be forced to change to sizeof, and
read/write to `Data` will follow the range indicated by MessageLength.
But yes, that will enforce developers to update their platform level
implementations accordingly.

Regards,
Kun

On 06/23/2021 08:26, Laszlo Ersek wrote:
On 06/23/21 08:54, Marvin Häuser wrote:
On 22.06.21 17:34, Laszlo Ersek wrote:
On 06/18/21 11:37, Marvin Häuser wrote:
On 16.06.21 22:58, Kun Qin wrote:
On 06/16/2021 00:02, Marvin Häuser wrote:
2) Is it feasible yet with the current set of supported
compilers to
support flexible arrays?
My impression is that flexible arrays are already supported (as
seen
in UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h).
Please correct me if I am wrong.

Would you mind letting me know why this is applicable here? We are
trying to seek ideas on how to catch developer mistakes caused by
this
change. So any input is appreciated.
Huh, interesting. Last time I tried I was told about
incompatibilities
with MSVC, but I know some have been dropped since then (2005 and
2008
if I recall correctly?), so that'd be great to allow globally.
I too am surprised to see
"UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h". The
flexible array member is a C99 feature, and I didn't even know
that we
disallowed it for the sake of particular VS toolchains -- I
thought we
had a more general reason than just "not supported by VS versions X
and Y".

The behavior of OFFSET_OF() would be interesting -- the OFFSET_OF()
macro definition for non-gcc / non-clang:

#define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))

borders on undefined behavior as far as I can tell, so its
behavior is
totally up to the compiler. It works thus far okay on Visual
Studio, but
I couldn't say if it extended correctly to flexible array members.
Yes, it's UB by the standard, but this is actually how MS implements
them (or used to anyway?). I don't see why it'd cause issues with
flexible arrays, as only the start of the array is relevant (which is
constant for all instances of the structure no matter the amount of
elements actually stored). Any specific concern? If so, they could be
addressed by appropriate STATIC_ASSERTs.
No specific concern; my point was that two aspects of the same "class"
of undefined behavior didn't need to be consistent with each other.

Thanks
Laszlo


Re: [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER

Laszlo Ersek
 

On 06/25/21 20:47, Kun Qin wrote:
Hi Mike,

Thanks for the information. I can update the patch and proposed spec
change to use flexible array in v-next if there is no other concerns.

After switching to flexible array, OFFSET_OF (Data) should lead to the
same result as sizeof.
This may be true on specific compilers, but it is *not* guaranteed by
the C language standard.

Quoting C99 6.7.2.1 "Structure and union specifiers", paragraph 16:

"In most situations, the flexible array member is ignored. In
particular, the size of the structure is as if the flexible array member
were omitted except that it may have more trailing padding than the
omission would imply."

Quoting footnotes 17 and 19,

(17) [...]
struct s { int n; double d[]; };
[...]

(19) [...]
the expressions:
[...]
sizeof (struct s) >= offsetof(struct s, d)

are always equal to 1.

Thanks
Laszlo



While sizeof would be a preferred way to move
forward.

Regards,
Kun

On 06/24/2021 08:25, Kinney, Michael D wrote:
Hello,

Flexible array members are supported and should be used.  The old style
of adding an array of size [1] at the end of a structure was used at a
time
flexible array members were not supported by all compilers (late 1990's).
The workarounds used to handle the array of size [1] are very
confusing when
reading the C  code and the fact that sizeof() does not produce the
expected
result make it even worse.

If we use flexible array members in this proposed change then there is
no need to use OFFSET_OF().  Correct?

Mike


-----Original Message-----
From: Marvin Häuser <mhaeuser@posteo.de>
Sent: Thursday, June 24, 2021 1:00 AM
To: Kun Qin <kuqin12@gmail.com>; Laszlo Ersek <lersek@redhat.com>;
devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
<hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray
<ray.ni@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang
<zhiguang.liu@intel.com>; Andrew Fish <afish@apple.com>; Leif
Lindholm <leif@nuviainc.com>; Bret Barkelew
<Bret.Barkelew@microsoft.com>; michael.kubacki@microsoft.com
Subject: Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI
Specification: Update EFI_MM_COMMUNICATE_HEADER

Hey Kun,

Why would you rely on undefined behaviours? The OFFSET_OF macro is
well-defined for GCC and Clang as it's implemented by an intrinsic, and
while the expression for the MSVC compiler is undefined behaviour as per
the C standard, it is well-defined for MSVC due to their own
implementation being identical. From my standpoint, all supported
compilers will yield well-defined behaviour even this way. OFFSET_OF on
flexible arrays is not UB in any case to my knowledge.

However, the same way as your new suggestion, you can replace OFFSET_OF
with sizeof. While this *can* lead to wasted space with certain
structure layouts (e.g. when the flexible array overlays padding bytes),
this is not the case here, and otherwise just loses you a few bytes. I
think this comes down to preference.

The pattern you mentioned arguably is less nice syntax when used
(involves address calculation and casting), but the biggest problem here
is alignment constraints. For packed structures, you lose the ability of
automatic unaligned accesses (irrelevant here because the structure is
manually padded anyway). For non-packed structures, you still need to
ensure the alignment requirement of the trailing array data is met
manually. With flexible array members, the compiler takes care of both
cases automatically.

Best regards,
Marvin

On 24.06.21 02:24, Kun Qin wrote:
Hi Marvin,

I would prefer not to rely on undefined behaviors from different
compilers. Instead of using flexible arrays, is it better to remove
the `Data` field, pack the structure and follow
"VARIABLE_LOCK_ON_VAR_STATE_POLICY" pattern?

In that case, OFFSET_OF will be forced to change to sizeof, and
read/write to `Data` will follow the range indicated by MessageLength.
But yes, that will enforce developers to update their platform level
implementations accordingly.

Regards,
Kun

On 06/23/2021 08:26, Laszlo Ersek wrote:
On 06/23/21 08:54, Marvin Häuser wrote:
On 22.06.21 17:34, Laszlo Ersek wrote:
On 06/18/21 11:37, Marvin Häuser wrote:
On 16.06.21 22:58, Kun Qin wrote:
On 06/16/2021 00:02, Marvin Häuser wrote:
2) Is it feasible yet with the current set of supported
compilers to
support flexible arrays?
My impression is that flexible arrays are already supported (as
seen
in UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h).
Please correct me if I am wrong.

Would you mind letting me know why this is applicable here? We are
trying to seek ideas on how to catch developer mistakes caused by
this
change. So any input is appreciated.
Huh, interesting. Last time I tried I was told about
incompatibilities
with MSVC, but I know some have been dropped since then (2005 and
2008
if I recall correctly?), so that'd be great to allow globally.
I too am surprised to see
"UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h". The
flexible array member is a C99 feature, and I didn't even know
that we
disallowed it for the sake of particular VS toolchains -- I
thought we
had a more general reason than just "not supported by VS versions X
and Y".

The behavior of OFFSET_OF() would be interesting -- the OFFSET_OF()
macro definition for non-gcc / non-clang:

#define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))

borders on undefined behavior as far as I can tell, so its
behavior is
totally up to the compiler. It works thus far okay on Visual
Studio, but
I couldn't say if it extended correctly to flexible array members.
Yes, it's UB by the standard, but this is actually how MS implements
them (or used to anyway?). I don't see why it'd cause issues with
flexible arrays, as only the start of the array is relevant (which is
constant for all instances of the structure no matter the amount of
elements actually stored). Any specific concern? If so, they could be
addressed by appropriate STATIC_ASSERTs.
No specific concern; my point was that two aspects of the same "class"
of undefined behavior didn't need to be consistent with each other.

Thanks
Laszlo


Re: [PATCH 4/6] NetworkPkg/IScsiDxe: support multiple hash algorithms for CHAP

Laszlo Ersek
 

Hi Maciej,

[snipping liberally, comments below]

On 06/25/21 16:56, Rabeda, Maciej wrote:
On 22-Jun-21 17:57, Laszlo Ersek wrote:
On 06/11/21 13:54, Rabeda, Maciej wrote:
On 08-Jun-21 15:06, Laszlo Ersek wrote:
+typedef struct {
+ UINT8 Algorithm; //
ISCSI_CHAP_ALGORITHM_*, CHAP_A
Any chance to define an enum type for Algorithm? IMHO it brings more
meaning to that number and limits the set of values it is expected
to have.
I picked UINT8 intentionally; I wanted to stay close to the
definition at

https://www.iana.org/assignments/ppp-numbers/ppp-numbers.xhtml#ppp-numbers-9


which says, "A one octet field". (Hence the reference to "CHAP_A" in
the comment as well.)

If we want an enum here, then I need to prepend a patch, for first
replacing the existent ISCSI_CHAP_ALGORITHM_MD5 macro with an enum
type / constant.

I still like UINT8 for this, but if you strongly prefer the enum, I
can certainly do it. Please advise :)
Okay, let's stick to UINT8 based on IANA definition.
Thanks!

- if (CompareMem (VerifyRsp, TargetResponse, MD5_DIGEST_SIZE) !=
0) {
+ if (CompareMem (VerifyRsp, TargetResponse,
+ AuthData->Hash->DigestSize) != 0) {
Status = EFI_SECURITY_VIOLATION;
}
Either break like regular function call or leave it in a single line
for readability, since UEFI coding standard section 5.1.1 allows for
lines up to 120. I opt for the latter, since param break will look
ugly, but if it looks better in your editor than a line over 80
chars in size - go for it.
My mistake, sorry -- I forgot that the "condensed style" that we
actually prefer in ArmVirtPkg and OvmfPkg is not accepted in other
packages.
Making it the other way around. Apart from historical reasons (EDK2
coding style evolving) - why is a different style accepted in other
packages? We have a EDK2 coding style document for a reason.
If it is not perfect, it does not suit our needs or simply hurts our
eyes (which 'param per line' in if statements does to me), we can try
to change it to the "condensed style".
* Submitted on 11 Aug 2017:

[edk2] [edk2-CCodingStandardsSpecification PATCH 0/2] improvements related to line wrapping
[edk2] [edk2-CCodingStandardsSpecification PATCH 1/2] Source Files / General Rules: limit line lengths to 80 columns
[edk2] [edk2-CCodingStandardsSpecification PATCH 2/2] Source Files / Spacing / Multi-line func. calls: allow condensed arguments

http://mid.mail-archive.com/20170811164851.9466-1-lersek@redhat.com
http://mid.mail-archive.com/20170811164851.9466-2-lersek@redhat.com
http://mid.mail-archive.com/20170811164851.9466-3-lersek@redhat.com

The condensed arguments style is described in patch 2/2.

* Filed in September 2017, from the discussion under patch 2/2:

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

In CONFIRMED status currently.

(The link to the original email discussion (in comment#0 of the BZ) no
longer works, because Intel has killed off the old <lists.01.org>
archive links meanwhile. But the <mail-archive.com> links should still
work.)

Thanks
Laszlo


[PATCH] OvmfPkg/XenPlatformPei: Relocate shared_info page mapping

Anthony PERARD
 

From: Anthony PERARD <anthony.perard@citrix.com>

Unfortunately, Xen isn't ready to deal with mapping at the top of the
physical address space, so we relocate the mapping after the LAPIC
location.

See this thread about the issue with the mapping:
- https://lore.kernel.org/xen-devel/f8c4151a-6dac-d87c-ef46-eb35ada07bd9@suse.com/

The PhysicalAddressIdentityMapping() call isn't going to do anything
anymore since everything under 4GB is already mapped, but there is no
need to remove the call.

CC: Jan Beulich <JBeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
OvmfPkg/XenPlatformPei/Xen.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
index a4e82b356936..9c6641895970 100644
--- a/OvmfPkg/XenPlatformPei/Xen.c
+++ b/OvmfPkg/XenPlatformPei/Xen.c
@@ -569,7 +569,7 @@ CalibrateLapicTimer (
EFI_STATUS Status;


- SharedInfo = (VOID*)((1ULL << mPhysMemAddressWidth) - EFI_PAGE_SIZE);
+ SharedInfo = (VOID*)((UINTN)PcdGet32 (PcdCpuLocalApicBaseAddress) + SIZE_1MB);
Status = PhysicalAddressIdentityMapping ((EFI_PHYSICAL_ADDRESS)SharedInfo);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR,
--
Anthony PERARD


Re: [PATCH v3 1/3] Acpi: reimplement PlatformHasAcpi for Cloud Hypervisor

Sami Mujawar
 

Hi Jianyong,

Thank you for this patch.

This patch looks good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

On 28/06/2021 10:55 AM, Jianyong Wu wrote:
The current implementation of PlatformHasAcpiDt is not a common
library and is on behalf of qemu. So give a specific version for
Cloud Hypervisor here.

Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
---
.../CloudHvHasAcpiDtDxe.inf | 43 ++++++++++++
.../CloudHvHasAcpiDtDxe.c | 69 +++++++++++++++++++
2 files changed, 112 insertions(+)
create mode 100644 ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.inf
create mode 100644 ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.c

diff --git a/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.inf b/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.inf
new file mode 100644
index 000000000000..eb63a4136545
--- /dev/null
+++ b/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.inf
@@ -0,0 +1,43 @@
+## @file
+# Decide whether the firmware should expose an ACPI- and/or a Device Tree-based
+# hardware description to the operating system.
+#
+# Copyright (c) 2021, Arm Limited. All rights reserved.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+
+[Defines]
+ INF_VERSION = 1.25
+ BASE_NAME = CloudHvPlatformHasAcpiDtDxe
+ FILE_GUID = 71fe72f9-6dc1-199d-5054-13b4200ee88d
+ MODULE_TYPE = DXE_DRIVER
+ VERSION_STRING = 1.0
+ ENTRY_POINT = PlatformHasAcpiDt
+
+[Sources]
+ CloudHvHasAcpiDtDxe.c
+
+[Packages]
+ ArmVirtPkg/ArmVirtPkg.dec
+ EmbeddedPkg/EmbeddedPkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ MdePkg/MdePkg.dec
+ OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+ BaseLib
+ DebugLib
+ PcdLib
+ UefiBootServicesTableLib
+ UefiDriverEntryPoint
+
+[Guids]
+ gEdkiiPlatformHasAcpiGuid ## SOMETIMES_PRODUCES ## PROTOCOL
+ gEdkiiPlatformHasDeviceTreeGuid ## SOMETIMES_PRODUCES ## PROTOCOL
+
+[Pcd]
+ gArmVirtTokenSpaceGuid.PcdForceNoAcpi
+
+[Depex]
+ gEfiVariableArchProtocolGuid
diff --git a/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.c b/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.c
new file mode 100644
index 000000000000..48a446c68a45
--- /dev/null
+++ b/ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.c
@@ -0,0 +1,69 @@
+/** @file
+ Decide whether the firmware should expose an ACPI- and/or a Device Tree-based
+ hardware description to the operating system.
+
+ Copyright (c) 2021, Arm Limited. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <Guid/PlatformHasAcpi.h>
+#include <Guid/PlatformHasDeviceTree.h>
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+#include <Library/PcdLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+
+EFI_STATUS
+EFIAPI
+PlatformHasAcpiDt (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ EFI_STATUS Status;
+
+ //
+ // If we fail to install any of the necessary protocols below, the OS will be
+ // unbootable anyway (due to lacking hardware description), so tolerate no
+ // errors here.
+ //
+ if (MAX_UINTN == MAX_UINT64 &&
+ !PcdGetBool (PcdForceNoAcpi))
+ {
+ Status = gBS->InstallProtocolInterface (
+ &ImageHandle,
+ &gEdkiiPlatformHasAcpiGuid,
+ EFI_NATIVE_INTERFACE,
+ NULL
+ );
+ if (EFI_ERROR (Status)) {
+ goto Failed;
+ }
+
+ return Status;
+ }
+
+ //
+ // Expose the Device Tree otherwise.
+ //
+ Status = gBS->InstallProtocolInterface (
+ &ImageHandle,
+ &gEdkiiPlatformHasDeviceTreeGuid,
+ EFI_NATIVE_INTERFACE,
+ NULL
+ );
+ if (EFI_ERROR (Status)) {
+ goto Failed;
+ }
+
+ return Status;
+
+Failed:
+ ASSERT_EFI_ERROR (Status);
+ CpuDeadLoop ();
+ //
+ // Keep compilers happy.
+ //
+ return Status;
+}


Re: [PATCH v3 3/3] ArmVirtCloudHv: support Cloud Hypervisor in edk2

Sami Mujawar
 

Hi Jianyong,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

On 28/06/2021 10:55 AM, Jianyong Wu wrote:
Cloud Hypervisor is KVM based VMM and is implemented in rust. Just like
other VMMs it needs UEFI support to let ACPI work. That is why
Cloud Hypervisor is introduced here.

Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
---
ArmVirtPkg/ArmVirtCloudHv.dsc | 397 ++++++++++++++++++++++++++++++++++
ArmVirtPkg/ArmVirtCloudHv.fdf | 274 +++++++++++++++++++++++
2 files changed, 671 insertions(+)
create mode 100644 ArmVirtPkg/ArmVirtCloudHv.dsc
create mode 100644 ArmVirtPkg/ArmVirtCloudHv.fdf

diff --git a/ArmVirtPkg/ArmVirtCloudHv.dsc b/ArmVirtPkg/ArmVirtCloudHv.dsc
new file mode 100644
index 000000000000..0d811971aad7
--- /dev/null
+++ b/ArmVirtPkg/ArmVirtCloudHv.dsc
@@ -0,0 +1,397 @@
+#
+# Copyright (c) 2021, ARM Limited. All rights reserved.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+#
+
+################################################################################
+#
+# Defines Section - statements that will be processed to create a Makefile.
+#
+################################################################################
+[Defines]
+ PLATFORM_NAME = ArmVirtCloudHv
+ PLATFORM_GUID = DFFED32B-DFFE-D32B-DFFE-D32BDFFED32B
+ PLATFORM_VERSION = 0.1
+ DSC_SPECIFICATION = 0x00010005
+ OUTPUT_DIRECTORY = Build/ArmVirtCloudHv-$(ARCH)
+ SUPPORTED_ARCHITECTURES = AARCH64|ARM
+ BUILD_TARGETS = DEBUG|RELEASE|NOOPT
+ SKUID_IDENTIFIER = DEFAULT
+ FLASH_DEFINITION = ArmVirtPkg/ArmVirtCloudHv.fdf
+
+ #
+ # Defines for default states. These can be changed on the command line.
+ # -D FLAG=VALUE
+ #
+ DEFINE TTY_TERMINAL = FALSE
+ DEFINE SECURE_BOOT_ENABLE = FALSE
+ DEFINE TPM2_ENABLE = FALSE
+ DEFINE TPM2_CONFIG_ENABLE = FALSE
[SAMI] Is TPM2 supported on CloudHypervisor? If not, would it be good to remove these options. Otherwise it may confuse someone who would try to enable them.
+
+!include ArmVirtPkg/ArmVirt.dsc.inc
+
+[LibraryClasses.common]
+ ArmLib|ArmPkg/Library/ArmLib/ArmBaseLib.inf
+ ArmMmuLib|ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
+
+ # Virtio Support
+ VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
+ VirtioMmioDeviceLib|OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
+
+ ArmPlatformLib|ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.inf
+
+ TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf
+ CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
+ BootLogoLib|MdeModulePkg/Library/BootLogoLib/BootLogoLib.inf
+ PlatformBootManagerLib|ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
+ PlatformBmPrintScLib|OvmfPkg/Library/PlatformBmPrintScLib/PlatformBmPrintScLib.inf
+ CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
+ FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf
+ QemuBootOrderLib|OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf
+ FileExplorerLib|MdeModulePkg/Library/FileExplorerLib/FileExplorerLib.inf
+ PciPcdProducerLib|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
+ PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
+ PciHostBridgeLib|ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf
+ PciHostBridgeUtilityLib|ArmVirtPciHostBridgeUtilityLib/ArmVirtPciHostBridgeUtilityLib.inf
+
+!if $(TPM2_ENABLE) == TRUE
+ Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
+ TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
+!else
+ TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
+!endif
+
+!include MdePkg/MdeLibs.dsc.inc
+
+[LibraryClasses.common.PEIM]
+ ArmVirtMemInfoLib|ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
[SAMI] Minor, 2 spaces at the begining, instead of 3.
+
+!if $(TPM2_ENABLE) == TRUE
+ BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
+ ResetSystemLib|MdeModulePkg/Library/PeiResetSystemLib/PeiResetSystemLib.inf
+ Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
+!endif
+
+[LibraryClasses.common.DXE_DRIVER]
+ ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
+
+[LibraryClasses.common.UEFI_DRIVER]
+ UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
+
+[BuildOptions]
+!include NetworkPkg/NetworkBuildOptions.dsc.inc
+
+################################################################################
+#
+# Pcd Section - list of all EDK II PCD Entries defined by this Platform
+#
+################################################################################
+
+[PcdsFeatureFlag.common]
+ ## If TRUE, Graphics Output Protocol will be installed on virtual handle created by ConsplitterDxe.
+ # It could be set FALSE to save size.
+ gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|TRUE
+ gEfiMdeModulePkgTokenSpaceGuid.PcdConOutUgaSupport|FALSE
+
+ gEfiMdeModulePkgTokenSpaceGuid.PcdTurnOffUsbLegacySupport|TRUE
+
+ gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled|$(TPM2_ENABLE)
+
+[PcdsFixedAtBuild.common]
+!if $(ARCH) == AARCH64
+ gArmTokenSpaceGuid.PcdVFPEnabled|1
+!endif
+
+ gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x4007c000
+ gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0
+ gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x4000
+ gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
+ gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
+
+ # Rsdp base address in Cloud Hypervisor
+ gArmVirtTokenSpaceGuid.PcdCloudHvAcpiRsdpBaseAddress|0x40200000
+
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0x4000000
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x40000
+ gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable|TRUE
+!if $(NETWORK_TLS_ENABLE) == TRUE
+ #
+ # The cumulative and individual VOLATILE variable size limits should be set
+ # high enough for accommodating several and/or large CA certificates.
+ #
+ gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x80000
+ gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize|0x40000
+!endif
+
+ # Size of the region used by UEFI in permanent memory (Reserved 64MB)
+ gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize|0x04000000
+
+ #
+ # ARM PrimeCell
+ #
+
+ ## PL011 - Serial Terminal
+ gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|38400
+
+ ## Default Terminal Type
+ ## 0-PCANSI, 1-VT100, 2-VT00+, 3-UTF8, 4-TTYTERM
+ gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|4
+
+ # System Memory Base -- fixed at 0x4000_0000
+ gArmTokenSpaceGuid.PcdSystemMemoryBase|0x40000000
+
+ # initial location of the device tree blob passed by Cloud Hypervisor -- base of DRAM
+ gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress|0x40000000
+
+
[SAMI] Minor, please remove extra blank line.
+ gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
+ gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{ 0x21, 0xaa, 0x2c, 0x46, 0x14, 0x76, 0x03, 0x45, 0x83, 0x6e, 0x8a, 0xb6, 0xf4, 0x66, 0x23, 0x31 }
+
+ #
+ # The maximum physical I/O addressability of the processor, set with
+ # BuildCpuHob().
+ #
+ gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|16
+
+ #
+ # Enable the non-executable DXE stack. (This gets set up by DxeIpl)
+ #
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
+
+!if $(SECURE_BOOT_ENABLE) == TRUE
+ # override the default values from SecurityPkg to ensure images from all sources are verified in secure boot
+ gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x04
+ gEfiSecurityPkgTokenSpaceGuid.PcdFixedMediaImageVerificationPolicy|0x04
+ gEfiSecurityPkgTokenSpaceGuid.PcdRemovableMediaImageVerificationPolicy|0x04
+!endif
+
+ gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|3
+ gEfiShellPkgTokenSpaceGuid.PcdShellFileOperationSize|0x20000
+
+[PcdsFixedAtBuild.AARCH64]
+ # Clearing BIT0 in this PCD prevents installing a 32-bit SMBIOS entry point,
+ # if the entry point version is >= 3.0. AARCH64 OSes cannot assume the
+ # presence of the 32-bit entry point anyway (because many AARCH64 systems
+ # don't have 32-bit addressable physical RAM), and the additional allocations
+ # below 4 GB needlessly fragment the memory map. So expose the 64-bit entry
+ # point only, for entry point versions >= 3.0.
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosEntryPointProvideMethod|0x2
+
+[PcdsDynamicDefault.common]
+ gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
+
+ ## If TRUE, OvmfPkg/AcpiPlatformDxe will not wait for PCI
+ # enumeration to complete before installing ACPI tables.
+ gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration|TRUE
+
+ # System Memory Size -- 1 MB initially, actual size will be fetched from DT
+ gArmTokenSpaceGuid.PcdSystemMemorySize|0x00100000
+
+ gArmTokenSpaceGuid.PcdArmArchTimerSecIntrNum|0x0
+ gArmTokenSpaceGuid.PcdArmArchTimerIntrNum|0x0
+ gArmTokenSpaceGuid.PcdArmArchTimerVirtIntrNum|0x0
+ gArmTokenSpaceGuid.PcdArmArchTimerHypIntrNum|0x0
+
+ #
+ # ARM General Interrupt Controller
+ #
+ gArmTokenSpaceGuid.PcdGicDistributorBase|0x0
+ gArmTokenSpaceGuid.PcdGicRedistributorsBase|0x0
+ gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase|0x0
+
+ ## PL031 RealTimeClock
+ gArmPlatformTokenSpaceGuid.PcdPL031RtcBase|0x0
+
+ # set PcdPciExpressBaseAddress to MAX_UINT64, which signifies that this
+ # PCD and PcdPciDisableBusEnumeration above have not been assigned yet
+ gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xFFFFFFFFFFFFFFFF
+
+ gArmTokenSpaceGuid.PcdPciIoTranslation|0
+
+ #
+ # TPM2 support
+ #
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress|0x0
+!if $(TPM2_ENABLE) == TRUE
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpm2HashMask|0
+!endif
+
+[PcdsDynamicHii]
+ gArmVirtTokenSpaceGuid.PcdForceNoAcpi|L"ForceNoAcpi"|gArmVirtVariableGuid|0x0|FALSE|NV,BS
+
+################################################################################
+#
+# Components Section - list of all EDK II Modules needed by this Platform
+#
+################################################################################
+[Components.common]
+ #
+ # PEI Phase modules
+ #
+ ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
+ MdeModulePkg/Core/Pei/PeiMain.inf
+ MdeModulePkg/Universal/PCD/Pei/Pcd.inf {
+ <LibraryClasses>
+ PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
+ }
+ ArmPlatformPkg/PlatformPei/PlatformPeim.inf
+ ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
+ ArmPkg/Drivers/CpuPei/CpuPei.inf
+
+ MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
+
+!if $(TPM2_ENABLE) == TRUE
+ MdeModulePkg/Universal/ResetSystemPei/ResetSystemPei.inf {
+ <LibraryClasses>
+ ResetSystemLib|ArmVirtPkg/Library/ArmVirtPsciResetSystemPeiLib/ArmVirtPsciResetSystemPeiLib.inf
+ }
+!endif
+
+ MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf {
+ <LibraryClasses>
+ NULL|MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecompressLib.inf
+ }
+
+ #
+ # DXE
+ #
+ MdeModulePkg/Core/Dxe/DxeMain.inf {
+ <LibraryClasses>
+ NULL|MdeModulePkg/Library/DxeCrc32GuidedSectionExtractLib/DxeCrc32GuidedSectionExtractLib.inf
+ DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
+ }
+ MdeModulePkg/Universal/PCD/Dxe/Pcd.inf {
+ <LibraryClasses>
+ PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
+ }
+
+ #
+ # Architectural Protocols
+ #
+ ArmPkg/Drivers/CpuDxe/CpuDxe.inf
+ MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
+ MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
+ <LibraryClasses>
+ NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
+ # don't use unaligned CopyMem () on the UEFI varstore NOR flash region
+ BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
+ }
+!if $(SECURE_BOOT_ENABLE) == TRUE
+ MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
+ <LibraryClasses>
+ NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
+!if $(TPM2_ENABLE) == TRUE
+ NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
+!endif
+ }
+ SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
+ OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf
+!else
+ MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
+!endif
+ MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
+ MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
+ MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
+ MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf
+ EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf {
+ <LibraryClasses>
+ NULL|ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf
+ }
+ EmbeddedPkg/MetronomeDxe/MetronomeDxe.inf
+
+ MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatformDxe.inf
+ MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf
+ MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf
+ MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
+ MdeModulePkg/Universal/SerialDxe/SerialDxe.inf
+
+ MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf
+
+ ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
+ ArmPkg/Drivers/TimerDxe/TimerDxe.inf {
+ <LibraryClasses>
+ NULL|ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.inf
+ }
+ MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
+
+ #
+ # Status Code Routing
+ #
+ MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/ReportStatusCodeRouterRuntimeDxe.inf
+
+ #
+ # Platform Driver
+ #
+ ArmVirtPkg/VirtioFdtDxe/VirtioFdtDxe.inf
+ ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
+ ArmVirtPkg/HighMemDxe/HighMemDxe.inf
+ OvmfPkg/VirtioBlkDxe/VirtioBlk.inf
+ OvmfPkg/VirtioScsiDxe/VirtioScsi.inf
+ OvmfPkg/VirtioNetDxe/VirtioNet.inf
+ OvmfPkg/VirtioRngDxe/VirtioRng.inf
+
+ #
+ # FAT filesystem + GPT/MBR partitioning + UDF filesystem + virtio-fs
+ #
+ MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIoDxe.inf
+ MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf
+ MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
+ FatPkg/EnhancedFatDxe/Fat.inf
+ MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf
+ OvmfPkg/VirtioFsDxe/VirtioFsDxe.inf
+
+ #
+ # Bds
+ #
+ MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf {
+ <LibraryClasses>
+ DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
+ PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
+ }
+ MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
+ MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
+ MdeModulePkg/Universal/DriverHealthManagerDxe/DriverHealthManagerDxe.inf
+ MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
+ MdeModulePkg/Logo/LogoDxe.inf
+ MdeModulePkg/Application/UiApp/UiApp.inf {
+ <LibraryClasses>
+ NULL|MdeModulePkg/Library/DeviceManagerUiLib/DeviceManagerUiLib.inf
+ NULL|MdeModulePkg/Library/BootManagerUiLib/BootManagerUiLib.inf
+ NULL|MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanceManagerUiLib.inf
+ }
+
+ #
+ # SCSI Bus and Disk Driver
+ #
+ MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
+ MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
+
+ #
+ # PCI support
+ #
+ ArmPkg/Drivers/ArmPciCpuIo2Dxe/ArmPciCpuIo2Dxe.inf {
+ <LibraryClasses>
+ NULL|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
+ }
+ MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
+ MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf {
+ <LibraryClasses>
+ NULL|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
+ }
+ OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
+ OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
+ OvmfPkg/Virtio10Dxe/Virtio10.inf
+
+ #
+ # ACPI Support
+ #
+ ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.inf
+[Components.AARCH64]
+ MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf
+ ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf {
+ <LibraryClasses>
+ NULL|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
+ }
diff --git a/ArmVirtPkg/ArmVirtCloudHv.fdf b/ArmVirtPkg/ArmVirtCloudHv.fdf
new file mode 100644
index 000000000000..47243113409b
--- /dev/null
+++ b/ArmVirtPkg/ArmVirtCloudHv.fdf
@@ -0,0 +1,274 @@
+#
+# Copyright (c) 2021, ARM Limited. All rights reserved.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+
+################################################################################
+#
+# FD Section
+# The [FD] Section is made up of the definition statements and a
+# description of what goes into the Flash Device Image. Each FD section
+# defines one flash "device" image. A flash device image may be one of
+# the following: Removable media bootable image (like a boot floppy
+# image,) an Option ROM image (that would be "flashed" into an add-in
+# card,) a System "Flash" image (that would be burned into a system's
+# flash) or an Update ("Capsule") image that will be used to update and
+# existing system flash.
+#
+################################################################################
+
+[Defines]
+!if $(FD_SIZE_IN_MB) == 2
+ DEFINE FVMAIN_COMPACT_SIZE = 0x1ff000
+!endif
+!if $(FD_SIZE_IN_MB) == 3
+ DEFINE FVMAIN_COMPACT_SIZE = 0x2ff000
+!endif
+
+[FD.CLOUDHV_EFI]
+BaseAddress = 0x00000000|gArmTokenSpaceGuid.PcdFdBaseAddress # cloud-hypervisor assigns 0 - 0x8000000 for a BootROM
+Size = $(FD_SIZE)|gArmTokenSpaceGuid.PcdFdSize # The size in bytes of the FLASH Device
+ErasePolarity = 1
+
+# This one is tricky, it must be: BlockSize * NumBlocks = Size
+BlockSize = 0x00001000
+NumBlocks = $(FD_NUM_BLOCKS)
+
+################################################################################
+#
+# Following are lists of FD Region layout which correspond to the locations of different
+# images within the flash device.
+#
+# Regions must be defined in ascending order and may not overlap.
+#
+# A Layout Region start with a eight digit hex offset (leading "0x" required) followed by
+# the pipe "|" character, followed by the size of the region, also in hex with the leading
+# "0x" characters. Like:
+# Offset|Size
+# PcdOffsetCName|PcdSizeCName
+# RegionType <FV, DATA, or FILE>
+#
+################################################################################
+
+#
+# UEFI has trouble dealing with FVs that reside at physical address 0x0.
+# So instead, put a hardcoded 'jump to 0x1000' at offset 0x0, and put the
+# real FV at offset 0x1000
+#
+0x00000000|0x00001000
+DATA = {
+!if $(ARCH) == AARCH64
+ 0x00, 0x04, 0x00, 0x14 # 'b 0x1000' in AArch64 ASM
+!else
+ 0xfe, 0x03, 0x00, 0xea # 'b 0x1000' in AArch32 ASM
+!endif
+}
+
+0x00001000|$(FVMAIN_COMPACT_SIZE)
+gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize
+FV = FVMAIN_COMPACT
+
+!include VarStore.fdf.inc
+
+################################################################################
+#
+# FV Section
+#
+# [FV] section is used to define what components or modules are placed within a flash
+# device file. This section also defines order the components and modules are positioned
+# within the image. The [FV] section consists of define statements, set statements and
+# module statements.
+#
+################################################################################
+
+[FV.FvMain]
+FvNameGuid = 2A88A00E-E267-C8BF-0E80-AE1BD504ED90
+BlockSize = 0x40
+NumBlocks = 0 # This FV gets compressed so make it just big enough
+FvAlignment = 16 # FV alignment and FV attributes setting.
+ERASE_POLARITY = 1
+MEMORY_MAPPED = TRUE
+STICKY_WRITE = TRUE
+LOCK_CAP = TRUE
+LOCK_STATUS = TRUE
+WRITE_DISABLED_CAP = TRUE
+WRITE_ENABLED_CAP = TRUE
+WRITE_STATUS = TRUE
+WRITE_LOCK_CAP = TRUE
+WRITE_LOCK_STATUS = TRUE
+READ_DISABLED_CAP = TRUE
+READ_ENABLED_CAP = TRUE
+READ_STATUS = TRUE
+READ_LOCK_CAP = TRUE
+READ_LOCK_STATUS = TRUE
+
+ INF MdeModulePkg/Core/Dxe/DxeMain.inf
+ INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
+ INF ArmVirtPkg/VirtioFdtDxe/VirtioFdtDxe.inf
+ INF ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
+ INF ArmVirtPkg/HighMemDxe/HighMemDxe.inf
+
+ #
+ # PI DXE Drivers producing Architectural Protocols (EFI Services)
+ #
+ INF ArmPkg/Drivers/CpuDxe/CpuDxe.inf
+ INF MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
+ INF MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
+ INF MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
+ INF MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
+ INF MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
+!if $(SECURE_BOOT_ENABLE) == TRUE
+ INF SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
+!endif
+ INF MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
+ INF MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf
+ INF EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf
+ INF EmbeddedPkg/MetronomeDxe/MetronomeDxe.inf
+ INF MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf
+
+ #
+ # Multiple Console IO support
+ #
+ INF MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatformDxe.inf
+ INF MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf
+ INF MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf
+ INF MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf
+ INF MdeModulePkg/Universal/SerialDxe/SerialDxe.inf
+
+ INF ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
+ INF ArmPkg/Drivers/TimerDxe/TimerDxe.inf
+ INF MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
+
+ #
+ # FAT filesystem + GPT/MBR partitioning + UDF filesystem + virtio-fs
+ #
+ INF MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIoDxe.inf
+ INF MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf
+ INF FatPkg/EnhancedFatDxe/Fat.inf
+ INF MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
+ INF MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf
+ INF OvmfPkg/VirtioFsDxe/VirtioFsDxe.inf
+
+ #
+ # Status Code Routing
+ #
+ INF MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/ReportStatusCodeRouterRuntimeDxe.inf
+
+ #
+ # Platform Driver
+ #
+ INF OvmfPkg/VirtioBlkDxe/VirtioBlk.inf
+ INF OvmfPkg/VirtioNetDxe/VirtioNet.inf
+ INF OvmfPkg/VirtioScsiDxe/VirtioScsi.inf
+ INF OvmfPkg/VirtioRngDxe/VirtioRng.inf
+
+ #
+ # UEFI application (Shell Embedded Boot Loader)
+ #
+ INF ShellPkg/Application/Shell/Shell.inf
+ INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
+ INF ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf
+ INF OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
+
+ #
+ # Bds
+ #
+ INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
+ INF MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
+ INF MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
+ INF MdeModulePkg/Universal/DriverHealthManagerDxe/DriverHealthManagerDxe.inf
+ INF MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
+ INF MdeModulePkg/Application/UiApp/UiApp.inf
+
+ #
+ # SCSI Bus and Disk Driver
+ #
+ INF MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
+ INF MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
+
+ #
+ # ACPI Support
+ #
+ INF ArmVirtPkg/CloudHvPlatformHasAcpiDtDxe/CloudHvHasAcpiDtDxe.inf
+!if $(ARCH) == AARCH64
+ INF MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
+ INF MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf
+ INF ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf
+
+ #
+ # EBC support
+ #
+ INF MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
+!endif
+
+ #
+ # PCI support
+ #
+ INF ArmPkg/Drivers/ArmPciCpuIo2Dxe/ArmPciCpuIo2Dxe.inf
+ INF MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
+ INF MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
+ INF OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf
+ INF OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
+ INF OvmfPkg/Virtio10Dxe/Virtio10.inf
+
+ #
+ # TPM2 support
+ #
+!if $(TPM2_ENABLE) == TRUE
+ INF SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf
+!if $(TPM2_CONFIG_ENABLE) == TRUE
+ INF SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
+!endif
+!endif
+
+ #
+ # TianoCore logo (splash screen)
+ #
+ INF MdeModulePkg/Logo/LogoDxe.inf
+
+ #
+ # Ramdisk support
+ #
+ INF MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
+
+[FV.FVMAIN_COMPACT]
+FvAlignment = 16
+ERASE_POLARITY = 1
+MEMORY_MAPPED = TRUE
+STICKY_WRITE = TRUE
+LOCK_CAP = TRUE
+LOCK_STATUS = TRUE
+WRITE_DISABLED_CAP = TRUE
+WRITE_ENABLED_CAP = TRUE
+WRITE_STATUS = TRUE
+WRITE_LOCK_CAP = TRUE
+WRITE_LOCK_STATUS = TRUE
+READ_DISABLED_CAP = TRUE
+READ_ENABLED_CAP = TRUE
+READ_STATUS = TRUE
+READ_LOCK_CAP = TRUE
+READ_LOCK_STATUS = TRUE
+
+ INF ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
+ INF MdeModulePkg/Core/Pei/PeiMain.inf
+ INF ArmPlatformPkg/PlatformPei/PlatformPeim.inf
+ INF ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
+ INF ArmPkg/Drivers/CpuPei/CpuPei.inf
+ INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
+ INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
+ INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+
+!if $(TPM2_ENABLE) == TRUE
+ INF MdeModulePkg/Universal/ResetSystemPei/ResetSystemPei.inf
+ INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+ INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
+!endif
+
+ FILE FV_IMAGE = 9E21FD93-9C72-4c15-8C4B-E77F1DB2D792 {
+ SECTION GUIDED EE4E5898-3914-4259-9D6E-DC7BD79403CF PROCESSING_REQUIRED = TRUE {
+ SECTION FV_IMAGE = FVMAIN
+ }
+ }
+
+!include ArmVirtRules.fdf.inc


Re: [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

Laszlo Ersek
 

On 06/24/21 00:07, Kinney, Michael D wrote:
Hi Laszlo,

I understand your point.

I am trying to balance the ease of use for developers, reducing overhead for maintainers, and
prevent bad commits.

I think you are saying that you want to make sure a maintainer carefully reviews changes
across multiple PRs that are in the same area of code. The CI checks will of course make
sure the code builds and passes the basic boot tests, but those tests do not have full
coverage so an interaction issue between two PRs that pass build and boot but have
unintended behavior side effects are what require detailed manual review.

I am going to remove the auto-rebase by default and add a optional label that can
be set by a maintainer to enable auto-rebase. If a maintainer is confident that
a set of PRs being submitted at the same time with the 'push' label are independent,
then the maintainer can also set 'auto-rebase'. If they are not confident, then
they can send PRs one at a time with only 'push' label and manually rebase each
additional PR and review the manual rebase to make sure there are no unintended
side effects.
Sounds great, thank you!
Laszlo


Any objections to this direction?

Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
Sent: Wednesday, June 23, 2021 12:45 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
<jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

On 06/23/21 20:44, Kinney, Michael D wrote:

Hi Laszlo,

Thank you for the test case.

I created 2 PRs against edk2-codereview using your patches.
I made minor update to commit messages to pass patch check.

https://github.com/tianocore/edk2-codereview/pull/18
https://github.com/tianocore/edk2-codereview/pull/19

Found another issue with PatchCheck for the Mergify merge commit and
fixed that.

Mergify did process #18 and merged it in after passing all CI. Mergify
rebased #19 successfully and merged it after passing all CI. I do not
think this was your expected result.
Indeed, my "desired" result at least would have been for mergify to
reject the rebase.

I looked more closely at the patches you provided. They were not
overlapping in the lines of Readme.rst. This is why no merge conflict
was detected.
More precisely, a contextual conflict *was* determined between the
patches, but that conflict was auto-resolved.

This is risky when done in an automated fashion. It is an extremely
convenient feature of git, when used interactively; that is, when the
auto-merge (automatic conflict resolution) is semantically verified by a
human. Git takes away the chore of conflict resolution, presents a
"likely good" end result, and a human only needs to *look* at the end
result, not *implement* it.

But that "human look" is exactly what's missing from mergify.

Basically what I'd like for mergify is to turn off automatic conflict
resolution.

More or less, speaking in terms of the stand-alone "patch" utility
<https://man7.org/linux/man-pages/man1/patch.1.html>, my preference is
to set the "fuzz factor" to zero.


One way a human reviews such context differences is with git-range-diff.
Continuing my previous example commands:

$ git range-diff --color master..b2 b1..b2-rebase

1: 02dc81e58bd6 ! 1: 2cf39d4b1790 world
@@ -6,8 +6,8 @@
--- a/ReadMe.rst
+++ b/ReadMe.rst
@@
-
A modern, feature-rich, cross-platform firmware development
+ HELLO
environment for the UEFI and PI specifications from www.uefi.org.
+ WORLD

This output shows that the "world" addition is the same (it is identical
between pre-rebase and post-rebase in the commit), but the context has
changed. During the rebase, the leading empty line of the context
disappeared, and a HELLO line in the middle of the leading context
appeared.

This result may or may not be semantically correct; it needs a human
decision. What if the original purpose of the "world" patch author was
to say WORLD but only without HELLO? When they looked at the code, there
was no HELLO yet.

git-range-diff is very powerful, but reading its output takes some
getting used to. (Colorization with the "--color" option is basically
required for understanding; I can't reproduce it in this email, alas.)

I don't want to obsess about this forever, I just want us all to be
aware that this risk exists.

Thanks,
Laszlo


I then created 2 new PRs that added text to the same line # in Readme.rst.

https://github.com/tianocore/edk2-codereview/pull/21
https://github.com/tianocore/edk2-codereview/pull/22

PR #21 passed all CI tests and was merged. Mergify then attempted to
rebase #22 and got a merge conflict and is still in the open state waiting
for the developer to manually handle the merge conflict.
This case is not worrisome; when there is a clear conflict that cannot be auto-resolved, I'm not concerned.

My concern is the sneaky contextual conflict that *appears* auto-resolvable, but is semantically broken. Those things
exist.

Thanks
Laszlo





Re: [PATCH v3 2/3] Acpi: Install Acpi tables for Cloud hypervisor

Sami Mujawar
 

Hi Jianyong,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

On 28/06/2021 10:55 AM, Jianyong Wu wrote:
There is no device like Fw-cfg in Qemu in Cloud Hypervisor, so a specific
Acpi handler is introduced here.

The handler implemented here is in a very simple way:
1. acquire the RSDP from the PCD variable in the top ".dsc";
2. get the XSDT address from RSDP structure;
3. get the ACPI tables following the XSDT structure and install them
one by one;
4. get DSDT address from FADT and install DSDT table.

Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
---
ArmVirtPkg/ArmVirtPkg.dec | 6 +
.../CloudHvAcpiPlatformDxe.inf | 47 ++++++
.../CloudHvAcpiPlatformDxe/CloudHvAcpi.c | 141 ++++++++++++++++++
3 files changed, 194 insertions(+)
create mode 100644 ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf
create mode 100644 ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c

diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
index bf82f7f1f3f2..4e4d758015bc 100644
--- a/ArmVirtPkg/ArmVirtPkg.dec
+++ b/ArmVirtPkg/ArmVirtPkg.dec
@@ -66,6 +66,12 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
#
gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, 0x4D}|VOID*|0x00000007
+ ##
+ # This is the physical address of Rsdp which is the core struct of Acpi.
+ # Cloud Hypervisor has no other way to pass Rsdp address to the guest except use a PCD.
+ #
+ gArmVirtTokenSpaceGuid.PcdCloudHvAcpiRsdpBaseAddress|0x0|UINT64|0x00000005
+
[PcdsDynamic]
#
# Whether to force disable ACPI, regardless of the fw_cfg settings
diff --git a/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf
new file mode 100644
index 000000000000..01de76486686
--- /dev/null
+++ b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf
@@ -0,0 +1,47 @@
+## @file
+# ACPI Platform Driver for Cloud Hypervisor
+#
+# Copyright (c) 2021, ARM Limited. All rights reserved.<BR>
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION = 0x00010005
+ BASE_NAME = CloudHvgAcpiPlatform
+ FILE_GUID = 6c76e407-73f2-dc1c-938f-5d6c4691ea93
+ MODULE_TYPE = DXE_DRIVER
+ VERSION_STRING = 1.0
+ ENTRY_POINT = CloudHvAcpiPlatformEntryPoint
+
+#
+# The following information is for reference only and not required by the build tools.
+#
+ VALID_ARCHITECTURES = AARCH64
+#
+
+[Sources]
+ CloudHvAcpi.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ OvmfPkg/OvmfPkg.dec
+ ArmVirtPkg/ArmVirtPkg.dec
+
+[LibraryClasses]
+ BaseLib
+ DebugLib
+ MemoryAllocationLib
+ OrderedCollectionLib
+ UefiBootServicesTableLib
+ UefiDriverEntryPoint
+
+[Protocols]
+ gEfiAcpiTableProtocolGuid # PROTOCOL ALWAYS_CONSUMED
+
+[Pcd]
+ gArmVirtTokenSpaceGuid.PcdCloudHvAcpiRsdpBaseAddress
+
+[Depex]
+ gEfiAcpiTableProtocolGuid
diff --git a/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
new file mode 100644
index 000000000000..0f1a50d63cd6
--- /dev/null
+++ b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
@@ -0,0 +1,141 @@
+/** @file
+ Install Acpi tables for Cloud Hypervisor
+
+ Copyright (c) 2021, Arm Limited. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <Library/BaseLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <IndustryStandard/Acpi63.h>
+#include <Protocol/AcpiTable.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiDriverEntryPoint.h>
+#include <Library/DebugLib.h>
+
+/**
+ Find Acpi table Protocol and return it
[SAMI]Please add description of value returned by this function.
[/SAMI]
+**/
+STATIC
+EFI_ACPI_TABLE_PROTOCOL *
+FindAcpiTableProtocol (
+ VOID
+ )
+{
+ EFI_STATUS Status;
+ EFI_ACPI_TABLE_PROTOCOL *AcpiTable;
+
+ Status = gBS->LocateProtocol (
+ &gEfiAcpiTableProtocolGuid,
+ NULL,
+ (VOID**)&AcpiTable
+ );
+ ASSERT_EFI_ERROR (Status);
+ return AcpiTable;
+}
+
+/** Install Acpi tables for Cloud Hypervisor
+
+ @param [in] AcpiProtocol Acpi Protocol which is used to install Acpi talbles
+
+ @return EFI_SUCCESS The table was successfully inserted.
+ @return EFI_INVALID_PARAMETER Either AcpiTableBuffer is NULL, TableKey is NULL, or AcpiTableBufferSize
+ and the size field embedded in the ACPI table pointed to by AcpiTableBuffer
+ are not in sync.
+ @return EFI_OUT_OF_RESOURCES Insufficient resources exist to complete the request.
+ @retval EFI_ACCESS_DENIED The table signature matches a table already
+ present in the system and platform policy
+ does not allow duplicate tables of this type.
+**/
+EFI_STATUS
+EFIAPI
+InstallCloudHvAcpiTables (
+ IN EFI_ACPI_TABLE_PROTOCOL *AcpiProtocol
+ )
+{
+ UINTN InstalledKey, TableSize, AcpiTableLength;
[SAMI] Define each local variable separately on a new line.
+ UINT64 RsdpPtr, XsdtPtr, TableOffset, AcpiTablePtr, DsdtPtr = ~0;
[SAMI] I think DsdtPtr can be UINT64 pointer and initialised to NULL before first use.
+ EFI_STATUS Status = EFI_SUCCESS;
+ BOOLEAN GotFacp = FALSE;
[SAMI] I think GotFacp could probably be avoided, see comments below.
+
+ RsdpPtr = PcdGet64 (PcdCloudHvAcpiRsdpBaseAddress);
+ XsdtPtr = ((EFI_ACPI_6_3_ROOT_SYSTEM_DESCRIPTION_POINTER *) RsdpPtr)->XsdtAddress;
[SAMI] No space between typecast and their object. Same comment for similar instances in this patch series.
+ AcpiTableLength = ((EFI_ACPI_COMMON_HEADER *) XsdtPtr)->Length;
+ TableOffset = sizeof (EFI_ACPI_DESCRIPTION_HEADER);
+
[SAMI] Set DsdtPtr = NULL, here.
+ while (!EFI_ERROR(Status)
+ && (TableOffset < AcpiTableLength))
+ {
[SAMI] Starting curly brace at the end of previous line, please.
+ AcpiTablePtr = *(UINT64 *) (XsdtPtr + TableOffset);
+ TableSize = ((EFI_ACPI_COMMON_HEADER *) AcpiTablePtr)->Length;
+
+ //
+ // Install ACPI tables from XSDT
+ //
+ Status = AcpiProtocol->InstallAcpiTable (
[SAMI] AcpiProtocol pointer not checked before use. In release builds the ASSERTs in FindAcpiTableProtocol() would vanish and a failure to get the protocol would result in a crash when dereferencing the pointer here.
+ AcpiProtocol,
+ (VOID *)(UINT64)AcpiTablePtr,
[SAMI] Can you check if typecast to UINT64 is needed here, please? Similarly, also check at other places.
+ TableSize,
+ &InstalledKey
+ );
[SAMI] Please reconsider error handling in this function. Probably best check and return failure from here. This would mean the status would not need to be checked in the while () statement and correspondingly there is no need to initialise Status to EFI_SUCCESS at the begining of this function.
+
+ TableOffset += sizeof (UINT64);
+
+ //
+ // Get DSDT from FADT
+ //
+ if (!GotFacp
+ && !AsciiStrnCmp ((CHAR8 *) &((EFI_ACPI_COMMON_HEADER *) AcpiTablePtr)->Signature, "FACP", 4))
+ {
[SAMI] Curly brace on previous line, please. '!GotFacp' could be replaced with (DsdtPtr != NULL).
+ DsdtPtr = ((EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE *) AcpiTablePtr)->XDsdt;
+ GotFacp = TRUE;
+ }
+ }
+
+ if (DsdtPtr == ~0) {
[SAMI] Please change to' If (DsdtPtr == NULL)'.
+ DEBUG ((DEBUG_ERROR, "%a: no DSDT found\n", __FUNCTION__));
+ ASSERT (FALSE);
+ CpuDeadLoop ();
[SAMI] I think EFI_NOT_FOUND could be returned here, and the CpuDeadLoop () could be moved to CloudHvAcpiPlatformEntryPoint().
+ }
+
+ //
+ // Install DSDT table
+ //
+ TableSize = ((EFI_ACPI_COMMON_HEADER *) DsdtPtr)->Length;
+ Status = AcpiProtocol->InstallAcpiTable (
+ AcpiProtocol,
+ (VOID *)(UINT64) DsdtPtr,
+ TableSize,
+ &InstalledKey
+ );
+
+ return Status;
+}
+
+/** Entry point for Cloud Hypervisor Platform Dxe
+
+ @param [in] ImageHandle Handle for this image.
+ @param [in] SystemTable Pointer to the EFI system table.
+
+ @return EFI_SUCCESS The table was successfully inserted.
+ @return EFI_INVALID_PARAMETER Either AcpiTableBuffer is NULL, TableKey is NULL, or AcpiTableBufferSize
+ and the size field embedded in the ACPI table pointed to by AcpiTableBuffer
+ are not in sync.
+ @return EFI_OUT_OF_RESOURCES Insufficient resources exist to complete the request.
+ @retval EFI_ACCESS_DENIED The table signature matches a table already
+ present in the system and platform policy
+ does not allow duplicate tables of this type.
+**/
+EFI_STATUS
+EFIAPI
+CloudHvAcpiPlatformEntryPoint (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ EFI_STATUS Status;
+
+ Status = InstallCloudHvAcpiTables (FindAcpiTableProtocol ());
[SAMI] Check status code here and on failure execute CpuDeadLoop ().
+ return Status;
+}


Re: [PATCH] UefiPayloadPkg/PayloadLoader: Add more checks to verify ELF images

Marvin Häuser
 

Hey Ray,

Sorry for not having properly checked yet, I definitely plan to still. However, I probably won't till a pointer alignment macro lands (I plan to submit a bunch of things, including this, within the next two weeks). Once it has been merged, I think this patch can be improved with alignment checks.

Thanks for your time!

Best regards,
Marvin

On 12.06.21 08:03, Ni, Ray wrote:
More checks are added to verify ELF image.
ParseElfImage() is changed to InitializeElfContext()

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Marvin Häuser <mhaeuser@posteo.de>
Cc: Guo Dong <guo.dong@intel.com>
Cc: Benjamin You <benjamin.you@intel.com>
---
UefiPayloadPkg/PayloadLoaderPeim/ElfLib.h | 11 +-
.../PayloadLoaderPeim/ElfLib/Elf32Lib.c | 38 ++--
.../PayloadLoaderPeim/ElfLib/Elf64Lib.c | 39 ++--
.../PayloadLoaderPeim/ElfLib/ElfLib.c | 210 +++++++++++++-----
.../PayloadLoaderPeim/PayloadLoaderPeim.c | 6 +-
5 files changed, 188 insertions(+), 116 deletions(-)

diff --git a/UefiPayloadPkg/PayloadLoaderPeim/ElfLib.h b/UefiPayloadPkg/PayloadLoaderPeim/ElfLib.h
index 9cfc2912cf..0ed93140a9 100644
--- a/UefiPayloadPkg/PayloadLoaderPeim/ElfLib.h
+++ b/UefiPayloadPkg/PayloadLoaderPeim/ElfLib.h
@@ -17,7 +17,6 @@
#define ELF_PT_LOAD 1


typedef struct {

- RETURN_STATUS ParseStatus; ///< Return the status after ParseElfImage().

UINT8 *FileBase; ///< The source location in memory.

UINTN FileSize; ///< The size including sections that don't require loading.

UINT8 *PreferredImageAddress; ///< The preferred image to be loaded. No relocation is needed if loaded to this address.

@@ -45,7 +44,10 @@ typedef struct {
/**

Parse the ELF image info.


- @param[in] ImageBase Memory address of an image.

+ On return, all fields in ElfCt are updated except ImageAddress.

+

+ @param[in] FileBase Memory address of an image.

+ @param[in] MaxFileSize The maximum file size.

@param[out] ElfCt The EFL image context pointer.


@retval EFI_INVALID_PARAMETER Input parameters are not valid.

@@ -55,8 +57,9 @@ typedef struct {
**/

EFI_STATUS

EFIAPI

-ParseElfImage (

- IN VOID *ImageBase,

+InitializeElfContext (

+ IN VOID *FileBase,

+ IN UINTN MaxFileSize,

OUT ELF_IMAGE_CONTEXT *ElfCt

);


diff --git a/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf32Lib.c b/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf32Lib.c
index 3fa100ce4a..79f4ce623b 100644
--- a/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf32Lib.c
+++ b/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf32Lib.c
@@ -115,7 +115,7 @@ ProcessRelocation32 (
UINT32 Type;


for ( Index = 0

- ; RelaEntrySize * Index < RelaSize

+ ; Index < RelaSize / RelaEntrySize

; Index++, Rela = ELF_NEXT_ENTRY (Elf32_Rela, Rela, RelaEntrySize)

) {

//

@@ -137,7 +137,6 @@ ProcessRelocation32 (
// Dynamic section doesn't contain entries of this type.

//

DEBUG ((DEBUG_INFO, "Unsupported relocation type %02X\n", Type));

- ASSERT (FALSE);

} else {

*Ptr += (UINT32) Delta;

}

@@ -164,7 +163,6 @@ ProcessRelocation32 (
// Calculation: B + A

//

if (RelaType == SHT_RELA) {

- ASSERT (*Ptr == 0);

*Ptr = (UINT32) Delta + Rela->r_addend;

} else {

//

@@ -177,7 +175,6 @@ ProcessRelocation32 (
// non-Dynamic section doesn't contain entries of this type.

//

DEBUG ((DEBUG_INFO, "Unsupported relocation type %02X\n", Type));

- ASSERT (FALSE);

}

break;


@@ -236,12 +233,12 @@ RelocateElf32Dynamic (
//

// It's abnormal a DYN ELF doesn't contain a dynamic section.

//

- ASSERT (DynShdr != NULL);

if (DynShdr == NULL) {

return EFI_UNSUPPORTED;

}

- ASSERT (DynShdr->sh_type == SHT_DYNAMIC);

- ASSERT (DynShdr->sh_entsize >= sizeof (*Dyn));

+ if ((DynShdr->sh_type != SHT_DYNAMIC) || DynShdr->sh_entsize < sizeof (*Dyn)) {

+ return EFI_UNSUPPORTED;

+ }


//

// 2. Locate the relocation section from the dynamic section.

@@ -286,9 +283,6 @@ RelocateElf32Dynamic (
}


if (RelaOffset == MAX_UINT64) {

- ASSERT (RelaCount == 0);

- ASSERT (RelaEntrySize == 0);

- ASSERT (RelaSize == 0);

//

// It's fine that a DYN ELF doesn't contain relocation section.

//

@@ -299,23 +293,22 @@ RelocateElf32Dynamic (
// Verify the existence of the relocation section.

//

RelShdr = GetElf32SectionByRange (ElfCt->FileBase, RelaOffset, RelaSize);

- ASSERT (RelShdr != NULL);

if (RelShdr == NULL) {

return EFI_UNSUPPORTED;

}

- ASSERT (RelShdr->sh_type == RelaType);

- ASSERT (RelShdr->sh_entsize == RelaEntrySize);

+ if ((RelShdr->sh_type != RelaType) || (RelShdr->sh_entsize != RelaEntrySize)) {

+ return EFI_UNSUPPORTED;

+ }


//

// 3. Process the relocation section.

//

- ProcessRelocation32 (

- (Elf32_Rela *) (ElfCt->FileBase + RelShdr->sh_offset),

- RelShdr->sh_size, RelShdr->sh_entsize, RelShdr->sh_type,

- (UINTN) ElfCt->ImageAddress - (UINTN) ElfCt->PreferredImageAddress,

- TRUE

- );

- return EFI_SUCCESS;

+ return ProcessRelocation32 (

+ (Elf32_Rela *) (ElfCt->FileBase + RelShdr->sh_offset),

+ RelShdr->sh_size, RelShdr->sh_entsize, RelShdr->sh_type,

+ (UINTN) ElfCt->ImageAddress - (UINTN) ElfCt->PreferredImageAddress,

+ TRUE

+ );

}


/**

@@ -331,7 +324,6 @@ RelocateElf32Sections (
IN ELF_IMAGE_CONTEXT *ElfCt

)

{

- EFI_STATUS Status;

Elf32_Ehdr *Ehdr;

Elf32_Shdr *RelShdr;

Elf32_Shdr *Shdr;

@@ -351,9 +343,7 @@ RelocateElf32Sections (
//

if (Ehdr->e_type == ET_DYN) {

DEBUG ((DEBUG_INFO, "DYN ELF: Relocate using dynamic sections...\n"));

- Status = RelocateElf32Dynamic (ElfCt);

- ASSERT_EFI_ERROR (Status);

- return Status;

+ return RelocateElf32Dynamic (ElfCt);

}


//

diff --git a/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf64Lib.c b/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf64Lib.c
index e364807007..cfe70639ca 100644
--- a/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf64Lib.c
+++ b/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf64Lib.c
@@ -115,7 +115,7 @@ ProcessRelocation64 (
UINT32 Type;


for ( Index = 0

- ; MultU64x64 (RelaEntrySize, Index) < RelaSize

+ ; Index < DivU64x64Remainder (RelaSize, RelaEntrySize, NULL)

; Index++, Rela = ELF_NEXT_ENTRY (Elf64_Rela, Rela, RelaEntrySize)

) {

//

@@ -138,7 +138,6 @@ ProcessRelocation64 (
// Dynamic section doesn't contain entries of this type.

//

DEBUG ((DEBUG_INFO, "Unsupported relocation type %02X\n", Type));

- ASSERT (FALSE);

} else {

*Ptr += Delta;

}

@@ -149,7 +148,6 @@ ProcessRelocation64 (
// Dynamic section doesn't contain entries of this type.

//

DEBUG ((DEBUG_INFO, "Unsupported relocation type %02X\n", Type));

- ASSERT (FALSE);

break;


case R_X86_64_RELATIVE:

@@ -173,7 +171,6 @@ ProcessRelocation64 (
// Calculation: B + A

//

if (RelaType == SHT_RELA) {

- ASSERT (*Ptr == 0);

*Ptr = Delta + Rela->r_addend;

} else {

//

@@ -186,7 +183,6 @@ ProcessRelocation64 (
// non-Dynamic section doesn't contain entries of this type.

//

DEBUG ((DEBUG_INFO, "Unsupported relocation type %02X\n", Type));

- ASSERT (FALSE);

}

break;


@@ -245,12 +241,12 @@ RelocateElf64Dynamic (
//

// It's abnormal a DYN ELF doesn't contain a dynamic section.

//

- ASSERT (DynShdr != NULL);

if (DynShdr == NULL) {

return EFI_UNSUPPORTED;

}

- ASSERT (DynShdr->sh_type == SHT_DYNAMIC);

- ASSERT (DynShdr->sh_entsize >= sizeof (*Dyn));

+ if ((DynShdr->sh_type != SHT_DYNAMIC) || DynShdr->sh_entsize < sizeof (*Dyn)) {

+ return EFI_UNSUPPORTED;

+ }


//

// 2. Locate the relocation section from the dynamic section.

@@ -295,9 +291,6 @@ RelocateElf64Dynamic (
}


if (RelaOffset == MAX_UINT64) {

- ASSERT (RelaCount == 0);

- ASSERT (RelaEntrySize == 0);

- ASSERT (RelaSize == 0);

//

// It's fine that a DYN ELF doesn't contain relocation section.

//

@@ -308,23 +301,22 @@ RelocateElf64Dynamic (
// Verify the existence of the relocation section.

//

RelShdr = GetElf64SectionByRange (ElfCt->FileBase, RelaOffset, RelaSize);

- ASSERT (RelShdr != NULL);

if (RelShdr == NULL) {

return EFI_UNSUPPORTED;

}

- ASSERT (RelShdr->sh_type == RelaType);

- ASSERT (RelShdr->sh_entsize == RelaEntrySize);

+ if ((RelShdr->sh_type != RelaType) || (RelShdr->sh_entsize != RelaEntrySize)) {

+ return EFI_UNSUPPORTED;

+ }


//

// 3. Process the relocation section.

//

- ProcessRelocation64 (

- (Elf64_Rela *) (ElfCt->FileBase + RelShdr->sh_offset),

- RelShdr->sh_size, RelShdr->sh_entsize, RelShdr->sh_type,

- (UINTN) ElfCt->ImageAddress - (UINTN) ElfCt->PreferredImageAddress,

- TRUE

- );

- return EFI_SUCCESS;

+ return ProcessRelocation64 (

+ (Elf64_Rela *) (ElfCt->FileBase + RelShdr->sh_offset),

+ RelShdr->sh_size, RelShdr->sh_entsize, RelShdr->sh_type,

+ (UINTN) ElfCt->ImageAddress - (UINTN) ElfCt->PreferredImageAddress,

+ TRUE

+ );

}


/**

@@ -340,7 +332,6 @@ RelocateElf64Sections (
IN ELF_IMAGE_CONTEXT *ElfCt

)

{

- EFI_STATUS Status;

Elf64_Ehdr *Ehdr;

Elf64_Shdr *RelShdr;

Elf64_Shdr *Shdr;

@@ -360,9 +351,7 @@ RelocateElf64Sections (
//

if (Ehdr->e_type == ET_DYN) {

DEBUG ((DEBUG_INFO, "DYN ELF: Relocate using dynamic sections...\n"));

- Status = RelocateElf64Dynamic (ElfCt);

- ASSERT_EFI_ERROR (Status);

- return Status;

+ return RelocateElf64Dynamic (ElfCt);

}


//

diff --git a/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/ElfLib.c b/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/ElfLib.c
index 531b3486d2..70de81c3ac 100644
--- a/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/ElfLib.c
+++ b/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/ElfLib.c
@@ -11,22 +11,32 @@
/**

Check if the ELF image is valid.


- @param[in] ImageBase Memory address of an image.

+ @param[in] FileBase Memory address of an image.

+ @param[in] MaxFileSize The maximum file size.


@retval TRUE if valid.


**/

BOOLEAN

IsElfFormat (

- IN CONST UINT8 *ImageBase

+ IN CONST UINT8 *FileBase,

+ IN UINTN MaxFileSize

)

{

Elf32_Ehdr *Elf32Hdr;

Elf64_Ehdr *Elf64Hdr;


- ASSERT (ImageBase != NULL);

+ ASSERT (FileBase != NULL);


- Elf32Hdr = (Elf32_Ehdr *)ImageBase;

+ Elf32Hdr = (Elf32_Ehdr *)FileBase;

+ Elf64Hdr = (Elf64_Ehdr *)FileBase;

+

+ //

+ // Make sure MaxFileSize covers e_ident[].

+ //

+ if (MaxFileSize < sizeof (Elf32Hdr->e_ident)) {

+ return FALSE;

+ }


//

// Start with correct signature "\7fELF"

@@ -50,15 +60,13 @@ IsElfFormat (
// Check 32/64-bit architecture

//

if (Elf32Hdr->e_ident[EI_CLASS] == ELFCLASS64) {

- Elf64Hdr = (Elf64_Ehdr *)Elf32Hdr;

- Elf32Hdr = NULL;

- } else if (Elf32Hdr->e_ident[EI_CLASS] == ELFCLASS32) {

- Elf64Hdr = NULL;

- } else {

- return FALSE;

- }

+ //

+ // Before accessing fields in Elf64_Ehdr, make sure the MaxFileSize covers the entire header.

+ //

+ if (MaxFileSize < sizeof (Elf64_Ehdr)) {

+ return FALSE;

+ }


- if (Elf64Hdr != NULL) {

//

// Support intel architecture only for now

//

@@ -79,7 +87,7 @@ IsElfFormat (
if (Elf64Hdr->e_version != EV_CURRENT) {

return FALSE;

}

- } else {

+ } else if (Elf32Hdr->e_ident[EI_CLASS] == ELFCLASS32) {

//

// Support intel architecture only for now

//

@@ -100,7 +108,10 @@ IsElfFormat (
if (Elf32Hdr->e_version != EV_CURRENT) {

return FALSE;

}

+ } else {

+ return FALSE;

}

+

return TRUE;

}


@@ -108,6 +119,7 @@ IsElfFormat (
Calculate a ELF file size.


@param[in] ElfCt ELF image context pointer.

+ @param[in] MaxFileSize The maximum file size.

@param[out] FileSize Return the file size.


@retval EFI_INVALID_PARAMETER ElfCt or SecPos is NULL.

@@ -117,12 +129,12 @@ IsElfFormat (
EFI_STATUS

CalculateElfFileSize (

IN ELF_IMAGE_CONTEXT *ElfCt,

+ IN UINTN MaxFileSize,

OUT UINTN *FileSize

)

{

EFI_STATUS Status;

- UINTN FileSize1;

- UINTN FileSize2;

+ UINT32 Index;

Elf32_Ehdr *Elf32Hdr;

Elf64_Ehdr *Elf64Hdr;

UINTN Offset;

@@ -132,24 +144,34 @@ CalculateElfFileSize (
return EFI_INVALID_PARAMETER;

}


- // Use last section as end of file

- Status = GetElfSectionPos (ElfCt, ElfCt->ShNum - 1, &Offset, &Size);

- if (EFI_ERROR(Status)) {

- return EFI_UNSUPPORTED;

- }

- FileSize1 = Offset + Size;

-

- // Use end of section header as end of file

- FileSize2 = 0;

+ //

+ // Optional section headers might exist in the end of file.

+ //

+ *FileSize = 0;

if (ElfCt->EiClass == ELFCLASS32) {

Elf32Hdr = (Elf32_Ehdr *)ElfCt->FileBase;

- FileSize2 = Elf32Hdr->e_shoff + Elf32Hdr->e_shentsize * Elf32Hdr->e_shnum;

+ *FileSize = Elf32Hdr->e_shoff + Elf32Hdr->e_shentsize * Elf32Hdr->e_shnum;

} else if (ElfCt->EiClass == ELFCLASS64) {

Elf64Hdr = (Elf64_Ehdr *)ElfCt->FileBase;

- FileSize2 = (UINTN)(Elf64Hdr->e_shoff + Elf64Hdr->e_shentsize * Elf64Hdr->e_shnum);

+ *FileSize = (UINTN)(Elf64Hdr->e_shoff + Elf64Hdr->e_shentsize * Elf64Hdr->e_shnum);

}


- *FileSize = MAX(FileSize1, FileSize2);

+ //

+ // Get the end of section body.

+ //

+ for (Index = 0; Index < ElfCt->ShNum; Index++) {

+ Status = GetElfSectionPos (ElfCt, Index, &Offset, &Size);

+ if (EFI_ERROR (Status)) {

+ return Status;

+ }

+ if ((Offset >= MaxFileSize) || (Size > MaxFileSize - Offset)) {

+ //

+ // Section body is outside of file range.

+ //

+ return EFI_UNSUPPORTED;

+ }

+ *FileSize = MAX (*FileSize, Offset + Size);

+ }


return EFI_SUCCESS;

}

@@ -213,7 +235,8 @@ GetElfSegmentInfo (

On return, all fields in ElfCt are updated except ImageAddress.


- @param[in] ImageBase Memory address of an image.

+ @param[in] FileBase Memory address of an image.

+ @param[in] MaxFileSize The maximum file size.

@param[out] ElfCt The EFL image context pointer.


@retval EFI_INVALID_PARAMETER Input parameters are not valid.

@@ -223,8 +246,9 @@ GetElfSegmentInfo (
**/

EFI_STATUS

EFIAPI

-ParseElfImage (

- IN VOID *ImageBase,

+InitializeElfContext (

+ IN VOID *FileBase,

+ IN UINTN MaxFileSize,

OUT ELF_IMAGE_CONTEXT *ElfCt

)

{

@@ -238,30 +262,58 @@ ParseElfImage (
UINTN End;

UINTN Base;


- if (ElfCt == NULL) {

- return EFI_INVALID_PARAMETER;

- }

+ ASSERT (ElfCt != NULL);

+

ZeroMem (ElfCt, sizeof(ELF_IMAGE_CONTEXT));


- if (ImageBase == NULL) {

- return (ElfCt->ParseStatus = EFI_INVALID_PARAMETER);

+ if (FileBase == NULL) {

+ return EFI_INVALID_PARAMETER;

}


- ElfCt->FileBase = (UINT8 *)ImageBase;

- if (!IsElfFormat (ElfCt->FileBase)) {

- return (ElfCt->ParseStatus = EFI_UNSUPPORTED);

+ ElfCt->FileBase = (UINT8 *)FileBase;

+ if (!IsElfFormat (ElfCt->FileBase, MaxFileSize)) {

+ return EFI_UNSUPPORTED;

}


Elf32Hdr = (Elf32_Ehdr *)ElfCt->FileBase;

ElfCt->EiClass = Elf32Hdr->e_ident[EI_CLASS];

if (ElfCt->EiClass == ELFCLASS32) {

if ((Elf32Hdr->e_type != ET_EXEC) && (Elf32Hdr->e_type != ET_DYN)) {

- return (ElfCt->ParseStatus = EFI_UNSUPPORTED);

+ return EFI_UNSUPPORTED;

}

- Elf32Shdr = (Elf32_Shdr *)GetElf32SectionByIndex (ElfCt->FileBase, Elf32Hdr->e_shstrndx);

+

+ if ((Elf32Hdr->e_phoff >= MaxFileSize) || ((UINT32) (Elf32Hdr->e_phentsize * Elf32Hdr->e_phnum) > MaxFileSize - Elf32Hdr->e_phoff)) {

+ //

+ // Program headers are outside of the file range.

+ //

+ return EFI_UNSUPPORTED;

+ }

+

+ if ((Elf32Hdr->e_shoff >= MaxFileSize) || ((UINT32) (Elf32Hdr->e_shentsize * Elf32Hdr->e_shnum) > MaxFileSize - Elf32Hdr->e_shoff)) {

+ //

+ // Section headers are outside of the file range.

+ //

+ return EFI_UNSUPPORTED;

+ }

+

+ if (Elf32Hdr->e_entry >= MaxFileSize) {

+ //

+ // Entrypoint is outside of the file range.

+ //

+ return EFI_UNSUPPORTED;

+ }

+

+ Elf32Shdr = GetElf32SectionByIndex (ElfCt->FileBase, Elf32Hdr->e_shstrndx);

if (Elf32Shdr == NULL) {

- return (ElfCt->ParseStatus = EFI_UNSUPPORTED);

+ return EFI_UNSUPPORTED;

}

+ if ((Elf32Shdr->sh_offset >= MaxFileSize) || (Elf32Shdr->sh_size > MaxFileSize - Elf32Shdr->sh_offset)) {

+ //

+ // String section is outside of the file range.

+ //

+ return EFI_UNSUPPORTED;

+ }

+

ElfCt->EntryPoint = (UINTN)Elf32Hdr->e_entry;

ElfCt->ShNum = Elf32Hdr->e_shnum;

ElfCt->PhNum = Elf32Hdr->e_phnum;

@@ -270,12 +322,41 @@ ParseElfImage (
} else {

Elf64Hdr = (Elf64_Ehdr *)Elf32Hdr;

if ((Elf64Hdr->e_type != ET_EXEC) && (Elf64Hdr->e_type != ET_DYN)) {

- return (ElfCt->ParseStatus = EFI_UNSUPPORTED);

+ return EFI_UNSUPPORTED;

+ }

+

+ if ((Elf64Hdr->e_phoff >= MaxFileSize) || ((UINT32) Elf64Hdr->e_phentsize * Elf64Hdr->e_phnum > MaxFileSize - (UINTN) Elf64Hdr->e_phoff)) {

+ //

+ // Program headers are outside of the file range.

+ //

+ return EFI_UNSUPPORTED;

+ }

+

+ if ((Elf64Hdr->e_shoff >= MaxFileSize) || ((UINT32) Elf64Hdr->e_shentsize * Elf64Hdr->e_shnum > MaxFileSize - (UINTN) Elf64Hdr->e_shoff)) {

+ //

+ // Section headers are outside of the file range.

+ //

+ return EFI_UNSUPPORTED;

+ }

+

+ if (Elf64Hdr->e_entry >= MaxFileSize) {

+ //

+ // Entrypoint is outside of the file range.

+ //

+ return EFI_UNSUPPORTED;

}

- Elf64Shdr = (Elf64_Shdr *)GetElf64SectionByIndex (ElfCt->FileBase, Elf64Hdr->e_shstrndx);

+

+ Elf64Shdr = GetElf64SectionByIndex (ElfCt->FileBase, Elf64Hdr->e_shstrndx);

if (Elf64Shdr == NULL) {

- return (ElfCt->ParseStatus = EFI_UNSUPPORTED);

+ return EFI_UNSUPPORTED;

+ }

+ if ((Elf64Shdr->sh_offset >= MaxFileSize) || (Elf64Shdr->sh_size > MaxFileSize - Elf64Shdr->sh_offset)) {

+ //

+ // String section is outside of the file range.

+ //

+ return EFI_UNSUPPORTED;

}

+

ElfCt->EntryPoint = (UINTN)Elf64Hdr->e_entry;

ElfCt->ShNum = Elf64Hdr->e_shnum;

ElfCt->PhNum = Elf64Hdr->e_phnum;

@@ -297,6 +378,13 @@ ParseElfImage (
continue;

}


+ //

+ // Loadable process segments must have congruent values for p_vaddr and p_offset, modulo the page size.

+ //

+ if ((SegInfo.MemAddr % EFI_PAGE_SIZE) != (SegInfo.Offset % EFI_PAGE_SIZE)) {

+ return EFI_UNSUPPORTED;

+ }

+

if (SegInfo.MemLen != SegInfo.Length) {

//

// Not enough space to execute at current location.

@@ -317,8 +405,7 @@ ParseElfImage (
ElfCt->ImageSize = End - Base + 1;

ElfCt->PreferredImageAddress = (VOID *) Base;


- CalculateElfFileSize (ElfCt, &ElfCt->FileSize);

- return (ElfCt->ParseStatus = EFI_SUCCESS);;

+ return CalculateElfFileSize (ElfCt, MaxFileSize, &ElfCt->FileSize);

}


/**

@@ -348,10 +435,6 @@ LoadElfImage (
return EFI_INVALID_PARAMETER;

}


- if (EFI_ERROR (ElfCt->ParseStatus)) {

- return ElfCt->ParseStatus;

- }

-

if (ElfCt->ImageAddress == NULL) {

return EFI_INVALID_PARAMETER;

}

@@ -370,6 +453,8 @@ LoadElfImage (
/**

Get a ELF section name from its index.


+ ElfCt is returned from InitializeElfContext().

+

@param[in] ElfCt ELF image context pointer.

@param[in] SectionIndex ELF section index.

@param[out] SectionName The pointer to the section name.

@@ -389,25 +474,25 @@ GetElfSectionName (
Elf32_Shdr *Elf32Shdr;

Elf64_Shdr *Elf64Shdr;

CHAR8 *Name;

+ UINTN MaxSize;


if ((ElfCt == NULL) || (SectionName == NULL)) {

return EFI_INVALID_PARAMETER;

}


- if (EFI_ERROR (ElfCt->ParseStatus)) {

- return ElfCt->ParseStatus;

- }

-

- Name = NULL;

+ Name = NULL;

+ MaxSize = 0;

if (ElfCt->EiClass == ELFCLASS32) {

Elf32Shdr = GetElf32SectionByIndex (ElfCt->FileBase, SectionIndex);

if ((Elf32Shdr != NULL) && (Elf32Shdr->sh_name < ElfCt->ShStrLen)) {

- Name = (CHAR8 *)(ElfCt->FileBase + ElfCt->ShStrOff + Elf32Shdr->sh_name);

+ Name = (CHAR8 *)(ElfCt->FileBase + ElfCt->ShStrOff + Elf32Shdr->sh_name);

+ MaxSize = ElfCt->ShStrLen - Elf32Shdr->sh_name;

}

} else if (ElfCt->EiClass == ELFCLASS64) {

Elf64Shdr = GetElf64SectionByIndex (ElfCt->FileBase, SectionIndex);

if ((Elf64Shdr != NULL) && (Elf64Shdr->sh_name < ElfCt->ShStrLen)) {

- Name = (CHAR8 *)(ElfCt->FileBase + ElfCt->ShStrOff + Elf64Shdr->sh_name);

+ Name = (CHAR8 *)(ElfCt->FileBase + ElfCt->ShStrOff + Elf64Shdr->sh_name);

+ MaxSize = ElfCt->ShStrLen - Elf64Shdr->sh_name;

}

}


@@ -415,6 +500,13 @@ GetElfSectionName (
return EFI_NOT_FOUND;

}


+ if (AsciiStrnLenS (Name, MaxSize) == MaxSize) {

+ //

+ // No null terminator is found for the section name.

+ //

+ return EFI_NOT_FOUND;

+ }

+

*SectionName = Name;

return EFI_SUCCESS;

}

@@ -449,10 +541,6 @@ GetElfSectionPos (
return EFI_INVALID_PARAMETER;

}


- if (EFI_ERROR (ElfCt->ParseStatus)) {

- return ElfCt->ParseStatus;

- }

-

if (ElfCt->EiClass == ELFCLASS32) {

Elf32Shdr = GetElf32SectionByIndex (ElfCt->FileBase, Index);

if (Elf32Shdr != NULL) {

diff --git a/UefiPayloadPkg/PayloadLoaderPeim/PayloadLoaderPeim.c b/UefiPayloadPkg/PayloadLoaderPeim/PayloadLoaderPeim.c
index 44639f9fd2..efedaef1b3 100644
--- a/UefiPayloadPkg/PayloadLoaderPeim/PayloadLoaderPeim.c
+++ b/UefiPayloadPkg/PayloadLoaderPeim/PayloadLoaderPeim.c
@@ -69,8 +69,10 @@ PeiLoadFileLoadPayload (
return Status;

}


- ZeroMem (&Context, sizeof (Context));

- Status = ParseElfImage (Elf, &Context);

+ //

+ // Trust the ELF image loaded from FV.

+ //

+ Status = InitializeElfContext (Elf, MAX_UINTN - (UINTN) Elf, &Context);

} while (EFI_ERROR (Status));


DEBUG ((


[PATCH v3 5/5] OvmfPkg/X86QemuLoadImageLib: State fw_cfg dependency in file header

Dov Murik
 

Make it clear that X86QemuLoadImageLib relies on fw_cfg; prepare the
ground to add a warning about the incompatibility with boot verification
process.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3457
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf | 3 +++
OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c | 3 +++
2 files changed, 6 insertions(+)

diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf b/=
OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
index e1615badd2ba..c7ec041cb706 100644
--- a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
+++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
@@ -2,6 +2,9 @@
# X86 specific implementation of QemuLoadImageLib library class interface=
=0D
# with support for loading mixed mode images and non-EFI stub images=0D
#=0D
+# Note that this implementation reads the cmdline (and possibly kernel, s=
etup=0D
+# data, and initrd in the legacy boot mode) from fw_cfg directly.=0D
+#=0D
# Copyright (c) 2020, ARM Ltd. All rights reserved.<BR>=0D
#=0D
# SPDX-License-Identifier: BSD-2-Clause-Patent=0D
diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c b/Ov=
mfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
index 6b1e7e649014..9f30df29736d 100644
--- a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
+++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
@@ -2,6 +2,9 @@
X86 specific implementation of QemuLoadImageLib library class interface=
=0D
with support for loading mixed mode images and non-EFI stub images=0D
=0D
+ Note that this implementation reads the cmdline (and possibly kernel, se=
tup=0D
+ data, and initrd in the legacy boot mode) from fw_cfg directly.=0D
+=0D
Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>=0D
Copyright (c) 2020, ARM Ltd. All rights reserved.<BR>=0D
=0D
--=20
2.25.1


[PATCH v3 3/5] Revert "OvmfPkg/QemuKernelLoaderFsDxe: don't expose kernel command line"

Dov Murik
 

This reverts commit efc52d67e1573ce174d301b52fa1577d552c8441.

Manually fixed conflicts in:
OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c

Note that besides re-exposing the kernel command line as a file in the
synthetic filesystem, we also revert back to AllocatePool instead of
AllocatePages.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3457
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c b/OvmfPk=
g/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
index b09ff6a3590d..c7ddd86f5c75 100644
--- a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
+++ b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
@@ -33,6 +33,7 @@
typedef enum {=0D
KernelBlobTypeKernel,=0D
KernelBlobTypeInitrd,=0D
+ KernelBlobTypeCommandLine,=0D
KernelBlobTypeMax=0D
} KERNEL_BLOB_TYPE;=0D
=0D
@@ -59,6 +60,11 @@ STATIC KERNEL_BLOB mKernelBlob[KernelBlobTypeMax] =3D {
{=0D
{ QemuFwCfgItemInitrdSize, QemuFwCfgItemInitrdData, },=0D
}=0D
+ }, {=0D
+ L"cmdline",=0D
+ {=0D
+ { QemuFwCfgItemCommandLineSize, QemuFwCfgItemCommandLineData, },=0D
+ }=0D
}=0D
};=0D
=0D
@@ -948,7 +954,7 @@ FetchBlob (
//=0D
// Read blob.=0D
//=0D
- Blob->Data =3D AllocatePages (EFI_SIZE_TO_PAGES ((UINTN)Blob->Size));=0D
+ Blob->Data =3D AllocatePool (Blob->Size);=0D
if (Blob->Data =3D=3D NULL) {=0D
DEBUG ((DEBUG_ERROR, "%a: failed to allocate %Ld bytes for \"%s\"\n",=
=0D
__FUNCTION__, (INT64)Blob->Size, Blob->Name));=0D
@@ -1083,8 +1089,7 @@ FreeBlobs:
while (BlobType > 0) {=0D
CurrentBlob =3D &mKernelBlob[--BlobType];=0D
if (CurrentBlob->Data !=3D NULL) {=0D
- FreePages (CurrentBlob->Data,=0D
- EFI_SIZE_TO_PAGES ((UINTN)CurrentBlob->Size));=0D
+ FreePool (CurrentBlob->Data);=0D
CurrentBlob->Size =3D 0;=0D
CurrentBlob->Data =3D NULL;=0D
}=0D
--=20
2.25.1


[PATCH v3 4/5] OvmfPkg/GenericQemuLoadImageLib: Read cmdline from QemuKernelLoaderFs

Dov Murik
 

Remove the QemuFwCfgLib interface used to read the QEMU cmdline
(-append argument) and the initrd size. Instead, use the synthetic
filesystem QemuKernelLoaderFs which has three files: "kernel", "initrd",
and "cmdline".

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3457
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf | 3 =
+-
OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c | 151 =
++++++++++++++++++--
2 files changed, 139 insertions(+), 15 deletions(-)

diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLi=
b.inf b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
index b262cb926a4d..9c9e35b1c5b9 100644
--- a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
+++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
@@ -25,14 +25,15 @@ [Packages]
=0D
[LibraryClasses]=0D
DebugLib=0D
+ FileHandleLib=0D
MemoryAllocationLib=0D
PrintLib=0D
- QemuFwCfgLib=0D
UefiBootServicesTableLib=0D
=0D
[Protocols]=0D
gEfiDevicePathProtocolGuid=0D
gEfiLoadedImageProtocolGuid=0D
+ gEfiSimpleFileSystemProtocolGuid=0D
=0D
[Guids]=0D
gQemuKernelLoaderFsMediaGuid=0D
diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLi=
b.c b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
index 8a29976ae172..66e029397bd6 100644
--- a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
+++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
@@ -11,13 +11,14 @@
#include <Base.h>=0D
#include <Guid/QemuKernelLoaderFsMedia.h>=0D
#include <Library/DebugLib.h>=0D
+#include <Library/FileHandleLib.h>=0D
#include <Library/MemoryAllocationLib.h>=0D
#include <Library/PrintLib.h>=0D
-#include <Library/QemuFwCfgLib.h>=0D
#include <Library/QemuLoadImageLib.h>=0D
#include <Library/UefiBootServicesTableLib.h>=0D
#include <Protocol/DevicePath.h>=0D
#include <Protocol/LoadedImage.h>=0D
+#include <Protocol/SimpleFileSystem.h>=0D
=0D
#pragma pack (1)=0D
typedef struct {=0D
@@ -30,6 +31,11 @@ typedef struct {
KERNEL_FILE_DEVPATH FileNode;=0D
EFI_DEVICE_PATH_PROTOCOL EndNode;=0D
} KERNEL_VENMEDIA_FILE_DEVPATH;=0D
+=0D
+typedef struct {=0D
+ VENDOR_DEVICE_PATH VenMediaNode;=0D
+ EFI_DEVICE_PATH_PROTOCOL EndNode;=0D
+} SINGLE_VENMEDIA_NODE_DEVPATH;=0D
#pragma pack ()=0D
=0D
STATIC CONST KERNEL_VENMEDIA_FILE_DEVPATH mKernelDevicePath =3D {=0D
@@ -51,6 +57,82 @@ STATIC CONST KERNEL_VENMEDIA_FILE_DEVPATH mKernelDeviceP=
ath =3D {
}=0D
};=0D
=0D
+STATIC CONST SINGLE_VENMEDIA_NODE_DEVPATH mQemuKernelLoaderFsDevicePath =
=3D {=0D
+ {=0D
+ {=0D
+ MEDIA_DEVICE_PATH, MEDIA_VENDOR_DP,=0D
+ { sizeof (VENDOR_DEVICE_PATH) }=0D
+ },=0D
+ QEMU_KERNEL_LOADER_FS_MEDIA_GUID=0D
+ }, {=0D
+ END_DEVICE_PATH_TYPE, END_ENTIRE_DEVICE_PATH_SUBTYPE,=0D
+ { sizeof (EFI_DEVICE_PATH_PROTOCOL) }=0D
+ }=0D
+};=0D
+=0D
+STATIC=0D
+EFI_STATUS=0D
+GetQemuKernelLoaderBlobSize (=0D
+ IN EFI_FILE_HANDLE Root,=0D
+ IN CHAR16 *FileName,=0D
+ OUT UINTN *Size=0D
+ )=0D
+{=0D
+ EFI_STATUS Status;=0D
+ EFI_FILE_HANDLE FileHandle;=0D
+ UINT64 FileSize;=0D
+=0D
+ Status =3D Root->Open (Root, &FileHandle, FileName, EFI_FILE_MODE_READ, =
0);=0D
+ if (EFI_ERROR (Status)) {=0D
+ return Status;=0D
+ }=0D
+ Status =3D FileHandleGetSize (FileHandle, &FileSize);=0D
+ if (EFI_ERROR (Status)) {=0D
+ goto CloseFile;=0D
+ }=0D
+ if (FileSize > MAX_UINTN) {=0D
+ Status =3D EFI_UNSUPPORTED;=0D
+ goto CloseFile;=0D
+ }=0D
+ *Size =3D (UINTN)FileSize;=0D
+ Status =3D EFI_SUCCESS;=0D
+CloseFile:=0D
+ FileHandle->Close (FileHandle);=0D
+ return Status;=0D
+}=0D
+=0D
+STATIC=0D
+EFI_STATUS=0D
+ReadWholeQemuKernelLoaderBlob (=0D
+ IN EFI_FILE_HANDLE Root,=0D
+ IN CHAR16 *FileName,=0D
+ IN UINTN Size,=0D
+ OUT VOID *Buffer=0D
+ )=0D
+{=0D
+ EFI_STATUS Status;=0D
+ EFI_FILE_HANDLE FileHandle;=0D
+ UINTN ReadSize;=0D
+=0D
+ Status =3D Root->Open (Root, &FileHandle, FileName, EFI_FILE_MODE_READ, =
0);=0D
+ if (EFI_ERROR (Status)) {=0D
+ return Status;=0D
+ }=0D
+ ReadSize =3D Size;=0D
+ Status =3D FileHandle->Read (FileHandle, &ReadSize, Buffer);=0D
+ if (EFI_ERROR (Status)) {=0D
+ goto CloseFile;=0D
+ }=0D
+ if (ReadSize !=3D Size) {=0D
+ Status =3D EFI_PROTOCOL_ERROR;=0D
+ goto CloseFile;=0D
+ }=0D
+ Status =3D EFI_SUCCESS;=0D
+CloseFile:=0D
+ FileHandle->Close (FileHandle);=0D
+ return Status;=0D
+}=0D
+=0D
/**=0D
Download the kernel, the initial ramdisk, and the kernel command line fr=
om=0D
QEMU's fw_cfg. The kernel will be instructed via its command line to loa=
d=0D
@@ -76,12 +158,16 @@ QemuLoadKernelImage (
OUT EFI_HANDLE *ImageHandle=0D
)=0D
{=0D
- EFI_STATUS Status;=0D
- EFI_HANDLE KernelImageHandle;=0D
- EFI_LOADED_IMAGE_PROTOCOL *KernelLoadedImage;=0D
- UINTN CommandLineSize;=0D
- CHAR8 *CommandLine;=0D
- UINTN InitrdSize;=0D
+ EFI_STATUS Status;=0D
+ EFI_HANDLE KernelImageHandle;=0D
+ EFI_LOADED_IMAGE_PROTOCOL *KernelLoadedImage;=0D
+ EFI_DEVICE_PATH_PROTOCOL *DevicePathNode;=0D
+ EFI_HANDLE FsVolumeHandle;=0D
+ EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *FsProtocol;=0D
+ EFI_FILE_HANDLE Root;=0D
+ UINTN CommandLineSize;=0D
+ CHAR8 *CommandLine;=0D
+ UINTN InitrdSize;=0D
=0D
//=0D
// Load the image. This should call back into the QEMU EFI loader file s=
ystem.=0D
@@ -124,8 +210,38 @@ QemuLoadKernelImage (
);=0D
ASSERT_EFI_ERROR (Status);=0D
=0D
- QemuFwCfgSelectItem (QemuFwCfgItemCommandLineSize);=0D
- CommandLineSize =3D (UINTN)QemuFwCfgRead32 ();=0D
+ //=0D
+ // Open the Qemu Kernel Loader abstract filesystem (volume) which will b=
e=0D
+ // used to query the "initrd" and to read the "cmdline" synthetic files.=
=0D
+ //=0D
+ DevicePathNode =3D (EFI_DEVICE_PATH_PROTOCOL *)&mQemuKernelLoaderFsDevic=
ePath;=0D
+ Status =3D gBS->LocateDevicePath (=0D
+ &gEfiSimpleFileSystemProtocolGuid,=0D
+ &DevicePathNode,=0D
+ &FsVolumeHandle=0D
+ );=0D
+ if (EFI_ERROR (Status)) {=0D
+ goto UnloadImage;=0D
+ }=0D
+=0D
+ Status =3D gBS->HandleProtocol (=0D
+ FsVolumeHandle,=0D
+ &gEfiSimpleFileSystemProtocolGuid,=0D
+ (VOID **)&FsProtocol=0D
+ );=0D
+ if (EFI_ERROR (Status)) {=0D
+ goto UnloadImage;=0D
+ }=0D
+=0D
+ Status =3D FsProtocol->OpenVolume (FsVolumeHandle, &Root);=0D
+ if (EFI_ERROR (Status)) {=0D
+ goto UnloadImage;=0D
+ }=0D
+=0D
+ Status =3D GetQemuKernelLoaderBlobSize (Root, L"cmdline", &CommandLineSi=
ze);=0D
+ if (EFI_ERROR (Status)) {=0D
+ goto CloseRoot;=0D
+ }=0D
=0D
if (CommandLineSize =3D=3D 0) {=0D
KernelLoadedImage->LoadOptionsSize =3D 0;=0D
@@ -133,11 +249,14 @@ QemuLoadKernelImage (
CommandLine =3D AllocatePool (CommandLineSize);=0D
if (CommandLine =3D=3D NULL) {=0D
Status =3D EFI_OUT_OF_RESOURCES;=0D
- goto UnloadImage;=0D
+ goto CloseRoot;=0D
}=0D
=0D
- QemuFwCfgSelectItem (QemuFwCfgItemCommandLineData);=0D
- QemuFwCfgReadBytes (CommandLineSize, CommandLine);=0D
+ Status =3D ReadWholeQemuKernelLoaderBlob (Root, L"cmdline", CommandLin=
eSize,=0D
+ CommandLine);=0D
+ if (EFI_ERROR (Status)) {=0D
+ goto FreeCommandLine;=0D
+ }=0D
=0D
//=0D
// Verify NUL-termination of the command line.=0D
@@ -155,8 +274,10 @@ QemuLoadKernelImage (
KernelLoadedImage->LoadOptionsSize =3D (UINT32)((CommandLineSize - 1) =
* 2);=0D
}=0D
=0D
- QemuFwCfgSelectItem (QemuFwCfgItemInitrdSize);=0D
- InitrdSize =3D (UINTN)QemuFwCfgRead32 ();=0D
+ Status =3D GetQemuKernelLoaderBlobSize (Root, L"initrd", &InitrdSize);=0D
+ if (EFI_ERROR (Status)) {=0D
+ goto FreeCommandLine;=0D
+ }=0D
=0D
if (InitrdSize > 0) {=0D
//=0D
@@ -199,6 +320,8 @@ FreeCommandLine:
if (CommandLineSize > 0) {=0D
FreePool (CommandLine);=0D
}=0D
+CloseRoot:=0D
+ Root->Close (Root);=0D
UnloadImage:=0D
if (EFI_ERROR (Status)) {=0D
gBS->UnloadImage (KernelImageHandle);=0D
--=20
2.25.1


[PATCH v3 0/5] OvmfPkg: Use QemuKernelLoaderFs to read cmdline/initrd

Dov Murik
 

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

In order to support measured SEV boot with kernel/initrd/cmdline, we'd
like to have one place that reads those blobs; in the future we'll add
the measurement and verification in that place.

We already have a synthetic filesystem (QemuKernelLoaderFs) which holds
three files: "kernel", "initrd", and "cmdline". The kernel is indeed
read from this filesystem in LoadImage; but the cmdline (and the length
of initrd) are read from QemuFwCfgLib items.

This patch series first fixes two identical memory leak bugs in
GenericQemuLoadImageLib and X86QemuLoadImageLib; then modifies
GenericQemuLoadImageLib to read cmdline (and the initrd size) from the
QemuKernelLoaderFs synthetic filesystem, thus removing the dependency on
QemuFwCfgLib.

Note that X86QemuLoadImageLib is not modified, because it contains a
QemuLoadLegacyImage() which reads other items of the QemuFwCfg which are
not available in QemuKernelLoaderFs. Since we don't want to support the
legacy boot path in the future measured SEV boot, we leave
X86QemuLoadImageLib as-is (except for a comment addition in patch 3) and
will force use for GenericQemuLoadImageLib in the measured SEV boot
implementation.

Relevant discussion threads start in:
https://edk2.groups.io/g/devel/message/76069

To test this on x86_64, I forced the use of GenericQemuLoadImageLib
using the following local patch:


diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 0a237a905866..46442b543bcf 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -404,7 +404,7 @@ [LibraryClasses.common.DXE_DRIVER]
PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
- QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
+ QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf # XXX don't commit this or someone will be mad
!if $(TPM_ENABLE) == TRUE
Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibTcg.inf
Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf


I tested boot with QEMU and OVMF with the following QEMU arguments:

-kernel a
-kernel a -initrd b
-kernel a -cmdline c
-kernel a -initrd b -cmdline c

(and also without -kernel)


Code is at
https://github.com/confidential-containers-demo/edk2/tree/use-synthetic-fs-for-cmdline-v3

v3 changes:
- Insert patches 1+2 at the top of the series to fix cmdline leak bugs
- Organize #include and .inf
- Add UINTN overflow check
- Fix error paths and function epilogue to properly release all resources
- Clarity: rename long variables, reword comments

v2: https://edk2.groups.io/g/devel/message/76664
v2 changes:
- Add comment to header of X86QemuLoadImageLib.inf
- Clearer function names in GenericQemuLoadImageLib.c
- Fix coding style issues

v1: https://edk2.groups.io/g/devel/message/76265


Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>


Dov Murik (5):
OvmfPkg/GenericQemuLoadImageLib: plug cmdline blob leak on success
OvmfPkg/X86QemuLoadImageLib: plug cmdline blob leak on success
Revert "OvmfPkg/QemuKernelLoaderFsDxe: don't expose kernel command
line"
OvmfPkg/GenericQemuLoadImageLib: Read cmdline from QemuKernelLoaderFs
OvmfPkg/X86QemuLoadImageLib: State fw_cfg dependency in file header

OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf | 3 +-
OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf | 3 +
OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c | 157 ++++++++++++++++++--
OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c | 9 +-
OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c | 11 +-
5 files changed, 161 insertions(+), 22 deletions(-)

--
2.25.1


[PATCH v3 2/5] OvmfPkg/X86QemuLoadImageLib: plug cmdline blob leak on success

Dov Murik
 

When QemuLoadKernelImage() ends successfully, the command-line blob is
not freed, even though it is not used elsewhere (its content is already
copied to KernelLoadedImage->LoadOptions). The memory leak bug was
introduced in commit 7c47d89003a6 ("OvmfPkg: implement QEMU loader
library for X86 with legacy fallback", 2020-03-05).

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
Reported-by: Laszlo Ersek <lersek@redhat.com>
Fixes: 7c47d89003a6f8f7f6f0ce8ca7d3e87c630d14cc
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c b/Ov=
mfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
index 1177582ab051..6b1e7e649014 100644
--- a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
+++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
@@ -446,14 +446,16 @@ QemuLoadKernelImage (
}=0D
=0D
*ImageHandle =3D KernelImageHandle;=0D
- return EFI_SUCCESS;=0D
+ Status =3D EFI_SUCCESS;=0D
=0D
FreeCommandLine:=0D
if (CommandLineSize > 0) {=0D
FreePool (CommandLine);=0D
}=0D
UnloadImage:=0D
- gBS->UnloadImage (KernelImageHandle);=0D
+ if (EFI_ERROR (Status)) {=0D
+ gBS->UnloadImage (KernelImageHandle);=0D
+ }=0D
=0D
return Status;=0D
}=0D
--=20
2.25.1


[PATCH v3 1/5] OvmfPkg/GenericQemuLoadImageLib: plug cmdline blob leak on success

Dov Murik
 

When QemuLoadKernelImage() ends successfully, the command-line blob is
not freed, even though it is not used elsewhere (its content is already
copied to KernelLoadedImage->LoadOptions). The memory leak bug was
introduced in commit ddd2be6b0026 ("OvmfPkg: provide a generic
implementation of QemuLoadImageLib", 2020-03-05).

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
Reported-by: Laszlo Ersek <lersek@redhat.com>
Fixes: ddd2be6b0026abcd0f819b3915fc80c3de81dd62
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c | 6 ++++=
--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLi=
b.c b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
index 114db7e8441f..8a29976ae172 100644
--- a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
+++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
@@ -193,14 +193,16 @@ QemuLoadKernelImage (
}=0D
=0D
*ImageHandle =3D KernelImageHandle;=0D
- return EFI_SUCCESS;=0D
+ Status =3D EFI_SUCCESS;=0D
=0D
FreeCommandLine:=0D
if (CommandLineSize > 0) {=0D
FreePool (CommandLine);=0D
}=0D
UnloadImage:=0D
- gBS->UnloadImage (KernelImageHandle);=0D
+ if (EFI_ERROR (Status)) {=0D
+ gBS->UnloadImage (KernelImageHandle);=0D
+ }=0D
=0D
return Status;=0D
}=0D
--=20
2.25.1


[PATCH v3 2/3] Acpi: Install Acpi tables for Cloud hypervisor

Jianyong Wu
 

There is no device like Fw-cfg in Qemu in Cloud Hypervisor, so a specific
Acpi handler is introduced here.

The handler implemented here is in a very simple way:
1. acquire the RSDP from the PCD variable in the top ".dsc";
2. get the XSDT address from RSDP structure;
3. get the ACPI tables following the XSDT structure and install them
one by one;
4. get DSDT address from FADT and install DSDT table.

Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
---
ArmVirtPkg/ArmVirtPkg.dec | 6 +
.../CloudHvAcpiPlatformDxe.inf | 47 ++++++
.../CloudHvAcpiPlatformDxe/CloudHvAcpi.c | 141 ++++++++++++++++++
3 files changed, 194 insertions(+)
create mode 100644 ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf
create mode 100644 ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c

diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
index bf82f7f1f3f2..4e4d758015bc 100644
--- a/ArmVirtPkg/ArmVirtPkg.dec
+++ b/ArmVirtPkg/ArmVirtPkg.dec
@@ -66,6 +66,12 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
#
gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, 0x4D}|VOID*|0x00000007

+ ##
+ # This is the physical address of Rsdp which is the core struct of Acpi.
+ # Cloud Hypervisor has no other way to pass Rsdp address to the guest except use a PCD.
+ #
+ gArmVirtTokenSpaceGuid.PcdCloudHvAcpiRsdpBaseAddress|0x0|UINT64|0x00000005
+
[PcdsDynamic]
#
# Whether to force disable ACPI, regardless of the fw_cfg settings
diff --git a/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf
new file mode 100644
index 000000000000..01de76486686
--- /dev/null
+++ b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf
@@ -0,0 +1,47 @@
+## @file
+# ACPI Platform Driver for Cloud Hypervisor
+#
+# Copyright (c) 2021, ARM Limited. All rights reserved.<BR>
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION = 0x00010005
+ BASE_NAME = CloudHvgAcpiPlatform
+ FILE_GUID = 6c76e407-73f2-dc1c-938f-5d6c4691ea93
+ MODULE_TYPE = DXE_DRIVER
+ VERSION_STRING = 1.0
+ ENTRY_POINT = CloudHvAcpiPlatformEntryPoint
+
+#
+# The following information is for reference only and not required by the build tools.
+#
+ VALID_ARCHITECTURES = AARCH64
+#
+
+[Sources]
+ CloudHvAcpi.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ OvmfPkg/OvmfPkg.dec
+ ArmVirtPkg/ArmVirtPkg.dec
+
+[LibraryClasses]
+ BaseLib
+ DebugLib
+ MemoryAllocationLib
+ OrderedCollectionLib
+ UefiBootServicesTableLib
+ UefiDriverEntryPoint
+
+[Protocols]
+ gEfiAcpiTableProtocolGuid # PROTOCOL ALWAYS_CONSUMED
+
+[Pcd]
+ gArmVirtTokenSpaceGuid.PcdCloudHvAcpiRsdpBaseAddress
+
+[Depex]
+ gEfiAcpiTableProtocolGuid
diff --git a/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
new file mode 100644
index 000000000000..0f1a50d63cd6
--- /dev/null
+++ b/ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpi.c
@@ -0,0 +1,141 @@
+/** @file
+ Install Acpi tables for Cloud Hypervisor
+
+ Copyright (c) 2021, Arm Limited. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <Library/BaseLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <IndustryStandard/Acpi63.h>
+#include <Protocol/AcpiTable.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiDriverEntryPoint.h>
+#include <Library/DebugLib.h>
+
+/**
+ Find Acpi table Protocol and return it
+**/
+STATIC
+EFI_ACPI_TABLE_PROTOCOL *
+FindAcpiTableProtocol (
+ VOID
+ )
+{
+ EFI_STATUS Status;
+ EFI_ACPI_TABLE_PROTOCOL *AcpiTable;
+
+ Status = gBS->LocateProtocol (
+ &gEfiAcpiTableProtocolGuid,
+ NULL,
+ (VOID**)&AcpiTable
+ );
+ ASSERT_EFI_ERROR (Status);
+ return AcpiTable;
+}
+
+/** Install Acpi tables for Cloud Hypervisor
+
+ @param [in] AcpiProtocol Acpi Protocol which is used to install Acpi talbles
+
+ @return EFI_SUCCESS The table was successfully inserted.
+ @return EFI_INVALID_PARAMETER Either AcpiTableBuffer is NULL, TableKey is NULL, or AcpiTableBufferSize
+ and the size field embedded in the ACPI table pointed to by AcpiTableBuffer
+ are not in sync.
+ @return EFI_OUT_OF_RESOURCES Insufficient resources exist to complete the request.
+ @retval EFI_ACCESS_DENIED The table signature matches a table already
+ present in the system and platform policy
+ does not allow duplicate tables of this type.
+**/
+EFI_STATUS
+EFIAPI
+InstallCloudHvAcpiTables (
+ IN EFI_ACPI_TABLE_PROTOCOL *AcpiProtocol
+ )
+{
+ UINTN InstalledKey, TableSize, AcpiTableLength;
+ UINT64 RsdpPtr, XsdtPtr, TableOffset, AcpiTablePtr, DsdtPtr = ~0;
+ EFI_STATUS Status = EFI_SUCCESS;
+ BOOLEAN GotFacp = FALSE;
+
+ RsdpPtr = PcdGet64 (PcdCloudHvAcpiRsdpBaseAddress);
+ XsdtPtr = ((EFI_ACPI_6_3_ROOT_SYSTEM_DESCRIPTION_POINTER *) RsdpPtr)->XsdtAddress;
+ AcpiTableLength = ((EFI_ACPI_COMMON_HEADER *) XsdtPtr)->Length;
+ TableOffset = sizeof (EFI_ACPI_DESCRIPTION_HEADER);
+
+ while (!EFI_ERROR(Status)
+ && (TableOffset < AcpiTableLength))
+ {
+ AcpiTablePtr = *(UINT64 *) (XsdtPtr + TableOffset);
+ TableSize = ((EFI_ACPI_COMMON_HEADER *) AcpiTablePtr)->Length;
+
+ //
+ // Install ACPI tables from XSDT
+ //
+ Status = AcpiProtocol->InstallAcpiTable (
+ AcpiProtocol,
+ (VOID *)(UINT64)AcpiTablePtr,
+ TableSize,
+ &InstalledKey
+ );
+
+ TableOffset += sizeof (UINT64);
+
+ //
+ // Get DSDT from FADT
+ //
+ if (!GotFacp
+ && !AsciiStrnCmp ((CHAR8 *) &((EFI_ACPI_COMMON_HEADER *) AcpiTablePtr)->Signature, "FACP", 4))
+ {
+ DsdtPtr = ((EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE *) AcpiTablePtr)->XDsdt;
+ GotFacp = TRUE;
+ }
+ }
+
+ if (DsdtPtr == ~0) {
+ DEBUG ((DEBUG_ERROR, "%a: no DSDT found\n", __FUNCTION__));
+ ASSERT (FALSE);
+ CpuDeadLoop ();
+ }
+
+ //
+ // Install DSDT table
+ //
+ TableSize = ((EFI_ACPI_COMMON_HEADER *) DsdtPtr)->Length;
+ Status = AcpiProtocol->InstallAcpiTable (
+ AcpiProtocol,
+ (VOID *)(UINT64) DsdtPtr,
+ TableSize,
+ &InstalledKey
+ );
+
+ return Status;
+}
+
+/** Entry point for Cloud Hypervisor Platform Dxe
+
+ @param [in] ImageHandle Handle for this image.
+ @param [in] SystemTable Pointer to the EFI system table.
+
+ @return EFI_SUCCESS The table was successfully inserted.
+ @return EFI_INVALID_PARAMETER Either AcpiTableBuffer is NULL, TableKey is NULL, or AcpiTableBufferSize
+ and the size field embedded in the ACPI table pointed to by AcpiTableBuffer
+ are not in sync.
+ @return EFI_OUT_OF_RESOURCES Insufficient resources exist to complete the request.
+ @retval EFI_ACCESS_DENIED The table signature matches a table already
+ present in the system and platform policy
+ does not allow duplicate tables of this type.
+**/
+EFI_STATUS
+EFIAPI
+CloudHvAcpiPlatformEntryPoint (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ EFI_STATUS Status;
+
+ Status = InstallCloudHvAcpiTables (FindAcpiTableProtocol ());
+ return Status;
+}
--
2.17.1

5201 - 5220 of 82316