[RESEND] [PATCH v2 0/2] OvmfPkg: Enable measured direct boot on AMD SEV-SNP

[RESEND] [PATCH v2 0/2] OvmfPkg: Enable measured direct boot on AMD SEV-SNP

[Dov Murik]

[Resending due to missing Cc in actual patches emails.]

(Note: This is a new version of this one-year-old patch series; the v1
series [1] got a few Acked-by but it's been so long that I don't
consider them relevant anymore.)

AMD SEV and SEV-ES support measured direct boot with
kernel/initrd/cmdline hashes injected by QEMU and verified by OVMF
during boot.

To enable the same approach for AMD SEV-SNP we make sure the page in
which QEMU inserts the hashes of kernel/initrd/cmdline is not already
pre-validated, as SNP doesn't allow validating a page twice.

The first patch rearranges the pages in AmdSevX64's MEMFD so they are in
the same order both as in the main target (OvmfPkgX64), with the
exception of the SEV Launch Secret page which isn't defined in
OvmfPkgX64.

The second patch modifies the SNP metadata structure such that on
AmdSev target the SEV Launch Secret page is not included in the ranges
that are pre-validated (zero pages) by the VMM; instead the VMM will
insert content into this page (the hashes table), or mark it explicitly
as a zero page if no hashes are added.

This series is available at:
https://github.com/confidential-containers-demo/edk2/tree/snp-kernel-hashes-v2

The corresponding RFC patch series for QEMU is in:
https://lore.kernel.org/qemu-devel/20230216084913.2148508-1-dovmurik@linux.ibm.com/
or use this tree:
https://github.com/confidential-containers-demo/qemu/tree/snp-kernel-hashes-v2

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Michael Roth <michael.roth@amd.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: Mario Smarduch <mario.smarduch@amd.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>

---

v2 changes:
* Rebased on master
* Updated AmdSev MEMFD size to match OvmfX64

v1:
[1] https://edk2.groups.io/g/devel/message/88137


Dov Murik (2):
  OvmfPkg/AmdSev: Reorder MEMFD pages to match the order in
    OvmfPkgX64.fdf
  OvmfPkg/ResetVector: Exclude SEV launch secrets page from
    pre-validation

 OvmfPkg/AmdSev/AmdSevX64.fdf          | 27 ++++++++++----------
 OvmfPkg/ResetVector/ResetVector.nasmb | 14 +++++++++-
 2 files changed, 27 insertions(+), 14 deletions(-)

-- 
2.25.1

[RESEND] [PATCH v2 1/2] OvmfPkg/AmdSev: Reorder MEMFD pages to match the order in OvmfPkgX64.fdf

[Dov Murik]

Resize the MEMFD section of AmdSevX64.fdf and reorder its pages so that
it matches the same size and order used in OvmfPkgX64.fdf.

After this change, this is the difference in the MEMFD of the two
targets:

$ diff -u \
       <(sed -ne '/FD.MEMFD/,/FV.SECFV/p' OvmfPkg/OvmfPkgX64.fdf) \
       <(sed -ne '/FD.MEMFD/,/FV.SECFV/p' OvmfPkg/AmdSev/AmdSevX64.fdf)
--- /dev/fd/63  2023-02-16 07:06:15.365308683 +0000
+++ /dev/fd/62  2023-02-16 07:06:15.365308683 +0000
@@ -32,6 +32,12 @@
 0x00E000|0x001000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidSize

+0x00F000|0x000C00
+gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
+
+0x00FC00|0x000400
+gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize
+
 0x010000|0x010000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize

Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 OvmfPkg/AmdSev/AmdSevX64.fdf | 27 ++++++++++----------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf
index 5fb3b5d27632..54ba9ecf5149 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.fdf
+++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
@@ -36,10 +36,10 @@ FV = SECFV
 
 [FD.MEMFD]
 BaseAddress   = $(MEMFD_BASE_ADDRESS)
-Size          = 0xD00000
+Size          = 0xE00000
 ErasePolarity = 1
 BlockSize     = 0x10000
-NumBlocks     = 0xD0
+NumBlocks     = 0xE0
 
 0x000000|0x006000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
@@ -59,21 +59,21 @@ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmf
 0x00B000|0x001000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize
 
-0x00C000|0x000C00
-gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
-
-0x00CC00|0x000400
-gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize
-
-0x00D000|0x001000
+0x00C000|0x001000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize
 
-0x00E000|0x001000
+0x00D000|0x001000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize
 
-0x00F000|0x001000
+0x00E000|0x001000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidSize
 
+0x00F000|0x000C00
+gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
+
+0x00FC00|0x000400
+gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize
+
 0x010000|0x010000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
 
