[XEN PATCH 0/7] address violations of MISRA C:2012 Rule 2.1

[XEN PATCH 0/7] address violations of MISRA C:2012 Rule 2.1

[Nicola Vetrini]

MISRA C:2012 Rule 2.1 states: "A project shall not contain unreachable code".
As such, this series eliminates various instances of unreachable code found in
Xen, by providing equivalent compliant constructs.

This series is loosely based on my earlier series [1], but the overall approach
has changed since; for instance, declarations without initialization are now
ignored, as detailed in docs/misra/deviations.rst.

[1] https://lore.kernel.org/xen-devel/cover.1690985045.git.nicola.vetrini@bugseng.com/

Nicola Vetrini (7):
  xen/shutdown: address MISRA C:2012 Rule 2.1
  x86/mm: address MISRA C:2012 Rule 2.1
  xen/arm: address MISRA C:2012 Rule 2.1
  xen/sched: address MISRA C:2012 Rule 2.1
  xen/arm: traps: add ASSERT_UNREACHABLE() where needed
  x86/platform: removed break to address MISRA C:2012 Rule 2.1
  x86/xstate: move BUILD_BUG_ON to address MISRA C:2012 Rule 2.1

 xen/arch/arm/mm.c                 |  2 +-
 xen/arch/arm/traps.c              |  6 +++---
 xen/arch/arm/vgic-v3-its.c        | 10 +++++-----
 xen/arch/x86/mm.c                 |  6 +++---
 xen/arch/x86/platform_hypercall.c |  1 -
 xen/arch/x86/xstate.c             |  3 ++-
 xen/common/sched/core.c           |  1 -
 xen/common/shutdown.c             | 11 ++---------
 xen/include/xen/shutdown.h        |  2 +-
 9 files changed, 17 insertions(+), 25 deletions(-)

-- 
2.34.1

[XEN PATCH 1/7] xen/shutdown: address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

Given that 'hwdom_shutdown' is a noreturn function, unreachable
breaks can be eliminated to resolve violations of Rule 2.1.
On the occasion, the type of its parameter is changed to uint8_t.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/common/shutdown.c      | 11 ++---------
 xen/include/xen/shutdown.h |  2 +-
 2 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/xen/common/shutdown.c b/xen/common/shutdown.c
index 37901a4f3391..290f90d70fe1 100644
--- a/xen/common/shutdown.c
+++ b/xen/common/shutdown.c
@@ -30,7 +30,7 @@ static void noreturn maybe_reboot(void)
     }
 }
 
-void hwdom_shutdown(u8 reason)
+void hwdom_shutdown(uint8_t reason)
 {
     switch ( reason )
     {
@@ -38,39 +38,32 @@ void hwdom_shutdown(u8 reason)
         printk("Hardware Dom%u halted: halting machine\n",
                hardware_domain->domain_id);
         machine_halt();
-        break; /* not reached */
 
     case SHUTDOWN_crash:
         debugger_trap_immediate();
         printk("Hardware Dom%u crashed: ", hardware_domain->domain_id);
         kexec_crash(CRASHREASON_HWDOM);
         maybe_reboot();
-        break; /* not reached */
 
     case SHUTDOWN_reboot:
         printk("Hardware Dom%u shutdown: rebooting machine\n",
                hardware_domain->domain_id);
         machine_restart(0);
-        break; /* not reached */
 
     case SHUTDOWN_watchdog:
         printk("Hardware Dom%u shutdown: watchdog rebooting machine\n",
                hardware_domain->domain_id);
         kexec_crash(CRASHREASON_WATCHDOG);
         machine_restart(0);
-        break; /* not reached */
 
     case SHUTDOWN_soft_reset:
         printk("Hardware domain %d did unsupported soft reset, rebooting.\n",
                hardware_domain->domain_id);
         machine_restart(0);
-        break; /* not reached */
 
     default:
         printk("Hardware Dom%u shutdown (unknown reason %u): ",
                hardware_domain->domain_id, reason);
         maybe_reboot();
-        break; /* not reached */
     }
-}  
-
+}
diff --git a/xen/include/xen/shutdown.h b/xen/include/xen/shutdown.h
index 668aed0be580..3537c30e0a1b 100644
--- a/xen/include/xen/shutdown.h
+++ b/xen/include/xen/shutdown.h
@@ -6,7 +6,7 @@
 /* opt_noreboot: If true, machine will need manual reset on error. */
 extern bool opt_noreboot;
 
-void noreturn hwdom_shutdown(u8 reason);
+void noreturn hwdom_shutdown(uint8_t reason);
 
 void noreturn machine_restart(unsigned int delay_millisecs);
 void noreturn machine_halt(void);
-- 
2.34.1

[XEN PATCH 2/7] x86/mm: address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

The "return 0" after the swich statement in 'xen/arch/x86/mm.c'
is unreachable because all switch clauses end with returns.
However, some of them can be substituted with "break"s to allow
the "return 0" outside the switch to be reachable.

No functional changes.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/mm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 0a66db10b959..8b31426a5348 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4753,7 +4753,7 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         }
 
         spin_unlock(&d->arch.e820_lock);
-        return 0;
+        break;
     }
 
     case XENMEM_machine_memory_map:
@@ -4818,7 +4818,7 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( __copy_to_guest(arg, &ctxt.map, 1) )
             return -EFAULT;
 
-        return 0;
+        break;
     }
 
     case XENMEM_machphys_mapping:
@@ -4834,7 +4834,7 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( copy_to_guest(arg, &mapping, 1) )
             return -EFAULT;
 
-        return 0;
+        break;
     }
 
 #ifdef CONFIG_HVM
-- 
2.34.1

[XEN PATCH 3/7] xen/arm: address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

The "return 1;" statements at the end of some cases in the switch
of function 'vgic_v3_its_mmio_write' in 'vcig-v3-its.c' cause the
unreachability of the "return 1;" statement after the switch, thus
violating MISRA C:2012 Rule 2.1:
"A project shall not contain unreachable code".

The same is true for the switch in 'arch_memory_op' from
'xen/arch/arm/mm.c'.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/arm/mm.c          |  2 +-
 xen/arch/arm/vgic-v3-its.c | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index eeb65ca6bb79..9be8e711f61e 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -277,7 +277,7 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
     /* XXX: memsharing not working yet */
     case XENMEM_get_sharing_shared_pages:
     case XENMEM_get_sharing_freed_pages:
-        return 0;
+        break;
 
     default:
         return -ENOSYS;
diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index 05429030b539..b9195bbd0538 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -1255,7 +1255,7 @@ static int vgic_v3_its_mmio_write(struct vcpu *v, mmio_info_t *info,
                                                      reg32 & GITS_CTLR_ENABLE);
         spin_unlock(&its->its_lock);
         spin_unlock(&its->vcmd_lock);
-        return 1;
+        break;
     }
 
     case VREG32(GITS_IIDR):
@@ -1292,7 +1292,7 @@ static int vgic_v3_its_mmio_write(struct vcpu *v, mmio_info_t *info,
         its->creadr = 0;
         spin_unlock(&its->its_lock);
 
-        return 1;
+        break;
 
     case VREG64(GITS_CWRITER):
         if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
@@ -1308,7 +1308,7 @@ static int vgic_v3_its_mmio_write(struct vcpu *v, mmio_info_t *info,
 
         spin_unlock(&its->vcmd_lock);
 
-        return 1;
+        break;
 
     case VREG64(GITS_CREADR):
         goto write_ignore_64;
@@ -1353,7 +1353,7 @@ static int vgic_v3_its_mmio_write(struct vcpu *v, mmio_info_t *info,
 
         its->baser_dev = reg;
         spin_unlock(&its->its_lock);
-        return 1;
+        break;
 
     case VREG64(GITS_BASER1):           /* collection table */
         if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
@@ -1384,7 +1384,7 @@ static int vgic_v3_its_mmio_write(struct vcpu *v, mmio_info_t *info,
             its->max_collections = 0;
         its->baser_coll = reg;
         spin_unlock(&its->its_lock);
-        return 1;
+        break;
 
     case VRANGE64(GITS_BASER2, GITS_BASER7):
         goto write_ignore_64;
-- 
2.34.1

[XEN PATCH 4/7] xen/sched: address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

The break statement after the return statement is definitely unreachable
and can be removed with no functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/common/sched/core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index f6ac1e5af8bd..d177c675c81b 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -2239,7 +2239,6 @@ static bool sched_tasklet_check_cpu(unsigned int cpu)
         /* fallthrough */
     case TASKLET_enqueued|TASKLET_scheduled:
         return true;
-        break;
     case TASKLET_scheduled:
         clear_bit(_TASKLET_scheduled, tasklet_work);
         /* fallthrough */
-- 
2.34.1

[XEN PATCH 5/7] xen/arm: traps: add ASSERT_UNREACHABLE() where needed

[Nicola Vetrini]

The branches of the switch after a call to 'do_unexpected_trap'
cannot return, but there is one path that may return, hence
only some clauses are marked with ASSERT_UNREACHABLE().

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/arm/traps.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 3784e8276ef6..e7c920b043d2 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2152,7 +2152,7 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)
     case HSR_EC_SVE:
         /* An SVE exception is a bug somewhere in hypervisor code */
         do_unexpected_trap("SVE trap at EL2", regs);
-        break;
+        ASSERT_UNREACHABLE();
 #endif
     case HSR_EC_DATA_ABORT_CURR_EL:
     case HSR_EC_INSTR_ABORT_CURR_EL:
@@ -2171,13 +2171,13 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)
             dump_hyp_walk(get_hfar(is_data));
 
         do_unexpected_trap(fault, regs);
-
-        break;
+        ASSERT_UNREACHABLE();
     }
     default:
         printk("Hypervisor Trap. HSR=%#"PRIregister" EC=0x%x IL=%x Syndrome=0x%"PRIx32"\n",
                hsr.bits, hsr.ec, hsr.len, hsr.iss);
         do_unexpected_trap("Hypervisor", regs);
+        ASSERT_UNREACHABLE();
     }
 }
 
