Topics

[PATCH 3/4] OvmfPkg: create a SEV secret area in the AmdSev memfd


James Bottomley <jejb@...>
 

SEV needs an area to place an injected secret where OVMF can find it
and pass it up as a ConfigurationTable. This patch implements the
area itself as an addition to the SEV enhanced reset vector. The
reset vector scheme allows additions but not removals. If the size of
the reset vector is 22, it only contains the AP reset IP, but if it is
30 (or greater) it contains the SEV secret page location and size.

Signed-off-by: James Bottomley <jejb@...>
---
OvmfPkg/OvmfPkg.dec | 5 +++++
OvmfPkg/AmdSev/AmdSevX64.fdf | 3 +++
OvmfPkg/ResetVector/ResetVector.inf | 4 ++++
OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 4 ++++
OvmfPkg/ResetVector/ResetVector.nasmb | 2 ++
5 files changed, 18 insertions(+)

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 3fbf7a0ee1..b00f083417 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -117,6 +117,7 @@
gLinuxEfiInitrdMediaGuid =3D {0x5568e427, 0x68fc, 0x4f3d, {=
0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68}}=0D
gQemuKernelLoaderFsMediaGuid =3D {0x1428f772, 0xb64a, 0x441e, {=
0xb8, 0xc3, 0x9e, 0xbd, 0xd7, 0xf8, 0x93, 0xc7}}=0D
gGrubFileGuid =3D {0xb5ae312c, 0xbc8a, 0x43b1, {=
0x9c, 0x62, 0xeb, 0xb8, 0x26, 0xdd, 0x5d, 0x07}}=0D
+ gSevLaunchSecretGuid =3D {0xadf956ad, 0xe98c, 0x484c, {=
0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47}}=0D
=0D
[Ppis]=0D
# PPI whose presence in the PPI database signals that the TPM base addre=
ss=0D
@@ -304,6 +305,10 @@
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|0|UINT32|0x40=0D
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize|0|UINT32|0x41=0D
=0D
+ ## The base address and size of the SEV Launch Secret Area=0D
+ gSevLaunchSecretGuid.PcdSevLaunchSecretBase|0x0|UINT32|0=0D
+ gSevLaunchSecretGuid.PcdSevLaunchSecretSize|0x0|UINT32|1=0D
+=0D
[PcdsDynamic, PcdsDynamicEx]=0D
gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2=0D
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x1=
0=0D
diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf
index 689386612d..1fd38b3fe2 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.fdf
+++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
@@ -59,6 +59,9 @@ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPk=
gTokenSpaceGuid.PcdOvmf
0x00B000|0x001000=0D
gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.P=
cdSevEsWorkAreaSize=0D
=0D
+0x00C000|0x001000=0D
+gSevLaunchSecretGuid.PcdSevLaunchSecretBase|gSevLaunchSecretGuid.PcdSevLau=
nchSecretSize=0D
+=0D
0x010000|0x010000=0D
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpace=
Guid.PcdOvmfSecPeiTempRamSize=0D
=0D
diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/Rese=
tVector.inf
index a53ae6c194..72fd78eef4 100644
--- a/OvmfPkg/ResetVector/ResetVector.inf
+++ b/OvmfPkg/ResetVector/ResetVector.inf
@@ -43,3 +43,7 @@
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize=0D
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase=0D
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize=0D
+=0D
+[FixedPcd]=0D
+ gSevLaunchSecretGuid.PcdSevLaunchSecretBase=0D
+ gSevLaunchSecretGuid.PcdSevLaunchSecretSize=0D
diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/ResetVe=
ctor/Ia16/ResetVectorVtf0.asm
index 980e0138e7..7d3214e55d 100644
--- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
@@ -35,6 +35,8 @@ ALIGN 16
; the build time RIP value. The GUID must always be 48 bytes from the=0D
; end of the firmware.=0D
;=0D
+; 0xffffffc2 (-0x3e) - Base Location of the SEV Launch Secret=0D
+; 0xffffffc6 (-0x3a) - Size of SEV Launch Secret=0D
; 0xffffffca (-0x36) - IP value=0D
; 0xffffffcc (-0x34) - CS segment base [31:16]=0D
; 0xffffffce (-0x32) - Size of the SEV-ES reset block=0D
@@ -51,6 +53,8 @@ ALIGN 16
TIMES (32 - (sevEsResetBlockEnd - sevEsResetBlockStart)) DB 0=0D
=0D
sevEsResetBlockStart:=0D
+ DD SEV_LAUNCH_SECRET_BASE=0D
+ DD SEV_LAUNCH_SECRET_SIZE=0D
DD SEV_ES_AP_RESET_IP=0D
DW sevEsResetBlockEnd - sevEsResetBlockStart=0D
DB 0xDE, 0x71, 0xF7, 0x00, 0x7E, 0x1A, 0xCB, 0x4F=0D
diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/Re=
setVector.nasmb
index 4913b379a9..c5e0fe93ab 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -83,5 +83,7 @@
%include "Main.asm"=0D
=0D
%define SEV_ES_AP_RESET_IP FixedPcdGet32 (PcdSevEsWorkAreaBase)=0D
+ %define SEV_LAUNCH_SECRET_BASE FixedPcdGet32 (PcdSevLaunchSecretBase)=0D
+ %define SEV_LAUNCH_SECRET_SIZE FixedPcdGet32 (PcdSevLaunchSecretSize)=0D
%include "Ia16/ResetVectorVtf0.asm"=0D
=0D
--=20
2.26.2


Laszlo Ersek
 

On 11/12/20 01:13, James Bottomley wrote:
SEV needs an area to place an injected secret where OVMF can find it
and pass it up as a ConfigurationTable. This patch implements the
area itself as an addition to the SEV enhanced reset vector. The
reset vector scheme allows additions but not removals. If the size of
the reset vector is 22, it only contains the AP reset IP, but if it is
30 (or greater) it contains the SEV secret page location and size.

Signed-off-by: James Bottomley <jejb@...>
---
OvmfPkg/OvmfPkg.dec | 5 +++++
OvmfPkg/AmdSev/AmdSevX64.fdf | 3 +++
OvmfPkg/ResetVector/ResetVector.inf | 4 ++++
OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 4 ++++
OvmfPkg/ResetVector/ResetVector.nasmb | 2 ++
5 files changed, 18 insertions(+)

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 3fbf7a0ee1..b00f083417 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -117,6 +117,7 @@
gLinuxEfiInitrdMediaGuid = {0x5568e427, 0x68fc, 0x4f3d, {0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68}}
gQemuKernelLoaderFsMediaGuid = {0x1428f772, 0xb64a, 0x441e, {0xb8, 0xc3, 0x9e, 0xbd, 0xd7, 0xf8, 0x93, 0xc7}}
gGrubFileGuid = {0xb5ae312c, 0xbc8a, 0x43b1, {0x9c, 0x62, 0xeb, 0xb8, 0x26, 0xdd, 0x5d, 0x07}}
+ gSevLaunchSecretGuid = {0xadf956ad, 0xe98c, 0x484c, {0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47}}

[Ppis]
# PPI whose presence in the PPI database signals that the TPM base address
@@ -304,6 +305,10 @@
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|0|UINT32|0x40
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize|0|UINT32|0x41

+ ## The base address and size of the SEV Launch Secret Area
(1) I request that we please include the expression "remote attestation"
in the above comment, somehow.


(2) Please extend the comment with a statement that, in case the
platform sets either PCD to nonzero, the platform is responsible for
protecting the area from DXE-phase overwrites.


+ gSevLaunchSecretGuid.PcdSevLaunchSecretBase|0x0|UINT32|0
+ gSevLaunchSecretGuid.PcdSevLaunchSecretSize|0x0|UINT32|1
+
(3) Please fold these new PCD declarations into the
"gUefiOvmfPkgTokenSpaceGuid" token space, and (consequently) please use
token values 0x42 and 0x43.


[PcdsDynamic, PcdsDynamicEx]
gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf
index 689386612d..1fd38b3fe2 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.fdf
+++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
@@ -59,6 +59,9 @@ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmf
0x00B000|0x001000
gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize

+0x00C000|0x001000
+gSevLaunchSecretGuid.PcdSevLaunchSecretBase|gSevLaunchSecretGuid.PcdSevLaunchSecretSize
+
0x010000|0x010000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
(4) This hunk looks OK, but please move it over to the next patch (4/4).

I feel it allows for nicer diffstats if:

- patch#3 introduces the new PCDs and extends the (kind of general)
reset vector with exposing the PCD values in the flash, and

- patch#4 modifies the new platform to both set the PCDs and satisfy the
requirement described under (2) -- by way of including SecretPei.


diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
index a53ae6c194..72fd78eef4 100644
--- a/OvmfPkg/ResetVector/ResetVector.inf
+++ b/OvmfPkg/ResetVector/ResetVector.inf
@@ -43,3 +43,7 @@
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
+
+[FixedPcd]
+ gSevLaunchSecretGuid.PcdSevLaunchSecretBase
+ gSevLaunchSecretGuid.PcdSevLaunchSecretSize
OK.


diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
index 980e0138e7..7d3214e55d 100644
--- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
@@ -35,6 +35,8 @@ ALIGN 16
; the build time RIP value. The GUID must always be 48 bytes from the
; end of the firmware.
;
+; 0xffffffc2 (-0x3e) - Base Location of the SEV Launch Secret
+; 0xffffffc6 (-0x3a) - Size of SEV Launch Secret
; 0xffffffca (-0x36) - IP value
; 0xffffffcc (-0x34) - CS segment base [31:16]
; 0xffffffce (-0x32) - Size of the SEV-ES reset block
@@ -51,6 +53,8 @@ ALIGN 16
TIMES (32 - (sevEsResetBlockEnd - sevEsResetBlockStart)) DB 0

sevEsResetBlockStart:
+ DD SEV_LAUNCH_SECRET_BASE
+ DD SEV_LAUNCH_SECRET_SIZE
DD SEV_ES_AP_RESET_IP
DW sevEsResetBlockEnd - sevEsResetBlockStart
DB 0xDE, 0x71, 0xF7, 0x00, 0x7E, 0x1A, 0xCB, 0x4F
(5) I'd prefer if we could introduce a new GUID-ed structure for these
new fields. The logic in QEMU should be extended to start scanning at
4GB-48 for GUIDS. If the GUID is not recognized, then terminate
scanning. Otherwise, act upon the GUID-ed structure found there as
necessary, and then determine the next GUID *candidate* location by
subtracting the last recognized GUID-ed structure's "size" field.

For a bit larger context from this file, we have (pre-patch):

;
; SEV-ES Processor Reset support
;
; sevEsResetBlock:
; For the initial boot of an AP under SEV-ES, the "reset" RIP must be
; programmed to the RAM area defined by SEV_ES_AP_RESET_IP. A known offset
; and GUID will be used to locate this block in the firmware and extract
; the build time RIP value. The GUID must always be 48 bytes from the
; end of the firmware.
;
; 0xffffffca (-0x36) - IP value
; 0xffffffcc (-0x34) - CS segment base [31:16]
; 0xffffffce (-0x32) - Size of the SEV-ES reset block
; 0xffffffd0 (-0x30) - SEV-ES reset block GUID
; (00f771de-1a7e-4fcb-890e-68c77e2fb44e)
;
; A hypervisor reads the CS segement base and IP value. The CS segment base
; value represents the high order 16-bits of the CS segment base, so the
; hypervisor must left shift the value of the CS segement base by 16 bits to
; form the full CS segment base for the CS segment register. It would then
; program the EIP register with the IP value as read.
;

TIMES (32 - (sevEsResetBlockEnd - sevEsResetBlockStart)) DB 0

sevEsResetBlockStart:
DD SEV_ES_AP_RESET_IP
DW sevEsResetBlockEnd - sevEsResetBlockStart
DB 0xDE, 0x71, 0xF7, 0x00, 0x7E, 0x1A, 0xCB, 0x4F
DB 0x89, 0x0E, 0x68, 0xC7, 0x7E, 0x2F, 0xB4, 0x4E
sevEsResetBlockEnd:
I'm not exactly sure why we added the padding (TIMES ... DB 0) in edk2
commit 30937f2f98c4 ("OvmfPkg: Use the SEV-ES work area for the SEV-ES
AP reset vector", 2020-08-17). I can imagine it was *already* for the
same purpose -- to deterministically terminate the above-described
backwards-traversal of the GUID-ed structures (and at the same time
remain aligned to 32 bytes, regarding the cumulative size of all
provided structures).

So, in that vein, I'd propose something like this (relative to master @
d448574e7310):

diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
index 980e0138e7fe..957356ff997e 100644
--- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
@@ -16,55 +16,83 @@ ALIGN 16
; Pad the image size to 4k when page tables are in VTF0
;
; If the VTF0 image has page tables built in, then we need to make
; sure the end of VTF0 is 4k above where the page tables end.
;
; This is required so the page tables will be 4k aligned when VTF0 is
; located just below 0x100000000 (4GB) in the firmware device.
;
%ifdef ALIGN_TOP_TO_4K_FOR_PAGING
TIMES (0x1000 - ($ - EndOfPageTables) - 0x20) DB 0
%endif

+;
+; pre-pad the sequence of GUIDed structures to a multiple of 32 bytes
+;
+TIMES (31 - (guidedStructuresEnd - guidedStructuresStart + 31) % 32) DB 0
+
+guidedStructuresStart:
+;
+; Zero GUID to terminate decreasing address order traversal.
+;
+TIMES 16 DB 0
+
+;
+; Expose the location of the SEV Launch Secret area to the hypervisor
+; (necessary when using the remote attestation firmware platform).
+;
+; sevLaunchSecretDescriptor:
+; This GUIDed structure is chained in decreasing address order from
+; sevEsResetBlock. It describes the guest RAM area where the hypervisor has
+; to securely inject the SEV Launch Secret. The GUID is
+; 78C93F1E-ADBC-4259-B92B-CE81E523FBC4.
+;
+sevLaunchSecretDescriptorStart:
+ DD SEV_LAUNCH_SECRET_BASE
+ DD SEV_LAUNCH_SECRET_SIZE
+ DW sevLaunchSecretDescriptorEnd - sevLaunchSecretDescriptorStart
+ DB 0x1E, 0x3F, 0xC9, 0x78, 0xBC, 0xAD, 0x59, 0x42
+ DB 0xB9, 0x2B, 0xCE, 0x81, 0xE5, 0x23, 0xFB, 0xC4
+sevLaunchSecretDescriptorEnd:
+
;
; SEV-ES Processor Reset support
;
; sevEsResetBlock:
; For the initial boot of an AP under SEV-ES, the "reset" RIP must be
; programmed to the RAM area defined by SEV_ES_AP_RESET_IP. A known offset
; and GUID will be used to locate this block in the firmware and extract
; the build time RIP value. The GUID must always be 48 bytes from the
; end of the firmware.
;
; 0xffffffca (-0x36) - IP value
; 0xffffffcc (-0x34) - CS segment base [31:16]
; 0xffffffce (-0x32) - Size of the SEV-ES reset block
; 0xffffffd0 (-0x30) - SEV-ES reset block GUID
; (00f771de-1a7e-4fcb-890e-68c77e2fb44e)
;
; A hypervisor reads the CS segement base and IP value. The CS segment base
; value represents the high order 16-bits of the CS segment base, so the
; hypervisor must left shift the value of the CS segement base by 16 bits to
; form the full CS segment base for the CS segment register. It would then
; program the EIP register with the IP value as read.
;

-TIMES (32 - (sevEsResetBlockEnd - sevEsResetBlockStart)) DB 0
-
sevEsResetBlockStart:
DD SEV_ES_AP_RESET_IP
DW sevEsResetBlockEnd - sevEsResetBlockStart
DB 0xDE, 0x71, 0xF7, 0x00, 0x7E, 0x1A, 0xCB, 0x4F
DB 0x89, 0x0E, 0x68, 0xC7, 0x7E, 0x2F, 0xB4, 0x4E
sevEsResetBlockEnd:
+guidedStructuresEnd:

ALIGN 16

applicationProcessorEntryPoint:
;
; Application Processors entry point
;
; GenFv generates code aligned on a 4k boundary which will jump to this
; location. (0xffffffe0) This allows the Local APIC Startup IPI to be
; used to wake up the application processors.
;
jmp EarlyApInitReal16
Back to your patch:

On 11/12/20 01:13, James Bottomley wrote:
diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
index 4913b379a9..c5e0fe93ab 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -83,5 +83,7 @@
%include "Main.asm"

%define SEV_ES_AP_RESET_IP FixedPcdGet32 (PcdSevEsWorkAreaBase)
+ %define SEV_LAUNCH_SECRET_BASE FixedPcdGet32 (PcdSevLaunchSecretBase)
+ %define SEV_LAUNCH_SECRET_SIZE FixedPcdGet32 (PcdSevLaunchSecretSize)
%include "Ia16/ResetVectorVtf0.asm"

OK.

Thanks,
Laszlo


Lendacky, Thomas
 

On 11/16/20 4:46 PM, Laszlo Ersek via groups.io wrote:
On 11/12/20 01:13, James Bottomley wrote:
SEV needs an area to place an injected secret where OVMF can find it
and pass it up as a ConfigurationTable. This patch implements the
area itself as an addition to the SEV enhanced reset vector. The
reset vector scheme allows additions but not removals. If the size of
the reset vector is 22, it only contains the AP reset IP, but if it is
30 (or greater) it contains the SEV secret page location and size.

Signed-off-by: James Bottomley <jejb@...>
---
OvmfPkg/OvmfPkg.dec | 5 +++++
OvmfPkg/AmdSev/AmdSevX64.fdf | 3 +++
OvmfPkg/ResetVector/ResetVector.inf | 4 ++++
OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 4 ++++
OvmfPkg/ResetVector/ResetVector.nasmb | 2 ++
5 files changed, 18 insertions(+)
...

;
; SEV-ES Processor Reset support
;
; sevEsResetBlock:
; For the initial boot of an AP under SEV-ES, the "reset" RIP must be
; programmed to the RAM area defined by SEV_ES_AP_RESET_IP. A known offset
; and GUID will be used to locate this block in the firmware and extract
; the build time RIP value. The GUID must always be 48 bytes from the
; end of the firmware.
;
; 0xffffffca (-0x36) - IP value
; 0xffffffcc (-0x34) - CS segment base [31:16]
; 0xffffffce (-0x32) - Size of the SEV-ES reset block
; 0xffffffd0 (-0x30) - SEV-ES reset block GUID
; (00f771de-1a7e-4fcb-890e-68c77e2fb44e)
;
; A hypervisor reads the CS segement base and IP value. The CS segment base
; value represents the high order 16-bits of the CS segment base, so the
; hypervisor must left shift the value of the CS segement base by 16 bits to
; form the full CS segment base for the CS segment register. It would then
; program the EIP register with the IP value as read.
;

TIMES (32 - (sevEsResetBlockEnd - sevEsResetBlockStart)) DB 0

