[PATCH v2 1/3] xen/keyhandler: add missing noreturn attribute

[PATCH v2 1/3] xen/keyhandler: add missing noreturn attribute

[victorm.lira]

From: Nicola Vetrini <nicola.vetrini@bugseng.com>

Function `reboot_machine' does not return, but lacks 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>
---
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/common/keyhandler.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
index 0bb842ec00..b0a2051408 100644
--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -251,7 +251,7 @@ static void cf_check dump_hwdom_registers(unsigned char key)
     }
 }

-static void cf_check reboot_machine(unsigned char key, bool unused)
+static void noreturn cf_check reboot_machine(unsigned char key, bool unused)
 {
     printk("'%c' pressed -> rebooting machine\n", key);
     machine_restart(0);
--
2.25.1

[PATCH v2 2/3] xen/arm: add missing noreturn attributes

[victorm.lira]

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>
---
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/arm/efi/efi-boot.h            | 2 +-
 xen/arch/arm/include/asm/arm64/traps.h | 2 +-
 xen/arch/arm/include/asm/processor.h   | 2 +-
 xen/arch/arm/setup.c                   | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index d2a09ad3a1..ee80560e13 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -934,7 +934,7 @@ static void __init efi_arch_blexit(void)
         efi_bs->FreePool(memmap);
 }

-static void __init efi_arch_halt(void)
+static void noreturn __init efi_arch_halt(void)
 {
     stop_cpu();
 }
diff --git a/xen/arch/arm/include/asm/arm64/traps.h b/xen/arch/arm/include/asm/arm64/traps.h
index 3be2fa69ee..b7435c6e73 100644
--- a/xen/arch/arm/include/asm/arm64/traps.h
+++ b/xen/arch/arm/include/asm/arm64/traps.h
@@ -6,7 +6,7 @@ void inject_undef64_exception(struct cpu_user_regs *regs);
 void do_sysreg(struct cpu_user_regs *regs,
                const union hsr hsr);

-void do_bad_mode(struct cpu_user_regs *regs, int reason);
+void noreturn do_bad_mode(struct cpu_user_regs *regs, int reason);

 #endif /* __ASM_ARM64_TRAPS__ */
 /*
diff --git a/xen/arch/arm/include/asm/processor.h b/xen/arch/arm/include/asm/processor.h
index 9cbc4f9110..92c8bc1a31 100644
--- a/xen/arch/arm/include/asm/processor.h
+++ b/xen/arch/arm/include/asm/processor.h
@@ -571,7 +571,7 @@ extern register_t __cpu_logical_map[];
 #endif

 #ifndef __ASSEMBLY__
-void panic_PAR(uint64_t par);
+void noreturn panic_PAR(uint64_t par);

 /* Debugging functions are declared with external linkage to aid development. */
 void show_registers(const struct cpu_user_regs *regs);
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 734e23da44..ed72317af3 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -63,7 +63,7 @@ bool __read_mostly acpi_disabled;

 domid_t __read_mostly max_init_domid;

-static __used void init_done(void)
+static __used void noreturn init_done(void)
 {
     int rc;

--
2.25.1

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

[victorm.lira]

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>
---
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)
 {
     unsigned int cpu;
     struct extra_state state;
--
2.25.1

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

[Stefano Stabellini]

On Fri, 6 Jun 2025, 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>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

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

[Roger Pau Monné]

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".

Thanks, Roger.

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

[Nicola Vetrini]

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, Roger.

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

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

[Roger Pau Monné]

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.

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

[Jan Beulich]

On 18.06.2025 18:16, 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:
>>> --- 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.

Right. In fact for non-static functions the attribute normally would go
on the declaration. The need to have it on the definition in the two
cases above is an aspect that may also want to go into the amended
description.

Jan

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

[Stefano Stabellini]

On Fri, 6 Jun 2025, 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>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Re: [PATCH v2 1/3] xen/keyhandler: add missing noreturn attribute

[Jan Beulich]

On 06.06.2025 23:27, victorm.lira@amd.com wrote:
> From: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> Function `reboot_machine' does not return, but lacks 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.

I continue to be unhappy about this: I still fail to see how this
"might" could materialize in the specific case here. My earlier
request for commit message improvement was specifically about making
the description match the particular case, not to add further general
wording. That said, if others feel like ack-ing this in this form, so
be it.

Jan

Re: [PATCH v2 1/3] xen/keyhandler: add missing noreturn attribute

[Stefano Stabellini]

On Fri, 6 Jun 2025, victorm.lira@amd.com wrote:
> From: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> Function `reboot_machine' does not return, but lacks 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>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>