Re: [PATCH 4/5] x86: Fix missing breaks

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

On 2025-12-10 19:30, Andrew Cooper wrote:
> With the wider testing, some more violations have been spotted.  This
> addresses violations of Rule 16.3 which requires all case statements to 
> be
> terminated with a break or other unconditional control flow change.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

with one nit below

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: consulting@bugseng.com <consulting@bugseng.com>
> CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>  xen/arch/x86/domain.c          | 1 +
>  xen/arch/x86/mm/shadow/hvm.c   | 1 +
>  xen/arch/x86/pv/emul-priv-op.c | 1 +
>  xen/arch/x86/pv/emulate.c      | 1 +
>  xen/common/livepatch.c         | 1 -
>  xen/common/livepatch_elf.c     | 1 +
>  6 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 5e37bfbd17d6..b15120180993 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1517,6 +1517,7 @@ int arch_set_info_guest(
>          {
>          case -EINTR:
>              rc = -ERESTART;
> +            fallthrough;
>          case -ERESTART:
>              break;
>          case 0:
> diff --git a/xen/arch/x86/mm/shadow/hvm.c 
> b/xen/arch/x86/mm/shadow/hvm.c
> index 114957a3e1ec..69334c095608 100644
> --- a/xen/arch/x86/mm/shadow/hvm.c
> +++ b/xen/arch/x86/mm/shadow/hvm.c
> @@ -268,6 +268,7 @@ hvm_emulate_cmpxchg(enum x86_segment seg,
>      default:
>          SHADOW_PRINTK("cmpxchg size %u is not supported\n", bytes);
>          prev = ~old;
> +        break;
>      }
> 
>      if ( prev != old )
> diff --git a/xen/arch/x86/pv/emul-priv-op.c 
> b/xen/arch/x86/pv/emul-priv-op.c
> index 08dec9990e39..fb6d57d6fbd3 100644
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -407,6 +407,7 @@ static void _guest_io_write(unsigned int port, 
> unsigned int bytes,
> 
>      default:
>          ASSERT_UNREACHABLE();
> +        break;
>      }
>  }
> 
> diff --git a/xen/arch/x86/pv/emulate.c b/xen/arch/x86/pv/emulate.c
> index 8c44dea12330..b201ea1c6a97 100644
> --- a/xen/arch/x86/pv/emulate.c
> +++ b/xen/arch/x86/pv/emulate.c
> @@ -120,6 +120,7 @@ void pv_set_reg(struct vcpu *v, unsigned int reg, 
> uint64_t val)
>          printk(XENLOG_G_ERR "%s(%pv, 0x%08x, 0x%016"PRIx64") Bad 
> register\n",
>                 __func__, v, reg, val);
>          domain_crash(d);
> +        break;
>      }
>  }
> 
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 9285f88644f4..b39f8d7bfe20 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -1924,7 +1924,6 @@ static void noinline do_livepatch_work(void)
>                              p->name);
>                      ASSERT_UNREACHABLE();
>                  }
> -            default:
>                  break;

could I talk you into changing to

   fallthrough;
default:
   break;

while it's clear that the meaning is the same, the LIVEPATCH_ACTION_* 
constants being switched on are not in an enum, which is made more 
obvious by the presence of a default clause.

>              }
>          }
> diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
> index 25ce1bd5a0ad..2e82f2cb8c46 100644
> --- a/xen/common/livepatch_elf.c
> +++ b/xen/common/livepatch_elf.c
> @@ -347,6 +347,7 @@ int livepatch_elf_resolve_symbols(struct 
> livepatch_elf *elf)
>                  dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Symbol resolved: 
> %s => %#"PRIxElfAddr" (%s)\n",
>                         elf->name, elf->sym[i].name,
>                         st_value, elf->sec[idx].name);
> +            break;
>          }
> 
>          if ( rc )

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253