| Message ID | 20240930055035.31412-1-suravee.suthikulpanit@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: SVM: Disable AVIC on SNP-enabled system without HvInUseWrAllowed feature | expand |
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 >
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
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
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.
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
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) {
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(+)