With the severe stability issues we are having with SLE10sp1 on x86-64, things
start pointing pretty closely at the int80 direct trap patch we imported from
-unstable. While I just now realized that there''s been a fix for these
problems
for quite a while (don''t know how this slipped my attention), I still
have a few
notes:
- even compat_restore_all_guest now asserts interrupts are disabled, despite
32-bit restore_all_guest not doing so (and the iret path not generally needing
this)
- int80_direct_trap checks for non-zero TRAPBOUNCE_flags, yet
{,compat_}create_bounce_frame clear the low byte of these flags (i.e.
including TBF_exception, which is in this lower byte); it appears to be only a
lucky coincidence that this still works as the cmp (again!) is suffix-less and
hence gets sized as a 32-bit compare, accidentally covering TRAPBOUNCE_cs
- from the above, why is it that only the lower byte (if anything) needs
clearing?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
On 25/4/07 10:56, "Jan Beulich" <jbeulich@novell.com> wrote:> - even compat_restore_all_guest now asserts interrupts are disabled, despite > 32-bit restore_all_guest not doing so (and the iret path not generally > needing this)The 32-bit restore_all_guest ought to have the assertion added. I must have forgotten about it. Interrupts need to be disabled during return to guest so that the tests for softirq work and event-notification to the guest do not race against new interrupts. Of course this issue is much less fatal than the restore-rsp;sysret bug!> - int80_direct_trap checks for non-zero TRAPBOUNCE_flags, yet > {,compat_}create_bounce_frame clear the low byte of these flags (i.e. > including TBF_exception, which is in this lower byte); it appears to be only > a lucky coincidence that this still works as the cmp (again!) is suffix-less > and hence gets sized as a 32-bit compare, accidentally coveringTRAPBOUNCE_csOoo, good catch. It''s a tiny bit gross but the best fix is probably just to restore the flags field after the call to create_bounce_frame. And of course change the cmp to a cmpb. Agree?> - from the above, why is it that only the lower byte (if anything) needs > clearing?Really it''s a one-byte field: it''s consistently treated that way in asm code. The upper byte is always zero. We should probably make the field explicitly uint8_t. Agree? -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 25/4/07 11:10, "Keir Fraser" <keir@xensource.com> wrote:>> - from the above, why is it that only the lower byte (if anything) needs >> clearing? > > Really it''s a one-byte field: it''s consistently treated that way in asm > code. The upper byte is always zero. We should probably make the field > explicitly uint8_t. Agree?FYI, we need to clear that field usually because the contents are one-shot but the structure itself is permanent and checked on every return to guest. A non-zero flags field is what we use to indicate whether the structure is ''primed'' or not. So the passing in of a different, non-one-shot, structure on the direct-int80 path is a bit abusive of the interface. But I think we can live with it if we reset the flags field manually and include a comment. :-) -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>> - int80_direct_trap checks for non-zero TRAPBOUNCE_flags, yet >> {,compat_}create_bounce_frame clear the low byte of these flags (i.e. >> including TBF_exception, which is in this lower byte); it appears to be only >> a lucky coincidence that this still works as the cmp (again!) is suffix-less >> and hence gets sized as a 32-bit compare, accidentally coveringTRAPBOUNCE_cs > >Ooo, good catch. It''s a tiny bit gross but the best fix is probably just to >restore the flags field after the call to create_bounce_frame. And of course >change the cmp to a cmpb. Agree?That''s the alternative solution I considered. The preferable one is to do the compat/native distinction before the null check, and then be consistent with the rest of the code and check cs for 32-bit guest and eip for 64-bit ones. That''s how I''m preparing a patch right now.>> - from the above, why is it that only the lower byte (if anything) needs >> clearing? > >Really it''s a one-byte field: it''s consistently treated that way in asm >code. The upper byte is always zero. We should probably make the field >explicitly uint8_t. Agree?Making it a uint8_t is fine. It is, however, far from being consistently handled in assembly code: x86_32/entry.S: 4 word refs and 3 byte refs x86_64/entry.S: 6 word refs, 3 byte refs, and one size-less ref x86_64/compat/entry.S: 4 word refs and 3 byte refs Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 25/4/07 11:33, "Jan Beulich" <jbeulich@novell.com> wrote:> That''s the alternative solution I considered. The preferable one is to do the > compat/native distinction before the null check, and then be consistent with > the rest of the code and check cs for 32-bit guest and eip for 64-bit ones. > That''s how I''m preparing a patch right now.Attached is my own proposed patch which I think cleans up all the issues. Checking just flags in asm and keeping the null-bounce check in init_int80_direct_trap() seems fine to me. -- Keir>>> - from the above, why is it that only the lower byte (if anything) needs >>> clearing? >> >> Really it''s a one-byte field: it''s consistently treated that way in asm >> code. The upper byte is always zero. We should probably make the field >> explicitly uint8_t. Agree? > > Making it a uint8_t is fine. It is, however, far from being consistently > handled > in assembly code: > x86_32/entry.S: 4 word refs and 3 byte refs > x86_64/entry.S: 6 word refs, 3 byte refs, and one size-less ref > x86_64/compat/entry.S: 4 word refs and 3 byte refs_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 25/4/07 11:41, "Keir Fraser" <keir@xensource.com> wrote:> Attached is my own proposed patch which I think cleans up all the issues. > Checking just flags in asm and keeping the null-bounce check in > init_int80_direct_trap() seems fine to me.The change of a movw $0 to a movb $TBF_EXCEPTION in that patch is wrong, by the way. Should be movb $0. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> Keir Fraser <keir@xensource.com> 25.04.07 12:56 >>> >On 25/4/07 11:41, "Keir Fraser" <keir@xensource.com> wrote: > >> Attached is my own proposed patch which I think cleans up all the issues. >> Checking just flags in asm and keeping the null-bounce check in >> init_int80_direct_trap() seems fine to me. > >The change of a movw $0 to a movb $TBF_EXCEPTION in that patch is wrong, by >the way. Should be movb $0.Which means there''s not really a dependency on this being non-zero... The patch looks otherwise okay to me, though I think there''s one more issue here: There''s another suffix-less instruction (updating UREGS_rip in int80_slow_path) - this must be a subq, and it must imply that no 32-bit guest places an int $0x80 at 0xfffffffe. And my patch has a not directly related adjustment removing the movl $TRAP_syscall,UREGS_entry_vector+8(%rsp) close to the end of compat_create_bounce_frame, as this is meaningless here. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 25/4/07 12:11, "Jan Beulich" <jbeulich@novell.com> wrote:> Which means there''s not really a dependency on this being non-zero...Yeah, I limited that dependency just to the handle_exception path. It now zeroes the flags field itself which. It might be even cleaner to host the zero of the flags field to before the indirect call to the exception-specific C handler (that''s the only thing that might set up the bounce structure).> The patch looks otherwise okay to me, though I think there''s one more > issue here: There''s another suffix-less instruction (updating UREGS_rip > in int80_slow_path) - this must be a subq, and it must imply that no 32-bit > guest places an int $0x80 at 0xfffffffe.Yep.> And my patch has a not directly related adjustment removing the > > movl $TRAP_syscall,UREGS_entry_vector+8(%rsp) > > close to the end of compat_create_bounce_frame, as this is meaningless > here.Should we also remove it from compat_hypercall? -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>> And my patch has a not directly related adjustment removing the >> >> movl $TRAP_syscall,UREGS_entry_vector+8(%rsp) >> >> close to the end of compat_create_bounce_frame, as this is meaningless >> here. > >Should we also remove it from compat_hypercall?Probably not, to be able to identify the frame properly in case something crashes in the process of handling the hypercall. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel