| Message ID | 1552529944-24864-1-git-send-email-lirongqing@baidu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: svm: merge incomplete IPI emulation handling | expand |
Li RongQing <lirongqing@baidu.com> 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@baidu.com> > Signed-off-by: Zhang Yu <zhangyu31@baidu.com> > --- > 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. > 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.
> -----邮件原件----- > 发件人: Vitaly Kuznetsov [mailto:vkuznets@redhat.com] > 发送时间: 2019年3月14日 21:23 > 收件人: Li,Rongqing <lirongqing@baidu.com> > 抄送: x86@kernel.org; kvm@vger.kernel.org > 主题: Re: [PATCH] KVM: svm: merge incomplete IPI emulation handling > > Li RongQing <lirongqing@baidu.com> 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@baidu.com> > > Signed-off-by: Zhang Yu <zhangyu31@baidu.com> > > --- > > 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 Thanks -RongQing > > 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. > > -- > Vitaly
"Li,Rongqing" <lirongqing@baidu.com> writes: >> -----邮件原件----- >> 发件人: Vitaly Kuznetsov [mailto:vkuznets@redhat.com] >> 发送时间: 2019年3月14日 21:23 >> 收件人: Li,Rongqing <lirongqing@baidu.com> >> 抄送: x86@kernel.org; kvm@vger.kernel.org >> 主题: Re: [PATCH] KVM: svm: merge incomplete IPI emulation handling >> >> Li RongQing <lirongqing@baidu.com> 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@baidu.com> >> > Signed-off-by: Zhang Yu <zhangyu31@baidu.com> >> > --- >> > 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).
On Fri, Mar 15, 2019 at 10:14:47AM +0100, Vitaly Kuznetsov wrote: > "Li,Rongqing" <lirongqing@baidu.com> writes: > > >> -----邮件原件----- > >> 发件人: Vitaly Kuznetsov [mailto:vkuznets@redhat.com] > >> 发送时间: 2019年3月14日 21:23 > >> 收件人: Li,Rongqing <lirongqing@baidu.com> > >> 抄送: x86@kernel.org; kvm@vger.kernel.org > >> 主题: Re: [PATCH] KVM: svm: merge incomplete IPI emulation handling > >> > >> Li RongQing <lirongqing@baidu.com> 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@baidu.com> > >> > Signed-off-by: Zhang Yu <zhangyu31@baidu.com> > >> > --- > >> > 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);
Sean Christopherson <sean.j.christopherson@intel.com> writes: > On Fri, Mar 15, 2019 at 10:14:47AM +0100, Vitaly Kuznetsov wrote: >> "Li,Rongqing" <lirongqing@baidu.com> writes: >> >> >> -----邮件原件----- >> >> 发件人: Vitaly Kuznetsov [mailto:vkuznets@redhat.com] >> >> 发送时间: 2019年3月14日 21:23 >> >> 收件人: Li,Rongqing <lirongqing@baidu.com> >> >> 抄送: x86@kernel.org; kvm@vger.kernel.org >> >> 主题: Re: [PATCH] KVM: svm: merge incomplete IPI emulation handling >> >> >> >> Li RongQing <lirongqing@baidu.com> 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@baidu.com> >> >> > Signed-off-by: Zhang Yu <zhangyu31@baidu.com> >> >> > --- >> >> > 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. Sounds awesome to me! Back-to-back case statements obviously don't require "fall through" comments.
> -----邮件原件----- > 发件人: Sean Christopherson [mailto:sean.j.christopherson@intel.com] > 发送时间: 2019年3月15日 22:51 > 收件人: Vitaly Kuznetsov <vkuznets@redhat.com> > 抄送: Li,Rongqing <lirongqing@baidu.com>; x86@kernel.org; > kvm@vger.kernel.org > 主题: Re: 答复: [PATCH] KVM: svm: merge incomplete IPI emulation handling > > On Fri, Mar 15, 2019 at 10:14:47AM +0100, Vitaly Kuznetsov wrote: > > "Li,Rongqing" <lirongqing@baidu.com> writes: > > > > >> -----邮件原件----- > > >> 发件人: Vitaly Kuznetsov [mailto:vkuznets@redhat.com] > > >> 发送时间: 2019年3月14日 21:23 > > >> 收件人: Li,Rongqing <lirongqing@baidu.com> > > >> 抄送: x86@kernel.org; kvm@vger.kernel.org > > >> 主题: Re: [PATCH] KVM: svm: merge incomplete IPI emulation handling > > >> > > >> Li RongQing <lirongqing@baidu.com> 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@baidu.com> > > >> > Signed-off-by: Zhang Yu <zhangyu31@baidu.com> > > >> > --- > > >> > 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.: Thanks, I will send V2 -RongQing > > 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);
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; 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.