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
