Re: [Xen-devel] [PATCH 1/4] x86/pv: Drop int80_bounce from struct pv_vcpu

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.05.17 at 17:48, <andrew.cooper3@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -233,12 +233,36 @@ UNLIKELY_END(msi_check)
>  
>          GET_CURRENT(bx)
>  
> -        /* Check that the callback is non-null. */
> -        leaq  VCPU_int80_bounce(%rbx),%rdx
> -        cmpb  $0,TRAPBOUNCE_flags(%rdx)
> +        mov   VCPU_trap_ctxt(%rbx), %rsi
> +        mov   VCPU_domain(%rbx), %rax
> +
> +        /*
> +         * if ( null_trap_bounce(v, &v->arch.pv_vcpu.trap_ctxt[0x80]) )
> +         *    goto int80_slow_path;
> +         */
> +        mov    0x80 * TRAPINFO_sizeof + TRAPINFO_eip(%rsi), %rdi
> +        movzwl 0x80 * TRAPINFO_sizeof + TRAPINFO_cs (%rsi), %ecx
> +
> +        mov   %ecx, %edx
> +        and   $~3, %edx
> +
> +        testb $1, DOMAIN_is_32bit_pv(%rax)

While we may have other such instances, this is dangerous (and long
ago I think we had actual issues with constructs like this one). Either
"cmpb" against zero, or "testb" with 0xff.

> +        cmove %rdi, %rdx

As there's nothing "equal" here, but only the question of ZF being
set of clear, the more natural form would be "cmovz" (just like you
use "jz" and "setnz" below).

> +        test  %rdx, %rdx
>          jz    int80_slow_path
>  
> -        movq  VCPU_domain(%rbx),%rax
> +        /* Construct trap_bounce from trap_ctxt[0x80]. */
> +        lea   VCPU_trap_bounce(%rbx), %rdx
> +        movw  %cx, TRAPBOUNCE_cs(%rdx)
> +        movq  %rdi, TRAPBOUNCE_eip(%rdx)
> +
> +        /* TB_flags = TBF_EXCEPTION | (TI_GET_IF(ti) ? TBF_INTERRUPT : 0); */
> +        testb $4, 0x80 * TRAPINFO_sizeof + TRAPINFO_flags(%rsi)
> +        setnz %cl
> +        lea   TBF_EXCEPTION(, %rcx, TBF_INTERRUPT), %ecx
> +        movb  %cl, TRAPBOUNCE_flags(%rdx)

In code further up you avoided mnemonic suffixes where possible,
yet in this code section you're (partly) using them even when a
register operand makes them redundant.

Since I understand this code will go away with subsequent patches
anyway, I don't insist on changing these though. Hence with or
without the adjustments
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel