Re: [PATCH v2 1/3] KVM: X86: Move handling of INVPCID types to x86

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Babu Moger <babu.moger@amd.com> writes:

> INVPCID instruction handling is mostly same across both VMX and
> SVM. So, move the code to common x86.c.
>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  arch/x86/kvm/vmx/vmx.c |   68 +----------------------------------------
>  arch/x86/kvm/x86.c     |   79 ++++++++++++++++++++++++++++++++++++++++++++++++
>  arch/x86/kvm/x86.h     |    3 +-
>  3 files changed, 82 insertions(+), 68 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 170cc76a581f..b4140cfd15fd 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -5477,11 +5477,7 @@ static int handle_invpcid(struct kvm_vcpu *vcpu)
>  {
>  	u32 vmx_instruction_info;
>  	unsigned long type;
> -	bool pcid_enabled;
>  	gva_t gva;
> -	struct x86_exception e;
> -	unsigned i;
> -	unsigned long roots_to_free = 0;
>  	struct {
>  		u64 pcid;
>  		u64 gla;
> @@ -5508,69 +5504,7 @@ static int handle_invpcid(struct kvm_vcpu *vcpu)
>  				sizeof(operand), &gva))
>  		return 1;
>  
> -	if (kvm_read_guest_virt(vcpu, gva, &operand, sizeof(operand), &e)) {
> -		kvm_inject_emulated_page_fault(vcpu, &e);
> -		return 1;
> -	}
> -
> -	if (operand.pcid >> 12 != 0) {
> -		kvm_inject_gp(vcpu, 0);
> -		return 1;
> -	}
> -
> -	pcid_enabled = kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE);
> -
> -	switch (type) {
> -	case INVPCID_TYPE_INDIV_ADDR:
> -		if ((!pcid_enabled && (operand.pcid != 0)) ||
> -		    is_noncanonical_address(operand.gla, vcpu)) {
> -			kvm_inject_gp(vcpu, 0);
> -			return 1;
> -		}
> -		kvm_mmu_invpcid_gva(vcpu, operand.gla, operand.pcid);
> -		return kvm_skip_emulated_instruction(vcpu);
> -
> -	case INVPCID_TYPE_SINGLE_CTXT:
> -		if (!pcid_enabled && (operand.pcid != 0)) {
> -			kvm_inject_gp(vcpu, 0);
> -			return 1;
> -		}
> -
> -		if (kvm_get_active_pcid(vcpu) == operand.pcid) {
> -			kvm_mmu_sync_roots(vcpu);
> -			kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
> -		}
> -
> -		for (i = 0; i < KVM_MMU_NUM_PREV_ROOTS; i++)
> -			if (kvm_get_pcid(vcpu, vcpu->arch.mmu->prev_roots[i].pgd)
> -			    == operand.pcid)
> -				roots_to_free |= KVM_MMU_ROOT_PREVIOUS(i);
> -
> -		kvm_mmu_free_roots(vcpu, vcpu->arch.mmu, roots_to_free);
> -		/*
> -		 * If neither the current cr3 nor any of the prev_roots use the
> -		 * given PCID, then nothing needs to be done here because a
> -		 * resync will happen anyway before switching to any other CR3.
> -		 */
> -
> -		return kvm_skip_emulated_instruction(vcpu);
> -
> -	case INVPCID_TYPE_ALL_NON_GLOBAL:
> -		/*
> -		 * Currently, KVM doesn't mark global entries in the shadow
> -		 * page tables, so a non-global flush just degenerates to a
> -		 * global flush. If needed, we could optimize this later by
> -		 * keeping track of global entries in shadow page tables.
> -		 */
> -
> -		/* fall-through */
> -	case INVPCID_TYPE_ALL_INCL_GLOBAL:
> -		kvm_mmu_unload(vcpu);
> -		return kvm_skip_emulated_instruction(vcpu);
> -
> -	default:
> -		BUG(); /* We have already checked above that type <= 3 */
> -	}
> +	return kvm_handle_invpcid_types(vcpu,  gva, type);

Nit: redundant space.

