[PATCH 00/12] address most remaining Misra rule 2.1 violations

[PATCH 00/12] address most remaining Misra rule 2.1 violations

[Jan Beulich]

Let's try to get the unreachable code rule almost clean: With this series
in place, as per [1] x86_64-allcode has one violation left, x86_64-amd has
two; ARM64-* are clean. But see also "gnttab: unreachable code when
GNTTAB_MAX_VERSION < 2" [2].

The first two thirds of this series are hopefully largely uncontroversial,
whereas the last third may be.

01: x86/IO-APIC: address Misra 2.1 rule violations
02: x86/mm: pagetable_dying() is HVM+SHADOW_PAGING only
03: x86/shadow: eliminate unused forms of sh_map_and_validate_gl<N>e()
04: x86: add noreturn in a few more places
05: x86/crash: address Misra 2.1 rule violation
06: kexec: machine_reboot_kexec() doesn't return
07: altp2m: address Misra 2.1 rule violation
08: Arm/GIC: add noreturn in a few more places
09: Eclair: deviate BUILD_ERROR() wrt rule 2.1 and introduce variants
10: PCI/physdev: address Misra 2.1 rule violation
11: x86/HVM: address Misra 2.1 rule violations
12: x86/nSVM: address Misra 2.1 rule violation

Jan

[1] https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/2796557177
[2] https://lists.xen.org/archives/html/xen-devel/2026-07/msg01293.html

[PATCH 01/12] x86/IO-APIC: address Misra 2.1 rule violations

[Jan Beulich]

In both functions cases 0..3 are handled, and a 2-bit mask is applied to
the switch() expression. Therefore the default: cases are reported
unreachable by Eclair. Subsume the "case 2" blocks each into the
corresponding default ones.

While there also drop all the pointless figure braces inside the various
case blocks, inserting blank lines instead between them.

No functional change.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -804,66 +804,48 @@ static int __init MPBIOS_polarity(int id
     switch (mp_irqs[idx].mpc_irqflag & 3)
     {
     case 0: /* conforms, ie. bus-type dependent polarity */
-    {
         switch (mp_bus_id_to_type[bus])
         {
         case MP_BUS_ISA: /* ISA pin */
-        {
             polarity = default_ISA_polarity(idx);
             break;
-        }
+
         case MP_BUS_EISA: /* EISA pin */
-        {
             polarity = default_EISA_polarity(idx);
             break;
-        }
+
         case MP_BUS_PCI: /* PCI pin */
-        {
             polarity = default_PCI_polarity(idx);
             break;
-        }
+
         case MP_BUS_MCA: /* MCA pin */
-        {
             polarity = default_MCA_polarity(idx);
             break;
-        }
+
         case MP_BUS_NEC98: /* NEC 98 pin */
-        {
             polarity = default_NEC98_polarity(idx);
             break;
-        }
+
         default:
-        {
             printk(KERN_WARNING "broken BIOS!!\n");
             polarity = 1;
             break;
         }
-        }
         break;
-    }
+
     case 1: /* high active */
-    {
         polarity = 0;
         break;
-    }
-    case 2: /* reserved */
-    {
-        printk(KERN_WARNING "broken BIOS!!\n");
-        polarity = 1;
-        break;
-    }
+
     case 3: /* low active */
-    {
         polarity = 1;
         break;
-    }
-    default: /* invalid */
-    {
+
+    default: /* reserved */
         printk(KERN_WARNING "broken BIOS!!\n");
         polarity = 1;
         break;
     }
-    }
     return polarity;
 }
 
@@ -878,66 +860,48 @@ static int MPBIOS_trigger(int idx)
     switch ((mp_irqs[idx].mpc_irqflag>>2) & 3)
     {
     case 0: /* conforms, ie. bus-type dependent */
-    {
         switch (mp_bus_id_to_type[bus])
         {
         case MP_BUS_ISA: /* ISA pin */
-        {
             trigger = default_ISA_trigger(idx);
             break;
-        }
+
         case MP_BUS_EISA: /* EISA pin */
-        {
             trigger = default_EISA_trigger(idx);
             break;
-        }
+
         case MP_BUS_PCI: /* PCI pin */
-        {
             trigger = default_PCI_trigger(idx);
             break;
-        }
+
         case MP_BUS_MCA: /* MCA pin */
-        {
             trigger = default_MCA_trigger(idx);
             break;
-        }
+
         case MP_BUS_NEC98: /* NEC 98 pin */
-        {
             trigger = default_NEC98_trigger(idx);
             break;
-        }
+
         default:
-        {
             printk(KERN_WARNING "broken BIOS!!\n");
             trigger = 1;
             break;
         }
-        }
         break;
-    }
+
     case 1: /* edge */
-    {
         trigger = 0;
         break;
-    }
-    case 2: /* reserved */
-    {
-        printk(KERN_WARNING "broken BIOS!!\n");
-        trigger = 1;
-        break;
-    }
+
     case 3: /* level */
-    {
         trigger = 1;
         break;
-    }
-    default: /* invalid */
-    {
+
+    default: /* reserved */
         printk(KERN_WARNING "broken BIOS!!\n");
         trigger = 0;
         break;
     }
-    }
     return trigger;
 }
 

[PATCH 02/12] x86/mm: pagetable_dying() is HVM+SHADOW_PAGING only

[Jan Beulich]

The referenced commit didn't go far enough, leaving a Misra rule 2.1
(unreachable code) violation: The function lacks "noreturn" in this
configuration. Since with SHADOW_PAGING=n paging_mode_shadow() is compile-
time-constant false, the compiler can DCE the call site. Hence we can
avoid building the function itself altogether.

No functional change.

Fixes: 2fb2dee1ac62 ("x86/mm: pagetable_dying() is HVM-only")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -872,22 +872,18 @@ int paging_enable(struct domain *d, u32
 }
 #endif
 
-#ifdef CONFIG_HVM
+#if defined(CONFIG_HVM) && defined(CONFIG_SHADOW_PAGING)
 /* Called from the guest to indicate that a process is being torn down
  * and therefore its pagetables will soon be discarded */
 void pagetable_dying(paddr_t gpa)
 {
-#ifdef CONFIG_SHADOW_PAGING
     struct vcpu *curr = current;
 
     ASSERT(paging_mode_shadow(curr->domain));
 
     curr->arch.paging.mode->shadow.pagetable_dying(gpa);
-#else
-    BUG();
-#endif
 }
-#endif /* CONFIG_HVM */
+#endif /* HVM && SHADOW_PAGING */
 
 /* Print paging-assistance info to the console */
 void paging_dump_domain_info(struct domain *d)

[PATCH 03/12] x86/shadow: eliminate unused forms of sh_map_and_validate_gl<N>e()

[Jan Beulich]

The L2H, L3, and L4 forms only have GUEST_PAGING_LEVELS=4 call sites, i.e.
their 2- and 3-level forms are unreachable, violating Misra rule 2.1. The
L2H form additionally is unused (call site DCE-d) with PV32=n.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -1789,35 +1789,30 @@ sh_map_and_validate(struct vcpu *v, mfn_
     return result;
 }
 
+#if GUEST_PAGING_LEVELS >= 4
 
 int
 sh_map_and_validate_gl4e(struct vcpu *v, mfn_t gl4mfn,
                           void *new_gl4p, u32 size)
 {
-#if GUEST_PAGING_LEVELS >= 4
     return sh_map_and_validate(v, gl4mfn, new_gl4p, size,
                                 SH_type_l4_shadow,
                                 shadow_l4_index,
                                 validate_gl4e);
-#else // ! GUEST_PAGING_LEVELS >= 4
-    BUG(); /* Called in wrong paging mode! */
-#endif
 }
 
 int
 sh_map_and_validate_gl3e(struct vcpu *v, mfn_t gl3mfn,
                           void *new_gl3p, u32 size)
 {
-#if GUEST_PAGING_LEVELS >= 4
     return sh_map_and_validate(v, gl3mfn, new_gl3p, size,
                                 SH_type_l3_shadow,
                                 shadow_l3_index,
                                 validate_gl3e);
-#else // ! GUEST_PAGING_LEVELS >= 4
-    BUG(); /* Called in wrong paging mode! */
-#endif
 }
 
+#endif /* GUEST_PAGING_LEVELS >= 4 */
+
 int
 sh_map_and_validate_gl2e(struct vcpu *v, mfn_t gl2mfn,
                           void *new_gl2p, u32 size)
@@ -1828,19 +1823,17 @@ sh_map_and_validate_gl2e(struct vcpu *v,
                                 validate_gl2e);
 }
 
+#if defined(CONFIG_PV32) && GUEST_PAGING_LEVELS >= 4
 int
 sh_map_and_validate_gl2he(struct vcpu *v, mfn_t gl2mfn,
                            void *new_gl2p, u32 size)
 {
-#if GUEST_PAGING_LEVELS >= 4 && defined(CONFIG_PV32)
     return sh_map_and_validate(v, gl2mfn, new_gl2p, size,
                                 SH_type_l2h_shadow,
                                 shadow_l2_index,
                                 validate_gl2e);
-#else /* Non-PAE guests don't have different kinds of l2 table */
-    BUG(); /* Called in wrong paging mode! */
-#endif
 }
+#endif /* CONFIG_PV32 && GUEST_PAGING_LEVELS >= 4 */
 
 int
 sh_map_and_validate_gl1e(struct vcpu *v, mfn_t gl1mfn,

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

[Jan Beulich]

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 <jbeulich@suse.com>
---
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)?

--- 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;

[PATCH 05/12] x86/crash: address Misra 2.1 rule violation

[Jan Beulich]

The use of unreachable(), when unreachability is visible to Eclair (and
compilers), is deemed a violation. Drop the redundant statement.

No functional change.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/crash.c
+++ b/xen/arch/x86/crash.c
@@ -118,8 +118,6 @@ static int cf_check do_nmi_crash(
 
     for ( ; ; )
         halt();
-
-    unreachable();
 }
 
 static void nmi_shootdown_cpus(void)

[PATCH 06/12] kexec: machine_reboot_kexec() doesn't return

[Jan Beulich]

Mark it as such, and then remove the code following at its sole call site,
for Eclair flagging that as a Misra rule 2.1 (unreachable code) violation.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -401,7 +401,7 @@ void kexec_crash(enum crash_reason reaso
     BUG();
 }
 
