| Message ID | a20efca7042ea8f351516ea521edccd89b475929.1719218291.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 24.06.2024 11:04, Federico Serafini wrote: > --- 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(); > } This or very similar comment are replaced elsewhere in this patch. I'm sure we have more of them. Hence an alternative would be to deviate those variations of what we already deviate. I recall there was a mail from Julien asking to avoid extending the set, unless some forms are used pretty frequently. Sadly nothing towards judgement between the alternatives is said in the description. > @@ -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 */ What about this? It doesn't match anything I see in deviations.rst. > default: > hvm_emulate_writeback(&ctxt); > + break; > } > > return rc; > @@ -2799,10 +2801,11 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr, > memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data, > hvio->mmio_insn_bytes); > } > - /* Fall-through */ > + fallthrough; > default: > ctx.set_context = (kind == EMUL_KIND_SET_CONTEXT_DATA); > rc = hvm_emulate_one(&ctx, VIO_no_completion); > + break; > } While not as much of a problem for the comment, I view a statement like this as mis-indented. > @@ -5283,6 +5287,8 @@ 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; Why the extra blank line here, ... > --- 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 && ... when e.g. here there's none? I'm afraid this goes back to an unfinished discussion as to whether to have blank lines between blocks in fall-through situations. My view is that not having them in these cases is helping to make the falling through visually noticeable. Jan
On Mon, 24 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> > --- > Changes in v2: > - replace hypened fallthrough with the pseudo keyword. > --- > xen/arch/x86/hvm/emulate.c | 9 ++++++--- > xen/arch/x86/hvm/hvm.c | 6 ++++++ > xen/arch/x86/hvm/hypercall.c | 1 + > xen/arch/x86/hvm/irq.c | 1 + > 4 files changed, 14 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c > index 02e378365b..859c21a5ab 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; same here > } > > 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; > @@ -2799,10 +2801,11 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr, > memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data, > hvio->mmio_insn_bytes); > } > - /* Fall-through */ > + fallthrough; > 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..c263e562ff 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; same here > } > > out: > @@ -5020,6 +5022,8 @@ static int compat_altp2m_op( > > default: > ASSERT_UNREACHABLE(); > + rc = -EOPNOTSUPP; > + break; same here > } > > return rc; > @@ -5283,6 +5287,8 @@ 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; > } > -- > 2.34.1 > >
On 24/06/24 17:32, Jan Beulich wrote: > On 24.06.2024 11:04, Federico Serafini wrote: >> --- 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(); >> } > > This or very similar comment are replaced elsewhere in this patch. I'm > sure we have more of them. Hence an alternative would be to deviate those > variations of what we already deviate. I recall there was a mail from > Julien asking to avoid extending the set, unless some forms are used > pretty frequently. Sadly nothing towards judgement between the > alternatives is said in the description. I found few occurrences of the hypened fallthrough, It doesn't seem like a very used form to me, and most of them are in emulate.c, a file I needed to touch anyway. The fact that the pseudo keyword is the one preferred is mentioned in deviations.rst, but I can mention this also in the description. >> @@ -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 */ > > What about this? It doesn't match anything I see in deviations.rst. The last item for R16.3 in deviations.rst explicitly says that existing comment of this form are considered as safe (i.e., deviated) but deprecated, meaning that the pseudo keyword should be used for new cases. We can consider a rephrasing if it is not clear enough. > >> default: >> hvm_emulate_writeback(&ctxt); >> + break; >> } >> >> return rc; >> @@ -2799,10 +2801,11 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr, >> memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data, >> hvio->mmio_insn_bytes); >> } >> - /* Fall-through */ >> + fallthrough; >> default: >> ctx.set_context = (kind == EMUL_KIND_SET_CONTEXT_DATA); >> rc = hvm_emulate_one(&ctx, VIO_no_completion); >> + break; >> } > > While not as much of a problem for the comment, I view a statement like > this as mis-indented. > >> @@ -5283,6 +5287,8 @@ 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; > > Why the extra blank line here, ... > >> --- 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 && > > ... when e.g. here there's none? I'm afraid this goes back to an > unfinished discussion as to whether to have blank lines between blocks > in fall-through situations. My view is that not having them in these > cases is helping to make the falling through visually noticeable. I looked ad the context to preserve the style of the existing cases. What do you think about: -keep the existing style when a break needs to be inserted; -no blank line if a fallthrough needs to inserted.
On 25.06.2024 09:21, Federico Serafini wrote: > On 24/06/24 17:32, Jan Beulich wrote: >> On 24.06.2024 11:04, Federico Serafini wrote: >>> @@ -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 */ >> >> What about this? It doesn't match anything I see in deviations.rst. > > The last item for R16.3 in deviations.rst explicitly says that > existing comment of this form are considered as safe (i.e., deviated) > but deprecated, meaning that the pseudo keyword should be used for new > cases. We can consider a rephrasing if it is not clear enough. Apologies. I mistakenly looked at grep output rather than actual file contents. Please disregard this comment of mine. >>> @@ -5283,6 +5287,8 @@ 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; >> >> Why the extra blank line here, ... >> >>> --- 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 && >> >> ... when e.g. here there's none? I'm afraid this goes back to an >> unfinished discussion as to whether to have blank lines between blocks >> in fall-through situations. My view is that not having them in these >> cases is helping to make the falling through visually noticeable. > > I looked ad the context to preserve the style > of the existing cases. > > What do you think about: > -keep the existing style when a break needs to be inserted; Even that may be a judgment call, I'd say. But commonly: Yes. > -no blank line if a fallthrough needs to inserted. Yes here, but others (Andrew?) may disagree with me. Jan
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index 02e378365b..859c21a5ab 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; @@ -2799,10 +2801,11 @@ void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr, memcpy(hvio->mmio_insn, curr->arch.vm_event->emul.insn.data, hvio->mmio_insn_bytes); } - /* Fall-through */ + fallthrough; 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..c263e562ff 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,8 @@ 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; }
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 v2: - replace hypened fallthrough with the pseudo keyword. --- xen/arch/x86/hvm/emulate.c | 9 ++++++--- xen/arch/x86/hvm/hvm.c | 6 ++++++ xen/arch/x86/hvm/hypercall.c | 1 + xen/arch/x86/hvm/irq.c | 1 + 4 files changed, 14 insertions(+), 3 deletions(-)