[PATCH 0/2] Improved support for AMD SEV firmware loading

[PATCH 0/2] Improved support for AMD SEV firmware loading

[Daniel P. Berrangé]

The AMD SEV build of EDK2 only emits a single file, intended to be
mapped readonly. There is explicitly no separate writable VARS
store for persisting non-volatile firmware variables.

This can be used with QEMU's traditional pflash configuration
mechanism by only populating pflash0, leaving pflash1 unconfigured.
Conceptually, however, it is odd to be using pflash at all when we
have no intention of supporting any writable variables. The -bios
option should be sufficient for any firmware that is exclusively
readonly code.


A second issue is that the firmware descriptor schema does not allow
for describing a firmware that uses pflash, without any associated
non-volatile storage.

In docs/interop/firmware.json

 'struct' : 'FirmwareMappingFlash',
  'data'   : { 'executable'     : 'FirmwareFlashFile',
               'nvram-template' : 'FirmwareFlashFile' } }

Notice that nvram-template is mandatory, and when consuming these
files libvirt will thus complain if the nvram-template field is
missing.

We could in theory make nvram-template optional in the schema and
then update libvirt to take account of it, but this feels dubious
when we have a perfectly good way of describing a firmware without
NVRAM, using 'FirmwareMappingMemory' which is intended to be used
with QEMU's -bios option.


A third issue is in libvirt, where again the code handling the
configuration of pflash supports two scenarios

 - A single pflash image, which is writable
 - A pair of pflash images, one writable one readonly

There is no support for a single read-only pflash image in libvirt
today.


This all points towards the fact that we should be using -bios
to load the AMD SEV firmware build of EDK.

The only thing preventing us doing that is that QEMU does not
initialize the SEV firmware when using -bios. That is fairly
easily solved, as done in this patch series.

For testing I've launched QEMU in in these scenarios

  - SEV guest using -bios and boot from HD
  - SEV guest using pflash and boot from HD
  - SEV-ES guest using -bios and direct kernel boot
  - SEV-ES guest using pflash and direct kernel boot

In all these cases I was able to validate the reported SEV
guest measurement.

Daniel P. Berrangé (2):
  hw/i386: refactor logic for setting up SEV firmware
  hw/i386: support loading OVMF using -bios too

 hw/i386/pc_sysfw.c            | 24 +++---------------------
 hw/i386/pc_sysfw_ovmf-stubs.c | 10 ++++++++++
 hw/i386/pc_sysfw_ovmf.c       | 27 +++++++++++++++++++++++++++
 hw/i386/x86.c                 |  5 +++++
 include/hw/i386/pc.h          |  1 +
 5 files changed, 46 insertions(+), 21 deletions(-)

-- 
2.33.1

[PATCH 1/2] hw/i386: refactor logic for setting up SEV firmware

[Daniel P. Berrangé]

Currently this logic is only run in the pflash codepath
but we want to run it in other scenarios too.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/i386/pc_sysfw.c            | 24 +++---------------------
 hw/i386/pc_sysfw_ovmf-stubs.c | 10 ++++++++++
 hw/i386/pc_sysfw_ovmf.c       | 27 +++++++++++++++++++++++++++
 include/hw/i386/pc.h          |  1 +
 4 files changed, 41 insertions(+), 21 deletions(-)

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index c8b17af953..b056526077 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -146,9 +146,6 @@ static void pc_system_flash_map(PCMachineState *pcms,
     int64_t size;
     PFlashCFI01 *system_flash;
     MemoryRegion *flash_mem;
-    void *flash_ptr;
-    int flash_size;
-    int ret;
 
     assert(PC_MACHINE_GET_CLASS(pcms)->pci_enabled);
 
@@ -192,24 +189,9 @@ static void pc_system_flash_map(PCMachineState *pcms,
             flash_mem = pflash_cfi01_get_memory(system_flash);
             pc_isa_bios_init(rom_memory, flash_mem, size);
 
-            /* Encrypt the pflash boot ROM */
-            if (sev_enabled()) {
-                flash_ptr = memory_region_get_ram_ptr(flash_mem);
-                flash_size = memory_region_size(flash_mem);
-                /*
-                 * OVMF places a GUIDed structures in the flash, so
-                 * search for them
-                 */
-                pc_system_parse_ovmf_flash(flash_ptr, flash_size);
-
-                ret = sev_es_save_reset_vector(flash_ptr, flash_size);
-                if (ret) {
-                    error_report("failed to locate and/or save reset vector");
-                    exit(1);
-                }
-
-                sev_encrypt_flash(flash_ptr, flash_size, &error_fatal);
-            }
+            pc_system_ovmf_initialize_sev(
+                memory_region_get_ram_ptr(flash_mem),
+                memory_region_size(flash_mem));
         }
     }
 }
diff --git a/hw/i386/pc_sysfw_ovmf-stubs.c b/hw/i386/pc_sysfw_ovmf-stubs.c
index aabe78b271..d88970af88 100644
--- a/hw/i386/pc_sysfw_ovmf-stubs.c
+++ b/hw/i386/pc_sysfw_ovmf-stubs.c
@@ -14,6 +14,7 @@
 
 #include "qemu/osdep.h"
 #include "hw/i386/pc.h"
+#include "sev.h"
 
 bool pc_system_ovmf_table_find(const char *entry, uint8_t **data, int *data_len)
 {
@@ -24,3 +25,12 @@ void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t flash_size)
 {
     g_assert_not_reached();
 }
+
+void pc_system_ovmf_initialize_sev(void *ptr, size_t size)
+{
+    if (!sev_enabled()) {
+        return;
+    }
+
+    g_assert_not_reached();
+}
diff --git a/hw/i386/pc_sysfw_ovmf.c b/hw/i386/pc_sysfw_ovmf.c
index f4dd92c588..180500a035 100644
--- a/hw/i386/pc_sysfw_ovmf.c
+++ b/hw/i386/pc_sysfw_ovmf.c
@@ -24,8 +24,10 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "hw/i386/pc.h"
 #include "cpu.h"
+#include "sev.h"
 
 #define OVMF_TABLE_FOOTER_GUID "96b582de-1fb2-45f7-baea-a366c55a082d"
 
@@ -149,3 +151,28 @@ bool pc_system_ovmf_table_find(const char *entry, uint8_t **data,
     }
     return false;
 }
+
+
+void pc_system_ovmf_initialize_sev(void *ptr, size_t size)
+{
+    int ret;
+
+    /* Encrypt the boot ROM */
+    if (!sev_enabled()) {
+        return;
+    }
+
+    /*
+     * OVMF places a GUIDed structures in the flash, so
+     * search for them
+     */
+    pc_system_parse_ovmf_flash(ptr, size);
+
+    ret = sev_es_save_reset_vector(ptr, size);
+    if (ret) {
+        error_report("failed to locate and/or save reset vector");
+        exit(1);
+    }
+
+    sev_encrypt_flash(ptr, size, &error_fatal);
+}
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 9c9f4ac748..47f5bc82ba 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -191,6 +191,7 @@ void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
 bool pc_system_ovmf_table_find(const char *entry, uint8_t **data,
                                int *data_len);
 void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t flash_size);
+void pc_system_ovmf_initialize_sev(void *ptr, size_t size);
 
 /* hw/i386/acpi-common.c */
 void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
-- 
2.33.1

[PATCH 2/2] hw/i386: support loading OVMF using -bios too

[Daniel P. Berrangé]

Traditionally the OVMF firmware has been loaded using the pflash
mechanism. This is because it is usually provided as a pair of
files, one read-only containing the code and one writable to
provided persistence of non-volatile firmware variables.

The AMD SEV build of EDK2, however, is provided as a single
file that contains only the code. This is intended to be used
read-only and explicitly does not provide any ability for
persistance of non-volatile firmware variables. While it is
possible to configure this with the pflash mechanism, by only
providing one of the 2 pflash blobs, conceptually it is a
little strange to use pflash if there won't be any persistent
data.

A stateless OVMF build can be loaded with -bios, however, QEMU
does not currently initialize SEV in that scenario. This patch
introduces the call needed for SEV initialization of the
firmware.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/i386/x86.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index b84840a1bb..c79d84936f 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -45,6 +45,7 @@
 #include "target/i386/cpu.h"
 #include "hw/i386/topology.h"
 #include "hw/i386/fw_cfg.h"
+#include "hw/i386/pc.h"
 #include "hw/intc/i8259.h"
 #include "hw/rtc/mc146818rtc.h"
 #include "target/i386/sev.h"
@@ -1157,6 +1158,10 @@ void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
     memory_region_add_subregion(rom_memory,
                                 (uint32_t)(-bios_size),
                                 bios);
+
+    pc_system_ovmf_initialize_sev(
+        rom_ptr((uint32_t)-bios_size, bios_size),
+        bios_size);
 }
 
 bool x86_machine_is_smm_enabled(const X86MachineState *x86ms)
-- 
2.33.1

Re: [PATCH 2/2] hw/i386: support loading OVMF using -bios too

[Michael S. Tsirkin]

On Thu, Jan 13, 2022 at 04:55:11PM +0000, Daniel P. Berrangé wrote:
> Traditionally the OVMF firmware has been loaded using the pflash
> mechanism. This is because it is usually provided as a pair of
> files, one read-only containing the code and one writable to
> provided persistence of non-volatile firmware variables.
> 
> The AMD SEV build of EDK2, however, is provided as a single
> file that contains only the code. This is intended to be used
> read-only and explicitly does not provide any ability for
> persistance of non-volatile firmware variables. While it is
> possible to configure this with the pflash mechanism, by only
> providing one of the 2 pflash blobs, conceptually it is a
> little strange to use pflash if there won't be any persistent
> data.
> 
> A stateless OVMF build can be loaded with -bios, however, QEMU
> does not currently initialize SEV in that scenario. This patch
> introduces the call needed for SEV initialization of the
> firmware.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  hw/i386/x86.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index b84840a1bb..c79d84936f 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -45,6 +45,7 @@
>  #include "target/i386/cpu.h"
>  #include "hw/i386/topology.h"
>  #include "hw/i386/fw_cfg.h"
> +#include "hw/i386/pc.h"
>  #include "hw/intc/i8259.h"
>  #include "hw/rtc/mc146818rtc.h"
>  #include "target/i386/sev.h"

This builds fine because there's a stub in pc_sysfw_ovmf-stubs.c

The unfortunate thing about this however is that it's too easy to pull
in a PC dependency, and people building with CONFIG_PC will not notice
until it breaks for others.

Is it time we split pc.h further and had pc_sysfw_ovmf.h ?

> @@ -1157,6 +1158,10 @@ void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
>      memory_region_add_subregion(rom_memory,
>                                  (uint32_t)(-bios_size),
>                                  bios);
> +
> +    pc_system_ovmf_initialize_sev(
> +        rom_ptr((uint32_t)-bios_size, bios_size),
> +        bios_size);

Just curious about the formatting here:

    pc_system_ovmf_initialize_sev(rom_ptr((uint32_t)-bios_size, bios_size),
				  bios_size);

would be prettier ...

>  }
>  
>  bool x86_machine_is_smm_enabled(const X86MachineState *x86ms)
> -- 
> 2.33.1