sevEsResetBlockStart:
DD SEV_ES_AP_RESET_IP
DW sevEsResetBlockEnd - sevEsResetBlockStart
DB 0xDE, 0x71, 0xF7, 0x00, 0x7E, 0x1A, 0xCB, 0x4F
DB 0x89, 0x0E, 0x68, 0xC7, 0x7E, 0x2F, 0xB4, 0x4E
sevEsResetBlockEnd:
I'm not exactly sure why we added the padding (TIMES ... DB 0) in edk2
commit 30937f2f98c4 ("OvmfPkg: Use the SEV-ES work area for the SEV-ES
AP reset vector", 2020-08-17). I can imagine it was *already* for the
same purpose -- to deterministically terminate the above-described
backwards-traversal of the GUID-ed structures (and at the same time
remain aligned to 32 bytes, regarding the cumulative size of all
provided structures).
The padding is required to "push" the GUID into the proper location at exactly 48 bytes from the end of the file. Without the padding, the GUID doesn't line up correctly and can't be located.

Thanks,
Tom

So, in that vein, I'd propose something like this (relative to master @
d448574e7310):

diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
index 980e0138e7fe..957356ff997e 100644
--- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
@@ -16,55 +16,83 @@ ALIGN 16
; Pad the image size to 4k when page tables are in VTF0
;
; If the VTF0 image has page tables built in, then we need to make
; sure the end of VTF0 is 4k above where the page tables end.
;
; This is required so the page tables will be 4k aligned when VTF0 is
; located just below 0x100000000 (4GB) in the firmware device.
;
%ifdef ALIGN_TOP_TO_4K_FOR_PAGING
TIMES (0x1000 - ($ - EndOfPageTables) - 0x20) DB 0
%endif

+;
+; pre-pad the sequence of GUIDed structures to a multiple of 32 bytes
+;
+TIMES (31 - (guidedStructuresEnd - guidedStructuresStart + 31) % 32) DB 0
+
+guidedStructuresStart:
+;
+; Zero GUID to terminate decreasing address order traversal.
+;
+TIMES 16 DB 0
+
+;
+; Expose the location of the SEV Launch Secret area to the hypervisor
+; (necessary when using the remote attestation firmware platform).
+;
+; sevLaunchSecretDescriptor:
+; This GUIDed structure is chained in decreasing address order from
+; sevEsResetBlock. It describes the guest RAM area where the hypervisor has
+; to securely inject the SEV Launch Secret. The GUID is
+; 78C93F1E-ADBC-4259-B92B-CE81E523FBC4.
+;
+sevLaunchSecretDescriptorStart:
+ DD SEV_LAUNCH_SECRET_BASE
+ DD SEV_LAUNCH_SECRET_SIZE
+ DW sevLaunchSecretDescriptorEnd - sevLaunchSecretDescriptorStart
+ DB 0x1E, 0x3F, 0xC9, 0x78, 0xBC, 0xAD, 0x59, 0x42
+ DB 0xB9, 0x2B, 0xCE, 0x81, 0xE5, 0x23, 0xFB, 0xC4
+sevLaunchSecretDescriptorEnd:
+
;
; SEV-ES Processor Reset support
;
; sevEsResetBlock:
; For the initial boot of an AP under SEV-ES, the "reset" RIP must be
; programmed to the RAM area defined by SEV_ES_AP_RESET_IP. A known offset
; and GUID will be used to locate this block in the firmware and extract
; the build time RIP value. The GUID must always be 48 bytes from the
; end of the firmware.
;
; 0xffffffca (-0x36) - IP value
; 0xffffffcc (-0x34) - CS segment base [31:16]
; 0xffffffce (-0x32) - Size of the SEV-ES reset block
; 0xffffffd0 (-0x30) - SEV-ES reset block GUID
; (00f771de-1a7e-4fcb-890e-68c77e2fb44e)
;
; A hypervisor reads the CS segement base and IP value. The CS segment base
; value represents the high order 16-bits of the CS segment base, so the
; hypervisor must left shift the value of the CS segement base by 16 bits to
; form the full CS segment base for the CS segment register. It would then
; program the EIP register with the IP value as read.
;

-TIMES (32 - (sevEsResetBlockEnd - sevEsResetBlockStart)) DB 0
-
sevEsResetBlockStart:
DD SEV_ES_AP_RESET_IP
DW sevEsResetBlockEnd - sevEsResetBlockStart
DB 0xDE, 0x71, 0xF7, 0x00, 0x7E, 0x1A, 0xCB, 0x4F
DB 0x89, 0x0E, 0x68, 0xC7, 0x7E, 0x2F, 0xB4, 0x4E
sevEsResetBlockEnd:
+guidedStructuresEnd:

ALIGN 16

applicationProcessorEntryPoint:
;
; Application Processors entry point
;
; GenFv generates code aligned on a 4k boundary which will jump to this
; location. (0xffffffe0) This allows the Local APIC Startup IPI to be
; used to wake up the application processors.
;
jmp EarlyApInitReal16
Back to your patch:
On 11/12/20 01:13, James Bottomley wrote:
diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
index 4913b379a9..c5e0fe93ab 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -83,5 +83,7 @@
%include "Main.asm"

%define SEV_ES_AP_RESET_IP FixedPcdGet32 (PcdSevEsWorkAreaBase)
+ %define SEV_LAUNCH_SECRET_BASE FixedPcdGet32 (PcdSevLaunchSecretBase)
+ %define SEV_LAUNCH_SECRET_SIZE FixedPcdGet32 (PcdSevLaunchSecretSize)
%include "Ia16/ResetVectorVtf0.asm"

OK.
Thanks,
Laszlo


Laszlo Ersek
 

On 11/18/20 21:23, James Bottomley wrote:
On Mon, 2020-11-16 at 23:46 +0100, Laszlo Ersek wrote:
On 11/12/20 01:13, James Bottomley wrote:
[... I made all the changes above this]
diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
index 980e0138e7..7d3214e55d 100644
--- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
@@ -35,6 +35,8 @@ ALIGN 16
; the build time RIP value. The GUID must always be 48 bytes
from the
; end of the firmware.
;
+; 0xffffffc2 (-0x3e) - Base Location of the SEV Launch Secret
+; 0xffffffc6 (-0x3a) - Size of SEV Launch Secret
; 0xffffffca (-0x36) - IP value
; 0xffffffcc (-0x34) - CS segment base [31:16]
; 0xffffffce (-0x32) - Size of the SEV-ES reset block
@@ -51,6 +53,8 @@ ALIGN 16
TIMES (32 - (sevEsResetBlockEnd - sevEsResetBlockStart)) DB 0

