Re: [Xen-devel] [PATCH v2 1/3] x86/svm: Simplify svm_get_insn_len()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.12.18 at 21:22, <andrew.cooper3@xxxxxxxxxx> wrote:
> @@ -98,13 +97,10 @@ int __get_instruction_length_from_list(struct vcpu *v,
>       * 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;
> -
> -    if ( vmcb->exitcode == VMEXIT_IOIO )
> -        return vmcb->exitinfo2 - vmcb->rip;
> +    if ( (nrip_len = svm_nextrip_insn_length(v)) > MAX_INST_LEN )

With this and you now using variables slightly differently, wouldn't
it be better to pull this out of the #ifdef to avoid ...

> @@ -114,46 +110,43 @@ 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 )
> +    nrip_len = svm_nextrip_insn_length(v);

... duplication here? The potentially unnecessary call would
happen in debug builds only.

Preferably with these taken care of
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel