[PATCH] KVM: APIC: avoid instruction emulation for EOI writes

[PATCH] KVM: APIC: avoid instruction emulation for EOI writes

[Tian, Kevin]

[-- Attachment #1: Type: text/plain, Size: 3392 bytes --]

Hi, Avi,

Here comes the patch:

    KVM: APIC: avoid instruction emulation for EOI writes
    
    Instruction emulation for EOI writes can be skipped, since sane
    guest simply uses MOV instead of string operations. This is a nice
    improvement when guest doesn't support x2apic or hyper-V EOI
    support.
    
    a single VM bandwidth is observed with ~8% bandwidth improvement
    (7.4Gbps->8Gbps), by saving ~5% cycles from EOI emulation.
    
    Signed-off-by: Kevin Tian <kevin.tian@intel.com>
    <Based on earlier work from>:
    Signed-off-by: Eddie Dong <eddie.dong@intel.com>
    Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 57dcbd4..933187e 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -864,6 +864,15 @@ static int apic_mmio_write(struct kvm_io_device *this,
 	return 0;
 }
 
+void kvm_lapic_set_eoi(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
+	if (apic)
+		apic_set_eoi(vcpu->arch.apic);
+}
+EXPORT_SYMBOL_GPL(kvm_lapic_set_eoi);
+
 void kvm_free_lapic(struct kvm_vcpu *vcpu)
 {
 	if (!vcpu->arch.apic)
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 52c9e6b..8287243 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -26,6 +26,7 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu);
 void kvm_lapic_reset(struct kvm_vcpu *vcpu);
 u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu);
 void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8);
+void kvm_lapic_set_eoi(struct kvm_vcpu *vcpu);
 void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value);
 u64 kvm_lapic_get_base(struct kvm_vcpu *vcpu);
 void kvm_apic_set_version(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 5e8d411..35e4af7 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4540,6 +4540,22 @@ static int handle_xsetbv(struct kvm_vcpu *vcpu)
 
 static int handle_apic_access(struct kvm_vcpu *vcpu)
 {
+	unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
+	int access_type, offset;
+
+	access_type = (exit_qualification >> 12) & 0xf;
+	offset = exit_qualification & 0xfff;
+	/*
+	 * Sane guest uses MOV instead of string operations to
+	 * write EOI, with written value not cared. So make a
+	 * short-circuit here by avoiding heavy instruction 
+	 * emulation.
+	 */
+	if ((access_type == 1) && (offset == APIC_EOI)) {
+		kvm_lapic_set_eoi(vcpu);
+		skip_emulated_instruction(vcpu);
+		return 1;
+	}
 	return emulate_instruction(vcpu, 0) == EMULATE_DONE;
 }

Thanks
Kevin

> -----Original Message-----
> From: Avi Kivity [mailto:avi@redhat.com]
> Sent: Thursday, August 25, 2011 12:39 PM
> To: Tian, Kevin
> Cc: kvm@vger.kernel.org; Nakajima, Jun
> Subject: Re: about vEOI optimization
> 
> On 08/25/2011 05:24 AM, Tian, Kevin wrote:
> > >
> > >  Another option is the hyper-V EOI support, which can also eliminate the
> > >  EOI exit when no additional interrupt is pending.  This can improve EOI
> > >  performance even more.
> > >
> >
> > yes, and this is an orthogonal option.
> >
> > So if you agree, I'll send out an updated patch atop their work.
> >
> >
> 
> Thanks.
> 
> --
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.


[-- Attachment #2: 20110829_kvm_eoi_opt.patch --]
[-- Type: application/octet-stream, Size: 2656 bytes --]

commit 0a52eaa0e8781bbbab46dd4e8b768bc522f6cb16
Author: Kevin Tian <kevin.tian@intel.com>
Date:   Mon Aug 29 13:08:28 2011 +0800

    KVM: APIC: avoid instruction emulation for EOI writes
    
    Instruction emulation for EOI writes can be skipped, since sane
    guest simply uses MOV instead of string operations. This is a nice
    improvement when guest doesn't support x2apic or hyper-V EOI
    support.
    
    a single VM bandwidth is observed with ~8% bandwidth improvement
    (7.4Gbps->8Gbps), by saving ~5% cycles from EOI emulation.
    
    Signed-off-by: Kevin Tian <kevin.tian@intel.com>
    <Based on earlier work from>:
    Signed-off-by: Eddie Dong <eddie.dong@intel.com>
    Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 57dcbd4..933187e 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -864,6 +864,15 @@ static int apic_mmio_write(struct kvm_io_device *this,
 	return 0;
 }
 
+void kvm_lapic_set_eoi(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
+	if (apic)
+		apic_set_eoi(vcpu->arch.apic);
+}
+EXPORT_SYMBOL_GPL(kvm_lapic_set_eoi);
+
 void kvm_free_lapic(struct kvm_vcpu *vcpu)
 {
 	if (!vcpu->arch.apic)
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 52c9e6b..8287243 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -26,6 +26,7 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu);
 void kvm_lapic_reset(struct kvm_vcpu *vcpu);
 u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu);
 void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8);
+void kvm_lapic_set_eoi(struct kvm_vcpu *vcpu);
 void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value);
 u64 kvm_lapic_get_base(struct kvm_vcpu *vcpu);
 void kvm_apic_set_version(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 5e8d411..35e4af7 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4540,6 +4540,22 @@ static int handle_xsetbv(struct kvm_vcpu *vcpu)
 
 static int handle_apic_access(struct kvm_vcpu *vcpu)
 {
+	unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
+	int access_type, offset;
+
+	access_type = (exit_qualification >> 12) & 0xf;
+	offset = exit_qualification & 0xfff;
+	/*
+	 * Sane guest uses MOV instead of string operations to
+	 * write EOI, with written value not cared. So make a
+	 * short-circuit here by avoiding heavy instruction 
+	 * emulation.
+	 */
+	if ((access_type == 1) && (offset == APIC_EOI)) {
+		kvm_lapic_set_eoi(vcpu);
+		skip_emulated_instruction(vcpu);
+		return 1;
+	}
 	return emulate_instruction(vcpu, 0) == EMULATE_DONE;
 }
 

Re: [PATCH] KVM: APIC: avoid instruction emulation for EOI writes

[Avi Kivity]

On 08/29/2011 09:09 AM, Tian, Kevin wrote:
>
>   static int handle_apic_access(struct kvm_vcpu *vcpu)
>   {
> +	unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
> +	int access_type, offset;
> +
> +	access_type = (exit_qualification>>  12)&  0xf;
> +	offset = exit_qualification&  0xfff;
> +	/*
> +	 * Sane guest uses MOV instead of string operations to
> +	 * write EOI, with written value not cared. So make a
> +	 * short-circuit here by avoiding heavy instruction
> +	 * emulation.
> +	 */
> +	if ((access_type == 1)&&  (offset == APIC_EOI)) {

Please replace this 1 with a #define.

> +		kvm_lapic_set_eoi(vcpu);
> +		skip_emulated_instruction(vcpu);
> +		return 1;
> +	}
>   	return emulate_instruction(vcpu, 0) == EMULATE_DONE;
>   }
>
>

Looks good otherwise.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

RE: [PATCH] KVM: APIC: avoid instruction emulation for EOI writes

[Tian, Kevin]

[-- Attachment #1: Type: text/plain, Size: 3498 bytes --]

> From: Avi Kivity [mailto:avi@redhat.com]
> Sent: Monday, August 29, 2011 3:24 PM
> 
> On 08/29/2011 09:09 AM, Tian, Kevin wrote:
> >
> >   static int handle_apic_access(struct kvm_vcpu *vcpu)
> >   {
> > +	unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
> > +	int access_type, offset;
> > +
> > +	access_type = (exit_qualification>>  12)&  0xf;
> > +	offset = exit_qualification&  0xfff;
> > +	/*
> > +	 * Sane guest uses MOV instead of string operations to
> > +	 * write EOI, with written value not cared. So make a
> > +	 * short-circuit here by avoiding heavy instruction
> > +	 * emulation.
> > +	 */
> > +	if ((access_type == 1)&&  (offset == APIC_EOI)) {
> 
> Please replace this 1 with a #define.
> 

updated.

--

    KVM: APIC: avoid instruction emulation for EOI writes
    
    Instruction emulation for EOI writes can be skipped, since sane
    guest simply uses MOV instead of string operations. This is a nice
    improvement when guest doesn't support x2apic or hyper-V EOI
    support.
    
    a single VM bandwidth is observed with ~8% bandwidth improvement
    (7.4Gbps->8Gbps), by saving ~5% cycles from EOI emulation.
    
    Signed-off-by: Kevin Tian <kevin.tian@intel.com>
    <Based on earlier work from>:
    Signed-off-by: Eddie Dong <eddie.dong@intel.com>
    Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 57dcbd4..933187e 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -864,6 +864,15 @@ static int apic_mmio_write(struct kvm_io_device *this,
 	return 0;
 }
 
+void kvm_lapic_set_eoi(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
+	if (apic)
+		apic_set_eoi(vcpu->arch.apic);
+}
+EXPORT_SYMBOL_GPL(kvm_lapic_set_eoi);
+
 void kvm_free_lapic(struct kvm_vcpu *vcpu)
 {
 	if (!vcpu->arch.apic)
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 52c9e6b..8287243 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -26,6 +26,7 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu);
 void kvm_lapic_reset(struct kvm_vcpu *vcpu);
 u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu);
 void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8);
+void kvm_lapic_set_eoi(struct kvm_vcpu *vcpu);
 void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value);
 u64 kvm_lapic_get_base(struct kvm_vcpu *vcpu);
 void kvm_apic_set_version(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 5e8d411..34c094f 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4538,8 +4538,25 @@ static int handle_xsetbv(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
+#define APIC_ACCESS_TYPE_W 1 /* Linear write access during inst execution */
 static int handle_apic_access(struct kvm_vcpu *vcpu)
 {
+	unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
+	int access_type, offset;
+
+	access_type = (exit_qualification >> 12) & 0xf;
+	offset = exit_qualification & 0xfff;
+	/*
+	 * Sane guest uses MOV instead of string operations to
+	 * write EOI, with written value not cared. So make a
+	 * short-circuit here by avoiding heavy instruction 
+	 * emulation.
+	 */
+	if ((access_type == APIC_ACCESS_TYPE_W) && (offset == APIC_EOI)) {
+		kvm_lapic_set_eoi(vcpu);
+		skip_emulated_instruction(vcpu);
+		return 1;
+	}
 	return emulate_instruction(vcpu, 0) == EMULATE_DONE;
 }

Thanks
Kevin

[-- Attachment #2: 20110829_kvm_eoi_opt.patch --]
[-- Type: application/octet-stream, Size: 2766 bytes --]

commit c873f0a162472d2a16a57645f2f8307c62ae15f5
Author: Kevin Tian <kevin.tian@intel.com>
Date:   Mon Aug 29 13:08:28 2011 +0800

    KVM: APIC: avoid instruction emulation for EOI writes
    
    Instruction emulation for EOI writes can be skipped, since sane
    guest simply uses MOV instead of string operations. This is a nice
    improvement when guest doesn't support x2apic or hyper-V EOI
    support.
    
    a single VM bandwidth is observed with ~8% bandwidth improvement
    (7.4Gbps->8Gbps), by saving ~5% cycles from EOI emulation.
    
    Signed-off-by: Kevin Tian <kevin.tian@intel.com>
    <Based on earlier work from>:
    Signed-off-by: Eddie Dong <eddie.dong@intel.com>
    Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 57dcbd4..933187e 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -864,6 +864,15 @@ static int apic_mmio_write(struct kvm_io_device *this,
 	return 0;
 }
 
+void kvm_lapic_set_eoi(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
+	if (apic)
+		apic_set_eoi(vcpu->arch.apic);
+}
+EXPORT_SYMBOL_GPL(kvm_lapic_set_eoi);
+
 void kvm_free_lapic(struct kvm_vcpu *vcpu)
 {
 	if (!vcpu->arch.apic)
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 52c9e6b..8287243 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -26,6 +26,7 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu);
 void kvm_lapic_reset(struct kvm_vcpu *vcpu);
 u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu);
 void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8);
+void kvm_lapic_set_eoi(struct kvm_vcpu *vcpu);
 void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value);
 u64 kvm_lapic_get_base(struct kvm_vcpu *vcpu);
 void kvm_apic_set_version(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 5e8d411..34c094f 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4538,8 +4538,25 @@ static int handle_xsetbv(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
+#define APIC_ACCESS_TYPE_W 1 /* Linear write access during inst execution */
 static int handle_apic_access(struct kvm_vcpu *vcpu)
 {
+	unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
+	int access_type, offset;
+
+	access_type = (exit_qualification >> 12) & 0xf;
+	offset = exit_qualification & 0xfff;
+	/*
+	 * Sane guest uses MOV instead of string operations to
+	 * write EOI, with written value not cared. So make a
+	 * short-circuit here by avoiding heavy instruction 
+	 * emulation.
+	 */
+	if ((access_type == APIC_ACCESS_TYPE_W) && (offset == APIC_EOI)) {
+		kvm_lapic_set_eoi(vcpu);
+		skip_emulated_instruction(vcpu);
+		return 1;
+	}
 	return emulate_instruction(vcpu, 0) == EMULATE_DONE;
 }
 

RE: [PATCH] KVM: APIC: avoid instruction emulation for EOI writes

[Sasha Levin]

On Mon, 2011-08-29 at 15:35 +0800, Tian, Kevin wrote:
> > From: Avi Kivity [mailto:avi@redhat.com]
> > Sent: Monday, August 29, 2011 3:24 PM
> > 
> > On 08/29/2011 09:09 AM, Tian, Kevin wrote:
> > >
> > >   static int handle_apic_access(struct kvm_vcpu *vcpu)
> > >   {
> > > +	unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
> > > +	int access_type, offset;
> > > +
> > > +	access_type = (exit_qualification>>  12)&  0xf;
> > > +	offset = exit_qualification&  0xfff;
> > > +	/*
> > > +	 * Sane guest uses MOV instead of string operations to
> > > +	 * write EOI, with written value not cared. So make a
> > > +	 * short-circuit here by avoiding heavy instruction
> > > +	 * emulation.
> > > +	 */
> > > +	if ((access_type == 1)&&  (offset == APIC_EOI)) {
> > 
> > Please replace this 1 with a #define.
> > 
> 
> updated.
> 
> --
> 
>     KVM: APIC: avoid instruction emulation for EOI writes
>     
>     Instruction emulation for EOI writes can be skipped, since sane
>     guest simply uses MOV instead of string operations. This is a nice
>     improvement when guest doesn't support x2apic or hyper-V EOI
>     support.
>     
>     a single VM bandwidth is observed with ~8% bandwidth improvement
>     (7.4Gbps->8Gbps), by saving ~5% cycles from EOI emulation.
>     
>     Signed-off-by: Kevin Tian <kevin.tian@intel.com>
>     <Based on earlier work from>:
>     Signed-off-by: Eddie Dong <eddie.dong@intel.com>
>     Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 57dcbd4..933187e 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -864,6 +864,15 @@ static int apic_mmio_write(struct kvm_io_device *this,
>  	return 0;
>  }
>  
> +void kvm_lapic_set_eoi(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_lapic *apic = vcpu->arch.apic;
> +
> +	if (apic)
> +		apic_set_eoi(vcpu->arch.apic);
> +}
> +EXPORT_SYMBOL_GPL(kvm_lapic_set_eoi);
> +
>  void kvm_free_lapic(struct kvm_vcpu *vcpu)
>  {
>  	if (!vcpu->arch.apic)
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index 52c9e6b..8287243 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -26,6 +26,7 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu);
>  void kvm_lapic_reset(struct kvm_vcpu *vcpu);
>  u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu);
>  void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8);
> +void kvm_lapic_set_eoi(struct kvm_vcpu *vcpu);
>  void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value);
>  u64 kvm_lapic_get_base(struct kvm_vcpu *vcpu);
>  void kvm_apic_set_version(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 5e8d411..34c094f 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -4538,8 +4538,25 @@ static int handle_xsetbv(struct kvm_vcpu *vcpu)
>  	return 1;
>  }
>  
> +#define APIC_ACCESS_TYPE_W 1 /* Linear write access during inst execution */
>  static int handle_apic_access(struct kvm_vcpu *vcpu)
>  {
> +	unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
> +	int access_type, offset;
> +
> +	access_type = (exit_qualification >> 12) & 0xf;
> +	offset = exit_qualification & 0xfff;

Maybe it's worth moving that #define above, and '#define'ing all the
magic that happens in these 2 lines in vmx.h along with all the other
exit qualification parameters?

> +	/*
> +	 * Sane guest uses MOV instead of string operations to
> +	 * write EOI, with written value not cared. So make a
> +	 * short-circuit here by avoiding heavy instruction 
> +	 * emulation.
> +	 */
> +	if ((access_type == APIC_ACCESS_TYPE_W) && (offset == APIC_EOI)) {
> +		kvm_lapic_set_eoi(vcpu);
> +		skip_emulated_instruction(vcpu);
> +		return 1;
> +	}
>  	return emulate_instruction(vcpu, 0) == EMULATE_DONE;
>  }
> 
> Thanks
> Kevin


-- 

Sasha.

Re: [PATCH] KVM: APIC: avoid instruction emulation for EOI writes

[Avi Kivity]

On 08/29/2011 11:15 AM, Sasha Levin wrote:
> >
> >  +#define APIC_ACCESS_TYPE_W 1 /* Linear write access during inst execution */
> >   static int handle_apic_access(struct kvm_vcpu *vcpu)
> >   {
> >  +	unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
> >  +	int access_type, offset;
> >  +
> >  +	access_type = (exit_qualification>>  12)&  0xf;
> >  +	offset = exit_qualification&  0xfff;
>
> Maybe it's worth moving that #define above, and '#define'ing all the
> magic that happens in these 2 lines in vmx.h along with all the other
> exit qualification parameters?

Sure, that's what vmx.h is for.  And 0xfff is ~PAGE_MASK.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] KVM: APIC: avoid instruction emulation for EOI writes

[Jan Kiszka]

On 2011-08-29 08:09, Tian, Kevin wrote:
> Hi, Avi,
> 
> Here comes the patch:
> 
>     KVM: APIC: avoid instruction emulation for EOI writes
>     
>     Instruction emulation for EOI writes can be skipped, since sane
>     guest simply uses MOV instead of string operations. This is a nice
>     improvement when guest doesn't support x2apic or hyper-V EOI
>     support.
>     
>     a single VM bandwidth is observed with ~8% bandwidth improvement
>     (7.4Gbps->8Gbps), by saving ~5% cycles from EOI emulation.
>     
>     Signed-off-by: Kevin Tian <kevin.tian@intel.com>
>     <Based on earlier work from>:
>     Signed-off-by: Eddie Dong <eddie.dong@intel.com>
>     Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 57dcbd4..933187e 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -864,6 +864,15 @@ static int apic_mmio_write(struct kvm_io_device *this,
>  	return 0;
>  }
>  
> +void kvm_lapic_set_eoi(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_lapic *apic = vcpu->arch.apic;
> +
> +	if (apic)
> +		apic_set_eoi(vcpu->arch.apic);
> +}
> +EXPORT_SYMBOL_GPL(kvm_lapic_set_eoi);
> +
>  void kvm_free_lapic(struct kvm_vcpu *vcpu)
>  {
>  	if (!vcpu->arch.apic)
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index 52c9e6b..8287243 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -26,6 +26,7 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu);
>  void kvm_lapic_reset(struct kvm_vcpu *vcpu);
>  u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu);
>  void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8);
> +void kvm_lapic_set_eoi(struct kvm_vcpu *vcpu);
>  void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value);
>  u64 kvm_lapic_get_base(struct kvm_vcpu *vcpu);
>  void kvm_apic_set_version(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 5e8d411..35e4af7 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -4540,6 +4540,22 @@ static int handle_xsetbv(struct kvm_vcpu *vcpu)
>  
>  static int handle_apic_access(struct kvm_vcpu *vcpu)
>  {
> +	unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
> +	int access_type, offset;
> +
> +	access_type = (exit_qualification >> 12) & 0xf;
> +	offset = exit_qualification & 0xfff;
> +	/*
> +	 * Sane guest uses MOV instead of string operations to
> +	 * write EOI, with written value not cared. So make a
> +	 * short-circuit here by avoiding heavy instruction 
> +	 * emulation.
> +	 */

Is there no cheap way to validate this assumption and fall back to the
slow path in case it doesn't apply? E.g. reading the first instruction
byte and matching it against a whitelist? Even if the ignored scenarios
are highly unlikely, I think we so far tried hard to provide both fast
and accurate results to the guest in all cases.

> +	if ((access_type == 1) && (offset == APIC_EOI)) {
> +		kvm_lapic_set_eoi(vcpu);
> +		skip_emulated_instruction(vcpu);
> +		return 1;
> +	}
>  	return emulate_instruction(vcpu, 0) == EMULATE_DONE;
>  }
> 
> Thanks
> Kevin

Nice optimization otherwise!

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH] KVM: APIC: avoid instruction emulation for EOI writes

[Avi Kivity]

On 08/29/2011 01:24 PM, Jan Kiszka wrote:
> >
> >   static int handle_apic_access(struct kvm_vcpu *vcpu)
> >   {
> >  +	unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
> >  +	int access_type, offset;
> >  +
> >  +	access_type = (exit_qualification>>  12)&  0xf;
> >  +	offset = exit_qualification&  0xfff;
> >  +	/*
> >  +	 * Sane guest uses MOV instead of string operations to
> >  +	 * write EOI, with written value not cared. So make a
> >  +	 * short-circuit here by avoiding heavy instruction
> >  +	 * emulation.
> >  +	 */
>
> Is there no cheap way to validate this assumption and fall back to the
> slow path in case it doesn't apply? E.g. reading the first instruction
> byte and matching it against a whitelist? Even if the ignored scenarios
> are highly unlikely, I think we so far tried hard to provide both fast
> and accurate results to the guest in all cases.
>

Just reading the first byte requires a guest page table walk.  This is 
probably the highest cost in emulation (which also requires a walk for 
the data access).

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] KVM: APIC: avoid instruction emulation for EOI writes

[Jan Kiszka]

On 2011-08-29 12:59, Avi Kivity wrote:
> On 08/29/2011 01:24 PM, Jan Kiszka wrote:
>>>
>>>   static int handle_apic_access(struct kvm_vcpu *vcpu)
>>>   {
>>>  +	unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
>>>  +	int access_type, offset;
>>>  +
>>>  +	access_type = (exit_qualification>>  12)&  0xf;
>>>  +	offset = exit_qualification&  0xfff;
>>>  +	/*
>>>  +	 * Sane guest uses MOV instead of string operations to
>>>  +	 * write EOI, with written value not cared. So make a
>>>  +	 * short-circuit here by avoiding heavy instruction
>>>  +	 * emulation.
>>>  +	 */
>>
>> Is there no cheap way to validate this assumption and fall back to the
>> slow path in case it doesn't apply? E.g. reading the first instruction
>> byte and matching it against a whitelist? Even if the ignored scenarios
>> are highly unlikely, I think we so far tried hard to provide both fast
>> and accurate results to the guest in all cases.
>>
> 
> Just reading the first byte requires a guest page table walk.  This is 
> probably the highest cost in emulation (which also requires a walk for 
> the data access).

And what about caching the result of the first walk? Usually, a "sane
guest" won't have many code pages that issue the EIO.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH] KVM: APIC: avoid instruction emulation for EOI writes

[Avi Kivity]

On 08/29/2011 02:03 PM, Jan Kiszka wrote:
> >
> >  Just reading the first byte requires a guest page table walk.  This is
> >  probably the highest cost in emulation (which also requires a walk for
> >  the data access).
>
> And what about caching the result of the first walk? Usually, a "sane
> guest" won't have many code pages that issue the EIO.
>

There's no way to know when to invalidate the cache.

We could go a bit further, and cache the the whole thing.  On the first 
exit, do the entire emulation, and remember %rip.  On the second exit, 
if %rip matches, skip directly to kvm_lapic_eoi().

But I don't think it's worth it.  This also has failure modes, and 
really, no guest will ever write to EOI with stosl.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] KVM: APIC: avoid instruction emulation for EOI writes

