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

[PATCH v3 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 [1].  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/2 adds support to detect the feature.

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

[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

Based on 6.14-rc1.

v3:
 - Assign allowed_sev_features based on user-provided vmsa_features mask (Sean)
 - Users now have to explicitly opt-in with a qemu "allowed-sev-features=on" switch.
 - Rebased on top of 6.14-rc1 and reworked authorship chain (tglx)

v2: https://lore.kernel.org/lkml/20240822221938.2192109-1-kim.phillips@amd.com/
 - Added some SEV_FEATURES require to be explicitly allowed by
   ALLOWED_SEV_FEATURES wording (Sean).
 - Added Nikunj's Reviewed-by.

v1: https://lore.kernel.org/lkml/20240802015732.3192877-3-kim.phillips@amd.com/

Kim Phillips (1):
  KVM: SEV: Configure "ALLOWED_SEV_FEATURES" VMCB Field

Kishon Vijay Abraham I (1):
  x86/cpufeatures: Add "Allowed SEV Features" Feature

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

-- 
2.43.0

[PATCH v3 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 508c0dad116b..a80a4164d110 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -453,6 +453,7 @@
 #define X86_FEATURE_DEBUG_SWAP		(19*32+14) /* "debug_swap" SEV-ES full debug state swap support */
 #define X86_FEATURE_RMPREAD		(19*32+21) /* RMPREAD instruction */
 #define X86_FEATURE_SEGMENTED_RMP	(19*32+23) /* Segmented RMP support */
+#define X86_FEATURE_ALLOWED_SEV_FEATURES (19*32+27) /* Allowed SEV Features */
 #define X86_FEATURE_SVSM		(19*32+28) /* "svsm" SVSM present */
 #define X86_FEATURE_HV_INUSE_WR_ALLOWED	(19*32+30) /* Allow Write to in-use hypervisor-owned pages */
 
-- 
2.43.0

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

[Tom Lendacky]

On 2/7/25 17:34, 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.
> 
> Signed-off-by: Kishon Vijay Abraham I <kvijayab@amd.com>
> Signed-off-by: Kim Phillips <kim.phillips@amd.com>

Reviewed-by: Tom Lendacky <thomas.lendacky@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 508c0dad116b..a80a4164d110 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -453,6 +453,7 @@
>  #define X86_FEATURE_DEBUG_SWAP		(19*32+14) /* "debug_swap" SEV-ES full debug state swap support */
>  #define X86_FEATURE_RMPREAD		(19*32+21) /* RMPREAD instruction */
>  #define X86_FEATURE_SEGMENTED_RMP	(19*32+23) /* Segmented RMP support */
> +#define X86_FEATURE_ALLOWED_SEV_FEATURES (19*32+27) /* Allowed SEV Features */
>  #define X86_FEATURE_SVSM		(19*32+28) /* "svsm" SVSM present */
>  #define X86_FEATURE_HV_INUSE_WR_ALLOWED	(19*32+30) /* Allow Write to in-use hypervisor-owned pages */
>  

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

[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 [1].  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.

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.

Some SEV_FEATURES - currently PmcVirtualization and SecureAvic
(see Appendix B, Table B-4) - 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.

[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

Co-developed-by: Kishon Vijay Abraham I <kvijayab@amd.com>
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 |  5 ++++-
 arch/x86/kvm/svm/sev.c     | 17 +++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
index e2fac21471f5..6d94a727cc1a 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.
@@ -289,6 +291,7 @@ static_assert((X2AVIC_MAX_PHYSICAL_ID & AVIC_PHYSICAL_MAX_INDEX_MASK) == X2AVIC_
 #define SVM_SEV_FEAT_RESTRICTED_INJECTION		BIT(3)
 #define SVM_SEV_FEAT_ALTERNATE_INJECTION		BIT(4)
 #define SVM_SEV_FEAT_DEBUG_SWAP				BIT(5)
+#define SVM_SEV_FEAT_ALLOWED_SEV_FEATURES		BIT_ULL(63)
 
 #define SVM_SEV_FEAT_INT_INJ_MODES		\
 	(SVM_SEV_FEAT_RESTRICTED_INJECTION |	\
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index a2a794c32050..a9e16792cac0 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -894,9 +894,19 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
 	return 0;
 }
 
+static u64 allowed_sev_features(struct kvm_sev_info *sev)
+{
+	if (cpu_feature_enabled(X86_FEATURE_ALLOWED_SEV_FEATURES) &&
+	    (sev->vmsa_features & SVM_SEV_FEAT_ALLOWED_SEV_FEATURES))
+		return sev->vmsa_features;
+
+	return 0;
+}
+
 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;
@@ -906,6 +916,8 @@ static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu,
 		return -EINVAL;
 	}
 
+	svm->vmcb->control.allowed_sev_features = allowed_sev_features(sev);
+
 	/* Perform some pre-encryption checks against the VMSA */
 	ret = sev_es_sync_vmsa(svm);
 	if (ret)
@@ -2447,6 +2459,8 @@ static int snp_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
 		struct vcpu_svm *svm = to_svm(vcpu);
 		u64 pfn = __pa(svm->sev_es.vmsa) >> PAGE_SHIFT;
 
+		svm->vmcb->control.allowed_sev_features = allowed_sev_features(sev);
+
 		ret = sev_es_sync_vmsa(svm);
 		if (ret)
 			return ret;
@@ -3069,6 +3083,9 @@ void __init sev_hardware_setup(void)
 	sev_supported_vmsa_features = 0;
 	if (sev_es_debug_swap_enabled)
 		sev_supported_vmsa_features |= SVM_SEV_FEAT_DEBUG_SWAP;
+
+	if (sev_es_enabled && cpu_feature_enabled(X86_FEATURE_ALLOWED_SEV_FEATURES))
+		sev_supported_vmsa_features |= SVM_SEV_FEAT_ALLOWED_SEV_FEATURES;
 }
 
 void sev_hardware_unsetup(void)
-- 
2.43.0

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

[Tom Lendacky]

On 2/7/25 17:34, Kim Phillips wrote:
> 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].  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.
> 
> 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.
> 
> Some SEV_FEATURES - currently PmcVirtualization and SecureAvic
> (see Appendix B, Table B-4) - 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.
> 
> [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
> 
> Co-developed-by: Kishon Vijay Abraham I <kvijayab@amd.com>
> 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 |  5 ++++-
>  arch/x86/kvm/svm/sev.c     | 17 +++++++++++++++++
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> index e2fac21471f5..6d94a727cc1a 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.
> @@ -289,6 +291,7 @@ static_assert((X2AVIC_MAX_PHYSICAL_ID & AVIC_PHYSICAL_MAX_INDEX_MASK) == X2AVIC_
>  #define SVM_SEV_FEAT_RESTRICTED_INJECTION		BIT(3)
>  #define SVM_SEV_FEAT_ALTERNATE_INJECTION		BIT(4)
>  #define SVM_SEV_FEAT_DEBUG_SWAP				BIT(5)
> +#define SVM_SEV_FEAT_ALLOWED_SEV_FEATURES		BIT_ULL(63)

Hmmm... I believe it is safe to define this bit value, as the Allowed
SEV features VMCB field shows bits 61:0 being used for the allowed
features mask and we know that the SEV_FEATURES field is used in the SEV
Features MSR left-shifted 2 bits, so we only expect bits 61:0 to be used
and bits 62 and 63 will always be reserved. But, given that I think we
need two functions:

- get_allowed_sev_features()
  keeping it as you have it below, where it returns the
  sev->vmsa_features bitmap if SVM_SEV_FEAT_ALLOWED_SEV_FEATURES is set
  or 0 if SVM_SEV_FEAT_ALLOWED_SEV_FEATURES is not set.

- get_vmsa_sev_features()
  which removes the SVM_SEV_FEAT_ALLOWED_SEV_FEATURES bit, since it is
  not defined in the VMSA SEV_FEATURES definition.

>  
>  #define SVM_SEV_FEAT_INT_INJ_MODES		\
>  	(SVM_SEV_FEAT_RESTRICTED_INJECTION |	\
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index a2a794c32050..a9e16792cac0 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -894,9 +894,19 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
>  	return 0;
>  }
>  
> +static u64 allowed_sev_features(struct kvm_sev_info *sev)
> +{
> +	if (cpu_feature_enabled(X86_FEATURE_ALLOWED_SEV_FEATURES) &&

Not sure if the cpu_feature_enabled() check is necessary, as init should
have failed if SVM_SEV_FEAT_ALLOWED_SEV_FEATURES wasn't set in
sev_supported_vmsa_features.

Thanks,
Tom

> +	    (sev->vmsa_features & SVM_SEV_FEAT_ALLOWED_SEV_FEATURES))
> +		return sev->vmsa_features;
> +
> +	return 0;
> +}
> +
>  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;
> @@ -906,6 +916,8 @@ static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu,
>  		return -EINVAL;
>  	}
>  
> +	svm->vmcb->control.allowed_sev_features = allowed_sev_features(sev);
> +
>  	/* Perform some pre-encryption checks against the VMSA */
>  	ret = sev_es_sync_vmsa(svm);
>  	if (ret)
> @@ -2447,6 +2459,8 @@ static int snp_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  		struct vcpu_svm *svm = to_svm(vcpu);
>  		u64 pfn = __pa(svm->sev_es.vmsa) >> PAGE_SHIFT;
>  
> +		svm->vmcb->control.allowed_sev_features = allowed_sev_features(sev);
> +
>  		ret = sev_es_sync_vmsa(svm);
>  		if (ret)
>  			return ret;
> @@ -3069,6 +3083,9 @@ void __init sev_hardware_setup(void)
>  	sev_supported_vmsa_features = 0;
>  	if (sev_es_debug_swap_enabled)
>  		sev_supported_vmsa_features |= SVM_SEV_FEAT_DEBUG_SWAP;
> +
> +	if (sev_es_enabled && cpu_feature_enabled(X86_FEATURE_ALLOWED_SEV_FEATURES))
> +		sev_supported_vmsa_features |= SVM_SEV_FEAT_ALLOWED_SEV_FEATURES;
>  }
>  
>  void sev_hardware_unsetup(void)

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

[Sean Christopherson]

On Mon, Feb 10, 2025, Tom Lendacky wrote:
> On 2/7/25 17:34, Kim Phillips wrote:
> > @@ -289,6 +291,7 @@ static_assert((X2AVIC_MAX_PHYSICAL_ID & AVIC_PHYSICAL_MAX_INDEX_MASK) == X2AVIC_
> >  #define SVM_SEV_FEAT_RESTRICTED_INJECTION		BIT(3)
> >  #define SVM_SEV_FEAT_ALTERNATE_INJECTION		BIT(4)
> >  #define SVM_SEV_FEAT_DEBUG_SWAP				BIT(5)
> > +#define SVM_SEV_FEAT_ALLOWED_SEV_FEATURES		BIT_ULL(63)
> 
> Hmmm... I believe it is safe to define this bit value, as the Allowed
> SEV features VMCB field shows bits 61:0 being used for the allowed
> features mask and we know that the SEV_FEATURES field is used in the SEV
> Features MSR left-shifted 2 bits, so we only expect bits 61:0 to be used
> and bits 62 and 63 will always be reserved. But, given that I think we
> need two functions:
> 
> - get_allowed_sev_features()
>   keeping it as you have it below, where it returns the
>   sev->vmsa_features bitmap if SVM_SEV_FEAT_ALLOWED_SEV_FEATURES is set
>   or 0 if SVM_SEV_FEAT_ALLOWED_SEV_FEATURES is not set.
> 
> - get_vmsa_sev_features()
>   which removes the SVM_SEV_FEAT_ALLOWED_SEV_FEATURES bit, since it is
>   not defined in the VMSA SEV_FEATURES definition.

Or just don't add wrappers that do more harm than good?

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index a9e16792cac0..4d0b5a020b65 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -894,15 +894,6 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
        return 0;
 }
 
-static u64 allowed_sev_features(struct kvm_sev_info *sev)
-{
-       if (cpu_feature_enabled(X86_FEATURE_ALLOWED_SEV_FEATURES) &&
-           (sev->vmsa_features & SVM_SEV_FEAT_ALLOWED_SEV_FEATURES))
-               return sev->vmsa_features;
-
-       return 0;
-}
-
 static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu,
                                    int *error)
 {
@@ -916,7 +907,8 @@ static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu,
                return -EINVAL;
        }
 
-       svm->vmcb->control.allowed_sev_features = allowed_sev_features(sev);
+       if (cpu_feature_enabled(X86_FEATURE_ALLOWED_SEV_FEATURES))
+               svm->vmcb->control.allowed_sev_features = sev->vmsa_features;
 
        /* Perform some pre-encryption checks against the VMSA */
        ret = sev_es_sync_vmsa(svm);
@@ -2459,7 +2451,8 @@ static int snp_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
                struct vcpu_svm *svm = to_svm(vcpu);
                u64 pfn = __pa(svm->sev_es.vmsa) >> PAGE_SHIFT;
 
-               svm->vmcb->control.allowed_sev_features = allowed_sev_features(sev);
+               if (cpu_feature_enabled(X86_FEATURE_ALLOWED_SEV_FEATURES))
+                       svm->vmcb->control.allowed_sev_features = sev->vmsa_features;
 
                ret = sev_es_sync_vmsa(svm);
                if (ret)

> >  #define SVM_SEV_FEAT_INT_INJ_MODES		\
> >  	(SVM_SEV_FEAT_RESTRICTED_INJECTION |	\
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index a2a794c32050..a9e16792cac0 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -894,9 +894,19 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
> >  	return 0;
> >  }
> >  
> > +static u64 allowed_sev_features(struct kvm_sev_info *sev)
> > +{
> > +	if (cpu_feature_enabled(X86_FEATURE_ALLOWED_SEV_FEATURES) &&
> 
> Not sure if the cpu_feature_enabled() check is necessary, as init should
> have failed if SVM_SEV_FEAT_ALLOWED_SEV_FEATURES wasn't set in
> sev_supported_vmsa_features.

Two things missing from this series:

 1: KVM enforcement.  No way is KVM going to rely on userspace to opt-in to
    preventing the guest from enabling features.

 2: Backwards compatilibity if KVM unconditionally enforces ALLOWED_SEV_FEATURES.
    Although maybe there's nothing to do here?  I vaguely recall all of the gated
    features being unsupported, or something...

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

[Kim Phillips]

On 2/11/25 3:46 PM, Sean Christopherson wrote:
> On Mon, Feb 10, 2025, Tom Lendacky wrote:
>> On 2/7/25 17:34, Kim Phillips wrote:
>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>> index a2a794c32050..a9e16792cac0 100644
>>> --- a/arch/x86/kvm/svm/sev.c
>>> +++ b/arch/x86/kvm/svm/sev.c
>>> @@ -894,9 +894,19 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
>>>   	return 0;
>>>   }
>>>   
>>> +static u64 allowed_sev_features(struct kvm_sev_info *sev)
>>> +{
>>> +	if (cpu_feature_enabled(X86_FEATURE_ALLOWED_SEV_FEATURES) &&
>>
>> Not sure if the cpu_feature_enabled() check is necessary, as init should
>> have failed if SVM_SEV_FEAT_ALLOWED_SEV_FEATURES wasn't set in
>> sev_supported_vmsa_features.
> 
> Two things missing from this series:
> 
>   1: KVM enforcement.  No way is KVM going to rely on userspace to opt-in to
>      preventing the guest from enabling features.
>   2: Backwards compatilibity if KVM unconditionally enforces ALLOWED_SEV_FEATURES.
>      Although maybe there's nothing to do here?  I vaguely recall all of the gated
>      features being unsupported, or something...

This contradicts your review comment from the previous version of the series [1].

If KVM enforces ALLOWED_SEV_FEATURES, it can break existing VMs, thus
the explicit userspace allowed-sev-features=on opt-in [2].

Thanks,

Kim

[1] https://lore.kernel.org/kvm/ZsfKYHFkWA-Rh23C@google.com/
[2] https://lore.kernel.org/kvm/20250207233327.130770-1-kim.phillips@amd.com/

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

[Sean Christopherson]

On Thu, Feb 13, 2025, Kim Phillips wrote:
> On 2/11/25 3:46 PM, Sean Christopherson wrote:
> > On Mon, Feb 10, 2025, Tom Lendacky wrote:
> > > On 2/7/25 17:34, Kim Phillips wrote:
> > > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > > > index a2a794c32050..a9e16792cac0 100644
> > > > --- a/arch/x86/kvm/svm/sev.c
> > > > +++ b/arch/x86/kvm/svm/sev.c
> > > > @@ -894,9 +894,19 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
> > > >   	return 0;
> > > >   }
> > > > +static u64 allowed_sev_features(struct kvm_sev_info *sev)
> > > > +{
> > > > +	if (cpu_feature_enabled(X86_FEATURE_ALLOWED_SEV_FEATURES) &&
> > > 
> > > Not sure if the cpu_feature_enabled() check is necessary, as init should
> > > have failed if SVM_SEV_FEAT_ALLOWED_SEV_FEATURES wasn't set in
> > > sev_supported_vmsa_features.
> > 
> > Two things missing from this series:
> > 
> >   1: KVM enforcement.  No way is KVM going to rely on userspace to opt-in to
> >      preventing the guest from enabling features.
> >   2: Backwards compatilibity if KVM unconditionally enforces ALLOWED_SEV_FEATURES.
> >      Although maybe there's nothing to do here?  I vaguely recall all of the gated
> >      features being unsupported, or something...
> 
> This contradicts your review comment from the previous version of the series [1].

First off, my comment was anything but decisive.  I don't see how anyone can read
this and come away thinking "this is exactly what Sean wants".

  This may need additional uAPI so that userspace can opt-in.  Dunno.  I hope guests
  aren't abusing features, but IIUC, flipping this on has the potential to break
  existing VMs, correct?

Second, there's a clear question there that went unanswered.  Respond to questions
and elaborate as needed until we're all on the same page.  Don't just send patches.

Third, letting userspace opt-in to something doesn't necessarily mean giving
userspace full control.  Which is the entire reason I asked the question about
whether or not this can break userspace.  E.g. we can likely get away with only
making select features opt-in, and enforcing everything else by default.

I don't think RESTRICTED_INJECTION or ALTERNATE_INJECTION can work without KVM
cooperation, so enforcing those shouldn't break anything.

It's still not clear to me that we don't have a bug with DEBUG_SWAP.  AIUI,
DEBUG_SWAP is allowed by default.  I.e. if ALLOWED_FEATURES is unsupported, then
the guest can use DEBUG_SWAP via SVM_VMGEXIT_AP_CREATE without KVM's knowledge.

So _maybe_ we have to let userspace opt-in to enforcing DEBUG_SWAP, but I suspect
we can get away with fully enabling ALLOWED_FEATURES without userspace's blessing.

> If KVM enforces ALLOWED_SEV_FEATURES, it can break existing VMs, thus
> the explicit userspace allowed-sev-features=on opt-in [2].
> 
> Thanks,
> 
> Kim
> 
> [1] https://lore.kernel.org/kvm/ZsfKYHFkWA-Rh23C@google.com/
> [2] https://lore.kernel.org/kvm/20250207233327.130770-1-kim.phillips@amd.com/

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

[Kim Phillips]

On 2/13/25 6:55 PM, Sean Christopherson wrote:
> On Thu, Feb 13, 2025, Kim Phillips wrote:
>> On 2/11/25 3:46 PM, Sean Christopherson wrote:
>>> On Mon, Feb 10, 2025, Tom Lendacky wrote:
>>>> On 2/7/25 17:34, Kim Phillips wrote:
>>>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>>>> index a2a794c32050..a9e16792cac0 100644
>>>>> --- a/arch/x86/kvm/svm/sev.c
>>>>> +++ b/arch/x86/kvm/svm/sev.c
>>>>> @@ -894,9 +894,19 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
>>>>>    	return 0;
>>>>>    }
>>>>> +static u64 allowed_sev_features(struct kvm_sev_info *sev)
>>>>> +{
>>>>> +	if (cpu_feature_enabled(X86_FEATURE_ALLOWED_SEV_FEATURES) &&
>>>>
>>>> Not sure if the cpu_feature_enabled() check is necessary, as init should
>>>> have failed if SVM_SEV_FEAT_ALLOWED_SEV_FEATURES wasn't set in
>>>> sev_supported_vmsa_features.
>>>
>>> Two things missing from this series:
>>>
>>>    1: KVM enforcement.  No way is KVM going to rely on userspace to opt-in to
>>>       preventing the guest from enabling features.
>>>    2: Backwards compatilibity if KVM unconditionally enforces ALLOWED_SEV_FEATURES.
>>>       Although maybe there's nothing to do here?  I vaguely recall all of the gated
>>>       features being unsupported, or something...
>>
>> This contradicts your review comment from the previous version of the series [1].
> 
> First off, my comment was anything but decisive.  I don't see how anyone can read
> this and come away thinking "this is exactly what Sean wants".
> 
>    This may need additional uAPI so that userspace can opt-in.  Dunno.  I hope guests
>    aren't abusing features, but IIUC, flipping this on has the potential to break
>    existing VMs, correct?
> 
> Second, there's a clear question there that went unanswered.  Respond to questions
> and elaborate as needed until we're all on the same page.  Don't just send patches.
> 
> Third, letting userspace opt-in to something doesn't necessarily mean giving
> userspace full control.  Which is the entire reason I asked the question about
> whether or not this can break userspace.  E.g. we can likely get away with only
> making select features opt-in, and enforcing everything else by default.
> 
> I don't think RESTRICTED_INJECTION or ALTERNATE_INJECTION can work without KVM
> cooperation, so enforcing those shouldn't break anything.
> 
> It's still not clear to me that we don't have a bug with DEBUG_SWAP.  AIUI,
> DEBUG_SWAP is allowed by default.  I.e. if ALLOWED_FEATURES is unsupported, then
> the guest can use DEBUG_SWAP via SVM_VMGEXIT_AP_CREATE without KVM's knowledge.
> 
> So _maybe_ we have to let userspace opt-in to enforcing DEBUG_SWAP, but I suspect
> we can get away with fully enabling ALLOWED_FEATURES without userspace's blessing.

If I hardcode DEBUG_SWAP (bit 5) in the vmsa->sev_features assignment
in wakeup_cpu_via_vmgexit(), such guest boots successfully with the
kvm_amd module's debug_swap parameter set.

The guest *doesn't* boot if I also turn on allowed_sev_features=1 with
qemu and this patchseries.

So, the answer is yes, always enforcing ALLOWED_SEV_FEATURES does break
existing guests, thus the userspace opt-in for it.

Thanks,

Kim

> 
>> If KVM enforces ALLOWED_SEV_FEATURES, it can break existing VMs, thus
>> the explicit userspace allowed-sev-features=on opt-in [2].
>>
>> Thanks,
>>
>> Kim
>>
>> [1] https://lore.kernel.org/kvm/ZsfKYHFkWA-Rh23C@google.com/
>> [2] https://lore.kernel.org/kvm/20250207233327.130770-1-kim.phillips@amd.com/

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

[Sean Christopherson]

On Fri, Feb 14, 2025, Kim Phillips wrote:
> On 2/13/25 6:55 PM, Sean Christopherson wrote:
> > On Thu, Feb 13, 2025, Kim Phillips wrote:
> > > > > Not sure if the cpu_feature_enabled() check is necessary, as init should
> > > > > have failed if SVM_SEV_FEAT_ALLOWED_SEV_FEATURES wasn't set in
> > > > > sev_supported_vmsa_features.
> > > > 
> > > > Two things missing from this series:
> > > > 
> > > >    1: KVM enforcement.  No way is KVM going to rely on userspace to opt-in to
> > > >       preventing the guest from enabling features.
> > > >    2: Backwards compatilibity if KVM unconditionally enforces ALLOWED_SEV_FEATURES.
> > > >       Although maybe there's nothing to do here?  I vaguely recall all of the gated
> > > >       features being unsupported, or something...
> > > 
> > > This contradicts your review comment from the previous version of the series [1].
> > 
> > First off, my comment was anything but decisive.  I don't see how anyone can read
> > this and come away thinking "this is exactly what Sean wants".
> > 
> >    This may need additional uAPI so that userspace can opt-in.  Dunno.  I hope guests
> >    aren't abusing features, but IIUC, flipping this on has the potential to break
> >    existing VMs, correct?
> > 
> > Second, there's a clear question there that went unanswered.  Respond to questions
> > and elaborate as needed until we're all on the same page.  Don't just send patches.
> > 
> > Third, letting userspace opt-in to something doesn't necessarily mean giving
> > userspace full control.  Which is the entire reason I asked the question about
> > whether or not this can break userspace.  E.g. we can likely get away with only
> > making select features opt-in, and enforcing everything else by default.
> > 
> > I don't think RESTRICTED_INJECTION or ALTERNATE_INJECTION can work without KVM
> > cooperation, so enforcing those shouldn't break anything.
> > 
> > It's still not clear to me that we don't have a bug with DEBUG_SWAP.  AIUI,
> > DEBUG_SWAP is allowed by default.  I.e. if ALLOWED_FEATURES is unsupported, then
> > the guest can use DEBUG_SWAP via SVM_VMGEXIT_AP_CREATE without KVM's knowledge.
> > 
> > So _maybe_ we have to let userspace opt-in to enforcing DEBUG_SWAP, but I suspect
> > we can get away with fully enabling ALLOWED_FEATURES without userspace's blessing.
> 
> If I hardcode DEBUG_SWAP (bit 5) in the vmsa->sev_features assignment
> in wakeup_cpu_via_vmgexit(), such guest boots successfully with the
> kvm_amd module's debug_swap parameter set.
> 
> The guest *doesn't* boot if I also turn on allowed_sev_features=1 with
> qemu and this patchseries.
> 
> So, the answer is yes, always enforcing ALLOWED_SEV_FEATURES does break
> existing guests, thus the userspace opt-in for it.

That is not an "existing" guest.  That's a deliberately misconfigured guest that
serves as testcase/reproducer.  IIUC, the BSP can't enable DEBUG_SWAP through a
backdoor, so I don't think it's at all sane/reasonable for the guest to expect
that enabling DEBUG_SWAP only on APs will function.  Ah, and KVM will still set
the DR7 intercepts, i.e. the guest can't read/write DR7, so this is definitely a
nonsensical/unsupported configuration.

So unless I've missed something, KVM can unconditionally enforce ALLOWED_SEV_FEATURES.

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

[Naveen N Rao]

On Thu, Feb 13, 2025 at 04:55:13PM -0800, Sean Christopherson wrote:
> On Thu, Feb 13, 2025, Kim Phillips wrote:
> > On 2/11/25 3:46 PM, Sean Christopherson wrote:
> > > On Mon, Feb 10, 2025, Tom Lendacky wrote:
> > > > On 2/7/25 17:34, Kim Phillips wrote:
> 
> Third, letting userspace opt-in to something doesn't necessarily mean giving
> userspace full control.  Which is the entire reason I asked the question about
> whether or not this can break userspace.  E.g. we can likely get away with only
> making select features opt-in, and enforcing everything else by default.
> 
> I don't think RESTRICTED_INJECTION or ALTERNATE_INJECTION can work without KVM
> cooperation, so enforcing those shouldn't break anything.
> 
> It's still not clear to me that we don't have a bug with DEBUG_SWAP.  AIUI,
> DEBUG_SWAP is allowed by default.  I.e. if ALLOWED_FEATURES is unsupported, then
> the guest can use DEBUG_SWAP via SVM_VMGEXIT_AP_CREATE without KVM's knowledge.

In sev_es_prepare_switch_to_guest(), we save host debug register state 
(DR0-DR3) only if KVM is aware of DEBUG_SWAP being enabled in the guest 
(via vmsa_features). So, from what I can tell, it looks like the guest 
will end up overwriting host state if it enables DEBUG_SWAP without 
KVM's knowledge?

Not sure if that's reason enough to enforce ALLOWED_SEV_FEATURES for 
DEBUG_SWAP :)

If ALLOWED_SEV_FEATURES is not supported, we may still have to 
unconditionally save the host DR0-DR3 registers.


- Naveen

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

[Sean Christopherson]

On Mon, Feb 17, 2025, Naveen N Rao wrote:
> On Thu, Feb 13, 2025 at 04:55:13PM -0800, Sean Christopherson wrote:
> > On Thu, Feb 13, 2025, Kim Phillips wrote:
> > > On 2/11/25 3:46 PM, Sean Christopherson wrote:
> > > > On Mon, Feb 10, 2025, Tom Lendacky wrote:
> > > > > On 2/7/25 17:34, Kim Phillips wrote:
> > 
> > Third, letting userspace opt-in to something doesn't necessarily mean giving
> > userspace full control.  Which is the entire reason I asked the question about
> > whether or not this can break userspace.  E.g. we can likely get away with only
> > making select features opt-in, and enforcing everything else by default.
> > 
> > I don't think RESTRICTED_INJECTION or ALTERNATE_INJECTION can work without KVM
> > cooperation, so enforcing those shouldn't break anything.
> > 
> > It's still not clear to me that we don't have a bug with DEBUG_SWAP.  AIUI,
> > DEBUG_SWAP is allowed by default.  I.e. if ALLOWED_FEATURES is unsupported, then
> > the guest can use DEBUG_SWAP via SVM_VMGEXIT_AP_CREATE without KVM's knowledge.
> 
> In sev_es_prepare_switch_to_guest(), we save host debug register state 
> (DR0-DR3) only if KVM is aware of DEBUG_SWAP being enabled in the guest 
> (via vmsa_features). So, from what I can tell, it looks like the guest 
> will end up overwriting host state if it enables DEBUG_SWAP without 
> KVM's knowledge?

Yes, that's what I'm effectively "asking".

> Not sure if that's reason enough to enforce ALLOWED_SEV_FEATURES for 
> DEBUG_SWAP :)
> 
> If ALLOWED_SEV_FEATURES is not supported, we may still have to 
> unconditionally save the host DR0-DR3 registers.

Yes, that's my understanding of the situation.  If the CPU supports DEBUG_SWAP,
KVM must assume the guest can enable it without KVM's knowledge.

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

[Sean Christopherson]

On Mon, Feb 17, 2025, Naveen N Rao wrote:
> On Thu, Feb 13, 2025 at 04:55:13PM -0800, Sean Christopherson wrote:
> > On Thu, Feb 13, 2025, Kim Phillips wrote:
> > > On 2/11/25 3:46 PM, Sean Christopherson wrote:
> > > > On Mon, Feb 10, 2025, Tom Lendacky wrote:
> > > > > On 2/7/25 17:34, Kim Phillips wrote:
> > 
> > Third, letting userspace opt-in to something doesn't necessarily mean giving
> > userspace full control.  Which is the entire reason I asked the question about
> > whether or not this can break userspace.  E.g. we can likely get away with only
> > making select features opt-in, and enforcing everything else by default.
> > 
> > I don't think RESTRICTED_INJECTION or ALTERNATE_INJECTION can work without KVM
> > cooperation, so enforcing those shouldn't break anything.
> > 
> > It's still not clear to me that we don't have a bug with DEBUG_SWAP.  AIUI,
> > DEBUG_SWAP is allowed by default.  I.e. if ALLOWED_FEATURES is unsupported, then
> > the guest can use DEBUG_SWAP via SVM_VMGEXIT_AP_CREATE without KVM's knowledge.
> 
> In sev_es_prepare_switch_to_guest(), we save host debug register state 
> (DR0-DR3) only if KVM is aware of DEBUG_SWAP being enabled in the guest 
> (via vmsa_features). So, from what I can tell, it looks like the guest 
> will end up overwriting host state if it enables DEBUG_SWAP without 
> KVM's knowledge?
> 
> Not sure if that's reason enough to enforce ALLOWED_SEV_FEATURES for 
> DEBUG_SWAP :)
> 
> If ALLOWED_SEV_FEATURES is not supported, we may still have to 
> unconditionally save the host DR0-DR3 registers.

Aha!  We do not.  The existing KVM code is actually flawed in the opposite
direction, in that saving host DR0..DR3 during sev_es_prepare_switch_to_guest()
is superfluous.

DR7 is reset on #VMEXIT (swap type "C"), and so KVM only needs to ensure DR0..DR3
are restored before loading DR7 with the host's value.  KVM takes care of that in
common x86 code via hw_breakpoint_restore().

However, there is still a bug, as the AMD-specific *masks* are not restored.  KVM
doesn't support MSR_F16H_DRx_ADDR_MASK emulation or passthrough for normal guests,
so the guest can't set those values either, i.e. will get a #VC.  But the masks
do need to be restored, because the CPU will clobber them with '0' when DebugSwap
is enabled.

And of course the DR0..DR3 loads in sev_es_prepare_switch_to_guest() are a
complete waste of cycles.

*sigh*

Ugh, it gets worse.  The awfulness goes in both direction.  Unless I've misunderstood
how this all works, just because *KVM* enables DebugSwap for the BSP doesn't mean
the guest will enable DebugSwap for APs.  And so KVM _can't_ rely on DebugSwap to
actually swap DR0..DR3, because KVM has no way of knowing whether or not a given
vCPU actually has it enabled.  And that's true even when SEV_ALLOWED_FEATURES
comes along, which means treating DR0..DR3 as swap type B is utterly worthless.

What a mess.  I'll send a small series to try and clean things up.

Also, I told y'all so: https://lore.kernel.org/all/YWnbfCet84Vup6q9@google.com

P.S. Can someone please get the APM updated to actually explain WTF enabling
     Debug Virtualization in SEV_FEATURES does?  The APM does not at all match
     what y'all have told me.  A sane reading of the APM would be that DRs are
     *unconditionally* swap type B when DebugSwap is supported, whereas I've been
     told from the very beginning[*] that treating them as type B requires software
     enabling:

         AMD Milan introduces support for the swapping, as type 'B', of DR[0-3]
         and DR[0-3]_ADDR_MASK registers. It requires that SEV_FEATURES[5] be set
         in the VMSA.

[*] https://lore.kernel.org/all/20221201021948.9259-3-aik@amd.com