diff mbox series

[for-4.19] xen/arm64: domctl: Avoid unreachable code in subarch_do_domctl()

Message ID 20231023175220.42781-1-julien@xen.org (mailing list archive)
State New, archived
Headers show
Series [for-4.19] xen/arm64: domctl: Avoid unreachable code in subarch_do_domctl() | expand

Commit Message

Julien Grall Oct. 23, 2023, 5:52 p.m. UTC 
From: Julien Grall <jgrall@amazon.com>

The 'break' the XEN_DOMCTL_set_address_size is unreachable and tools
like Eclair will report as a violation of Misra Rule 2.1.

Furthermore, the nested switch is not very easy to read. So move
out the nested switch in a separate function to improve the
readability and hopefully address the MISRA violation.

Reported-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>

---

Only compiled tested. Waiting for the CI to confirm there is no
regression.
---
 xen/arch/arm/arm64/domctl.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

Comments

Henry Wang Oct. 24, 2023, 4:13 a.m. UTC | #1 
Hi Julien,

(+Stefano and Bertrand)

> On Oct 24, 2023, at 01:52, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> The 'break' the XEN_DOMCTL_set_address_size is unreachable and tools
> like Eclair will report as a violation of Misra Rule 2.1.
> 
> Furthermore, the nested switch is not very easy to read. So move
> out the nested switch in a separate function to improve the
> readability and hopefully address the MISRA violation.
> 
> Reported-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Henry Wang <Henry.Wang@arm.com>

> ---
> Only compiled tested. Waiting for the CI to confirm there is no
> regression.

I also tested this patch on top of today’s staging in Arm’s internal CI, and this
patch looks good.

Tested-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry
Julien Grall Oct. 24, 2023, 10:14 a.m. UTC | #2 
Hi,

I forgot to CC the maintainers.