-- 
2.34.1

[XEN PATCH 6/7] x86/platform: removed break to address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

The break statement is redundant, hence it can be removed.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/platform_hypercall.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index 4dde71db275c..7556c6e6cd0c 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -723,7 +723,6 @@ ret_t do_platform_op(
 
         ret = continue_hypercall_on_cpu(
             0, cpu_down_helper, (void *)(unsigned long)cpu);
-        break;
     }
     break;
 
-- 
2.34.1

[XEN PATCH 7/7] x86/xstate: move BUILD_BUG_ON to address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

The string literal inside the expansion of BUILD_BUG_ON is considered
unreachable code; however, such statement can be moved earlier
with no functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
The motivation for this code movement is that keeping it inside the switch
statement matches MISRA's definition of unreachable code, but does not fall into
the category of declarations without initialization, which is already a deviated
aspect. An alternative approach would be to deviate BUILD_BUG_ON as well.
---
 xen/arch/x86/xstate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index cf94761d0542..99f0526c8988 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -396,9 +396,10 @@ void xrstor(struct vcpu *v, uint64_t mask)
      */
     for ( prev_faults = faults = 0; ; prev_faults = faults )
     {
+        BUILD_BUG_ON(sizeof(faults) != 4); /* Clang doesn't support %z in asm. */
+
         switch ( __builtin_expect(ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET], 8) )
         {
-            BUILD_BUG_ON(sizeof(faults) != 4); /* Clang doesn't support %z in asm. */
 #define _xrstor(insn) \
         asm volatile ( "1: .byte " insn "\n" \
                        "3:\n" \
-- 
2.34.1

Re: [XEN PATCH 3/7] xen/arm: address MISRA C:2012 Rule 2.1

[Julien Grall]

Hi,

On 11/12/2023 10:30, Nicola Vetrini wrote:
> The "return 1;" statements at the end of some cases in the switch
> of function 'vgic_v3_its_mmio_write' in 'vcig-v3-its.c' cause the
> unreachability of the "return 1;" statement after the switch, thus
> violating MISRA C:2012 Rule 2.1:
> "A project shall not contain unreachable code".
> 
> The same is true for the switch in 'arch_memory_op' from
> 'xen/arch/arm/mm.c'.

For both cases, I actually much prefer the "return" version in the 
cases. In particular for the vGIC emulation the switch is quite large 
and it would not be trivial to know what happens after the break.

IOW, I would much prefer if we remove the "return ..." outside of the 
switch.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 5/7] xen/arm: traps: add ASSERT_UNREACHABLE() where needed

[Julien Grall]

Hi,

On 11/12/2023 10:30, Nicola Vetrini wrote:
> The branches of the switch after a call to 'do_unexpected_trap'
> cannot return, but there is one path that may return, hence
> only some clauses are marked with ASSERT_UNREACHABLE().
I don't understand why this is necessary. The code should never be 
reachable because do_unexpected_trap() is a noreturn().

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 3/7] xen/arm: address MISRA C:2012 Rule 2.1

[Michal Orzel]

On 11/12/2023 13:29, Julien Grall wrote:
> 
> 
> Hi,
> 
> On 11/12/2023 10:30, Nicola Vetrini wrote:
>> The "return 1;" statements at the end of some cases in the switch
>> of function 'vgic_v3_its_mmio_write' in 'vcig-v3-its.c' cause the
>> unreachability of the "return 1;" statement after the switch, thus
>> violating MISRA C:2012 Rule 2.1:
>> "A project shall not contain unreachable code".
>>
>> The same is true for the switch in 'arch_memory_op' from
>> 'xen/arch/arm/mm.c'.
> 
> For both cases, I actually much prefer the "return" version in the
> cases. In particular for the vGIC emulation the switch is quite large
> and it would not be trivial to know what happens after the break.
Because of this...

> 
> IOW, I would much prefer if we remove the "return ..." outside of the
> switch.
wouldn't it be better to add ASSERT_UNREACHABLE() before this return instead of removing it?
This is what we have in e.g. vpl011 and it prevents mistakes.

~Michal

Re: [XEN PATCH 4/7] xen/sched: address MISRA C:2012 Rule 2.1

[George Dunlap]

On Mon, Dec 11, 2023 at 10:30 AM Nicola Vetrini
<nicola.vetrini@bugseng.com> wrote:
>
> The break statement after the return statement is definitely unreachable
> and can be removed with no functional change.
>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Acked-by: George Dunlap <george.dunlap@cloud.com>

Re: [XEN PATCH 3/7] xen/arm: address MISRA C:2012 Rule 2.1

[Julien Grall]

On 11/12/2023 13:06, Michal Orzel wrote:
> 
> 
> On 11/12/2023 13:29, Julien Grall wrote:
>>
>>
>> Hi,
>>
>> On 11/12/2023 10:30, Nicola Vetrini wrote:
>>> The "return 1;" statements at the end of some cases in the switch
>>> of function 'vgic_v3_its_mmio_write' in 'vcig-v3-its.c' cause the
>>> unreachability of the "return 1;" statement after the switch, thus
>>> violating MISRA C:2012 Rule 2.1:
>>> "A project shall not contain unreachable code".
>>>
>>> The same is true for the switch in 'arch_memory_op' from
>>> 'xen/arch/arm/mm.c'.
>>
>> For both cases, I actually much prefer the "return" version in the
>> cases. In particular for the vGIC emulation the switch is quite large
>> and it would not be trivial to know what happens after the break.
> Because of this...
> 
>>
>> IOW, I would much prefer if we remove the "return ..." outside of the
>> switch.
> wouldn't it be better to add ASSERT_UNREACHABLE() before this return instead of removing it?
> This is what we have in e.g. vpl011 and it prevents mistakes.

I am ok with that. But I am not sure if this would make ECLAIR unhappy.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 3/7] xen/arm: address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

On 2023-12-11 15:14, Julien Grall wrote:
> On 11/12/2023 13:06, Michal Orzel wrote:
>> 
>> 
>> On 11/12/2023 13:29, Julien Grall wrote:
>>> 
>>> 
>>> Hi,
>>> 
>>> On 11/12/2023 10:30, Nicola Vetrini wrote:
>>>> The "return 1;" statements at the end of some cases in the switch
>>>> of function 'vgic_v3_its_mmio_write' in 'vcig-v3-its.c' cause the
>>>> unreachability of the "return 1;" statement after the switch, thus
>>>> violating MISRA C:2012 Rule 2.1:
>>>> "A project shall not contain unreachable code".
>>>> 
>>>> The same is true for the switch in 'arch_memory_op' from
>>>> 'xen/arch/arm/mm.c'.
>>> 
>>> For both cases, I actually much prefer the "return" version in the
>>> cases. In particular for the vGIC emulation the switch is quite large
>>> and it would not be trivial to know what happens after the break.
>> Because of this...
>> 
>>> 
>>> IOW, I would much prefer if we remove the "return ..." outside of the
>>> switch.
>> wouldn't it be better to add ASSERT_UNREACHABLE() before this return 
>> instead of removing it?
>> This is what we have in e.g. vpl011 and it prevents mistakes.
> 
> I am ok with that. But I am not sure if this would make ECLAIR unhappy.
> 
> Cheers,

Should be okay. I'll test and report back

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 5/7] xen/arm: traps: add ASSERT_UNREACHABLE() where needed

[Nicola Vetrini]

On 2023-12-11 13:32, Julien Grall wrote:
> Hi,
> 
> On 11/12/2023 10:30, Nicola Vetrini wrote:
>> The branches of the switch after a call to 'do_unexpected_trap'
>> cannot return, but there is one path that may return, hence
>> only some clauses are marked with ASSERT_UNREACHABLE().
> I don't understand why this is necessary. The code should never be 
> reachable because do_unexpected_trap() is a noreturn().
> 
> Cheers,

It was meant as a safeguard against mistakes. There are MISRA rules that 
deal with this aspect (e.g., a noreturn function should not return), but 
they are not in Amendment 2, which is what Xen is following.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 5/7] xen/arm: traps: add ASSERT_UNREACHABLE() where needed

[Julien Grall]

Hi Nicola,

On 11/12/2023 14:54, Nicola Vetrini wrote:
> On 2023-12-11 13:32, Julien Grall wrote:
>> Hi,
>>
>> On 11/12/2023 10:30, Nicola Vetrini wrote:
>>> The branches of the switch after a call to 'do_unexpected_trap'
>>> cannot return, but there is one path that may return, hence
>>> only some clauses are marked with ASSERT_UNREACHABLE().
>> I don't understand why this is necessary. The code should never be 
>> reachable because do_unexpected_trap() is a noreturn().
>>
>> Cheers,
> 
> It was meant as a safeguard against mistakes. 

I am confused about which mistakes you are trying to prevent. Are you 
saying you are not trusting the noreturn attribute?

And if so, are you intending to add ASSERT_UNREACHABLE() after every 
single call to noreturn functions?

> There are MISRA rules that 
> deal with this aspect (e.g., a noreturn function should not return), but 
> they are not in Amendment 2, which is what Xen is following.
Do you mind providing a bit more details about the rules you are talking 
about? At least the numbers would be helpful.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 5/7] xen/arm: traps: add ASSERT_UNREACHABLE() where needed

[Julien Grall]

On 11/12/2023 15:59, Julien Grall wrote:
> Hi Nicola,
> 
> On 11/12/2023 14:54, Nicola Vetrini wrote:
>> On 2023-12-11 13:32, Julien Grall wrote:
>>> Hi,
>>>
>>> On 11/12/2023 10:30, Nicola Vetrini wrote:
>>>> The branches of the switch after a call to 'do_unexpected_trap'
>>>> cannot return, but there is one path that may return, hence
>>>> only some clauses are marked with ASSERT_UNREACHABLE().
>>> I don't understand why this is necessary. The code should never be 
>>> reachable because do_unexpected_trap() is a noreturn().
>>>
>>> Cheers,
>>
>> It was meant as a safeguard against mistakes. 
> 
> I am confused about which mistakes you are trying to prevent. Are you 
> saying you are not trusting the noreturn attribute?
> 
> And if so, are you intending to add ASSERT_UNREACHABLE() after every 
> single call to noreturn functions?

Replying to myself. What's confusing the most is that in [1], you 
decided to not add the ASSERT_UNREACHABLE(). Yet the problem is similar.

I'd also like to point out that by removing the "break", then if the 
'noreturn' function turns out to return, then in prod build you would 
fallthrough to the next case. And who knows what's going to happen...

All of this really adds some confusion...

> 
>> There are MISRA rules that deal with this aspect (e.g., a noreturn 
>> function should not return), but they are not in Amendment 2, which is 
>> what Xen is following.
> Do you mind providing a bit more details about the rules you are talking 
> about? At least the numbers would be helpful.

[1] 
https://lore.kernel.org/xen-devel/c0a8a12e39d688e101936d221af0f8eeefabe352.1702283415.git.nicola.vetrini@bugseng.com/

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 5/7] xen/arm: traps: add ASSERT_UNREACHABLE() where needed

[Nicola Vetrini]

On 2023-12-11 17:05, Julien Grall wrote:
> On 11/12/2023 15:59, Julien Grall wrote:
>> Hi Nicola,
>> 
>> On 11/12/2023 14:54, Nicola Vetrini wrote:
>>> On 2023-12-11 13:32, Julien Grall wrote:
>>>> Hi,
>>>> 
>>>> On 11/12/2023 10:30, Nicola Vetrini wrote:
>>>>> The branches of the switch after a call to 'do_unexpected_trap'
>>>>> cannot return, but there is one path that may return, hence
>>>>> only some clauses are marked with ASSERT_UNREACHABLE().
>>>> I don't understand why this is necessary. The code should never be 
>>>> reachable because do_unexpected_trap() is a noreturn().
>>>> 
>>>> Cheers,
>>> 
>>> It was meant as a safeguard against mistakes.
>> 
>> I am confused about which mistakes you are trying to prevent. Are you 
>> saying you are not trusting the noreturn attribute?
>> 
>> And if so, are you intending to add ASSERT_UNREACHABLE() after every 
>> single call to noreturn functions?
> 
> Replying to myself. What's confusing the most is that in [1], you 
> decided to not add the ASSERT_UNREACHABLE(). Yet the problem is 
> similar.
> 
> I'd also like to point out that by removing the "break", then if the 
> 'noreturn' function turns out to return, then in prod build you would 
> fallthrough to the next case. And who knows what's going to happen...
> 
> All of this really adds some confusion...
> 

I should have checked before responding: do_trap_hyp_sync is not afaik 
noreturn. Specifically, do_trap_brk may return. If I worked under the 
wrong assumption, then certainly the ASSERT_UNREACHABLE-s should be 
dropped.

>> 
>>> There are MISRA rules that deal with this aspect (e.g., a noreturn 
>>> function should not return), but they are not in Amendment 2, which 
>>> is what Xen is following.
>> Do you mind providing a bit more details about the rules you are 
>> talking about? At least the numbers would be helpful.
> 
> [1] 
> https://lore.kernel.org/xen-devel/c0a8a12e39d688e101936d221af0f8eeefabe352.1702283415.git.nicola.vetrini@bugseng.com/
> 
> Cheers,

Sure. Here are the rules concerned with _Noreturn:

R17.9: A function declared with a _Noreturn function specifier shall not 
return to its caller
R17.10: A function declared with a _Noreturn function specifier shall 
have void return type
R17.11: A function that never returns should be declared with a 
_Noreturn function specifier

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 5/7] xen/arm: traps: add ASSERT_UNREACHABLE() where needed

[Stefano Stabellini]

On Mon, 11 Dec 2023, Nicola Vetrini wrote:
> On 2023-12-11 17:05, Julien Grall wrote:
> > On 11/12/2023 15:59, Julien Grall wrote:
> > > Hi Nicola,
> > > 
> > > On 11/12/2023 14:54, Nicola Vetrini wrote:
> > > > On 2023-12-11 13:32, Julien Grall wrote:
> > > > > Hi,
> > > > > 
> > > > > On 11/12/2023 10:30, Nicola Vetrini wrote:
> > > > > > The branches of the switch after a call to 'do_unexpected_trap'
> > > > > > cannot return, but there is one path that may return, hence
> > > > > > only some clauses are marked with ASSERT_UNREACHABLE().
> > > > > I don't understand why this is necessary. The code should never be
> > > > > reachable because do_unexpected_trap() is a noreturn().
> > > > > 
> > > > > Cheers,
> > > > 
> > > > It was meant as a safeguard against mistakes.
> > > 
> > > I am confused about which mistakes you are trying to prevent. Are you
> > > saying you are not trusting the noreturn attribute?
> > > 
> > > And if so, are you intending to add ASSERT_UNREACHABLE() after every
> > > single call to noreturn functions?
> > 
> > Replying to myself. What's confusing the most is that in [1], you decided to
> > not add the ASSERT_UNREACHABLE(). Yet the problem is similar.
> > 
> > I'd also like to point out that by removing the "break", then if the
> > 'noreturn' function turns out to return, then in prod build you would
> > fallthrough to the next case. And who knows what's going to happen...
> > 
> > All of this really adds some confusion...
> > 
> 
> I should have checked before responding: do_trap_hyp_sync is not afaik
> noreturn. Specifically, do_trap_brk may return. If I worked under the wrong
> assumption, then certainly the ASSERT_UNREACHABLE-s should be dropped.

It looks like we could add noreturn to do_trap_brk. Julien what do you
think?

Re: [XEN PATCH 1/7] xen/shutdown: address MISRA C:2012 Rule 2.1

[Stefano Stabellini]

On Mon, 11 Dec 2023, Nicola Vetrini wrote:
> Given that 'hwdom_shutdown' is a noreturn function, unreachable
> breaks can be eliminated to resolve violations of Rule 2.1.
> On the occasion, the type of its parameter is changed to uint8_t.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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

Re: [XEN PATCH 2/7] x86/mm: address MISRA C:2012 Rule 2.1

[Stefano Stabellini]

On Mon, 11 Dec 2023, Nicola Vetrini wrote:
> The "return 0" after the swich statement in 'xen/arch/x86/mm.c'
> is unreachable because all switch clauses end with returns.
> However, some of them can be substituted with "break"s to allow
> the "return 0" outside the switch to be reachable.
> 
> No functional changes.

This is correct but makes the code inconsistent. I would either remove
the return 0; at the end of arch_memory_op, or do the following:

- initialize rc to 0 at the beginning: int rc = 0;
- all switch clauses break instead of return;
- at the end: return rc;


> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>  xen/arch/x86/mm.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 0a66db10b959..8b31426a5348 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4753,7 +4753,7 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          }
>  
>          spin_unlock(&d->arch.e820_lock);
> -        return 0;
> +        break;
>      }
>  
>      case XENMEM_machine_memory_map:
> @@ -4818,7 +4818,7 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( __copy_to_guest(arg, &ctxt.map, 1) )
>              return -EFAULT;
>  
> -        return 0;
> +        break;
>      }
>  
>      case XENMEM_machphys_mapping:
> @@ -4834,7 +4834,7 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( copy_to_guest(arg, &mapping, 1) )
>              return -EFAULT;
>  
> -        return 0;
> +        break;
>      }
>  
>  #ifdef CONFIG_HVM
> -- 
> 2.34.1
> 
> 

