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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/05/17 15:49, Jan Beulich wrote:
>>>> 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.

Hmm.  This was a straight copy of the logic to switch between the exit
paths.

I think I will prepare a fix for this general issue in a separate patch,
to avoid being mixed in with this logical change.

>
>> +        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.

Ah yes.  I had a bug originally where I was moving %ecx rather than %cl
and clobbering flags, so reintroduced the suffixes to match the struct
trap_bounce definition.

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

This code (hopefully) isn't going to stay around long, but it is
certainly not removed in this series.  I will try to leave it in a good
state.

~Andrew

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