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

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

[Suravee Suthikulpanit]

On SNP-enabled system, VMRUN marks AVIC Backing Page as in-use while
the guest is running for both secure and non-secure guest. Any hypervisor
write to the in-use vCPU's AVIC backing page (e.g. to inject an interrupt)
will generate unexpected #PF in the host.

Currently, 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 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).

This table describes AVIC support matrix w.r.t. SNP enablement:

               | Non-SNP system |     SNP system
-----------------------------------------------------
 Non-SNP guest |  AVIC Activate | AVIC Activate iff
               |                | HvInuseWrAllowed=1
-----------------------------------------------------
     SNP guest |      N/A       |    Secure AVIC
               |                |    x2APIC only

Introduce APICV_INHIBIT_REASON_HVINUSEWR_NOT_ALLOWED to deactivate AVIC
when the feature is not available on SNP-enabled system.

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)

Fixes: 216d106c7ff7 ("x86/sev: Add SEV-SNP host initialization support")
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---

Change log v2:
 * Use APICv inhibit bit instead of disabling AVIC in driver.

 arch/x86/include/asm/cpufeatures.h | 1 +
 arch/x86/include/asm/kvm_host.h    | 9 ++++++++-
 arch/x86/kvm/svm/avic.c            | 6 ++++++
 arch/x86/kvm/svm/svm.h             | 3 ++-
 4 files changed, 17 insertions(+), 2 deletions(-)

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/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 4a68cb3eba78..1fef50025512 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1276,6 +1276,12 @@ enum kvm_apicv_inhibit {
 	 */
 	APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED,
 
+	/*
+	 * Non-SNP guest cannot activate AVIC on SNP-enabled system w/o
+	 * CPUID HvInUseWrAllowed feature.
+	 */
+	APICV_INHIBIT_REASON_HVINUSEWR_NOT_ALLOWED,
+
 	NR_APICV_INHIBIT_REASONS,
 };
 
@@ -1294,7 +1300,8 @@ enum kvm_apicv_inhibit {
 	__APICV_INHIBIT_REASON(IRQWIN),			\
 	__APICV_INHIBIT_REASON(PIT_REINJ),		\
 	__APICV_INHIBIT_REASON(SEV),			\
-	__APICV_INHIBIT_REASON(LOGICAL_ID_ALIASED)
+	__APICV_INHIBIT_REASON(LOGICAL_ID_ALIASED),	\
+	__APICV_INHIBIT_REASON(HVINUSEWR_NOT_ALLOWED)
 
 struct kvm_arch {
 	unsigned long n_used_mmu_pages;
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 4b74ea91f4e6..cc4f0c00334a 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -202,6 +202,12 @@ int avic_vm_init(struct kvm *kvm)
 	if (!enable_apicv)
 		return 0;
 
+	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP) &&
+	    !boot_cpu_has(X86_FEATURE_HV_INUSE_WR_ALLOWED)) {
+		pr_debug("APICv Inhibit due to Missing HvInUseWrAllowed.\n");
+		kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_HVINUSEWR_NOT_ALLOWED);
+	}
+
 	/* Allocating physical APIC ID table (4KB) */
 	p_page = alloc_page(GFP_KERNEL_ACCOUNT | __GFP_ZERO);
 	if (!p_page)
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 76107c7d0595..13046bad2d6e 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -682,7 +682,8 @@ extern struct kvm_x86_nested_ops svm_nested_ops;
 	BIT(APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED) |	\
 	BIT(APICV_INHIBIT_REASON_APIC_ID_MODIFIED) |	\
 	BIT(APICV_INHIBIT_REASON_APIC_BASE_MODIFIED) |	\
-	BIT(APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED)	\
+	BIT(APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED) |	\
+	BIT(APICV_INHIBIT_REASON_HVINUSEWR_NOT_ALLOWED)	\
 )
 
 bool avic_hardware_setup(void);
-- 
2.34.1

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

[Joao Martins]

