Re: kernel: supervisor trap asynchronous system trap, code=0

[yamt%mwd.biglobe.ne.jp@localhost (YAMAMOTO Takashi)]

> On Sat, Jun 30, 2007 at 08:09:47PM +0900, YAMAMOTO Takashi wrote:
> > > Index: i386/locore.S
> > > ===================================================================
> > > RCS file: /cvsroot/src/sys/arch/xen/i386/locore.S,v
> > > retrieving revision 1.25
> > > diff -u -r1.25 locore.S
> > > --- i386/locore.S 17 May 2007 14:51:35 -0000      1.25
> > > +++ i386/locore.S 28 Jun 2007 13:27:49 -0000
> > > @@ -663,6 +663,7 @@
> > >    * Switch to newlwp's stack.
> > >    */
> > >  
> > > + CLI(%ebx)
> > >   movl    L_ADDR(%edi),%ebx
> > >   movl    PCB_EBP(%ebx),%ebp
> > >   movl    PCB_ESP(%ebx),%esp
> > 
> > can you explain why it's necessary?
> 
> I didn't find why an interrupt coming in there would cause problem,
> but I can confirm that this prevent the KASSERT panic that was being reported.
> Also, I notice that the code in netbsd-4 does the process stack swicthing
> with interrupts disabled.

can you narrow down which code is really need to be protected wrt KASSERT?

> > anyway, please don't call ras_lookup with interrupt disabled.
> 
> OK. I just wanted to make sure the new context was fully installed before
> re-taking interrupts. I didn't notice a C function was being called.
> 
> > 
> > > @@ -780,9 +789,29 @@
> > >   call    _C_LABEL(trap)
> > >   addl    $4,%esp
> > >   jmp     .Lsyscall_checkast
> > > -1:       STI(%eax)
> > > - CHECK_DEFERRED_SWITCH(%eax)
> > > +1:       CHECK_DEFERRED_SWITCH(%eax)
> > >   jnz     9f
> > > + STIC(%eax)
> > > + jz      14f
> > > + call    _C_LABEL(stipending)
> > > + testl   %eax,%eax
> > > + jz      14f
> > > + /* process pending interrupts */
> > > + CLI(%eax)
> > > + movl    CPUVAR(ILEVEL), %ebx
> > > + movl    $.Lsyscall_resume, %esi # address to resume loop at
> > > +.Lsyscall_resume:
> > > + movl    %ebx,%eax               # get cpl
> > > + movl    CPUVAR(IUNMASK)(,%eax,4),%eax
> > > + andl    CPUVAR(IPENDING),%eax   # any non-masked bits left?
> > > + jz      17f
> > > + bsrl    %eax,%eax
> > > + btrl    %eax,CPUVAR(IPENDING)
> > > + movl    CPUVAR(ISOURCES)(,%eax,4),%eax
> > > + jmp     *IS_RESUME(%eax)
> > > +17:      movl    %ebx, CPUVAR(ILEVEL)    #restore cpl
> > > + jmp     .Lsyscall_checkast
> > > +14:
> > >  #ifndef DIAGNOSTIC
> > >   INTRFASTEXIT
> > >  #else /* DIAGNOSTIC */
> > 
> > can't these duplicated code be shared?
> 
> it could probably, but this would make some more call/return, or jump.
> I'm not sure it's worth it.

it's worth, IMO.

> > i'm not sure why stipending() was needed in the first place,
> > in addition to __sti() and do_hypervisor_callback().
> 
> __sti() is intended for C code, but it takes an extrat trap and 4 context
> switches to process pending interrupts. processing interrupts in the current
> context it more efficient.

why can't you use do_hypervisor_callback()?

YAMAMOTO Takashi