@@ -81,12 +81,13 @@ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.P
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize
 FV = PEIFV
 
-0x100000|0xC00000
+0x100000|0xD00000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvSize
 FV = DXEFV
 
 ##########################################################################################
-# Set the SEV-ES specific work area PCDs
+# Set the SEV-ES specific work area PCDs (used for all forms of SEV since the
+# the SEV STATUS MSR is now saved in the work area)
 #
 SET gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase = $(MEMFD_BASE_ADDRESS) +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfConfidentialComputingWorkAreaHeader
 SET gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize - gUefiOvmfPkgTokenSpaceGuid.PcdOvmfConfidentialComputingWorkAreaHeader
-- 
2.25.1

[RESEND] [PATCH v2 2/2] OvmfPkg/ResetVector: Exclude SEV launch secrets page from pre-validation

[Dov Murik]

In order to allow the VMM (such as QEMU) to add a page with hashes of
kernel/initrd/cmdline for measured direct boot on SNP, this page must
not be part of the SNP metadata list reported to the VMM.

Check if that page is defined; if it is, skip it in the metadata list.
In such case, VMM should fill the page with the hashes content, or
explicitly update it as a zero page (if kernel hashes are not used).

Note that for SNP, the launch secret part of the page (lower 3KB) are
not relevant and will stay zero.  The last 1KB is used for the hashes.

This should have no effect on OvmfPkgX64 targets (which don't define
PcdSevLaunchSecretBase).

Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 OvmfPkg/ResetVector/ResetVector.nasmb | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
index 94fbb0a87b37..16f3daf49d82 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -75,7 +75,19 @@
 ;
 %define SNP_SEC_MEM_BASE_DESC_2       (GHCB_BASE + 0x1000)
 %define SNP_SEC_MEM_SIZE_DESC_2       (SEV_SNP_SECRETS_BASE - SNP_SEC_MEM_BASE_DESC_2)
-%define SNP_SEC_MEM_BASE_DESC_3       (CPUID_BASE + CPUID_SIZE)
+%if (FixedPcdGet32 (PcdSevLaunchSecretBase) > 0)
+  ; There's a reserved page for SEV secrets and hashes; the VMM will fill and
+  ; validate the page, or mark it as a zero page.
+  %define EXPECTED_END_OF_LAUNCH_SECRET_PAGE (FixedPcdGet32 (PcdSevLaunchSecretBase) + \
+                                              FixedPcdGet32 (PcdSevLaunchSecretSize) + \
+                                              FixedPcdGet32 (PcdQemuHashTableSize))
+  %if (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) != EXPECTED_END_OF_LAUNCH_SECRET_PAGE)
+    %error "PcdOvmfSecPeiTempRamBase must start directly after the SEV Launch Secret page"
+  %endif
+  %define SNP_SEC_MEM_BASE_DESC_3     (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase))
+%else
+  %define SNP_SEC_MEM_BASE_DESC_3     (CPUID_BASE + CPUID_SIZE)
+%endif
 %define SNP_SEC_MEM_SIZE_DESC_3       (FixedPcdGet32 (PcdOvmfPeiMemFvBase) - SNP_SEC_MEM_BASE_DESC_3)
 
 %ifdef ARCH_X64
-- 
2.25.1

Re: [RESEND] [PATCH v2 2/2] OvmfPkg/ResetVector: Exclude SEV launch secrets page from pre-validation

[Lendacky, Thomas]

On 2/20/23 02:49, Dov Murik wrote:
> In order to allow the VMM (such as QEMU) to add a page with hashes of
> kernel/initrd/cmdline for measured direct boot on SNP, this page must
> not be part of the SNP metadata list reported to the VMM.
> 
> Check if that page is defined; if it is, skip it in the metadata list.
> In such case, VMM should fill the page with the hashes content, or
> explicitly update it as a zero page (if kernel hashes are not used).

Would it be better to define a new section type (similar to what I did in 
the SVSM PoC)? This way, it remains listed in the metadata and allows the 
VMM to detect it and decide how to handle it.

Thanks,
Tom