Re: [XEN PATCH 4/7] xen/sched: address MISRA C:2012 Rule 2.1

[Stefano Stabellini]

On Mon, 11 Dec 2023, Nicola Vetrini wrote:
> The break statement after the return statement is definitely unreachable
> and can be removed with no functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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

Re: [XEN PATCH 6/7] x86/platform: removed break to address MISRA C:2012 Rule 2.1

[Stefano Stabellini]

On Mon, 11 Dec 2023, Nicola Vetrini wrote:
> The break statement is redundant, hence it can be removed.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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

Re: [XEN PATCH 7/7] x86/xstate: move BUILD_BUG_ON to address MISRA C:2012 Rule 2.1

[Stefano Stabellini]

On Mon, 11 Dec 2023, Nicola Vetrini wrote:
> The string literal inside the expansion of BUILD_BUG_ON is considered
> unreachable code; however, such statement can be moved earlier
> with no functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> The motivation for this code movement is that keeping it inside the switch
> statement matches MISRA's definition of unreachable code, but does not fall into
> the category of declarations without initialization, which is already a deviated
> aspect. An alternative approach would be to deviate BUILD_BUG_ON as well.

I think that deviating BUILD_BUG_ON would be totally fine. But given
that this patch is obviously correct:

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


> ---
>  xen/arch/x86/xstate.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
> index cf94761d0542..99f0526c8988 100644
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -396,9 +396,10 @@ void xrstor(struct vcpu *v, uint64_t mask)
>       */
>      for ( prev_faults = faults = 0; ; prev_faults = faults )
>      {
> +        BUILD_BUG_ON(sizeof(faults) != 4); /* Clang doesn't support %z in asm. */
> +
>          switch ( __builtin_expect(ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET], 8) )
>          {
> -            BUILD_BUG_ON(sizeof(faults) != 4); /* Clang doesn't support %z in asm. */
>  #define _xrstor(insn) \
>          asm volatile ( "1: .byte " insn "\n" \
>                         "3:\n" \
> -- 
> 2.34.1
> 

Re: [XEN PATCH 2/7] x86/mm: address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

On 2023-12-12 02:42, Stefano Stabellini wrote:
> On Mon, 11 Dec 2023, Nicola Vetrini wrote:
>> The "return 0" after the swich statement in 'xen/arch/x86/mm.c'
>> is unreachable because all switch clauses end with returns.
>> However, some of them can be substituted with "break"s to allow
>> the "return 0" outside the switch to be reachable.
>> 
>> No functional changes.
> 
> This is correct but makes the code inconsistent. I would either remove
> the return 0; at the end of arch_memory_op, or do the following:
> 
> - initialize rc to 0 at the beginning: int rc = 0;
> - all switch clauses break instead of return;
> - at the end: return rc;
> 

Given the feedback on the Arm side, the first solution is likely to be 
preferred.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 5/7] xen/arm: traps: add ASSERT_UNREACHABLE() where needed

[Julien Grall]

Hi Stefano,

On 12/12/2023 01:36, Stefano Stabellini wrote:
> On Mon, 11 Dec 2023, Nicola Vetrini wrote:
>> On 2023-12-11 17:05, Julien Grall wrote:
>>> On 11/12/2023 15:59, Julien Grall wrote:
>>>> Hi Nicola,
>>>>
>>>> On 11/12/2023 14:54, Nicola Vetrini wrote:
>>>>> On 2023-12-11 13:32, Julien Grall wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 11/12/2023 10:30, Nicola Vetrini wrote:
>>>>>>> The branches of the switch after a call to 'do_unexpected_trap'
>>>>>>> cannot return, but there is one path that may return, hence
>>>>>>> only some clauses are marked with ASSERT_UNREACHABLE().
>>>>>> I don't understand why this is necessary. The code should never be
>>>>>> reachable because do_unexpected_trap() is a noreturn().
>>>>>>
>>>>>> Cheers,
>>>>>
>>>>> It was meant as a safeguard against mistakes.
>>>>
>>>> I am confused about which mistakes you are trying to prevent. Are you
>>>> saying you are not trusting the noreturn attribute?
>>>>
>>>> And if so, are you intending to add ASSERT_UNREACHABLE() after every
>>>> single call to noreturn functions?
>>>
>>> Replying to myself. What's confusing the most is that in [1], you decided to
>>> not add the ASSERT_UNREACHABLE(). Yet the problem is similar.
>>>
>>> I'd also like to point out that by removing the "break", then if the
>>> 'noreturn' function turns out to return, then in prod build you would
>>> fallthrough to the next case. And who knows what's going to happen...
>>>
>>> All of this really adds some confusion...
>>>
>>
>> I should have checked before responding: do_trap_hyp_sync is not afaik
>> noreturn. Specifically, do_trap_brk may return. If I worked under the wrong
>> assumption, then certainly the ASSERT_UNREACHABLE-s should be dropped.
> 
> It looks like we could add noreturn to do_trap_brk. Julien what do you
> think?

You can't add noreturn to do_trap_brk(). It is used to handle WARN_ON() 
that *will* return.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 1/7] xen/shutdown: address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 11.12.2023 11:30, Nicola Vetrini wrote:
> Given that 'hwdom_shutdown' is a noreturn function, unreachable
> breaks can be eliminated to resolve violations of Rule 2.1.
> On the occasion, the type of its parameter is changed to uint8_t.

I don't particularly mind that extra change, but it's entirely unrelated
here. We do such when we touch lines (or maybe adjacent lines) anyway,
but here you had no need to touch either declaration or the header of
the definition.

