Re: [PATCH v2 3/3] xen/x86: add missing noreturn attributes

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, Jun 18, 2025 at 06:16:30PM +0200, Nicola Vetrini wrote:
> On 2025-06-18 17:18, Roger Pau Monné wrote:
> > On Fri, Jun 06, 2025 at 02:27:09PM -0700, victorm.lira@amd.com wrote:
> > > From: Nicola Vetrini <nicola.vetrini@bugseng.com>
> > > 
> > > The marked functions never return to their caller, but lack the
> > > `noreturn' attribute.
> > > 
> > > Functions that never return should be declared with a `noreturn'
> > > attribute.
> > > 
> > > The lack of `noreturn' causes a violation of MISRA C Rule 17.11 (not
> > > currently accepted in Xen), and also Rule 2.1: "A project shall not
> > > contain unreachable code". Depending on the compiler used and the
> > > compiler optimization used, the lack of `noreturn' might lead to the
> > > presence of unreachable code.
> > > 
> > > No functional change.
> > > 
> > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> > > Signed-off-by: Victor Lira <victorm.lira@amd.com>
> > 
> > Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> > 
> > One question below.
> > 
> > > ---
> > > Changes in v2:
> > > - improved commit message
> > > ---
> > > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > > Cc: Anthony PERARD <anthony.perard@vates.tech>
> > > Cc: Michal Orzel <michal.orzel@amd.com>
> > > Cc: Jan Beulich <jbeulich@suse.com>
> > > Cc: Julien Grall <julien@xen.org>
> > > Cc: Roger Pau Monné <roger.pau@citrix.com>
> > > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > > Cc: Nicola Vetrini <nicola.vetrini@bugseng.com>
> > > Cc: Federico Serafini <federico.serafini@bugseng.com>
> > > Cc: Bertrand Marquis <bertrand.marquis@arm.com>
> > > ---
> > >  xen/arch/x86/cpu/mcheck/mce.c | 3 ++-
> > >  xen/arch/x86/efi/efi-boot.h   | 2 +-
> > >  xen/arch/x86/smp.c            | 2 +-
> > >  xen/arch/x86/traps.c          | 2 +-
> > >  xen/arch/x86/x86_64/traps.c   | 2 +-
> > >  5 files changed, 6 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/xen/arch/x86/cpu/mcheck/mce.c
> > > b/xen/arch/x86/cpu/mcheck/mce.c
> > > index 1c348e557d..79214ce56b 100644
> > > --- a/xen/arch/x86/cpu/mcheck/mce.c
> > > +++ b/xen/arch/x86/cpu/mcheck/mce.c
> > > @@ -79,7 +79,8 @@ static int __init cf_check mce_set_verbosity(const
> > > char *str)
> > >  custom_param("mce_verbosity", mce_set_verbosity);
> > > 
> > >  /* Handle unconfigured int18 (should never happen) */
> > > -static void cf_check unexpected_machine_check(const struct
> > > cpu_user_regs *regs)
> > > +static void noreturn cf_check
> > > +unexpected_machine_check(const struct cpu_user_regs *regs)
> > >  {
> > >      console_force_unlock();
> > >      printk("Unexpected Machine Check Exception\n");
> > > diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> > > index 0ecf4ca53f..0194720003 100644
> > > --- a/xen/arch/x86/efi/efi-boot.h
> > > +++ b/xen/arch/x86/efi/efi-boot.h
> > > @@ -769,7 +769,7 @@ static void __init efi_arch_blexit(void)
> > >          efi_bs->FreePages(ucode.addr, PFN_UP(ucode.size));
> > >  }
> > > 
> > > -static void __init efi_arch_halt(void)
> > > +static void noreturn __init efi_arch_halt(void)
> > >  {
> > >      local_irq_disable();
> > >      for ( ; ; )
> > > diff --git a/xen/arch/x86/smp.c b/xen/arch/x86/smp.c
> > > index 516dab5528..7936294f5f 100644
> > > --- a/xen/arch/x86/smp.c
> > > +++ b/xen/arch/x86/smp.c
> > > @@ -343,7 +343,7 @@ void __stop_this_cpu(void)
> > >      cpumask_clear_cpu(smp_processor_id(), &cpu_online_map);
> > >  }
> > > 
> > > -static void cf_check stop_this_cpu(void *dummy)
> > > +static void noreturn cf_check stop_this_cpu(void *dummy)
> > >  {
> > >      const bool *stop_aps = dummy;
> > > 
> > > diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> > > index 092c7e4197..34dc077cad 100644
> > > --- a/xen/arch/x86/traps.c
> > > +++ b/xen/arch/x86/traps.c
> > > @@ -805,7 +805,7 @@ void fatal_trap(const struct cpu_user_regs
> > > *regs, bool show_remote)
> > >            (regs->eflags & X86_EFLAGS_IF) ? "" : " IN INTERRUPT
> > > CONTEXT");
> > >  }
> > > 
> > > -void asmlinkage do_unhandled_trap(struct cpu_user_regs *regs)
> > > +void asmlinkage noreturn do_unhandled_trap(struct cpu_user_regs
> > > *regs)
> > >  {
> > >      fatal_trap(regs, false);
> > >  }
> > > diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
> > > index c77f304bb0..8460a4a1ae 100644
> > > --- a/xen/arch/x86/x86_64/traps.c
> > > +++ b/xen/arch/x86/x86_64/traps.c
> > > @@ -293,7 +293,7 @@ void show_page_walk(unsigned long addr)
> > >             l1_table_offset(addr), l1e_get_intpte(l1e), pfn);
> > >  }
> > > 
> > > -void asmlinkage do_double_fault(struct cpu_user_regs *regs)
> > > +void asmlinkage noreturn do_double_fault(struct cpu_user_regs *regs)
> > 
> > Does noreturn matter for functions called from assembly (asmlinkage
> > ones)?  In that case the hint is not useful for code generation, since
> > it's hand written assembly already?  (it's arguably useful for the
> > developer writing the code)
> > 
> > Might be worth mentioning in the commit message if the above is
> > accurate.  For example by adding to the commit message: "noreturn is
> > not relevant for functions called from assembly, but can be used as a
> > hint for the developers writing the code".
> > 
> 
> Yes, it is relevant because the rule considers only the single function, not
> the context where it is called (that is orders of magnitude more difficult
> to check automatically). For my part, I'm ok with your suggestion.

Thanks, so if my understanding is correct the usage of the noreturn
attribute together with asmlinkage is only for the benefit of the
analysis tools, as from a code generation perspective it makes no
difference.

Roger.