[PATCH 0/2] KVM: SVM: Final C-bit fixes?

[PATCH 0/2] KVM: SVM: Final C-bit fixes?

[Sean Christopherson]

Patch 01 reverts the C-bit truncation patch as the reserved #PF was
confirmed to be due to a magic HyperTransport region (how many magic
addresses are there!?!).  Hopefully the original patch simply be dropped,
but just in case...

Patch 02 reverts the C-bit clearing in the #NPF handler.  If that somehow
turns out to be incorrect, i.e. there are flows where the CPU doesn't
mask off the C-bit, then it can be conditional on a SEV guest.

I'll be offline for the next two weeks, fingers crossed I've undone all
the damage.  :-)

Thanks!

Sean Christopherson (2):
  Revert "KVM: x86: Truncate reported guest MAXPHYADDR to C-bit if SEV
    is supported"
  KVM: SVM: Revert clearing of C-bit on GPA in #NPF handler

 arch/x86/kvm/cpuid.c   | 11 -----------
 arch/x86/kvm/svm/svm.c | 39 +++++++++------------------------------
 arch/x86/kvm/x86.c     |  3 ---
 arch/x86/kvm/x86.h     |  1 -
 4 files changed, 9 insertions(+), 45 deletions(-)

-- 
2.32.0.93.g670b81a890-goog

[PATCH 1/2] Revert "KVM: x86: Truncate reported guest MAXPHYADDR to C-bit if SEV is supported"

[Sean Christopherson]

It turns out that non-SEV guest can indeed use bit 47, and the unexpected
reserved #PF observed when setting bit 47 is due to a magic 12gb
HyperTransport region being off limits, even for GPAs.  Per Tom:

  I think you may be hitting a special HT region that is at the top 12GB
  of the 48-bit memory range and is reserved, even for GPAs.  Can you
  somehow get the test to use an address below 0xfffd_0000_0000? That
  would show that bit 47 is valid for the legacy guest while staying out
  of the HT region.

And indeed, accessing 0xfffd00000000 generates a reserved #PF, while
dropping down a single page to 0xfffcfffff000 does not.

This reverts commit 3675f005c87c4026713c9f863924de511fdd36c4.

Cc: Peter Gonda <pgonda@google.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/cpuid.c   | 11 -----------
 arch/x86/kvm/svm/svm.c | 37 ++++++++-----------------------------
 arch/x86/kvm/x86.c     |  3 ---
 arch/x86/kvm/x86.h     |  1 -
 4 files changed, 8 insertions(+), 44 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 0edda1fc4fe7..ca7866d63e98 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -955,17 +955,6 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
 		else if (!g_phys_as)
 			g_phys_as = phys_as;
 
-		/*
-		 * The exception to the exception is if hardware supports SEV,
-		 * in which case the C-bit is reserved for non-SEV guests and
-		 * isn't a GPA bit for SEV guests.
-		 *
-		 * Note, KVM always reports '0' for the number of reduced PA
-		 * bits (see 0x8000001F).
-		 */
-		if (tdp_enabled && sev_c_bit)
-			g_phys_as = min(g_phys_as, (unsigned int)sev_c_bit);
-
 		entry->eax = g_phys_as | (virt_as << 8);
 		entry->edx = 0;
 		cpuid_entry_override(entry, CPUID_8000_0008_EBX);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 616b9679ddcc..8834822c00cd 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -860,26 +860,6 @@ static void shrink_ple_window(struct kvm_vcpu *vcpu)
 	}
 }
 
-static __init u8 svm_get_c_bit(bool sev_only)
-{
-	unsigned int eax, ebx, ecx, edx;
-	u64 msr;
-
-	if (cpuid_eax(0x80000000) < 0x8000001f)
-		return 0;
-
-	if (rdmsrl_safe(MSR_AMD64_SYSCFG, &msr) ||
-	    !(msr & MSR_AMD64_SYSCFG_MEM_ENCRYPT))
-		return 0;
-
-	cpuid(0x8000001f, &eax, &ebx, &ecx, &edx);
-
-	if (sev_only && !(eax & feature_bit(SEV)))
-		return 0;
-
-	return ebx & 0x3f;
-}
-
 /*
  * The default MMIO mask is a single bit (excluding the present bit),
  * which could conflict with the memory encryption bit. Check for
@@ -889,13 +869,18 @@ static __init u8 svm_get_c_bit(bool sev_only)
 static __init void svm_adjust_mmio_mask(void)
 {
 	unsigned int enc_bit, mask_bit;
-	u64 mask;
+	u64 msr, mask;
+
+	/* If there is no memory encryption support, use existing mask */
+	if (cpuid_eax(0x80000000) < 0x8000001f)
+		return;
 
 	/* If memory encryption is not enabled, use existing mask */
-	enc_bit = svm_get_c_bit(false);
-	if (!enc_bit)
+	rdmsrl(MSR_AMD64_SYSCFG, msr);
+	if (!(msr & MSR_AMD64_SYSCFG_MEM_ENCRYPT))
 		return;
 
+	enc_bit = cpuid_ebx(0x8000001f) & 0x3f;
 	mask_bit = boot_cpu_data.x86_phys_bits;
 
 	/* Increment the mask bit if it is the same as the encryption bit */
@@ -1028,12 +1013,6 @@ static __init int svm_hardware_setup(void)
 	kvm_configure_mmu(npt_enabled, get_max_npt_level(), PG_LEVEL_1G);
 	pr_info("kvm: Nested Paging %sabled\n", npt_enabled ? "en" : "dis");
 
-	/*
-	 * The SEV C-bit location is needed to correctly enumeration guest
-	 * MAXPHYADDR even if SEV is not fully supported.
-	 */
-	sev_c_bit = svm_get_c_bit(true);
-
 	/* Note, SEV setup consumes npt_enabled. */
 	sev_hardware_setup();
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4a597aafe637..13905ef5bb48 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -209,9 +209,6 @@ static struct kvm_user_return_msrs __percpu *user_return_msrs;
 				| XFEATURE_MASK_BNDCSR | XFEATURE_MASK_AVX512 \
 				| XFEATURE_MASK_PKRU)
 
-u8 __read_mostly sev_c_bit;
-EXPORT_SYMBOL_GPL(sev_c_bit);
-
 u64 __read_mostly host_efer;
 EXPORT_SYMBOL_GPL(host_efer);
 
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index bc3f5c9e3708..44ae10312740 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -328,7 +328,6 @@ extern u64 host_xcr0;
 extern u64 supported_xcr0;
 extern u64 host_xss;
 extern u64 supported_xss;
