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 <[email protected]>
> ---
>
> 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;
> }
_______________________________________________
Adeos-main mailing list
[email protected]
https://mail.gna.org/listinfo/adeos-main
