Patchwork 答复: [PATCH] KVM: svm: merge incomplete IPI emulation handling

login
register
mail settings
Submitter Christopherson, Sean J
Date March 15, 2019, 2:50 p.m.
Message ID <20190315145048.GA11324@linux.intel.com>
Download mbox | patch
Permalink /patch/749655/
State New
Headers show

Comments

Christopherson, Sean J - March 15, 2019, 2:50 p.m.
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.:
Vitaly Kuznetsov - March 15, 2019, 3:15 p.m.
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.
Li RongQing - March 18, 2019, 12:17 a.m.
> -----邮件原件-----

> 发件人: 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);

Patch

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);