[v1] KVM: SVM: Disable AVIC on SNP-enabled system without HvInUseWrAllowed feature

[PATCH] KVM: SVM: Disable AVIC on SNP-enabled system without HvInUseWrAllowed feature

Posted by Suravee Suthikulpanit 1 month, 4 weeks ago

On SNP-enabled system, VMRUN marks AVIC Backing Page as in-use while
the guest is running for both secure and non-secure guest. This causes
any attempts to modify the RMP entries for the backing page to result in
FAIL_INUSE response. This is to ensure that the AVIC backing page is not
maliciously assigned to an SNP guest while the unencrypted guest is active.

Currently, an attempt to run AVIC guest would result in the following error:

    BUG: unable to handle page fault for address: ff3a442e549cc270
    #PF: supervisor write access in kernel mode
    #PF: error_code(0x80000003) - RMP violation
    PGD b6ee01067 P4D b6ee02067 PUD 10096d063 PMD 11c540063 PTE 80000001149cc163
    SEV-SNP: PFN 0x1149cc unassigned, dumping non-zero entries in 2M PFN region: [0x114800 - 0x114a00]
    ...

Newer AMD system is enhanced to allow hypervisor to modify RMP entries of
the backing page for non-secure guest on SNP-enabled system. This
enhancement is available when the CPUID Fn8000_001F_EAX bit 30 is set
(HvInUseWrAllowed) See the AMD64 Architecture Programmer’s Manual (APM)
Volume 2 for detail. (https://www.amd.com/content/dam/amd/en/documents/
processor-tech-docs/programmer-references/40332.pdf)

Therefore, add logic to check the new CPUID bit before enabling AVIC
on SNP-enabled system.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/include/asm/cpufeatures.h | 1 +
 arch/x86/kvm/svm/avic.c            | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index dd4682857c12..921b6de80e24 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -448,6 +448,7 @@
 #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_SVSM		(19*32+28) /* "svsm" SVSM present */
+#define X86_FEATURE_HV_INUSE_WR_ALLOWED	(19*32+30) /* Write to in-use hypervisor-owned pages allowed */
 
 /* AMD-defined Extended Feature 2 EAX, CPUID level 0x80000021 (EAX), word 20 */
 #define X86_FEATURE_NO_NESTED_DATA_BP	(20*32+ 0) /* No Nested Data Breakpoints */
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 4b74ea91f4e6..42f2caf17d6a 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -1199,6 +1199,12 @@ bool avic_hardware_setup(void)
 		return false;
 	}
 
+	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP) &&
+	    !boot_cpu_has(X86_FEATURE_HV_INUSE_WR_ALLOWED)) {
+		pr_warn("AVIC disabled: missing HvInUseWrAllowed on SNP-enabled system");
+		return false;
+	}
+
 	if (boot_cpu_has(X86_FEATURE_AVIC)) {
 		pr_info("AVIC enabled\n");
 	} else if (force_avic) {
-- 
2.34.1

Re: [PATCH] KVM: SVM: Disable AVIC on SNP-enabled system without HvInUseWrAllowed feature

Posted by Joao Martins 1 month, 4 weeks ago

On 30/09/2024 06:50, Suravee Suthikulpanit wrote:
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index dd4682857c12..921b6de80e24 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -448,6 +448,7 @@
>  #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_SVSM		(19*32+28) /* "svsm" SVSM present */
> +#define X86_FEATURE_HV_INUSE_WR_ALLOWED	(19*32+30) /* Write to in-use hypervisor-owned pages allowed */
>  
>  /* AMD-defined Extended Feature 2 EAX, CPUID level 0x80000021 (EAX), word 20 */
>  #define X86_FEATURE_NO_NESTED_DATA_BP	(20*32+ 0) /* No Nested Data Breakpoints */
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 4b74ea91f4e6..42f2caf17d6a 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -1199,6 +1199,12 @@ bool avic_hardware_setup(void)
>  		return false;
>  	}
>  
> +	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP) &&
> +	    !boot_cpu_has(X86_FEATURE_HV_INUSE_WR_ALLOWED)) {
> +		pr_warn("AVIC disabled: missing HvInUseWrAllowed on SNP-enabled system");
> +		return false;
> +	}
> +

Wouldn't be better to make this is APICv inhibit to allow non-SNP guests to work
with AVIC?

	Joao

Re: [PATCH] KVM: SVM: Disable AVIC on SNP-enabled system without HvInUseWrAllowed feature

Posted by Suthikulpanit, Suravee 1 month, 1 week ago

Hi Joao,

