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

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

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