All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] KVM: nVMX: allow tests to use bad virtual-APIC page address
@ 2019-04-15 13:35 Paolo Bonzini
  2019-04-19 22:33 ` Krish Sadhukhan
  2019-04-20  5:35 ` Sean Christopherson
  0 siblings, 2 replies; 3+ messages in thread
From: Paolo Bonzini @ 2019-04-15 13:35 UTC (permalink / raw)
  To: linux-kernel, kvm

As mentioned in the comment, there are some special cases where we can simply
clear the TPR shadow bit from the CPU-based execution controls in the vmcs02.
Handle them so that we can remove some XFAILs from vmx.flat.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/vmx/nested.c | 25 ++++++++++++++++---------
 arch/x86/kvm/vmx/vmx.c    |  2 +-
 arch/x86/kvm/vmx/vmx.h    |  2 ++
 3 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 7ec9bb1dd723..a22af5a85540 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2873,20 +2873,27 @@ static void nested_get_vmcs12_pages(struct kvm_vcpu *vcpu)
 		/*
 		 * If translation failed, VM entry will fail because
 		 * prepare_vmcs02 set VIRTUAL_APIC_PAGE_ADDR to -1ull.
-		 * Failing the vm entry is _not_ what the processor
-		 * does but it's basically the only possibility we
-		 * have.  We could still enter the guest if CR8 load
-		 * exits are enabled, CR8 store exits are enabled, and
-		 * virtualize APIC access is disabled; in this case
-		 * the processor would never use the TPR shadow and we
-		 * could simply clear the bit from the execution
-		 * control.  But such a configuration is useless, so
-		 * let's keep the code simple.
 		 */
 		if (!is_error_page(page)) {
 			vmx->nested.virtual_apic_page = page;
 			hpa = page_to_phys(vmx->nested.virtual_apic_page);
 			vmcs_write64(VIRTUAL_APIC_PAGE_ADDR, hpa);
+		} else if (nested_cpu_has(vmcs12, CPU_BASED_CR8_LOAD_EXITING) &&
+		           nested_cpu_has(vmcs12, CPU_BASED_CR8_STORE_EXITING) &&
+			   !nested_cpu_has2(vmcs12, SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES)) {
+			/*
+			 * The processor will never use the TPR shadow, simply
+			 * clear the bit from the execution control.  Such a
+			 * configuration is useless, but it happens in tests.
+			 * For any other configuration, failing the vm entry is
+			 * _not_ what the processor does but it's basically the
+			 * only possibility we have.
+			 */
+			vmcs_clear_bits(CPU_BASED_VM_EXEC_CONTROL,
+					CPU_BASED_TPR_SHADOW);
+		} else {
+			printk("bad virtual-APIC page address\n");
+			dump_vmcs();
 		}
 	}
 
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index f8054dc1de65..14cacfd7ffd0 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5603,7 +5603,7 @@ static void vmx_dump_dtsel(char *name, uint32_t limit)
 	       vmcs_readl(limit + GUEST_GDTR_BASE - GUEST_GDTR_LIMIT));
 }
 
-static void dump_vmcs(void)
+void dump_vmcs(void)
 {
 	u32 vmentry_ctl = vmcs_read32(VM_ENTRY_CONTROLS);
 	u32 vmexit_ctl = vmcs_read32(VM_EXIT_CONTROLS);
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index a1e00d0a2482..f879529906b4 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -517,4 +517,6 @@ static inline void decache_tsc_multiplier(struct vcpu_vmx *vmx)
 	vmcs_write64(TSC_MULTIPLIER, vmx->current_tsc_ratio);
 }
 
+void dump_vmcs(void);
+
 #endif /* __KVM_X86_VMX_H */
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] KVM: nVMX: allow tests to use bad virtual-APIC page address
  2019-04-15 13:35 [PATCH] KVM: nVMX: allow tests to use bad virtual-APIC page address Paolo Bonzini
@ 2019-04-19 22:33 ` Krish Sadhukhan
  2019-04-20  5:35 ` Sean Christopherson
  1 sibling, 0 replies; 3+ messages in thread