On 18/10/2024 09:50, 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. Any hypervisor
> write to the in-use vCPU's AVIC backing page (e.g. to inject an interrupt)
> will generate unexpected #PF in the host.
> 
> Currently, 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 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).
> 
> This table describes AVIC support matrix w.r.t. SNP enablement:
> 
>                | Non-SNP system |     SNP system
> -----------------------------------------------------
>  Non-SNP guest |  AVIC Activate | AVIC Activate iff
>                |                | HvInuseWrAllowed=1
> -----------------------------------------------------
>      SNP guest |      N/A       |    Secure AVIC
>                |                |    x2APIC only
> 
> Introduce APICV_INHIBIT_REASON_HVINUSEWR_NOT_ALLOWED to deactivate AVIC
> when the feature is not available on SNP-enabled system.
> 
I misread your first sentence in v1 wrt to non-secure guests -- but it's a lot
more obvious now. If this was sort of a dynamic condition at runtime (like the
other inhibits triggered by guest behavior or something that can change at
runtime post-boot, or modparam) then the inhibit system would be best acquainted
for preventing enabling AVIC on a per-vm basis. But it appears this is
global-defined-at-boot that blocks any non-secure guest from using AVIC if we
boot as an SNP-enabled host i.e. based on testing BSP-defined feature bits solely.

Your original proposal perhaps is better where you disable AVIC globally in
avic_hardware_setup(). Apologies for (mistankenly) misleading you and wasting
your time :/

> 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)
> 
> Fixes: 216d106c7ff7 ("x86/sev: Add SEV-SNP host initialization support")
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
> 
> Change log v2:
>  * Use APICv inhibit bit instead of disabling AVIC in driver.
> 
>  arch/x86/include/asm/cpufeatures.h | 1 +
>  arch/x86/include/asm/kvm_host.h    | 9 ++++++++-
>  arch/x86/kvm/svm/avic.c            | 6 ++++++
>  arch/x86/kvm/svm/svm.h             | 3 ++-
>  4 files changed, 17 insertions(+), 2 deletions(-)
> 
> 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/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 4a68cb3eba78..1fef50025512 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1276,6 +1276,12 @@ enum kvm_apicv_inhibit {
>  	 */
>  	APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED,
>  
> +	/*
> +	 * Non-SNP guest cannot activate AVIC on SNP-enabled system w/o
> +	 * CPUID HvInUseWrAllowed feature.
> +	 */
> +	APICV_INHIBIT_REASON_HVINUSEWR_NOT_ALLOWED,
> +
>  	NR_APICV_INHIBIT_REASONS,
>  };
>  
> @@ -1294,7 +1300,8 @@ enum kvm_apicv_inhibit {
>  	__APICV_INHIBIT_REASON(IRQWIN),			\
>  	__APICV_INHIBIT_REASON(PIT_REINJ),		\
>  	__APICV_INHIBIT_REASON(SEV),			\
> -	__APICV_INHIBIT_REASON(LOGICAL_ID_ALIASED)
> +	__APICV_INHIBIT_REASON(LOGICAL_ID_ALIASED),	\
> +	__APICV_INHIBIT_REASON(HVINUSEWR_NOT_ALLOWED)
>  
>  struct kvm_arch {
>  	unsigned long n_used_mmu_pages;
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 4b74ea91f4e6..cc4f0c00334a 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -202,6 +202,12 @@ int avic_vm_init(struct kvm *kvm)
>  	if (!enable_apicv)
>  		return 0;
>  
> +	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP) &&
> +	    !boot_cpu_has(X86_FEATURE_HV_INUSE_WR_ALLOWED)) {
> +		pr_debug("APICv Inhibit due to Missing HvInUseWrAllowed.\n");
> +		kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_HVINUSEWR_NOT_ALLOWED);
> +	}
> +
>  	/* Allocating physical APIC ID table (4KB) */
>  	p_page = alloc_page(GFP_KERNEL_ACCOUNT | __GFP_ZERO);
>  	if (!p_page)
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 76107c7d0595..13046bad2d6e 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -682,7 +682,8 @@ extern struct kvm_x86_nested_ops svm_nested_ops;
>  	BIT(APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED) |	\
>  	BIT(APICV_INHIBIT_REASON_APIC_ID_MODIFIED) |	\
>  	BIT(APICV_INHIBIT_REASON_APIC_BASE_MODIFIED) |	\
> -	BIT(APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED)	\
> +	BIT(APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED) |	\
> +	BIT(APICV_INHIBIT_REASON_HVINUSEWR_NOT_ALLOWED)	\
>  )
>  
>  bool avic_hardware_setup(void);

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

