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
