diff mbox series

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

Message ID 20240930055035.31412-1-suravee.suthikulpanit@amd.com (mailing list archive)
State New
Headers show
Series KVM: SVM: Disable AVIC on SNP-enabled system without HvInUseWrAllowed feature | expand

Commit Message

Suthikulpanit, Suravee Sept. 30, 2024, 5:50 a.m. UTC 
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(+)

Comments

Sean Christopherson Sept. 30, 2024, 4:04 p.m. UTC | #1 
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
>
Suthikulpanit, Suravee Oct. 1, 2024, 8:59 a.m. UTC | #2 
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
Joao Martins Oct. 1, 2024, 11:04 a.m. UTC | #3 
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
Sean Christopherson Oct. 1, 2024, 7:16 p.m. UTC | #4 
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.
diff mbox series

Patch

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