x86: Fix root domain state restoring on exception return
PATCH v2 1/1 x86: Fix root domain state restoring on exception return
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.
---
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.
arch/x86/kernel/ipipe.c | 81 +++++++++++++++++++++++++++++----------------- 1 files changed, 51 insertions(+), 30 deletions(-)
arch/x86/kernel/ipipe.c +51 -30
@@ -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 ; }
Signature · 1 line
1.6.0.2
Quoted 13 lines · Jan Kiszka
If we enter
__ipipe_handle_exceptionover 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_iffrom the root domain after running the ipipe handler.
Quoted 6 lines
Signed-off-by Jan Kiszka <[email protected]>---
Next try. But this time I think I finally understood what scenario
__fixup_ifis 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
Quoted 137 lines
arch/x86/kernel/ipipe.c | 81 +++++++++++++++++++++++++++++----------------- 1 files changed, 51 insertions(+), 30 deletions(-)arch/x86/kernel/ipipe.c +51
-30@@ -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 ; }
Quoted 21 lines · Wolfgang Mauerer
Quoted 13 lines · Jan Kiszka
If we enter
__ipipe_handle_exceptionover 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_iffrom the root domain after running the ipipe handler.Quoted 6 lines
Signed-off-by Jan Kiszka <[email protected]>---
Next try. But this time I think I finally understood what scenario
__fixup_ifis actually fixing. Please correct me if I'm still missing one.
Quoted 6 lines
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 theraw_irqs_disabled_flagsfixup 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?
Quoted 137 lines
Wolfgang
Quoted 134 lines
arch/x86/kernel/ipipe.c | 81 +++++++++++++++++++++++++++++----------------- 1 files changed, 51 insertions(+), 30 deletions(-)arch/x86/kernel/ipipe.c +51
-30@@ -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);
Quoted 3 lines
arch/x86/kernel/ipipe.c int __ipipe_divert_exception(struct pt_regs *regs, int vector) · line 840
Quoted 2 lines
arch/x86/kernel/ipipe.c +0
-0return 0 ; }
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 31 lines · Jan Kiszka
Quoted 20 lines · Wolfgang Mauerer
Quoted 13 lines · Jan Kiszka
If we enter
__ipipe_handle_exceptionover 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_iffrom the root domain after running the ipipe handler.Quoted 6 lines
Signed-off-by Jan Kiszka <[email protected]>---
Next try. But this time I think I finally understood what scenario
__fixup_ifis actually fixing. Please correct me if I'm still missing one.Quoted 6 lines
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 theraw_irqs_disabled_flagsfixup 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?
(...)
Quoted 11 lines
Quoted 11 lines
arch/x86/kernel/ipipe.c int __ipipe_handle_exception(struct pt_regs *regs, long error_code, int vector) · line 746
Quoted 11 lines
arch/x86/kernel/ipipe.c +10
-1-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
Quoted 70 lines
Quoted 65 lines
Quoted 65 lines
arch/x86/kernel/ipipe.c +25
-11+ }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);Quoted 2 lines
Review int __ipipe_divert_exception(struct pt_regs *regs, int vector) · line 840
Quoted 2 lines
(quoted diff) +0
-0return 0; }Jan
Quoted 49 lines · Wolfgang Mauerer
Quoted 30 lines · Jan Kiszka
Quoted 20 lines · Wolfgang Mauerer
Quoted 13 lines · Jan Kiszka
If we enter
__ipipe_handle_exceptionover 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_iffrom the root domain after running the ipipe handler.Quoted 6 lines
Signed-off-by Jan Kiszka <[email protected]>---
Next try. But this time I think I finally understood what scenario
__fixup_ifis actually fixing. Please correct me if I'm still missing one.Quoted 6 lines
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 theraw_irqs_disabled_flagsfixup 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?(...)
Quoted 11 lines
Quoted 11 lines
arch/x86/kernel/ipipe.c int __ipipe_handle_exception(struct pt_regs *regs, long error_code, int vector) · line 746
Quoted 11 lines
arch/x86/kernel/ipipe.c +10
-1-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
Quoted 5 lines
if (!root_entry) do_something();Could be that we're talking about to the same case, although I'm not sure ;-)
Right, that's the case I described above. What problem do you precisely see or what concerns to you have about the suggested behavior?
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 48 lines · Jan Kiszka
Quoted 47 lines · Wolfgang Mauerer
Quoted 30 lines · Jan Kiszka
Quoted 20 lines · Wolfgang Mauerer
Quoted 13 lines · Jan Kiszka
If we enter
__ipipe_handle_exceptionover 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_iffrom the root domain after running the ipipe handler.Quoted 6 lines
Signed-off-by Jan Kiszka <[email protected]>---
Next try. But this time I think I finally understood what scenario
__fixup_ifis actually fixing. Please correct me if I'm still missing one.Quoted 6 lines
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 theraw_irqs_disabled_flagsfixup 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?(...)
Quoted 11 lines
Quoted 11 lines
arch/x86/kernel/ipipe.c int __ipipe_handle_exception(struct pt_regs *regs, long error_code, int vector) · line 746
Quoted 11 lines
arch/x86/kernel/ipipe.c +10
-1-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
Quoted 8 lines
Quoted 5 lines
if (!root_entry) do_something();Could be that we're talking about to the same case, although I'm not sure ;-)
Right, that's the case I described above. What problem do you precisely see or what concerns to you have about the suggested behavior?
None. I'd just like to be able to trigger it to avoid that there are any unforseen problems we're still missing. Since this corner of Ipipe seems to have proven tricky before AFAIK, I thought it might perhaps be worth while to really excercise every possible code path.
Wolfgang
Quoted 54 lines · Wolfgang Mauerer
Quoted 53 lines · Jan Kiszka
Quoted 52 lines · Wolfgang Mauerer
Quoted 30 lines · Jan Kiszka
Quoted 20 lines · Wolfgang Mauerer
Quoted 13 lines · Jan Kiszka
If we enter
__ipipe_handle_exceptionover 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_iffrom the root domain after running the ipipe handler.Quoted 6 lines
Signed-off-by Jan Kiszka <[email protected]>---
Next try. But this time I think I finally understood what scenario
__fixup_ifis actually fixing. Please correct me if I'm still missing one.Quoted 6 lines
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 theraw_irqs_disabled_flagsfixup 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?(...)
Quoted 11 lines
Quoted 11 lines
arch/x86/kernel/ipipe.c int __ipipe_handle_exception(struct pt_regs *regs, long error_code, int vector) · line 746
Quoted 11 lines
arch/x86/kernel/ipipe.c +10
-1-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 triggersif (!root_entry) do_something();Could be that we're talking about to the same case, although I'm not sure ;-)
Quoted 8 lines
Quoted 2 lines
Right, that's the case I described above. What problem do you precisely see or what concerns to you have about the suggested behavior?
None. I'd just like to be able to trigger it to avoid that there are any unforseen problems we're still missing. Since this corner of Ipipe seems to have proven tricky before AFAIK, I thought it might perhaps be worth while to really excercise every possible code path.
To trigger it, you need to recreate the scenario that our colleagues managed to generate on the MARS: Have Linux IRQs disabled (or enabled, to have both variations), schedule in a Xenomai task, let it raise a fault so that Xenomai migrates it back.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 63 lines · Jan Kiszka
Quoted 54 lines · Wolfgang Mauerer
Quoted 53 lines · Jan Kiszka
Quoted 52 lines · Wolfgang Mauerer
Quoted 30 lines · Jan Kiszka
Quoted 20 lines · Wolfgang Mauerer
Quoted 13 lines · Jan Kiszka
If we enter
__ipipe_handle_exceptionover 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_iffrom the root domain after running the ipipe handler.Quoted 6 lines
Signed-off-by Jan Kiszka <[email protected]>---
Next try. But this time I think I finally understood what scenario
__fixup_ifis actually fixing. Please correct me if I'm still missing one.Quoted 6 lines
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 theraw_irqs_disabled_flagsfixup 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?(...)
Quoted 11 lines
Quoted 11 lines
Review
Quoted 11 lines
(quoted diff) +10
-1- 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 triggersif (!root_entry) do_something();Could be that we're talking about to the same case, although I'm not sure ;-)
Quoted 7 lines
Quoted 2 lines
Right, that's the case I described above. What problem do you precisely see or what concerns to you have about the suggested behavior?
None. I'd just like to be able to trigger it to avoid that there are any unforseen problems we're still missing. Since this corner of Ipipe seems to have proven tricky before AFAIK, I thought it might perhaps be worth while to really excercise every possible code path.
Quoted 4 lines
To trigger it, you need to recreate the scenario that our colleagues managed to generate on the MARS: Have Linux IRQs disabled (or enabled, to have both variations), schedule in a Xenomai task, let it raise a fault so that Xenomai migrates it back.
Yep, but allocating a large amount of memory in non-rt, switching to rt and then accessing this and switching back to non-rt in the hope of generating many faults so that the scenario would be synthesised without expensive and hard to access hardware was unsuccessful, so my little innocent question was really just if you happened to have a testcase.
But as I said in the very beginning, the patch nevertheless looks good to me.
Cheers, Wolfgang
Quoted 68 lines · Wolfgang Mauerer
Quoted 62 lines · Jan Kiszka
Quoted 54 lines · Wolfgang Mauerer
Quoted 53 lines · Jan Kiszka
Quoted 52 lines · Wolfgang Mauerer
Quoted 30 lines · Jan Kiszka
Quoted 20 lines · Wolfgang Mauerer
Quoted 13 lines · Jan Kiszka
If we enter
__ipipe_handle_exceptionover 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_iffrom the root domain after running the ipipe handler.Quoted 6 lines
Signed-off-by Jan Kiszka <[email protected]>---
Next try. But this time I think I finally understood what scenario
__fixup_ifis actually fixing. Please correct me if I'm still missing one.Quoted 6 lines
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 theraw_irqs_disabled_flagsfixup 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?(...)
Quoted 11 lines
Quoted 11 lines
Review
Quoted 11 lines
(quoted diff) +10
-1- 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 triggersif (!root_entry) do_something();Could be that we're talking about to the same case, although I'm not sure ;-)
Quoted 7 lines
Quoted 2 lines
Right, that's the case I described above. What problem do you precisely see or what concerns to you have about the suggested behavior?
None. I'd just like to be able to trigger it to avoid that there are any unforseen problems we're still missing. Since this corner of Ipipe seems to have proven tricky before AFAIK, I thought it might perhaps be worth while to really excercise every possible code path.
Quoted 4 lines
To trigger it, you need to recreate the scenario that our colleagues managed to generate on the MARS: Have Linux IRQs disabled (or enabled, to have both variations), schedule in a Xenomai task, let it raise a fault so that Xenomai migrates it back.
Quoted 6 lines
Yep, but allocating a large amount of memory in non-rt, switching to rt and then accessing this and switching back to non-rt in the hope of generating many faults so that the scenario would be synthesised without expensive and hard to access hardware was unsuccessful, so my little innocent question was really just if you happened to have a testcase.
OK, then I totally misunderstood your question.
You do not need to generate a fixable fault, and unfixable one (*(int *)NULL = 0;) should suffice.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 68 lines · Jan Kiszka
Quoted 67 lines · Wolfgang Mauerer
Quoted 66 lines · Jan Kiszka
Quoted 54 lines · Wolfgang Mauerer
Quoted 53 lines · Jan Kiszka
Quoted 52 lines · Wolfgang Mauerer
Quoted 30 lines · Jan Kiszka
Quoted 20 lines · Wolfgang Mauerer
Quoted 13 lines · Jan Kiszka
If we enter
__ipipe_handle_exceptionover 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_iffrom the root domain after running the ipipe handler.Quoted 6 lines
Signed-off-by Jan Kiszka <[email protected]>---
Next try. But this time I think I finally understood what scenario
__fixup_ifis actually fixing. Please correct me if I'm still missing one.Quoted 6 lines
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 theraw_irqs_disabled_flagsfixup 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?(...)
Quoted 11 lines
Quoted 11 lines
Review
Quoted 11 lines
(quoted diff) +10
-1- 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 triggersif (!root_entry) do_something();Could be that we're talking about to the same case, although I'm not sure ;-)
Quoted 7 lines
Quoted 2 lines
Right, that's the case I described above. What problem do you precisely see or what concerns to you have about the suggested behavior?
None. I'd just like to be able to trigger it to avoid that there are any unforseen problems we're still missing. Since this corner of Ipipe seems to have proven tricky before AFAIK, I thought it might perhaps be worth while to really excercise every possible code path.
To trigger it, you need to recreate the scenario that our colleagues managed to generate on the MARS: Have Linux IRQs disabled (or enabled, to have both variations), schedule in a Xenomai task, let it raise a fault so that Xenomai migrates it back.
Quoted 8 lines
Quoted 6 lines
Yep, but allocating a large amount of memory in non-rt, switching to rt and then accessing this and switching back to non-rt in the hope of generating many faults so that the scenario would be synthesised without expensive and hard to access hardware was unsuccessful, so my little innocent question was really just if you happened to have a testcase.
OK, then I totally misunderstood your question.
Seems we have nevertheless finally managed to reach an agreement on what we mean ;-)
Quoted 3 lines
You do not need to generate a fixable fault, and unfixable one (*(int *)NULL = 0;) should suffice.
yeah, although that would maybe be a bit ugly wrt. handling the SEGV and continuing to allow continuous faults without
having to restart the program all the time.
As we just discussed offline, this would anyway not yet trigger the "fault in iret"-scenario, and I don't think it's worth while spending the large extra effort for this considering that the code will sooner or later die anyway. Or does maybe anyone have an idea how to trigger such a fault with simple means?
Cheers, Wolfgang
Quoted 86 lines · Wolfgang Mauerer
Quoted 68 lines · Jan Kiszka
Quoted 67 lines · Wolfgang Mauerer
Quoted 66 lines · Jan Kiszka
Quoted 54 lines · Wolfgang Mauerer
Quoted 53 lines · Jan Kiszka
Quoted 52 lines · Wolfgang Mauerer
Quoted 30 lines · Jan Kiszka
Quoted 20 lines · Wolfgang Mauerer
Quoted 13 lines · Jan Kiszka
If we enter
__ipipe_handle_exceptionover 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_iffrom the root domain after running the ipipe handler.Quoted 6 lines
Signed-off-by Jan Kiszka <[email protected]>---
Next try. But this time I think I finally understood what scenario
__fixup_ifis actually fixing. Please correct me if I'm still missing one.Quoted 6 lines
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 theraw_irqs_disabled_flagsfixup 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?(...)
Quoted 11 lines
Quoted 11 lines
Review
Quoted 11 lines
(quoted diff) +10
-1- 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 triggersif (!root_entry) do_something();Could be that we're talking about to the same case, although I'm not sure ;-)
Quoted 7 lines
Quoted 2 lines
Right, that's the case I described above. What problem do you precisely see or what concerns to you have about the suggested behavior?
None. I'd just like to be able to trigger it to avoid that there are any unforseen problems we're still missing. Since this corner of Ipipe seems to have proven tricky before AFAIK, I thought it might perhaps be worth while to really excercise every possible code path.
To trigger it, you need to recreate the scenario that our colleagues managed to generate on the MARS: Have Linux IRQs disabled (or enabled, to have both variations), schedule in a Xenomai task, let it raise a fault so that Xenomai migrates it back.
Quoted 7 lines
Quoted 6 lines
Yep, but allocating a large amount of memory in non-rt, switching to rt and then accessing this and switching back to non-rt in the hope of generating many faults so that the scenario would be synthesised without expensive and hard to access hardware was unsuccessful, so my little innocent question was really just if you happened to have a testcase.
OK, then I totally misunderstood your question.
Seems we have nevertheless finally managed to reach an agreement on what we mean ;-)
Quoted 2 lines
You do not need to generate a fixable fault, and unfixable one (*(int *)NULL = 0;) should suffice.
yeah, although that would maybe be a bit ugly wrt. handling the SEGV and continuing to allow continuous faults without
having to restart the program all the time.
Quoted 5 lines
As we just discussed offline, this would anyway not yet trigger the "fault in iret"-scenario, and I don't think it's worth while spending the large extra effort for this considering that the code will sooner or later die anyway. Or does maybe anyone have an idea how to trigger such a fault with simple means?
I think I will write a test case which tests the few cases we really have, and check that __ipipe_handle_exception works as expected. A small rtdm driver with some specialized ioctls should do.
Quoted 3 lines
Cheers, Wolfgang
Signature · 4 lines
Gilles Chanteperdrix, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com