[Jan Kiszka]

On 2011-08-29 13:11, Avi Kivity wrote:
> On 08/29/2011 02:03 PM, Jan Kiszka wrote:
>>>
>>>  Just reading the first byte requires a guest page table walk.  This is
>>>  probably the highest cost in emulation (which also requires a walk for
>>>  the data access).
>>
>> And what about caching the result of the first walk? Usually, a "sane
>> guest" won't have many code pages that issue the EIO.
>>
> 
> There's no way to know when to invalidate the cache.

Set the affected code page read-only?

> 
> We could go a bit further, and cache the the whole thing.  On the first 
> exit, do the entire emulation, and remember %rip.  On the second exit, 
> if %rip matches, skip directly to kvm_lapic_eoi().
> 
> But I don't think it's worth it.  This also has failure modes, and 
> really, no guest will ever write to EOI with stosl.

...or add/sub/and/or etc. Well, we've done other crazy things in the
past just to keep even the unlikely case correct. I was just wondering
if that policy changed.

However, I just realized that user space is able to avoid this
inaccuracy for potentially insane guests by not using in-kernel
irqchips. So we have at least a knob.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH] KVM: APIC: avoid instruction emulation for EOI writes

[Avi Kivity]

On 08/29/2011 04:55 PM, Jan Kiszka wrote:
> On 2011-08-29 13:11, Avi Kivity wrote:
> >  On 08/29/2011 02:03 PM, Jan Kiszka wrote:
> >>>
> >>>   Just reading the first byte requires a guest page table walk.  This is
> >>>   probably the highest cost in emulation (which also requires a walk for
> >>>   the data access).
> >>
> >>  And what about caching the result of the first walk? Usually, a "sane
> >>  guest" won't have many code pages that issue the EIO.
> >>
> >
> >  There's no way to know when to invalidate the cache.
>
> Set the affected code page read-only?

