[PATCH 0/2] KVM: SEV: Add support for the ALLOWED_SEV_FEATURES feature

[PATCH 0/2] KVM: SEV: Add support for the ALLOWED_SEV_FEATURES feature

[Kim Phillips]

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. The ALLOWED_SEV_FEATURES 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.

Patch 1 adds support to detect the feature.

Patch 2 configures the ALLOWED_SEV_FEATURES field in the VMCB
according to the features the hypervisor supports.

Based on tip/master commit a2767e7f31ad ("Merge branch into tip/master: 'x86/timers'")

Kishon Vijay Abraham I (2):
  x86/cpufeatures: Add "Allowed SEV Features" Feature
  KVM: SEV: Configure "ALLOWED_SEV_FEATURES" VMCB Field

 arch/x86/include/asm/cpufeatures.h | 1 +
 arch/x86/include/asm/svm.h         | 6 +++++-
 arch/x86/kvm/svm/sev.c             | 5 +++++
 3 files changed, 11 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH 1/2] x86/cpufeatures: Add "Allowed SEV Features" Feature

[Kim Phillips]

From: Kishon Vijay Abraham I <kvijayab@amd.com>

Add CPU feature detection for "Allowed SEV Features" to allow the
Hypervisor to enforce that SEV-ES and SEV-SNP guest VMs cannot
enable features (via SEV_FEATURES) that the Hypervisor does not
support or wish to be enabled.

Signed-off-by: Kishon Vijay Abraham I <kvijayab@amd.com>
Signed-off-by: Kim Phillips <kim.phillips@amd.com>
---
 arch/x86/include/asm/cpufeatures.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index dd4682857c12..0c73da91a041 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -447,6 +447,7 @@
 #define X86_FEATURE_V_TSC_AUX		(19*32+ 9) /* Virtual TSC_AUX */
 #define X86_FEATURE_SME_COHERENT	(19*32+10) /* AMD hardware-enforced cache coherency */
 #define X86_FEATURE_DEBUG_SWAP		(19*32+14) /* "debug_swap" AMD SEV-ES full debug state swap support */
+#define X86_FEATURE_ALLOWED_SEV_FEATURES (19*32+27) /* AMD Allowed SEV Features */
 #define X86_FEATURE_SVSM		(19*32+28) /* "svsm" SVSM present */
 
 /* AMD-defined Extended Feature 2 EAX, CPUID level 0x80000021 (EAX), word 20 */
-- 
2.34.1

Re: [PATCH 1/2] x86/cpufeatures: Add "Allowed SEV Features" Feature

[Thomas Gleixner]

On Thu, Aug 01 2024 at 20:57, Kim Phillips wrote:
> From: Kishon Vijay Abraham I <kvijayab@amd.com>
>
> Add CPU feature detection for "Allowed SEV Features" to allow the
> Hypervisor to enforce that SEV-ES and SEV-SNP guest VMs cannot
> enable features (via SEV_FEATURES) that the Hypervisor does not
> support or wish to be enabled.

Can you please add this new feature bit to the CPUID database

    https://gitlab.com/x86-cpuid.org/x86-cpuid-db

Thanks,

        tglx

[PATCH 2/2] KVM: SEV: Configure "ALLOWED_SEV_FEATURES" VMCB Field

[Kim Phillips]

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>
---
 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)
-- 
2.34.1

Re: [PATCH 2/2] KVM: SEV: Configure "ALLOWED_SEV_FEATURES" VMCB Field

[Nikunj A. Dadhania]

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)

Re: [PATCH 2/2] KVM: SEV: Configure "ALLOWED_SEV_FEATURES" VMCB Field

[Sean Christopherson]

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?

Re: [PATCH 2/2] KVM: SEV: Configure "ALLOWED_SEV_FEATURES" VMCB Field

[Kim Phillips]

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

Re: [PATCH 2/2] KVM: SEV: Configure "ALLOWED_SEV_FEATURES" VMCB Field

[Sean Christopherson]

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?

Re: [PATCH 2/2] KVM: SEV: Configure "ALLOWED_SEV_FEATURES" VMCB Field

[Kim Phillips]

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

Re: [PATCH 2/2] KVM: SEV: Configure "ALLOWED_SEV_FEATURES" VMCB Field

[Sean Christopherson]

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.