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

On 2026-08-28 09:01, Jan Beulich wrote:
> start_secondary(), do_double_fault(), play_dead(), and tboot_s3_error()
> never return, so would better be annotated anyway. The 
> do_double_fault()
> change needs accompanying by adjustments to entry_from_{pv,xen}(), as
> Eclair then deems the "return" there as unreachable.
> 
> context_switch() and continue_running() are odd: We can't
> (unconditionally) add noreturn to their declarations, as Arm's variants 
> do
> return. Put the attribute on x86'es definitions instead (the use of
> unreachable() in reset_stack_and_call_ind() allows the compiler to 
> figure
> that out itself, but Eclair wants the annotation in addition).
> 
> Signed-off-by: Jan Beulich <[email protected]>

Reviewed-by: Nicola Vetrini <[email protected]>

> ---
> entry_from_pv() wants the annotation only when PV=n, yet once added gcc
> then warns about "return" being used in a "noreturn" function. Is there
> any other approach to address this besides adding #ifdef inside the
> function (i.e. replacing the !IS_ENABLED(CONFIG_PV) check that's 
> there)?
> 

Besides GCC's warning, this would violate MISRA C's Rule 17.9 ("A 
function declared with a _Noreturn function specifier shall not return 
to its caller")
which is not (yet) adopted by Xen, as it comes with MISRA C:2012 
Amendment 3, whereas as you know Xen is based on MISRA C:2012 Amendment 
2 rules.
Besides this, perhaps an alternative could be something like this 
(untested):

#define __noreturn_0
#define __noreturn_1 __attribute__((noreturn))

#define __noreturn_select(x) __noreturn_select_(x)
#define __noreturn_select_(x) __noreturn_ ## x

#define noreturn(cond) __noreturn_select(cond)

assuming use sites such as noreturn(IS_ENABLED(CONFIG_FOO))

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -2163,7 +2163,7 @@ static void __context_switch(void)
>      per_cpu(curr_vcpu, cpu) = n;
>  }
> 
> -void context_switch(struct vcpu *prev, struct vcpu *next)
> +void noreturn context_switch(struct vcpu *prev, struct vcpu *next)
>  {
>      unsigned int cpu = smp_processor_id();
>      struct cpu_info *info = get_cpu_info();
> @@ -2240,7 +2240,7 @@ void context_switch(struct vcpu *prev, s
>      reset_stack_and_call_ind(nextd->arch.ctxt_switch->tail);
>  }
> 
> -void continue_running(struct vcpu *same)
> +void noreturn continue_running(struct vcpu *same)
>  {
>      reset_stack_and_call_ind(same->domain->arch.ctxt_switch->tail);
>  }
> --- a/xen/arch/x86/include/asm/cpuidle.h
> +++ b/xen/arch/x86/include/asm/cpuidle.h
> @@ -26,7 +26,7 @@ static inline int mwait_idle_init(struct
>  int cpuidle_init_cpu(unsigned int cpu);
>  void cf_check default_dead_idle(void);
>  void cf_check acpi_dead_idle(void);
> -void play_dead(void);
> +void noreturn play_dead(void);
>  void trace_exit_reason(u32 *irq_traced);
>  void update_idle_stats(struct acpi_processor_power *power,
>                         struct acpi_processor_cx *cx,
> --- a/xen/arch/x86/include/asm/tboot.h
> +++ b/xen/arch/x86/include/asm/tboot.h
> @@ -126,7 +126,7 @@ int tboot_in_measured_env(void);
>  int tboot_protect_mem_regions(void);
>  int cf_check tboot_parse_dmar_table(acpi_table_handler dmar_handler);
>  int tboot_s3_resume(void);
> -void tboot_s3_error(int error);
> +void noreturn tboot_s3_error(int error);
>  int tboot_wake_ap(int apicid, unsigned long sipi_vec);
>  #else
>  static inline void tboot_probe(void) {}
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -326,7 +326,7 @@ static void set_cpu_sibling_map(unsigned
>      }
>  }
> 
> -void asmlinkage start_secondary(void)
> +void asmlinkage noreturn start_secondary(void)
>  {
>      struct cpu_info *info = get_cpu_info();
>      unsigned int cpu = smp_processor_id();
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1080,7 +1080,7 @@ const char *vector_name(unsigned int vec
>      return (vec < ARRAY_SIZE(names) && names[vec][0]) ? names[vec] : 
> "???";
>  }
> 
> -void asmlinkage do_double_fault(struct cpu_user_regs *regs)
> +void asmlinkage noreturn do_double_fault(struct cpu_user_regs *regs)
>  {
>      unsigned int cpu;
>      struct extra_state state;
> @@ -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;

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253