Re: [Xen-devel] [PATCH 1/2] x86/pv: Fix bugs with the handling of int80_bounce

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/05/17 11:52, Jan Beulich wrote:
>>>> On 08.05.17 at 12:04, <andrew.cooper3@xxxxxxxxxx> wrote:
>> Testing has revealed two issues:
>>
>>  1) Passing a NULL handle to set_trap_table() is intended to flush the entire
>>     table.  The 64bit guest case (and 32bit guest on 32bit Xen, when it
>>     existed) called init_int80_direct_trap() to reset int80_bounce, but c/s
>>     cda335c279 which introduced the 32bit guest on 64bit Xen support omitted
>>     this step.  Previously therefore, it was impossible for a 32bit guest to
>>     reset its registered int80_bounce details.
>>
>>  2) init_int80_direct_trap() doesn't honour the guests request to have
>>     interrupts disabled on entry.  PVops Linux requests that interrupts are
>>     disabled, but Xen currently leaves them enabled when following the int80
>>     fastpath.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> with a remark:
>
>> --- a/xen/arch/x86/x86_64/traps.c
>> +++ b/xen/arch/x86/x86_64/traps.c
>> @@ -427,12 +427,13 @@ void init_int80_direct_trap(struct vcpu *v)
>>      struct trap_info *ti = &v->arch.pv_vcpu.trap_ctxt[0x80];
>>      struct trap_bounce *tb = &v->arch.pv_vcpu.int80_bounce;
>>  
>> -    tb->flags = TBF_EXCEPTION;
>>      tb->cs    = ti->cs;
>>      tb->eip   = ti->address;
>>  
>>      if ( null_trap_bounce(v, tb) )
>>          tb->flags = 0;
>> +    else
>> +        tb->flags = TBF_EXCEPTION | (TI_GET_IF(ti) ? TBF_INTERRUPT : 0);
>>  }
> This certainly is a correct change to make, but it's not without risk:
> If some guest relies on previous buggy behavior (wrongly setting
> the flag but expecting interrupts to be on), ugly misbehavior in the
> guest could result. Initially I was afraid XenoLinux might be
> affected, but I've checked and it isn't.

So c/s bfad55585 (which introduces int80_bounce) is a very interesting read.

(Aside from the point here, I didn't realise that we ever had a copy of
the FreeBSD kernel in tree, or that the reason we have a separate IDT
per pcpu was for the predecessor to int80_bouce.)

At the time, there was generic set_fast_trap() which rewrote the IDT to
move straight from ring3 to ring1.  It had a few restrictions such as
only tolerating a vector of 0x80, and rejecting the setup if interrupts
were requested to be disabled (as there was no way of clearing the
evtchn_upcall_mask with this mechanism).

That patch introduced init_int80_direct_trap(), along with a comment
explaining why interrupt gates were rejected, although it was restricted
to 32bit hypervisors at that point.

The direct-trap path was never available in a 64bit build of Xen (owing
to the inability to have non long mode code segments in the IDT), and
c/s 3e1b9525de introduced the int80_direct_trap() path (which looks
remarkably unchanged in the 10 years its been in the codebase).

At this point, the 32bit version of int80_direct_trap() explained why it
couldn't tolerate interrupt gates, but the newly-introduced 64bit
version omitted any comment/code on this point, and would have worked
correctly for interrupt gates if the adjustment in this patch had been
considered.

Overall, I expect that Xenolinux purposefully never asked for an
interrupt gate (as Xen would have rejected that in the 32bit case), and
this point was never considered at all when PVOps was developed, which
followed Linux's normal expectation that int80 was an interrupt gate.

~Andrew

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