[Suthikulpanit, Suravee]

On 10/18/2024 4:57 PM, Joao Martins wrote:
> On 18/10/2024 09:50, 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. Any hypervisor
>> write to the in-use vCPU's AVIC backing page (e.g. to inject an interrupt)
>> will generate unexpected #PF in the host.
>>
>> Currently, 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 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).
>>
>> This table describes AVIC support matrix w.r.t. SNP enablement:
>>
>>                 | Non-SNP system |     SNP system
>> -----------------------------------------------------
>>   Non-SNP guest |  AVIC Activate | AVIC Activate iff
>>                 |                | HvInuseWrAllowed=1
>> -----------------------------------------------------
>>       SNP guest |      N/A       |    Secure AVIC
>>                 |                |    x2APIC only
>>
>> Introduce APICV_INHIBIT_REASON_HVINUSEWR_NOT_ALLOWED to deactivate AVIC
>> when the feature is not available on SNP-enabled system.
>>
> I misread your first sentence in v1 wrt to non-secure guests -- but it's a lot
> more obvious now. If this was sort of a dynamic condition at runtime (like the
> other inhibits triggered by guest behavior or something that can change at
> runtime post-boot, or modparam) then the inhibit system would be best acquainted
> for preventing enabling AVIC on a per-vm basis. But it appears this is
> global-defined-at-boot that blocks any non-secure guest from using AVIC if we
> boot as an SNP-enabled host i.e. based on testing BSP-defined feature bits solely.
> 
> Your original proposal perhaps is better where you disable AVIC globally in
> avic_hardware_setup(). Apologies for (mistankenly) misleading you and wasting
> your time :/

Repost from v1 thread:

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.

Thanks,
Suravee

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

[Sean Christopherson]

On Fri, Oct 18, 2024, Suravee Suthikulpanit wrote:
> On 10/18/2024 4:57 PM, Joao Martins wrote:
> > On 18/10/2024 09:50, 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. Any hypervisor
> > > write to the in-use vCPU's AVIC backing page (e.g. to inject an interrupt)
> > > will generate unexpected #PF in the host.
> > > 
> > > Currently, 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 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).
> > > 
> > > This table describes AVIC support matrix w.r.t. SNP enablement:
> > > 
> > >                 | Non-SNP system |     SNP system
> > > -----------------------------------------------------
> > >   Non-SNP guest |  AVIC Activate | AVIC Activate iff
> > >                 |                | HvInuseWrAllowed=1
> > > -----------------------------------------------------
> > >       SNP guest |      N/A       |    Secure AVIC
> > >                 |                |    x2APIC only
> > > 
> > > Introduce APICV_INHIBIT_REASON_HVINUSEWR_NOT_ALLOWED to deactivate AVIC

Please use human/reader friendly terms, that's a very convoluted way of saying:

	APICV_INHIBIT_REASON_IN_USE_AVIC_PAGE_READ_ONLY

> > > when the feature is not available on SNP-enabled system.
> > > 
> > I misread your first sentence in v1 wrt to non-secure guests -- but it's a lot
> > more obvious now. If this was sort of a dynamic condition at runtime (like the
> > other inhibits triggered by guest behavior or something that can change at
> > runtime post-boot, or modparam) then the inhibit system would be best acquainted
> > for preventing enabling AVIC on a per-vm basis. But it appears this is
> > global-defined-at-boot that blocks any non-secure guest from using AVIC if we
> > boot as an SNP-enabled host i.e. based on testing BSP-defined feature bits solely.
> > 
> > Your original proposal perhaps is better where you disable AVIC globally in
> > avic_hardware_setup(). Apologies for (mistankenly) misleading you and wasting
> > your time :/
> 
> Repost from v1 thread:
> 
> 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 don't follow.  I agree with Joao, this seems like an all-or-nothing situation.
There's no point in an inhibit unless Secure AVIC CPUs will exist WITHOUT
HvInuseWrAllowed, but even then, to keep things simple(r), I'm tempted to make
SNP+AVIC require HvInuseWrAllowed