Re: [Xenomai-core] [PATCH v2] x86: Fix root domain state restoring on exception return

[Wolfgang Mauerer <wolfgang.mauerer@domain.hid>]

Jan Kiszka wrote:
> Wolfgang Mauerer wrote:
>> Jan Kiszka wrote:
>>> If we enter __ipipe_handle_exception over a non-root domain and leave it
>>> due to migration in the event handler over root, we must not restore the
>>> root domain state so far saved on entry. This caused subtle pipeline
>>> state corruptions. Instead, only save and restore them if we were
>>> entering over root.
>>>
>>> However, the x86-32 regs.flags fixup is required nevertheless to take
>>> care of mismatches between the root domain state and the hardware flags
>>> on entry. That may happen if we fault in the iret path. But also in this
>>> case we must not restore an invalid root domain state. So if we entered
>>> over non-root, pick up the input for __fixup_if from the root domain
>>> after running the ipipe handler.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>> ---
>>>
>>> Next try. But this time I think I finally understood what scenario
>>> __fixup_if is actually fixing. Please correct me if I'm still missing
>>> one.
>> looks good - it works for my test cases and solves the problems with
>> the hw/pipeline state mismatch during early bootup. But do you happen
>> to have any scenario at hand with ipipe_domain_root_p && !root_entry?
>> Couldn't trigger this one yet so only the raw_irqs_disabled_flags
>> fixup is excercised, though I guess it can't do any harm to really
>> ensure that the explanation fits reality this time...
> 
> You mean non-root entry -> migration -> __fixup_if? In that case we pick
> up the flags for fixup _after_ the migration (raw_irqs_disabled()). Or
> what do you mean?

(...)
>>> -	if (unlikely(!ipipe_root_domain_p)) {
>>> +	if (likely(ipipe_root_domain_p)) {
>>> +		/*
>>> +		 * 32-bit: In case we faulted in the iret path, regs.flags do
>>> +		 * not match the root domain state as the low-level return
>>> +		 * code will evaluate it. Fix this up, either by the root
>>> +		 * state sampled on entry or, if we migrated to root, with the
>>> +		 * current state.
>>> +		 */
>>> +		__fixup_if(root_entry ? raw_irqs_disabled_flags(flags) :
>>> +					raw_irqs_disabled(), regs);

I'm referring to the case that evaluates to
__fixup_if(raw_irqs_disabled(), regs); That is, something that
triggers

		if (!root_entry)
			do_something();

Could be that we're talking about to the same case, although I'm not
sure ;-)

Cheers, Wolfgang


>>> +	} else {
>>>  		/* Detect unhandled faults over non-root domains. */
>>>  		struct ipipe_domain *ipd = ipipe_current_domain;
>>>  
>>> @@ -770,21 +778,29 @@ 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 (root_entry)
>>> +		local_irq_restore_nosync(flags);
>>>  
>>>  	return 0;
>>>  }
>>>  
>>>  int __ipipe_divert_exception(struct pt_regs *regs, int vector)
>>>  {
>>> -	unsigned long flags;
>>> -
>>> -	/* Same root state handling as in __ipipe_handle_exception. */
>>> -	local_save_flags(flags);
>>> +	bool root_entry = false;
>>> +	unsigned long flags = 0;
>>>  
>>>  	if (ipipe_root_domain_p) {
>>> -		if (irqs_disabled_hw())
>>> +		root_entry = true;
>>> +
>>> +		local_save_flags(flags);
>>> +
>>> +		if (irqs_disabled_hw()) {
>>> +			/*
>>> +			 * Same root state handling as in
>>> +			 * __ipipe_handle_exception.
>>> +			 */
>>>  			local_irq_disable();
>>> +		}
>>>  	}
>>>  #ifdef CONFIG_KGDB
>>>  	/* catch int1 and int3 over non-root domains */
>>> @@ -804,16 +820,21 @@ int __ipipe_divert_exception(struct pt_regs *regs, int vector)
>>>  #endif /* CONFIG_KGDB */
>>>  
>>>  	if (unlikely(ipipe_trap_notify(vector, regs))) {
>>> -		local_irq_restore_nosync(flags);
>>> +		if (root_entry)
>>> +			local_irq_restore_nosync(flags);
>>>  		return 1;
>>>  	}
>>>  
>>> +	if (likely(ipipe_root_domain_p)) {
>>> +		/* see __ipipe_handle_exception */
>>> +		__fixup_if(root_entry ? raw_irqs_disabled_flags(flags) :
>>> +					raw_irqs_disabled(), regs);
>>> +	}
>>> +
>>>  	/*
>>> -	 * 32-bit: Due to possible migration inside the event handler, we have
>>> -	 * to restore IF so that low-level return code sets the root domain
>>> -	 * state correctly.
>>> +	 * No need to restore root state in the 64-bit case, the Linux handler
>>> +	 * and the return code will take care of it.
>>>  	 */
>>> -	__fixup_if(raw_irqs_disabled_flags(flags), regs);
>>>  
>>>  	return 0;
>>>  }
> 
> Jan
>