On 2/10/25 12:53, Tom Lendacky wrote: > On 2/7/25 17:33, Kim Phillips wrote: >> The Allowed SEV Features feature allows the host kernel to control >> which SEV features it does not want the guest to enable [1]. >> >> This has to be explicitly opted-in by the user because it has the >> ability to break existing VMs if it were set automatically. >> >> Currently, both the PmcVirtualization and SecureAvic features >> require the Allowed SEV Features feature to be set. >> >> Based on a similar patch written for Secure TSC [2]. >> >> [1] Section 15.36.20 "Allowed SEV Features", AMD64 Architecture >> Programmer's Manual, Pub. 24593 Rev. 3.42 - March 2024: >> https://bugzilla.kernel.org/attachment.cgi?id=306250 >> >> [2] https://github.com/qemu/qemu/commit/4b2288dc6025ba32519ee8d202ca72d565cbbab7 >> >> Signed-off-by: Kim Phillips <kim.phillips@xxxxxxx> >> --- >> qapi/qom.json | 6 ++++- >> target/i386/sev.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++ >> target/i386/sev.h | 2 ++ >> 3 files changed, 67 insertions(+), 1 deletion(-) >> >> diff --git a/qapi/qom.json b/qapi/qom.json >> index 28ce24cd8d..113b44ad74 100644 >> --- a/qapi/qom.json >> +++ b/qapi/qom.json >> @@ -948,13 +948,17 @@ >> # designated guest firmware page for measured boot with -kernel >> # (default: false) (since 6.2) >> # >> +# @allowed-sev-features: true if secure allowed-sev-features feature >> +# is to be enabled in an SEV-ES or SNP guest. (default: false) >> +# >> # Since: 9.1 >> ## >> { 'struct': 'SevCommonProperties', >> 'data': { '*sev-device': 'str', >> '*cbitpos': 'uint32', >> 'reduced-phys-bits': 'uint32', >> - '*kernel-hashes': 'bool' } } >> + '*kernel-hashes': 'bool', >> + '*allowed-sev-features': 'bool' } } >> >> ## >> # @SevGuestProperties: >> diff --git a/target/i386/sev.c b/target/i386/sev.c >> index 0e1dbb6959..85ad73f9a0 100644 >> --- a/target/i386/sev.c >> +++ b/target/i386/sev.c >> @@ -98,6 +98,7 @@ struct SevCommonState { >> uint32_t cbitpos; >> uint32_t reduced_phys_bits; >> bool kernel_hashes; >> + uint64_t vmsa_features; >> >> /* runtime state */ >> uint8_t api_major; >> @@ -411,6 +412,33 @@ sev_get_reduced_phys_bits(void) >> return sev_common ? sev_common->reduced_phys_bits : 0; >> } >> >> +static __u64 >> +sev_supported_vmsa_features(void) > > s/sev_/sev_get_/ ? > >> +{ >> + uint64_t supported_vmsa_features = 0; >> + struct kvm_device_attr attr = { >> + .group = KVM_X86_GRP_SEV, >> + .attr = KVM_X86_SEV_VMSA_FEATURES, >> + .addr = (unsigned long) &supported_vmsa_features >> + }; >> + >> + bool sys_attr = kvm_check_extension(kvm_state, KVM_CAP_SYS_ATTRIBUTES); >> + if (!sys_attr) { >> + return 0; >> + } >> + >> + int rc = kvm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr); >> + if (rc < 0) { >> + if (rc != -ENXIO) { >> + warn_report("KVM_GET_DEVICE_ATTR(0, KVM_X86_SEV_VMSA_FEATURES) " >> + "error: %d", rc); >> + } >> + return 0; >> + } >> + >> + return supported_vmsa_features; >> +} >> + >> static SevInfo *sev_get_info(void) >> { >> SevInfo *info; >> @@ -1524,6 +1552,20 @@ static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) >> case KVM_X86_SNP_VM: { >> struct kvm_sev_init args = { 0 }; >> >> + if (sev_es_enabled()) { > > Shouldn't this be > > if (sev_es_enabled() && (sev_common->vmsa_features & SEV_VMSA_ALLOWED_SEV_FEATURES)) { Actually, I guess it doesn't matter. The vmsa_features field will be 0 by default and only be set if "allowed-sev-features=on" is specified. So doing this will just error out a bit earlier than KVM erroring out on the INIT2 call if some vmsa_feature bit is set that KVM doesn't know about. Thanks, Tom > >> + __u64 vmsa_features, supported_vmsa_features; > > s/__u64/uint64_t/ ? > >> + >> + supported_vmsa_features = sev_supported_vmsa_features(); >> + vmsa_features = sev_common->vmsa_features; >> + if ((vmsa_features & supported_vmsa_features) != vmsa_features) { >> + error_setg(errp, "%s: requested sev feature mask (0x%llx) " >> + "contains bits not supported by the host kernel " >> + " (0x%llx)", __func__, vmsa_features, >> + supported_vmsa_features); >> + return -1; >> + } > > Add a blank line > >> + args.vmsa_features = vmsa_features; >> + } > > Add a blank line > > Thanks, > Tom > >> ret = sev_ioctl(sev_common->sev_fd, KVM_SEV_INIT2, &args, &fw_error); >> break; >> } >> @@ -2044,6 +2086,19 @@ static void sev_common_set_kernel_hashes(Object *obj, bool value, Error **errp) >> SEV_COMMON(obj)->kernel_hashes = value; >> } >> >> +static bool >> +sev_snp_guest_get_allowed_sev_features(Object *obj, Error **errp) >> +{ >> + return SEV_COMMON(obj)->vmsa_features & SEV_VMSA_ALLOWED_SEV_FEATURES; >> +} >> + >> +static void >> +sev_snp_guest_set_allowed_sev_features(Object *obj, bool value, Error **errp) >> +{ >> + if (value) >> + SEV_COMMON(obj)->vmsa_features |= SEV_VMSA_ALLOWED_SEV_FEATURES; >> +} >> + >> static void >> sev_common_class_init(ObjectClass *oc, void *data) >> { >> @@ -2061,6 +2116,11 @@ sev_common_class_init(ObjectClass *oc, void *data) >> sev_common_set_kernel_hashes); >> object_class_property_set_description(oc, "kernel-hashes", >> "add kernel hashes to guest firmware for measured Linux boot"); >> + object_class_property_add_bool(oc, "allowed-sev-features", >> + sev_snp_guest_get_allowed_sev_features, >> + sev_snp_guest_set_allowed_sev_features); >> + object_class_property_set_description(oc, "allowed-sev-features", >> + "Enable the Allowed SEV Features feature"); >> } >> >> static void >> diff --git a/target/i386/sev.h b/target/i386/sev.h >> index 373669eaac..07447c4b01 100644 >> --- a/target/i386/sev.h >> +++ b/target/i386/sev.h >> @@ -44,6 +44,8 @@ bool sev_snp_enabled(void); >> #define SEV_SNP_POLICY_SMT 0x10000 >> #define SEV_SNP_POLICY_DBG 0x80000 >> >> +#define SEV_VMSA_ALLOWED_SEV_FEATURES BIT_ULL(63) >> + >> typedef struct SevKernelLoaderContext { >> char *setup_data; >> size_t setup_size;
