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.kis...@siemens.com>
> ---
> 
> 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;
>  }


_______________________________________________
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Reply via email to