Re: [PATCH 04/12] x86: add noreturn in a few more places

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2026-09-10 08:43, Jan Beulich wrote:
> On 09.09.2026 21:07, Nicola Vetrini wrote:
> > On 2026-09-01 08:26, Jan Beulich wrote:
> > > On 31.08.2026 21:13, Andrew Cooper wrote:
> > > > On 28/08/2026 8:01 am, Jan Beulich wrote:
> > > > > --- a/xen/arch/x86/traps.c
> > > > > +++ b/xen/arch/x86/traps.c
> > > > > @@ -2304,7 +2304,7 @@ void asmlinkage entry_from_pv(struct cpu
> > > > >      case X86_ET_HW_EXC:
> > > > >          switch ( vec )
> > > > >          {
> > > > > -        case X86_EXC_DF: return do_double_fault(regs);
> > > > > +        case X86_EXC_DF: do_double_fault(regs); /* noreturn */
> > > > >          case X86_EXC_MC: return do_machine_check(regs);
> > > > >          }
> > > > >          break;
> > > > > @@ -2615,7 +2615,7 @@ void asmlinkage entry_from_xen(struct cp
> > > > >      case X86_ET_HW_EXC:
> > > > >          switch ( regs->fred_ss.vector )
> > > > >          {
> > > > > -        case X86_EXC_DF: return do_double_fault(regs);
> > > > > +        case X86_EXC_DF: do_double_fault(regs); /* noreturn */
> > > > >          case X86_EXC_MC: return do_machine_check(regs);
> > > > >          }
> > > > >          break;
> > > > For starters you're missing a break, and the only reason this isn't 
> > > > a
> > > > compile error is the trailing comment.
> > > "break" there would again be unreachable, though.
> > >
> > > >   Second, it's a tailcall anyway. 
> > > > There really is nothing unreachable anywhere in this construct.
> > > Just that the concept of "tailcall" is an optimization, not something
> > > inherent to the language.
> > >
> > > > But by far the most important, it the singular noreturn attribute on
> > > > do_double_fault() (elsewhere, and not visible when reading these two
> > > > functions) which is preventing #DF falling into #MC.   This 
> > > > introduces
> > > > fragility which did not exist previously.
> > > I realized that when making the patch, yet what do you do when the 
> > > rule
> > > is as it is? Hence why I added the comment, really.
> > >
> > > > do_double_fault() would conditionally return if we ever got around 
> > > > to
> > > > fixing espfix64.
> > > And hence would have to lose its "noreturn". At which point call 
> > > sites
> > > would need inspecting. (As said - yes, I do realize the fragility.)
> > >
> > > > So no - I'm going to insist that Eclair is taught to accept "return
> > > > some_noreturn_fn();" as intentional.  It is objectively less fragile
> > > > than the MISRA-preferred option.
> > > Nicola, thoughts?
> > If you find a suitable argument from the toolchain that the generated
> > code is correct even though you return from a function where you
> > promised not to return in its declaration, I suppose that's fine, but
> > that MISRA Rule I mentioned ("A function declared with a _Noreturn
> > function specifier shall not return to its caller"), which is not 
> > (yet)
> > applied to Xen exists to defend from stumbling on UB 71 of C11: A
> > function declared with a _Noreturn function specifier shall not return
> > to its caller.
> >
> > So in general ECLAIR should not accept this by default. What you can 
> > do
> > is deviate these (hopefully few) cases if you have backing evidence of
> > the correct behavior.
> The disagreement between you suggesting a deviation and Andrew 
> demanding
> "that Eclair is taught to accept ..." will need resolving. The argument
> towards the code being overall less fragile in its original shape 
> cannot
> easily be put away. And Misra demanding code to be made more fragile
> than it needs to be cannot really be the goal either.
Well, I feel like I have explained my reasoning, but let me step back a 
bit and lay out the possible safe alternatives I see for this construct. 
By the way, perhaps it's a better idea to split off this change from the 
other additions of noreturn, which can probably go in as is.
Adding noreturn to do_double_fault() while keeping "return 
do_double_fault()" in the #DF path is likely subtly broken (i.e. the 
compiler can rightfully optimize assuming the function does not return) 
so that's not a feasible solution.
If noreturn is added to do_double_fault(), removing the return in 
entry_from_pv(), shouldn't that be guarded against falling trough via 
BUG() or equivalent constructs that do not vanish in release builds? My 
understanding, that may be incorrect, is that returning from 
do_double_fault() is currently not expected to happen (hence the panic() 
in it).
The third option is to ignore all this and not add noreturn to 
do_double_fault, adding a specific deviation. The deviation is not 
necessarily done via SAF, can also be something as shown below 
(untested):
-config=MC3A2.R2.1,reports+={deliberate, 
"any_area(decl(name(do_double_fault)))"}
and then in its documentation in rst you can summarize why it's not 
being touched.
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253