SEV-ES guest support
cover SEV-ES guest support
From: Tom Lendacky <[email protected]>
This patch series provides support for launching an SEV-ES guest.
Secure Encrypted Virtualization - Encrypted State (SEV-ES) expands on the SEV support to protect the guest register state from the hypervisor. See "AMD64 Architecture Programmer's Manual Volume 2: System Programming", section "15.35 Encrypted State (SEV-ES)" [1].
In order to allow a hypervisor to perform functions on behalf of a guest, there is architectural support for notifying a guest's operating system when certain types of VMEXITs are about to occur. This allows the guest to selectively share information with the hypervisor to satisfy the requested function. The notification is performed using a new exception, the VMM Communication exception (#VC). The information is shared through the Guest-Hypervisor Communication Block (GHCB) using the VMGEXIT instruction. The GHCB format and the protocol for using it is documented in "SEV-ES Guest-Hypervisor Communication Block Standardization" [2].
The main areas of the Qemu code that are updated to support SEV-ES are around the SEV guest launch process and AP booting in order to support booting multiple vCPUs.
There are no new command line switches required. Instead, the desire for SEV-ES is presented using the SEV policy object. Bit 2 of the SEV policy object indicates that SEV-ES is required.
The SEV launch process is updated in two ways. The first is that a the KVM_SEV_ES_INIT ioctl is used to initialize the guest instead of the standard KVM_SEV_INIT ioctl. The second is that before the SEV launch measurement is calculated, the LAUNCH_UPDATE_VMSA SEV API is invoked for each vCPU that Qemu has created. Once the LAUNCH_UPDATE_VMSA API has been invoked, no direct changes to the guest register state can be made.
AP booting poses some interesting challenges. The INIT-SIPI-SIPI sequence is typically used to boot the APs. However, the hypervisor is not allowed to update the guest registers. For the APs, the reset vector must be known in advance. An OVMF method to provide a known reset vector address exists by providing an SEV information block, identified by UUID, near the end of the firmware [3]. OVMF will program the jump to the actual reset vector in this area of memory. Since the memory location is known in advance, an AP can be created with the known reset vector address as its starting CS:IP. The GHCB document [2] talks about how SMP booting under SEV-ES is performed.
[1] https://www.amd.com/system/files/TechDocs/24593.pdf
[2] https://developer.amd.com/wp-content/resources/56421.pdf
[3] 30937f2f98c4 ("OvmfPkg: Use the SEV-ES work area for the SEV-ES AP reset vector")
https://github.com/tianocore/edk2/commit/30937f2f98c42496f2f143fe8374ae7f7e684847
---
These patches are based on commit:d0ed6a69d3 ("Update version for v5.1.0 release")
(I tried basing on the latest Qemu commit, but I was having build issues that level)
A version of the tree can be found at:
https://github.com/AMDESE/qemu/tree/sev-es-v9
Tom Lendacky (4):
sev/i386: Add initial support for SEV-ES sev/i386: Allow AP booting under SEV-ES sev/i386: Don't allow a system reset under an SEV-ES guest sev/i386: Enable an SEV-ES guest based on SEV policy
accel/kvm/kvm-all.c | 68 ++++++++++++++++++++++++++++ accel/stubs/kvm-stub.c | 5 +++ hw/i386/pc_sysfw.c | 10 ++++- include/sysemu/cpus.h | 2 + include/sysemu/hw_accel.h | 4 ++ include/sysemu/kvm.h | 18 ++++++++ include/sysemu/sev.h | 2 + softmmu/cpus.c | 5 +++ softmmu/vl.c | 5 ++- target/i386/cpu.c | 1 + target/i386/kvm.c | 2 + target/i386/sev-stub.c | 5 +++ target/i386/sev.c | 95 ++++++++++++++++++++++++++++++++++++++- target/i386/sev_i386.h | 1 + 14 files changed, 219 insertions(+), 4 deletions(-)
Signature · 1 line
2.28.0
PATCH 1/4 sev/i386: Add initial support for SEV-ES
From: Tom Lendacky <[email protected]>
Provide initial support for SEV-ES. This includes creating a function to indicate the guest is an SEV-ES guest (which will return false until all support is in place), performing the proper SEV initialization and ensuring that the guest CPU state is measured as part of the launch.
--- target/i386/cpu.c | 1 + target/i386/sev-stub.c | 5 +++++ target/i386/sev.c | 46 ++++++++++++++++++++++++++++++++++++++++-- target/i386/sev_i386.h | 1 + 4 files changed, 51 insertions(+), 2 deletions(-)
target/i386/cpu.c +1 -0
@@ -5969,6 +5969,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,break ;case 0x8000001F : *eax =sev_enabled () ?0x2 :0 ; + *eax |=sev_es_enabled () ?0x8 :0 ; *ebx =sev_get_cbit_position (); *ebx |=sev_get_reduced_phys_bits () <<6 ; *ecx =0 ;
target/i386/sev-stub.c +5 -0
@@ -49,3 +49,8 @@ SevCapability *sev_get_capabilities(Error **errp)error_setg (errp,"SEV is not available in this QEMU" );return NULL ; } + +bool sev_es_enabled (void ) +{ +return false; +}
target/i386/sev.c +44 -2
77 lines not on this page · raw message
target/i386/sev_i386.h +1 -0
@@ -29,6 +29,7 @@#define SEV_POLICY_SEV 0x20extern bool sev_enabled (void ); +extern bool sev_es_enabled (void );extern uint64_t sev_get_me_mask (void );extern SevInfo *sev_get_info (void );extern uint32_t sev_get_cbit_position (void );
Signature · 1 line
2.28.0
Re: [PATCH 1/4] sev/i386: Add initial support for SEV-ES
Quoted 10 lines · Tom Lendacky
From: Tom Lendacky <[email protected]>
Provide initial support for SEV-ES. This includes creating a function to indicate the guest is an SEV-ES guest (which will return false until all support is in place), performing the proper SEV initialization and ensuring that the guest CPU state is measured as part of the launch.
Co-developed-by Jiri Slaby <[email protected]>Signed-off-by Jiri Slaby <[email protected]>Signed-off-by Tom Lendacky <[email protected]>
Hi Tom!
Overall I think the patch set looks good. I mainly just have 1 question regarding some error handling and a couple of checkpatch related messages.
Quoted 24 lines
--- target/i386/cpu.c | 1 + target/i386/sev-stub.c | 5 +++++ target/i386/sev.c | 46 ++++++++++++++++++++++++++++++++++++++++-- target/i386/sev_i386.h | 1 + 4 files changed, 51 insertions(+), 2 deletions(-)target/i386/cpu.c +1
-0@@ -5969,6 +5969,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,break ;case 0x8000001F : *eax =sev_enabled () ?0x2 :0 ; + *eax |=sev_es_enabled () ?0x8 :0 ; *ebx =sev_get_cbit_position (); *ebx |=sev_get_reduced_phys_bits () <<6 ; *ecx =0 ;target/i386/sev-stub.c +0
-0
target/i386/sev-stub.c SevCapability *sev_get_capabilities(Error **errp) · line 53
Quoted 5 lines
target/i386/sev-stub.c +2
-0@@ -49,3 +49,8 @@ SevCapability *sev_get_capabilities(Error **errp)error_setg (errp,"SEV is not available in this QEMU" );return NULL ; } + +bool sev_es_enabled (void )
I don't think this bothers checkpatch, but it'd be consistent with the rest of your series if this function put the return type on the line above.
Quoted 44 lines
target/i386/sev-stub.c +3
-0+{ +return false; +}target/i386/sev.c +22
-0@@ -359,6 +359,12 @@ sev_enabled(void)return !!sev_guest; } +bool +sev_es_enabled (void ) +{ +return false; +} +uint64_t sev_get_me_mask (void ) {@@ -578,6 +584,22 @@ sev_launch_update_data(SevGuestState *sev, uint8_t *addr, uint64_t len)return ret; } +static int +sev_launch_update_vmsa (SevGuestState *sev) +{ +int ret, fw_error; + + ret =sev_ioctl (sev->sev_fd,KVM_SEV_LAUNCH_UPDATE_VMSA ,NULL , &fw_error); +if (ret) { +error_report ("%s: LAUNCH_UPDATE_VMSA ret=%d fw_error=%d '%s'" , + __func__, ret, fw_error,fw_error_to_str (fw_error)); + goto err; + } + +err: +return ret; +} +static void sev_launch_get_measure (Notifier *notifier,void *unused) {
target/i386/sev.c sev_launch_get_measure(Notifier *notifier, void *unused) · line 619
Quoted 8 lines
target/i386/sev.c +5
-0@@ -590,6 +612,14 @@ sev_launch_get_measure(Notifier *notifier, void *unused)return ; } +if (sev_es_enabled ()) { +/* measure all the VM save areas before getting launch_measure */ + ret =sev_launch_update_vmsa (sev); +if (ret) { +exit (1 );
Disclaimer: I'm still learning the QEMU source code, sorry if this comes across as naive.
Is exit() what we want here? I was looking around the rest of the source code and unfortunately the machine_init_done_notifiers mechanism doesn't allow for a return value to indicate an error, so I'm wondering if there's a more appropriate place in the initialization code to handle these fallible operations and if so, propagate the error down. This way if there are other resources that need to be cleaned up on the way out, they can be. Thoughts?
Quoted 50 lines
target/i386/sev.c +17
-2+ } + } + measurement = g_new0(struct kvm_sev_launch_measure, 1);/* query the measurement blob length */ @@ -684,7 +714,7 @@ sev_guest_init(const char *id) {SevGuestState *sev;char *devname; -int ret, fw_error; +int ret, fw_error, cmd;uint32_t ebx;uint32_t host_cbitpos;struct sev_user_data_status status = {};@@ -745,8 +775,20 @@ sev_guest_init(const char *id) sev->api_major = status.api_major; sev->api_minor = status.api_minor; +if (sev_es_enabled ()) { +if (!(status.flags &SEV_STATUS_FLAGS_CONFIG_ES )) { +error_report ("%s: guest policy requires SEV-ES, but " +"host SEV-ES support unavailable" , + __func__); + goto err; + } + cmd =KVM_SEV_ES_INIT ; + }else { + cmd =KVM_SEV_INIT ; + } +trace_kvm_sev_init (); - ret =sev_ioctl (sev->sev_fd,KVM_SEV_INIT ,NULL , &fw_error); + ret =sev_ioctl (sev->sev_fd, cmd,NULL , &fw_error);if (ret) {error_report ("%s: failed to initialize ret=%d fw_error=%d '%s'" , __func__, ret, fw_error,fw_error_to_str (fw_error));target/i386/sev_i386.h +1
-0@@ -29,6 +29,7 @@#define SEV_POLICY_SEV 0x20extern bool sev_enabled (void ); +extern bool sev_es_enabled (void );extern uint64_t sev_get_me_mask (void );extern SevInfo *sev_get_info (void );extern uint32_t sev_get_cbit_position (void );
Re: [PATCH 1/4] sev/i386: Add initial support for SEV-ES
Quoted 8 lines · Connor Kuehl
Quoted 7 lines · Tom Lendacky
From: Tom Lendacky <[email protected]>
Provide initial support for SEV-ES. This includes creating a function to indicate the guest is an SEV-ES guest (which will return false until all support is in place), performing the proper SEV initialization and ensuring that the guest CPU state is measured as part of the launch.
Quoted 5 lines
Quoted 3 lines
Co-developed-by Jiri Slaby <[email protected]>Signed-off-by Jiri Slaby <[email protected]>Signed-off-by Tom Lendacky <[email protected]>Hi Tom!
Hi Connor,
Quoted 2 lines
Overall I think the patch set looks good. I mainly just have 1 question regarding some error handling and a couple of checkpatch related messages.
Ugh, I was positive I ran checkpatch, but obviously I didn't.
Quoted 25 lines
Quoted 25 lines
---
target/i386/cpu.c | 1 +
target/i386/sev-stub.c| 5 +++++
target/i386/sev.c | 46 ++++++++++++++++++++++++++++++++++++++++--target/i386/sev_i386.h| 1 +4 files changed, 51 insertions(+), 2 deletions(-)target/i386/cpu.c +0
-0@@ -5969,6 +5969,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, break;case
0x8000001F:
*eax =sev_enabled()?0x2: 0;
+ *eax |=sev_es_enabled()?0x8: 0; *ebx =sev_get_cbit_position(); *ebx |=sev_get_reduced_phys_bits()<< 6; *ecx = 0;target/i386/sev-stub.c +0
-0
Quoted 8 lines
target/i386/sev-stub.c SevCapability *sev_get_capabilities(Error **errp) · line 53
Quoted 6 lines
target/i386/sev-stub.c +2
-0@@ -49,3 +49,8 @@ SevCapability *sev_get_capabilities(Error **errp) error_setg(errp, "SEV is not available in this QEMU"); return NULL; } + +bool sev_es_enabled (void )I don't think this bothers checkpatch, but it'd be consistent with the rest of your series if this function put the return type on the line above.
I was being consistent with the file that it is in where all the other functions are defined this way.
Quoted 56 lines
Quoted 45 lines
target/i386/sev-stub.c +3
-0+{ + return false; +}target/i386/sev.c +6
-0@@ -359,6 +359,12 @@ sev_enabled(void) return !!sev_guest; } +bool +sev_es_enabled (void ) +{ + return false; +} +
uint64_t
sev_get_me_mask(void)
{target/i386/sev.c +16
-0@@ -578,6 +584,22 @@ sev_launch_update_data(SevGuestState *sev, uint8_t *addr, uint64_t len) return ret; } +static int +sev_launch_update_vmsa (SevGuestState *sev) +{ + int ret, fw_error; + + ret = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_UPDATE_VMSA, NULL, &fw_error); + if (ret) { + error_report("%s: LAUNCH_UPDATE_VMSA ret=%d fw_error=%d '%s'", + __func__, ret, fw_error, fw_error_to_str(fw_error)); + goto err; + } + +err: + return ret; +} +static void
sev_launch_get_measure(Notifier *notifier, void *unused){target/i386/sev.c sev_launch_get_measure(Notifier *notifier, void · line 619
Quoted 8 lines
target/i386/sev.c +5
-0@@ -590,6 +612,14 @@ sev_launch_get_measure(Notifier *notifier, void *unused) return; } + if (sev_es_enabled()) { + /* measure all the VM save areas before getting launch_measure */ + ret = sev_launch_update_vmsa(sev); + if (ret) { + exit(1);Disclaimer: I'm still learning the QEMU source code, sorry if this comes across as naive.
Quoted 1 line
Is
exit()what we want here? I was looking around the rest of the source code and unfortunately themachine_init_done_notifiersmechanism doesn't allow for a return value to indicate an error, so I'm wondering if there's a more appropriate place in the initialization code to handle these fallible operations and if so, propagate the error down. This way if there are other resources that need to be cleaned up on the way out, they can be. Thoughts?
I was following the existing method of terminating that is being performed in this file. I'll see if others have an idea about how to handle these types of errors, which could probably be addressed as a separate patch series.
Thanks,
Tom
Quoted 49 lines
Quoted 48 lines
target/i386/sev.c +17
-2+ } + } + measurement = g_new0(struct kvm_sev_launch_measure, 1); /* query the measurement blob length */@@ -684,7 +714,7 @@ sev_guest_init(const char *id) { SevGuestState *sev; char *devname; - int ret, fw_error; + int ret, fw_error, cmd; uint32_t ebx; uint32_t host_cbitpos; struct sev_user_data_status status = {};@@ -745,8 +775,20 @@ sev_guest_init(const char *id) sev->api_major = status.api_major; sev->api_minor = status.api_minor; + if (sev_es_enabled()) { + if (!(status.flags & SEV_STATUS_FLAGS_CONFIG_ES)) { + error_report("%s: guest policy requires SEV-ES, but " + "host SEV-ES support unavailable", + __func__); + goto err; + } + cmd = KVM_SEV_ES_INIT; + } else { + cmd = KVM_SEV_INIT; + } + trace_kvm_sev_init(); - ret = sev_ioctl(sev->sev_fd, KVM_SEV_INIT, NULL, &fw_error); + ret = sev_ioctl(sev->sev_fd, cmd, NULL, &fw_error); if (ret) { error_report("%s: failed to initialize ret=%d fw_error=%d '%s'", __func__, ret, fw_error, fw_error_to_str(fw_error));target/i386/sev_i386.h +1
-0@@ -29,6 +29,7 @@ #define SEV_POLICY_SEV 0x20 extern bool sev_enabled(void); +extern bool sev_es_enabled (void ); extern uint64_t sev_get_me_mask(void); extern SevInfo *sev_get_info(void); extern uint32_t sev_get_cbit_position(void);
PATCH 2/4 sev/i386: Allow AP booting under SEV-ES
In reply to Tom LendackyFrom: Tom Lendacky <[email protected]>
When SEV-ES is enabled, it is not possible modify the guests register state after it has been initially created, encrypted and measured.
Normally, an INIT-SIPI-SIPI request is used to boot the AP. However, the hypervisor cannot emulate this because it cannot update the AP register state. For the very first boot by an AP, the reset vector CS segment value and the EIP value must be programmed before the register has been encrypted and measured.
--- accel/kvm/kvm-all.c | 60 ++++++++++++++++++++++++++++++++++++++++++ accel/stubs/kvm-stub.c | 5 ++++ hw/i386/pc_sysfw.c | 10 ++++++- include/sysemu/kvm.h | 16 +++++++++++ include/sysemu/sev.h | 2 ++ target/i386/kvm.c | 2 ++ target/i386/sev.c | 47 +++++++++++++++++++++++++++++++++ 7 files changed, 141 insertions(+), 1 deletion(-)
accel/kvm/kvm-all.c +60 -0
@@ -39,6 +39,7 @@#include "qemu/main-loop.h" #include "trace.h" #include "hw/irq.h" +#include "sysemu/kvm.h" #include "sysemu/sev.h" #include "qapi/visitor.h" #include "qapi/qapi-types-common.h"
@@ -120,6 +121,11 @@ struct KVMState/* memory encryption */ void *memcrypt_handle;int (*memcrypt_encrypt_data)(void *handle,uint8_t *ptr,uint64_t len); +int (*memcrypt_save_reset_vector)(void *handle,void *flash_ptr,uint64_t flash_size,uint32_t *addr); + +unsigned int reset_cs; +unsigned int reset_ip; +bool reset_valid;/* For "info mtree -f" to tell if an MR is registered in KVM */ int nr_as;
@@ -239,6 +245,59 @@ int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len)return 1 ; } +void kvm_memcrypt_set_reset_vector (CPUState *cpu) +{ +X86CPU *x86; +CPUX86State *env; + +/* Only update if we have valid reset information */ +if (!kvm_state->reset_valid) +return ; + +/* Do not update the BSP reset state */ +if (cpu->cpu_index ==0 ) +return ; + + x86 =X86_CPU (cpu); + env = &x86->env; + +cpu_x86_load_seg_cache (env,R_CS ,0xf000 , kvm_state->reset_cs,0xffff , +DESC_P_MASK |DESC_S_MASK |DESC_CS_MASK | +DESC_R_MASK |DESC_A_MASK ); + + env->eip = kvm_state->reset_ip; +} + +int kvm_memcrypt_save_reset_vector (void *flash_ptr,uint64_t flash_size) +{ +CPUState *cpu; +uint32_t addr; +int ret; + +if (kvm_memcrypt_enabled () && + kvm_state->memcrypt_save_reset_vector) { + + addr =0 ; + ret = kvm_state->memcrypt_save_reset_vector (kvm_state->memcrypt_handle, + flash_ptr, flash_size, &addr); +if (ret) { +return ret; + } + +if (addr) { + kvm_state->reset_cs = addr &0xffff0000 ; + kvm_state->reset_ip = addr &0x0000ffff ; + kvm_state->reset_valid = true; + +CPU_FOREACH (cpu) { +kvm_memcrypt_set_reset_vector (cpu); + } + } + } + +return 0 ; +} +/* Called with KVMMemoryListener.slots_lock held */ static KVMSlot *kvm_get_free_slot (KVMMemoryListener *kml) {
@@ -2193,6 +2252,7 @@ static int kvm_init(MachineState *ms) } kvm_state->memcrypt_encrypt_data = sev_encrypt_data; + kvm_state->memcrypt_save_reset_vector = sev_es_save_reset_vector; } ret =kvm_arch_init (ms, s);
accel/stubs/kvm-stub.c +5 -0
@@ -114,6 +114,11 @@ int kvm_memcrypt_encrypt_data(uint8_t *ptr, uint64_t len)return 1 ; } +int kvm_memcrypt_save_reset_vector (void *flash_ptr,uint64_t flash_size) +{ +return -ENOSYS ; +} +#ifndef CONFIG_USER_ONLY int kvm_irqchip_add_msi_route (KVMState *s,int vector,PCIDevice *dev) {
hw/i386/pc_sysfw.c +9 -1
@@ -156,7 +156,8 @@ static void pc_system_flash_map(PCMachineState *pcms,PFlashCFI01 *system_flash;MemoryRegion *flash_mem;void *flash_ptr; -int ret, flash_size; +uint64_t flash_size; +int ret;assert (PC_MACHINE_GET_CLASS (pcms)->pci_enabled);
@@ -204,6 +205,13 @@ static void pc_system_flash_map(PCMachineState *pcms,if (kvm_memcrypt_enabled ()) { flash_ptr =memory_region_get_ram_ptr (flash_mem); flash_size =memory_region_size (flash_mem); + + ret =kvm_memcrypt_save_reset_vector (flash_ptr, flash_size); +if (ret) { +error_report ("failed to locate and/or save reset vector" ); +exit (1 ); + } + ret =kvm_memcrypt_encrypt_data (flash_ptr, flash_size);if (ret) {error_report ("failed to encrypt pflash rom" );
include/sysemu/kvm.h +16 -0
@@ -247,6 +247,22 @@ bool kvm_memcrypt_enabled(void);*/ int kvm_memcrypt_encrypt_data (uint8_t *ptr,uint64_t len); +/** +* kvm_memcrypt_set_reset_vector - sets the CS/IP value for the AP if SEV-ES +* is active. +*/ +void kvm_memcrypt_set_reset_vector (CPUState *cpu); + +/** +* kvm_memcrypt_save_reset_vector - locates and saves the reset vector to be +* used as the initial CS/IP value for APs +* if SEV-ES is active. +* +* Return: 1 SEV-ES is active and failed to locate a valid reset vector +* 0 SEV-ES is not active or successfully located and saved the +* reset vector address +*/ +int kvm_memcrypt_save_reset_vector (void *flash_prt,uint64_t flash_size);#ifdef NEED_CPU_H #include "cpu.h"
include/sysemu/sev.h +2 -0
@@ -18,4 +18,6 @@void *sev_guest_init (const char *id);int sev_encrypt_data (void *handle,uint8_t *ptr,uint64_t len); +int sev_es_save_reset_vector (void *handle,void *flash_ptr,uint64_t flash_size,uint32_t *addr); +#endif
target/i386/kvm.c +2 -0
@@ -1912,6 +1912,8 @@ void kvm_arch_reset_vcpu(X86CPU *cpu) }/* enabled by default */ env->poll_control_msr =1 ; + +kvm_memcrypt_set_reset_vector (CPU (cpu)); }void kvm_arch_do_init_vcpu (X86CPU *cpu)
target/i386/sev.c +47 -0
59 lines not on this page · raw message
Signature · 1 line
2.28.0
Re: [PATCH 2/4] sev/i386: Allow AP booting under SEV-ES
Quoted 12 lines · Tom Lendacky
From: Tom Lendacky <[email protected]>
When SEV-ES is enabled, it is not possible modify the guests register state after it has been initially created, encrypted and measured.
Normally, an INIT-SIPI-SIPI request is used to boot the AP. However, the hypervisor cannot emulate this because it cannot update the AP register state. For the very first boot by an AP, the reset vector CS segment value and the EIP value must be programmed before the register has been encrypted and measured.
Signed-off-by Tom Lendacky <[email protected]>
Quoted 9 lines
--- accel/kvm/kvm-all.c | 60 ++++++++++++++++++++++++++++++++++++++++++ accel/stubs/kvm-stub.c | 5 ++++ hw/i386/pc_sysfw.c | 10 ++++++- include/sysemu/kvm.h | 16 +++++++++++ include/sysemu/sev.h | 2 ++ target/i386/kvm.c | 2 ++ target/i386/sev.c | 47 +++++++++++++++++++++++++++++++++ 7 files changed, 141 insertions(+), 1 deletion(-)
Just a heads-up: ./scripts/checkpatch.pl does report a couple of style errors. I've seen other series go by where maintainers didn't mind the line length errors, but there are a couple that have to do with braces around if-statement contents that may need to be addressed.
Re: [PATCH 2/4] sev/i386: Allow AP booting under SEV-ES
Quoted 23 lines · Connor Kuehl
Quoted 20 lines · Tom Lendacky
From: Tom Lendacky <[email protected]>
When SEV-ES is enabled, it is not possible modify the guests register state after it has been initially created, encrypted and measured.
Normally, an INIT-SIPI-SIPI request is used to boot the AP. However, the hypervisor cannot emulate this because it cannot update the AP register state. For the very first boot by an AP, the reset vector CS segment value and the EIP value must be programmed before the register has been encrypted and measured.
Signed-off-by Tom Lendacky <[email protected]>---
accel/kvm/kvm-all.c | 60 ++++++++++++++++++++++++++++++++++++++++++accel/stubs/kvm-stub.c| 5 ++++
hw/i386/pc_sysfw.c | 10 ++++++-
include/sysemu/kvm.h | 16 +++++++++++ include/sysemu/sev.h | 2 ++
target/i386/kvm.c | 2 ++
target/i386/sev.c | 47 +++++++++++++++++++++++++++++++++Quoted 1 line
7 files changed, 141 insertions(+), 1 deletion(-)
Quoted 1 line
Just a heads-up:
./scripts/checkpatch.pldoes report a couple of style errors. I've seen other series go by where maintainers didn't mind the line length errors, but there are a couple that have to do with braces around if-statement contents that may need to be addressed.
Yup, I'll run checkpatch and make the necessary changes.
Thanks,
Tom
Quoted 1 line
PATCH 3/4 sev/i386: Don't allow a system reset under an SEV-ES guest
In reply to Tom LendackyFrom: Tom Lendacky <[email protected]>
An SEV-ES guest does not allow register state to be altered once it has been measured. When a SEV-ES guest issues a reboot command, Qemu will reset the vCPU state and resume the guest. This will cause failures under SEV-ES, so prevent that from occurring.
--- accel/kvm/kvm-all.c | 8 ++++++++ include/sysemu/cpus.h | 2 ++ include/sysemu/hw_accel.h | 4 ++++ include/sysemu/kvm.h | 2 ++ softmmu/cpus.c | 5 +++++ softmmu/vl.c | 5 ++++- 6 files changed, 25 insertions(+), 1 deletion(-)
accel/kvm/kvm-all.c +8 -0
@@ -2384,6 +2384,14 @@ void kvm_flush_coalesced_mmio_buffer(void) s->coalesced_flush_in_progress = false; } +bool kvm_cpu_check_resettable (void ) +{ +/* If we have a valid reset vector override, then SEV-ES is active +* and the CPU can't be reset. +*/ +return !kvm_state->reset_valid; +} +static void do_kvm_cpu_synchronize_state (CPUState *cpu,run_on_cpu_data arg) {if (!cpu->vcpu_dirty) {
include/sysemu/cpus.h +2 -0
@@ -24,6 +24,8 @@ void dump_drift_info(void);void qemu_cpu_kick_self (void );void qemu_timer_notify_cb (void *opaque,QEMUClockType type); +bool cpu_is_resettable (void ); +void cpu_synchronize_all_states (void );void cpu_synchronize_all_post_reset (void );void cpu_synchronize_all_post_init (void );
include/sysemu/hw_accel.h +4 -0
@@ -17,6 +17,10 @@#include "sysemu/hvf.h" #include "sysemu/whpx.h" +static inline bool cpu_check_resettable (void ) +{ +return kvm_enabled () ?kvm_cpu_check_resettable () : true; +}static inline void cpu_synchronize_state (CPUState *cpu) {if (kvm_enabled ()) {
include/sysemu/kvm.h +2 -0
@@ -494,6 +494,8 @@ int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr,#endif /* NEED_CPU_H */ +bool kvm_cpu_check_resettable (void ); +void kvm_cpu_synchronize_state (CPUState *cpu);void kvm_cpu_synchronize_post_reset (CPUState *cpu);void kvm_cpu_synchronize_post_init (CPUState *cpu);
softmmu/cpus.c +5 -0
@@ -927,6 +927,11 @@ void hw_error(const char *fmt, ...)abort (); } +bool cpu_is_resettable (void ) +{ +return cpu_check_resettable (); +} +void cpu_synchronize_all_states (void ) {CPUState *cpu;
softmmu/vl.c +4 -1
@@ -1475,7 +1475,10 @@ void qemu_system_guest_crashloaded(GuestPanicInformation *info)void qemu_system_reset_request (ShutdownCause reason) { -if (no_reboot && reason !=SHUTDOWN_CAUSE_SUBSYSTEM_RESET ) { +if (!cpu_is_resettable ()) { +error_report ("cpus are not resettable, terminating" ); + shutdown_requested = reason; + }else if (no_reboot && reason !=SHUTDOWN_CAUSE_SUBSYSTEM_RESET ) { shutdown_requested = reason; }else { reset_requested = reason;
Signature · 1 line
2.28.0
Re: [PATCH 3/4] sev/i386: Don't allow a system reset under an SEV-ES guest
Quoted 54 lines · Tom Lendacky
From: Tom Lendacky <[email protected]>
An SEV-ES guest does not allow register state to be altered once it has been measured. When a SEV-ES guest issues a reboot command, Qemu will reset the vCPU state and resume the guest. This will cause failures under SEV-ES, so prevent that from occurring.
Signed-off-by Tom Lendacky <[email protected]>--- accel/kvm/kvm-all.c | 8 ++++++++ include/sysemu/cpus.h | 2 ++ include/sysemu/hw_accel.h | 4 ++++ include/sysemu/kvm.h | 2 ++ softmmu/cpus.c | 5 +++++ softmmu/vl.c | 5 ++++- 6 files changed, 25 insertions(+), 1 deletion(-)accel/kvm/kvm-all.c +8
-0@@ -2384,6 +2384,14 @@ void kvm_flush_coalesced_mmio_buffer(void) s->coalesced_flush_in_progress = false; } +bool kvm_cpu_check_resettable (void ) +{ +/* If we have a valid reset vector override, then SEV-ES is active +* and the CPU can't be reset. +*/ +return !kvm_state->reset_valid; +} +static void do_kvm_cpu_synchronize_state (CPUState *cpu,run_on_cpu_data arg) {if (!cpu->vcpu_dirty) {include/sysemu/cpus.h +2
-0@@ -24,6 +24,8 @@ void dump_drift_info(void);void qemu_cpu_kick_self (void );void qemu_timer_notify_cb (void *opaque,QEMUClockType type); +bool cpu_is_resettable (void ); +void cpu_synchronize_all_states (void );void cpu_synchronize_all_post_reset (void );void cpu_synchronize_all_post_init (void );include/sysemu/hw_accel.h +0
-0
include/sysemu/hw_accel.h line 23
Quoted 7 lines
include/sysemu/hw_accel.h +4
-0@@ -17,6 +17,10 @@#include "sysemu/hvf.h" #include "sysemu/whpx.h" +static inline bool cpu_check_resettable (void ) +{ +return kvm_enabled () ?kvm_cpu_check_resettable () : true; +}
There's a missing newline here that would separate the closing brace from the function below this one.
Quoted 49 lines
include/sysemu/hw_accel.h +0
-0static inline void cpu_synchronize_state (CPUState *cpu) {if (kvm_enabled ()) {include/sysemu/kvm.h +2
-0@@ -494,6 +494,8 @@ int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr,#endif /* NEED_CPU_H */ +bool kvm_cpu_check_resettable (void ); +void kvm_cpu_synchronize_state (CPUState *cpu);void kvm_cpu_synchronize_post_reset (CPUState *cpu);void kvm_cpu_synchronize_post_init (CPUState *cpu);softmmu/cpus.c +5
-0@@ -927,6 +927,11 @@ void hw_error(const char *fmt, ...)abort (); } +bool cpu_is_resettable (void ) +{ +return cpu_check_resettable (); +} +void cpu_synchronize_all_states (void ) {CPUState *cpu;softmmu/vl.c +4
-1@@ -1475,7 +1475,10 @@ void qemu_system_guest_crashloaded(GuestPanicInformation *info)void qemu_system_reset_request (ShutdownCause reason) { -if (no_reboot && reason !=SHUTDOWN_CAUSE_SUBSYSTEM_RESET ) { +if (!cpu_is_resettable ()) { +error_report ("cpus are not resettable, terminating" ); + shutdown_requested = reason; + }else if (no_reboot && reason !=SHUTDOWN_CAUSE_SUBSYSTEM_RESET ) { shutdown_requested = reason; }else { reset_requested = reason;
PATCH 4/4 sev/i386: Enable an SEV-ES guest based on SEV policy
In reply to Tom LendackyFrom: Tom Lendacky <[email protected]>
Update the sev_es_enabled() function return value to be based on the SEV policy that has been specified. SEV-ES is enabled if SEV is enabled and the SEV-ES policy bit is set in the policy object.
--- target/i386/sev.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
target/i386/sev.c +3 -1
@@ -70,6 +70,8 @@ struct SevGuestState {#define DEFAULT_GUEST_POLICY 0x1/* disable debug */ #define DEFAULT_SEV_DEVICE "/dev/sev" +#define GUEST_POLICY_SEV_ES_BIT (1 << 2) +/* SEV Information Block GUID = 00f771de-1a7e-4fcb-890e-68c77e2fb44e */ #define SEV_INFO_BLOCK_GUID "\xde\x71\xf7\x00\x7e\x1a\xcb\x4f\x89\x0e\x68\xc7\x7e\x2f\xb4\x4e"
@@ -374,7 +376,7 @@ sev_enabled(void)bool sev_es_enabled (void ) { -return false; +return (sev_enabled () && (sev_guest->policy &GUEST_POLICY_SEV_ES_BIT )); }uint64_t
Signature · 1 line
2.28.0
Re: [PATCH 4/4] sev/i386: Enable an SEV-ES guest based on SEV policy
Quoted 25 lines · Tom Lendacky
From: Tom Lendacky <[email protected]>
Update the
sev_es_enabled()function return value to be based on the SEV policy that has been specified. SEV-ES is enabled if SEV is enabled and the SEV-ES policy bit is set in the policy object.Signed-off-by Tom Lendacky <[email protected]>--- target/i386/sev.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)target/i386/sev.c +2
-0@@ -70,6 +70,8 @@ struct SevGuestState {#define DEFAULT_GUEST_POLICY 0x1/* disable debug */ #define DEFAULT_SEV_DEVICE "/dev/sev" +#define GUEST_POLICY_SEV_ES_BIT (1 << 2) +/* SEV Information Block GUID = 00f771de-1a7e-4fcb-890e-68c77e2fb44e */ #define SEV_INFO_BLOCK_GUID "\xde\x71\xf7\x00\x7e\x1a\xcb\x4f\x89\x0e\x68\xc7\x7e\x2f\xb4\x4e"
target/i386/sev.c sev_enabled(void) · line 379
Quoted 5 lines
target/i386/sev.c +1
-1@@ -374,7 +376,7 @@ sev_enabled(void)bool sev_es_enabled (void ) { -return false; +return (sev_enabled () && (sev_guest->policy &GUEST_POLICY_SEV_ES_BIT ));
checkpatch wants these outer parentheses removed :-)
Quoted 4 lines
target/i386/sev.c +0
-0}uint64_t
Quoted 3 lines · Tom Lendacky
From: Tom Lendacky <[email protected]>
This patch series provides support for launching an SEV-ES guest.
I've made the changes associated with the checkpatch script output. I'll wait a few more days for other feedback before submitting a v2.
Sorry about the miss in regards to running checkpatch.
Thanks,
Tom
Quoted 80 lines
Secure Encrypted Virtualization - Encrypted State (SEV-ES) expands on the SEV support to protect the guest register state from the hypervisor. See "AMD64 Architecture Programmer's Manual Volume 2: System Programming", section "15.35 Encrypted State (SEV-ES)" [1].
In order to allow a hypervisor to perform functions on behalf of a guest, there is architectural support for notifying a guest's operating system when certain types of VMEXITs are about to occur. This allows the guest to selectively share information with the hypervisor to satisfy the requested function. The notification is performed using a new exception, the VMM Communication exception (#VC). The information is shared through the Guest-Hypervisor Communication Block (GHCB) using the VMGEXIT instruction. The GHCB format and the protocol for using it is documented in "SEV-ES Guest-Hypervisor Communication Block Standardization" [2].
The main areas of the Qemu code that are updated to support SEV-ES are around the SEV guest launch process and AP booting in order to support booting multiple vCPUs.
There are no new command line switches required. Instead, the desire for SEV-ES is presented using the SEV policy object. Bit 2 of the SEV policy object indicates that SEV-ES is required.
The SEV launch process is updated in two ways. The first is that a the
KVM_SEV_ES_INITioctl is used to initialize the guest instead of the standardKVM_SEV_INITioctl. The second is that before the SEV launch measurement is calculated, theLAUNCH_UPDATE_VMSASEV API is invoked for each vCPU that Qemu has created. Once theLAUNCH_UPDATE_VMSAAPI has been invoked, no direct changes to the guest register state can be made.AP booting poses some interesting challenges. The INIT-SIPI-SIPI sequence is typically used to boot the APs. However, the hypervisor is not allowed to update the guest registers. For the APs, the reset vector must be known in advance. An OVMF method to provide a known reset vector address exists by providing an SEV information block, identified by UUID, near the end of the firmware [3]. OVMF will program the jump to the actual reset vector in this area of memory. Since the memory location is known in advance, an AP can be created with the known reset vector address as its starting CS:IP. The GHCB document [2] talks about how SMP booting under SEV-ES is performed.
[1] https://www.amd.com/system/files/TechDocs/24593.pdf
[2] https://developer.amd.com/wp-content/resources/56421.pdf
[3]30937f2f98c4("OvmfPkg: Use the SEV-ES work area for the SEV-ES AP reset vector")https://github.com/tianocore/edk2/commit/30937f2f98c42496f2f143fe8374ae7f7e684847---
These patches are based on commit:
d0ed6a69d3("Update version for v5.1.0 release")(I tried basing on the latest Qemu commit, but I was having build issues that level)
A version of the tree can be found at:
https://github.com/AMDESE/qemu/tree/sev-es-v9Tom Lendacky (4):
sev/i386: Add initial support for SEV-ES sev/i386: Allow AP booting under SEV-ES sev/i386: Don't allow a system reset under an SEV-ES guest sev/i386: Enable an SEV-ES guest based on SEV policyaccel/kvm/kvm-all.c | 68 ++++++++++++++++++++++++++++ accel/stubs/kvm-stub.c | 5 +++ hw/i386/pc_sysfw.c | 10 ++++- include/sysemu/cpus.h | 2 + include/sysemu/hw_accel.h | 4 ++ include/sysemu/kvm.h | 18 ++++++++ include/sysemu/sev.h | 2 + softmmu/cpus.c | 5 +++ softmmu/vl.c | 5 ++- target/i386/cpu.c | 1 + target/i386/kvm.c | 2 + target/i386/sev-stub.c | 5 +++ target/i386/sev.c | 95 ++++++++++++++++++++++++++++++++++++++- target/i386/sev_i386.h | 1 + 14 files changed, 219 insertions(+), 4 deletions(-)