Re: [PATCH 2/2] hw/i386: support loading OVMF using -bios too

[Philippe Mathieu-Daudé via]

On 14/1/22 19:07, Michael S. Tsirkin wrote:
> On Thu, Jan 13, 2022 at 04:55:11PM +0000, Daniel P. Berrangé wrote:
>> Traditionally the OVMF firmware has been loaded using the pflash
>> mechanism. This is because it is usually provided as a pair of
>> files, one read-only containing the code and one writable to
>> provided persistence of non-volatile firmware variables.
>>
>> The AMD SEV build of EDK2, however, is provided as a single
>> file that contains only the code. This is intended to be used
>> read-only and explicitly does not provide any ability for
>> persistance of non-volatile firmware variables. While it is
>> possible to configure this with the pflash mechanism, by only
>> providing one of the 2 pflash blobs, conceptually it is a
>> little strange to use pflash if there won't be any persistent
>> data.

It certainly would be simpler to have a ROM for the CODE part.
IIUC using CFI pflash allows the firmware to poll the underlying
device size.

>> A stateless OVMF build can be loaded with -bios, however, QEMU
>> does not currently initialize SEV in that scenario. This patch
>> introduces the call needed for SEV initialization of the
>> firmware.
>>
>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> ---
>>   hw/i386/x86.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>> index b84840a1bb..c79d84936f 100644
>> --- a/hw/i386/x86.c
>> +++ b/hw/i386/x86.c
>> @@ -45,6 +45,7 @@
>>   #include "target/i386/cpu.h"
>>   #include "hw/i386/topology.h"
>>   #include "hw/i386/fw_cfg.h"
>> +#include "hw/i386/pc.h"
>>   #include "hw/intc/i8259.h"
>>   #include "hw/rtc/mc146818rtc.h"
>>   #include "target/i386/sev.h"
> 
> This builds fine because there's a stub in pc_sysfw_ovmf-stubs.c
> 
> The unfortunate thing about this however is that it's too easy to pull
> in a PC dependency, and people building with CONFIG_PC will not notice
> until it breaks for others.
> 
> Is it time we split pc.h further and had pc_sysfw_ovmf.h ?

While "pc*" is specific to the PC machines, "x86*" is shared between
PC and microvm. "pc.h" must not be included in "x86.c". The shared
method introduced in the previous patch becomes
x86_system_ovmf_initialize_sev(). The dual pflash mechanism is proper
to OVMF, so having this method in "x86.h" seems correct.

Phil.

Re: [PATCH 0/2] Improved support for AMD SEV firmware loading

[Dov Murik]

[+cc Tom, Brijesh, Ashish - see SEV-related changes in this series]


On 13/01/2022 18:55, Daniel P. Berrangé wrote:
> The AMD SEV build of EDK2 only emits a single file, intended to be
> 
> mapped readonly. There is explicitly no separate writable VARS
> 
> store for persisting non-volatile firmware variables.
> 
> 
> 
> This can be used with QEMU's traditional pflash configuration
> 
> mechanism by only populating pflash0, leaving pflash1 unconfigured.
> 
> Conceptually, however, it is odd to be using pflash at all when we
> 
> have no intention of supporting any writable variables. The -bios
> 
> option should be sufficient for any firmware that is exclusively
> 
> readonly code.
> 
> 
> 
> 
> 
> A second issue is that the firmware descriptor schema does not allow
> 
> for describing a firmware that uses pflash, without any associated
> 
> non-volatile storage.
> 
> 
> 
> In docs/interop/firmware.json
> 
> 
> 
>  'struct' : 'FirmwareMappingFlash',
> 
>   'data'   : { 'executable'     : 'FirmwareFlashFile',
> 
>                'nvram-template' : 'FirmwareFlashFile' } }
> 
> 
> 
> Notice that nvram-template is mandatory, and when consuming these
> 
> files libvirt will thus complain if the nvram-template field is
> 
> missing.
> 
> 
> 
> We could in theory make nvram-template optional in the schema and
> 
> then update libvirt to take account of it, but this feels dubious
> 
> when we have a perfectly good way of describing a firmware without
> 
> NVRAM, using 'FirmwareMappingMemory' which is intended to be used
> 
> with QEMU's -bios option.
> 
> 
> 
> 
> 
> A third issue is in libvirt, where again the code handling the
> 
> configuration of pflash supports two scenarios
> 
> 
> 
>  - A single pflash image, which is writable
> 
>  - A pair of pflash images, one writable one readonly
> 
> 
> 
> There is no support for a single read-only pflash image in libvirt
> 
> today.
> 
> 
> 
> 
> 
> This all points towards the fact that we should be using -bios
> 
> to load the AMD SEV firmware build of EDK.
> 
> 
> 
> The only thing preventing us doing that is that QEMU does not
> 
> initialize the SEV firmware when using -bios. That is fairly
> 
> easily solved, as done in this patch series.
> 
> 
> 
> For testing I've launched QEMU in in these scenarios
> 
> 
> 
>   - SEV guest using -bios and boot from HD
> 
>   - SEV guest using pflash and boot from HD
> 
>   - SEV-ES guest using -bios and direct kernel boot
> 
>   - SEV-ES guest using pflash and direct kernel boot
> 
> 
> 
> In all these cases I was able to validate the reported SEV
> 
> guest measurement.
> 
> 


I'm having trouble testing this series (applied on top of master commit 69353c332c):
it hangs with -bios but works OK with pflash:

Here's with -bios:

$ sudo /home/dmurik/git/qemu/build/qemu-system-x86_64 -enable-kvm \
       -cpu host -machine q35 -smp 4 -m 2G \
       -machine confidential-guest-support=sev0 \
       -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1,policy=0x0 \
       -bios /home/dmurik/git/edk2/Build/AmdSev/DEBUG_GCC5/FV/OVMF.fd \
       -nographic \
       -global isa-debugcon.iobase=0x402 -debugcon file:ovmf-1.log \
       -monitor pty -trace 'enable=kvm_sev_*'

char device redirected to /dev/pts/14 (label compat_monitor0)
kvm_sev_init
kvm_sev_launch_start policy 0x0 session (nil) pdh (nil)
kvm_sev_change_state uninit -> launch-update
kvm_sev_launch_update_data addr 0x7f42e9bff010 len 0x400000
kvm_sev_change_state launch-update -> launch-secret
kvm_sev_launch_measurement data PF6n7+Vujx5sW8PC6iMRtHXfpXdJ4osbcfYvoknu7gg4ypMqs727NTzG86Ft8Llu
kvm_sev_launch_finish
kvm_sev_change_state launch-secret -> running


Here it hangs. The ovmf-1.log file is empty.

Notice that kvm_sev_launch_update_data is called, so the new
-bios behaviour is triggered correctly.  But the guest doesn't
start running.

Here is the guest's state:

(qemu) info registers
EAX=0000606b EBX=00001268 ECX=0000440c EDX=008328d2
ESI=000091e2 EDI=0000e9e3 EBP=0000a451 ESP=00009af0
EIP=00003612 EFL=00000082 [--S----] CPL=0 II=0 A20=1 SMM=0 HLT=0
ES =0000 00000000 0000ffff 00009300
CS =a76e 000a76e0 0000ffff 00009b00
SS =0000 00000000 0000ffff 00009300
DS =0000 00000000 0000ffff 00009300
FS =0000 00000000 0000ffff 00009300
GS =0000 00000000 0000ffff 00009300
LDT=0000 00000000 0000ffff 00008200
TR =0000 00000000 0000ffff 00008b00
GDT=     00000000 0000ffff
IDT=     00000000 0000ffff
CR0=60000010 CR2=00000000 CR3=00000000 CR4=00000000
DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000
DR6=00000000ffff0ff0 DR7=0000000000000400
EFER=0000000000000000
FCW=037f FSW=0000 [ST=0] FTW=00 MXCSR=00001f80
...

(qemu) info sev
handle: 1
state: running
build: 10
api version: 0.23
debug: on
key-sharing: on



If I try the same with pflash (instead of -bios), I get:

# sudo /home/dmurik/git/qemu/build/qemu-system-x86_64 -enable-kvm \
       -cpu host -machine q35 -smp 4 -m 2G \
       -machine confidential-guest-support=sev0 \
       -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1,policy=0x0 \
       -drive if=pflash,format=raw,unit=0,file=/home/dmurik/git/edk2/Build/AmdSev/DEBUG_GCC5/FV/OVMF.fd,readonly=on \
       -nographic \
       -global isa-debugcon.iobase=0x402 -debugcon file:ovmf-1.log \
       -monitor pty -trace 'enable=kvm_sev_*'

char device redirected to /dev/pts/14 (label compat_monitor0)
kvm_sev_init
kvm_sev_launch_start policy 0x0 session (nil) pdh (nil)
kvm_sev_change_state uninit -> launch-update
kvm_sev_launch_update_data addr 0x7f0343000000 len 0x400000
kvm_sev_change_state launch-update -> launch-secret
kvm_sev_launch_measurement data esqzlr4xX2eEY92xsKEKL7FyKRDW7VYiyIb+aXS4S/ctON2s1xxwFjAKU7ImfULJ
kvm_sev_launch_finish
kvm_sev_change_state launch-secret -> running
BdsDxe: failed to load Boot0003 "Grub Bootloader" from Fv(7CB8BDC9-F8EB-4F34-AAEA-3EE4AF6516A1)/FvFile(B5AE312C-BC8A-43B1-9C62-EBB826DD5D07): Not
 Found


The "failed to load Grub" is what I expected. So this works OK. 
The ovmf-1.log file shows normal sequence of AmdSev boot. 


I tried the two options with the standard OvmfX64 package as well - same behaviour.


Do I need to build the OVMF file differently to use with -bios ?


-Dov


> 
> Daniel P. Berrangé (2):
> 
>   hw/i386: refactor logic for setting up SEV firmware
> 
>   hw/i386: support loading OVMF using -bios too
> 
> 
> 
>  hw/i386/pc_sysfw.c            | 24 +++---------------------
> 
>  hw/i386/pc_sysfw_ovmf-stubs.c | 10 ++++++++++
> 
>  hw/i386/pc_sysfw_ovmf.c       | 27 +++++++++++++++++++++++++++
> 
>  hw/i386/x86.c                 |  5 +++++
> 
>  include/hw/i386/pc.h          |  1 +
> 
>  5 files changed, 46 insertions(+), 21 deletions(-)
> 
> 
> 

Re: [PATCH 2/2] hw/i386: support loading OVMF using -bios too

[Michael S. Tsirkin]