>  }
>  
>  static int handle_pml_full(struct kvm_vcpu *vcpu)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 9e41b5135340..9c858ca0e592 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -70,6 +70,7 @@
>  #include <asm/irq_remapping.h>
>  #include <asm/mshyperv.h>
>  #include <asm/hypervisor.h>
> +#include <asm/tlbflush.h>
>  #include <asm/intel_pt.h>
>  #include <asm/emulate_prefix.h>
>  #include <clocksource/hyperv_timer.h>
> @@ -10714,6 +10715,84 @@ u64 kvm_spec_ctrl_valid_bits(struct kvm_vcpu *vcpu)
>  }
>  EXPORT_SYMBOL_GPL(kvm_spec_ctrl_valid_bits);
>  
> +int kvm_handle_invpcid_types(struct kvm_vcpu *vcpu, gva_t gva,
> +			     unsigned long type)

(sorry if this was discussed before) do we really need '_types' suffix?

> +{
> +	unsigned long roots_to_free = 0;
> +	struct x86_exception e;
> +	bool pcid_enabled;
> +	unsigned int i;
> +	struct {
> +		u64 pcid;
> +		u64 gla;
> +	} operand;
> +
> +	if (kvm_read_guest_virt(vcpu, gva, &operand, sizeof(operand), &e)) {
> +		kvm_inject_emulated_page_fault(vcpu, &e);
> +		return 1;
> +	}
> +
> +	if (operand.pcid >> 12 != 0) {
> +		kvm_inject_gp(vcpu, 0);
> +		return 1;
> +	}
> +
> +	pcid_enabled = kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE);
> +
> +	switch (type) {
> +	case INVPCID_TYPE_INDIV_ADDR:
> +		if ((!pcid_enabled && (operand.pcid != 0)) ||
> +		    is_noncanonical_address(operand.gla, vcpu)) {
> +			kvm_inject_gp(vcpu, 0);
> +			return 1;
> +		}
> +		kvm_mmu_invpcid_gva(vcpu, operand.gla, operand.pcid);
> +		return kvm_skip_emulated_instruction(vcpu);
> +
> +	case INVPCID_TYPE_SINGLE_CTXT:
> +		if (!pcid_enabled && (operand.pcid != 0)) {
> +			kvm_inject_gp(vcpu, 0);
> +			return 1;
> +		}
> +
> +		if (kvm_get_active_pcid(vcpu) == operand.pcid) {
> +			kvm_mmu_sync_roots(vcpu);
> +			kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
> +		}
> +
> +		for (i = 0; i < KVM_MMU_NUM_PREV_ROOTS; i++)
> +			if (kvm_get_pcid(vcpu, vcpu->arch.mmu->prev_roots[i].pgd)
> +			    == operand.pcid)
> +				roots_to_free |= KVM_MMU_ROOT_PREVIOUS(i);
> +
> +		kvm_mmu_free_roots(vcpu, vcpu->arch.mmu, roots_to_free);
> +		/*
> +		 * If neither the current cr3 nor any of the prev_roots use the
> +		 * given PCID, then nothing needs to be done here because a
> +		 * resync will happen anyway before switching to any other CR3.
> +		 */
> +
> +		return kvm_skip_emulated_instruction(vcpu);
> +
> +	case INVPCID_TYPE_ALL_NON_GLOBAL:
> +		/*
> +		 * Currently, KVM doesn't mark global entries in the shadow
> +		 * page tables, so a non-global flush just degenerates to a
> +		 * global flush. If needed, we could optimize this later by
> +		 * keeping track of global entries in shadow page tables.
> +		 */
> +
> +		/* fall-through */
> +	case INVPCID_TYPE_ALL_INCL_GLOBAL:
> +		kvm_mmu_unload(vcpu);
> +		return kvm_skip_emulated_instruction(vcpu);
> +
> +	default:
> +		BUG(); /* We have already checked above that type <= 3 */

The check was left in VMX' handle_invpcid() so we either need to update
the comment to something like "the caller was supposed to check that
type <= 3" or move the check to kvm_handle_invpcid_types().

> +	}
> +}
> +EXPORT_SYMBOL_GPL(kvm_handle_invpcid_types);
> +
>  EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_exit);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_fast_mmio);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_inj_virq);
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index 6eb62e97e59f..f706f6f7196d 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -365,5 +365,6 @@ void kvm_load_guest_xsave_state(struct kvm_vcpu *vcpu);
>  void kvm_load_host_xsave_state(struct kvm_vcpu *vcpu);
>  u64 kvm_spec_ctrl_valid_bits(struct kvm_vcpu *vcpu);
>  bool kvm_vcpu_exit_request(struct kvm_vcpu *vcpu);
> -
> +int kvm_handle_invpcid_types(struct kvm_vcpu *vcpu, gva_t gva,
> +			     unsigned long type);
>  #endif
>

-- 
Vitaly