[PATCH v3 0/2] kvm/cpuid: set proper GuestPhysBits in CPUID.0x80000008

[PATCH v3 0/2] kvm/cpuid: set proper GuestPhysBits in CPUID.0x80000008

[Gerd Hoffmann]

Use the GuestPhysBits field (EAX[23:16]) to communicate the max
addressable GPA to the guest.  Typically this is identical to the max
effective GPA, except in case the CPU supports MAXPHYADDR > 48 but does
not support 5-level TDP.

See commit messages and source code comments for details.

Gerd Hoffmann (2):
  kvm/cpuid: remove GuestPhysBits code.
  kvm/cpuid: set proper GuestPhysBits in CPUID.0x80000008

 arch/x86/kvm/mmu.h     |  2 ++
 arch/x86/kvm/cpuid.c   | 53 ++++++++++++++++++++++++++++++------------
 arch/x86/kvm/mmu/mmu.c |  5 ++++
 3 files changed, 45 insertions(+), 15 deletions(-)

-- 
2.44.0

[PATCH v3 1/2] kvm/cpuid: remove GuestPhysBits code.

[Gerd Hoffmann]

GuestPhysBits (cpuid leaf 80000008, eax[23:16]) is intended for software
use.  Physical CPUs do not set that field.  The current code which
propagates the host's GuestPhysBits to the guest's PhysBits does not
make sense.  Remove it.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 arch/x86/kvm/cpuid.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index adba49afb5fe..c638b5fb2144 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1221,26 +1221,22 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
 		entry->eax = entry->ebx = entry->ecx = 0;
 		break;
 	case 0x80000008: {
-		unsigned g_phys_as = (entry->eax >> 16) & 0xff;
-		unsigned virt_as = max((entry->eax >> 8) & 0xff, 48U);
-		unsigned phys_as = entry->eax & 0xff;
+		unsigned int virt_as = max((entry->eax >> 8) & 0xff, 48U);
+		unsigned int phys_as;
 
-		/*
-		 * If TDP (NPT) is disabled use the adjusted host MAXPHYADDR as
-		 * the guest operates in the same PA space as the host, i.e.
-		 * reductions in MAXPHYADDR for memory encryption affect shadow
-		 * paging, too.
-		 *
-		 * If TDP is enabled but an explicit guest MAXPHYADDR is not
-		 * provided, use the raw bare metal MAXPHYADDR as reductions to
-		 * the HPAs do not affect GPAs.
-		 */
-		if (!tdp_enabled)
-			g_phys_as = boot_cpu_data.x86_phys_bits;
-		else if (!g_phys_as)
-			g_phys_as = phys_as;
+		if (!tdp_enabled) {
+			/*
+			 * If TDP (NPT) is disabled use the adjusted host
+			 * MAXPHYADDR as the guest operates in the same PA
+			 * space as the host, i.e.  reductions in MAXPHYADDR
+			 * for memory encryption affect shadow paging, too.
+			 */
+			phys_as = boot_cpu_data.x86_phys_bits;
+		} else {
+			phys_as = entry->eax & 0xff;
+		}
 
-		entry->eax = g_phys_as | (virt_as << 8);
+		entry->eax = phys_as | (virt_as << 8);
 		entry->ecx &= ~(GENMASK(31, 16) | GENMASK(11, 8));
 		entry->edx = 0;
 		cpuid_entry_override(entry, CPUID_8000_0008_EBX);
-- 
2.44.0

Re: [PATCH v3 1/2] kvm/cpuid: remove GuestPhysBits code.

[Xiaoyao Li]

On 3/11/2024 6:41 PM, Gerd Hoffmann wrote:
 > [PATCH v3 1/2] kvm/cpuid: remove GuestPhysBits code.

No need for a ending . in the title.

> GuestPhysBits (cpuid leaf 80000008, eax[23:16]) is intended for software
> use.  Physical CPUs do not set that field.  The current code which
> propagates the host's GuestPhysBits to the guest's PhysBits does not
> make sense.  Remove it.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   arch/x86/kvm/cpuid.c | 32 ++++++++++++++------------------
>   1 file changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index adba49afb5fe..c638b5fb2144 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1221,26 +1221,22 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>   		entry->eax = entry->ebx = entry->ecx = 0;
>   		break;
>   	case 0x80000008: {
> -		unsigned g_phys_as = (entry->eax >> 16) & 0xff;
> -		unsigned virt_as = max((entry->eax >> 8) & 0xff, 48U);
> -		unsigned phys_as = entry->eax & 0xff;
> +		unsigned int virt_as = max((entry->eax >> 8) & 0xff, 48U);
> +		unsigned int phys_as;
>   
> -		/*
> -		 * If TDP (NPT) is disabled use the adjusted host MAXPHYADDR as
> -		 * the guest operates in the same PA space as the host, i.e.
> -		 * reductions in MAXPHYADDR for memory encryption affect shadow
> -		 * paging, too.
> -		 *
> -		 * If TDP is enabled but an explicit guest MAXPHYADDR is not
> -		 * provided, use the raw bare metal MAXPHYADDR as reductions to
> -		 * the HPAs do not affect GPAs.
> -		 */
> -		if (!tdp_enabled)
> -			g_phys_as = boot_cpu_data.x86_phys_bits;
> -		else if (!g_phys_as)
> -			g_phys_as = phys_as;
> +		if (!tdp_enabled) {
> +			/*
> +			 * If TDP (NPT) is disabled use the adjusted host
> +			 * MAXPHYADDR as the guest operates in the same PA
> +			 * space as the host, i.e.  reductions in MAXPHYADDR
> +			 * for memory encryption affect shadow paging, too.
> +			 */

I suggest keeping as the original comment to stay out of if check, and 
keeping the second paragraph with guest MAXPHYADDR thing removed as 
below, because the second paragraph is also useful.

   If TDP is enabled, use the raw bare metal MAXPHYADDR as reductions to
   the HPAs do not affect GPAs.

Otherwise,

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> +			phys_as = boot_cpu_data.x86_phys_bits;
> +		} else {
> +			phys_as = entry->eax & 0xff;
> +		}
>   
> -		entry->eax = g_phys_as | (virt_as << 8);
> +		entry->eax = phys_as | (virt_as << 8);
>   		entry->ecx &= ~(GENMASK(31, 16) | GENMASK(11, 8));
>   		entry->edx = 0;
>   		cpuid_entry_override(entry, CPUID_8000_0008_EBX);

[PATCH v3 2/2] kvm/cpuid: set proper GuestPhysBits in CPUID.0x80000008

[Gerd Hoffmann]

The AMD APM (3.35) defines GuestPhysBits (EAX[23:16]) as:

  Maximum guest physical address size in bits.  This number applies
  only to guests using nested paging.  When this field is zero, refer
  to the PhysAddrSize field for the maximum guest physical address size.

Tom Lendacky confirmed that the purpose of GuestPhysBits is software use
and KVM can use it as described below.  Hardware always returns zero
here.

Use the GuestPhysBits field to communicate the max addressable GPA to
the guest.  Typically this is identical to the max effective GPA, except
in case the CPU supports MAXPHYADDR > 48 but does not support 5-level
TDP.

GuestPhysBits is set only in case TDP is enabled, otherwise it is left
at zero.

GuestPhysBits will be used by the guest firmware to make sure resources
like PCI bars are mapped into the addressable GPA.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 arch/x86/kvm/mmu.h     |  2 ++
 arch/x86/kvm/cpuid.c   | 31 +++++++++++++++++++++++++++++--
 arch/x86/kvm/mmu/mmu.c |  5 +++++
 3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index 60f21bb4c27b..b410a227c601 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -100,6 +100,8 @@ static inline u8 kvm_get_shadow_phys_bits(void)
 	return boot_cpu_data.x86_phys_bits;
 }
 
+u8 kvm_mmu_get_max_tdp_level(void);
+
 void kvm_mmu_set_mmio_spte_mask(u64 mmio_value, u64 mmio_mask, u64 access_mask);
 void kvm_mmu_set_me_spte_mask(u64 me_value, u64 me_mask);
 void kvm_mmu_set_ept_masks(bool has_ad_bits, bool has_exec_only);
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index c638b5fb2144..cd627dead9ce 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1221,8 +1221,22 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
 		entry->eax = entry->ebx = entry->ecx = 0;
 		break;
 	case 0x80000008: {
+		/*
+		 * GuestPhysAddrSize (EAX[23:16]) is intended for software
+		 * use.
+		 *
+		 * KVM's ABI is to report the effective MAXPHYADDR for the
+		 * guest in PhysAddrSize (phys_as), and the maximum
+		 * *addressable* GPA in GuestPhysAddrSize (g_phys_as).
+		 *
+		 * GuestPhysAddrSize is valid if and only if TDP is enabled,
+		 * in which case the max GPA that can be addressed by KVM may
+		 * be less than the max GPA that can be legally generated by
+		 * the guest, e.g. if MAXPHYADDR>48 but the CPU doesn't
+		 * support 5-level TDP.
+		 */
 		unsigned int virt_as = max((entry->eax >> 8) & 0xff, 48U);
-		unsigned int phys_as;
+		unsigned int phys_as, g_phys_as;
 
 		if (!tdp_enabled) {
 			/*
@@ -1232,11 +1246,24 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
 			 * for memory encryption affect shadow paging, too.
 			 */
 			phys_as = boot_cpu_data.x86_phys_bits;
+			g_phys_as = 0;
 		} else {
+			/*
+			 * If TDP is enabled, the effective guest MAXPHYADDR
+			 * is the same as the raw bare metal MAXPHYADDR, as
+			 * reductions to HPAs don't affect GPAs.  The max
+			 * addressable GPA is the same as the max effective
+			 * GPA, except that it's capped at 48 bits if 5-level
+			 * TDP isn't supported (hardware processes bits 51:48
+			 * only when walking the fifth level page table).
+			 */
 			phys_as = entry->eax & 0xff;
+			g_phys_as = phys_as;
+			if (kvm_mmu_get_max_tdp_level() < 5)
+				g_phys_as = min(g_phys_as, 48);
 		}
 
-		entry->eax = phys_as | (virt_as << 8);
+		entry->eax = phys_as | (virt_as << 8) | (g_phys_as << 16);
 		entry->ecx &= ~(GENMASK(31, 16) | GENMASK(11, 8));
 		entry->edx = 0;
 		cpuid_entry_override(entry, CPUID_8000_0008_EBX);
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 2d6cdeab1f8a..ffd32400fd8c 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -5267,6 +5267,11 @@ static inline int kvm_mmu_get_tdp_level(struct kvm_vcpu *vcpu)
 	return max_tdp_level;
 }
 
+u8 kvm_mmu_get_max_tdp_level(void)
+{
+	return tdp_root_level ? tdp_root_level : max_tdp_level;
+}
+
 static union kvm_mmu_page_role
 kvm_calc_tdp_mmu_root_page_role(struct kvm_vcpu *vcpu,
 				union kvm_cpu_role cpu_role)
-- 
2.44.0

Re: [PATCH v3 2/2] kvm/cpuid: set proper GuestPhysBits in CPUID.0x80000008

[Xiaoyao Li]

On 3/11/2024 6:41 PM, Gerd Hoffmann wrote:
> The AMD APM (3.35) defines GuestPhysBits (EAX[23:16]) as:
> 
>    Maximum guest physical address size in bits.  This number applies
>    only to guests using nested paging.  When this field is zero, refer
>    to the PhysAddrSize field for the maximum guest physical address size.
> 
> Tom Lendacky confirmed that the purpose of GuestPhysBits is software use
> and KVM can use it as described below.  Hardware always returns zero
> here.
> 
> Use the GuestPhysBits field to communicate the max addressable GPA to
> the guest.  Typically this is identical to the max effective GPA, except
> in case the CPU supports MAXPHYADDR > 48 but does not support 5-level
> TDP.
> 
> GuestPhysBits is set only in case TDP is enabled, otherwise it is left
> at zero.
> 
> GuestPhysBits will be used by the guest firmware to make sure resources
> like PCI bars are mapped into the addressable GPA.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   arch/x86/kvm/mmu.h     |  2 ++
>   arch/x86/kvm/cpuid.c   | 31 +++++++++++++++++++++++++++++--
>   arch/x86/kvm/mmu/mmu.c |  5 +++++
>   3 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> index 60f21bb4c27b..b410a227c601 100644
> --- a/arch/x86/kvm/mmu.h
> +++ b/arch/x86/kvm/mmu.h
> @@ -100,6 +100,8 @@ static inline u8 kvm_get_shadow_phys_bits(void)
>   	return boot_cpu_data.x86_phys_bits;
>   }
>   
> +u8 kvm_mmu_get_max_tdp_level(void);
> +
>   void kvm_mmu_set_mmio_spte_mask(u64 mmio_value, u64 mmio_mask, u64 access_mask);
>   void kvm_mmu_set_me_spte_mask(u64 me_value, u64 me_mask);
>   void kvm_mmu_set_ept_masks(bool has_ad_bits, bool has_exec_only);
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index c638b5fb2144..cd627dead9ce 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1221,8 +1221,22 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>   		entry->eax = entry->ebx = entry->ecx = 0;
>   		break;
>   	case 0x80000008: {
> +		/*
> +		 * GuestPhysAddrSize (EAX[23:16]) is intended for software
> +		 * use.
> +		 *
> +		 * KVM's ABI is to report the effective MAXPHYADDR for the
> +		 * guest in PhysAddrSize (phys_as), and the maximum
> +		 * *addressable* GPA in GuestPhysAddrSize (g_phys_as).
> +		 *
> +		 * GuestPhysAddrSize is valid if and only if TDP is enabled,
> +		 * in which case the max GPA that can be addressed by KVM may
> +		 * be less than the max GPA that can be legally generated by
> +		 * the guest, e.g. if MAXPHYADDR>48 but the CPU doesn't
> +		 * support 5-level TDP.
> +		 */
>   		unsigned int virt_as = max((entry->eax >> 8) & 0xff, 48U);
> -		unsigned int phys_as;
> +		unsigned int phys_as, g_phys_as;
>   
>   		if (!tdp_enabled) {
>   			/*
> @@ -1232,11 +1246,24 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>   			 * for memory encryption affect shadow paging, too.
>   			 */
>   			phys_as = boot_cpu_data.x86_phys_bits;
> +			g_phys_as = 0;
>   		} else {
> +			/*
> +			 * If TDP is enabled, the effective guest MAXPHYADDR
> +			 * is the same as the raw bare metal MAXPHYADDR, as
> +			 * reductions to HPAs don't affect GPAs.  The max
> +			 * addressable GPA is the same as the max effective
> +			 * GPA, except that it's capped at 48 bits if 5-level
> +			 * TDP isn't supported (hardware processes bits 51:48
> +			 * only when walking the fifth level page table).
> +			 */

If the comment in previous patch gets changed as I suggested, this one 
needs to be updated as well.

Anyway, the patch itself looks good.

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

>   			phys_as = entry->eax & 0xff;
> +			g_phys_as = phys_as;
> +			if (kvm_mmu_get_max_tdp_level() < 5)
> +				g_phys_as = min(g_phys_as, 48);
>   		}
>   
> -		entry->eax = phys_as | (virt_as << 8);
> +		entry->eax = phys_as | (virt_as << 8) | (g_phys_as << 16);
>   		entry->ecx &= ~(GENMASK(31, 16) | GENMASK(11, 8));
>   		entry->edx = 0;
>   		cpuid_entry_override(entry, CPUID_8000_0008_EBX);
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 2d6cdeab1f8a..ffd32400fd8c 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -5267,6 +5267,11 @@ static inline int kvm_mmu_get_tdp_level(struct kvm_vcpu *vcpu)
>   	return max_tdp_level;
>   }
>   
> +u8 kvm_mmu_get_max_tdp_level(void)
> +{
> +	return tdp_root_level ? tdp_root_level : max_tdp_level;
> +}
> +
>   static union kvm_mmu_page_role
>   kvm_calc_tdp_mmu_root_page_role(struct kvm_vcpu *vcpu,
>   				union kvm_cpu_role cpu_role)

Re: [PATCH v3 2/2] kvm/cpuid: set proper GuestPhysBits in CPUID.0x80000008

[Tao Su]

On Mon, Mar 11, 2024 at 11:41:17AM +0100, Gerd Hoffmann wrote:
> The AMD APM (3.35) defines GuestPhysBits (EAX[23:16]) as:
> 
>   Maximum guest physical address size in bits.  This number applies
>   only to guests using nested paging.  When this field is zero, refer
>   to the PhysAddrSize field for the maximum guest physical address size.
> 
> Tom Lendacky confirmed that the purpose of GuestPhysBits is software use
> and KVM can use it as described below.  Hardware always returns zero
> here.
> 
> Use the GuestPhysBits field to communicate the max addressable GPA to
> the guest.  Typically this is identical to the max effective GPA, except
> in case the CPU supports MAXPHYADDR > 48 but does not support 5-level
> TDP.
> 
> GuestPhysBits is set only in case TDP is enabled, otherwise it is left
> at zero.
> 
> GuestPhysBits will be used by the guest firmware to make sure resources
> like PCI bars are mapped into the addressable GPA.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  arch/x86/kvm/mmu.h     |  2 ++
>  arch/x86/kvm/cpuid.c   | 31 +++++++++++++++++++++++++++++--
>  arch/x86/kvm/mmu/mmu.c |  5 +++++
>  3 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> index 60f21bb4c27b..b410a227c601 100644
> --- a/arch/x86/kvm/mmu.h
> +++ b/arch/x86/kvm/mmu.h
> @@ -100,6 +100,8 @@ static inline u8 kvm_get_shadow_phys_bits(void)
>  	return boot_cpu_data.x86_phys_bits;
>  }
>  
> +u8 kvm_mmu_get_max_tdp_level(void);
> +
>  void kvm_mmu_set_mmio_spte_mask(u64 mmio_value, u64 mmio_mask, u64 access_mask);
>  void kvm_mmu_set_me_spte_mask(u64 me_value, u64 me_mask);
>  void kvm_mmu_set_ept_masks(bool has_ad_bits, bool has_exec_only);
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index c638b5fb2144..cd627dead9ce 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1221,8 +1221,22 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>  		entry->eax = entry->ebx = entry->ecx = 0;
>  		break;
>  	case 0x80000008: {
> +		/*
> +		 * GuestPhysAddrSize (EAX[23:16]) is intended for software
> +		 * use.
> +		 *
> +		 * KVM's ABI is to report the effective MAXPHYADDR for the
> +		 * guest in PhysAddrSize (phys_as), and the maximum
> +		 * *addressable* GPA in GuestPhysAddrSize (g_phys_as).
> +		 *
> +		 * GuestPhysAddrSize is valid if and only if TDP is enabled,
> +		 * in which case the max GPA that can be addressed by KVM may
> +		 * be less than the max GPA that can be legally generated by
> +		 * the guest, e.g. if MAXPHYADDR>48 but the CPU doesn't
> +		 * support 5-level TDP.
> +		 */
>  		unsigned int virt_as = max((entry->eax >> 8) & 0xff, 48U);
> -		unsigned int phys_as;
> +		unsigned int phys_as, g_phys_as;
>  
>  		if (!tdp_enabled) {
>  			/*
> @@ -1232,11 +1246,24 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>  			 * for memory encryption affect shadow paging, too.
>  			 */
>  			phys_as = boot_cpu_data.x86_phys_bits;
> +			g_phys_as = 0;
>  		} else {
> +			/*
> +			 * If TDP is enabled, the effective guest MAXPHYADDR
> +			 * is the same as the raw bare metal MAXPHYADDR, as
> +			 * reductions to HPAs don't affect GPAs.  The max
> +			 * addressable GPA is the same as the max effective
> +			 * GPA, except that it's capped at 48 bits if 5-level
> +			 * TDP isn't supported (hardware processes bits 51:48
> +			 * only when walking the fifth level page table).
> +			 */
>  			phys_as = entry->eax & 0xff;
> +			g_phys_as = phys_as;
> +			if (kvm_mmu_get_max_tdp_level() < 5)
> +				g_phys_as = min(g_phys_as, 48);
>  		}
>  
> -		entry->eax = phys_as | (virt_as << 8);
> +		entry->eax = phys_as | (virt_as << 8) | (g_phys_as << 16);

When g_phys_as==phys_as, I would suggest advertising g_phys_as==0,
otherwise application can easily know whether it is in a VM, I’m
concerned this could be abused by application.

>  		entry->ecx &= ~(GENMASK(31, 16) | GENMASK(11, 8));
>  		entry->edx = 0;
>  		cpuid_entry_override(entry, CPUID_8000_0008_EBX);
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 2d6cdeab1f8a..ffd32400fd8c 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -5267,6 +5267,11 @@ static inline int kvm_mmu_get_tdp_level(struct kvm_vcpu *vcpu)
>  	return max_tdp_level;
>  }
>  
> +u8 kvm_mmu_get_max_tdp_level(void)
> +{
> +	return tdp_root_level ? tdp_root_level : max_tdp_level;
> +}
> +
>  static union kvm_mmu_page_role
>  kvm_calc_tdp_mmu_root_page_role(struct kvm_vcpu *vcpu,
>  				union kvm_cpu_role cpu_role)
> -- 
> 2.44.0
> 
> 

