| Message ID | 87cfe4d3e75c3a7d4174393a31aaaf80e0e60633.1719383180.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: address some violations of MISRA C Rule 16.3 | expand |
On Wed, 26 Jun 2024, Federico Serafini wrote: > MISRA C Rule 16.3 states that "An unconditional `break' statement shall > terminate every switch-clause". > > Add pseudo keyword fallthrough or missing break statement > to address violations of the rule. > > As a defensive measure, return -EOPNOTSUPP in case an unreachable > return statement is reached. > > Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> Aside from the ASSERT_UNREACHABLE which is still under discussion: Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
On 26.06.2024 11:28, Federico Serafini wrote: > @@ -2798,11 +2800,12 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr, > hvio->mmio_insn_bytes = sizeof(hvio->mmio_insn); > memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data, > hvio->mmio_insn_bytes); > + fallthrough; > } > - /* Fall-through */ > default: Can you clarify for me please whether this arrangement actually helps? I'm pretty sure it'll result in a Coverity complaint, as my understanding is that for them the marker (comment or pseudo-keyword) has to immediately precede the subsequent label. IOW even if you confirmed that Eclair is smarter in this regard, it may still need converting to hvio->mmio_insn_bytes = sizeof(hvio->mmio_insn); memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data, hvio->mmio_insn_bytes); } fallthrough; default: > --- a/xen/arch/x86/hvm/hypercall.c > +++ b/xen/arch/x86/hvm/hypercall.c > @@ -111,6 +111,7 @@ int hvm_hypercall(struct cpu_user_regs *regs) > case 8: > eax = regs->rax; > /* Fallthrough to permission check. */ > + fallthrough; > case 4: > case 2: > if ( currd->arch.monitor.guest_request_userspace_enabled && Arguably the comment could then be dropped in exchange. Yet I won't insist on you doing so (and others may also disagree). Jan
On 01/07/24 10:47, Jan Beulich wrote: > On 26.06.2024 11:28, Federico Serafini wrote: >> @@ -2798,11 +2800,12 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr, >> hvio->mmio_insn_bytes = sizeof(hvio->mmio_insn); >> memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data, >> hvio->mmio_insn_bytes); >> + fallthrough; >> } >> - /* Fall-through */ >> default: > > Can you clarify for me please whether this arrangement actually helps? > I'm pretty sure it'll result in a Coverity complaint, as my understanding > is that for them the marker (comment or pseudo-keyword) has to immediately > precede the subsequent label. IOW even if you confirmed that Eclair is > smarter in this regard, it may still need converting to > > hvio->mmio_insn_bytes = sizeof(hvio->mmio_insn); > memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data, > hvio->mmio_insn_bytes); > } > fallthrough; > default: > Yes, this is ok for ECLAIR.
On Tue, 2 Jul 2024, Federico Serafini wrote: > On 01/07/24 10:47, Jan Beulich wrote: > > On 26.06.2024 11:28, Federico Serafini wrote: > > > @@ -2798,11 +2800,12 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, > > > unsigned int trapnr, > > > hvio->mmio_insn_bytes = sizeof(hvio->mmio_insn); > > > memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data, > > > hvio->mmio_insn_bytes); > > > + fallthrough; > > > } > > > - /* Fall-through */ > > > default: > > > > Can you clarify for me please whether this arrangement actually helps? > > I'm pretty sure it'll result in a Coverity complaint, as my understanding > > is that for them the marker (comment or pseudo-keyword) has to immediately > > precede the subsequent label. IOW even if you confirmed that Eclair is > > smarter in this regard, it may still need converting to > > > > hvio->mmio_insn_bytes = sizeof(hvio->mmio_insn); > > memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data, > > hvio->mmio_insn_bytes); > > } > > fallthrough; > > default: > > > > Yes, this is ok for ECLAIR. Given that Jan might be right that Coverity and others would prefer the keyword on the line immediately above "default", and given that it works anyway for ECLAIR, then I think it would be better to stay on the safe side and move the "fallback" right on top of default. If you are OK with it, please resend this patch and following patches. Patches 1-6 are fully acked and I'd be happy to take them in my for-4.20 branch.
On 13/07/24 01:25, Stefano Stabellini wrote: > On Tue, 2 Jul 2024, Federico Serafini wrote: >> On 01/07/24 10:47, Jan Beulich wrote: >>> On 26.06.2024 11:28, Federico Serafini wrote: >>>> @@ -2798,11 +2800,12 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, >>>> unsigned int trapnr, >>>> hvio->mmio_insn_bytes = sizeof(hvio->mmio_insn); >>>> memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data, >>>> hvio->mmio_insn_bytes); >>>> + fallthrough; >>>> } >>>> - /* Fall-through */ >>>> default: >>> >>> Can you clarify for me please whether this arrangement actually helps? >>> I'm pretty sure it'll result in a Coverity complaint, as my understanding >>> is that for them the marker (comment or pseudo-keyword) has to immediately >>> precede the subsequent label. IOW even if you confirmed that Eclair is >>> smarter in this regard, it may still need converting to >>> >>> hvio->mmio_insn_bytes = sizeof(hvio->mmio_insn); >>> memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data, >>> hvio->mmio_insn_bytes); >>> } >>> fallthrough; >>> default: >>> >> >> Yes, this is ok for ECLAIR. > > Given that Jan might be right that Coverity and others would prefer the > keyword on the line immediately above "default", and given that it works > anyway for ECLAIR, then I think it would be better to stay on the safe > side and move the "fallback" right on top of default. > > If you are OK with it, please resend this patch and following patches. > Patches 1-6 are fully acked and I'd be happy to take them in my for-4.20 > branch. V4 sent: https://lists.xenproject.org/archives/html/xen-devel/2024-07/msg00823.html
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index 02e378365b..f5dd08f510 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -339,7 +339,7 @@ static int hvmemul_do_io( } case X86EMUL_UNIMPLEMENTED: ASSERT_UNREACHABLE(); - /* Fall-through */ + fallthrough; default: BUG(); } @@ -2674,6 +2674,7 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt, default: ASSERT_UNREACHABLE(); + break; } if ( hvmemul_ctxt->ctxt.retire.singlestep ) @@ -2764,6 +2765,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla) /* fallthrough */ default: hvm_emulate_writeback(&ctxt); + break; } return rc; @@ -2798,11 +2800,12 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr, hvio->mmio_insn_bytes = sizeof(hvio->mmio_insn); memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data, hvio->mmio_insn_bytes); + fallthrough; } - /* Fall-through */ default: ctx.set_context = (kind == EMUL_KIND_SET_CONTEXT_DATA); rc = hvm_emulate_one(&ctx, VIO_no_completion); + break; } switch ( rc ) @@ -2818,7 +2821,7 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr, case X86EMUL_UNIMPLEMENTED: if ( hvm_monitor_emul_unimplemented() ) return; - /* fall-through */ + fallthrough; case X86EMUL_UNHANDLEABLE: hvm_dump_emulation_state(XENLOG_G_DEBUG, "Mem event", &ctx, rc); hvm_inject_hw_exception(trapnr, errcode); diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 7f4b627b1f..d7f195ba9a 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -4919,6 +4919,8 @@ static int do_altp2m_op( default: ASSERT_UNREACHABLE(); + rc = -EOPNOTSUPP; + break; } out: @@ -5020,6 +5022,8 @@ static int compat_altp2m_op( default: ASSERT_UNREACHABLE(); + rc = -EOPNOTSUPP; + break; } return rc; @@ -5283,6 +5287,7 @@ void hvm_get_segment_register(struct vcpu *v, enum x86_segment seg, * %cs and %tr are unconditionally present. SVM ignores these present * bits and will happily run without them set. */ + fallthrough; case x86_seg_cs: reg->p = 1; break; diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c index 7fb3136f0c..2271afe02a 100644 --- a/xen/arch/x86/hvm/hypercall.c +++ b/xen/arch/x86/hvm/hypercall.c @@ -111,6 +111,7 @@ int hvm_hypercall(struct cpu_user_regs *regs) case 8: eax = regs->rax; /* Fallthrough to permission check. */ + fallthrough; case 4: case 2: if ( currd->arch.monitor.guest_request_userspace_enabled && diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c index 210cebb0e6..1eab44defc 100644 --- a/xen/arch/x86/hvm/irq.c +++ b/xen/arch/x86/hvm/irq.c @@ -282,6 +282,7 @@ static void hvm_set_callback_irq_level(struct vcpu *v) __hvm_pci_intx_assert(d, pdev, pintx); else __hvm_pci_intx_deassert(d, pdev, pintx); + break; default: break; } diff --git a/xen/arch/x86/hvm/pmtimer.c b/xen/arch/x86/hvm/pmtimer.c index 97099ac305..87a7a01c9f 100644 --- a/xen/arch/x86/hvm/pmtimer.c +++ b/xen/arch/x86/hvm/pmtimer.c @@ -185,6 +185,7 @@ static int cf_check handle_evt_io( gdprintk(XENLOG_WARNING, "Bad ACPI PM register write: %x bytes (%x) at %x\n", bytes, *val, port); + break; } } /* Fix up the SCI state to match the new register state */
MISRA C Rule 16.3 states that "An unconditional `break' statement shall terminate every switch-clause". Add pseudo keyword fallthrough or missing break statement to address violations of the rule. As a defensive measure, return -EOPNOTSUPP in case an unreachable return statement is reached. Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> --- Changes in v3: - squashed here modifications of pmtimer.c; - no blank line after fallthrough; - better indentation of fallthrough. --- xen/arch/x86/hvm/emulate.c | 9 ++++++--- xen/arch/x86/hvm/hvm.c | 5 +++++ xen/arch/x86/hvm/hypercall.c | 1 + xen/arch/x86/hvm/irq.c | 1 + xen/arch/x86/hvm/pmtimer.c | 1 + 5 files changed, 14 insertions(+), 3 deletions(-)