> --- a/xen/common/shutdown.c
> +++ b/xen/common/shutdown.c
> @@ -30,7 +30,7 @@ static void noreturn maybe_reboot(void)
>      }
>  }
>  
> -void hwdom_shutdown(u8 reason)
> +void hwdom_shutdown(uint8_t reason)
>  {
>      switch ( reason )
>      {
> @@ -38,39 +38,32 @@ void hwdom_shutdown(u8 reason)
>          printk("Hardware Dom%u halted: halting machine\n",
>                 hardware_domain->domain_id);
>          machine_halt();
> -        break; /* not reached */
>  
>      case SHUTDOWN_crash:
>          debugger_trap_immediate();
>          printk("Hardware Dom%u crashed: ", hardware_domain->domain_id);
>          kexec_crash(CRASHREASON_HWDOM);
>          maybe_reboot();
> -        break; /* not reached */

Given the function's name, I don't view this removal (and the similar one
further down) as acceptable. The function first needs renaming so that
from its name one can no longer (wrongly) infer that it may reboot or else
return.

Jan

Re: [XEN PATCH 2/7] x86/mm: address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 12.12.2023 10:12, Nicola Vetrini wrote:
> On 2023-12-12 02:42, Stefano Stabellini wrote:
>> On Mon, 11 Dec 2023, Nicola Vetrini wrote:
>>> The "return 0" after the swich statement in 'xen/arch/x86/mm.c'
>>> is unreachable because all switch clauses end with returns.
>>> However, some of them can be substituted with "break"s to allow
>>> the "return 0" outside the switch to be reachable.
>>>
>>> No functional changes.
>>
>> This is correct but makes the code inconsistent. I would either remove
>> the return 0; at the end of arch_memory_op, or do the following:
>>
>> - initialize rc to 0 at the beginning: int rc = 0;
>> - all switch clauses break instead of return;
>> - at the end: return rc;
> 
> Given the feedback on the Arm side, the first solution is likely to be 
> preferred.

I wouldn't mind either option, with
- the former ensured to be okay with all compiler versions we (still)
  support,
- the latter having the initialize rc to 0 part dropped; imo it's better
  if every case block makes sure to set the intended value explicitly.

Jan

Re: [XEN PATCH 1/7] xen/shutdown: address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

On 2023-12-12 10:45, Jan Beulich wrote:
> On 11.12.2023 11:30, Nicola Vetrini wrote:
>> Given that 'hwdom_shutdown' is a noreturn function, unreachable
>> breaks can be eliminated to resolve violations of Rule 2.1.
>> On the occasion, the type of its parameter is changed to uint8_t.
> 
> I don't particularly mind that extra change, but it's entirely 
> unrelated
> here. We do such when we touch lines (or maybe adjacent lines) anyway,
> but here you had no need to touch either declaration or the header of
> the definition.
> 

Ok. I can make this a separate patch, since I'm already touching the 
function

>> --- a/xen/common/shutdown.c
>> +++ b/xen/common/shutdown.c
>> @@ -30,7 +30,7 @@ static void noreturn maybe_reboot(void)
>>      }
>>  }
>> 
>> -void hwdom_shutdown(u8 reason)
>> +void hwdom_shutdown(uint8_t reason)
>>  {
>>      switch ( reason )
>>      {
>> @@ -38,39 +38,32 @@ void hwdom_shutdown(u8 reason)
>>          printk("Hardware Dom%u halted: halting machine\n",
>>                 hardware_domain->domain_id);
>>          machine_halt();
>> -        break; /* not reached */
>> 
>>      case SHUTDOWN_crash:
>>          debugger_trap_immediate();
>>          printk("Hardware Dom%u crashed: ", 
>> hardware_domain->domain_id);
>>          kexec_crash(CRASHREASON_HWDOM);
>>          maybe_reboot();
>> -        break; /* not reached */
> 
> Given the function's name, I don't view this removal (and the similar 
> one
> further down) as acceptable. The function first needs renaming so that
> from its name one can no longer (wrongly) infer that it may reboot or 
> else
> return.
> 
> Jan

Something like "reboot_or_halt" would be okay?

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 7/7] x86/xstate: move BUILD_BUG_ON to address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 11.12.2023 11:30, Nicola Vetrini wrote:
> The string literal inside the expansion of BUILD_BUG_ON is considered
> unreachable code; however, such statement can be moved earlier
> with no functional change.

First: Why is this deemed dead code in its present position, but okay when
moved? Second: While moving is indeed no functional change (really
BUILD_BUG_ON() can be moved about anywhere, for not producing any code in
the final binary), it removes the connection between it and the respective
asm() (where %z would have been nice to use).

Jan

Re: [XEN PATCH 7/7] x86/xstate: move BUILD_BUG_ON to address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 12.12.2023 11:04, Jan Beulich wrote:
> On 11.12.2023 11:30, Nicola Vetrini wrote:
>> The string literal inside the expansion of BUILD_BUG_ON is considered
>> unreachable code; however, such statement can be moved earlier
>> with no functional change.
> 
> First: Why is this deemed dead code in its present position, but okay when
> moved? Second: While moving is indeed no functional change (really
> BUILD_BUG_ON() can be moved about anywhere, for not producing any code in
> the final binary), it removes the connection between it and the respective
> asm() (where %z would have been nice to use).

Oh, and third: Which string literal? I expect you're not building with
an ancient compiler, so it got to be

#define BUILD_BUG_ON(cond) ({ _Static_assert(!(cond), "!(" #cond ")"); })

which you see in use. Yet that string literal isn't "code" or "data", but
an argument to _Static_assert(). Is Eclair perhaps not properly aware of
_Static_assert()?

Jan

Re: [XEN PATCH 6/7] x86/platform: removed break to address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 11.12.2023 11:30, Nicola Vetrini wrote:
> The break statement is redundant, hence it can be removed.

Except ...

> --- a/xen/arch/x86/platform_hypercall.c
> +++ b/xen/arch/x86/platform_hypercall.c
> @@ -723,7 +723,6 @@ ret_t do_platform_op(
>  
>          ret = continue_hypercall_on_cpu(
>              0, cpu_down_helper, (void *)(unsigned long)cpu);
> -        break;
>      }
>      break;

... it wants to be the other break that is removed, imo. Andrew, Roger,
what do you think? There are many such (again: imo) oddly placed break-s
in that switch() ... In some cases there are also inner scopes without
there being new local variables in there. IOW imo throughout this
switch()
- pointless inner scopes want dropping,
- all "main" break-s want to have the same indentation.

Jan

Re: [XEN PATCH 1/7] xen/shutdown: address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 12.12.2023 10:53, Nicola Vetrini wrote:
> On 2023-12-12 10:45, Jan Beulich wrote:
>> On 11.12.2023 11:30, Nicola Vetrini wrote:
>>> @@ -38,39 +38,32 @@ void hwdom_shutdown(u8 reason)
>>>          printk("Hardware Dom%u halted: halting machine\n",
>>>                 hardware_domain->domain_id);
>>>          machine_halt();
>>> -        break; /* not reached */
>>>
>>>      case SHUTDOWN_crash:
>>>          debugger_trap_immediate();
>>>          printk("Hardware Dom%u crashed: ", 
>>> hardware_domain->domain_id);
>>>          kexec_crash(CRASHREASON_HWDOM);
>>>          maybe_reboot();
>>> -        break; /* not reached */
>>
>> Given the function's name, I don't view this removal (and the similar 
>> one
>> further down) as acceptable. The function first needs renaming so that
>> from its name one can no longer (wrongly) infer that it may reboot or 
>> else
>> return.
> 
> Something like "reboot_or_halt" would be okay?

I think so, yes.

Jan

Re: [XEN PATCH 7/7] x86/xstate: move BUILD_BUG_ON to address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

On 2023-12-12 11:07, Jan Beulich wrote:
> On 12.12.2023 11:04, Jan Beulich wrote:
>> On 11.12.2023 11:30, Nicola Vetrini wrote:
>>> The string literal inside the expansion of BUILD_BUG_ON is considered
>>> unreachable code; however, such statement can be moved earlier
>>> with no functional change.
>> 
>> First: Why is this deemed dead code in its present position, but okay 
>> when
>> moved? Second: While moving is indeed no functional change (really
>> BUILD_BUG_ON() can be moved about anywhere, for not producing any code 
>> in
>> the final binary), it removes the connection between it and the 
>> respective
>> asm() (where %z would have been nice to use).
> 
> Oh, and third: Which string literal? I expect you're not building with
> an ancient compiler, so it got to be
> 
> #define BUILD_BUG_ON(cond) ({ _Static_assert(!(cond), "!(" #cond ")"); 
> })
> 
> which you see in use. Yet that string literal isn't "code" or "data", 
> but
> an argument to _Static_assert(). Is Eclair perhaps not properly aware 
> of
> _Static_assert()?
> 
> Jan

On further inspection, this should have fallen into the deviation for 
pure decls. This patch can be dropped, we'll adjust this inside ECLAIR.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 7/7] x86/xstate: move BUILD_BUG_ON to address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 12.12.2023 14:38, Nicola Vetrini wrote:
> On 2023-12-12 11:07, Jan Beulich wrote:
>> On 12.12.2023 11:04, Jan Beulich wrote:
>>> On 11.12.2023 11:30, Nicola Vetrini wrote:
>>>> The string literal inside the expansion of BUILD_BUG_ON is considered
>>>> unreachable code; however, such statement can be moved earlier
>>>> with no functional change.
>>>
>>> First: Why is this deemed dead code in its present position, but okay 
>>> when
>>> moved? Second: While moving is indeed no functional change (really
>>> BUILD_BUG_ON() can be moved about anywhere, for not producing any code 
>>> in
>>> the final binary), it removes the connection between it and the 
>>> respective
>>> asm() (where %z would have been nice to use).
>>
>> Oh, and third: Which string literal? I expect you're not building with
>> an ancient compiler, so it got to be
>>
>> #define BUILD_BUG_ON(cond) ({ _Static_assert(!(cond), "!(" #cond ")"); 
>> })
>>
>> which you see in use. Yet that string literal isn't "code" or "data", 
>> but
>> an argument to _Static_assert(). Is Eclair perhaps not properly aware 
>> of
>> _Static_assert()?
> 
> On further inspection, this should have fallen into the deviation for 
> pure decls. This patch can be dropped, we'll adjust this inside ECLAIR.

What's the connection to "pure" here? Or are you merely piggybacking on
that attribute for this non-function?

Jan

Re: [XEN PATCH 7/7] x86/xstate: move BUILD_BUG_ON to address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

On 2023-12-12 15:01, Jan Beulich wrote:
> On 12.12.2023 14:38, Nicola Vetrini wrote:
>> On 2023-12-12 11:07, Jan Beulich wrote:
>>> On 12.12.2023 11:04, Jan Beulich wrote:
>>>> On 11.12.2023 11:30, Nicola Vetrini wrote:
>>>>> The string literal inside the expansion of BUILD_BUG_ON is 
>>>>> considered
>>>>> unreachable code; however, such statement can be moved earlier
>>>>> with no functional change.
>>>> 
>>>> First: Why is this deemed dead code in its present position, but 
>>>> okay
>>>> when
>>>> moved? Second: While moving is indeed no functional change (really
>>>> BUILD_BUG_ON() can be moved about anywhere, for not producing any 
>>>> code
>>>> in
>>>> the final binary), it removes the connection between it and the
>>>> respective
>>>> asm() (where %z would have been nice to use).
>>> 
>>> Oh, and third: Which string literal? I expect you're not building 
>>> with
>>> an ancient compiler, so it got to be
>>> 
>>> #define BUILD_BUG_ON(cond) ({ _Static_assert(!(cond), "!(" #cond 
>>> ")");
>>> })
>>> 
>>> which you see in use. Yet that string literal isn't "code" or "data",
>>> but
>>> an argument to _Static_assert(). Is Eclair perhaps not properly aware
>>> of
>>> _Static_assert()?
>> 
>> On further inspection, this should have fallen into the deviation for
>> pure decls. This patch can be dropped, we'll adjust this inside 
>> ECLAIR.
> 
> What's the connection to "pure" here? Or are you merely piggybacking on
> that attribute for this non-function?
> 
> Jan

Just a naming coincidence, there aren't any attributes involved. No 
change to Xen code is needed.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 5/7] xen/arm: traps: add ASSERT_UNREACHABLE() where needed

[Julien Grall]

Hi,