From: Krish Sadhukhan @ 2019-04-19 22:33 UTC (permalink / raw)
  To: Paolo Bonzini, linux-kernel, kvm



On 04/15/2019 06:35 AM, Paolo Bonzini wrote:
> As mentioned in the comment, there are some special cases where we can simply
> clear the TPR shadow bit from the CPU-based execution controls in the vmcs02.
> Handle them so that we can remove some XFAILs from vmx.flat.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   arch/x86/kvm/vmx/nested.c | 25 ++++++++++++++++---------
>   arch/x86/kvm/vmx/vmx.c    |  2 +-
>   arch/x86/kvm/vmx/vmx.h    |  2 ++
>   3 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 7ec9bb1dd723..a22af5a85540 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2873,20 +2873,27 @@ static void nested_get_vmcs12_pages(struct kvm_vcpu *vcpu)
>   		/*
>   		 * If translation failed, VM entry will fail because
>   		 * prepare_vmcs02 set VIRTUAL_APIC_PAGE_ADDR to -1ull.
> -		 * Failing the vm entry is _not_ what the processor
> -		 * does but it's basically the only possibility we
> -		 * have.  We could still enter the guest if CR8 load
> -		 * exits are enabled, CR8 store exits are enabled, and
> -		 * virtualize APIC access is disabled; in this case
> -		 * the processor would never use the TPR shadow and we
> -		 * could simply clear the bit from the execution
> -		 * control.  But such a configuration is useless, so
> -		 * let's keep the code simple.
>   		 */
>   		if (!is_error_page(page)) {
>   			vmx->nested.virtual_apic_page = page;
>   			hpa = page_to_phys(vmx->nested.virtual_apic_page);
>   			vmcs_write64(VIRTUAL_APIC_PAGE_ADDR, hpa);
> +		} else if (nested_cpu_has(vmcs12, CPU_BASED_CR8_LOAD_EXITING) &&
> +		           nested_cpu_has(vmcs12, CPU_BASED_CR8_STORE_EXITING) &&
> +			   !nested_cpu_has2(vmcs12, SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES)) {
> +			/*
> +			 * The processor will never use the TPR shadow, simply
> +			 * clear the bit from the execution control.  Such a
> +			 * configuration is useless, but it happens in tests.
> +			 * For any other configuration, failing the vm entry is
> +			 * _not_ what the processor does but it's basically the
> +			 * only possibility we have.
> +			 */
> +			vmcs_clear_bits(CPU_BASED_VM_EXEC_CONTROL,
> +					CPU_BASED_TPR_SHADOW);
> +		} else {
> +			printk("bad virtual-APIC page address\n");
> +			dump_vmcs();
>   		}
>   	}
>   
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index f8054dc1de65..14cacfd7ffd0 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -5603,7 +5603,7 @@ static void vmx_dump_dtsel(char *name, uint32_t limit)
>   	       vmcs_readl(limit + GUEST_GDTR_BASE - GUEST_GDTR_LIMIT));
>   }
>   
> -static void dump_vmcs(void)
> +void dump_vmcs(void)
>   {
>   	u32 vmentry_ctl = vmcs_read32(VM_ENTRY_CONTROLS);
>   	u32 vmexit_ctl = vmcs_read32(VM_EXIT_CONTROLS);
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index a1e00d0a2482..f879529906b4 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -517,4 +517,6 @@ static inline void decache_tsc_multiplier(struct vcpu_vmx *vmx)
>   	vmcs_write64(TSC_MULTIPLIER, vmx->current_tsc_ratio);
>   }
>   
> +void dump_vmcs(void);
> +
>   #endif /* __KVM_X86_VMX_H */

Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] KVM: nVMX: allow tests to use bad virtual-APIC page address
  2019-04-15 13:35 [PATCH] KVM: nVMX: allow tests to use bad virtual-APIC page address Paolo Bonzini
  2019-04-19 22:33 ` Krish Sadhukhan
@ 2019-04-20  5:35 ` Sean Christopherson
  1 sibling, 0 replies; 3+ messages in thread
