diff mbox series

[XEN,v2,6/7] xen/arm: vcpreg: address violation of MISRA C Rule 2.1

Message ID 9816362a11aeb7b9618500dea9bbf32e4b5483a9.1702891792.git.nicola.vetrini@bugseng.com (mailing list archive)
State New
Headers show
Series address violations of MISRA C:2012 Rule 2.1 | expand

Commit Message

Nicola Vetrini Dec. 18, 2023, 10:17 a.m. UTC 
There is no path that reaches the call to 'advance_pc', thus violating MISRA C
Rule 2.1.
A call to ASSERT_UNREACHABLE() is added after the switch, despite this being
useful to detect errors only in debug builds; if that marker is ever reached,
a domain crash is triggered, as a defensive coding measure.

No functional change.

Signed-off-by: Julien Grall <julien@xen.org>
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
The code changes (including the comment) were made by Julien in [1]; I added the
commit text and all other informations.

All the switch clauses, when expanded, end with a return statement
and the default clause has an unconditional return, therefore
advance_pc() is never reached.

However, it has been deemed safer to crash the domain if the switch is ever
exited.

[1] https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2312151232580.3175268@ubuntu-linux-20-04-desktop/T/#maa91d8025532455a6317119a1e4affa00a99e1ce
---
 xen/arch/arm/vcpreg.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Stefano Stabellini Dec. 19, 2023, 1:39 a.m. UTC | #1 
On Mon, 18 Dec 2023, Nicola Vetrini wrote:
> There is no path that reaches the call to 'advance_pc', thus violating MISRA C
> Rule 2.1.
> A call to ASSERT_UNREACHABLE() is added after the switch, despite this being
> useful to detect errors only in debug builds; if that marker is ever reached,
> a domain crash is triggered, as a defensive coding measure.
> 
> No functional change.
> 
> Signed-off-by: Julien Grall <julien@xen.org>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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


> ---
> The code changes (including the comment) were made by Julien in [1]; I added the
> commit text and all other informations.
> 
> All the switch clauses, when expanded, end with a return statement
> and the default clause has an unconditional return, therefore
> advance_pc() is never reached.
> 
> However, it has been deemed safer to crash the domain if the switch is ever
> exited.
> 
> [1] https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2312151232580.3175268@ubuntu-linux-20-04-desktop/T/#maa91d8025532455a6317119a1e4affa00a99e1ce
> ---
>  xen/arch/arm/vcpreg.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
> index 39aeda9dab62..a2d050070473 100644
> --- a/xen/arch/arm/vcpreg.c
> +++ b/xen/arch/arm/vcpreg.c
> @@ -707,8 +707,14 @@ void do_cp10(struct cpu_user_regs *regs, const union hsr hsr)
>          inject_undef_exception(regs, 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)
> -- 
> 2.34.1
>
diff mbox series

Patch

diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
index 39aeda9dab62..a2d050070473 100644
--- a/xen/arch/arm/vcpreg.c
+++ b/xen/arch/arm/vcpreg.c
@@ -707,8 +707,14 @@  void do_cp10(struct cpu_user_regs *regs, const union hsr hsr)
         inject_undef_exception(regs, 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)