sevEsResetBlockStart:
+ DD SEV_LAUNCH_SECRET_BASE
+ DD SEV_LAUNCH_SECRET_SIZE
DD SEV_ES_AP_RESET_IP
DW sevEsResetBlockEnd - sevEsResetBlockStart
DB 0xDE, 0x71, 0xF7, 0x00, 0x7E, 0x1A, 0xCB, 0x4F
(5) I'd prefer if we could introduce a new GUID-ed structure for
these new fields. The logic in QEMU should be extended to start
scanning at 4GB-48 for GUIDS. If the GUID is not recognized, then
terminate scanning. Otherwise, act upon the GUID-ed structure found
there as necessary, and then determine the next GUID *candidate*
location by subtracting the last recognized GUID-ed structure's
"size" field.
So for this one, we can do it either way. However, the current design
of the sevEsRestBlock is (according to AMD) to allow the addition of
SEV specific information. Each piece of information is a specific
offset from the GUID and the length of the structure can only grow, so
the ordering is fixed once the info is added and you can tell if the
section contains what you're looking for is present if the length
covers it.

We can certainly move this to a fully GUID based system, which would
allow us to have an unordered list rather than the strict definition
the never decreasing length scheme allows, but if we do that, the
length word above becomes redundant.
Well, GUIDed structs in UEFI/PI are sometimes permitted to grow
compatibily, and for that, either a revision field or a size field is
necessary / used. I kind of desire both here -- it makes sense to extend
(for example) the SEV-ES reset block with relevant information, and to
add other blocks of information (identified with different GUIDs).

Basically I wouldn't want to finalize the SEV-ES AP reset block just
yet, *but* I also think this new information does not beloing in the
SEV-ES *AP reset block*. The new info is related to SEV-ES alright, but
not to the AP reset block, in my opinion. If you read the larger context
(the docs) in the assembly source around "sevEsResetBlockStart", the
launch secret just doesn't seem to fit that.

I don't have a huge preference for either mechanism ... they seem to
work equally well, but everyone should agree before I replace the
length based scheme.
I agree we should all agree about it first.

And, to reiterate, I'd like to keep both the length fields and the
GUID-ed identification. In other words, a GUID should not imply an exact
struct size, just a minimum struct size.

Thanks!
Laszlo


Laszlo Ersek
 

On 11/18/20 21:39, Tom Lendacky wrote:
On 11/16/20 4:46 PM, Laszlo Ersek via groups.io wrote:
On 11/12/20 01:13, James Bottomley wrote:
SEV needs an area to place an injected secret where OVMF can find it
and pass it up as a ConfigurationTable.  This patch implements the
area itself as an addition to the SEV enhanced reset vector.  The
reset vector scheme allows additions but not removals.  If the size of
the reset vector is 22, it only contains the AP reset IP, but if it is
30 (or greater) it contains the SEV secret page location and size.

Signed-off-by: James Bottomley <jejb@...>
---
  OvmfPkg/OvmfPkg.dec                          | 5 +++++
  OvmfPkg/AmdSev/AmdSevX64.fdf                 | 3 +++
  OvmfPkg/ResetVector/ResetVector.inf          | 4 ++++
  OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 4 ++++
  OvmfPkg/ResetVector/ResetVector.nasmb        | 2 ++
  5 files changed, 18 insertions(+)
...

;
; SEV-ES Processor Reset support
;
; sevEsResetBlock:
;   For the initial boot of an AP under SEV-ES, the "reset" RIP must be
;   programmed to the RAM area defined by SEV_ES_AP_RESET_IP. A known
offset
;   and GUID will be used to locate this block in the firmware and
extract
;   the build time RIP value. The GUID must always be 48 bytes from the
;   end of the firmware.
;
;   0xffffffca (-0x36) - IP value
;   0xffffffcc (-0x34) - CS segment base [31:16]
;   0xffffffce (-0x32) - Size of the SEV-ES reset block
;   0xffffffd0 (-0x30) - SEV-ES reset block GUID
;                        (00f771de-1a7e-4fcb-890e-68c77e2fb44e)
;
;   A hypervisor reads the CS segement base and IP value. The CS
segment base
;   value represents the high order 16-bits of the CS segment base,
so the
;   hypervisor must left shift the value of the CS segement base by
16 bits to
;   form the full CS segment base for the CS segment register. It
would then
;   program the EIP register with the IP value as read.
;

TIMES (32 - (sevEsResetBlockEnd - sevEsResetBlockStart)) DB 0

sevEsResetBlockStart:
     DD      SEV_ES_AP_RESET_IP
     DW      sevEsResetBlockEnd - sevEsResetBlockStart
     DB      0xDE, 0x71, 0xF7, 0x00, 0x7E, 0x1A, 0xCB, 0x4F
     DB      0x89, 0x0E, 0x68, 0xC7, 0x7E, 0x2F, 0xB4, 0x4E
sevEsResetBlockEnd:
I'm not exactly sure why we added the padding (TIMES ... DB 0) in edk2
commit 30937f2f98c4 ("OvmfPkg: Use the SEV-ES work area for the SEV-ES
AP reset vector", 2020-08-17). I can imagine it was *already* for the
same purpose -- to deterministically terminate the above-described
backwards-traversal of the GUID-ed structures (and at the same time
remain aligned to 32 bytes, regarding the cumulative size of all
provided structures).
The padding is required to "push" the GUID into the proper location at
exactly 48 bytes from the end of the file. Without the padding, the GUID
doesn't line up correctly and can't be located.
OK, thanks! So I think that my proposed movement / reworking of the
prepended padding should be fine.

Laszlo


Thanks,
Tom


So, in that vein, I'd propose something like this (relative to master @
d448574e7310):

diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
index 980e0138e7fe..957356ff997e 100644
--- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
@@ -16,55 +16,83 @@ ALIGN   16
  ; Pad the image size to 4k when page tables are in VTF0
  ;
  ; If the VTF0 image has page tables built in, then we need to make
  ; sure the end of VTF0 is 4k above where the page tables end.
  ;
  ; This is required so the page tables will be 4k aligned when VTF0 is
  ; located just below 0x100000000 (4GB) in the firmware device.
  ;
  %ifdef ALIGN_TOP_TO_4K_FOR_PAGING
      TIMES (0x1000 - ($ - EndOfPageTables) - 0x20) DB 0
  %endif

