Re: Refactor arm64/domctl.c 'subarch_do_domctl' to avoid unreachable break.

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 23/10/2023 16:15, Jan Beulich wrote:
> On 23.10.2023 17:00, Julien Grall wrote:
> > On 23/10/2023 15:51, Nicola Vetrini wrote:
> > > Hi,
> > Hi Nicola,
> >
> > > while taking care of some patches regarding MISRA C Rule 2.1 (code
> > > shouldn't be unreachable), I
> > > came across this function:
> > >
> > > 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;
> > >
> > >       default:
> > >           return -ENOSYS;
> > >       }
> > > }
> > >
> > > here the break after the innermost switch is clearly unreachable, but
> > > it's also guarding a possible fallthrough.
> > > I can see a couple of solutions to this:
> > >
> > > - mark the part after the switch unreachable;
> > > - introduce a variable 'long rc' to store the return value, and
> > > consequently rework the control flow of all the switches
> > >     (e.g. rc = -EINVAL and similar);
> > > - remove the break, but I consider this a risky move, unless -ENOSYS
> > > would be an ok value to be returned if some case
> > >     from the switch above does not have a return statement.
> > - move the nested switch in a separate function, so the code in
> > subarch_do_domctl() can be replaced with:
> >
> > return set_address_size(...);
> But that would help only if inside the new function you still re-
> layout the switch() (or replace it by, say, if/else-if/else),
> wouldn't it?
I am not sure why I would need to re-layout the switch. This should work 
(untested):
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;

Cheers,

--
Julien Grall