| Message ID | 2ec365fa84338168164cdaea85050cbb022ab932.1712141833.git.roy.hopkins@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,01/10] meson: Add optional dependency on IGVM library | expand |
On Wed, Apr 03, 2024 at 12:11:39PM +0100, Roy Hopkins wrote: > The ConfidentialGuestSupport object defines a number of virtual > functions that are called during processing of IGVM directives to query > or configure initial guest state. In order to support processing of IGVM > files, these functions need to be implemented by relevant isolation > hardware support code such as SEV. > > This commit implements the required functions for SEV-ES and adds > support for processing IGVM files for configuring the guest. > > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com> > --- > target/i386/sev.c | 137 ++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 137 insertions(+) > > diff --git a/target/i386/sev.c b/target/i386/sev.c > index 31dfdc3fe5..46313e7024 100644 > --- a/target/i386/sev.c > +++ b/target/i386/sev.c > @@ -37,6 +37,7 @@ > #include "qapi/qapi-commands-misc-target.h" > #include "exec/confidential-guest-support.h" > #include "hw/i386/pc.h" > +#include "hw/i386/e820_memory_layout.h" > #include "exec/address-spaces.h" > > #define TYPE_SEV_GUEST "sev-guest" > @@ -170,6 +171,9 @@ static const char *const sev_fw_errlist[] = { > > #define SEV_FW_MAX_ERROR ARRAY_SIZE(sev_fw_errlist) > > +static int sev_launch_update_data(SevGuestState *sev_guest, uint8_t *addr, > + uint64_t len); > + > static int > sev_ioctl(int fd, int cmd, void *data, int *error) > { > @@ -304,6 +308,14 @@ sev_guest_finalize(Object *obj) > { > } > > +static int cgs_check_support(ConfidentialGuestPlatformType platform, > + uint16_t platform_version, uint8_t highest_vtl, > + uint64_t shared_gpa_boundary) > +{ > + return (((platform == CGS_PLATFORM_SEV_ES) && sev_es_enabled()) || > + ((platform == CGS_PLATFORM_SEV) && sev_enabled())) ? 1 : 0; > +} > + > static void sev_apply_cpu_context(CPUState *cpu) > { > SevGuestState *sev_guest = SEV_GUEST(MACHINE(qdev_get_machine())->cgs); > @@ -384,6 +396,54 @@ static void sev_apply_cpu_context(CPUState *cpu) > } > } > > +static int check_vmsa_supported(const struct sev_es_save_area *vmsa) > +{ > + struct sev_es_save_area vmsa_check; > + size_t i; > + /* > + * Clear all supported fields so we can then check the entire structure > + * is zero. > + */ > + memcpy(&vmsa_check, vmsa, sizeof(struct sev_es_save_area)); > + memset(&vmsa_check.es, 0, sizeof(vmsa_check.es)); > + memset(&vmsa_check.cs, 0, sizeof(vmsa_check.cs)); > + memset(&vmsa_check.ss, 0, sizeof(vmsa_check.ss)); > + memset(&vmsa_check.ds, 0, sizeof(vmsa_check.ds)); > + memset(&vmsa_check.fs, 0, sizeof(vmsa_check.fs)); > + memset(&vmsa_check.gs, 0, sizeof(vmsa_check.gs)); > + vmsa_check.efer = 0; > + vmsa_check.cr0 = 0; > + vmsa_check.cr3 = 0; > + vmsa_check.cr4 = 0; > + vmsa_check.xcr0 = 0; > + vmsa_check.dr6 = 0; > + vmsa_check.dr7 = 0; > + vmsa_check.rax = 0; > + vmsa_check.rcx = 0; > + vmsa_check.rdx = 0; > + vmsa_check.rbx = 0; > + vmsa_check.rsp = 0; > + vmsa_check.rbp = 0; > + vmsa_check.rsi = 0; > + vmsa_check.rdi = 0; > + vmsa_check.r8 = 0; > + vmsa_check.r9 = 0; > + vmsa_check.r10 = 0; > + vmsa_check.r11 = 0; > + vmsa_check.r12 = 0; > + vmsa_check.r13 = 0; > + vmsa_check.r14 = 0; > + vmsa_check.r15 = 0; > + vmsa_check.rip = 0; > + > + for (i = 0; i < sizeof(vmsa_check); ++i) { > + if (((uint8_t *)&vmsa_check)[i]) { > + return 0; > + } > + } > + return 1; Can this just be: return !buffer_is_zero(vmsa_check, sizeof(vmsa_check)) > +} > + > static int sev_set_cpu_context(uint16_t cpu_index, const void *ctx, > uint32_t ctx_len, hwaddr gpa) > { > @@ -446,6 +506,77 @@ static int sev_set_cpu_context(uint16_t cpu_index, const void *ctx, > return 0; > } > > +static int cgs_set_guest_state(hwaddr gpa, uint8_t *ptr, uint64_t len, > + ConfidentialGuestPageType memory_type, > + uint16_t cpu_index, Error **errp) > +{ > + SevGuestState *sev = SEV_GUEST(MACHINE(qdev_get_machine())->cgs); > + int ret = 1; This results in a return '1' in some errors, but several of the callers are checking '< 0' for the error condition. This variable is redundant anyway, i'd suggest just putting 'return -1' calls after error_setg > + > + if (!sev_enabled()) { > + error_setg(errp, "%s: attempt to configure guest memory, but SEV " > + "is not enabled", > + __func__); > + } else if (memory_type == CGS_PAGE_TYPE_VMSA) { > + if (!sev_es_enabled()) { > + error_setg(errp, > + "%s: attempt to configure initial VMSA, but SEV-ES " > + "is not supported", > + __func__); > + } else { > + if (!check_vmsa_supported((const struct sev_es_save_area *)ptr)) { > + error_setg(errp, > + "%s: The VMSA contains fields that are not " > + "synchronized with KVM. Continuing would result in " > + "either unpredictable guest behavior, or a " > + "mismatched launch measurement.", > + __func__); > + } else { > + ret = sev_set_cpu_context(cpu_index, ptr, len, gpa); This needs to set 'errp' if 'ret' is non-zero, or assert that it is always zer0. > + } > + } > + } else if ((memory_type == CGS_PAGE_TYPE_ZERO) || > + (memory_type == CGS_PAGE_TYPE_NORMAL)) { > + ret = sev_launch_update_data(sev, ptr, len); Likewise needs to set 'errp' or assert. > + } else if (memory_type != CGS_PAGE_TYPE_UNMEASURED) { > + error_setg( > + errp, > + "%s: attempted to configure guest memory to use memory_type %d, " > + "but this type is not supported", > + __func__, (int)memory_type); > + } > + return ret; > +} With regards, Daniel
On Tue, 2024-04-16 at 15:30 +0100, Daniel P. Berrangé wrote: > On Wed, Apr 03, 2024 at 12:11:39PM +0100, Roy Hopkins wrote: > > The ConfidentialGuestSupport object defines a number of virtual > > functions that are called during processing of IGVM directives to query > > or configure initial guest state. In order to support processing of IGVM > > files, these functions need to be implemented by relevant isolation > > hardware support code such as SEV. > > > > This commit implements the required functions for SEV-ES and adds > > support for processing IGVM files for configuring the guest. > > > > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com> > > --- > > target/i386/sev.c | 137 ++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 137 insertions(+) > > > > diff --git a/target/i386/sev.c b/target/i386/sev.c > > index 31dfdc3fe5..46313e7024 100644 > > --- a/target/i386/sev.c > > +++ b/target/i386/sev.c > > @@ -37,6 +37,7 @@ > > #include "qapi/qapi-commands-misc-target.h" > > #include "exec/confidential-guest-support.h" > > #include "hw/i386/pc.h" > > +#include "hw/i386/e820_memory_layout.h" > > #include "exec/address-spaces.h" > > > > #define TYPE_SEV_GUEST "sev-guest" > > @@ -170,6 +171,9 @@ static const char *const sev_fw_errlist[] = { > > > > #define SEV_FW_MAX_ERROR ARRAY_SIZE(sev_fw_errlist) > > > > +static int sev_launch_update_data(SevGuestState *sev_guest, uint8_t *addr, > > + uint64_t len); > > + > > static int > > sev_ioctl(int fd, int cmd, void *data, int *error) > > { > > @@ -304,6 +308,14 @@ sev_guest_finalize(Object *obj) > > { > > } > > > > +static int cgs_check_support(ConfidentialGuestPlatformType platform, > > + uint16_t platform_version, uint8_t > > highest_vtl, > > + uint64_t shared_gpa_boundary) > > +{ > > + return (((platform == CGS_PLATFORM_SEV_ES) && sev_es_enabled()) || > > + ((platform == CGS_PLATFORM_SEV) && sev_enabled())) ? 1 : 0; > > +} > > + > > static void sev_apply_cpu_context(CPUState *cpu) > > { > > SevGuestState *sev_guest = SEV_GUEST(MACHINE(qdev_get_machine())->cgs); > > @@ -384,6 +396,54 @@ static void sev_apply_cpu_context(CPUState *cpu) > > } > > } > > > > +static int check_vmsa_supported(const struct sev_es_save_area *vmsa) > > +{ > > + struct sev_es_save_area vmsa_check; > > + size_t i; > > + /* > > + * Clear all supported fields so we can then check the entire structure > > + * is zero. > > + */ > > + memcpy(&vmsa_check, vmsa, sizeof(struct sev_es_save_area)); > > + memset(&vmsa_check.es, 0, sizeof(vmsa_check.es)); > > + memset(&vmsa_check.cs, 0, sizeof(vmsa_check.cs)); > > + memset(&vmsa_check.ss, 0, sizeof(vmsa_check.ss)); > > + memset(&vmsa_check.ds, 0, sizeof(vmsa_check.ds)); > > + memset(&vmsa_check.fs, 0, sizeof(vmsa_check.fs)); > > + memset(&vmsa_check.gs, 0, sizeof(vmsa_check.gs)); > > + vmsa_check.efer = 0; > > + vmsa_check.cr0 = 0; > > + vmsa_check.cr3 = 0; > > + vmsa_check.cr4 = 0; > > + vmsa_check.xcr0 = 0; > > + vmsa_check.dr6 = 0; > > + vmsa_check.dr7 = 0; > > + vmsa_check.rax = 0; > > + vmsa_check.rcx = 0; > > + vmsa_check.rdx = 0; > > + vmsa_check.rbx = 0; > > + vmsa_check.rsp = 0; > > + vmsa_check.rbp = 0; > > + vmsa_check.rsi = 0; > > + vmsa_check.rdi = 0; > > + vmsa_check.r8 = 0; > > + vmsa_check.r9 = 0; > > + vmsa_check.r10 = 0; > > + vmsa_check.r11 = 0; > > + vmsa_check.r12 = 0; > > + vmsa_check.r13 = 0; > > + vmsa_check.r14 = 0; > > + vmsa_check.r15 = 0; > > + vmsa_check.rip = 0; > > + > > + for (i = 0; i < sizeof(vmsa_check); ++i) { > > + if (((uint8_t *)&vmsa_check)[i]) { > > + return 0; > > + } > > + } > > + return 1; > > Can this just be: > > return !buffer_is_zero(vmsa_check, sizeof(vmsa_check)) > > > > +} > > + > > static int sev_set_cpu_context(uint16_t cpu_index, const void *ctx, > > uint32_t ctx_len, hwaddr gpa) > > { > > @@ -446,6 +506,77 @@ static int sev_set_cpu_context(uint16_t cpu_index, > > const void *ctx, > > return 0; > > } > > > > +static int cgs_set_guest_state(hwaddr gpa, uint8_t *ptr, uint64_t len, > > + ConfidentialGuestPageType memory_type, > > + uint16_t cpu_index, Error **errp) > > +{ > > + SevGuestState *sev = SEV_GUEST(MACHINE(qdev_get_machine())->cgs); > > + int ret = 1; > > This results in a return '1' in some errors, but several of the callers > are checking '< 0' for the error condition. This variable is redundant > anyway, i'd suggest just putting 'return -1' calls after error_setg > > > + > > + if (!sev_enabled()) { > > + error_setg(errp, "%s: attempt to configure guest memory, but SEV " > > + "is not enabled", > > + __func__); > > + } else if (memory_type == CGS_PAGE_TYPE_VMSA) { > > + if (!sev_es_enabled()) { > > + error_setg(errp, > > + "%s: attempt to configure initial VMSA, but SEV-ES " > > + "is not supported", > > + __func__); > > + } else { > > + if (!check_vmsa_supported((const struct sev_es_save_area > > *)ptr)) { > > + error_setg(errp, > > + "%s: The VMSA contains fields that are not " > > + "synchronized with KVM. Continuing would result > > in " > > + "either unpredictable guest behavior, or a " > > + "mismatched launch measurement.", > > + __func__); > > + } else { > > + ret = sev_set_cpu_context(cpu_index, ptr, len, gpa); > > This needs to set 'errp' if 'ret' is non-zero, or assert > that it is always zer0. > > > + } > > + } > > + } else if ((memory_type == CGS_PAGE_TYPE_ZERO) || > > + (memory_type == CGS_PAGE_TYPE_NORMAL)) { > > + ret = sev_launch_update_data(sev, ptr, len); > > Likewise needs to set 'errp' or assert. > > > + } else if (memory_type != CGS_PAGE_TYPE_UNMEASURED) { > > + error_setg( > > + errp, > > + "%s: attempted to configure guest memory to use memory_type %d, > > " > > + "but this type is not supported", > > + __func__, (int)memory_type); > > + } > > + return ret; > > +} > > With regards, > Daniel Thanks Daniel, I'll be addressing all of these in the next version. Regards, Roy
diff --git a/target/i386/sev.c b/target/i386/sev.c index 31dfdc3fe5..46313e7024 100644 --- a/target/i386/sev.c +++ b/target/i386/sev.c @@ -37,6 +37,7 @@ #include "qapi/qapi-commands-misc-target.h" #include "exec/confidential-guest-support.h" #include "hw/i386/pc.h" +#include "hw/i386/e820_memory_layout.h" #include "exec/address-spaces.h" #define TYPE_SEV_GUEST "sev-guest" @@ -170,6 +171,9 @@ static const char *const sev_fw_errlist[] = { #define SEV_FW_MAX_ERROR ARRAY_SIZE(sev_fw_errlist) +static int sev_launch_update_data(SevGuestState *sev_guest, uint8_t *addr, + uint64_t len); + static int sev_ioctl(int fd, int cmd, void *data, int *error) { @@ -304,6 +308,14 @@ sev_guest_finalize(Object *obj) { } +static int cgs_check_support(ConfidentialGuestPlatformType platform, + uint16_t platform_version, uint8_t highest_vtl, + uint64_t shared_gpa_boundary) +{ + return (((platform == CGS_PLATFORM_SEV_ES) && sev_es_enabled()) || + ((platform == CGS_PLATFORM_SEV) && sev_enabled())) ? 1 : 0; +} + static void sev_apply_cpu_context(CPUState *cpu) { SevGuestState *sev_guest = SEV_GUEST(MACHINE(qdev_get_machine())->cgs); @@ -384,6 +396,54 @@ static void sev_apply_cpu_context(CPUState *cpu) } } +static int check_vmsa_supported(const struct sev_es_save_area *vmsa) +{ + struct sev_es_save_area vmsa_check; + size_t i; + /* + * Clear all supported fields so we can then check the entire structure + * is zero. + */ + memcpy(&vmsa_check, vmsa, sizeof(struct sev_es_save_area)); + memset(&vmsa_check.es, 0, sizeof(vmsa_check.es)); + memset(&vmsa_check.cs, 0, sizeof(vmsa_check.cs)); + memset(&vmsa_check.ss, 0, sizeof(vmsa_check.ss)); + memset(&vmsa_check.ds, 0, sizeof(vmsa_check.ds)); + memset(&vmsa_check.fs, 0, sizeof(vmsa_check.fs)); + memset(&vmsa_check.gs, 0, sizeof(vmsa_check.gs)); + vmsa_check.efer = 0; + vmsa_check.cr0 = 0; + vmsa_check.cr3 = 0; + vmsa_check.cr4 = 0; + vmsa_check.xcr0 = 0; + vmsa_check.dr6 = 0; + vmsa_check.dr7 = 0; + vmsa_check.rax = 0; + vmsa_check.rcx = 0; + vmsa_check.rdx = 0; + vmsa_check.rbx = 0; + vmsa_check.rsp = 0; + vmsa_check.rbp = 0; + vmsa_check.rsi = 0; + vmsa_check.rdi = 0; + vmsa_check.r8 = 0; + vmsa_check.r9 = 0; + vmsa_check.r10 = 0; + vmsa_check.r11 = 0; + vmsa_check.r12 = 0; + vmsa_check.r13 = 0; + vmsa_check.r14 = 0; + vmsa_check.r15 = 0; + vmsa_check.rip = 0; + + for (i = 0; i < sizeof(vmsa_check); ++i) { + if (((uint8_t *)&vmsa_check)[i]) { + return 0; + } + } + return 1; +} + static int sev_set_cpu_context(uint16_t cpu_index, const void *ctx, uint32_t ctx_len, hwaddr gpa) { @@ -446,6 +506,77 @@ static int sev_set_cpu_context(uint16_t cpu_index, const void *ctx, return 0; } +static int cgs_set_guest_state(hwaddr gpa, uint8_t *ptr, uint64_t len, + ConfidentialGuestPageType memory_type, + uint16_t cpu_index, Error **errp) +{ + SevGuestState *sev = SEV_GUEST(MACHINE(qdev_get_machine())->cgs); + int ret = 1; + + if (!sev_enabled()) { + error_setg(errp, "%s: attempt to configure guest memory, but SEV " + "is not enabled", + __func__); + } else if (memory_type == CGS_PAGE_TYPE_VMSA) { + if (!sev_es_enabled()) { + error_setg(errp, + "%s: attempt to configure initial VMSA, but SEV-ES " + "is not supported", + __func__); + } else { + if (!check_vmsa_supported((const struct sev_es_save_area *)ptr)) { + error_setg(errp, + "%s: The VMSA contains fields that are not " + "synchronized with KVM. Continuing would result in " + "either unpredictable guest behavior, or a " + "mismatched launch measurement.", + __func__); + } else { + ret = sev_set_cpu_context(cpu_index, ptr, len, gpa); + } + } + } else if ((memory_type == CGS_PAGE_TYPE_ZERO) || + (memory_type == CGS_PAGE_TYPE_NORMAL)) { + ret = sev_launch_update_data(sev, ptr, len); + } else if (memory_type != CGS_PAGE_TYPE_UNMEASURED) { + error_setg( + errp, + "%s: attempted to configure guest memory to use memory_type %d, " + "but this type is not supported", + __func__, (int)memory_type); + } + return ret; +} + +static int cgs_get_mem_map_entry(int index, + ConfidentialGuestMemoryMapEntry *entry, + Error **errp) +{ + if ((index < 0) || (index >= e820_get_num_entries())) { + return 1; + } + entry->gpa = e820_table[index].address; + entry->size = e820_table[index].length; + switch (e820_table[index].type) { + case E820_RAM: + entry->type = CGS_MEM_RAM; + break; + case E820_RESERVED: + entry->type = CGS_MEM_RESERVED; + break; + case E820_ACPI: + entry->type = CGS_MEM_ACPI; + break; + case E820_NVS: + entry->type = CGS_MEM_NVS; + break; + case E820_UNUSABLE: + entry->type = CGS_MEM_UNUSABLE; + break; + } + return 0; +} + static char * sev_guest_get_session_file(Object *obj, Error **errp) { @@ -537,6 +668,7 @@ static void sev_guest_instance_init(Object *obj) { SevGuestState *sev = SEV_GUEST(obj); + ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj); sev->sev_device = g_strdup(DEFAULT_SEV_DEVICE); sev->policy = DEFAULT_GUEST_POLICY; @@ -549,6 +681,11 @@ sev_guest_instance_init(Object *obj) object_property_add_uint32_ptr(obj, "reduced-phys-bits", &sev->reduced_phys_bits, OBJ_PROP_FLAG_READWRITE); + + cgs->check_support = cgs_check_support; + cgs->set_guest_state = cgs_set_guest_state; + cgs->get_mem_map_entry = cgs_get_mem_map_entry; + QTAILQ_INIT(&sev->launch_vmsa); }
The ConfidentialGuestSupport object defines a number of virtual functions that are called during processing of IGVM directives to query or configure initial guest state. In order to support processing of IGVM files, these functions need to be implemented by relevant isolation hardware support code such as SEV. This commit implements the required functions for SEV-ES and adds support for processing IGVM files for configuring the guest. Signed-off-by: Roy Hopkins <roy.hopkins@suse.com> --- target/i386/sev.c | 137 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 137 insertions(+)