On Sun, Jan 16, 2022 at 10:05:58PM +0100, Philippe Mathieu-Daudé wrote:
> On 14/1/22 19:07, Michael S. Tsirkin wrote:
> > On Thu, Jan 13, 2022 at 04:55:11PM +0000, Daniel P. Berrangé wrote:
> > > Traditionally the OVMF firmware has been loaded using the pflash
> > > mechanism. This is because it is usually provided as a pair of
> > > files, one read-only containing the code and one writable to
> > > provided persistence of non-volatile firmware variables.
> > > 
> > > The AMD SEV build of EDK2, however, is provided as a single
> > > file that contains only the code. This is intended to be used
> > > read-only and explicitly does not provide any ability for
> > > persistance of non-volatile firmware variables. While it is
> > > possible to configure this with the pflash mechanism, by only
> > > providing one of the 2 pflash blobs, conceptually it is a
> > > little strange to use pflash if there won't be any persistent
> > > data.
> 
> It certainly would be simpler to have a ROM for the CODE part.
> IIUC using CFI pflash allows the firmware to poll the underlying
> device size.
> 
> > > A stateless OVMF build can be loaded with -bios, however, QEMU
> > > does not currently initialize SEV in that scenario. This patch
> > > introduces the call needed for SEV initialization of the
> > > firmware.
> > > 
> > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > ---
> > >   hw/i386/x86.c | 5 +++++
> > >   1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> > > index b84840a1bb..c79d84936f 100644
> > > --- a/hw/i386/x86.c
> > > +++ b/hw/i386/x86.c
> > > @@ -45,6 +45,7 @@
> > >   #include "target/i386/cpu.h"
> > >   #include "hw/i386/topology.h"
> > >   #include "hw/i386/fw_cfg.h"
> > > +#include "hw/i386/pc.h"
> > >   #include "hw/intc/i8259.h"
> > >   #include "hw/rtc/mc146818rtc.h"
> > >   #include "target/i386/sev.h"
> > 
> > This builds fine because there's a stub in pc_sysfw_ovmf-stubs.c
> > 
> > The unfortunate thing about this however is that it's too easy to pull
> > in a PC dependency, and people building with CONFIG_PC will not notice
> > until it breaks for others.
> > 
> > Is it time we split pc.h further and had pc_sysfw_ovmf.h ?
> 
> While "pc*" is specific to the PC machines, "x86*" is shared between
> PC and microvm. "pc.h" must not be included in "x86.c". The shared
> method introduced in the previous patch becomes
> x86_system_ovmf_initialize_sev(). The dual pflash mechanism is proper
> to OVMF, so having this method in "x86.h" seems correct.
> 
> Phil.

Well.  E.g. pc_system_parse_ovmf_flash is defined in hw/i386/pc_sysfw_ovmf.c
and declared in pc.h. If you want to move all pc_sysfw_ovmf.c
declarations to x86.h that might be fine, but please do not
split declarations between multiple headers, that's messy.
And really, when in doubt do a separate header is a good rule.

-- 
MST

Re: [PATCH 0/2] Improved support for AMD SEV firmware loading

[Daniel P. Berrangé]

On Mon, Jan 17, 2022 at 09:34:30AM +0200, Dov Murik wrote:
> [+cc Tom, Brijesh, Ashish - see SEV-related changes in this series]
> 
> 
> On 13/01/2022 18:55, Daniel P. Berrangé wrote:
> > The AMD SEV build of EDK2 only emits a single file, intended to be
> > 
> > mapped readonly. There is explicitly no separate writable VARS
> > 
> > store for persisting non-volatile firmware variables.
> > 
> > 
> > 
> > This can be used with QEMU's traditional pflash configuration
> > 
> > mechanism by only populating pflash0, leaving pflash1 unconfigured.
> > 
> > Conceptually, however, it is odd to be using pflash at all when we
> > 
> > have no intention of supporting any writable variables. The -bios
> > 
> > option should be sufficient for any firmware that is exclusively
> > 
> > readonly code.
> > 
> > 
> > 
> > 
> > 
> > A second issue is that the firmware descriptor schema does not allow
> > 
> > for describing a firmware that uses pflash, without any associated
> > 
> > non-volatile storage.
> > 
> > 
> > 
> > In docs/interop/firmware.json
> > 
> > 
> > 
> >  'struct' : 'FirmwareMappingFlash',
> > 
> >   'data'   : { 'executable'     : 'FirmwareFlashFile',
> > 
> >                'nvram-template' : 'FirmwareFlashFile' } }
> > 
> > 
> > 
> > Notice that nvram-template is mandatory, and when consuming these
> > 
> > files libvirt will thus complain if the nvram-template field is
> > 
> > missing.
> > 
> > 
> > 
> > We could in theory make nvram-template optional in the schema and
> > 
> > then update libvirt to take account of it, but this feels dubious
> > 
> > when we have a perfectly good way of describing a firmware without
> > 
> > NVRAM, using 'FirmwareMappingMemory' which is intended to be used
> > 
> > with QEMU's -bios option.
> > 
> > 
> > 
> > 
> > 
> > A third issue is in libvirt, where again the code handling the
> > 
> > configuration of pflash supports two scenarios
> > 
> > 
> > 
> >  - A single pflash image, which is writable
> > 
> >  - A pair of pflash images, one writable one readonly
> > 
> > 
> > 
> > There is no support for a single read-only pflash image in libvirt
> > 
> > today.
> > 
> > 
> > 
> > 
> > 
> > This all points towards the fact that we should be using -bios
> > 
> > to load the AMD SEV firmware build of EDK.
> > 
> > 
> > 
> > The only thing preventing us doing that is that QEMU does not
> > 
> > initialize the SEV firmware when using -bios. That is fairly
> > 
> > easily solved, as done in this patch series.
> > 
> > 
> > 
> > For testing I've launched QEMU in in these scenarios
> > 
> > 
> > 
> >   - SEV guest using -bios and boot from HD
> > 
> >   - SEV guest using pflash and boot from HD
> > 
> >   - SEV-ES guest using -bios and direct kernel boot
> > 
> >   - SEV-ES guest using pflash and direct kernel boot
> > 
> > 
> > 
> > In all these cases I was able to validate the reported SEV
> > 
> > guest measurement.
> > 
> > 
> 
> 
> I'm having trouble testing this series (applied on top of master commit 69353c332c):
> it hangs with -bios but works OK with pflash:
> 
> Here's with -bios:
> 
> $ sudo /home/dmurik/git/qemu/build/qemu-system-x86_64 -enable-kvm \
>        -cpu host -machine q35 -smp 4 -m 2G \
>        -machine confidential-guest-support=sev0 \
>        -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1,policy=0x0 \
>        -bios /home/dmurik/git/edk2/Build/AmdSev/DEBUG_GCC5/FV/OVMF.fd \
>        -nographic \
>        -global isa-debugcon.iobase=0x402 -debugcon file:ovmf-1.log \
>        -monitor pty -trace 'enable=kvm_sev_*'
> 
> char device redirected to /dev/pts/14 (label compat_monitor0)
> kvm_sev_init
> kvm_sev_launch_start policy 0x0 session (nil) pdh (nil)
> kvm_sev_change_state uninit -> launch-update
> kvm_sev_launch_update_data addr 0x7f42e9bff010 len 0x400000
> kvm_sev_change_state launch-update -> launch-secret
> kvm_sev_launch_measurement data PF6n7+Vujx5sW8PC6iMRtHXfpXdJ4osbcfYvoknu7gg4ypMqs727NTzG86Ft8Llu
> kvm_sev_launch_finish
> kvm_sev_change_state launch-secret -> running
> 
> 
> Here it hangs. The ovmf-1.log file is empty.
> 
> Notice that kvm_sev_launch_update_data is called, so the new
> -bios behaviour is triggered correctly.  But the guest doesn't
> start running.