On 11/12/2023 12:32, Julien Grall wrote:
> Hi,
> 
> On 11/12/2023 10:30, Nicola Vetrini wrote:
>> The branches of the switch after a call to 'do_unexpected_trap'
>> cannot return, but there is one path that may return, hence
>> only some clauses are marked with ASSERT_UNREACHABLE().
> I don't understand why this is necessary. The code should never be 
> reachable because do_unexpected_trap() is a noreturn().

 From the matrix discussion, it wasn't clear what was my position on 
this patch.

I would much prefer if the breaks are kept. I could accept:

ASSERT_UNREACHABLE();
break;

But this solution is a Nack because if you are concerned about functions 
like do_unexpected_trap() to return by mistaken, then it needs to also 
be safe in production.

The current proposal is not safe.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 6/7] x86/platform: removed break to address MISRA C:2012 Rule 2.1

[Stefano Stabellini]

On Tue, 12 Dec 2023, Jan Beulich wrote:
> On 11.12.2023 11:30, Nicola Vetrini wrote:
> > The break statement is redundant, hence it can be removed.
> 
> Except ...
> 
> > --- a/xen/arch/x86/platform_hypercall.c
> > +++ b/xen/arch/x86/platform_hypercall.c
> > @@ -723,7 +723,6 @@ ret_t do_platform_op(
> >  
> >          ret = continue_hypercall_on_cpu(
> >              0, cpu_down_helper, (void *)(unsigned long)cpu);
> > -        break;
> >      }
> >      break;
> 
> ... it wants to be the other break that is removed, imo.

I was also about to comment about which of the two breaks to remove... I
didn't know if there are x86 specific conventions so I didn't say
anything about it. But I also think it is more natural to keep the other
break.

Re: [XEN PATCH 6/7] x86/platform: removed break to address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

On 2023-12-12 11:13, Jan Beulich wrote:
> On 11.12.2023 11:30, Nicola Vetrini wrote:
>> The break statement is redundant, hence it can be removed.
> 
> Except ...
> 
>> --- a/xen/arch/x86/platform_hypercall.c
>> +++ b/xen/arch/x86/platform_hypercall.c
>> @@ -723,7 +723,6 @@ ret_t do_platform_op(
>> 
>>          ret = continue_hypercall_on_cpu(
>>              0, cpu_down_helper, (void *)(unsigned long)cpu);
>> -        break;
>>      }
>>      break;
> 
> ... it wants to be the other break that is removed, imo. Andrew, Roger,
> what do you think? There are many such (again: imo) oddly placed 
> break-s
> in that switch() ... In some cases there are also inner scopes without
> there being new local variables in there. IOW imo throughout this
> switch()
> - pointless inner scopes want dropping,
> - all "main" break-s want to have the same indentation.
> 
> Jan

Ok. I'm not particularly keen on doing a full style cleanup; I can drop 
the other break, for the sake of resolving the MISRA violation, so that 
the cleanup can happen anytime.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 5/7] xen/arm: traps: add ASSERT_UNREACHABLE() where needed

[Nicola Vetrini]

On 2023-12-12 16:49, Julien Grall wrote:
> Hi,
> 
> On 11/12/2023 12:32, Julien Grall wrote:
>> Hi,
>> 
>> On 11/12/2023 10:30, Nicola Vetrini wrote:
>>> The branches of the switch after a call to 'do_unexpected_trap'
>>> cannot return, but there is one path that may return, hence
>>> only some clauses are marked with ASSERT_UNREACHABLE().
>> I don't understand why this is necessary. The code should never be 
>> reachable because do_unexpected_trap() is a noreturn().
> 
> From the matrix discussion, it wasn't clear what was my position on 
> this patch.
> 
> I would much prefer if the breaks are kept. I could accept:
> 
> ASSERT_UNREACHABLE();
> break;
> 
> But this solution is a Nack because if you are concerned about 
> functions like do_unexpected_trap() to return by mistaken, then it 
> needs to also be safe in production.
> 
> The current proposal is not safe.
> 
> Cheers,

Ok. I wonder whether the should be applied here in vcpreg.c:

diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
index 39aeda9dab62..089d2f03eb5e 100644
--- a/xen/arch/arm/vcpreg.c
+++ b/xen/arch/arm/vcpreg.c
@@ -707,7 +707,8 @@ void do_cp10(struct cpu_user_regs *regs, const union 
hsr hsr)
          inject_undef_exception(regs, hsr);
          return;
      }
-
+
+    ASSERT_UNREACHABLE();
      advance_pc(regs, hsr);
  }

the rationale being that, should the switch somehow fail to return, the 
advance_pc would be called, rather than doing nothing.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 2/7] x86/mm: address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

On 2023-12-12 10:53, Jan Beulich wrote:
> On 12.12.2023 10:12, Nicola Vetrini wrote:
>> On 2023-12-12 02:42, Stefano Stabellini wrote:
>>> On Mon, 11 Dec 2023, Nicola Vetrini wrote:
>>>> The "return 0" after the swich statement in 'xen/arch/x86/mm.c'
>>>> is unreachable because all switch clauses end with returns.
>>>> However, some of them can be substituted with "break"s to allow
>>>> the "return 0" outside the switch to be reachable.
>>>> 
>>>> No functional changes.
>>> 
>>> This is correct but makes the code inconsistent. I would either 
>>> remove
>>> the return 0; at the end of arch_memory_op, or do the following:
>>> 
>>> - initialize rc to 0 at the beginning: int rc = 0;
>>> - all switch clauses break instead of return;
>>> - at the end: return rc;
>> 
>> Given the feedback on the Arm side, the first solution is likely to be
>> preferred.
> 
> I wouldn't mind either option, with
> - the former ensured to be okay with all compiler versions we (still)
>   support,

I tested a stripped-down version of the switch on godbolt.org (as far 
back as gcc-4.8.5) and it doesn't complain. It should be tested on a 
real Xen build, though.

> - the latter having the initialize rc to 0 part dropped; imo it's 
> better
>   if every case block makes sure to set the intended value explicitly.
> 

This is a lot of churn, I'd rather avoid it.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 2/7] x86/mm: address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 13.12.2023 15:44, Nicola Vetrini wrote:
> On 2023-12-12 10:53, Jan Beulich wrote:
>> On 12.12.2023 10:12, Nicola Vetrini wrote:
>>> On 2023-12-12 02:42, Stefano Stabellini wrote:
>>>> On Mon, 11 Dec 2023, Nicola Vetrini wrote:
>>>>> The "return 0" after the swich statement in 'xen/arch/x86/mm.c'
>>>>> is unreachable because all switch clauses end with returns.
>>>>> However, some of them can be substituted with "break"s to allow
>>>>> the "return 0" outside the switch to be reachable.
>>>>>
>>>>> No functional changes.
>>>>
>>>> This is correct but makes the code inconsistent. I would either 
>>>> remove
>>>> the return 0; at the end of arch_memory_op, or do the following:
>>>>
>>>> - initialize rc to 0 at the beginning: int rc = 0;
>>>> - all switch clauses break instead of return;
>>>> - at the end: return rc;
>>>
>>> Given the feedback on the Arm side, the first solution is likely to be
>>> preferred.
>>
>> I wouldn't mind either option, with
>> - the former ensured to be okay with all compiler versions we (still)
>>   support,
> 
> I tested a stripped-down version of the switch on godbolt.org (as far 
> back as gcc-4.8.5) and it doesn't complain. It should be tested on a 
> real Xen build, though.

I didn't fear any issue when going back to just 4.8. Quoting ./README:

      - For x86:
        - GCC 4.1.2_20070115 or later

>> - the latter having the initialize rc to 0 part dropped; imo it's 
>> better
>>   if every case block makes sure to set the intended value explicitly.
> 
> This is a lot of churn, I'd rather avoid it.

Rant (sorry): There's already excessive churn for entirely benign issues
that Misra claims need adjusting.

Jan

Re: [XEN PATCH 2/7] x86/mm: address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

On 2023-12-14 08:57, Jan Beulich wrote:
> On 13.12.2023 15:44, Nicola Vetrini wrote:
>> On 2023-12-12 10:53, Jan Beulich wrote:
>>> On 12.12.2023 10:12, Nicola Vetrini wrote:
>>>> On 2023-12-12 02:42, Stefano Stabellini wrote:
>>>>> On Mon, 11 Dec 2023, Nicola Vetrini wrote:
>>>>>> The "return 0" after the swich statement in 'xen/arch/x86/mm.c'
>>>>>> is unreachable because all switch clauses end with returns.
>>>>>> However, some of them can be substituted with "break"s to allow
>>>>>> the "return 0" outside the switch to be reachable.
>>>>>> 
>>>>>> No functional changes.
>>>>> 
>>>>> This is correct but makes the code inconsistent. I would either
>>>>> remove
>>>>> the return 0; at the end of arch_memory_op, or do the following:
>>>>> 
>>>>> - initialize rc to 0 at the beginning: int rc = 0;
>>>>> - all switch clauses break instead of return;
>>>>> - at the end: return rc;
>>>> 
>>>> Given the feedback on the Arm side, the first solution is likely to 
>>>> be
>>>> preferred.
>>> 
>>> I wouldn't mind either option, with
>>> - the former ensured to be okay with all compiler versions we (still)
>>>   support,
>> 
>> I tested a stripped-down version of the switch on godbolt.org (as far
>> back as gcc-4.8.5) and it doesn't complain. It should be tested on a
>> real Xen build, though.
> 
> I didn't fear any issue when going back to just 4.8. Quoting ./README:
> 
>       - For x86:
>         - GCC 4.1.2_20070115 or later
> 

