On Fri, Feb 07, 2025 at 05:33:27PM -0600, 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 Despite that URL, that commit also does not appear to be merged into the QEMU git repo, and indeed I can't find any record of it even being posted as a patch for review on qemu-devel. This is horribly misleading to reviewers, suggesting that the referenced patch was already accepted :-( > > 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) Missing 'since' annotation. > +# > # 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) > +{ > + 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()) { > + __u64 vmsa_features, supported_vmsa_features; > + > + 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); This logic is being applied unconditionally, and not connected to the setting of the new 'allowed-sev-features' flag value. Is that correct ? Will this end up breaking existing deployed guests, or is this a scenario that would have been blocked with an error later on regardless ? > + return -1; Malformed indentation. > + } > + args.vmsa_features = vmsa_features; > + } > 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; > -- > 2.43.0 > > With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
