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

[Babu Moger <babu.moger@amd.com>]

> -----Original Message-----
> From: Vitaly Kuznetsov <vkuznets@redhat.com>
> Sent: Wednesday, June 17, 2020 6:56 AM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org;
> wanpengli@tencent.com; joro@8bytes.org; x86@kernel.org;
> sean.j.christopherson@intel.com; mingo@redhat.com; bp@alien8.de;
> hpa@zytor.com; pbonzini@redhat.com; tglx@linutronix.de;
> jmattson@google.com
> Subject: Re: [PATCH v2 1/3] KVM: X86: Move handling of INVPCID types to x86
> 
> 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.

Sure. Will fix.

> 
> >  }
> >
> >  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?

Ok. I will remove _types.

> 
> > +{
> > +	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().

Ok. Will update the comment. Thanks

> 
> > +	}
> > +}
> > +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