| Message ID | 8e49a1aed06d4f98c13a98fbbe3a48cdeb6e0070.1718979106.git.roy.hopkins@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce support for IGVM files | expand |
On Fri, Jun 21, 2024 at 03:29:18PM +0100, Roy Hopkins wrote: > IGVM files can contain an initial VMSA that should be applied to each > vcpu as part of the initial guest state. The sev_features flags are > provided as part of the VMSA structure. However, KVM only allows > sev_features to be set during initialization and not as the guest is > being prepared for launch. > > This patch queries KVM for the supported set of sev_features flags and > processes the IGVM file during kvm_init to determine any sev_features > flags set in the IGVM file. These are then provided in the call to > KVM_SEV_INIT2 to ensure the guest state matches that specified in the > IGVM file. > > This does cause the IGVM file to be processed twice. Firstly to extract > the sev_features then secondly to actually configure the guest. However, > the first pass is largely ignored meaning the overhead is minimal. > > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com> > --- > target/i386/sev.c | 145 ++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 126 insertions(+), 19 deletions(-) > > static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) > { > char *devname; > @@ -1743,6 +1783,10 @@ static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) > } > } > > + if (sev_init_supported_features(sev_common, errp) < 0) { > + return -1; > + } > + > trace_kvm_sev_init(); > if (x86_klass->kvm_type(X86_CONFIDENTIAL_GUEST(sev_common)) == KVM_X86_DEFAULT_VM) { > cmd = sev_es_enabled() ? KVM_SEV_ES_INIT : KVM_SEV_INIT; > @@ -1750,6 +1794,38 @@ static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) > ret = sev_ioctl(sev_common->sev_fd, cmd, NULL, &fw_error); > } else { > struct kvm_sev_init args = { 0 }; > + MachineState *machine = MACHINE(qdev_get_machine()); > + > + /* > + * If configuration is provided via an IGVM file then the IGVM file > + * might contain configuration of the initial vcpu context. For SEV > + * the vcpu context includes the sev_features which should be applied > + * to the vcpu. > + * > + * KVM does not synchronize sev_features from CPU state. Instead it > + * requires sev_features to be provided as part of this initialization > + * call which is subsequently automatically applied to the VMSA of > + * each vcpu. > + * > + * The IGVM file is normally processed after initialization. Therefore > + * we need to pre-process it here to extract sev_features in order to > + * provide it to KVM_SEV_INIT2. Each cgs_* function that is called by > + * the IGVM processor detects this pre-process by observing the state > + * as SEV_STATE_UNINIT. > + */ > + if (machine->igvm) { > + if (IGVM_CFG_GET_CLASS(machine->igvm) > + ->process(machine->igvm, machine->cgs, errp) == -1) { > + return -1; > + } > + /* > + * KVM maintains a bitmask of allowed sev_features. This does not > + * include SVM_SEV_FEAT_SNP_ACTIVE which is set accordingly by KVM > + * itself. Therefore we need to clear this flag. > + */ > + args.vmsa_features = sev_common->sev_features & > + ~SVM_SEV_FEAT_SNP_ACTIVE; > + } > > ret = sev_ioctl(sev_common->sev_fd, KVM_SEV_INIT2, &args, &fw_error); > } What happens if the code path takes us down the KVM_SEV_INIT route, rather than KVM_SEV_INIT2 ? Should we be reporting an error indicating that IGVM usage is incompatible with the legacy KVM_SEV_INIT path ? With regards, Daniel
On Mon, 2024-06-24 at 15:14 +0100, Daniel P. Berrangé wrote: > On Fri, Jun 21, 2024 at 03:29:18PM +0100, Roy Hopkins wrote: > > IGVM files can contain an initial VMSA that should be applied to each > > vcpu as part of the initial guest state. The sev_features flags are > > provided as part of the VMSA structure. However, KVM only allows > > sev_features to be set during initialization and not as the guest is > > being prepared for launch. > > > > This patch queries KVM for the supported set of sev_features flags and > > processes the IGVM file during kvm_init to determine any sev_features > > flags set in the IGVM file. These are then provided in the call to > > KVM_SEV_INIT2 to ensure the guest state matches that specified in the > > IGVM file. > > > > This does cause the IGVM file to be processed twice. Firstly to extract > > the sev_features then secondly to actually configure the guest. However, > > the first pass is largely ignored meaning the overhead is minimal. > > > > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com> > > --- > > target/i386/sev.c | 145 ++++++++++++++++++++++++++++++++++++++++------ > > 1 file changed, 126 insertions(+), 19 deletions(-) > > > > > static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) > > { > > char *devname; > > @@ -1743,6 +1783,10 @@ static int > > sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) > > } > > } > > > > + if (sev_init_supported_features(sev_common, errp) < 0) { > > + return -1; > > + } > > + > > trace_kvm_sev_init(); > > if (x86_klass->kvm_type(X86_CONFIDENTIAL_GUEST(sev_common)) == > > KVM_X86_DEFAULT_VM) { > > cmd = sev_es_enabled() ? KVM_SEV_ES_INIT : KVM_SEV_INIT; > > @@ -1750,6 +1794,38 @@ static int > > sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) > > ret = sev_ioctl(sev_common->sev_fd, cmd, NULL, &fw_error); > > } else { > > struct kvm_sev_init args = { 0 }; > > + MachineState *machine = MACHINE(qdev_get_machine()); > > + > > + /* > > + * If configuration is provided via an IGVM file then the IGVM file > > + * might contain configuration of the initial vcpu context. For SEV > > + * the vcpu context includes the sev_features which should be > > applied > > + * to the vcpu. > > + * > > + * KVM does not synchronize sev_features from CPU state. Instead it > > + * requires sev_features to be provided as part of this > > initialization > > + * call which is subsequently automatically applied to the VMSA of > > + * each vcpu. > > + * > > + * The IGVM file is normally processed after initialization. > > Therefore > > + * we need to pre-process it here to extract sev_features in order > > to > > + * provide it to KVM_SEV_INIT2. Each cgs_* function that is called > > by > > + * the IGVM processor detects this pre-process by observing the > > state > > + * as SEV_STATE_UNINIT. > > + */ > > + if (machine->igvm) { > > + if (IGVM_CFG_GET_CLASS(machine->igvm) > > + ->process(machine->igvm, machine->cgs, errp) == -1) { > > + return -1; > > + } > > + /* > > + * KVM maintains a bitmask of allowed sev_features. This does > > not > > + * include SVM_SEV_FEAT_SNP_ACTIVE which is set accordingly by > > KVM > > + * itself. Therefore we need to clear this flag. > > + */ > > + args.vmsa_features = sev_common->sev_features & > > + ~SVM_SEV_FEAT_SNP_ACTIVE; > > + } > > > > ret = sev_ioctl(sev_common->sev_fd, KVM_SEV_INIT2, &args, > > &fw_error); > > } > > What happens if the code path takes us down the KVM_SEV_INIT > route, rather than KVM_SEV_INIT2 ? Should we be reporting an > error indicating that IGVM usage is incompatible with the legacy > KVM_SEV_INIT path ? > > > With regards, > Daniel The idea is that sev_common->supported_sev_features is initialised to 0 for the legacy path meaning that the call to check_sev_features() that occurs when the IGVM file is parsed will flag an error stating 'VMSA contains unsupported sev_features' if any features are set. This should ensure the IGVM file matches the settings in the legacy kernel. However, I've just run this against an older kernel and found an error: the kvm_ioctl in sev_init_supported_features() is only supported in newer kernels meaning that the launch is aborted on older kernels. I'll update this patch to fix this and ensure supported_sev_features is 0 for the KVM_SEV_INIT case. Regards, Roy
diff --git a/target/i386/sev.c b/target/i386/sev.c index 688b378c42..4b9d74207b 100644 --- a/target/i386/sev.c +++ b/target/i386/sev.c @@ -117,6 +117,8 @@ struct SevCommonState { uint32_t cbitpos; uint32_t reduced_phys_bits; bool kernel_hashes; + uint64_t sev_features; + uint64_t supported_sev_features; /* runtime state */ uint8_t api_major; @@ -490,7 +492,40 @@ static void sev_apply_cpu_context(CPUState *cpu) } } -static int check_vmsa_supported(hwaddr gpa, const struct sev_es_save_area *vmsa, +static int check_sev_features(SevCommonState *sev_common, uint64_t sev_features, + Error **errp) +{ + /* + * Ensure SEV_FEATURES is configured for correct SEV hardware and that + * the requested features are supported. If SEV-SNP is enabled then + * that feature must be enabled, otherwise it must be cleared. + */ + if (sev_snp_enabled() && !(sev_features & SVM_SEV_FEAT_SNP_ACTIVE)) { + error_setg( + errp, + "%s: SEV_SNP is enabled but is not enabled in VMSA sev_features", + __func__); + return -1; + } else if (!sev_snp_enabled() && + (sev_features & SVM_SEV_FEAT_SNP_ACTIVE)) { + error_setg( + errp, + "%s: SEV_SNP is not enabled but is enabled in VMSA sev_features", + __func__); + return -1; + } + if (sev_features & ~sev_common->supported_sev_features) { + error_setg(errp, + "%s: VMSA contains unsupported sev_features: %lX, " + "supported features: %lX", + __func__, sev_features, sev_common->supported_sev_features); + return -1; + } + return 0; +} + +static int check_vmsa_supported(SevCommonState *sev_common, hwaddr gpa, + const struct sev_es_save_area *vmsa, Error **errp) { struct sev_es_save_area vmsa_check; @@ -556,24 +591,10 @@ static int check_vmsa_supported(hwaddr gpa, const struct sev_es_save_area *vmsa, vmsa_check.x87_fcw = 0; vmsa_check.mxcsr = 0; - if (sev_snp_enabled()) { - if (vmsa_check.sev_features != SVM_SEV_FEAT_SNP_ACTIVE) { - error_setg(errp, - "%s: sev_features in the VMSA contains an unsupported " - "value. For SEV-SNP, sev_features must be set to %x.", - __func__, SVM_SEV_FEAT_SNP_ACTIVE); - return -1; - } - vmsa_check.sev_features = 0; - } else { - if (vmsa_check.sev_features != 0) { - error_setg(errp, - "%s: sev_features in the VMSA contains an unsupported " - "value. For SEV-ES and SEV, sev_features must be " - "set to 0.", __func__); - return -1; - } + if (check_sev_features(sev_common, vmsa_check.sev_features, errp) < 0) { + return -1; } + vmsa_check.sev_features = 0; if (!buffer_is_zero(&vmsa_check, sizeof(vmsa_check))) { error_setg(errp, @@ -1663,6 +1684,25 @@ static int sev_snp_kvm_type(X86ConfidentialGuest *cg) return KVM_X86_SNP_VM; } +static int sev_init_supported_features(SevCommonState *sev_common, Error **errp) +{ + /* Query KVM for the supported set of sev_features */ + struct kvm_device_attr attr = { + .group = KVM_X86_GRP_SEV, + .attr = KVM_X86_SEV_VMSA_FEATURES, + .addr = (unsigned long)&sev_common->supported_sev_features, + }; + if (kvm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr) < 0) { + error_setg(errp, "%s: failed to query supported sev_features", + __func__); + return -1; + } + if (sev_snp_enabled()) { + sev_common->supported_sev_features |= SVM_SEV_FEAT_SNP_ACTIVE; + } + return 0; +} + static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) { char *devname; @@ -1743,6 +1783,10 @@ static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) } } + if (sev_init_supported_features(sev_common, errp) < 0) { + return -1; + } + trace_kvm_sev_init(); if (x86_klass->kvm_type(X86_CONFIDENTIAL_GUEST(sev_common)) == KVM_X86_DEFAULT_VM) { cmd = sev_es_enabled() ? KVM_SEV_ES_INIT : KVM_SEV_INIT; @@ -1750,6 +1794,38 @@ static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) ret = sev_ioctl(sev_common->sev_fd, cmd, NULL, &fw_error); } else { struct kvm_sev_init args = { 0 }; + MachineState *machine = MACHINE(qdev_get_machine()); + + /* + * If configuration is provided via an IGVM file then the IGVM file + * might contain configuration of the initial vcpu context. For SEV + * the vcpu context includes the sev_features which should be applied + * to the vcpu. + * + * KVM does not synchronize sev_features from CPU state. Instead it + * requires sev_features to be provided as part of this initialization + * call which is subsequently automatically applied to the VMSA of + * each vcpu. + * + * The IGVM file is normally processed after initialization. Therefore + * we need to pre-process it here to extract sev_features in order to + * provide it to KVM_SEV_INIT2. Each cgs_* function that is called by + * the IGVM processor detects this pre-process by observing the state + * as SEV_STATE_UNINIT. + */ + if (machine->igvm) { + if (IGVM_CFG_GET_CLASS(machine->igvm) + ->process(machine->igvm, machine->cgs, errp) == -1) { + return -1; + } + /* + * KVM maintains a bitmask of allowed sev_features. This does not + * include SVM_SEV_FEAT_SNP_ACTIVE which is set accordingly by KVM + * itself. Therefore we need to clear this flag. + */ + args.vmsa_features = sev_common->sev_features & + ~SVM_SEV_FEAT_SNP_ACTIVE; + } ret = sev_ioctl(sev_common->sev_fd, KVM_SEV_INIT2, &args, &fw_error); } @@ -2348,6 +2424,24 @@ static int cgs_set_guest_state(hwaddr gpa, uint8_t *ptr, uint64_t len, SevCommonState *sev_common = SEV_COMMON(MACHINE(qdev_get_machine())->cgs); SevCommonStateClass *klass = SEV_COMMON_GET_CLASS(sev_common); + if (sev_common->state == SEV_STATE_UNINIT) { + /* Pre-processing of IGVM file called from sev_common_kvm_init() */ + if ((cpu_index == 0) && (memory_type == CGS_PAGE_TYPE_VMSA)) { + const struct sev_es_save_area *sa = + (const struct sev_es_save_area *)ptr; + if (len < sizeof(*sa)) { + error_setg(errp, "%s: invalid VMSA length encountered", + __func__); + return -1; + } + if (check_sev_features(sev_common, sa->sev_features, errp) < 0) { + return -1; + } + sev_common->sev_features = sa->sev_features; + } + return 0; + } + if (!sev_enabled()) { error_setg(errp, "%s: attempt to configure guest memory, but SEV " "is not enabled", __func__); @@ -2367,7 +2461,8 @@ static int cgs_set_guest_state(hwaddr gpa, uint8_t *ptr, uint64_t len, __func__); return -1; } - if (check_vmsa_supported(gpa, (const struct sev_es_save_area *)ptr, + if (check_vmsa_supported(sev_common, gpa, + (const struct sev_es_save_area *)ptr, errp) < 0) { return -1; } @@ -2421,6 +2516,12 @@ static int cgs_get_mem_map_entry(int index, ConfidentialGuestMemoryMapEntry *entry, Error **errp) { + SevCommonState *sev_common = SEV_COMMON(MACHINE(qdev_get_machine())->cgs); + if (sev_common->state == SEV_STATE_UNINIT) { + /* Pre-processing of IGVM file called from sev_common_kvm_init() */ + return 1; + } + if ((index < 0) || (index >= e820_get_num_entries())) { return 1; } @@ -2451,6 +2552,12 @@ static int cgs_set_guest_policy(ConfidentialGuestPolicyType policy_type, uint32_t policy_data1_size, void *policy_data2, uint32_t policy_data2_size, Error **errp) { + SevCommonState *sev_common = SEV_COMMON(MACHINE(qdev_get_machine())->cgs); + if (sev_common->state == SEV_STATE_UNINIT) { + /* Pre-processing of IGVM file called from sev_common_kvm_init() */ + return 0; + } + if (policy_type != GUEST_POLICY_SEV) { error_setg(errp, "%s: Invalid guest policy type provided for SEV: %d", __func__, policy_type);
IGVM files can contain an initial VMSA that should be applied to each vcpu as part of the initial guest state. The sev_features flags are provided as part of the VMSA structure. However, KVM only allows sev_features to be set during initialization and not as the guest is being prepared for launch. This patch queries KVM for the supported set of sev_features flags and processes the IGVM file during kvm_init to determine any sev_features flags set in the IGVM file. These are then provided in the call to KVM_SEV_INIT2 to ensure the guest state matches that specified in the IGVM file. This does cause the IGVM file to be processed twice. Firstly to extract the sev_features then secondly to actually configure the guest. However, the first pass is largely ignored meaning the overhead is minimal. Signed-off-by: Roy Hopkins <roy.hopkins@suse.com> --- target/i386/sev.c | 145 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 126 insertions(+), 19 deletions(-)