Sorry, it seems I failed to test this properly before rushing into
sending last week. I was too focused on making sure the SEV measurement
was working and not the rest.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/2] Improved support for AMD SEV firmware loading

[Brijesh Singh]

On 1/17/22 1:34 AM, Dov Murik wrote:
> [+cc Tom, Brijesh, Ashish - see SEV-related changes in this series]
>
>
> On 13/01/2022 18:55, Daniel P. Berrangé wrote:
>> The AMD SEV build of EDK2 only emits a single file, intended to be
>>
>> mapped readonly. There is explicitly no separate writable VARS
>>
>> store for persisting non-volatile firmware variables.
>>
>>
>>
>> This can be used with QEMU's traditional pflash configuration
>>
>> mechanism by only populating pflash0, leaving pflash1 unconfigured.
>>
>> Conceptually, however, it is odd to be using pflash at all when we
>>
>> have no intention of supporting any writable variables. The -bios
>>
>> option should be sufficient for any firmware that is exclusively
>>
>> readonly code.
>>
>>
>>
>>
>>
>> A second issue is that the firmware descriptor schema does not allow
>>
>> for describing a firmware that uses pflash, without any associated
>>
>> non-volatile storage.
>>
>>
>>
>> In docs/interop/firmware.json
>>
>>
>>
>>  'struct' : 'FirmwareMappingFlash',
>>
>>   'data'   : { 'executable'     : 'FirmwareFlashFile',
>>
>>                'nvram-template' : 'FirmwareFlashFile' } }
>>
>>
>>
>> Notice that nvram-template is mandatory, and when consuming these
>>
>> files libvirt will thus complain if the nvram-template field is
>>
>> missing.
>>
>>
>>
>> We could in theory make nvram-template optional in the schema and
>>
>> then update libvirt to take account of it, but this feels dubious
>>
>> when we have a perfectly good way of describing a firmware without
>>
>> NVRAM, using 'FirmwareMappingMemory' which is intended to be used
>>
>> with QEMU's -bios option.
>>
>>
>>
>>
>>
>> A third issue is in libvirt, where again the code handling the
>>
>> configuration of pflash supports two scenarios
>>
>>
>>
>>  - A single pflash image, which is writable
>>
>>  - A pair of pflash images, one writable one readonly
>>
>>
>>
>> There is no support for a single read-only pflash image in libvirt
>>
>> today.
>>
>>
>>
>>
>>
>> This all points towards the fact that we should be using -bios
>>
>> to load the AMD SEV firmware build of EDK.
>>
>>
>>
>> The only thing preventing us doing that is that QEMU does not
>>
>> initialize the SEV firmware when using -bios. That is fairly
>>
>> easily solved, as done in this patch series.
>>
>>
>>
>> For testing I've launched QEMU in in these scenarios
>>
>>
>>
>>   - SEV guest using -bios and boot from HD
>>
>>   - SEV guest using pflash and boot from HD
>>
>>   - SEV-ES guest using -bios and direct kernel boot
>>
>>   - SEV-ES guest using pflash and direct kernel boot
>>
>>
>>
>> In all these cases I was able to validate the reported SEV
>>
>> guest measurement.
>>
>>
>
> I'm having trouble testing this series (applied on top of master commit 69353c332c):
> it hangs with -bios but works OK with pflash:
>
> Here's with -bios:
>
> $ sudo /home/dmurik/git/qemu/build/qemu-system-x86_64 -enable-kvm \
>        -cpu host -machine q35 -smp 4 -m 2G \
>        -machine confidential-guest-support=sev0 \
>        -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1,policy=0x0 \
>        -bios /home/dmurik/git/edk2/Build/AmdSev/DEBUG_GCC5/FV/OVMF.fd \
>        -nographic \
>        -global isa-debugcon.iobase=0x402 -debugcon file:ovmf-1.log \
>        -monitor pty -trace 'enable=kvm_sev_*'
>
> char device redirected to /dev/pts/14 (label compat_monitor0)
> kvm_sev_init
> kvm_sev_launch_start policy 0x0 session (nil) pdh (nil)
> kvm_sev_change_state uninit -> launch-update
> kvm_sev_launch_update_data addr 0x7f42e9bff010 len 0x400000
> kvm_sev_change_state launch-update -> launch-secret
> kvm_sev_launch_measurement data PF6n7+Vujx5sW8PC6iMRtHXfpXdJ4osbcfYvoknu7gg4ypMqs727NTzG86Ft8Llu
> kvm_sev_launch_finish
> kvm_sev_change_state launch-secret -> running
>
>
> Here it hangs. The ovmf-1.log file is empty.
>
> Notice that kvm_sev_launch_update_data is called, so the new
> -bios behaviour is triggered correctly.  But the guest doesn't
> start running.