On 10/1/2024 6:04 PM, Joao Martins wrote:
> On 30/09/2024 06:50, Suravee Suthikulpanit wrote:
>> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
>> index dd4682857c12..921b6de80e24 100644
>> --- a/arch/x86/include/asm/cpufeatures.h
>> +++ b/arch/x86/include/asm/cpufeatures.h
>> @@ -448,6 +448,7 @@
>>   #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_SVSM		(19*32+28) /* "svsm" SVSM present */
>> +#define X86_FEATURE_HV_INUSE_WR_ALLOWED	(19*32+30) /* Write to in-use hypervisor-owned pages allowed */
>>   
>>   /* AMD-defined Extended Feature 2 EAX, CPUID level 0x80000021 (EAX), word 20 */
>>   #define X86_FEATURE_NO_NESTED_DATA_BP	(20*32+ 0) /* No Nested Data Breakpoints */
>> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
>> index 4b74ea91f4e6..42f2caf17d6a 100644
>> --- a/arch/x86/kvm/svm/avic.c
>> +++ b/arch/x86/kvm/svm/avic.c
>> @@ -1199,6 +1199,12 @@ bool avic_hardware_setup(void)
>>   		return false;
>>   	}
>>   
>> +	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP) &&
>> +	    !boot_cpu_has(X86_FEATURE_HV_INUSE_WR_ALLOWED)) {
>> +		pr_warn("AVIC disabled: missing HvInUseWrAllowed on SNP-enabled system");
>> +		return false;
>> +	}
>> +
> 
> Wouldn't be better to make this is APICv inhibit to allow non-SNP guests to work
> with AVIC?

I was considering the APICV inhibit as well, and decided to go with 
disabling AVIC since it does not require additional 
APICV_INHIBIT_REASON_XXX flag, and we can simply disable AVIC support 
during kvm-amd driver initialization.

After rethink this, it is better to use per-VM APICv inhibition instead 
since certain AVIC data structures will be needed for secure AVIC 
support in the future.

I will update this patch and send out V2.

Thanks,
Suravee

Re: [PATCH] KVM: SVM: Disable AVIC on SNP-enabled system without HvInUseWrAllowed feature

Posted by Sean Christopherson 1 month, 4 weeks ago

On Mon, Sep 30, 2024, Suravee Suthikulpanit wrote:
> On SNP-enabled system, VMRUN marks AVIC Backing Page as in-use while
> the guest is running for both secure and non-secure guest. This causes
> any attempts to modify the RMP entries for the backing page to result in
> FAIL_INUSE response. This is to ensure that the AVIC backing page is not
> maliciously assigned to an SNP guest while the unencrypted guest is active.
> 
> Currently, an attempt to run AVIC guest would result in the following error:
> 
>     BUG: unable to handle page fault for address: ff3a442e549cc270
>     #PF: supervisor write access in kernel mode
>     #PF: error_code(0x80000003) - RMP violation
>     PGD b6ee01067 P4D b6ee02067 PUD 10096d063 PMD 11c540063 PTE 80000001149cc163
>     SEV-SNP: PFN 0x1149cc unassigned, dumping non-zero entries in 2M PFN region: [0x114800 - 0x114a00]
>     ...

