From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <4B5D372E.2090301@domain.hid> Date: Mon, 25 Jan 2010 07:16:14 +0100 From: Gilles Chanteperdrix MIME-Version: 1.0 References: <4B5B02E3.8060103@domain.hid> <4B5CCF35.2040402@domain.hid> In-Reply-To: <4B5CCF35.2040402@domain.hid> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Xenomai-core] [RFC][PATCH] x86: Fix root domain state restoring on exception return List-Id: Xenomai life and development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: adeos-main , Wolfgang Mauerer , xenomai-core Gilles Chanteperdrix 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. Actually, we only need to save the state if we enter >> over the root domain and have to align its state to the hardware >> interrupt mask. >> >> Moreover, the x86-32 regs.eflags fix-up must happen based on the current >> root domain state to avoid more spurious corruptions. >> >> Signed-off-by: Jan Kiszka >> --- >> >> This patch is so far running fine on the x86-64 boxes of our colleagues >> @Healthcare. It currently makes most sense to me, also for (untested) >> x86-32, but maybe I'm still missing a problematic scenario. >> >> arch/x86/kernel/ipipe.c | 64 ++++++++++++++++++++++++++-------------------- >> 1 files changed, 36 insertions(+), 28 deletions(-) >> >> diff --git a/arch/x86/kernel/ipipe.c b/arch/x86/kernel/ipipe.c >> index 4442d96..8253993 100644 >> --- a/arch/x86/kernel/ipipe.c >> +++ b/arch/x86/kernel/ipipe.c >> @@ -702,19 +702,17 @@ 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 restore_flags = false; >> + unsigned long flags = 0; >> >> - if (ipipe_root_domain_p) { >> + if (ipipe_root_domain_p && irqs_disabled_hw()) { > > I really do not understand this hunk. It differs a lot from the current > situation. In the current situation __fixup_if really does something, > even if irqs were not masked on entry. > Ok, but on x86_64, you have: + bool restore_flags = false; + if (ipipe_root_domain_p && irqs_disabled_hw()) { + restore_flags = true; } + if (restore_flags) + local_irq_restore_nosync(flags); Which I do not understand. local_irq_restore_nosync still has an effect even if irqs were not disabled on entry. -- Gilles Chanteperdrix, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com