I found no issue in 4.1.2 (see https://godbolt.org/z/cxecnKseG)

>>> - the latter having the initialize rc to 0 part dropped; imo it's
>>> better
>>>   if every case block makes sure to set the intended value 
>>> explicitly.
>> 
>> This is a lot of churn, I'd rather avoid it.
> 
> Rant (sorry): There's already excessive churn for entirely benign 
> issues
> that Misra claims need adjusting.
> 
> Jan

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 5/7] xen/arm: traps: add ASSERT_UNREACHABLE() where needed

[Julien Grall]

Hi,

On 13/12/2023 14:02, Nicola Vetrini wrote:
> On 2023-12-12 16:49, Julien Grall wrote:
>> Hi,
>>
>> On 11/12/2023 12:32, Julien Grall wrote:
>>> Hi,
>>>
>>> On 11/12/2023 10:30, Nicola Vetrini wrote:
>>>> The branches of the switch after a call to 'do_unexpected_trap'
>>>> cannot return, but there is one path that may return, hence
>>>> only some clauses are marked with ASSERT_UNREACHABLE().
>>> I don't understand why this is necessary. The code should never be 
>>> reachable because do_unexpected_trap() is a noreturn().
>>
>> From the matrix discussion, it wasn't clear what was my position on 
>> this patch.
>>
>> I would much prefer if the breaks are kept. I could accept:
>>
>> ASSERT_UNREACHABLE();
>> break;
>>
>> But this solution is a Nack because if you are concerned about 
>> functions like do_unexpected_trap() to return by mistaken, then it 
>> needs to also be safe in production.
>>
>> The current proposal is not safe.
>>
>> Cheers,
> 
> Ok. I wonder whether the should be applied here in vcpreg.c:
> 
> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
> index 39aeda9dab62..089d2f03eb5e 100644
> --- a/xen/arch/arm/vcpreg.c
> +++ b/xen/arch/arm/vcpreg.c
> @@ -707,7 +707,8 @@ void do_cp10(struct cpu_user_regs *regs, const union 
> hsr hsr)
>           inject_undef_exception(regs, hsr);
>           return;
>       }
> -
> +
> +    ASSERT_UNREACHABLE();
>       advance_pc(regs, hsr);
>   }
> 
> the rationale being that, should the switch somehow fail to return, the 
> advance_pc would be called, rather than doing nothing.

To clarify, advance_pc(regs, hsr) would still be called in production 
build. So if you are concerned about advance_pc() been called, then 
adding an ASSERT_UNREACHABLE() is not going to help.

It took me a little while to confirm that none of the path effectively 
returns due to the macros (in hindsight, it wasn't a good idea of mine 
to introduce them).

Depending on what we are trying to solve there are 3 possible approach:
   1. Leave advance_pc(). This means we could potentially
      a. Advance the PC twice (if it was already called) and therefore 
skipping an instruction
      b. Advance the PC once without an emulation
   2. Remove advance_pc(). If we already called the function, then there 
is no problem. Otherwise, we would trap in a loop effectively rendering 
the guest vCPU unusable.
   3. Replace with domain_crash()

Here it feels, that 3 is more suitable as this gives a clear indication 
why/where the emulation gone wrong.

This may still need to be accompanied with a ASSERT_UNREACHABLE() to 
please MISRA.

Bertrand, Michal, Stefano, what do you think?

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 5/7] xen/arm: traps: add ASSERT_UNREACHABLE() where needed

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 3916 bytes --]

On Thu, 14 Dec 2023, Julien Grall wrote:
> Hi,
> 
> On 13/12/2023 14:02, Nicola Vetrini wrote:
> > On 2023-12-12 16:49, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 11/12/2023 12:32, Julien Grall wrote:
> > > > Hi,
> > > > 
> > > > On 11/12/2023 10:30, Nicola Vetrini wrote:
> > > > > The branches of the switch after a call to 'do_unexpected_trap'
> > > > > cannot return, but there is one path that may return, hence
> > > > > only some clauses are marked with ASSERT_UNREACHABLE().
> > > > I don't understand why this is necessary. The code should never be
> > > > reachable because do_unexpected_trap() is a noreturn().
> > >
> > > From the matrix discussion, it wasn't clear what was my position on this
> > > patch.
> > > 
> > > I would much prefer if the breaks are kept. I could accept:
> > > 
> > > ASSERT_UNREACHABLE();
> > > break;
> > > 
> > > But this solution is a Nack because if you are concerned about functions
> > > like do_unexpected_trap() to return by mistaken, then it needs to also be
> > > safe in production.
> > > 
> > > The current proposal is not safe.

I re-read the email thread. I also do not think that this is useful:

         do_unexpected_trap("SVE trap at EL2", regs);
-        break;
+        ASSERT_UNREACHABLE();

I also do not think that we should be concerned about functions like
do_unexpected_trap() to return by mistaken.

That said, what is the problem from MISRA point of view that we are
trying to fix? Is the only problem the presence of break; after the call
to a noreturn function?

If that's not allowed, I would suggest to do this:


         do_unexpected_trap("SVE trap at EL2", regs);
-        break;
+        /* break; */


Or deviate "break" globally as it doesn't seem to be a safety risk in my
opinion. If nothing else, it should make the code a bit safer because in
case of mistakes in do_unexpected_trap, at least we would continue to
follow a more reasonable code path rather than blindly falling through
the next switch case by accident.


> > Ok. I wonder whether the should be applied here in vcpreg.c:
> > 
> > diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
> > index 39aeda9dab62..089d2f03eb5e 100644
> > --- a/xen/arch/arm/vcpreg.c
> > +++ b/xen/arch/arm/vcpreg.c
> > @@ -707,7 +707,8 @@ void do_cp10(struct cpu_user_regs *regs, const union hsr
> > hsr)
> >           inject_undef_exception(regs, hsr);
> >           return;
> >       }
> > -
> > +
> > +    ASSERT_UNREACHABLE();
> >       advance_pc(regs, hsr);
> >   }
> > 
> > the rationale being that, should the switch somehow fail to return, the
> > advance_pc would be called, rather than doing nothing.
> 
> To clarify, advance_pc(regs, hsr) would still be called in production build.
> So if you are concerned about advance_pc() been called, then adding an
> ASSERT_UNREACHABLE() is not going to help.
> 
> It took me a little while to confirm that none of the path effectively returns
> due to the macros (in hindsight, it wasn't a good idea of mine to introduce
> them).
> 
> Depending on what we are trying to solve there are 3 possible approach:
>   1. Leave advance_pc(). This means we could potentially
>      a. Advance the PC twice (if it was already called) and therefore skipping
> an instruction
>      b. Advance the PC once without an emulation
>   2. Remove advance_pc(). If we already called the function, then there is no
> problem. Otherwise, we would trap in a loop effectively rendering the guest
> vCPU unusable.
>   3. Replace with domain_crash()
> 
> Here it feels, that 3 is more suitable as this gives a clear indication
> why/where the emulation gone wrong.
> 
> This may still need to be accompanied with a ASSERT_UNREACHABLE() to please
> MISRA.
> 
> Bertrand, Michal, Stefano, what do you think?

Yes, I would go with 3., replace advance_pc with domain_crash. Assuming
that it would also solve the violation in ECLAIR.

Re: [XEN PATCH 5/7] xen/arm: traps: add ASSERT_UNREACHABLE() where needed

[Nicola Vetrini]

On 2023-12-14 23:32, Stefano Stabellini wrote:
> On Thu, 14 Dec 2023, Julien Grall wrote:
>> Hi,
>> 
>> On 13/12/2023 14:02, Nicola Vetrini wrote:
>> > On 2023-12-12 16:49, Julien Grall wrote:
>> > > Hi,
>> > >
>> > > On 11/12/2023 12:32, Julien Grall wrote:
>> > > > Hi,
>> > > >
>> > > > On 11/12/2023 10:30, Nicola Vetrini wrote:
>> > > > > The branches of the switch after a call to 'do_unexpected_trap'
>> > > > > cannot return, but there is one path that may return, hence
>> > > > > only some clauses are marked with ASSERT_UNREACHABLE().
>> > > > I don't understand why this is necessary. The code should never be
>> > > > reachable because do_unexpected_trap() is a noreturn().
>> > >
>> > > From the matrix discussion, it wasn't clear what was my position on this
>> > > patch.
>> > >
>> > > I would much prefer if the breaks are kept. I could accept:
>> > >
>> > > ASSERT_UNREACHABLE();
>> > > break;
>> > >
>> > > But this solution is a Nack because if you are concerned about functions
>> > > like do_unexpected_trap() to return by mistaken, then it needs to also be
>> > > safe in production.
>> > >
>> > > The current proposal is not safe.
> 
> I re-read the email thread. I also do not think that this is useful:
> 
>          do_unexpected_trap("SVE trap at EL2", regs);
> -        break;
> +        ASSERT_UNREACHABLE();
> 
> I also do not think that we should be concerned about functions like
> do_unexpected_trap() to return by mistaken.
> 
> That said, what is the problem from MISRA point of view that we are
> trying to fix? Is the only problem the presence of break; after the 
> call
> to a noreturn function?
> 
> If that's not allowed, I would suggest to do this:
> 
> 
>          do_unexpected_trap("SVE trap at EL2", regs);
> -        break;
> +        /* break; */
> 
> 
> Or deviate "break" globally as it doesn't seem to be a safety risk in 
> my
> opinion. If nothing else, it should make the code a bit safer because 
> in
> case of mistakes in do_unexpected_trap, at least we would continue to
> follow a more reasonable code path rather than blindly falling through
> the next switch case by accident.
> 
> 

That doesn't seem like a good idea to deviate just "break". However, 
Julien's earlier proposal

ASSERT_UNREACHABLE();
break;

is ok, though it could be shrunk in a macro

#define unreachable_break ASSERT_UNREACHABLE(); break;

or just

#define unreachable_break break;

so that "unreachable_break" can be deviated.

>> > Ok. I wonder whether the should be applied here in vcpreg.c:
>> >
>> > diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
>> > index 39aeda9dab62..089d2f03eb5e 100644
>> > --- a/xen/arch/arm/vcpreg.c
>> > +++ b/xen/arch/arm/vcpreg.c
>> > @@ -707,7 +707,8 @@ void do_cp10(struct cpu_user_regs *regs, const union hsr
>> > hsr)
>> >           inject_undef_exception(regs, hsr);
>> >           return;
>> >       }
>> > -
>> > +
>> > +    ASSERT_UNREACHABLE();
>> >       advance_pc(regs, hsr);
>> >   }
>> >
>> > the rationale being that, should the switch somehow fail to return, the
>> > advance_pc would be called, rather than doing nothing.
>> 
>> To clarify, advance_pc(regs, hsr) would still be called in production 
>> build.
>> So if you are concerned about advance_pc() been called, then adding an
>> ASSERT_UNREACHABLE() is not going to help.
>> 
>> It took me a little while to confirm that none of the path effectively 
>> returns
>> due to the macros (in hindsight, it wasn't a good idea of mine to 
>> introduce
>> them).
>> 
>> Depending on what we are trying to solve there are 3 possible 
>> approach:
>>   1. Leave advance_pc(). This means we could potentially
>>      a. Advance the PC twice (if it was already called) and therefore 
>> skipping
>> an instruction
>>      b. Advance the PC once without an emulation
>>   2. Remove advance_pc(). If we already called the function, then 
>> there is no
>> problem. Otherwise, we would trap in a loop effectively rendering the 
>> guest
>> vCPU unusable.
>>   3. Replace with domain_crash()
>> 
>> Here it feels, that 3 is more suitable as this gives a clear 
>> indication
>> why/where the emulation gone wrong.
>> 
>> This may still need to be accompanied with a ASSERT_UNREACHABLE() to 
>> please
>> MISRA.
>> 
>> Bertrand, Michal, Stefano, what do you think?
> 
> Yes, I would go with 3., replace advance_pc with domain_crash. Assuming
> that it would also solve the violation in ECLAIR.

It needs to be prefixed with an ASSERT_UNREACHABLE(), though, because 
it's still a violation if there is no execution path leading to 
domain_crash(), but other than that it seems the safest choice.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 5/7] xen/arm: traps: add ASSERT_UNREACHABLE() where needed

[Nicola Vetrini]

Hi Julien,

>> Yes, I would go with 3., replace advance_pc with domain_crash. 
>> Assuming
>> that it would also solve the violation in ECLAIR.
> 
> It needs to be prefixed with an ASSERT_UNREACHABLE(), though, because 
> it's still a violation if there is no execution path leading to 
> domain_crash(), but other than that it seems the safest choice.

Assuming there are no objections to going forward with this proposal, 
would you mind telling me how can I do the proper domain_crash call. 
Most of the examples get a "struct domain *" from a parameter or from 
the macro "current", so I was thinking of

domain_crash(current->domain);

but I'm not so sure about this, as there are no other uses in vcpreg.c. 
You can also submit the patch yourself, if you prefer.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 5/7] xen/arm: traps: add ASSERT_UNREACHABLE() where needed

[Julien Grall]

Hi,

On 15/12/2023 14:08, Nicola Vetrini wrote:
>>> Yes, I would go with 3., replace advance_pc with domain_crash. Assuming
>>> that it would also solve the violation in ECLAIR.
>>
>> It needs to be prefixed with an ASSERT_UNREACHABLE(), though, because 
>> it's still a violation if there is no execution path leading to 
>> domain_crash(), but other than that it seems the safest choice.
> 
> Assuming there are no objections to going forward with this proposal, 
> would you mind telling me how can I do the proper domain_crash call. 
> Most of the examples get a "struct domain *" from a parameter or from 
> the macro "current", so I was thinking of
> 
> domain_crash(current->domain);
> 
> but I'm not so sure about this, as there are no other uses in vcpreg.c. 

That would be correct. All the helpers in vcpreg.c are meant to work on 
the current vCPU.

> You can also submit the patch yourself, if you prefer.

I am not entirely sure about which justification you want to use for 
MISRA. So here the diff:

diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
index 39aeda9dab62..485b3cb63c86 100644
--- a/xen/arch/arm/vcpreg.c
+++ b/xen/arch/arm/vcpreg.c
@@ -708,7 +708,13 @@ void do_cp10(struct cpu_user_regs *regs, const 
union hsr hsr)
          return;
      }

-    advance_pc(regs, hsr);
+    /*
+     * All the cases in the switch should return. If this is not the
+     * case, then something went wrong and it is best to crash the
+     * domain.
+     */
+    ASSERT_UNREACHABLE();
+    domain_crash(current->domain);
  }

  void do_cp(struct cpu_user_regs *regs, const union hsr hsr)

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 5/7] xen/arm: traps: add ASSERT_UNREACHABLE() where needed

[Stefano Stabellini]

On Fri, 15 Dec 2023, Nicola Vetrini wrote:
> On 2023-12-14 23:32, Stefano Stabellini wrote:
> > On Thu, 14 Dec 2023, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 13/12/2023 14:02, Nicola Vetrini wrote:
> > > > On 2023-12-12 16:49, Julien Grall wrote:
> > > > > Hi,
> > > > >
> > > > > On 11/12/2023 12:32, Julien Grall wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On 11/12/2023 10:30, Nicola Vetrini wrote:
> > > > > > > The branches of the switch after a call to 'do_unexpected_trap'
> > > > > > > cannot return, but there is one path that may return, hence
> > > > > > > only some clauses are marked with ASSERT_UNREACHABLE().
> > > > > > I don't understand why this is necessary. The code should never be
> > > > > > reachable because do_unexpected_trap() is a noreturn().
> > > > >
> > > > > From the matrix discussion, it wasn't clear what was my position on
> > > this
> > > > > patch.
> > > > >
> > > > > I would much prefer if the breaks are kept. I could accept:
> > > > >
> > > > > ASSERT_UNREACHABLE();
> > > > > break;
> > > > >
> > > > > But this solution is a Nack because if you are concerned about
> > > functions
> > > > > like do_unexpected_trap() to return by mistaken, then it needs to also
> > > be
> > > > > safe in production.
> > > > >
> > > > > The current proposal is not safe.
> > 
> > I re-read the email thread. I also do not think that this is useful:
> > 
> >          do_unexpected_trap("SVE trap at EL2", regs);
> > -        break;
> > +        ASSERT_UNREACHABLE();
> > 
> > I also do not think that we should be concerned about functions like
> > do_unexpected_trap() to return by mistaken.
> > 
> > That said, what is the problem from MISRA point of view that we are
> > trying to fix? Is the only problem the presence of break; after the call
> > to a noreturn function?
> > 
> > If that's not allowed, I would suggest to do this:
> > 
> > 
> >          do_unexpected_trap("SVE trap at EL2", regs);
> > -        break;
> > +        /* break; */
> > 
> > 
> > Or deviate "break" globally as it doesn't seem to be a safety risk in my
> > opinion. If nothing else, it should make the code a bit safer because in
> > case of mistakes in do_unexpected_trap, at least we would continue to
> > follow a more reasonable code path rather than blindly falling through
> > the next switch case by accident.
> > 
> > 
> 
> That doesn't seem like a good idea to deviate just "break". However, Julien's
> earlier proposal
> 
> ASSERT_UNREACHABLE();
> break;
> 
> is ok, though it could be shrunk in a macro
> 
> #define unreachable_break ASSERT_UNREACHABLE(); break;
> 
> or just
> 
> #define unreachable_break break;
> 
> so that "unreachable_break" can be deviated.

Let's just go with:

ASSERT_UNREACHABLE();
break;

If Julien is OK with it too.


Just to make sure we are all aligned on what the problem and solution
are:
- we are not concerned that do_unexpected_trap could return
- it is just forbidden to have any code after something that doesn't
  return
- not even a break;
- so we need to use ASSERT_UNREACHABLE() as a marker to tell ECLAIR to
  ignore the violation