The virt-phys mapping could change too.  And please, don't think of new 
reasons to write protect pages, they break up my lovely 2M maps.

> >
> >  We could go a bit further, and cache the the whole thing.  On the first
> >  exit, do the entire emulation, and remember %rip.  On the second exit,
> >  if %rip matches, skip directly to kvm_lapic_eoi().
> >
> >  But I don't think it's worth it.  This also has failure modes, and
> >  really, no guest will ever write to EOI with stosl.
>
> ...or add/sub/and/or etc.

Argh, yes, flags can be updated.

Actually, this might work - if we get a read access first as part of the 
RMW, we'll emulate the instruction.  No idea what the hardware does in 
this case.

>   Well, we've done other crazy things in the
> past just to keep even the unlikely case correct. I was just wondering
> if that policy changed.

I can't answer yes to that question.  But I see no way to make it work 
both fast and correct.

>
> However, I just realized that user space is able to avoid this
> inaccuracy for potentially insane guests by not using in-kernel
> irqchips. So we have at least a knob.

Could/should have a flag to disable this in the kernel as well.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] KVM: APIC: avoid instruction emulation for EOI writes

[ya su]

hi,Kevin:

    I applied the patch in 2.6.39-rc7+, and run a winxp vm, there is
many ICR write emulations, trace-cmd is as the following:

