[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 1/3] x86/svm: Remove list functionality from __get_instruction_length_* infrastructure
On 12/31/18 5:37 AM, Andrew Cooper wrote: > The existing __get_instruction_length_from_list() has a single user > which uses the list functionality. That user however should be looking > specifically for INVD or WBINVD, as reported by the vmexit exit reason. > > Modify svm_vmexit_do_invalidate_cache() to ask for the correct > instruction, and drop all list functionality from the helper. > > Take the opportunity to rename it to svm_get_insn_len(), and drop the > IOIO length handling whch has never been used. > > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx> Acked-by: Brian Woods <brian.woods@xxxxxxx> > --- > CC: Wei Liu <wei.liu2@xxxxxxxxxx> > CC: Roger Pau Monné <roger.pau@xxxxxxxxxx> > CC: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> > CC: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx> > CC: Brian Woods <brian.woods@xxxxxxx> > > v2: > * New > v3: > * Deduplicate the calls to svm_nextrip_insn_length() > --- > xen/arch/x86/hvm/svm/emulate.c | 76 > +++++++++++++++++------------------ > xen/arch/x86/hvm/svm/nestedsvm.c | 9 +++-- > xen/arch/x86/hvm/svm/svm.c | 39 +++++++++--------- > xen/include/asm-x86/hvm/svm/emulate.h | 9 +---- > 4 files changed, 61 insertions(+), 72 deletions(-) > > diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c > index 4abeab8..7799908 100644 > --- a/xen/arch/x86/hvm/svm/emulate.c > +++ b/xen/arch/x86/hvm/svm/emulate.c > @@ -84,28 +84,31 @@ static const struct { > [INSTR_CPUID] = { X86EMUL_OPC(0x0f, 0xa2) }, > }; > > -int __get_instruction_length_from_list(struct vcpu *v, > - const enum instruction_index *list, unsigned int list_count) > +/* > + * First-gen SVM didn't have the NextRIP feature, meaning that when we take a > + * fault-style vmexit, we have to decode the instruction stream to calculate > + * how many bytes to move %rip forwards by. > + * > + * To double check the implementation, in debug builds, always compare the > + * hardware reported instruction length (if available) with the result from > + * x86_decode_insn(). > + */ > +unsigned int svm_get_insn_len(struct vcpu *v, enum instruction_index insn) > { > struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb; > struct hvm_emulate_ctxt ctxt; > struct x86_emulate_state *state; > - unsigned long inst_len, j; > + unsigned long nrip_len, emul_len; > unsigned int modrm_rm, modrm_reg; > int modrm_mod; > > - /* > - * In debug builds, always use x86_decode_insn() and compare with > - * hardware. > - */ > -#ifdef NDEBUG > - if ( (inst_len = svm_nextrip_insn_length(v)) > MAX_INST_LEN ) > - gprintk(XENLOG_WARNING, "NRip reported inst_len %lu\n", inst_len); > - else if ( inst_len != 0 ) > - return inst_len; > + nrip_len = svm_nextrip_insn_length(v); > > - if ( vmcb->exitcode == VMEXIT_IOIO ) > - return vmcb->exitinfo2 - vmcb->rip; > +#ifdef NDEBUG > + if ( nrip_len > MAX_INST_LEN ) > + gprintk(XENLOG_WARNING, "NRip reported inst_len %lu\n", nrip_len); > + else if ( nrip_len != 0 ) > + return nrip_len; > #endif > > ASSERT(v == current); > @@ -115,41 +118,34 @@ int __get_instruction_length_from_list(struct vcpu *v, > if ( IS_ERR_OR_NULL(state) ) > return 0; > > - inst_len = x86_insn_length(state, &ctxt.ctxt); > + emul_len = x86_insn_length(state, &ctxt.ctxt); > modrm_mod = x86_insn_modrm(state, &modrm_rm, &modrm_reg); > x86_emulate_free_state(state); > + > #ifndef NDEBUG > - if ( vmcb->exitcode == VMEXIT_IOIO ) > - j = vmcb->exitinfo2 - vmcb->rip; > - else > - j = svm_nextrip_insn_length(v); > - if ( j && j != inst_len ) > + if ( nrip_len && nrip_len != emul_len ) > { > gprintk(XENLOG_WARNING, "insn-len[%02x]=%lu (exp %lu)\n", > - ctxt.ctxt.opcode, inst_len, j); > - return j; > + ctxt.ctxt.opcode, nrip_len, emul_len); > + return nrip_len; > } > #endif > > - for ( j = 0; j < list_count; j++ ) > + if ( insn >= ARRAY_SIZE(opc_tab) ) > { > - unsigned int instr = list[j]; > - > - if ( instr >= ARRAY_SIZE(opc_tab) ) > - { > - ASSERT_UNREACHABLE(); > - break; > - } > - if ( opc_tab[instr].opcode == ctxt.ctxt.opcode ) > - { > - if ( !opc_tab[instr].modrm.mod ) > - return inst_len; > - > - if ( modrm_mod == opc_tab[instr].modrm.mod && > - (modrm_rm & 7) == opc_tab[instr].modrm.rm && > - (modrm_reg & 7) == opc_tab[instr].modrm.reg ) > - return inst_len; > - } > + ASSERT_UNREACHABLE(); > + return 0; > + } > + > + if ( opc_tab[insn].opcode == ctxt.ctxt.opcode ) > + { > + if ( !opc_tab[insn].modrm.mod ) > + return emul_len; > + > + if ( modrm_mod == opc_tab[insn].modrm.mod && > + (modrm_rm & 7) == opc_tab[insn].modrm.rm && > + (modrm_reg & 7) == opc_tab[insn].modrm.reg ) > + return emul_len; > } > > gdprintk(XENLOG_WARNING, > diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c > b/xen/arch/x86/hvm/svm/nestedsvm.c > index 9660202..35c1a04 100644 > --- a/xen/arch/x86/hvm/svm/nestedsvm.c > +++ b/xen/arch/x86/hvm/svm/nestedsvm.c > @@ -743,8 +743,9 @@ nsvm_vcpu_vmrun(struct vcpu *v, struct cpu_user_regs > *regs) > struct nestedvcpu *nv = &vcpu_nestedhvm(v); > struct nestedsvm *svm = &vcpu_nestedsvm(v); > > - inst_len = __get_instruction_length(v, INSTR_VMRUN); > - if (inst_len == 0) { > + inst_len = svm_get_insn_len(v, INSTR_VMRUN); > + if ( inst_len == 0 ) > + { > svm->ns_vmexit.exitcode = VMEXIT_SHUTDOWN; > return -1; > } > @@ -1616,7 +1617,7 @@ void svm_vmexit_do_stgi(struct cpu_user_regs *regs, > struct vcpu *v) > return; > } > > - if ( (inst_len = __get_instruction_length(v, INSTR_STGI)) == 0 ) > + if ( (inst_len = svm_get_insn_len(v, INSTR_STGI)) == 0 ) > return; > > nestedsvm_vcpu_stgi(v); > @@ -1637,7 +1638,7 @@ void svm_vmexit_do_clgi(struct cpu_user_regs *regs, > struct vcpu *v) > return; > } > > - if ( (inst_len = __get_instruction_length(v, INSTR_CLGI)) == 0 ) > + if ( (inst_len = svm_get_insn_len(v, INSTR_CLGI)) == 0 ) > return; > > nestedsvm_vcpu_clgi(v); > diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c > index 954822c..2584b90 100644 > --- a/xen/arch/x86/hvm/svm/svm.c > +++ b/xen/arch/x86/hvm/svm/svm.c > @@ -2244,8 +2244,8 @@ static void svm_do_msr_access(struct cpu_user_regs > *regs) > { > struct vcpu *curr = current; > bool rdmsr = curr->arch.hvm.svm.vmcb->exitinfo1 == 0; > - int rc, inst_len = __get_instruction_length( > - curr, rdmsr ? INSTR_RDMSR : INSTR_WRMSR); > + int rc, inst_len = svm_get_insn_len(curr, rdmsr ? INSTR_RDMSR > + : INSTR_WRMSR); > > if ( inst_len == 0 ) > return; > @@ -2272,7 +2272,7 @@ static void svm_vmexit_do_hlt(struct vmcb_struct *vmcb, > { > unsigned int inst_len; > > - if ( (inst_len = __get_instruction_length(current, INSTR_HLT)) == 0 ) > + if ( (inst_len = svm_get_insn_len(current, INSTR_HLT)) == 0 ) > return; > __update_guest_eip(regs, inst_len); > > @@ -2283,7 +2283,6 @@ static void svm_vmexit_do_rdtsc(struct cpu_user_regs > *regs, bool rdtscp) > { > struct vcpu *curr = current; > const struct domain *currd = curr->domain; > - enum instruction_index insn = rdtscp ? INSTR_RDTSCP : INSTR_RDTSC; > unsigned int inst_len; > > if ( rdtscp && !currd->arch.cpuid->extd.rdtscp ) > @@ -2292,7 +2291,8 @@ static void svm_vmexit_do_rdtsc(struct cpu_user_regs > *regs, bool rdtscp) > return; > } > > - if ( (inst_len = __get_instruction_length(curr, insn)) == 0 ) > + if ( (inst_len = svm_get_insn_len(curr, rdtscp ? INSTR_RDTSCP > + : INSTR_RDTSC)) == 0 ) > return; > > __update_guest_eip(regs, inst_len); > @@ -2307,7 +2307,7 @@ static void svm_vmexit_do_pause(struct cpu_user_regs > *regs) > { > unsigned int inst_len; > > - if ( (inst_len = __get_instruction_length(current, INSTR_PAUSE)) == 0 ) > + if ( (inst_len = svm_get_insn_len(current, INSTR_PAUSE)) == 0 ) > return; > __update_guest_eip(regs, inst_len); > > @@ -2374,7 +2374,7 @@ svm_vmexit_do_vmload(struct vmcb_struct *vmcb, > unsigned int inst_len; > struct page_info *page; > > - if ( (inst_len = __get_instruction_length(v, INSTR_VMLOAD)) == 0 ) > + if ( (inst_len = svm_get_insn_len(v, INSTR_VMLOAD)) == 0 ) > return; > > if ( !nsvm_efer_svm_enabled(v) ) > @@ -2409,7 +2409,7 @@ svm_vmexit_do_vmsave(struct vmcb_struct *vmcb, > unsigned int inst_len; > struct page_info *page; > > - if ( (inst_len = __get_instruction_length(v, INSTR_VMSAVE)) == 0 ) > + if ( (inst_len = svm_get_insn_len(v, INSTR_VMSAVE)) == 0 ) > return; > > if ( !nsvm_efer_svm_enabled(v) ) > @@ -2477,13 +2477,12 @@ static void svm_wbinvd_intercept(void) > flush_all(FLUSH_CACHE); > } > > -static void svm_vmexit_do_invalidate_cache(struct cpu_user_regs *regs) > +static void svm_vmexit_do_invalidate_cache(struct cpu_user_regs *regs, > + bool invld) > { > - static const enum instruction_index list[] = { INSTR_INVD, INSTR_WBINVD > }; > - int inst_len; > + unsigned int inst_len = svm_get_insn_len(current, invld ? INSTR_INVD > + : INSTR_WBINVD); > > - inst_len = __get_instruction_length_from_list( > - current, list, ARRAY_SIZE(list)); > if ( inst_len == 0 ) > return; > > @@ -2758,7 +2757,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs) > else > { > trap_type = X86_EVENTTYPE_PRI_SW_EXCEPTION; > - inst_len = __get_instruction_length(v, INSTR_ICEBP); > + inst_len = svm_get_insn_len(v, INSTR_ICEBP); > } > > rc = hvm_monitor_debug(regs->rip, > @@ -2775,7 +2774,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs) > break; > > case VMEXIT_EXCEPTION_BP: > - inst_len = __get_instruction_length(v, INSTR_INT3); > + inst_len = svm_get_insn_len(v, INSTR_INT3); > > if ( inst_len == 0 ) > break; > @@ -2866,7 +2865,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs) > > case VMEXIT_INVD: > case VMEXIT_WBINVD: > - svm_vmexit_do_invalidate_cache(regs); > + svm_vmexit_do_invalidate_cache(regs, exit_reason == VMEXIT_INVD); > break; > > case VMEXIT_TASK_SWITCH: { > @@ -2895,7 +2894,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs) > > case VMEXIT_CPUID: > { > - unsigned int inst_len = __get_instruction_length(v, INSTR_CPUID); > + unsigned int inst_len = svm_get_insn_len(v, INSTR_CPUID); > int rc = 0; > > if ( inst_len == 0 ) > @@ -2951,14 +2950,14 @@ void svm_vmexit_handler(struct cpu_user_regs *regs) > hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC); > break; > } > - if ( (inst_len = __get_instruction_length(v, INSTR_INVLPGA)) == 0 ) > + if ( (inst_len = svm_get_insn_len(v, INSTR_INVLPGA)) == 0 ) > break; > svm_invlpga_intercept(v, regs->rax, regs->ecx); > __update_guest_eip(regs, inst_len); > break; > > case VMEXIT_VMMCALL: > - if ( (inst_len = __get_instruction_length(v, INSTR_VMCALL)) == 0 ) > + if ( (inst_len = svm_get_insn_len(v, INSTR_VMCALL)) == 0 ) > break; > BUG_ON(vcpu_guestmode); > HVMTRACE_1D(VMMCALL, regs->eax); > @@ -3012,7 +3011,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs) > case VMEXIT_XSETBV: > if ( vmcb_get_cpl(vmcb) ) > hvm_inject_hw_exception(TRAP_gp_fault, 0); > - else if ( (inst_len = __get_instruction_length(v, INSTR_XSETBV)) && > + else if ( (inst_len = svm_get_insn_len(v, INSTR_XSETBV)) && > hvm_handle_xsetbv(regs->ecx, msr_fold(regs)) == > X86EMUL_OKAY ) > __update_guest_eip(regs, inst_len); > break; > diff --git a/xen/include/asm-x86/hvm/svm/emulate.h > b/xen/include/asm-x86/hvm/svm/emulate.h > index ca92abb..82359ec 100644 > --- a/xen/include/asm-x86/hvm/svm/emulate.h > +++ b/xen/include/asm-x86/hvm/svm/emulate.h > @@ -45,14 +45,7 @@ enum instruction_index { > > struct vcpu; > > -int __get_instruction_length_from_list( > - struct vcpu *, const enum instruction_index *, unsigned int list_count); > - > -static inline int __get_instruction_length( > - struct vcpu *v, enum instruction_index instr) > -{ > - return __get_instruction_length_from_list(v, &instr, 1); > -} > +unsigned int svm_get_insn_len(struct vcpu *v, enum instruction_index instr); > > #endif /* __ASM_X86_HVM_SVM_EMULATE_H__ */ > > _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |