Re: [Xenomai-core] Domain switch during page fault handling

[Philippe Gerum <rpm@xenomai.org>]

On Fri, 2010-01-22 at 19:08 +0100, Philippe Gerum wrote:
> On Fri, 2010-01-22 at 19:03 +0100, Jan Kiszka wrote:
> > Philippe Gerum wrote:
> > > On Fri, 2010-01-22 at 18:41 +0100, Jan Kiszka wrote:
> > >> Gilles Chanteperdrix wrote:
> > >>> Philippe Gerum wrote:
> > >>>> On Fri, 2010-01-22 at 17:58 +0100, Jan Kiszka wrote: 
> > >>>>> Hi guys,
> > >>>>>
> > >>>>> we are currently trying to catch an ugly Linux pipeline state corruption
> > >>>>> on x86-64.
> > >>>>>
> > >>>>> Conceptual question: If a Xenomai task causes a fault, we enter
> > >>>>> ipipe_trap_notify over the primary domain and leave it over the root
> > >>>>> domain, right? Now, if the root domain happened to be stalled when the
> > >>>>> exception happened, where should it normally be unstalled again,
> > >>>>> *for_that_task*? Our problem is that we generate a code path where this
> > >>>>> does not happen.
> > >>>> xnhadow_relax -> ipipe_reenter_root -> finish_task_switch ->
> > >>>> finish_lock_switch -> unstall
> > >>>>
> > >>>> Since xnshadow_relax is called on behalf the event dispatcher, we should
> > >>>> expect it to return with the root domain unstalled after a domain
> > >>>> downgrade, from primary to root.
> > >>> Ok, but what about local_irq_restore_nosync at the end of the function ?
> > >>>
> > >> That is, IMO, our problem: It replays the root state on fault entry, but
> > >> that one is totally unrelated to the (Xenomai) task that caused the fault.
> > > 
> > > The code seems fishy. Try restoring only when the incoming domain was
> > > the root one. Indeed.
> > > 
> > 
> > Something like this?
> > 
> > diff --git a/arch/x86/kernel/ipipe.c b/arch/x86/kernel/ipipe.c
> > index 4442d96..0558ea3 100644
> > --- a/arch/x86/kernel/ipipe.c
> > +++ b/arch/x86/kernel/ipipe.c
> > @@ -702,19 +702,21 @@ static int __ipipe_xlate_signo[] = {
> >  
> >  int __ipipe_handle_exception(struct pt_regs *regs, long error_code, int vector)
> >  {
> > +	bool restore_flags = false;
> >  	unsigned long flags;
> >  
> > -	/* Pick up the root domain state of the interrupted context. */
> > -	local_save_flags(flags);
> > +	if (ipipe_root_domain_p && irqs_disabled_hw()) {
> > +		/* Pick up the root domain state of the interrupted context. */
> > +		local_save_flags(flags);
> >  
> > -	if (ipipe_root_domain_p) {
> >  		/*
> >  		 * Replicate hw interrupt state into the virtual mask before
> >  		 * calling the I-pipe event handler over the root domain. Also
> >  		 * required later when calling the Linux exception handler.
> >  		 */
> > -		if (irqs_disabled_hw())
> > -			local_irq_disable();
> > +		local_irq_disable();
> > +
> > +		restore_flags = true;
> >  	}
> >  #ifdef CONFIG_KGDB
> >  	/* catch exception KGDB is interested in over non-root domains */
> > @@ -725,7 +727,8 @@ int __ipipe_handle_exception(struct pt_regs *regs, long error_code, int vector)
> >  #endif /* CONFIG_KGDB */
> >  
> >  	if (unlikely(ipipe_trap_notify(vector, regs))) {
> > -		local_irq_restore_nosync(flags);
> > +		if (restore_flags)
> > +			local_irq_restore_nosync(flags);
> >  		return 1;
> >  	}
> >  
> > @@ -770,7 +773,8 @@ int __ipipe_handle_exception(struct pt_regs *regs, long error_code, int vector)
> >  	 * Relevant for 64-bit: Restore root domain state as the low-level
> >  	 * return code will not align it to regs.flags.
> >  	 */
> > -	local_irq_restore_nosync(flags);
> > +	if (restore_flags)
> > +		local_irq_restore_nosync(flags);
> >  
> >  	return 0;
> >  }
> > 
> > 
> > We are currently not able to test this on the system that triggers it,
> > but we'll do so tomorrow (yeah...).
> > 
> 
> Should work. Famous last words.
> 

Strike that. This won't work, because the fixup code will use the saved
flags even when root is not the incoming domain and/or hw IRQs are on on
entry. In short, local_save_flags() must be done unconditionally, as
previously.

> > Jan
> > 
> 
> 


-- 
Philippe.