Re: [PATCH v3 2/2] kvm/cpuid: set proper GuestPhysBits in CPUID.0x80000008

[Xiaoyao Li]

On 3/13/2024 9:06 AM, Tao Su wrote:
> On Mon, Mar 11, 2024 at 11:41:17AM +0100, Gerd Hoffmann wrote:
>> The AMD APM (3.35) defines GuestPhysBits (EAX[23:16]) as:
>>
>>    Maximum guest physical address size in bits.  This number applies
>>    only to guests using nested paging.  When this field is zero, refer
>>    to the PhysAddrSize field for the maximum guest physical address size.
>>
>> Tom Lendacky confirmed that the purpose of GuestPhysBits is software use
>> and KVM can use it as described below.  Hardware always returns zero
>> here.
>>
>> Use the GuestPhysBits field to communicate the max addressable GPA to
>> the guest.  Typically this is identical to the max effective GPA, except
>> in case the CPU supports MAXPHYADDR > 48 but does not support 5-level
>> TDP.
>>
>> GuestPhysBits is set only in case TDP is enabled, otherwise it is left
>> at zero.
>>
>> GuestPhysBits will be used by the guest firmware to make sure resources
>> like PCI bars are mapped into the addressable GPA.
>>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>>   arch/x86/kvm/mmu.h     |  2 ++
>>   arch/x86/kvm/cpuid.c   | 31 +++++++++++++++++++++++++++++--
>>   arch/x86/kvm/mmu/mmu.c |  5 +++++
>>   3 files changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
>> index 60f21bb4c27b..b410a227c601 100644
>> --- a/arch/x86/kvm/mmu.h
>> +++ b/arch/x86/kvm/mmu.h
>> @@ -100,6 +100,8 @@ static inline u8 kvm_get_shadow_phys_bits(void)
>>   	return boot_cpu_data.x86_phys_bits;
>>   }
>>   
>> +u8 kvm_mmu_get_max_tdp_level(void);
>> +
>>   void kvm_mmu_set_mmio_spte_mask(u64 mmio_value, u64 mmio_mask, u64 access_mask);
>>   void kvm_mmu_set_me_spte_mask(u64 me_value, u64 me_mask);
>>   void kvm_mmu_set_ept_masks(bool has_ad_bits, bool has_exec_only);
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index c638b5fb2144..cd627dead9ce 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -1221,8 +1221,22 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>>   		entry->eax = entry->ebx = entry->ecx = 0;
>>   		break;
>>   	case 0x80000008: {
>> +		/*
>> +		 * GuestPhysAddrSize (EAX[23:16]) is intended for software
>> +		 * use.
>> +		 *
>> +		 * KVM's ABI is to report the effective MAXPHYADDR for the
>> +		 * guest in PhysAddrSize (phys_as), and the maximum
>> +		 * *addressable* GPA in GuestPhysAddrSize (g_phys_as).
>> +		 *
>> +		 * GuestPhysAddrSize is valid if and only if TDP is enabled,
>> +		 * in which case the max GPA that can be addressed by KVM may
>> +		 * be less than the max GPA that can be legally generated by
>> +		 * the guest, e.g. if MAXPHYADDR>48 but the CPU doesn't
>> +		 * support 5-level TDP.
>> +		 */
>>   		unsigned int virt_as = max((entry->eax >> 8) & 0xff, 48U);
>> -		unsigned int phys_as;
>> +		unsigned int phys_as, g_phys_as;
>>   
>>   		if (!tdp_enabled) {
>>   			/*
>> @@ -1232,11 +1246,24 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>>   			 * for memory encryption affect shadow paging, too.
>>   			 */
>>   			phys_as = boot_cpu_data.x86_phys_bits;
>> +			g_phys_as = 0;
>>   		} else {
>> +			/*
>> +			 * If TDP is enabled, the effective guest MAXPHYADDR
>> +			 * is the same as the raw bare metal MAXPHYADDR, as
>> +			 * reductions to HPAs don't affect GPAs.  The max
>> +			 * addressable GPA is the same as the max effective
>> +			 * GPA, except that it's capped at 48 bits if 5-level
>> +			 * TDP isn't supported (hardware processes bits 51:48
>> +			 * only when walking the fifth level page table).
>> +			 */
>>   			phys_as = entry->eax & 0xff;
>> +			g_phys_as = phys_as;
>> +			if (kvm_mmu_get_max_tdp_level() < 5)
>> +				g_phys_as = min(g_phys_as, 48);
>>   		}
>>   
>> -		entry->eax = phys_as | (virt_as << 8);
>> +		entry->eax = phys_as | (virt_as << 8) | (g_phys_as << 16);
> 
> When g_phys_as==phys_as, I would suggest advertising g_phys_as==0,
> otherwise application can easily know whether it is in a VM, I’m
> concerned this could be abused by application.

IMO, this should be protected by userspace VMM, e.g., QEMU to set actual 
g_phys_as. On KVM side, KVM only reports the capability to userspace.

>>   		entry->ecx &= ~(GENMASK(31, 16) | GENMASK(11, 8));
>>   		entry->edx = 0;
>>   		cpuid_entry_override(entry, CPUID_8000_0008_EBX);
>> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
>> index 2d6cdeab1f8a..ffd32400fd8c 100644
>> --- a/arch/x86/kvm/mmu/mmu.c
>> +++ b/arch/x86/kvm/mmu/mmu.c
>> @@ -5267,6 +5267,11 @@ static inline int kvm_mmu_get_tdp_level(struct kvm_vcpu *vcpu)
>>   	return max_tdp_level;
>>   }
>>   
>> +u8 kvm_mmu_get_max_tdp_level(void)
>> +{
>> +	return tdp_root_level ? tdp_root_level : max_tdp_level;
>> +}
>> +
>>   static union kvm_mmu_page_role
>>   kvm_calc_tdp_mmu_root_page_role(struct kvm_vcpu *vcpu,
>>   				union kvm_cpu_role cpu_role)
>> -- 
>> 2.44.0
>>
>>
> 

Re: [PATCH v3 2/2] kvm/cpuid: set proper GuestPhysBits in CPUID.0x80000008

[Gerd Hoffmann]

  Hi,

> > > -		entry->eax = phys_as | (virt_as << 8);
> > > +		entry->eax = phys_as | (virt_as << 8) | (g_phys_as << 16);
> > 
> > When g_phys_as==phys_as, I would suggest advertising g_phys_as==0,
> > otherwise application can easily know whether it is in a VM, I’m
> > concerned this could be abused by application.

There are *tons* of options to figure whenever you are running in a VM,
there is no need to go for this obscure way.

> IMO, this should be protected by userspace VMM, e.g., QEMU to set actual
> g_phys_as. On KVM side, KVM only reports the capability to userspace.

Yes, at the end of the day this is handled by qemu.

Current plan for qemu is to communicate it to the guest unconditionally
though.  When setting this only in case g_phys_as != phys_as the
firmware has the problem that it doesn't know the reason for finding
zero there.  Could be g_phys_as == phys_as, but could also be old kernel
/ qemu without GuestPhysBits support.  So the firmware doesn't know
whenever it is save to use phys_as.

take care,
  Gerd