This should be "fixed" by commit 75253db41a46 ("KVM: SEV: Make AVIC backing, VMSA
and VMCB memory allocation SNP safe"), no?

> Newer AMD system is enhanced to allow hypervisor to modify RMP entries of
> the backing page for non-secure guest on SNP-enabled system. This
> enhancement is available when the CPUID Fn8000_001F_EAX bit 30 is set
> (HvInUseWrAllowed) See the AMD64 Architecture Programmer’s Manual (APM)
> Volume 2 for detail. (https://www.amd.com/content/dam/amd/en/documents/
> processor-tech-docs/programmer-references/40332.pdf)
> 
> Therefore, add logic to check the new CPUID bit before enabling AVIC
> on SNP-enabled system.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  arch/x86/include/asm/cpufeatures.h | 1 +
>  arch/x86/kvm/svm/avic.c            | 6 ++++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index dd4682857c12..921b6de80e24 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -448,6 +448,7 @@
>  #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_SVSM		(19*32+28) /* "svsm" SVSM present */
> +#define X86_FEATURE_HV_INUSE_WR_ALLOWED	(19*32+30) /* Write to in-use hypervisor-owned pages allowed */
>  
>  /* AMD-defined Extended Feature 2 EAX, CPUID level 0x80000021 (EAX), word 20 */
>  #define X86_FEATURE_NO_NESTED_DATA_BP	(20*32+ 0) /* No Nested Data Breakpoints */
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 4b74ea91f4e6..42f2caf17d6a 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -1199,6 +1199,12 @@ bool avic_hardware_setup(void)
>  		return false;
>  	}
>  
> +	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP) &&
> +	    !boot_cpu_has(X86_FEATURE_HV_INUSE_WR_ALLOWED)) {
> +		pr_warn("AVIC disabled: missing HvInUseWrAllowed on SNP-enabled system");
> +		return false;
> +	}
> +
>  	if (boot_cpu_has(X86_FEATURE_AVIC)) {
>  		pr_info("AVIC enabled\n");
>  	} else if (force_avic) {
> -- 
> 2.34.1
>

Re: [PATCH] KVM: SVM: Disable AVIC on SNP-enabled system without HvInUseWrAllowed feature

Posted by Suthikulpanit, Suravee 1 month, 4 weeks ago

Hi Sean,

On 9/30/2024 11:04 PM, Sean Christopherson wrote:
> On Mon, Sep 30, 2024, Suravee Suthikulpanit wrote:
>> On SNP-enabled system, VMRUN marks AVIC Backing Page as in-use while
>> the guest is running for both secure and non-secure guest. This causes
>> any attempts to modify the RMP entries for the backing page to result in
>> FAIL_INUSE response. This is to ensure that the AVIC backing page is not
>> maliciously assigned to an SNP guest while the unencrypted guest is active.
>>
>> Currently, an attempt to run AVIC guest would result in the following error:
>>
>>      BUG: unable to handle page fault for address: ff3a442e549cc270
>>      #PF: supervisor write access in kernel mode
>>      #PF: error_code(0x80000003) - RMP violation
>>      PGD b6ee01067 P4D b6ee02067 PUD 10096d063 PMD 11c540063 PTE 80000001149cc163
>>      SEV-SNP: PFN 0x1149cc unassigned, dumping non-zero entries in 2M PFN region: [0x114800 - 0x114a00]
>>      ...
> This should be "fixed" by commit 75253db41a46 ("KVM: SEV: Make AVIC backing, VMSA
> and VMCB memory allocation SNP safe"), no?

The commit 75253db41a46 fixes another issue related to 2MB-aligned 
in-use page, where the CPU incorrectly treats the whole 2MB region as 
in-use and  signal an RMP violation #PF.

This enhancement is mainly to allow hypervisor to write to the AVIC 
backing page of non-secure guest on SNP-enabled system.

Note: This change might need to be ported to stable 6.9, 6.10, and 6.11 
tree as well.

Thanks,
Suravee

Re: [PATCH] KVM: SVM: Disable AVIC on SNP-enabled system without HvInUseWrAllowed feature

Posted by Sean Christopherson 1 month, 3 weeks ago

On Tue, Oct 01, 2024, Suravee Suthikulpanit wrote:
> Hi Sean,
> 
> On 9/30/2024 11:04 PM, Sean Christopherson wrote:
> > On Mon, Sep 30, 2024, Suravee Suthikulpanit wrote:
> > > On SNP-enabled system, VMRUN marks AVIC Backing Page as in-use while
> > > the guest is running for both secure and non-secure guest. This causes
> > > any attempts to modify the RMP entries for the backing page to result in
> > > FAIL_INUSE response. This is to ensure that the AVIC backing page is not
> > > maliciously assigned to an SNP guest while the unencrypted guest is active.
> > > 
> > > Currently, an attempt to run AVIC guest would result in the following error:
> > > 
> > >      BUG: unable to handle page fault for address: ff3a442e549cc270
> > >      #PF: supervisor write access in kernel mode
> > >      #PF: error_code(0x80000003) - RMP violation
> > >      PGD b6ee01067 P4D b6ee02067 PUD 10096d063 PMD 11c540063 PTE 80000001149cc163
> > >      SEV-SNP: PFN 0x1149cc unassigned, dumping non-zero entries in 2M PFN region: [0x114800 - 0x114a00]
> > >      ...
> > This should be "fixed" by commit 75253db41a46 ("KVM: SEV: Make AVIC backing, VMSA
> > and VMCB memory allocation SNP safe"), no?
> 
> The commit 75253db41a46 fixes another issue related to 2MB-aligned in-use
> page, where the CPU incorrectly treats the whole 2MB region as in-use and
> signal an RMP violation #PF.
> 
> This enhancement is mainly to allow hypervisor to write to the AVIC backing
> page of non-secure guest on SNP-enabled system.

In that case, the changelog needs to be rewritten, because the changelog very
explicitly talks about modifying RMP entries, whereas IIUC, the issue is that
cross-CPU writes to a vCPU's vAPIC page, e.g. to inject an interrupt, will generate
unexpected #PFs in the host.

> Note: This change might need to be ported to stable 6.9, 6.10, and 6.11 tree
> as well.

At the very least, it needs a fixes, which I believe is:

  Fixes: 216d106c7ff7 ("x86/sev: Add SEV-SNP host initialization support")

6.9 and 6.10 aren't LTS kernels, so backports to them aren't necessary.