600281: kvm_entry:            vcpu 0
600284: kvm_exit:             reason APIC_ACCESS rip 0x806e7b85 info 3
600285: kvm_emulate_insn:     0:806e7b85: f7 05 00 03 fe ff 00 10 00 0
600286: kvm_apic:             apic_read APIC_ICR = 0x40041
600286: kvm_mmio:             mmio read len 4 gpa 0xfee00300 val 0x40041
600287: kvm_mmio:             mmio write len 4 gpa 0xfee00300 val 0x40041
600287: kvm_apic:             apic_write APIC_ICR = 0x40041
600287: kvm_apic_ipi:         dst 1 vec 65 (Fixed|physical|de-assert|edge|self)
600288: kvm_apic_accept_irq:  apicid 0 vec 65 (Fixed|edge)

600288: kvm_entry:            vcpu 0
600289: kvm_exit:             reason APIC_ACCESS rip 0x806e7b91 info 1
600290: kvm_emulate_insn:     0:806e7b91: 89 0d 00 03 fe ff
600291: kvm_mmio:             mmio write len 4 gpa 0xfee00300 val 0x40041
600291: kvm_apic:             apic_write APIC_ICR = 0x40041
600291: kvm_apic_ipi:         dst 1 vec 65 (Fixed|physical|de-assert|edge|self)
600291: kvm_apic_accept_irq:  apicid 0 vec 65 (Fixed|edge) (coalesced)

