|
[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 |