> -----Original Message-----
> From: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> Sent: Wednesday, June 17, 2020 6:56 AM
> To: Moger, Babu <Babu.Moger@xxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx;
> wanpengli@xxxxxxxxxxx; joro@xxxxxxxxxx; x86@xxxxxxxxxx;
> sean.j.christopherson@xxxxxxxxx; mingo@xxxxxxxxxx; bp@xxxxxxxxx;
> hpa@xxxxxxxxx; pbonzini@xxxxxxxxxx; tglx@xxxxxxxxxxxxx;
> jmattson@xxxxxxxxxx
> Subject: Re: [PATCH v2 1/3] KVM: X86: Move handling of INVPCID types to x86
>
> Babu Moger <babu.moger@xxxxxxx> 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@xxxxxxx>
> > ---
> > 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