+;
+; pre-pad the sequence of GUIDed structures to a multiple of 32 bytes
+;
+TIMES (31 - (guidedStructuresEnd - guidedStructuresStart + 31) % 32)
DB 0
+
+guidedStructuresStart:
+;
+; Zero GUID to terminate decreasing address order traversal.
+;
+TIMES 16 DB 0
+
+;
+; Expose the location of the SEV Launch Secret area to the hypervisor
+; (necessary when using the remote attestation firmware platform).
+;
+; sevLaunchSecretDescriptor:
+;   This GUIDed structure is chained in decreasing address order from
+;   sevEsResetBlock. It describes the guest RAM area where the
hypervisor has
+;   to securely inject the SEV Launch Secret. The GUID is
+;   78C93F1E-ADBC-4259-B92B-CE81E523FBC4.
+;
+sevLaunchSecretDescriptorStart:
+    DD      SEV_LAUNCH_SECRET_BASE
+    DD      SEV_LAUNCH_SECRET_SIZE
+    DW      sevLaunchSecretDescriptorEnd -
sevLaunchSecretDescriptorStart
+    DB      0x1E, 0x3F, 0xC9, 0x78, 0xBC, 0xAD, 0x59, 0x42
+    DB      0xB9, 0x2B, 0xCE, 0x81, 0xE5, 0x23, 0xFB, 0xC4
+sevLaunchSecretDescriptorEnd:
+
  ;
  ; SEV-ES Processor Reset support
  ;
  ; sevEsResetBlock:
  ;   For the initial boot of an AP under SEV-ES, the "reset" RIP
must be
  ;   programmed to the RAM area defined by SEV_ES_AP_RESET_IP. A
known offset
  ;   and GUID will be used to locate this block in the firmware and
extract
  ;   the build time RIP value. The GUID must always be 48 bytes from
the
  ;   end of the firmware.
  ;
  ;   0xffffffca (-0x36) - IP value
  ;   0xffffffcc (-0x34) - CS segment base [31:16]
  ;   0xffffffce (-0x32) - Size of the SEV-ES reset block
  ;   0xffffffd0 (-0x30) - SEV-ES reset block GUID
  ;                        (00f771de-1a7e-4fcb-890e-68c77e2fb44e)
  ;
  ;   A hypervisor reads the CS segement base and IP value. The CS
segment base
  ;   value represents the high order 16-bits of the CS segment base,
so the
  ;   hypervisor must left shift the value of the CS segement base by
16 bits to
  ;   form the full CS segment base for the CS segment register. It
would then
  ;   program the EIP register with the IP value as read.
  ;

-TIMES (32 - (sevEsResetBlockEnd - sevEsResetBlockStart)) DB 0
-
  sevEsResetBlockStart:
      DD      SEV_ES_AP_RESET_IP
      DW      sevEsResetBlockEnd - sevEsResetBlockStart
      DB      0xDE, 0x71, 0xF7, 0x00, 0x7E, 0x1A, 0xCB, 0x4F
      DB      0x89, 0x0E, 0x68, 0xC7, 0x7E, 0x2F, 0xB4, 0x4E
  sevEsResetBlockEnd:
+guidedStructuresEnd:

  ALIGN   16

  applicationProcessorEntryPoint:
  ;
  ; Application Processors entry point
  ;
  ; GenFv generates code aligned on a 4k boundary which will jump to
this
  ; location.  (0xffffffe0)  This allows the Local APIC Startup IPI
to be
  ; used to wake up the application processors.
  ;
      jmp     EarlyApInitReal16
Back to your patch:

On 11/12/20 01:13, James Bottomley wrote:
diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb
b/OvmfPkg/ResetVector/ResetVector.nasmb
index 4913b379a9..c5e0fe93ab 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -83,5 +83,7 @@
  %include "Main.asm"

    %define SEV_ES_AP_RESET_IP  FixedPcdGet32 (PcdSevEsWorkAreaBase)
+  %define SEV_LAUNCH_SECRET_BASE  FixedPcdGet32
(PcdSevLaunchSecretBase)
+  %define SEV_LAUNCH_SECRET_SIZE  FixedPcdGet32
(PcdSevLaunchSecretSize)
  %include "Ia16/ResetVectorVtf0.asm"

OK.

Thanks,
Laszlo






James Bottomley <jejb@...>
 

On Mon, 2020-11-16 at 23:46 +0100, Laszlo Ersek wrote:
On 11/12/20 01:13, James Bottomley wrote:
[... I made all the changes above this]
diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
index 980e0138e7..7d3214e55d 100644
--- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
@@ -35,6 +35,8 @@ ALIGN 16
; the build time RIP value. The GUID must always be 48 bytes
from the
; end of the firmware.
;
+; 0xffffffc2 (-0x3e) - Base Location of the SEV Launch Secret
+; 0xffffffc6 (-0x3a) - Size of SEV Launch Secret
; 0xffffffca (-0x36) - IP value
; 0xffffffcc (-0x34) - CS segment base [31:16]
; 0xffffffce (-0x32) - Size of the SEV-ES reset block
@@ -51,6 +53,8 @@ ALIGN 16
TIMES (32 - (sevEsResetBlockEnd - sevEsResetBlockStart)) DB 0

sevEsResetBlockStart:
+ DD SEV_LAUNCH_SECRET_BASE
+ DD SEV_LAUNCH_SECRET_SIZE
DD SEV_ES_AP_RESET_IP
DW sevEsResetBlockEnd - sevEsResetBlockStart
DB 0xDE, 0x71, 0xF7, 0x00, 0x7E, 0x1A, 0xCB, 0x4F
(5) I'd prefer if we could introduce a new GUID-ed structure for
these new fields. The logic in QEMU should be extended to start
scanning at 4GB-48 for GUIDS. If the GUID is not recognized, then
terminate scanning. Otherwise, act upon the GUID-ed structure found
there as necessary, and then determine the next GUID *candidate*
location by subtracting the last recognized GUID-ed structure's
"size" field.
So for this one, we can do it either way. However, the current design
of the sevEsRestBlock is (according to AMD) to allow the addition of
SEV specific information. Each piece of information is a specific
offset from the GUID and the length of the structure can only grow, so
the ordering is fixed once the info is added and you can tell if the
section contains what you're looking for is present if the length
covers it.

We can certainly move this to a fully GUID based system, which would
allow us to have an unordered list rather than the strict definition
the never decreasing length scheme allows, but if we do that, the
length word above becomes redundant.

I don't have a huge preference for either mechanism ... they seem to
work equally well, but everyone should agree before I replace the
length based scheme.

James


Brijesh Singh <brijesh.singh@...>
 