600292: kvm_entry:            vcpu 0
600293: kvm_exit:             reason APIC_ACCESS rip 0x806e7b97 info 3
600294: kvm_emulate_insn:     0:806e7b97: f7 05 00 03 fe ff 00 10 00 0
600294: kvm_apic:             apic_read APIC_ICR = 0x40041
600295: kvm_mmio:             mmio read len 4 gpa 0xfee00300 val 0x40041
600295: kvm_mmio:             mmio write len 4 gpa 0xfee00300 val 0x40041
600295: kvm_apic:             apic_write APIC_ICR = 0x40041
600296: kvm_apic_ipi:         dst 1 vec 65 (Fixed|physical|de-assert|edge|self)
600296: kvm_apic_accept_irq:  apicid 0 vec 65 (Fixed|edge) (coalesced)

as the asm code on addr 0x80637b85:

0x80637b85:  testl $0x1000, 0xfffe0300
0x80637b8f:   jne 0x80637b85
0x80637b91:  mov %ecx, 0xfffe0300
0x80637b97:  testl $0x1000, 0xfffe0300
0x80637ba1:  jne 0x80637b97

    I wonder why testl operation will also cause a ICR write, from the
asm code, there should only issue one IPI, but from trace-cmd, it
issued 3 IPI, is there something wrong?

   Is it also possible to optimize ICR write emulation, from the