I have not looked at the patch detail yet but address looks wrong, it
looks like the hva 0x7f42e9bff010 end of the ROM. We need to encrypt the
entire ROM to boot, so I was hoping that hva will be 2MB aligned or a
page-aligned. You can enable the KVM trace to see if we are able to
enter and execute anything from guest.


> Here is the guest's state:
>
> (qemu) info registers
> EAX=0000606b EBX=00001268 ECX=0000440c EDX=008328d2
> ESI=000091e2 EDI=0000e9e3 EBP=0000a451 ESP=00009af0
> EIP=00003612 EFL=00000082 [--S----] CPL=0 II=0 A20=1 SMM=0 HLT=0
> ES =0000 00000000 0000ffff 00009300
> CS =a76e 000a76e0 0000ffff 00009b00
> SS =0000 00000000 0000ffff 00009300
> DS =0000 00000000 0000ffff 00009300
> FS =0000 00000000 0000ffff 00009300
> GS =0000 00000000 0000ffff 00009300
> LDT=0000 00000000 0000ffff 00008200
> TR =0000 00000000 0000ffff 00008b00
> GDT=     00000000 0000ffff
> IDT=     00000000 0000ffff
> CR0=60000010 CR2=00000000 CR3=00000000 CR4=00000000
> DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000
> DR6=00000000ffff0ff0 DR7=0000000000000400
> EFER=0000000000000000
> FCW=037f FSW=0000 [ST=0] FTW=00 MXCSR=00001f80
> ...
>
> (qemu) info sev
> handle: 1
> state: running
> build: 10
> api version: 0.23
> debug: on
> key-sharing: on
>
>
>
> If I try the same with pflash (instead of -bios), I get:
>
> # sudo /home/dmurik/git/qemu/build/qemu-system-x86_64 -enable-kvm \
>        -cpu host -machine q35 -smp 4 -m 2G \
>        -machine confidential-guest-support=sev0 \
>        -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1,policy=0x0 \
>        -drive if=pflash,format=raw,unit=0,file=/home/dmurik/git/edk2/Build/AmdSev/DEBUG_GCC5/FV/OVMF.fd,readonly=on \
>        -nographic \
>        -global isa-debugcon.iobase=0x402 -debugcon file:ovmf-1.log \
>        -monitor pty -trace 'enable=kvm_sev_*'
>
> char device redirected to /dev/pts/14 (label compat_monitor0)
> kvm_sev_init
> kvm_sev_launch_start policy 0x0 session (nil) pdh (nil)
> kvm_sev_change_state uninit -> launch-update
> kvm_sev_launch_update_data addr 0x7f0343000000 len 0x400000
> kvm_sev_change_state launch-update -> launch-secret
> kvm_sev_launch_measurement data esqzlr4xX2eEY92xsKEKL7FyKRDW7VYiyIb+aXS4S/ctON2s1xxwFjAKU7ImfULJ
> kvm_sev_launch_finish
> kvm_sev_change_state launch-secret -> running
> BdsDxe: failed to load Boot0003 "Grub Bootloader" from Fv(7CB8BDC9-F8EB-4F34-AAEA-3EE4AF6516A1)/FvFile(B5AE312C-BC8A-43B1-9C62-EBB826DD5D07): Not
>  Found
>
>
> The "failed to load Grub" is what I expected. So this works OK. 
> The ovmf-1.log file shows normal sequence of AmdSev boot. 
>
>
> I tried the two options with the standard OvmfX64 package as well - same behaviour.
>
>
> Do I need to build the OVMF file differently to use with -bios ?
>
>
> -Dov
>
>
>> Daniel P. Berrangé (2):
>>
>>   hw/i386: refactor logic for setting up SEV firmware
>>
>>   hw/i386: support loading OVMF using -bios too
>>
>>
>>
>>  hw/i386/pc_sysfw.c            | 24 +++---------------------
>>
>>  hw/i386/pc_sysfw_ovmf-stubs.c | 10 ++++++++++
>>
>>  hw/i386/pc_sysfw_ovmf.c       | 27 +++++++++++++++++++++++++++
>>
>>  hw/i386/x86.c                 |  5 +++++
>>
>>  include/hw/i386/pc.h          |  1 +
>>
>>  5 files changed, 46 insertions(+), 21 deletions(-)
>>
>>
>>

Re: [PATCH 0/2] Improved support for AMD SEV firmware loading

[Daniel P. Berrangé]