On 23/10/2023 18:52, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The 'break' the XEN_DOMCTL_set_address_size is unreachable and tools
> like Eclair will report as a violation of Misra Rule 2.1.
> 
> Furthermore, the nested switch is not very easy to read. So move
> out the nested switch in a separate function to improve the
> readability and hopefully address the MISRA violation.
> 
> Reported-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ---
> 
> Only compiled tested. Waiting for the CI to confirm there is no
> regression.
> ---
>   xen/arch/arm/arm64/domctl.c | 34 +++++++++++++++++++---------------
>   1 file changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/domctl.c b/xen/arch/arm/arm64/domctl.c
> index 14fc622e9956..8720d126c97d 100644
> --- a/xen/arch/arm/arm64/domctl.c
> +++ b/xen/arch/arm/arm64/domctl.c
> @@ -33,27 +33,31 @@ static long switch_mode(struct domain *d, enum domain_type type)
>       return 0;
>   }
>   
> +static long set_address_size(struct domain *d, uint32_t address_size)
> +{
> +    switch ( address_size )
> +    {
> +    case 32:
> +        if ( !cpu_has_el1_32 )
> +            return -EINVAL;
> +        /* SVE is not supported for 32 bit domain */
> +        if ( is_sve_domain(d) )
> +            return -EINVAL;
> +        return switch_mode(d, DOMAIN_32BIT);
> +    case 64:
> +        return switch_mode(d, DOMAIN_64BIT);
> +    default:
> +        return -EINVAL;
> +    }
> +}
> +
>   long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>                          XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>   {
>       switch ( domctl->cmd )
>       {
>       case XEN_DOMCTL_set_address_size:
> -        switch ( domctl->u.address_size.size )
> -        {
> -        case 32:
> -            if ( !cpu_has_el1_32 )
> -                return -EINVAL;
> -            /* SVE is not supported for 32 bit domain */
> -            if ( is_sve_domain(d) )
> -                return -EINVAL;
> -            return switch_mode(d, DOMAIN_32BIT);
> -        case 64:
> -            return switch_mode(d, DOMAIN_64BIT);
> -        default:
> -            return -EINVAL;
> -        }
> -        break;
> +        return set_address_size(d, domctl->u.address_size.size);
>   
>       default:
>           return -ENOSYS;
Bertrand Marquis Oct. 24, 2023, 1:33 p.m. UTC | #3 
Hi Julien,

> On 23 Oct 2023, at 19:52, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> The 'break' the XEN_DOMCTL_set_address_size is unreachable and tools
> like Eclair will report as a violation of Misra Rule 2.1.
> 
> Furthermore, the nested switch is not very easy to read. So move
> out the nested switch in a separate function to improve the
> readability and hopefully address the MISRA violation.
> 
> Reported-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> 
> ---
> 
> Only compiled tested. Waiting for the CI to confirm there is no
> regression.
> ---
> xen/arch/arm/arm64/domctl.c | 34 +++++++++++++++++++---------------
> 1 file changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/domctl.c b/xen/arch/arm/arm64/domctl.c
> index 14fc622e9956..8720d126c97d 100644
> --- a/xen/arch/arm/arm64/domctl.c
> +++ b/xen/arch/arm/arm64/domctl.c
> @@ -33,27 +33,31 @@ static long switch_mode(struct domain *d, enum domain_type type)
>     return 0;
> }
> 
> +static long set_address_size(struct domain *d, uint32_t address_size)
> +{
> +    switch ( address_size )
> +    {
> +    case 32:
> +        if ( !cpu_has_el1_32 )
> +            return -EINVAL;
> +        /* SVE is not supported for 32 bit domain */
> +        if ( is_sve_domain(d) )
> +            return -EINVAL;
> +        return switch_mode(d, DOMAIN_32BIT);
> +    case 64:
> +        return switch_mode(d, DOMAIN_64BIT);
> +    default:
> +        return -EINVAL;
> +    }
> +}
> +
> long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>                        XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> {
>     switch ( domctl->cmd )
>     {
>     case XEN_DOMCTL_set_address_size:
> -        switch ( domctl->u.address_size.size )
> -        {
> -        case 32:
> -            if ( !cpu_has_el1_32 )
> -                return -EINVAL;
> -            /* SVE is not supported for 32 bit domain */
> -            if ( is_sve_domain(d) )
> -                return -EINVAL;
> -            return switch_mode(d, DOMAIN_32BIT);
> -        case 64:
> -            return switch_mode(d, DOMAIN_64BIT);
> -        default:
> -            return -EINVAL;
> -        }
> -        break;
> +        return set_address_size(d, domctl->u.address_size.size);
> 
>     default:
>         return -ENOSYS;
> -- 
> 2.40.1
> 
>
Stefano Stabellini Oct. 24, 2023, 7:37 p.m. UTC | #4 
On Tue, 24 Oct 2023, Bertrand Marquis wrote:
> Hi Julien,
> 
> > On 23 Oct 2023, at 19:52, Julien Grall <julien@xen.org> wrote:
> > 
> > From: Julien Grall <jgrall@amazon.com>
> > 
> > The 'break' the XEN_DOMCTL_set_address_size is unreachable and tools
> > like Eclair will report as a violation of Misra Rule 2.1.
> > 
> > Furthermore, the nested switch is not very easy to read. So move
> > out the nested switch in a separate function to improve the
> > readability and hopefully address the MISRA violation.
> > 
> > Reported-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> > Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

I added the patch to my for-4.19 branch
diff mbox series

Patch

diff --git a/xen/arch/arm/arm64/domctl.c b/xen/arch/arm/arm64/domctl.c
index 14fc622e9956..8720d126c97d 100644
--- a/xen/arch/arm/arm64/domctl.c
+++ b/xen/arch/arm/arm64/domctl.c
@@ -33,27 +33,31 @@  static long switch_mode(struct domain *d, enum domain_type type)
     return 0;
 }
 
+static long set_address_size(struct domain *d, uint32_t address_size)
+{
+    switch ( address_size )
+    {
+    case 32:
+        if ( !cpu_has_el1_32 )
+            return -EINVAL;
+        /* SVE is not supported for 32 bit domain */
+        if ( is_sve_domain(d) )
+            return -EINVAL;
+        return switch_mode(d, DOMAIN_32BIT);
+    case 64:
+        return switch_mode(d, DOMAIN_64BIT);
+    default:
+        return -EINVAL;
+    }
+}
+
 long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
                        XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
 {
     switch ( domctl->cmd )
     {
     case XEN_DOMCTL_set_address_size:
-        switch ( domctl->u.address_size.size )
-        {
-        case 32:
-            if ( !cpu_has_el1_32 )
-                return -EINVAL;
-            /* SVE is not supported for 32 bit domain */
-            if ( is_sve_domain(d) )
-                return -EINVAL;
-            return switch_mode(d, DOMAIN_32BIT);
-        case 64:
-            return switch_mode(d, DOMAIN_64BIT);
-        default:
-            return -EINVAL;
-        }
-        break;
+        return set_address_size(d, domctl->u.address_size.size);
 
     default:
         return -ENOSYS;