| Message ID | 20250207233327.130770-1-kim.phillips@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] target/i386: sev: Add cmdline option to enable the Allowed SEV Features feature | expand |
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@amd.com> > --- > 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
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@amd.com> > --- > 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)) { > + __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;
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@amd.com> >> --- >> 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;
On 2/10/25 3:24 AM, Daniel P. Berrangé wrote: > 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 :-( Apologies, that was not the intent. I'll remove it from the next version. >> @@ -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 ? That's 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 ? It would have been blocked regardless by this check in kvm's __sev_guest_init: https://elixir.bootlin.com/linux/v6.13.2/source/arch/x86/kvm/svm/sev.c#L418 I've addressed all your other comments. Thank you for your review, Kim
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) +{ + 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); + return -1; + } + 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;
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@amd.com> --- qapi/qom.json | 6 ++++- target/i386/sev.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++ target/i386/sev.h | 2 ++ 3 files changed, 67 insertions(+), 1 deletion(-)