Jan Kiszka <[email protected]> writes:
> On 10.11.21 12:14, Philippe Gerum wrote: >> >> Jan Kiszka <[email protected]> writes: >> >>> On 10.11.21 09:58, Philippe Gerum wrote: >>>> >>>> Jan Kiszka <[email protected]> writes: >>>> >>>>> On 10.11.21 08:38, Philippe Gerum wrote: >>>>>> >>>>>> Jan Kiszka <[email protected]> writes: >>>>>> >>>>>>> On 09.11.21 14:35, Philippe Gerum wrote: >>>>>>>> >>>>>>>> Jan Kiszka <[email protected]> writes: >>>>>>>> >>>>>>>>> On 09.11.21 11:23, Philippe Gerum wrote: >>>>>>>>>> >>>>>>>>>> Jan Kiszka <[email protected]> writes: >>>>>>>>>> >>>>>>>>>>> On 08.11.21 18:57, Philippe Gerum wrote: >>>>>>>>>>>> >>>>>>>>>>>> Jan Kiszka <[email protected]> writes: >>>>>>>>>>>> >>>>>>>>>>>>> Hi Philippe, >>>>>>>>>>>>> >>>>>>>>>>>>> this dovetail commit makes the pipeline go red, crashing the >>>>>>>>>>>>> kernels >>>>>>>>>>>>> (e.g. [1][2]). I hope this is something we can quickly fix in >>>>>>>>>>>>> dovetail, >>>>>>>>>>>>> maybe via a config option? >>>>>>>>>>>>> >>>>>>>>>>>>> Jan >>>>>>>>>>>>> >>>>>>>>>>>>> [1] >>>>>>>>>>>>> https://source.denx.de/Xenomai/xenomai-images/-/jobs/348118#L966 >>>>>>>>>>>>> [2] >>>>>>>>>>>>> https://source.denx.de/Xenomai/xenomai-images/-/jobs/348121#L1429 >>>>>>>>>>>> >>>>>>>>>>>> Cobalt needs some update to cope with this now. I'll send a fix >>>>>>>>>>>> either >>>>>>>>>>>> way (dovetail or xenomai) tomorrow morning. >>>>>>>>>>> >>>>>>>>>>> This should be fixed in dovetail - API breakage. We can update >>>>>>>>>>> Xenomai >>>>>>>>>>> later, along with enabling this feature again. >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> We now have a change in the Dovetail tree which handles the fact that >>>>>>>>>> some Dovetail-based core might lag behind a bit API-wise regarding >>>>>>>>>> the >>>>>>>>>> new prctl-based call form. Since this simplifies the handling for any >>>>>>>>>> companion core in that particular case, this seems legitimate to add >>>>>>>>>> it. Tested on kvm-x86, -aarch64, and i.MX6-sabre with both Cobalt and >>>>>>>>>> EVL cores. Both test suites run properly, so far so good. >>>>>>>>> >>>>>>>>> I'm not against this change, but activating it is no Xenomai 3.2 >>>>>>>>> material as it will break the ABI. >>>>>>>> >>>>>>>> No, the ABI has never been affected by this series, the old call form >>>>>>>> Cobalt uses is still supported, the new prctl() call form is a mere >>>>>>>> addition, not a replacement. The problem did only affect the kernel >>>>>>>> interface between the pipeline core and Cobalt, which is strictly a >>>>>>>> kernel API issue, not revealed by my tests mainly with Xenomai4/EVL >>>>>>>> unfortunately. >>>>>>> >>>>>>> The whole purpose of having this addition is using it. And that does >>>>>>> make a lot of sense, as you described. So the plan is to activate AND >>>>>>> use this feature in Xenomai 3.3 - with the aforementioned impact on the >>>>>>> ABI. >>>>>>> >>>>>>> Xenomai 3.2 will continue to use the old syscall range extension scheme, >>>>>>> thus as no need and no desire to enable reporting of prctl calls to the >>>>>>> core. Therefore, Dovetail should continue to refrain from doing that for >>>>>>> Xenomai 3.2. The easiest way to achieve that is making the extension >>>>>>> build-time configurable. Other cores can then still enable it for their >>>>>>> *use*, and the fresh Xenomai 3.2 release will not break over the next >>>>>>> dovetail patch revision (which is urgently needed do to the apic-ack >>>>>>> fix). >>>>>>> >>>>>>> Makes sense? >>>>>>> >>>>>> >>>>>> It falls short of solving the real problem. >>>>> >>>>> Nor does your approach. If it were consequently ignoring stable >>>>> interfaces - there is no need for it in your in-tree model -, it would >>>>> have simply dropped the old syscall interface. Instead, it only provided >>>>> a half-stable solution for its users. >>>>> >>>> >>>> It looks ok so far on this end, thanks for caring anyway. >>>> >>>>>> >>>>>>> I really like to avoid avoid diverging developments again, but stability >>>>>>> trumps features and would enforce this if we cannot find a better >>>>>>> solution. >>>>>>> >>>>>> >>>>>> I agree, the best way is to decouple the code bases at this point, so >>>>>> that all development efforts can progress at their own pace, according >>>>>> to their own agenda and schedule, which are not compatible. Opening a >>>>>> new tree for maintaining a Xenomai3-specific pipeline will: >>>>>> >>>>>> - make things clearer to Xenomai3 users, providing them an unambiguous >>>>>> source for getting Dovetail support that works for it. >>>>>> >>>>>> - give you full control over this Dovetail tree, what goes there from >>>>>> the upstream code and what does not, when it does if it does. >>>>>> >>>>>> - keep all options open for the Dovetail upstream development. The >>>>>> whole point of starting Dovetail was to be able to evolve the dual >>>>>> kernel integration technique based on the evolving implementation of >>>>>> the mainline kernel, instead of being stuck for ages with legacy >>>>>> kernel interfaces. Kernel API changes are part of this process. >>>>>> >>>>>> We can cross-pollinate the trees, until Xenomai3 rebases on the next >>>>>> linux SLTS release which Dovetail upstream would support, and so on. >>>>>> >>>>> >>>>> As I said, this is very unfortunate, and I hope you will reconsider your >>>>> decisions. >>>>> >>>> >>>> I don't think this is a bad thing. This clarifies the intent, making >>>> things easier in the long run. >>>> >>>>> Meanwhile, I will stop 5.10.y-dovetail and instead start >>>>> linux-dovetail-stable.git with related branches. CI will be migrated as >>>>> well, stopping coverage of linux-dovetail head. >>>> >>>> Please call it linux-xenomai3-dovetail or anything you see fit except >>>> linux-dovetail-stable. Stability has a different meaning, and this name >>>> should be reserved for an upstream Dovetail tree. TIA, >>>> >>> >>> The result will not be Xenomai 3 specific. It will just use stable APIs >>> as additional requirement, perform the merge-based development process >>> for stable branches and provide regular CI-qualified releases again. >>> Just like under I-pipe. >>> >> >> Just find another name than linux-dovetail-stable, the rest is fine. >> > > dovetail-standlone created - but disabled again for now. > > https://source.denx.de/Xenomai/linux-dovetail/-/commit/8e3e74e0ce26c0ed146a65525c2c45465717ac58 > widely mitigates the issue for 3.2. You can still trigger the BUG via > invalid prctl calls, but that can be ignored and will be resolved in 3.2.1. > The idea is to allow the oob syscall handler to propagate unhandled (prctl) requests to the lower stage. This way, both the legacy and the new prctl-based call forms can co-exist without having to rebuild the application system, the kernel does not assume more than it should about the complete format of the requests the real-time core might expect, and makes sure to set EINVAL in errno as expected when a bad prctl option is given (instead of ENOSYS as currently). IOW, any invalid real-time prctl request detected by the oob handler would be passed to the in-band prctl handler for inspection and diagnosis, which would make sense. For this to happen, the prctl option tag should bear the __OOB_SYSCALL_BIT, the request should be issued from the oob stage and should also be out-of-range, all of which would already denote either really bad luck or insistence on doing stupid things. As a result, the bug assertion at [1] would become not only pointless but wrong, and should be removed. The possible combinations between stages and syscall forms can be tricky. I'll have a second look and more testing before merging upstream. [1] https://source.denx.de/Xenomai/xenomai/-/blob/43222fe53bbb81e76101e1a4265930f35a309f4d/kernel/cobalt/dovetail/syscall.c#L25 i.e: --- a/kernel/dovetail.c +++ b/kernel/dovetail.c @@ -65,8 +65,9 @@ void dovetail_stop_altsched(void) } EXPORT_SYMBOL_GPL(dovetail_stop_altsched); -void __weak handle_oob_syscall(struct pt_regs *regs) +int __weak handle_oob_syscall(struct pt_regs *regs) { + return 0; } int __weak handle_pipelined_syscall(struct irq_stage *stage, @@ -217,15 +218,17 @@ int pipeline_syscall(unsigned int nr, struct pt_regs *regs) */ if ((local_flags & _TLF_OOB) && maybe_oob_syscall(nr, regs)) { - handle_oob_syscall(regs); + ret = handle_oob_syscall(regs); local_flags = READ_ONCE(ti_local_flags(ti)); - if (local_flags & _TLF_OOB) { - if (test_ti_thread_flag(ti, TIF_MAYDAY)) - dovetail_call_mayday(regs); - return 1; /* don't pass down, no tail work. */ - } else { - WARN_ON_ONCE(dovetail_debug() && irqs_disabled()); - return -1; /* don't pass down, do tail work. */ + if (likely(ret)) { + if (local_flags & _TLF_OOB) { + if (test_ti_thread_flag(ti, TIF_MAYDAY)) + dovetail_call_mayday(regs); + return 1; /* don't pass down, no tail work. */ + } else { + WARN_ON_ONCE(dovetail_debug() && irqs_disabled()); + return -1; /* don't pass down, do tail work. */ + } } } diff --git a/kernel/cobalt/dovetail/syscall.c b/kernel/cobalt/dovetail/syscall.c index cec6c0244..440c069d5 100644 --- a/kernel/cobalt/dovetail/syscall.c +++ b/kernel/cobalt/dovetail/syscall.c @@ -19,8 +19,7 @@ int handle_pipelined_syscall(struct irq_stage *stage, struct pt_regs *regs) return handle_head_syscall(stage == &inband_stage, regs); } -void handle_oob_syscall(struct pt_regs *regs) +int handle_oob_syscall(struct pt_regs *regs) { - int ret = handle_head_syscall(false, regs); - XENO_BUG_ON(COBALT, ret == KEVENT_PROPAGATE); + return handle_head_syscall(false, regs); } -- Philippe.