result, winxp vm will produce a lot of ICR writes

Regards!

Suya.


2011/8/29 Avi Kivity <avi@redhat.com>:
> On 08/29/2011 04:55 PM, Jan Kiszka wrote:
>>
>> On 2011-08-29 13:11, Avi Kivity wrote:
>> >  On 08/29/2011 02:03 PM, Jan Kiszka wrote:
>> >>>
>> >>>   Just reading the first byte requires a guest page table walk.  This
>> >>> is
>> >>>   probably the highest cost in emulation (which also requires a walk
>> >>> for
>> >>>   the data access).
>> >>
>> >>  And what about caching the result of the first walk? Usually, a "sane
>> >>  guest" won't have many code pages that issue the EIO.
>> >>
>> >
>> >  There's no way to know when to invalidate the cache.
>>
>> Set the affected code page read-only?
>
> The virt-phys mapping could change too.  And please, don't think of new
> reasons to write protect pages, they break up my lovely 2M maps.
>
>> >
>> >  We could go a bit further, and cache the the whole thing.  On the first
>> >  exit, do the entire emulation, and remember %rip.  On the second exit,
>> >  if %rip matches, skip directly to kvm_lapic_eoi().
>> >
>> >  But I don't think it's worth it.  This also has failure modes, and
>> >  really, no guest will ever write to EOI with stosl.
>>
>> ...or add/sub/and/or etc.
>
> Argh, yes, flags can be updated.
>
> Actually, this might work - if we get a read access first as part of the
> RMW, we'll emulate the instruction.  No idea what the hardware does in this
> case.
>
>>  Well, we've done other crazy things in the
>> past just to keep even the unlikely case correct. I was just wondering
>> if that policy changed.
>
> I can't answer yes to that question.  But I see no way to make it work both
> fast and correct.
>
>>
>> However, I just realized that user space is able to avoid this
>> inaccuracy for potentially insane guests by not using in-kernel
>> irqchips. So we have at least a knob.
>
> Could/should have a flag to disable this in the kernel as well.
>
> --
> error compiling committee.c: too many arguments to function
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] KVM: APIC: avoid instruction emulation for EOI writes

[Avi Kivity]

On 09/10/2011 11:41 AM, ya su wrote:
> 0x80637b85:  testl $0x1000, 0xfffe0300
> 0x80637b8f:   jne 0x80637b85
> 0x80637b91:  mov %ecx, 0xfffe0300
> 0x80637b97:  testl $0x1000, 0xfffe0300
> 0x80637ba1:  jne 0x80637b97
>
>      I wonder why testl operation will also cause a ICR write, from the
> asm code, there should only issue one IPI, but from trace-cmd, it
> issued 3 IPI, is there something wrong?

It's a bug in test insn emulation, coincidentally I wrote a patch to fix 
it yesterday, not imagining that it actually happens in practice.

>     Is it also possible to optimize ICR write emulation, from the
> result, winxp vm will produce a lot of ICR writes
>

Unfortunately not.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH] KVM: APIC: avoid instruction emulation for EOI writes

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 876 bytes --]

On 2011-09-11 09:11, Avi Kivity wrote:
> On 09/10/2011 11:41 AM, ya su wrote:
>> 0x80637b85:  testl $0x1000, 0xfffe0300
>> 0x80637b8f:   jne 0x80637b85
>> 0x80637b91:  mov %ecx, 0xfffe0300
>> 0x80637b97:  testl $0x1000, 0xfffe0300
>> 0x80637ba1:  jne 0x80637b97
>>
>>      I wonder why testl operation will also cause a ICR write, from the
>> asm code, there should only issue one IPI, but from trace-cmd, it
>> issued 3 IPI, is there something wrong?
> 
> It's a bug in test insn emulation, coincidentally I wrote a patch to fix
> it yesterday, not imagining that it actually happens in practice.
> 
>>     Is it also possible to optimize ICR write emulation, from the
>> result, winxp vm will produce a lot of ICR writes
>>
> 
> Unfortunately not.

I'm just hoping we'll see hardware-assisted APIC, ideally also IOAPIC
virtualization soon.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]