On Mon, Jan 17, 2022 at 06:12:50AM -0600, Brijesh Singh wrote:
> 
> On 1/17/22 1:34 AM, Dov Murik wrote:
> > [+cc Tom, Brijesh, Ashish - see SEV-related changes in this series]
> >
> >
> > On 13/01/2022 18:55, Daniel P. Berrangé wrote:
> >> The AMD SEV build of EDK2 only emits a single file, intended to be
> >>
> >> mapped readonly. There is explicitly no separate writable VARS
> >>
> >> store for persisting non-volatile firmware variables.
> >>
> >>
> >>
> >> This can be used with QEMU's traditional pflash configuration
> >>
> >> mechanism by only populating pflash0, leaving pflash1 unconfigured.
> >>
> >> Conceptually, however, it is odd to be using pflash at all when we
> >>
> >> have no intention of supporting any writable variables. The -bios
> >>
> >> option should be sufficient for any firmware that is exclusively
> >>
> >> readonly code.
> >>
> >>
> >>
> >>
> >>
> >> A second issue is that the firmware descriptor schema does not allow
> >>
> >> for describing a firmware that uses pflash, without any associated
> >>
> >> non-volatile storage.
> >>
> >>
> >>
> >> In docs/interop/firmware.json
> >>
> >>
> >>
> >>  'struct' : 'FirmwareMappingFlash',
> >>
> >>   'data'   : { 'executable'     : 'FirmwareFlashFile',
> >>
> >>                'nvram-template' : 'FirmwareFlashFile' } }
> >>
> >>
> >>
> >> Notice that nvram-template is mandatory, and when consuming these
> >>
> >> files libvirt will thus complain if the nvram-template field is
> >>
> >> missing.
> >>
> >>
> >>
> >> We could in theory make nvram-template optional in the schema and
> >>
> >> then update libvirt to take account of it, but this feels dubious
> >>
> >> when we have a perfectly good way of describing a firmware without
> >>
> >> NVRAM, using 'FirmwareMappingMemory' which is intended to be used
> >>
> >> with QEMU's -bios option.
> >>
> >>
> >>
> >>
> >>
> >> A third issue is in libvirt, where again the code handling the
> >>
> >> configuration of pflash supports two scenarios
> >>
> >>
> >>
> >>  - A single pflash image, which is writable
> >>
> >>  - A pair of pflash images, one writable one readonly
> >>
> >>
> >>
> >> There is no support for a single read-only pflash image in libvirt
> >>
> >> today.
> >>
> >>
> >>
> >>
> >>
> >> This all points towards the fact that we should be using -bios
> >>
> >> to load the AMD SEV firmware build of EDK.
> >>
> >>
> >>
> >> The only thing preventing us doing that is that QEMU does not
> >>
> >> initialize the SEV firmware when using -bios. That is fairly
> >>
> >> easily solved, as done in this patch series.
> >>
> >>
> >>
> >> For testing I've launched QEMU in in these scenarios
> >>
> >>
> >>
> >>   - SEV guest using -bios and boot from HD
> >>
> >>   - SEV guest using pflash and boot from HD
> >>
> >>   - SEV-ES guest using -bios and direct kernel boot
> >>
> >>   - SEV-ES guest using pflash and direct kernel boot
> >>
> >>
> >>
> >> In all these cases I was able to validate the reported SEV
> >>
> >> guest measurement.
> >>
> >>
> >
> > I'm having trouble testing this series (applied on top of master commit 69353c332c):
> > it hangs with -bios but works OK with pflash:
> >
> > Here's with -bios:
> >
> > $ sudo /home/dmurik/git/qemu/build/qemu-system-x86_64 -enable-kvm \
> >        -cpu host -machine q35 -smp 4 -m 2G \
> >        -machine confidential-guest-support=sev0 \
> >        -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1,policy=0x0 \
> >        -bios /home/dmurik/git/edk2/Build/AmdSev/DEBUG_GCC5/FV/OVMF.fd \
> >        -nographic \
> >        -global isa-debugcon.iobase=0x402 -debugcon file:ovmf-1.log \
> >        -monitor pty -trace 'enable=kvm_sev_*'
> >
> > char device redirected to /dev/pts/14 (label compat_monitor0)
> > kvm_sev_init
> > kvm_sev_launch_start policy 0x0 session (nil) pdh (nil)
> > kvm_sev_change_state uninit -> launch-update
> > kvm_sev_launch_update_data addr 0x7f42e9bff010 len 0x400000
> > kvm_sev_change_state launch-update -> launch-secret
> > kvm_sev_launch_measurement data PF6n7+Vujx5sW8PC6iMRtHXfpXdJ4osbcfYvoknu7gg4ypMqs727NTzG86Ft8Llu
> > kvm_sev_launch_finish
> > kvm_sev_change_state launch-secret -> running
> >
> >
> > Here it hangs. The ovmf-1.log file is empty.
> >
> > Notice that kvm_sev_launch_update_data is called, so the new
> > -bios behaviour is triggered correctly.  But the guest doesn't
> > start running.
> 
> I have not looked at the patch detail yet but address looks wrong, it
> looks like the hva 0x7f42e9bff010 end of the ROM. We need to encrypt the
> entire ROM to boot, so I was hoping that hva will be 2MB aligned or a
> page-aligned. You can enable the KVM trace to see if we are able to
> enter and execute anything from guest.

On further investigation I think the use of -bios is probably
doomed. The way QEMU implements -bios is to load the firmware
as a ROM, then later when a CPU reset is triggered at the end
of machine construction, the ROM is copied into the RAM area.

For SEV we need the encrypted firmware in the RAM area, but
my patch here is applying the encryption in the ROM. QEMU
copies this encrypted code across in the CPU reset handler
but this can't work because IIUC that invalidates the
initialization vector preventing correct decryption.

We can't delay the encryption step until after the ROM
copying because by that time the SEV initialization sequence
has gone too far along.

I did a gross hack which disabled use of the ROM and ROM
copying and directly loaded the firmware into the RAM area
and that successfully ran, but it is too much of a hack to
consider proposing as a real patch.

I'm going to go back to just looking at how we can make
support for pflash-without-NVRAM a first class citizen
in QEMU firmware descriptors / libvirt, since that is
known to work without hacks.

So ignore this flawed series henceforth.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|