From: Sean Christopherson @ 2019-04-20  5:35 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, kvm

On Mon, Apr 15, 2019 at 03:35:33PM +0200, Paolo Bonzini wrote:
> As mentioned in the comment, there are some special cases where we can simply
> clear the TPR shadow bit from the CPU-based execution controls in the vmcs02.
> Handle them so that we can remove some XFAILs from vmx.flat.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 25 ++++++++++++++++---------
>  arch/x86/kvm/vmx/vmx.c    |  2 +-
>  arch/x86/kvm/vmx/vmx.h    |  2 ++
>  3 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 7ec9bb1dd723..a22af5a85540 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2873,20 +2873,27 @@ static void nested_get_vmcs12_pages(struct kvm_vcpu *vcpu)
>  		/*
>  		 * If translation failed, VM entry will fail because
>  		 * prepare_vmcs02 set VIRTUAL_APIC_PAGE_ADDR to -1ull.
> -		 * Failing the vm entry is _not_ what the processor
> -		 * does but it's basically the only possibility we
> -		 * have.  We could still enter the guest if CR8 load
> -		 * exits are enabled, CR8 store exits are enabled, and
> -		 * virtualize APIC access is disabled; in this case
> -		 * the processor would never use the TPR shadow and we
> -		 * could simply clear the bit from the execution
> -		 * control.  But such a configuration is useless, so
> -		 * let's keep the code simple.
>  		 */
>  		if (!is_error_page(page)) {
>  			vmx->nested.virtual_apic_page = page;
>  			hpa = page_to_phys(vmx->nested.virtual_apic_page);
>  			vmcs_write64(VIRTUAL_APIC_PAGE_ADDR, hpa);
> +		} else if (nested_cpu_has(vmcs12, CPU_BASED_CR8_LOAD_EXITING) &&
> +		           nested_cpu_has(vmcs12, CPU_BASED_CR8_STORE_EXITING) &&
> +			   !nested_cpu_has2(vmcs12, SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES)) {
> +			/*
> +			 * The processor will never use the TPR shadow, simply
> +			 * clear the bit from the execution control.  Such a
> +			 * configuration is useless, but it happens in tests.
> +			 * For any other configuration, failing the vm entry is
> +			 * _not_ what the processor does but it's basically the
> +			 * only possibility we have.
> +			 */
> +			vmcs_clear_bits(CPU_BASED_VM_EXEC_CONTROL,
> +					CPU_BASED_TPR_SHADOW);
> +		} else {
> +			printk("bad virtual-APIC page address\n");
> +			dump_vmcs();

I don't think we should dump the VMCS here, or have any form of print
at all.  dump_vmcs() is especially bad as it allows userspace to spam
the kernel log at the error level.  I haven't actually checked, but I
assume the nested consistency check tracing patch would be a better
way to debug something of this nature in production.

>  		}
>  	}
>  
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index f8054dc1de65..14cacfd7ffd0 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -5603,7 +5603,7 @@ static void vmx_dump_dtsel(char *name, uint32_t limit)
>  	       vmcs_readl(limit + GUEST_GDTR_BASE - GUEST_GDTR_LIMIT));
>  }
>  
> -static void dump_vmcs(void)
> +void dump_vmcs(void)
>  {
>  	u32 vmentry_ctl = vmcs_read32(VM_ENTRY_CONTROLS);
>  	u32 vmexit_ctl = vmcs_read32(VM_EXIT_CONTROLS);
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index a1e00d0a2482..f879529906b4 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -517,4 +517,6 @@ static inline void decache_tsc_multiplier(struct vcpu_vmx *vmx)
>  	vmcs_write64(TSC_MULTIPLIER, vmx->current_tsc_ratio);
>  }
>  
> +void dump_vmcs(void);
> +
>  #endif /* __KVM_X86_VMX_H */
> -- 
> 1.8.3.1
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-04-20  5:35 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-15 13:35 [PATCH] KVM: nVMX: allow tests to use bad virtual-APIC page address Paolo Bonzini
2019-04-19 22:33 ` Krish Sadhukhan
2019-04-20  5:35 ` Sean Christopherson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.