On Fri, Mar 15, 2019 at 10:14:47AM +0100, Vitaly Kuznetsov wrote: > "Li,Rongqing" <lirongqing@xxxxxxxxx> writes: > > >> -----邮件原件----- > >> 发件人: Vitaly Kuznetsov [mailto:vkuznets@xxxxxxxxxx] > >> 发送时间: 2019年3月14日 21:23 > >> 收件人: Li,Rongqing <lirongqing@xxxxxxxxx> > >> 抄送: x86@xxxxxxxxxx; kvm@xxxxxxxxxxxxxxx > >> 主题: Re: [PATCH] KVM: svm: merge incomplete IPI emulation handling > >> > >> Li RongQing <lirongqing@xxxxxxxxx> writes: > >> > >> > Invalid int type emulation and target not running emulation have same > >> > codes, which update APIC ICR high/low registers, and emulate sending > >> > the IPI. > >> > > >> > so fall through this switch cases to reduce duplicate codes > >> > > >> > Signed-off-by: Li RongQing <lirongqing@xxxxxxxxx> > >> > Signed-off-by: Zhang Yu <zhangyu31@xxxxxxxxx> > >> > --- > >> > arch/x86/kvm/svm.c | 5 ----- > >> > 1 file changed, 5 deletions(-) > >> > > >> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index > >> > 276ab8ab6c95..2e0c9cb349d2 100644 > >> > --- a/arch/x86/kvm/svm.c > >> > +++ b/arch/x86/kvm/svm.c > >> > @@ -4508,12 +4508,7 @@ static int avic_incomplete_ipi_interception(struct > >> vcpu_svm *svm) > >> > * formats are supported. All other IPI types cause > >> > * a #VMEXIT, which needs to emulated. > >> > */ > >> > - kvm_lapic_reg_write(apic, APIC_ICR2, icrh); > >> > - kvm_lapic_reg_write(apic, APIC_ICR, icrl); > >> > - break; > >> > >> > >> Doesn't checkpatch.pl complain about missing 'Fall through' comment here? In > >> any case it's probably worth it adding it. > >> > > > > This place is a empty case block, which does not need the mark "fall through" > > So checkpatch.pl did not complain, and gcc did not complain > > > > And I have sent patch to remove this unnecessary "fall through" before > > > > (I'm not insisting on the change) > > Generally it makes sense to explicitly say 'fall through' in each 'case' > without a 'break' to assist readers of the code, it's very easy to miss > this subtle thing otherwise (just IMO). How about redoing the comments so that the cases statements are back-to-back? I think that would resolve Vitaly's concern but also avoid the unnecessary "fall through" annotation. It also provides an opportunity to trim a few lines by widing the comment out to 80 columns. E.g.: diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index 426039285fd1..71de264c6865 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -4502,31 +4502,21 @@ static int avic_incomplete_ipi_interception(struct vcpu_svm *svm) switch (id) { case AVIC_IPI_FAILURE_INVALID_INT_TYPE: + case AVIC_IPI_FAILURE_TARGET_NOT_RUNNING: /* - * AVIC hardware handles the generation of - * IPIs when the specified Message Type is Fixed - * (also known as fixed delivery mode) and - * the Trigger Mode is edge-triggered. The hardware - * also supports self and broadcast delivery modes - * specified via the Destination Shorthand(DSH) - * field of the ICRL. Logical and physical APIC ID - * formats are supported. All other IPI types cause - * a #VMEXIT, which needs to emulated. + * AVIC hardware handles the generation of IPIs when the + * specified Message Type is Fixed (also known as fixed + * delivery mode) and the Trigger Mode is edge-triggered. + * The hardware also supports self and broadcast delivery modes + * specified via the Destination Shorthand(DSH) field of the + * ICRL. Logical and physical APIC ID formats are supported. + * All other IPI types cause a #VMEXIT, which needs to + * emulated. AVIC hardware also cannot handle IPIs to CPUs + * that are not running, emulate the IPI for that case as well. */ kvm_lapic_reg_write(apic, APIC_ICR2, icrh); kvm_lapic_reg_write(apic, APIC_ICR, icrl); break; - case AVIC_IPI_FAILURE_TARGET_NOT_RUNNING: { - struct kvm_lapic *apic = svm->vcpu.arch.apic; - - /* - * Update ICR high and low, then emulate sending IPI, - * which is handled when writing APIC_ICR. - */ - kvm_lapic_reg_write(apic, APIC_ICR2, icrh); - kvm_lapic_reg_write(apic, APIC_ICR, icrl); - break; - } case AVIC_IPI_FAILURE_INVALID_TARGET: WARN_ONCE(1, "Invalid IPI target: index=%u, vcpu=%d, icr=%#0x:%#0x\n", index, svm->vcpu.vcpu_id, icrh, icrl);
