| Message ID | 20240802015732.3192877-3-kim.phillips@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: SEV: Add support for the ALLOWED_SEV_FEATURES feature | expand |
On 8/2/2024 7:27 AM, Kim Phillips wrote: > From: Kishon Vijay Abraham I <kvijayab@amd.com> > > AMD EPYC 5th generation processors have introduced a feature that allows > the hypervisor to control the SEV_FEATURES that are set for or by a > guest [1]. The ALLOWED_SEV_FEATURES feature can be used by the hypervisor > to enforce that SEV-ES and SEV-SNP guests cannot enable features that the > hypervisor does not want to be enabled. > > When ALLOWED_SEV_FEATURES is enabled, a VMRUN will fail if any > non-reserved bits are 1 in SEV_FEATURES but are 0 in > ALLOWED_SEV_FEATURES. > > [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 > > Signed-off-by: Kishon Vijay Abraham I <kvijayab@amd.com> > Signed-off-by: Kim Phillips <kim.phillips@amd.com> Looks good to me. Reviewed-by: Nikunj A. Dadhania <nikunj@amd.com> > --- > arch/x86/include/asm/svm.h | 6 +++++- > arch/x86/kvm/svm/sev.c | 5 +++++ > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h > index f0dea3750ca9..59516ad2028b 100644 > --- a/arch/x86/include/asm/svm.h > +++ b/arch/x86/include/asm/svm.h > @@ -158,7 +158,9 @@ struct __attribute__ ((__packed__)) vmcb_control_area { > u64 avic_physical_id; /* Offset 0xf8 */ > u8 reserved_7[8]; > u64 vmsa_pa; /* Used for an SEV-ES guest */ > - u8 reserved_8[720]; > + u8 reserved_8[40]; > + u64 allowed_sev_features; /* Offset 0x138 */ > + u8 reserved_9[672]; > /* > * Offset 0x3e0, 32 bytes reserved > * for use by hypervisor/software. > @@ -294,6 +296,8 @@ static_assert((X2AVIC_MAX_PHYSICAL_ID & AVIC_PHYSICAL_MAX_INDEX_MASK) == X2AVIC_ > (SVM_SEV_FEAT_RESTRICTED_INJECTION | \ > SVM_SEV_FEAT_ALTERNATE_INJECTION) > > +#define VMCB_ALLOWED_SEV_FEATURES_VALID BIT_ULL(63) > + > struct vmcb_seg { > u16 selector; > u16 attrib; > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index a16c873b3232..d12b4d615b32 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -899,6 +899,7 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm) > static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu, > int *error) > { > + struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; > struct sev_data_launch_update_vmsa vmsa; > struct vcpu_svm *svm = to_svm(vcpu); > int ret; > @@ -908,6 +909,10 @@ static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu, > return -EINVAL; > } > > + if (cpu_feature_enabled(X86_FEATURE_ALLOWED_SEV_FEATURES)) > + svm->vmcb->control.allowed_sev_features = VMCB_ALLOWED_SEV_FEATURES_VALID | > + sev->vmsa_features; > + > /* Perform some pre-encryption checks against the VMSA */ > ret = sev_es_sync_vmsa(svm); > if (ret)
On Thu, Aug 01, 2024, Kim Phillips wrote: > From: Kishon Vijay Abraham I <kvijayab@amd.com> > > AMD EPYC 5th generation processors have introduced a feature that allows > the hypervisor to control the SEV_FEATURES that are set for or by a > guest [1]. The ALLOWED_SEV_FEATURES feature can be used by the hypervisor > to enforce that SEV-ES and SEV-SNP guests cannot enable features that the > hypervisor does not want to be enabled. How does the host communicate to the guest which features are allowed? And based on this blurb: Some SEV features can only be used if the Allowed SEV Features Mask is enabled, and the mask is configured to permit the corresponding feature. If the Allowed SEV Features Mask is not enabled, these features are not available (see SEV_FEATURES in Appendix B, Table B-4). and the appendix, this only applies to PmcVirtualization and SecureAvic. Adding that info in the changelog would be *very* helpful. And I see that SVM_SEV_FEAT_DEBUG_SWAP, a.k.a. DebugVirtualization, is a guest controlled feature and doesn't honor ALLOWED_SEV_FEATURES. Doesn't that mean sev_vcpu_has_debug_swap() is broken, i.e. that KVM must assume the guest can DebugVirtualization on and off at will? Or am I missing something?
On 8/16/24 5:59 PM, Sean Christopherson wrote: > On Thu, Aug 01, 2024, Kim Phillips wrote: >> From: Kishon Vijay Abraham I <kvijayab@amd.com> >> >> AMD EPYC 5th generation processors have introduced a feature that allows >> the hypervisor to control the SEV_FEATURES that are set for or by a >> guest [1]. The ALLOWED_SEV_FEATURES feature can be used by the hypervisor >> to enforce that SEV-ES and SEV-SNP guests cannot enable features that the >> hypervisor does not want to be enabled. > > How does the host communicate to the guest which features are allowed? I'm not familiar with any future plans to negotiate with the guest directly, but since commit ac5c48027bac ("KVM: SEV: publish supported VMSA features"), userspace can retrieve sev_supported_vmsa_features via an ioctl. > And based on this blurb: > > Some SEV features can only be used if the Allowed SEV Features Mask is enabled, > and the mask is configured to permit the corresponding feature. If the Allowed > SEV Features Mask is not enabled, these features are not available (see SEV_FEATURES > in Appendix B, Table B-4). > > and the appendix, this only applies to PmcVirtualization and SecureAvic. Adding > that info in the changelog would be *very* helpful. Ok, how about adding: "The PmcVirtualization and SecureAvic features explicitly require ALLOWED_SEV_FEATURES to enable them before they can be used." > And I see that SVM_SEV_FEAT_DEBUG_SWAP, a.k.a. DebugVirtualization, is a guest > controlled feature and doesn't honor ALLOWED_SEV_FEATURES. Doesn't that mean > sev_vcpu_has_debug_swap() is broken, i.e. that KVM must assume the guest can > DebugVirtualization on and off at will? Or am I missing something? My understanding is that users control KVM's DEBUG_SWAP setting with a module parameter since commit 4dd5ecacb9a4 ("KVM: SEV: allow SEV-ES DebugSwap again"). If the module parameter is not set, with this patch, VMRUN will fail since the host doesn't allow DEBUG_SWAP. Thanks for your review! Kim
On Mon, Aug 19, 2024, Kim Phillips wrote: > On 8/16/24 5:59 PM, Sean Christopherson wrote: > > On Thu, Aug 01, 2024, Kim Phillips wrote: > > > From: Kishon Vijay Abraham I <kvijayab@amd.com> > > > > > > AMD EPYC 5th generation processors have introduced a feature that allows > > > the hypervisor to control the SEV_FEATURES that are set for or by a > > > guest [1]. The ALLOWED_SEV_FEATURES feature can be used by the hypervisor > > > to enforce that SEV-ES and SEV-SNP guests cannot enable features that the > > > hypervisor does not want to be enabled. > > > > How does the host communicate to the guest which features are allowed? > > I'm not familiar with any future plans to negotiate with the guest directly, I feel like I'm missing something. What happens if the guest wants to enable PmcVirtualization and it's unexpectedly disallowed? Does the guest simply panic? > but since commit ac5c48027bac ("KVM: SEV: publish supported VMSA features"), > userspace can retrieve sev_supported_vmsa_features via an ioctl. > > > And based on this blurb: > > > > Some SEV features can only be used if the Allowed SEV Features Mask is enabled, > > and the mask is configured to permit the corresponding feature. If the Allowed > > SEV Features Mask is not enabled, these features are not available (see SEV_FEATURES > > in Appendix B, Table B-4). > > > > and the appendix, this only applies to PmcVirtualization and SecureAvic. Adding > > that info in the changelog would be *very* helpful. > > Ok, how about adding: > > "The PmcVirtualization and SecureAvic features explicitly require > ALLOWED_SEV_FEATURES to enable them before they can be used." > > > And I see that SVM_SEV_FEAT_DEBUG_SWAP, a.k.a. DebugVirtualization, is a guest > > controlled feature and doesn't honor ALLOWED_SEV_FEATURES. Doesn't that mean > > sev_vcpu_has_debug_swap() is broken, i.e. that KVM must assume the guest can > > DebugVirtualization on and off at will? Or am I missing something? > > My understanding is that users control KVM's DEBUG_SWAP setting > with a module parameter since commit 4dd5ecacb9a4 ("KVM: SEV: allow > SEV-ES DebugSwap again"). If the module parameter is not set, with > this patch, VMRUN will fail since the host doesn't allow DEBUG_SWAP. But that's just KVM's view of vmsa_features. With SNP's wonderful SVM_VMGEXIT_AP_CREATE, can't the guest create a VMSA with whatever sev_features it wants, so long as they aren't host-controllable, i.e. aren't PmcVirtualization or SecureAvic?
On 8/19/24 5:23 PM, Sean Christopherson wrote: > On Mon, Aug 19, 2024, Kim Phillips wrote: >> On 8/16/24 5:59 PM, Sean Christopherson wrote: >>> On Thu, Aug 01, 2024, Kim Phillips wrote: >>>> From: Kishon Vijay Abraham I <kvijayab@amd.com> >>>> >>>> AMD EPYC 5th generation processors have introduced a feature that allows >>>> the hypervisor to control the SEV_FEATURES that are set for or by a >>>> guest [1]. The ALLOWED_SEV_FEATURES feature can be used by the hypervisor >>>> to enforce that SEV-ES and SEV-SNP guests cannot enable features that the >>>> hypervisor does not want to be enabled. >>> >>> How does the host communicate to the guest which features are allowed? >> >> I'm not familiar with any future plans to negotiate with the guest directly, > > I feel like I'm missing something. What happens if the guest wants to enable > PmcVirtualization and it's unexpectedly disallowed? Does the guest simply panic? In SNP, VMRUN will return with an exit code of VMEXIT_INVALID (0xffffffff) if the guest tries to set it. In SEV-ES, the hypervisor can set it, and the same thing will happen to VMRUN. In both cases, SEV_FEATURES is saved to VMCB field GUEST_SEV_FEATURES at offset 140h on the VMEXIT, indicating to the host which feature was attempted but caught as not allowed. >> but since commit ac5c48027bac ("KVM: SEV: publish supported VMSA features"), >> userspace can retrieve sev_supported_vmsa_features via an ioctl. >> >>> And based on this blurb: >>> >>> Some SEV features can only be used if the Allowed SEV Features Mask is enabled, >>> and the mask is configured to permit the corresponding feature. If the Allowed >>> SEV Features Mask is not enabled, these features are not available (see SEV_FEATURES >>> in Appendix B, Table B-4). >>> >>> and the appendix, this only applies to PmcVirtualization and SecureAvic. Adding >>> that info in the changelog would be *very* helpful. >> >> Ok, how about adding: >> >> "The PmcVirtualization and SecureAvic features explicitly require >> ALLOWED_SEV_FEATURES to enable them before they can be used." >> >>> And I see that SVM_SEV_FEAT_DEBUG_SWAP, a.k.a. DebugVirtualization, is a guest >>> controlled feature and doesn't honor ALLOWED_SEV_FEATURES. Doesn't that mean >>> sev_vcpu_has_debug_swap() is broken, i.e. that KVM must assume the guest can >>> DebugVirtualization on and off at will? Or am I missing something? >> >> My understanding is that users control KVM's DEBUG_SWAP setting >> with a module parameter since commit 4dd5ecacb9a4 ("KVM: SEV: allow >> SEV-ES DebugSwap again"). If the module parameter is not set, with >> this patch, VMRUN will fail since the host doesn't allow DEBUG_SWAP. > > But that's just KVM's view of vmsa_features. With SNP's wonderful > SVM_VMGEXIT_AP_CREATE, can't the guest create a VMSA with whatever sev_features > it wants, so long as they aren't host-controllable, i.e. aren't PmcVirtualization > or SecureAvic? No, as above, if the guest tries any silly business the host will get a VMEXIT_INVALID, no matter if using the feature *requires* ALLOWED_SEV_FEATURES to be enabled and explicitly allow it (currently PmcVirtualization or SecureAvic). Kim
On Tue, Aug 20, 2024, Kim Phillips wrote: > On 8/19/24 5:23 PM, Sean Christopherson wrote: > > On Mon, Aug 19, 2024, Kim Phillips wrote: > > > but since commit ac5c48027bac ("KVM: SEV: publish supported VMSA features"), > > > userspace can retrieve sev_supported_vmsa_features via an ioctl. > > > > > > > And based on this blurb: > > > > > > > > Some SEV features can only be used if the Allowed SEV Features Mask is enabled, > > > > and the mask is configured to permit the corresponding feature. If the Allowed > > > > SEV Features Mask is not enabled, these features are not available (see SEV_FEATURES > > > > in Appendix B, Table B-4). > > > > > > > > and the appendix, this only applies to PmcVirtualization and SecureAvic. Adding > > > > that info in the changelog would be *very* helpful. > > > > > > Ok, how about adding: > > > > > > "The PmcVirtualization and SecureAvic features explicitly require > > > ALLOWED_SEV_FEATURES to enable them before they can be used." > > > > > > > And I see that SVM_SEV_FEAT_DEBUG_SWAP, a.k.a. DebugVirtualization, is a guest > > > > controlled feature and doesn't honor ALLOWED_SEV_FEATURES. Doesn't that mean > > > > sev_vcpu_has_debug_swap() is broken, i.e. that KVM must assume the guest can > > > > DebugVirtualization on and off at will? Or am I missing something? > > > > > > My understanding is that users control KVM's DEBUG_SWAP setting > > > with a module parameter since commit 4dd5ecacb9a4 ("KVM: SEV: allow > > > SEV-ES DebugSwap again"). If the module parameter is not set, with > > > this patch, VMRUN will fail since the host doesn't allow DEBUG_SWAP. > > > > But that's just KVM's view of vmsa_features. With SNP's wonderful > > SVM_VMGEXIT_AP_CREATE, can't the guest create a VMSA with whatever sev_features > > it wants, so long as they aren't host-controllable, i.e. aren't PmcVirtualization > > or SecureAvic? > > No, as above, if the guest tries any silly business the host will > get a VMEXIT_INVALID, no matter if using the feature *requires* > ALLOWED_SEV_FEATURES to be enabled and explicitly allow it (currently > PmcVirtualization or SecureAvic). Oooh, I finally get it. PmcVirtualization and SecureAvic require an opt-in via ALLOWED_SEV_FEATURES, i.e. are off-by-default, whereas all other features are effectively on-by-default, but still honor ALLOWED_SEV_FEATURES.
diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h index f0dea3750ca9..59516ad2028b 100644 --- a/arch/x86/include/asm/svm.h +++ b/arch/x86/include/asm/svm.h @@ -158,7 +158,9 @@ struct __attribute__ ((__packed__)) vmcb_control_area { u64 avic_physical_id; /* Offset 0xf8 */ u8 reserved_7[8]; u64 vmsa_pa; /* Used for an SEV-ES guest */ - u8 reserved_8[720]; + u8 reserved_8[40]; + u64 allowed_sev_features; /* Offset 0x138 */ + u8 reserved_9[672]; /* * Offset 0x3e0, 32 bytes reserved * for use by hypervisor/software. @@ -294,6 +296,8 @@ static_assert((X2AVIC_MAX_PHYSICAL_ID & AVIC_PHYSICAL_MAX_INDEX_MASK) == X2AVIC_ (SVM_SEV_FEAT_RESTRICTED_INJECTION | \ SVM_SEV_FEAT_ALTERNATE_INJECTION) +#define VMCB_ALLOWED_SEV_FEATURES_VALID BIT_ULL(63) + struct vmcb_seg { u16 selector; u16 attrib; diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index a16c873b3232..d12b4d615b32 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -899,6 +899,7 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm) static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu, int *error) { + struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; struct sev_data_launch_update_vmsa vmsa; struct vcpu_svm *svm = to_svm(vcpu); int ret; @@ -908,6 +909,10 @@ static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu, return -EINVAL; } + if (cpu_feature_enabled(X86_FEATURE_ALLOWED_SEV_FEATURES)) + svm->vmcb->control.allowed_sev_features = VMCB_ALLOWED_SEV_FEATURES_VALID | + sev->vmsa_features; + /* Perform some pre-encryption checks against the VMSA */ ret = sev_es_sync_vmsa(svm); if (ret)