On 11/19/20 1:50 AM, Laszlo Ersek wrote:
On 11/18/20 21:23, James Bottomley wrote:
On Mon, 2020-11-16 at 23:46 +0100, Laszlo Ersek wrote:
On 11/12/20 01:13, James Bottomley wrote:
[... I made all the changes above this]
diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
index 980e0138e7..7d3214e55d 100644
--- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
@@ -35,6 +35,8 @@ ALIGN 16
; the build time RIP value. The GUID must always be 48 bytes
from the
; end of the firmware.
;
+; 0xffffffc2 (-0x3e) - Base Location of the SEV Launch Secret
+; 0xffffffc6 (-0x3a) - Size of SEV Launch Secret
; 0xffffffca (-0x36) - IP value
; 0xffffffcc (-0x34) - CS segment base [31:16]
; 0xffffffce (-0x32) - Size of the SEV-ES reset block
@@ -51,6 +53,8 @@ ALIGN 16
TIMES (32 - (sevEsResetBlockEnd - sevEsResetBlockStart)) DB 0

sevEsResetBlockStart:
+ DD SEV_LAUNCH_SECRET_BASE
+ DD SEV_LAUNCH_SECRET_SIZE
DD SEV_ES_AP_RESET_IP
DW sevEsResetBlockEnd - sevEsResetBlockStart
DB 0xDE, 0x71, 0xF7, 0x00, 0x7E, 0x1A, 0xCB, 0x4F
(5) I'd prefer if we could introduce a new GUID-ed structure for
these new fields. The logic in QEMU should be extended to start
scanning at 4GB-48 for GUIDS. If the GUID is not recognized, then
terminate scanning. Otherwise, act upon the GUID-ed structure found
there as necessary, and then determine the next GUID *candidate*
location by subtracting the last recognized GUID-ed structure's
"size" field.
So for this one, we can do it either way. However, the current design
of the sevEsRestBlock is (according to AMD) to allow the addition of
SEV specific information. Each piece of information is a specific
offset from the GUID and the length of the structure can only grow, so
the ordering is fixed once the info is added and you can tell if the
section contains what you're looking for is present if the length
covers it.

We can certainly move this to a fully GUID based system, which would
allow us to have an unordered list rather than the strict definition
the never decreasing length scheme allows, but if we do that, the
length word above becomes redundant.
Well, GUIDed structs in UEFI/PI are sometimes permitted to grow
compatibily, and for that, either a revision field or a size field is
necessary / used. I kind of desire both here -- it makes sense to extend
(for example) the SEV-ES reset block with relevant information, and to
add other blocks of information (identified with different GUIDs).

Basically I wouldn't want to finalize the SEV-ES AP reset block just
yet, *but* I also think this new information does not beloing in the
SEV-ES *AP reset block*. The new info is related to SEV-ES alright, but
not to the AP reset block, in my opinion. If you read the larger context
(the docs) in the assembly source around "sevEsResetBlockStart", the
launch secret just doesn't seem to fit that.

I don't have a huge preference for either mechanism ... they seem to
work equally well, but everyone should agree before I replace the
length based scheme.
I agree we should all agree about it first.

And, to reiterate, I'd like to keep both the length fields and the
GUID-ed identification. In other words, a GUID should not imply an exact
struct size, just a minimum struct size.

I agree with the GUID based approach, it aligns well with the future
needs. Looking forwardm we will need to reserve couple of pages (secret
and cpuid) for the SNP. In my WIP patches I extended reset block to
define a new GUID for those new fields.

https://github.com/AMDESE/ovmf/commit/87d47319411763d91219b377da709efdb057e662#diff-0ca7ec2856c316694c87b519c95db3270e0cac798eb09745cce167aad7f2d46dR28

And I am using this qemu patch to iterate through all the GUIDs and call
the corresponding callbacks.

https://github.com/AMDESE/qemu/commit/16a1266353d372cbb7c1998f27081fb8aa4d31e9



Thanks!
Laszlo


Laszlo Ersek
 

On 11/20/20 07:29, James Bottomley wrote:
On Thu, 2020-11-19 at 13:41 -0600, Brijesh Singh wrote:
On 11/19/20 1:50 AM, Laszlo Ersek wrote:
On 11/18/20 21:23, James Bottomley wrote:
On Mon, 2020-11-16 at 23:46 +0100, Laszlo Ersek wrote:
On 11/12/20 01:13, James Bottomley wrote:
[... I made all the changes above this]
diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
index 980e0138e7..7d3214e55d 100644
--- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
@@ -35,6 +35,8 @@ ALIGN 16
; the build time RIP value. The GUID must always be 48
bytes
from the
; end of the firmware.
;
+; 0xffffffc2 (-0x3e) - Base Location of the SEV Launch
Secret
+; 0xffffffc6 (-0x3a) - Size of SEV Launch Secret
; 0xffffffca (-0x36) - IP value
; 0xffffffcc (-0x34) - CS segment base [31:16]
; 0xffffffce (-0x32) - Size of the SEV-ES reset block
@@ -51,6 +53,8 @@ ALIGN 16
TIMES (32 - (sevEsResetBlockEnd - sevEsResetBlockStart)) DB
0

sevEsResetBlockStart:
+ DD SEV_LAUNCH_SECRET_BASE
+ DD SEV_LAUNCH_SECRET_SIZE
DD SEV_ES_AP_RESET_IP
DW sevEsResetBlockEnd - sevEsResetBlockStart
DB 0xDE, 0x71, 0xF7, 0x00, 0x7E, 0x1A, 0xCB, 0x4F
(5) I'd prefer if we could introduce a new GUID-ed structure
for these new fields. The logic in QEMU should be extended to
start scanning at 4GB-48 for GUIDS. If the GUID is not
recognized, then terminate scanning. Otherwise, act upon the
GUID-ed structure found there as necessary, and then determine
the next GUID *candidate* location by subtracting the last
recognized GUID-ed structure's "size" field.
So for this one, we can do it either way. However, the current
design of the sevEsRestBlock is (according to AMD) to allow the
addition of SEV specific information. Each piece of information
is a specific offset from the GUID and the length of the
structure can only grow, so the ordering is fixed once the info
is added and you can tell if the section contains what you're
looking for is present if the length covers it.

We can certainly move this to a fully GUID based system, which
would allow us to have an unordered list rather than the strict
definition the never decreasing length scheme allows, but if we
do that, the length word above becomes redundant.
Well, GUIDed structs in UEFI/PI are sometimes permitted to grow
compatibily, and for that, either a revision field or a size field
is necessary / used. I kind of desire both here -- it makes sense
to extend (for example) the SEV-ES reset block with relevant
information, and to add other blocks of information (identified
with different GUIDs).

