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

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

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...

Wolfgang

> 
>  arch/x86/kernel/ipipe.c |   81 +++++++++++++++++++++++++++++-----------------
>  1 files changed, 51 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/x86/kernel/ipipe.c b/arch/x86/kernel/ipipe.c
> index 4442d96..b471355 100644
> --- a/arch/x86/kernel/ipipe.c
> +++ b/arch/x86/kernel/ipipe.c
> @@ -702,19 +702,23 @@ static int __ipipe_xlate_signo[] = {
>  
>  int __ipipe_handle_exception(struct pt_regs *regs, long error_code, int vector)
>  {
> -	unsigned long flags;
> -
> -	/* Pick up the root domain state of the interrupted context. */
> -	local_save_flags(flags);
> +	bool root_entry = false;
> +	unsigned long flags = 0;
>  
>  	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())
> +		root_entry = true;
> +
> +		local_save_flags(flags);
> +
> +		if (irqs_disabled_hw()) {
> +			/*
> +			 * 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.
> +			 */
>  			local_irq_disable();
> +		}
>  	}
>  #ifdef CONFIG_KGDB
>  	/* catch exception KGDB is interested in over non-root domains */
> @@ -725,18 +729,22 @@ 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 (root_entry)
> +			local_irq_restore_nosync(flags);
>  		return 1;
>  	}
>  
> -	/*
> -	 * 32-bit: In case we migrated to root domain inside the event
> -	 * handler, restore the original IF from exception entry as the
> -	 * low-level return code will evaluate it.
> -	 */
> -	__fixup_if(raw_irqs_disabled_flags(flags), regs);
> -
> -	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);
> +	} 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;
>  }