> 
> Note that for SNP, the launch secret part of the page (lower 3KB) are
> not relevant and will stay zero.  The last 1KB is used for the hashes.
> 
> This should have no effect on OvmfPkgX64 targets (which don't define
> PcdSevLaunchSecretBase).
> 
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
>   OvmfPkg/ResetVector/ResetVector.nasmb | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
> index 94fbb0a87b37..16f3daf49d82 100644
> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
> @@ -75,7 +75,19 @@
>   ;
>   %define SNP_SEC_MEM_BASE_DESC_2       (GHCB_BASE + 0x1000)
>   %define SNP_SEC_MEM_SIZE_DESC_2       (SEV_SNP_SECRETS_BASE - SNP_SEC_MEM_BASE_DESC_2)
> -%define SNP_SEC_MEM_BASE_DESC_3       (CPUID_BASE + CPUID_SIZE)
> +%if (FixedPcdGet32 (PcdSevLaunchSecretBase) > 0)
> +  ; There's a reserved page for SEV secrets and hashes; the VMM will fill and
> +  ; validate the page, or mark it as a zero page.
> +  %define EXPECTED_END_OF_LAUNCH_SECRET_PAGE (FixedPcdGet32 (PcdSevLaunchSecretBase) + \
> +                                              FixedPcdGet32 (PcdSevLaunchSecretSize) + \
> +                                              FixedPcdGet32 (PcdQemuHashTableSize))
> +  %if (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) != EXPECTED_END_OF_LAUNCH_SECRET_PAGE)
> +    %error "PcdOvmfSecPeiTempRamBase must start directly after the SEV Launch Secret page"
> +  %endif
> +  %define SNP_SEC_MEM_BASE_DESC_3     (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase))
> +%else
> +  %define SNP_SEC_MEM_BASE_DESC_3     (CPUID_BASE + CPUID_SIZE)
> +%endif
>   %define SNP_SEC_MEM_SIZE_DESC_3       (FixedPcdGet32 (PcdOvmfPeiMemFvBase) - SNP_SEC_MEM_BASE_DESC_3)
>   
>   %ifdef ARCH_X64

Re: [RESEND] [PATCH v2 2/2] OvmfPkg/ResetVector: Exclude SEV launch secrets page from pre-validation

[Gerd Hoffmann]

On Mon, Feb 20, 2023 at 08:44:23AM -0600, Tom Lendacky wrote:
> On 2/20/23 02:49, Dov Murik wrote:
> > In order to allow the VMM (such as QEMU) to add a page with hashes of
> > kernel/initrd/cmdline for measured direct boot on SNP, this page must
> > not be part of the SNP metadata list reported to the VMM.
> > 
> > Check if that page is defined; if it is, skip it in the metadata list.
> > In such case, VMM should fill the page with the hashes content, or
> > explicitly update it as a zero page (if kernel hashes are not used).
> 
> Would it be better to define a new section type (similar to what I did in
> the SVSM PoC)? This way, it remains listed in the metadata and allows the
> VMM to detect it and decide how to handle it.

Explicitly describing things sounds better to me too.

take care,
  Gerd

Re: [RESEND] [PATCH v2 2/2] OvmfPkg/ResetVector: Exclude SEV launch secrets page from pre-validation

[Dov Murik]

On 21/02/2023 11:38, Gerd Hoffmann wrote:
> On Mon, Feb 20, 2023 at 08:44:23AM -0600, Tom Lendacky wrote:
>> On 2/20/23 02:49, Dov Murik wrote:
>>> In order to allow the VMM (such as QEMU) to add a page with hashes of
>>> kernel/initrd/cmdline for measured direct boot on SNP, this page must
>>> not be part of the SNP metadata list reported to the VMM.
>>>
>>> Check if that page is defined; if it is, skip it in the metadata list.
>>> In such case, VMM should fill the page with the hashes content, or
>>> explicitly update it as a zero page (if kernel hashes are not used).
>>
>> Would it be better to define a new section type (similar to what I did in
>> the SVSM PoC)? This way, it remains listed in the metadata and allows the
>> VMM to detect it and decide how to handle it.
> 
> Explicitly describing things sounds better to me too.
> 

Thanks for the feedback Tom and Gerd.


I can define a new section type OVMF_SECTION_TYPE_KERNEL_HASHES. In the AmdSev
target it'll cover the single MEMFD page at 00F000 (after the CPUID page).  
Now there's a question for the QEMU side -- should QEMU then fill the page
and encrypt it (launch_update type=NORMAL)? (currently the whole hashes table
creation and encryption is done elsewhere there)

And on regular OvmfX64 builds - should that area should be with type
OVMF_SECTION_TYPE_SNP_SEC_MEM which is accepted as a type=ZERO page ?


Playing with this idea, the metadata list will add:


; Kernel hashes section for measured direct boot
%define OVMF_SECTION_TYPE_KERNEL_HASHES   0x5

...

; Kernel hashes for measured direct boot, or zero page if
; there are no kernel hashes / SEV secrets
SevSnpKernelHashes:
  DD  SEV_SNP_KERNEL_HASHES_BASE
  DD  SEV_SNP_KERNEL_HASHES_SIZE
  DD  SEV_SNP_KERNEL_HASHES_TYPE