-extern u8  sev_c_bit;
 
 static inline bool kvm_mpx_supported(void)
 {
-- 
2.32.0.93.g670b81a890-goog

[PATCH 2/2] KVM: SVM: Revert clearing of C-bit on GPA in #NPF handler

[Sean Christopherson]

Don't clear the C-bit in the #NPF handler, as it is a legal GPA bit for
non-SEV guests, and for SEV guests the C-bit is dropped before the GPA
hits the NPT in hardware.  Clearing the bit for non-SEV guests causes KVM
to mishandle #NPFs with that collide with the host's C-bit.

Although the APM doesn't explicitly state that the C-bit is not reserved
for non-SEV, Tom Lendacky confirmed that the following snippet about the
effective reduction due to the C-bit does indeed apply only to SEV guests.

  Note that because guest physical addresses are always translated
  through the nested page tables, the size of the guest physical address
  space is not impacted by any physical address space reduction indicated
  in CPUID 8000_001F[EBX]. If the C-bit is a physical address bit however,
  the guest physical address space is effectively reduced by 1 bit.

And for SEV guests, the APM clearly states that the bit is dropped before
walking the nested page tables.

  If the C-bit is an address bit, this bit is masked from the guest
  physical address when it is translated through the nested page tables.
  Consequently, the hypervisor does not need to be aware of which pages
  the guest has chosen to mark private.

Note, the bogus C-bit clearing was removed from legacy #PF handler in
commit 6d1b867d0456 ("KVM: SVM: Don't strip the C-bit from CR2 on #PF
interception").

Fixes: 0ede79e13224 ("KVM: SVM: Clear C-bit from the page fault address")
Cc: Peter Gonda <pgonda@google.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/svm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 8834822c00cd..ca5614a48b21 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1923,7 +1923,7 @@ static int npf_interception(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 
-	u64 fault_address = __sme_clr(svm->vmcb->control.exit_info_2);
+	u64 fault_address = svm->vmcb->control.exit_info_2;
 	u64 error_code = svm->vmcb->control.exit_info_1;
 
 	trace_kvm_page_fault(fault_address, error_code);
-- 
2.32.0.93.g670b81a890-goog

Re: [PATCH 2/2] KVM: SVM: Revert clearing of C-bit on GPA in #NPF handler

[Tom Lendacky]

On 6/24/21 9:03 PM, Sean Christopherson wrote:
> Don't clear the C-bit in the #NPF handler, as it is a legal GPA bit for
> non-SEV guests, and for SEV guests the C-bit is dropped before the GPA
> hits the NPT in hardware.  Clearing the bit for non-SEV guests causes KVM
> to mishandle #NPFs with that collide with the host's C-bit.
> 
> Although the APM doesn't explicitly state that the C-bit is not reserved
> for non-SEV, Tom Lendacky confirmed that the following snippet about the
> effective reduction due to the C-bit does indeed apply only to SEV guests.
> 
>   Note that because guest physical addresses are always translated
>   through the nested page tables, the size of the guest physical address
>   space is not impacted by any physical address space reduction indicated
>   in CPUID 8000_001F[EBX]. If the C-bit is a physical address bit however,
>   the guest physical address space is effectively reduced by 1 bit.
> 
> And for SEV guests, the APM clearly states that the bit is dropped before
> walking the nested page tables.
> 
>   If the C-bit is an address bit, this bit is masked from the guest
>   physical address when it is translated through the nested page tables.
>   Consequently, the hypervisor does not need to be aware of which pages
>   the guest has chosen to mark private.
> 
> Note, the bogus C-bit clearing was removed from legacy #PF handler in
> commit 6d1b867d0456 ("KVM: SVM: Don't strip the C-bit from CR2 on #PF
> interception").
> 
> Fixes: 0ede79e13224 ("KVM: SVM: Clear C-bit from the page fault address")
> Cc: Peter Gonda <pgonda@google.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Yep, definitely wasn't correct to be using an SME based macro on a guest
address. And as the APM states, the encryption bit is stripped for SEV
guests, so looks correct to me.

Acked-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>  arch/x86/kvm/svm/svm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 8834822c00cd..ca5614a48b21 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1923,7 +1923,7 @@ static int npf_interception(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  
> -	u64 fault_address = __sme_clr(svm->vmcb->control.exit_info_2);
> +	u64 fault_address = svm->vmcb->control.exit_info_2;
>  	u64 error_code = svm->vmcb->control.exit_info_1;
>  
>  	trace_kvm_page_fault(fault_address, error_code);
> 

Re: [PATCH 1/2] Revert "KVM: x86: Truncate reported guest MAXPHYADDR to C-bit if SEV is supported"

[Tom Lendacky]

On 6/24/21 9:03 PM, Sean Christopherson wrote:
> It turns out that non-SEV guest can indeed use bit 47, and the unexpected
> reserved #PF observed when setting bit 47 is due to a magic 12gb
> HyperTransport region being off limits, even for GPAs.  Per Tom:
> 
>   I think you may be hitting a special HT region that is at the top 12GB
>   of the 48-bit memory range and is reserved, even for GPAs.  Can you
>   somehow get the test to use an address below 0xfffd_0000_0000? That
>   would show that bit 47 is valid for the legacy guest while staying out
>   of the HT region.
> 
> And indeed, accessing 0xfffd00000000 generates a reserved #PF, while
> dropping down a single page to 0xfffcfffff000 does not.
> 
> This reverts commit 3675f005c87c4026713c9f863924de511fdd36c4.
> 
> Cc: Peter Gonda <pgonda@google.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Acked-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>  arch/x86/kvm/cpuid.c   | 11 -----------
>  arch/x86/kvm/svm/svm.c | 37 ++++++++-----------------------------
>  arch/x86/kvm/x86.c     |  3 ---
>  arch/x86/kvm/x86.h     |  1 -
>  4 files changed, 8 insertions(+), 44 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 0edda1fc4fe7..ca7866d63e98 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -955,17 +955,6 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>  		else if (!g_phys_as)
>  			g_phys_as = phys_as;
>  
> -		/*
> -		 * The exception to the exception is if hardware supports SEV,
> -		 * in which case the C-bit is reserved for non-SEV guests and
> -		 * isn't a GPA bit for SEV guests.
> -		 *
> -		 * Note, KVM always reports '0' for the number of reduced PA
> -		 * bits (see 0x8000001F).
> -		 */
> -		if (tdp_enabled && sev_c_bit)
> -			g_phys_as = min(g_phys_as, (unsigned int)sev_c_bit);
> -
>  		entry->eax = g_phys_as | (virt_as << 8);
>  		entry->edx = 0;
>  		cpuid_entry_override(entry, CPUID_8000_0008_EBX);
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 616b9679ddcc..8834822c00cd 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -860,26 +860,6 @@ static void shrink_ple_window(struct kvm_vcpu *vcpu)
>  	}
>  }
>  
> -static __init u8 svm_get_c_bit(bool sev_only)
> -{
> -	unsigned int eax, ebx, ecx, edx;
> -	u64 msr;
> -
> -	if (cpuid_eax(0x80000000) < 0x8000001f)
> -		return 0;
> -
> -	if (rdmsrl_safe(MSR_AMD64_SYSCFG, &msr) ||
> -	    !(msr & MSR_AMD64_SYSCFG_MEM_ENCRYPT))
> -		return 0;
> -
> -	cpuid(0x8000001f, &eax, &ebx, &ecx, &edx);
> -
> -	if (sev_only && !(eax & feature_bit(SEV)))
> -		return 0;
> -
> -	return ebx & 0x3f;
> -}
> -
>  /*
>   * The default MMIO mask is a single bit (excluding the present bit),
>   * which could conflict with the memory encryption bit. Check for
> @@ -889,13 +869,18 @@ static __init u8 svm_get_c_bit(bool sev_only)
>  static __init void svm_adjust_mmio_mask(void)
>  {
>  	unsigned int enc_bit, mask_bit;
> -	u64 mask;
> +	u64 msr, mask;
> +
> +	/* If there is no memory encryption support, use existing mask */
> +	if (cpuid_eax(0x80000000) < 0x8000001f)
> +		return;
>  
>  	/* If memory encryption is not enabled, use existing mask */
> -	enc_bit = svm_get_c_bit(false);
> -	if (!enc_bit)
> +	rdmsrl(MSR_AMD64_SYSCFG, msr);
> +	if (!(msr & MSR_AMD64_SYSCFG_MEM_ENCRYPT))
>  		return;
>  
> +	enc_bit = cpuid_ebx(0x8000001f) & 0x3f;
>  	mask_bit = boot_cpu_data.x86_phys_bits;
>  
>  	/* Increment the mask bit if it is the same as the encryption bit */
> @@ -1028,12 +1013,6 @@ static __init int svm_hardware_setup(void)
>  	kvm_configure_mmu(npt_enabled, get_max_npt_level(), PG_LEVEL_1G);
>  	pr_info("kvm: Nested Paging %sabled\n", npt_enabled ? "en" : "dis");
>  
> -	/*
> -	 * The SEV C-bit location is needed to correctly enumeration guest
> -	 * MAXPHYADDR even if SEV is not fully supported.
> -	 */
> -	sev_c_bit = svm_get_c_bit(true);
> -
>  	/* Note, SEV setup consumes npt_enabled. */
>  	sev_hardware_setup();
>  
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 4a597aafe637..13905ef5bb48 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -209,9 +209,6 @@ static struct kvm_user_return_msrs __percpu *user_return_msrs;
>  				| XFEATURE_MASK_BNDCSR | XFEATURE_MASK_AVX512 \
>  				| XFEATURE_MASK_PKRU)
>  
> -u8 __read_mostly sev_c_bit;
> -EXPORT_SYMBOL_GPL(sev_c_bit);
> -
>  u64 __read_mostly host_efer;
>  EXPORT_SYMBOL_GPL(host_efer);
>  
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index bc3f5c9e3708..44ae10312740 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -328,7 +328,6 @@ extern u64 host_xcr0;
>  extern u64 supported_xcr0;
>  extern u64 host_xss;
>  extern u64 supported_xss;
> -extern u8  sev_c_bit;
>  
>  static inline bool kvm_mpx_supported(void)
>  {
> 

Re: [PATCH 0/2] KVM: SVM: Final C-bit fixes?

[Paolo Bonzini]

On 25/06/21 04:03, Sean Christopherson wrote:
> Patch 01 reverts the C-bit truncation patch as the reserved #PF was
> confirmed to be due to a magic HyperTransport region (how many magic
> addresses are there!?!).  Hopefully the original patch simply be dropped,
> but just in case...
> 
> Patch 02 reverts the C-bit clearing in the #NPF handler.  If that somehow
> turns out to be incorrect, i.e. there are flows where the CPU doesn't
> mask off the C-bit, then it can be conditional on a SEV guest.
> 
> I'll be offline for the next two weeks, fingers crossed I've undone all
> the damage.  :-)
> 
> Thanks!
> 
> Sean Christopherson (2):
>    Revert "KVM: x86: Truncate reported guest MAXPHYADDR to C-bit if SEV
>      is supported"
>    KVM: SVM: Revert clearing of C-bit on GPA in #NPF handler
> 
>   arch/x86/kvm/cpuid.c   | 11 -----------
>   arch/x86/kvm/svm/svm.c | 39 +++++++++------------------------------
>   arch/x86/kvm/x86.c     |  3 ---
>   arch/x86/kvm/x86.h     |  1 -
>   4 files changed, 9 insertions(+), 45 deletions(-)
> 

Queued, thanks.

Paolo