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.hopk...@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