and the base/size/type of that region are defined in an
%if statement in ResetVector.nasmb:


%if (FixedPcdGet32 (PcdSevLaunchSecretBase) > 0)
  ; There's a reserved page for SEV secrets and hashes; the VMM will fill and
  ; validate the page
  %define SEV_SNP_KERNEL_HASHES_TYPE    OVMF_SECTION_TYPE_KERNEL_HASHES
  %define SEV_SNP_KERNEL_HASHES_BASE    (FixedPcdGet32 (PcdSevLaunchSecretBase))
%else
  ; No SEV secrets and hashes page; the VMM will validate it as another zero page
  %define SEV_SNP_KERNEL_HASHES_TYPE    OVMF_SECTION_TYPE_SNP_SEC_MEM
  %define SEV_SNP_KERNEL_HASHES_BASE    (CPUID_BASE + CPUID_SIZE)
%endif
%define SEV_SNP_KERNEL_HASHES_SIZE    (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) - SEV_SNP_KERNEL_HASHES_BASE)


(I still need to figure out the point about QEMU above.)


Is that what you had in mind?

-Dov

Re: [RESEND] [PATCH v2 2/2] OvmfPkg/ResetVector: Exclude SEV launch secrets page from pre-validation

[Dov Murik]

On 23/02/2023 16:58, Dov Murik wrote:
> 
> 
> On 21/02/2023 11:38, Gerd Hoffmann wrote:
>> On Mon, Feb 20, 2023 at 08:44:23AM -0600, Tom Lendacky wrote:
>>> On 2/20/23 02:49, Dov Murik wrote:
>>>> In order to allow the VMM (such as QEMU) to add a page with hashes of
>>>> kernel/initrd/cmdline for measured direct boot on SNP, this page must
>>>> not be part of the SNP metadata list reported to the VMM.
>>>>
>>>> Check if that page is defined; if it is, skip it in the metadata list.
>>>> In such case, VMM should fill the page with the hashes content, or
>>>> explicitly update it as a zero page (if kernel hashes are not used).
>>>
>>> Would it be better to define a new section type (similar to what I did in
>>> the SVSM PoC)? This way, it remains listed in the metadata and allows the
>>> VMM to detect it and decide how to handle it.
>>
>> Explicitly describing things sounds better to me too.
>>
> 
> Thanks for the feedback Tom and Gerd.
> 
> 
> I can define a new section type OVMF_SECTION_TYPE_KERNEL_HASHES. In the AmdSev
> target it'll cover the single MEMFD page at 00F000 (after the CPUID page).  
> Now there's a question for the QEMU side -- should QEMU then fill the page
> and encrypt it (launch_update type=NORMAL)? (currently the whole hashes table
> creation and encryption is done elsewhere there)
> 
> And on regular OvmfX64 builds - should that area should be with type
> OVMF_SECTION_TYPE_SNP_SEC_MEM which is accepted as a type=ZERO page ?
> 
> 
> Playing with this idea, the metadata list will add:
> 
> 
> ; Kernel hashes section for measured direct boot
> %define OVMF_SECTION_TYPE_KERNEL_HASHES   0x5
> 
> ...
> 
> ; Kernel hashes for measured direct boot, or zero page if
> ; there are no kernel hashes / SEV secrets
> SevSnpKernelHashes:
>   DD  SEV_SNP_KERNEL_HASHES_BASE
>   DD  SEV_SNP_KERNEL_HASHES_SIZE
>   DD  SEV_SNP_KERNEL_HASHES_TYPE
> 

Or maybe this metadata section ^^^^^ should be added only if the Pcd for
secrets+hashes page is defined?

-Dov

> 
> 
> and the base/size/type of that region are defined in an
> %if statement in ResetVector.nasmb:
> 
> 
> %if (FixedPcdGet32 (PcdSevLaunchSecretBase) > 0)
>   ; There's a reserved page for SEV secrets and hashes; the VMM will fill and
>   ; validate the page
>   %define SEV_SNP_KERNEL_HASHES_TYPE    OVMF_SECTION_TYPE_KERNEL_HASHES
>   %define SEV_SNP_KERNEL_HASHES_BASE    (FixedPcdGet32 (PcdSevLaunchSecretBase))
> %else
>   ; No SEV secrets and hashes page; the VMM will validate it as another zero page
>   %define SEV_SNP_KERNEL_HASHES_TYPE    OVMF_SECTION_TYPE_SNP_SEC_MEM
>   %define SEV_SNP_KERNEL_HASHES_BASE    (CPUID_BASE + CPUID_SIZE)
> %endif
> %define SEV_SNP_KERNEL_HASHES_SIZE    (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) - SEV_SNP_KERNEL_HASHES_BASE)
> 
> 
> (I still need to figure out the point about QEMU above.)
> 
> 
> Is that what you had in mind?
> 
> -Dov

Re: [RESEND] [PATCH v2 2/2] OvmfPkg/ResetVector: Exclude SEV launch secrets page from pre-validation

[Lendacky, Thomas]

On 2/23/23 09:04, Dov Murik wrote:
> 
> 
> On 23/02/2023 16:58, Dov Murik wrote:
>>
>>
>> On 21/02/2023 11:38, Gerd Hoffmann wrote:
>>> On Mon, Feb 20, 2023 at 08:44:23AM -0600, Tom Lendacky wrote:
>>>> On 2/20/23 02:49, Dov Murik wrote:
>>>>> In order to allow the VMM (such as QEMU) to add a page with hashes of
>>>>> kernel/initrd/cmdline for measured direct boot on SNP, this page must
>>>>> not be part of the SNP metadata list reported to the VMM.
>>>>>
>>>>> Check if that page is defined; if it is, skip it in the metadata list.
>>>>> In such case, VMM should fill the page with the hashes content, or
>>>>> explicitly update it as a zero page (if kernel hashes are not used).
>>>>
>>>> Would it be better to define a new section type (similar to what I did in
>>>> the SVSM PoC)? This way, it remains listed in the metadata and allows the
>>>> VMM to detect it and decide how to handle it.
>>>
>>> Explicitly describing things sounds better to me too.
>>>
>>
>> Thanks for the feedback Tom and Gerd.
>>
>>
>> I can define a new section type OVMF_SECTION_TYPE_KERNEL_HASHES. In the AmdSev
>> target it'll cover the single MEMFD page at 00F000 (after the CPUID page).
>> Now there's a question for the QEMU side -- should QEMU then fill the page
>> and encrypt it (launch_update type=NORMAL)? (currently the whole hashes table
>> creation and encryption is done elsewhere there)

Yes, I think that is the way to go. Allocate a page in Qemu, zero it out, 
fill in the hash values at the proper location and then do a launch update 
for type NORMAL page. You can use the section type to identify the data 
you need to retrieve and encrypt.

>>
>> And on regular OvmfX64 builds - should that area should be with type
>> OVMF_SECTION_TYPE_SNP_SEC_MEM which is accepted as a type=ZERO page ?
>>
>>
>> Playing with this idea, the metadata list will add:
>>
>>
>> ; Kernel hashes section for measured direct boot
>> %define OVMF_SECTION_TYPE_KERNEL_HASHES   0x5
>>
>> ...
>>
>> ; Kernel hashes for measured direct boot, or zero page if
>> ; there are no kernel hashes / SEV secrets
>> SevSnpKernelHashes:
>>    DD  SEV_SNP_KERNEL_HASHES_BASE
>>    DD  SEV_SNP_KERNEL_HASHES_SIZE
>>    DD  SEV_SNP_KERNEL_HASHES_TYPE
>>
> 
> Or maybe this metadata section ^^^^^ should be added only if the Pcd for
> secrets+hashes page is defined?

That would be optimal if you could do that.

Thanks,
Tom

> 
> -Dov
> 
>>
>>
>> and the base/size/type of that region are defined in an
>> %if statement in ResetVector.nasmb:
>>
>>
>> %if (FixedPcdGet32 (PcdSevLaunchSecretBase) > 0)
>>    ; There's a reserved page for SEV secrets and hashes; the VMM will fill and
>>    ; validate the page
>>    %define SEV_SNP_KERNEL_HASHES_TYPE    OVMF_SECTION_TYPE_KERNEL_HASHES
>>    %define SEV_SNP_KERNEL_HASHES_BASE    (FixedPcdGet32 (PcdSevLaunchSecretBase))
>> %else
>>    ; No SEV secrets and hashes page; the VMM will validate it as another zero page
>>    %define SEV_SNP_KERNEL_HASHES_TYPE    OVMF_SECTION_TYPE_SNP_SEC_MEM
>>    %define SEV_SNP_KERNEL_HASHES_BASE    (CPUID_BASE + CPUID_SIZE)
>> %endif
>> %define SEV_SNP_KERNEL_HASHES_SIZE    (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) - SEV_SNP_KERNEL_HASHES_BASE)
>>
>>
>> (I still need to figure out the point about QEMU above.)
>>
>>
>> Is that what you had in mind?
>>
>> -Dov