Basically I wouldn't want to finalize the SEV-ES AP reset block
just yet, *but* I also think this new information does not beloing
in the SEV-ES *AP reset block*. The new info is related to SEV-ES
alright, but not to the AP reset block, in my opinion. If you read
the larger context (the docs) in the assembly source around
"sevEsResetBlockStart", the launch secret just doesn't seem to fit
that.

I don't have a huge preference for either mechanism ... they seem
to work equally well, but everyone should agree before I replace
the length based scheme. I agree we should all agree about it
first.
And, to reiterate, I'd like to keep both the length fields and the
GUID-ed identification. In other words, a GUID should not imply an
exact struct size, just a minimum struct size.
I agree with the GUID based approach, it aligns well with the future
needs. Looking forwardm we will need to reserve couple of pages
(secret and cpuid) for the SNP. In my WIP patches I extended reset
block to define a new GUID for those new fields.

https://github.com/AMDESE/ovmf/commit/87d47319411763d91219b377da709efdb057e662#diff-0ca7ec2856c316694c87b519c95db3270e0cac798eb09745cce167aad7f2d46dR28

And I am using this qemu patch to iterate through all the GUIDs and
call the corresponding callbacks.

https://github.com/AMDESE/qemu/commit/16a1266353d372cbb7c1998f27081fb8aa4d31e9
OK, if that's not yet upstream, I think we should do this properly:
that means having a guid and length that identifies the entire table
and then all the incorporated guids and lengths. That way we don't
have a double meaning for the reset block guid as both identifying the
start of the table and the reset vector data. Also it means we don't
need a zero guid to signal the end of the table. And also means the
reset block GUID doesn't have to always be present (if it got
deprecated for some reason).

However, the downside is that you'll have to pull out the table by this
new guid at 0xffffffd0 and its length and then iterate over the table
to find the reset block guid ... but that will make it very easy to add
the additional guids.
I agree with doing things properly.

Thanks
Laszlo


James Bottomley
 

On Thu, 2020-11-19 at 13:41 -0600, Brijesh Singh wrote:
On 11/19/20 1:50 AM, Laszlo Ersek wrote:
On 11/18/20 21:23, James Bottomley wrote:
On Mon, 2020-11-16 at 23:46 +0100, Laszlo Ersek wrote:
On 11/12/20 01:13, James Bottomley wrote:
[... I made all the changes above this]
diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
index 980e0138e7..7d3214e55d 100644
--- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
@@ -35,6 +35,8 @@ ALIGN 16
; the build time RIP value. The GUID must always be 48
bytes
from the
; end of the firmware.
;
+; 0xffffffc2 (-0x3e) - Base Location of the SEV Launch
Secret
+; 0xffffffc6 (-0x3a) - Size of SEV Launch Secret
; 0xffffffca (-0x36) - IP value
; 0xffffffcc (-0x34) - CS segment base [31:16]
; 0xffffffce (-0x32) - Size of the SEV-ES reset block
@@ -51,6 +53,8 @@ ALIGN 16
TIMES (32 - (sevEsResetBlockEnd - sevEsResetBlockStart)) DB
0

sevEsResetBlockStart:
+ DD SEV_LAUNCH_SECRET_BASE
+ DD SEV_LAUNCH_SECRET_SIZE
DD SEV_ES_AP_RESET_IP
DW sevEsResetBlockEnd - sevEsResetBlockStart
DB 0xDE, 0x71, 0xF7, 0x00, 0x7E, 0x1A, 0xCB, 0x4F
(5) I'd prefer if we could introduce a new GUID-ed structure
for these new fields. The logic in QEMU should be extended to
start scanning at 4GB-48 for GUIDS. If the GUID is not
recognized, then terminate scanning. Otherwise, act upon the
GUID-ed structure found there as necessary, and then determine
the next GUID *candidate* location by subtracting the last
recognized GUID-ed structure's "size" field.
So for this one, we can do it either way. However, the current
design of the sevEsRestBlock is (according to AMD) to allow the
addition of SEV specific information. Each piece of information
is a specific offset from the GUID and the length of the
structure can only grow, so the ordering is fixed once the info
is added and you can tell if the section contains what you're
looking for is present if the length covers it.

We can certainly move this to a fully GUID based system, which
would allow us to have an unordered list rather than the strict
definition the never decreasing length scheme allows, but if we
do that, the length word above becomes redundant.
Well, GUIDed structs in UEFI/PI are sometimes permitted to grow
compatibily, and for that, either a revision field or a size field
is necessary / used. I kind of desire both here -- it makes sense
to extend (for example) the SEV-ES reset block with relevant
information, and to add other blocks of information (identified
with different GUIDs).

Basically I wouldn't want to finalize the SEV-ES AP reset block
just yet, *but* I also think this new information does not beloing
in the SEV-ES *AP reset block*. The new info is related to SEV-ES
alright, but not to the AP reset block, in my opinion. If you read
the larger context (the docs) in the assembly source around
"sevEsResetBlockStart", the launch secret just doesn't seem to fit
that.

I don't have a huge preference for either mechanism ... they seem
to work equally well, but everyone should agree before I replace
the length based scheme. I agree we should all agree about it
first.
And, to reiterate, I'd like to keep both the length fields and the
GUID-ed identification. In other words, a GUID should not imply an
exact struct size, just a minimum struct size.
I agree with the GUID based approach, it aligns well with the future
needs. Looking forwardm we will need to reserve couple of pages
(secret and cpuid) for the SNP. In my WIP patches I extended reset
block to define a new GUID for those new fields.

https://github.com/AMDESE/ovmf/commit/87d47319411763d91219b377da709efdb057e662#diff-0ca7ec2856c316694c87b519c95db3270e0cac798eb09745cce167aad7f2d46dR28

And I am using this qemu patch to iterate through all the GUIDs and
call the corresponding callbacks.

https://github.com/AMDESE/qemu/commit/16a1266353d372cbb7c1998f27081fb8aa4d31e9
OK, if that's not yet upstream, I think we should do this properly:
that means having a guid and length that identifies the entire table
and then all the incorporated guids and lengths. That way we don't
have a double meaning for the reset block guid as both identifying the
start of the table and the reset vector data. Also it means we don't
need a zero guid to signal the end of the table. And also means the
reset block GUID doesn't have to always be present (if it got
deprecated for some reason).

However, the downside is that you'll have to pull out the table by this
new guid at 0xffffffd0 and its length and then iterate over the table
to find the reset block guid ... but that will make it very easy to add
the additional guids.

James