-static long cf_check kexec_reboot(void *_image)
+static long noreturn cf_check kexec_reboot(void *_image)
 {
     struct kexec_image *image = _image;
 
@@ -409,9 +409,6 @@ static long cf_check kexec_reboot(void *
 
     kexec_common_shutdown();
     machine_reboot_kexec(image);
-
-    BUG();
-    return 0;
 }
 
 static void cf_check do_crashdump_trigger(unsigned char key)
--- a/xen/include/xen/kexec.h
+++ b/xen/include/xen/kexec.h
@@ -48,7 +48,7 @@ int machine_kexec_add_page(struct kexec_
 int machine_kexec_load(struct kexec_image *image);
 void machine_kexec_unload(struct kexec_image *image);
 void machine_kexec_reserved(xen_kexec_reserve_t *reservation);
-void machine_reboot_kexec(struct kexec_image *image);
+void noreturn machine_reboot_kexec(struct kexec_image *image);
 void machine_kexec(struct kexec_image *image);
 void kexec_crash(enum crash_reason reason);
 void kexec_crash_save_cpu(void);

[PATCH 07/12] altp2m: address Misra 2.1 rule violation

[Jan Beulich]

The stub altp2m_vcpu_idx() is recognized as "noreturn" function lacking
respective annotation (or having a return statement), which hence is deemed
unreachable code by Misra / Eclair. All call sites are guarded by
altp2m_active() checks, hence an inline function isn't needed. A
declaration will suffice, with call sites then getting DCE-d.

No functional change.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/asm-generic/altp2m.h
+++ b/xen/include/asm-generic/altp2m.h
@@ -14,13 +14,8 @@ static inline bool altp2m_active(const s
     return false;
 }
 
-/* Alternate p2m VCPU */
-static inline unsigned int altp2m_vcpu_idx(const struct vcpu *v)
-{
-    /* Not implemented on GENERIC, should not be reached. */
-    BUG();
-    return 0;
-}
+/* Alternate p2m VCPU - placeholder on GENERIC */
+unsigned int altp2m_vcpu_idx(const struct vcpu *v);
 
 #endif /* __ASM_GENERIC_ALTP2M_H */
 

[PATCH 08/12] Arm/GIC: add noreturn in a few more places

[Jan Beulich]

LPI related functions having just BUG() in them are disliked by Misra /
Eclair, as long as they don't also have a noreturn attribute.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
From its description "Unreachability caused by calls to the following
functions or macros is deliberate and there is no risk of code being
unexpectedly left out." I would have expected the respective entry in
deviations.ecl to cover all of these cases, but clearly that isn't the
case.

Of course having noreturn on functions returning non-void is somewhat odd.

--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -1315,7 +1315,7 @@ static int __init gicv2_init(void)
     return 0;
 }
 
-static void gicv2_do_LPI(unsigned int lpi)
+static void noreturn gicv2_do_LPI(unsigned int lpi)
 {
     /* No LPIs in a GICv2 */
     BUG();
--- a/xen/arch/arm/include/asm/gic_v3_its.h
+++ b/xen/arch/arm/include/asm/gic_v3_its.h
@@ -229,7 +229,7 @@ static inline unsigned int vgic_v3_its_c
     return 0;
 }
 
-static inline void gicv3_do_LPI(unsigned int lpi)
+static inline void noreturn gicv3_do_LPI(unsigned int lpi)
 {
     /* We don't enable LPIs without an ITS. */
     BUG();
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -718,14 +718,15 @@ static void vgic_v2_domain_free(struct d
     /* Nothing to be cleanup for this driver */
 }
 
-static struct pending_irq *vgic_v2_lpi_to_pending(struct domain *d,
-                                                  unsigned int vlpi)
+static struct pending_irq *noreturn vgic_v2_lpi_to_pending(struct domain *d,
+                                                           unsigned int vlpi)
 {
     /* Dummy function, no LPIs on a VGICv2. */
     BUG();
 }
 
-static int vgic_v2_lpi_get_priority(struct domain *d, unsigned int vlpi)
+static int noreturn vgic_v2_lpi_get_priority(struct domain *d,
+                                             unsigned int vlpi)
 {
     /* Dummy function, no LPIs on a VGICv2. */
     BUG();

[PATCH 09/12] Eclair: deviate BUILD_ERROR() wrt rule 2.1 and introduce variants

[Jan Beulich]

BUILD_ERROR() is even stronger a guard than assertions in general, and
ASSERT_UNREACHABLE() (or BUG()) in particular. Deviate it just like those
to allow use for marking unreachable portions of code.

In some cases code being unreachable is dependent upon configuration.
Introduce two variants, as constructs like

    if ( IS_ENABLED(CONFIG_...) )
        BUILD_ERROR("...");

results in the if() still being reported as unreachable. Sadly these two
new macros introduce a new 20.12 violation each, which hence also needs
deviating.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -19,6 +19,7 @@ Constant expressions and unreachable bra
 
 -doc_begin="Unreachability inside an ASSERT_UNREACHABLE() and analogous macro calls is deliberate and safe."
 -config=MC3A2.R2.1,reports+={deliberate, "any_area(any_loc(any_exp(macro(name(ASSERT_UNREACHABLE||PARSE_ERR_RET||PARSE_ERR||FAIL_MSR||FAIL_CPUID)))))"}
+-config=MC3A2.R2.1,reports+={deliberate, "any_area(any_loc(any_exp(macro(^BUILD_ERROR(|_IF(|_NOT))$))))"}
 -doc_end
 
 -doc_begin="The asm-offset files are not linked deliberately, since they are used to generate definitions for asm modules."
@@ -667,6 +668,7 @@ deliberate."
 to the # or ## operators within the following macros are deliberate, to provide
 useful diagnostic messages to the user."
 -config=MC3A2.R20.12,macros+={deliberate, "name(ASSERT||BUILD_BUG_ON||BUILD_BUG_ON_ZERO||RUNTIME_CHECK)"}
+-config=MC3A2.R20.12,macros+={deliberate, "^BUILD_ERROR(|_IF(|_NOT))$"}
 -doc_end
 
 -doc_begin="The helper macro GENERATE_CASE may use a macro parameter for ordinary
--- a/xen/include/xen/macros.h
+++ b/xen/include/xen/macros.h
@@ -64,6 +64,21 @@
  */
 #define BUILD_ERROR(msg) asm ( ".error \"" msg "\"" )
 
+/*
+ * Like above, but conditional upon @cfg (not) being enabled.  @cfg must be
+ * suitable to pass to IS_ENABLED().
+ */
+#define BUILD_ERROR_IF(cfg)                               \
+    (IS_ENABLED(cfg)                                      \
+     ? ({ BUILD_ERROR( #cfg " unexpectedly enabled"); })  \
+     : (void)0)
+
+#define BUILD_ERROR_IF_NOT(cfg)                           \
+    (!IS_ENABLED(cfg)                                     \
+     ? ({ BUILD_ERROR( #cfg " unexpectedly disabled"); }) \
+     : (void)0)
+
+
 /* Hide a value from the optimiser. */
 #define HIDE(x)                                 \
     ({                                          \

[PATCH 10/12] PCI/physdev: address Misra 2.1 rule violation

[Jan Beulich]

Cases 0..3 are handled, and a 2-bit mask is applied to the switch()
expression. Therefore the default: case is reported unreachable by Eclair.
Insert BUILD_ERROR() to annotate this for Eclair.

No functional change.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/pci/physdev.c
+++ b/xen/drivers/pci/physdev.c
@@ -114,7 +114,7 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_
             break;
 
         default:
-            ret = -EINVAL;
+            BUILD_ERROR("PCI_DEVICE_RESET_* inconsistency");
             break;
         }
         write_unlock(&pdev->domain->pci_lock);

[PATCH 11/12] x86/HVM: address Misra 2.1 rule violations

[Jan Beulich]

In hvm_set_cr3() the "bad_cr3" label is reachable only with
SHADOW_PAGING=y; the code being there is therefore a Misra rule 2.1
(unreachable code) violation when SHADOW_PAGING=n.

Similarly code past the initial switch() in hvm_debug_op() is reachable
only when CONFIG_INTEL_VMX=y.

No functional change.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2455,6 +2455,7 @@ int hvm_set_cr3(unsigned long value, boo
     return X86EMUL_OKAY;
 
  bad_cr3:
+    BUILD_ERROR_IF_NOT(CONFIG_SHADOW_PAGING);
     gdprintk(XENLOG_ERR, "Invalid CR3\n");
     domain_crash(currd);
     return X86EMUL_UNHANDLEABLE;
@@ -5201,6 +5202,8 @@ int hvm_debug_op(struct vcpu *v, int32_t
             return -ENOSYS;
     }
 
+    BUILD_ERROR_IF_NOT(CONFIG_INTEL_VMX);
+
     vcpu_pause(v);
 
     switch ( op )

[PATCH 12/12] x86/nSVM: address Misra 2.1 rule violation

[Jan Beulich]

The 16-bit range of "port" is fully handled by the switch(). Therefore the
default: case is reported unreachable by Eclair. Insert BUILD_ERROR() to
annotate this for Eclair.

No functional change.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -845,7 +845,7 @@ nsvm_vmcb_guest_intercepts_ioio(paddr_t
         ++gfn;
         break;
     default:
-        BUG();
+        BUILD_ERROR("I/O port range not fully covered");
         break;
     }
 

Re: [PATCH 01/12] x86/IO-APIC: address Misra 2.1 rule violations

[Nicola Vetrini]

On 2026-08-28 08:59, Jan Beulich wrote:
> In both functions cases 0..3 are handled, and a 2-bit mask is applied 
> to
> the switch() expression. Therefore the default: cases are reported
> unreachable by Eclair. Subsume the "case 2" blocks each into the
> corresponding default ones.
> 
> While there also drop all the pointless figure braces inside the 
> various
> case blocks, inserting blank lines instead between them.
> 
> No functional change.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 

Reviewed-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -804,66 +804,48 @@ static int __init MPBIOS_polarity(int id
>      switch (mp_irqs[idx].mpc_irqflag & 3)
>      {
>      case 0: /* conforms, ie. bus-type dependent polarity */
> -    {
>          switch (mp_bus_id_to_type[bus])
>          {
>          case MP_BUS_ISA: /* ISA pin */
> -        {
>              polarity = default_ISA_polarity(idx);
>              break;
> -        }
> +
>          case MP_BUS_EISA: /* EISA pin */
> -        {
>              polarity = default_EISA_polarity(idx);
>              break;
> -        }
> +
>          case MP_BUS_PCI: /* PCI pin */
> -        {
>              polarity = default_PCI_polarity(idx);
>              break;
> -        }
> +
>          case MP_BUS_MCA: /* MCA pin */
> -        {
>              polarity = default_MCA_polarity(idx);
>              break;
> -        }
> +
>          case MP_BUS_NEC98: /* NEC 98 pin */
> -        {
>              polarity = default_NEC98_polarity(idx);
>              break;
> -        }
> +
>          default:
> -        {
>              printk(KERN_WARNING "broken BIOS!!\n");
>              polarity = 1;
>              break;
>          }
> -        }
>          break;
> -    }
> +
>      case 1: /* high active */
> -    {
>          polarity = 0;
>          break;
> -    }
> -    case 2: /* reserved */
> -    {
> -        printk(KERN_WARNING "broken BIOS!!\n");
> -        polarity = 1;
> -        break;
> -    }
> +
>      case 3: /* low active */
> -    {
>          polarity = 1;
>          break;
> -    }
> -    default: /* invalid */
> -    {
> +
> +    default: /* reserved */
>          printk(KERN_WARNING "broken BIOS!!\n");
>          polarity = 1;
>          break;
>      }
> -    }
>      return polarity;
>  }
> 
> @@ -878,66 +860,48 @@ static int MPBIOS_trigger(int idx)
>      switch ((mp_irqs[idx].mpc_irqflag>>2) & 3)
>      {
>      case 0: /* conforms, ie. bus-type dependent */
> -    {
>          switch (mp_bus_id_to_type[bus])
>          {
>          case MP_BUS_ISA: /* ISA pin */
> -        {
>              trigger = default_ISA_trigger(idx);
>              break;
> -        }
> +
>          case MP_BUS_EISA: /* EISA pin */
> -        {
>              trigger = default_EISA_trigger(idx);
>              break;
> -        }
> +
>          case MP_BUS_PCI: /* PCI pin */
> -        {
>              trigger = default_PCI_trigger(idx);
>              break;
> -        }
> +
>          case MP_BUS_MCA: /* MCA pin */
> -        {
>              trigger = default_MCA_trigger(idx);
>              break;
> -        }
> +
>          case MP_BUS_NEC98: /* NEC 98 pin */
> -        {
>              trigger = default_NEC98_trigger(idx);
>              break;
> -        }
> +
>          default:
> -        {
>              printk(KERN_WARNING "broken BIOS!!\n");
>              trigger = 1;
>              break;
>          }
> -        }
>          break;
> -    }
> +
>      case 1: /* edge */
> -    {
>          trigger = 0;
>          break;
> -    }
> -    case 2: /* reserved */
> -    {
> -        printk(KERN_WARNING "broken BIOS!!\n");
> -        trigger = 1;
> -        break;
> -    }
> +
>      case 3: /* level */
> -    {
>          trigger = 1;
>          break;
> -    }
> -    default: /* invalid */
> -    {
> +
> +    default: /* reserved */
>          printk(KERN_WARNING "broken BIOS!!\n");
>          trigger = 0;
>          break;
>      }
> -    }
>      return trigger;
>  }

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

Re: [PATCH 06/12] kexec: machine_reboot_kexec() doesn't return

[Nicola Vetrini]

On 2026-08-28 09:02, Jan Beulich wrote:
> Mark it as such, and then remove the code following at its sole call 
> site,
> for Eclair flagging that as a Misra rule 2.1 (unreachable code) 
> violation.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 

Reviewed-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

> --- a/xen/common/kexec.c
> +++ b/xen/common/kexec.c
> @@ -401,7 +401,7 @@ void kexec_crash(enum crash_reason reaso
>      BUG();
>  }
> 
> -static long cf_check kexec_reboot(void *_image)
> +static long noreturn cf_check kexec_reboot(void *_image)
>  {
>      struct kexec_image *image = _image;
> 
> @@ -409,9 +409,6 @@ static long cf_check kexec_reboot(void *
> 
>      kexec_common_shutdown();
>      machine_reboot_kexec(image);
> -
> -    BUG();
> -    return 0;
>  }
> 
>  static void cf_check do_crashdump_trigger(unsigned char key)
> --- a/xen/include/xen/kexec.h
> +++ b/xen/include/xen/kexec.h
> @@ -48,7 +48,7 @@ int machine_kexec_add_page(struct kexec_
>  int machine_kexec_load(struct kexec_image *image);
>  void machine_kexec_unload(struct kexec_image *image);
>  void machine_kexec_reserved(xen_kexec_reserve_t *reservation);
> -void machine_reboot_kexec(struct kexec_image *image);
> +void noreturn machine_reboot_kexec(struct kexec_image *image);
>  void machine_kexec(struct kexec_image *image);
>  void kexec_crash(enum crash_reason reason);
>  void kexec_crash_save_cpu(void);

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

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

[Nicola Vetrini]

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 <jbeulich@suse.com>

Reviewed-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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

Re: [PATCH 09/12] Eclair: deviate BUILD_ERROR() wrt rule 2.1 and introduce variants

[Nicola Vetrini]

On 2026-08-28 09:04, Jan Beulich wrote:
> BUILD_ERROR() is even stronger a guard than assertions in general, and
> ASSERT_UNREACHABLE() (or BUG()) in particular. Deviate it just like 
> those
> to allow use for marking unreachable portions of code.
> 
> In some cases code being unreachable is dependent upon configuration.
> Introduce two variants, as constructs like
> 
>     if ( IS_ENABLED(CONFIG_...) )
>         BUILD_ERROR("...");
> 
> results in the if() still being reported as unreachable. Sadly these 
> two
> new macros introduce a new 20.12 violation each, which hence also needs
> deviating.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Presumably you did not fold at least one of the following patches where 
the construct is actually used into this one to separate concerns?

> 
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -19,6 +19,7 @@ Constant expressions and unreachable bra
> 
>  -doc_begin="Unreachability inside an ASSERT_UNREACHABLE() and 
> analogous macro calls is deliberate and safe."
>  -config=MC3A2.R2.1,reports+={deliberate, 
> "any_area(any_loc(any_exp(macro(name(ASSERT_UNREACHABLE||PARSE_ERR_RET||PARSE_ERR||FAIL_MSR||FAIL_CPUID)))))"}
> +-config=MC3A2.R2.1,reports+={deliberate, 
> "any_area(any_loc(any_exp(macro(^BUILD_ERROR(|_IF(|_NOT))$))))"}
>  -doc_end
> 
>  -doc_begin="The asm-offset files are not linked deliberately, since 
> they are used to generate definitions for asm modules."
> @@ -667,6 +668,7 @@ deliberate."
>  to the # or ## operators within the following macros are deliberate, 
> to provide
>  useful diagnostic messages to the user."
>  -config=MC3A2.R20.12,macros+={deliberate, 
> "name(ASSERT||BUILD_BUG_ON||BUILD_BUG_ON_ZERO||RUNTIME_CHECK)"}
> +-config=MC3A2.R20.12,macros+={deliberate, 
> "^BUILD_ERROR(|_IF(|_NOT))$"}
>  -doc_end
> 
>  -doc_begin="The helper macro GENERATE_CASE may use a macro parameter 
> for ordinary
> --- a/xen/include/xen/macros.h
> +++ b/xen/include/xen/macros.h
> @@ -64,6 +64,21 @@
>   */
>  #define BUILD_ERROR(msg) asm ( ".error \"" msg "\"" )
> 
> +/*
> + * Like above, but conditional upon @cfg (not) being enabled.  @cfg 
> must be
> + * suitable to pass to IS_ENABLED().
> + */
> +#define BUILD_ERROR_IF(cfg)                               \
> +    (IS_ENABLED(cfg)                                      \
> +     ? ({ BUILD_ERROR( #cfg " unexpectedly enabled"); })  \
> +     : (void)0)
> +
> +#define BUILD_ERROR_IF_NOT(cfg)                           \
> +    (!IS_ENABLED(cfg)                                     \
> +     ? ({ BUILD_ERROR( #cfg " unexpectedly disabled"); }) \
> +     : (void)0)
> +
> +
>  /* Hide a value from the optimiser. */
>  #define HIDE(x)                                 \
>      ({                                          \

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

Re: [PATCH 07/12] altp2m: address Misra 2.1 rule violation

[Stefano Stabellini]

On Fri, 28 Aug 2026, Jan Beulich wrote:
> The stub altp2m_vcpu_idx() is recognized as "noreturn" function lacking
> respective annotation (or having a return statement), which hence is deemed
> unreachable code by Misra / Eclair. All call sites are guarded by
> altp2m_active() checks, hence an inline function isn't needed. A
> declaration will suffice, with call sites then getting DCE-d.
> 
> No functional change.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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


> --- a/xen/include/asm-generic/altp2m.h
> +++ b/xen/include/asm-generic/altp2m.h
> @@ -14,13 +14,8 @@ static inline bool altp2m_active(const s
>      return false;
>  }
>  
> -/* Alternate p2m VCPU */
> -static inline unsigned int altp2m_vcpu_idx(const struct vcpu *v)
> -{
> -    /* Not implemented on GENERIC, should not be reached. */
> -    BUG();
> -    return 0;
> -}
> +/* Alternate p2m VCPU - placeholder on GENERIC */
> +unsigned int altp2m_vcpu_idx(const struct vcpu *v);
>  
>  #endif /* __ASM_GENERIC_ALTP2M_H */
>  
> 

Re: [PATCH 08/12] Arm/GIC: add noreturn in a few more places

[Stefano Stabellini]

On Fri, 28 Aug 2026, Jan Beulich wrote:
> LPI related functions having just BUG() in them are disliked by Misra /
> Eclair, as long as they don't also have a noreturn attribute.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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


> ---
> >From its description "Unreachability caused by calls to the following
> functions or macros is deliberate and there is no risk of code being
> unexpectedly left out." I would have expected the respective entry in
> deviations.ecl to cover all of these cases, but clearly that isn't the
> case.
> 
> Of course having noreturn on functions returning non-void is somewhat odd.
> 
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -1315,7 +1315,7 @@ static int __init gicv2_init(void)
>      return 0;
>  }
>  
> -static void gicv2_do_LPI(unsigned int lpi)
> +static void noreturn gicv2_do_LPI(unsigned int lpi)
>  {
>      /* No LPIs in a GICv2 */
>      BUG();
> --- a/xen/arch/arm/include/asm/gic_v3_its.h
> +++ b/xen/arch/arm/include/asm/gic_v3_its.h
> @@ -229,7 +229,7 @@ static inline unsigned int vgic_v3_its_c
>      return 0;
>  }
>  
> -static inline void gicv3_do_LPI(unsigned int lpi)
> +static inline void noreturn gicv3_do_LPI(unsigned int lpi)
>  {
>      /* We don't enable LPIs without an ITS. */
>      BUG();
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -718,14 +718,15 @@ static void vgic_v2_domain_free(struct d
>      /* Nothing to be cleanup for this driver */
>  }
>  
> -static struct pending_irq *vgic_v2_lpi_to_pending(struct domain *d,
> -                                                  unsigned int vlpi)
> +static struct pending_irq *noreturn vgic_v2_lpi_to_pending(struct domain *d,
> +                                                           unsigned int vlpi)
>  {
>      /* Dummy function, no LPIs on a VGICv2. */
>      BUG();
>  }
>  
> -static int vgic_v2_lpi_get_priority(struct domain *d, unsigned int vlpi)
> +static int noreturn vgic_v2_lpi_get_priority(struct domain *d,
> +                                             unsigned int vlpi)
>  {
>      /* Dummy function, no LPIs on a VGICv2. */
>      BUG();
> 

Re: [PATCH 10/12] PCI/physdev: address Misra 2.1 rule violation

[Stefano Stabellini]

On Fri, 28 Aug 2026, Jan Beulich wrote:
> Cases 0..3 are handled, and a 2-bit mask is applied to the switch()
> expression. Therefore the default: case is reported unreachable by Eclair.
> Insert BUILD_ERROR() to annotate this for Eclair.
> 
> No functional change.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

> --- a/xen/drivers/pci/physdev.c
> +++ b/xen/drivers/pci/physdev.c
> @@ -114,7 +114,7 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_
>              break;
>  
>          default:
> -            ret = -EINVAL;
> +            BUILD_ERROR("PCI_DEVICE_RESET_* inconsistency");
>              break;
>          }
>          write_unlock(&pdev->domain->pci_lock);
> 

Re: [PATCH 09/12] Eclair: deviate BUILD_ERROR() wrt rule 2.1 and introduce variants

[Stefano Stabellini]

On Fri, 28 Aug 2026, Jan Beulich wrote:
> BUILD_ERROR() is even stronger a guard than assertions in general, and
> ASSERT_UNREACHABLE() (or BUG()) in particular. Deviate it just like those
> to allow use for marking unreachable portions of code.
> 
> In some cases code being unreachable is dependent upon configuration.
> Introduce two variants, as constructs like
> 
>     if ( IS_ENABLED(CONFIG_...) )
>         BUILD_ERROR("...");
> 
> results in the if() still being reported as unreachable. Sadly these two
> new macros introduce a new 20.12 violation each, which hence also needs
> deviating.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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


> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -19,6 +19,7 @@ Constant expressions and unreachable bra
>  
>  -doc_begin="Unreachability inside an ASSERT_UNREACHABLE() and analogous macro calls is deliberate and safe."
>  -config=MC3A2.R2.1,reports+={deliberate, "any_area(any_loc(any_exp(macro(name(ASSERT_UNREACHABLE||PARSE_ERR_RET||PARSE_ERR||FAIL_MSR||FAIL_CPUID)))))"}
> +-config=MC3A2.R2.1,reports+={deliberate, "any_area(any_loc(any_exp(macro(^BUILD_ERROR(|_IF(|_NOT))$))))"}
>  -doc_end
>  
>  -doc_begin="The asm-offset files are not linked deliberately, since they are used to generate definitions for asm modules."
> @@ -667,6 +668,7 @@ deliberate."
>  to the # or ## operators within the following macros are deliberate, to provide
>  useful diagnostic messages to the user."
>  -config=MC3A2.R20.12,macros+={deliberate, "name(ASSERT||BUILD_BUG_ON||BUILD_BUG_ON_ZERO||RUNTIME_CHECK)"}
> +-config=MC3A2.R20.12,macros+={deliberate, "^BUILD_ERROR(|_IF(|_NOT))$"}
>  -doc_end
>  
>  -doc_begin="The helper macro GENERATE_CASE may use a macro parameter for ordinary
> --- a/xen/include/xen/macros.h
> +++ b/xen/include/xen/macros.h
> @@ -64,6 +64,21 @@
>   */
>  #define BUILD_ERROR(msg) asm ( ".error \"" msg "\"" )
>  
> +/*
> + * Like above, but conditional upon @cfg (not) being enabled.  @cfg must be
> + * suitable to pass to IS_ENABLED().
> + */
> +#define BUILD_ERROR_IF(cfg)                               \
> +    (IS_ENABLED(cfg)                                      \
> +     ? ({ BUILD_ERROR( #cfg " unexpectedly enabled"); })  \
> +     : (void)0)
> +
> +#define BUILD_ERROR_IF_NOT(cfg)                           \
> +    (!IS_ENABLED(cfg)                                     \
> +     ? ({ BUILD_ERROR( #cfg " unexpectedly disabled"); }) \
> +     : (void)0)
> +
> +
>  /* Hide a value from the optimiser. */
>  #define HIDE(x)                                 \
>      ({                                          \
> 

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

[Andrew Cooper]

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.  Second, it's a tailcall anyway. 
There really is nothing unreachable anywhere in this construct.

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.

do_double_fault() would conditionally return if we ever got around to
fixing espfix64.

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.

~Andrew

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

[Jan Beulich]

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?

Jan

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

[Jan Beulich]

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.  Second, it's a tailcall anyway. 
> There really is nothing unreachable anywhere in this construct.
> 
> 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.
> 
> do_double_fault() would conditionally return if we ever got around to
> fixing espfix64.
> 
> 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.

Oh, also: For a v2, how much of this change do you demand dropping /
splitting off? Just the two hunks above, or also the one adding noreturn
to do_double_fault()?

Jan

Re: [PATCH 09/12] Eclair: deviate BUILD_ERROR() wrt rule 2.1 and introduce variants

[Jan Beulich]

On 29.08.2026 16:04, Nicola Vetrini wrote:
> On 2026-08-28 09:04, Jan Beulich wrote:
>> BUILD_ERROR() is even stronger a guard than assertions in general, and
>> ASSERT_UNREACHABLE() (or BUG()) in particular. Deviate it just like 
>> those
>> to allow use for marking unreachable portions of code.
>>
>> In some cases code being unreachable is dependent upon configuration.
>> Introduce two variants, as constructs like
>>
>>     if ( IS_ENABLED(CONFIG_...) )
>>         BUILD_ERROR("...");
>>
>> results in the if() still being reported as unreachable. Sadly these 
>> two
>> new macros introduce a new 20.12 violation each, which hence also needs
>> deviating.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Thanks.

> Presumably you did not fold at least one of the following patches where 
> the construct is actually used into this one to separate concerns?

Partly for that, partly to keep subsequent patches possible to go in
independently, i.e. in any order.

Jan

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

[Jan Beulich]

On 29.08.2026 15:21, Nicola Vetrini wrote:
> 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 <jbeulich@suse.com>
> 
> Reviewed-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Thanks, yet Andrew's objection will need dealing with.

>> ---
>> 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))

But how is

void asmlinkage noreturn(!IS_ENABLED(CONFIG_PV))
entry_from_pv(struct cpu_user_regs *regs)

(besides of course not being correct to use this way) different from

void asmlinkage
#ifndef CONFIG_PV
noreturn
#endif
entry_from_pv(struct cpu_user_regs *regs)

? We'd still end up with a "noreturn" function having "return" statements.

Jan

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

[Nicola Vetrini]

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.


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

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

[Jan Beulich]

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.

Jan

Re: [PATCH 02/12] x86/mm: pagetable_dying() is HVM+SHADOW_PAGING only

[Roger Pau Monné]

On Fri, Aug 28, 2026 at 09:00:04AM +0200, Jan Beulich wrote:
> The referenced commit didn't go far enough, leaving a Misra rule 2.1
> (unreachable code) violation: The function lacks "noreturn" in this
> configuration. Since with SHADOW_PAGING=n paging_mode_shadow() is compile-
> time-constant false, the compiler can DCE the call site. Hence we can
> avoid building the function itself altogether.
> 
> No functional change.
> 
> Fixes: 2fb2dee1ac62 ("x86/mm: pagetable_dying() is HVM-only")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger@xenproject.org>

Thanks, Roger.

Re: [PATCH 01/12] x86/IO-APIC: address Misra 2.1 rule violations

[Roger Pau Monné]

On Fri, Aug 28, 2026 at 08:59:41AM +0200, Jan Beulich wrote:
> In both functions cases 0..3 are handled, and a 2-bit mask is applied to
> the switch() expression. Therefore the default: cases are reported
> unreachable by Eclair. Subsume the "case 2" blocks each into the
> corresponding default ones.
> 
> While there also drop all the pointless figure braces inside the various
> case blocks, inserting blank lines instead between them.
> 
> No functional change.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger@xenproject.org>

> 
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -804,66 +804,48 @@ static int __init MPBIOS_polarity(int id
>      switch (mp_irqs[idx].mpc_irqflag & 3)
>      {
>      case 0: /* conforms, ie. bus-type dependent polarity */
> -    {
>          switch (mp_bus_id_to_type[bus])
>          {
>          case MP_BUS_ISA: /* ISA pin */
> -        {
>              polarity = default_ISA_polarity(idx);
>              break;
> -        }
> +
>          case MP_BUS_EISA: /* EISA pin */
> -        {
>              polarity = default_EISA_polarity(idx);
>              break;
> -        }
> +
>          case MP_BUS_PCI: /* PCI pin */
> -        {
>              polarity = default_PCI_polarity(idx);
>              break;
> -        }
> +
>          case MP_BUS_MCA: /* MCA pin */
> -        {
>              polarity = default_MCA_polarity(idx);
>              break;
> -        }
> +
>          case MP_BUS_NEC98: /* NEC 98 pin */
> -        {
>              polarity = default_NEC98_polarity(idx);
>              break;
> -        }
> +
>          default:
> -        {
>              printk(KERN_WARNING "broken BIOS!!\n");
>              polarity = 1;
>              break;
>          }
> -        }
>          break;
> -    }
> +
>      case 1: /* high active */
> -    {
>          polarity = 0;
>          break;
> -    }
> -    case 2: /* reserved */
> -    {
> -        printk(KERN_WARNING "broken BIOS!!\n");
> -        polarity = 1;
> -        break;
> -    }
> +
>      case 3: /* low active */
> -    {
>          polarity = 1;
>          break;
> -    }
> -    default: /* invalid */
> -    {
> +
> +    default: /* reserved */
>          printk(KERN_WARNING "broken BIOS!!\n");

We should also see about improving those messages, because this is not
helpful at all.

Thanks, Roger.

Re: [PATCH 12/12] x86/nSVM: address Misra 2.1 rule violation

[Roger Pau Monné]

On Fri, Aug 28, 2026 at 09:06:20AM +0200, Jan Beulich wrote:
> The 16-bit range of "port" is fully handled by the switch(). Therefore the
> default: case is reported unreachable by Eclair. Insert BUILD_ERROR() to
> annotate this for Eclair.
> 
> No functional change.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger@xenproject.org>

That however is a bit weird IMO, as we have usually said we would
prefer not to use fixed width types in general, and hence one might
argue that port should be unsigned int. Then the BUILD_ERROR() might
trigger?

Thanks, Roger.

Re: [PATCH 03/12] x86/shadow: eliminate unused forms of sh_map_and_validate_gl<N>e()

[Roger Pau Monné]

On Fri, Aug 28, 2026 at 09:00:57AM +0200, Jan Beulich wrote:
> The L2H, L3, and L4 forms only have GUEST_PAGING_LEVELS=4 call sites, i.e.
> their 2- and 3-level forms are unreachable, violating Misra rule 2.1. The
> L2H form additionally is unused (call site DCE-d) with PV32=n.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger@xenproject.org>

Looks like a net win in the number of ifdefs.

Thanks, Roger.

Re: [PATCH 05/12] x86/crash: address Misra 2.1 rule violation

[Roger Pau Monné]

On Fri, Aug 28, 2026 at 09:02:18AM +0200, Jan Beulich wrote:
> The use of unreachable(), when unreachability is visible to Eclair (and
> compilers), is deemed a violation. Drop the redundant statement.

Urg, isn't that something that should be fixed in Eclair then?
Otherwise all the unreachable() calls in our codebase are likely to be
found by Eclair sooner or later, and will need to be removed.

Thanks, Roger.

Re: [PATCH 11/12] x86/HVM: address Misra 2.1 rule violations

[Roger Pau Monné]

On Fri, Aug 28, 2026 at 09:05:43AM +0200, Jan Beulich wrote:
> In hvm_set_cr3() the "bad_cr3" label is reachable only with
> SHADOW_PAGING=y; the code being there is therefore a Misra rule 2.1
> (unreachable code) violation when SHADOW_PAGING=n.
> 
> Similarly code past the initial switch() in hvm_debug_op() is reachable
> only when CONFIG_INTEL_VMX=y.
> 
> No functional change.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger@xenproject.org>

Thanks, Roger.

Re: [PATCH 05/12] x86/crash: address Misra 2.1 rule violation

[Jan Beulich]

On 10.09.2026 09:49, Roger Pau Monné wrote:
> On Fri, Aug 28, 2026 at 09:02:18AM +0200, Jan Beulich wrote:
>> The use of unreachable(), when unreachability is visible to Eclair (and
>> compilers), is deemed a violation. Drop the redundant statement.
> 
> Urg, isn't that something that should be fixed in Eclair then?
> Otherwise all the unreachable() calls in our codebase are likely to be
> found by Eclair sooner or later, and will need to be removed.

No, aiui most are covered by deviations. In particular ones in BUG() and
ASSERT_UNREACHABLE().

Jan

Re: [PATCH 12/12] x86/nSVM: address Misra 2.1 rule violation

[Jan Beulich]

On 10.09.2026 09:44, Roger Pau Monné wrote:
> On Fri, Aug 28, 2026 at 09:06:20AM +0200, Jan Beulich wrote:
>> The 16-bit range of "port" is fully handled by the switch(). Therefore the
>> default: case is reported unreachable by Eclair. Insert BUILD_ERROR() to
>> annotate this for Eclair.
>>
>> No functional change.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger@xenproject.org>

Thanks.

> That however is a bit weird IMO, as we have usually said we would
> prefer not to use fixed width types in general, and hence one might
> argue that port should be unsigned int. Then the BUILD_ERROR() might
> trigger?

Yes, such a type change would need accompanying by removal of that
BUILD_ERROR(). The default: case then also wouldn't be deemed
unreachable anymore (even though in practice it still would be).

Jan

Re: [PATCH 05/12] x86/crash: address Misra 2.1 rule violation

[Roger Pau Monné]

On Thu, Sep 10, 2026 at 10:38:34AM +0200, Jan Beulich wrote:
> On 10.09.2026 09:49, Roger Pau Monné wrote:
> > On Fri, Aug 28, 2026 at 09:02:18AM +0200, Jan Beulich wrote:
> >> The use of unreachable(), when unreachability is visible to Eclair (and
> >> compilers), is deemed a violation. Drop the redundant statement.
> > 
> > Urg, isn't that something that should be fixed in Eclair then?
> > Otherwise all the unreachable() calls in our codebase are likely to be
> > found by Eclair sooner or later, and will need to be removed.
> 
> No, aiui most are covered by deviations. In particular ones in BUG() and
> ASSERT_UNREACHABLE().

Shouldn't this be a deviation then also?  Maybe it would be helpful if
the commit message states why this is handled differently from other
unreachable() instances then.

Thanks, Roger.

Re: [PATCH 05/12] x86/crash: address Misra 2.1 rule violation

[Jan Beulich]

On 10.09.2026 11:30, Roger Pau Monné wrote:
> On Thu, Sep 10, 2026 at 10:38:34AM +0200, Jan Beulich wrote:
>> On 10.09.2026 09:49, Roger Pau Monné wrote:
>>> On Fri, Aug 28, 2026 at 09:02:18AM +0200, Jan Beulich wrote:
>>>> The use of unreachable(), when unreachability is visible to Eclair (and
>>>> compilers), is deemed a violation. Drop the redundant statement.
>>>
>>> Urg, isn't that something that should be fixed in Eclair then?
>>> Otherwise all the unreachable() calls in our codebase are likely to be
>>> found by Eclair sooner or later, and will need to be removed.
>>
>> No, aiui most are covered by deviations. In particular ones in BUG() and
>> ASSERT_UNREACHABLE().
> 
> Shouldn't this be a deviation then also?

Maybe, just that I had no good idea how to express such a deviation (preferably
without a SAF comment).

>  Maybe it would be helpful if
> the commit message states why this is handled differently from other
> unreachable() instances then.

I've added "..., , and the one here isn't covered by a deviation" to the first
sentence. Will that suffice?

Jan

Re: [PATCH 05/12] x86/crash: address Misra 2.1 rule violation

[Roger Pau Monné]

On Thu, Sep 10, 2026 at 11:52:10AM +0200, Jan Beulich wrote:
> On 10.09.2026 11:30, Roger Pau Monné wrote:
> > On Thu, Sep 10, 2026 at 10:38:34AM +0200, Jan Beulich wrote:
> >> On 10.09.2026 09:49, Roger Pau Monné wrote:
> >>> On Fri, Aug 28, 2026 at 09:02:18AM +0200, Jan Beulich wrote:
> >>>> The use of unreachable(), when unreachability is visible to Eclair (and
> >>>> compilers), is deemed a violation. Drop the redundant statement.
> >>>
> >>> Urg, isn't that something that should be fixed in Eclair then?
> >>> Otherwise all the unreachable() calls in our codebase are likely to be
> >>> found by Eclair sooner or later, and will need to be removed.
> >>
> >> No, aiui most are covered by deviations. In particular ones in BUG() and
> >> ASSERT_UNREACHABLE().
> > 
> > Shouldn't this be a deviation then also?
> 
> Maybe, just that I had no good idea how to express such a deviation (preferably
> without a SAF comment).
> 
> >  Maybe it would be helpful if
> > the commit message states why this is handled differently from other
> > unreachable() instances then.
> 
> I've added "..., , and the one here isn't covered by a deviation" to the first
> sentence. Will that suffice?

TBH, the handling of unreachable() feels inconsistent to me.  I don't
blame you for this, I know you are just trying to fix the remaining
issues.

I guess I will defer the change to someone more familiar with MISRA
and why some unreachable() usages are covered by deviations while
others aren't.

I think the point of adding something to the commit message is to
justify why this is removed vs a deviation being added.

Thanks, Roger.

Re: [PATCH 05/12] x86/crash: address Misra 2.1 rule violation

[Nicola Vetrini]

On 2026-09-10 14:29, Roger Pau Monné wrote:
> On Thu, Sep 10, 2026 at 11:52:10AM +0200, Jan Beulich wrote:
>> On 10.09.2026 11:30, Roger Pau Monné wrote:
>> > On Thu, Sep 10, 2026 at 10:38:34AM +0200, Jan Beulich wrote:
>> >> On 10.09.2026 09:49, Roger Pau Monné wrote:
>> >>> On Fri, Aug 28, 2026 at 09:02:18AM +0200, Jan Beulich wrote:
>> >>>> The use of unreachable(), when unreachability is visible to Eclair (and
>> >>>> compilers), is deemed a violation. Drop the redundant statement.
>> >>>
>> >>> Urg, isn't that something that should be fixed in Eclair then?
>> >>> Otherwise all the unreachable() calls in our codebase are likely to be
>> >>> found by Eclair sooner or later, and will need to be removed.
>> >>
>> >> No, aiui most are covered by deviations. In particular ones in BUG() and
>> >> ASSERT_UNREACHABLE().
>> >
>> > Shouldn't this be a deviation then also?
>> 
>> Maybe, just that I had no good idea how to express such a deviation 
>> (preferably
>> without a SAF comment).
>> 
>> >  Maybe it would be helpful if
>> > the commit message states why this is handled differently from other
>> > unreachable() instances then.
>> 
>> I've added "..., , and the one here isn't covered by a deviation" to 
>> the first
>> sentence. Will that suffice?
> 
> TBH, the handling of unreachable() feels inconsistent to me.  I don't
> blame you for this, I know you are just trying to fix the remaining
> issues.
> 
> I guess I will defer the change to someone more familiar with MISRA
> and why some unreachable() usages are covered by deviations while
> others aren't.
> 
> I think the point of adding something to the commit message is to
> justify why this is removed vs a deviation being added.
> 

Actually this should be done with a deviation, and I thought it was 
already taken care of by

-config=MC3A2.R2.1,statements+={deliberate, 
"call(decl(name(__builtin_unreachable||panic||do_unexpected_trap||machine_halt||machine_restart||reboot_or_halt)))"}

namely because unreachable() expands to a call to 
__builtin_unreachable(). It might be worth checking why that is not the 
case. Probably the configuration needs a slight tweaking.

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

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

[Nicola Vetrini]

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