[PATCH 0/3] KVM paravirt_ops implementation

[PATCH 0/3] KVM paravirt_ops implementation

[Anthony Liguori]

This is the start of a paravirt_ops implementation for KVM.  Most of it 
was done by Ingo Molnar, I just moved things around a bit.  I don't 
think there's a measurable performance benefit just yet but there are a 
few more optimizations that I think we can get in time for 2.6.23 that 
will be measurable.

Regards,

Anthony Liguori

[PATCH 1/3] KVM paravirt_ops infrastructure

[Anthony Liguori]

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

Regards,

Anthony Liguori

[-- Attachment #2: kvm-paravirt-guest.diff --]
[-- Type: text/x-patch, Size: 4616 bytes --]

Subject: [PATCH] Add KVM paravirt_ops implementation
From: Anthony Liguori <aliguori@us.ibm.com>

This patch adds the basic infrastructure for paravirtualizing a KVM guest.
Discovery of running under KVM is done by sharing a page of memory between
the guest and host (initially through an MSR write).

This is based on a patch written by Ingo Molnar.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Index: kvm/arch/i386/Kconfig
===================================================================
--- kvm.orig/arch/i386/Kconfig	2007-05-30 08:42:31.000000000 -0500
+++ kvm/arch/i386/Kconfig	2007-05-30 08:42:38.000000000 -0500
@@ -231,6 +231,13 @@
 	  at the moment), by linking the kernel to a GPL-ed ROM module
 	  provided by the hypervisor.
 
+config KVM_GUEST
+	bool "KVM paravirt-ops support"
+	depends on PARAVIRT
+	help
+	  This option enables various optimizations for running under the KVM
+          hypervisor.
+
 config ACPI_SRAT
 	bool
 	default y
Index: kvm/arch/i386/kernel/Makefile
===================================================================
--- kvm.orig/arch/i386/kernel/Makefile	2007-05-30 08:42:26.000000000 -0500
+++ kvm/arch/i386/kernel/Makefile	2007-05-30 08:42:38.000000000 -0500
@@ -41,6 +41,7 @@
 obj-$(CONFIG_K8_NB)		+= k8.o
 
 obj-$(CONFIG_VMI)		+= vmi.o vmiclock.o
+obj-$(CONFIG_KVM_GUEST)		+= kvm.o
 obj-$(CONFIG_PARAVIRT)		+= paravirt.o
 obj-y				+= pcspeaker.o
 
Index: kvm/arch/i386/kernel/kvm.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ kvm/arch/i386/kernel/kvm.c	2007-05-30 09:29:37.000000000 -0500
@@ -0,0 +1,100 @@
+/*
+ * KVM paravirt_ops implementation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ * Copyright (C) 2007, Red Hat, Inc., Ingo Molnar <mingo@redhat.com>
+ * Copyright IBM Corporation, 2007
+ *   Authors: Anthony Liguori <aliguori@us.ibm.com>
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/kvm_para.h>
+#include <linux/cpu.h>
+
+static DEFINE_PER_CPU(struct kvm_vcpu_para_state, para_state);
+extern unsigned char hypercall_addr[4];
+
+static void kvm_guest_setup(void)
+{
+	paravirt_ops.name = "KVM";
+	paravirt_ops.paravirt_enabled = 1;
+}
+
+/*
+ * This is the vm-syscall address - to be patched by the host to
+ * VMCALL (Intel) or VMMCALL (AMD), depending on the CPU model:
+ */
+asm (
+	".globl hypercall_addr\n"
+	".align 4\n"
+	"hypercall_addr:\n"
+	"movl $-38, %eax\n"
+	"ret\n"
+);
+
+static int kvm_guest_register_para(int cpu)
+{
+	struct kvm_vcpu_para_state *para_state = &per_cpu(para_state, cpu);
+
+	printk(KERN_DEBUG "kvm guest on VCPU#%d: trying to register para_state %p\n",
+	       cpu, para_state);
+
+	/*
+	 * Try to write to a magic MSR (which is invalid on any real CPU),
+	 * and thus signal to KVM that we wish to entering paravirtualized
+	 * mode:
+	 */
+	para_state->guest_version = KVM_PARA_API_VERSION;
+	para_state->host_version = -1;
+	para_state->size = sizeof(*para_state);
+	para_state->ret = 0;
+	para_state->hypercall_gpa = __pa(hypercall_addr);
+
+	if (wrmsr_safe(MSR_KVM_API_MAGIC, __pa(para_state), 0)) {
+		printk(KERN_INFO "KVM guest: WRMSR probe failed.\n");
+		return -ENOENT;
+	}
+
+	printk(KERN_DEBUG "kvm guest: host returned %d\n", para_state->ret);
+	printk(KERN_DEBUG "kvm guest: host version: %d\n", para_state->host_version);
+	printk(KERN_DEBUG "kvm guest: syscall entry: %02x %02x %02x %02x\n",
+	       hypercall_addr[0], hypercall_addr[1],
+	       hypercall_addr[2], hypercall_addr[3]);
+
+	if (para_state->ret) {
+		printk(KERN_ERR "kvm guest: host refused registration.\n");
+		return para_state->ret;
+	}
+
+	return 0;
+}
+
+static int __init kvm_guest_init(void)
+{
+	int rc;
+
+	rc = kvm_guest_register_para(smp_processor_id());
+	if (rc) {
+		printk(KERN_INFO "paravirt KVM unavailable\n");
+		goto out;
+	}
+
+	kvm_guest_setup();
+ out:
+	return 0;
+}
+core_initcall(kvm_guest_init);

[-- Attachment #3: Type: text/plain, Size: 184 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

RE: [kvm-devel] [PATCH 1/3] KVM paravirt_ops infrastructure

[Nakajima, Jun]

Anthony Liguori wrote:
> Regards,
> 
> Anthony Liguori

I think we should use the CPUID instruction (leaf 0x40000000) to detect
the hypervosor as we are doing in Xen. 

Jun
---
Intel Open Source Technology Center

Re: [PATCH 1/3] KVM paravirt_ops infrastructure

[Anthony Liguori]

Nakajima, Jun wrote:
> Anthony Liguori wrote:
>   
>> Regards,
>>
>> Anthony Liguori
>>     
>
> I think we should use the CPUID instruction (leaf 0x40000000) to detect
> the hypervosor as we are doing in Xen. 
>   

Is that leaf reserved for such use by Intel?

Regards,

Anthony Liguori

> Jun
> ---
> Intel Open Source Technology Center
>   


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH 1/3] KVM paravirt_ops infrastructure

[Nakajima, Jun]

Anthony Liguori wrote:
> Nakajima, Jun wrote:
> > Anthony Liguori wrote:
> > 
> > > Regards,
> > > 
> > > Anthony Liguori
> > > 
> > 
> > I think we should use the CPUID instruction (leaf 0x40000000) to
detect
> > the hypervosor as we are doing in Xen.
> > 
> 
> Is that leaf reserved for such use by Intel?
> 

What I can say is that we (including the H/W teams) reviewed it
internally.

> Regards,
> 
> Anthony Liguori
> 


Jun
---
Intel Open Source Technology Center

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH 1/3] KVM paravirt_ops infrastructure

[Anthony Liguori]

Nakajima, Jun wrote:
> Anthony Liguori wrote:
>   
>> Nakajima, Jun wrote:
>>     
>>> Anthony Liguori wrote:
>>>
>>>       
>>>> Regards,
>>>>
>>>> Anthony Liguori
>>>>
>>>>         
>>> I think we should use the CPUID instruction (leaf 0x40000000) to
>>>       
> detect
>   
>>> the hypervosor as we are doing in Xen.
>>>
>>>       
>> Is that leaf reserved for such use by Intel?
>>
>>     
>
> What I can say is that we (including the H/W teams) reviewed it
> internally.
>   

I've switched to using CPUID.  I noticed that Xen also uses 0x400000001 
so I'm presuming that that is okay too?

Regards,

Anthony Liguori

>> Regards,
>>
>> Anthony Liguori
>>
>>     
>
>
> Jun
> ---
> Intel Open Source Technology Center
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by DB2 Express
> Download DB2 Express C - the FREE version of DB2 express and take
> control of your XML. No limits. Just data. Click to get it now.
> http://sourceforge.net/powerbar/db2/
> _______________________________________________
> kvm-devel mailing list
> kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/kvm-devel
>
>   


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH 1/3] KVM paravirt_ops infrastructure

[Nakajima, Jun]

Anthony Liguori wrote:
> Nakajima, Jun wrote:
> > What I can say is that we (including the H/W teams) reviewed it
internally.
> > 
> 
> I've switched to using CPUID.  I noticed that Xen also uses
0x400000001
> so I'm presuming that that is okay too?

Yes.

> 
> Regards,
> 
> Anthony Liguori

Jun
---
Intel Open Source Technology Center

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH 1/3] KVM paravirt_ops infrastructure

[Rusty Russell]

On Wed, 2007-05-30 at 09:52 -0500, Anthony Liguori wrote:
> This patch adds the basic infrastructure for paravirtualizing a KVM
> guest.

Hi Anthony!

	Nice patch, comments below.

> Discovery of running under KVM is done by sharing a page of memory
> between
> the guest and host (initially through an MSR write).

I missed the shared page in this patch?  If you are going to do that,
perhaps putting the hypercall magic in that page is a good idea?

> +extern unsigned char hypercall_addr[4];

Perhaps in a header?

> +asm (
> +       ".globl hypercall_addr\n"
> +       ".align 4\n"
> +       "hypercall_addr:\n"
> +       "movl $-38, %eax\n"
> +       "ret\n"
> +);

I don't think we want the hypercall returning Linux error numbers, and
magic numbers are bad too.  ud2 here I think.

> +       para_state->guest_version = KVM_PARA_API_VERSION;
> +       para_state->host_version = -1;
> +       para_state->size = sizeof(*para_state);
> +       para_state->ret = 0;
> +       para_state->hypercall_gpa = __pa(hypercall_addr);

Two versions, size *and* ret?  This seems like overkill...

> +       if (wrmsr_safe(MSR_KVM_API_MAGIC, __pa(para_state), 0)) {
> +               printk(KERN_INFO "KVM guest: WRMSR probe failed.\n");
> +               return -ENOENT;
> +       }

How about printk(KERN_INFO "I am not a KVM guest\n");?

> +static int __init kvm_guest_init(void)
> +{
> +       int rc;
> +
> +       rc = kvm_guest_register_para(smp_processor_id());
> +       if (rc) {
> +               printk(KERN_INFO "paravirt KVM unavailable\n");

Double-printk when KVM isn't detected seems overkill.  Perhaps you could
just fold this all into one function...

(Personal gripe: I consider a variable named "rc" to be an admission of
semantic defeat... "err" would be better here...)

Thanks!
Rusty.

Re: [kvm-devel] [PATCH 1/3] KVM paravirt_ops infrastructure

[Anthony Liguori]

Rusty Russell wrote:
> On Wed, 2007-05-30 at 09:52 -0500, Anthony Liguori wrote:
>   
>> This patch adds the basic infrastructure for paravirtualizing a KVM
>> guest.
>>     
>
> Hi Anthony!
>
> 	Nice patch, comments below.
>
>   
>> Discovery of running under KVM is done by sharing a page of memory
>> between
>> the guest and host (initially through an MSR write).
>>     
>
> I missed the shared page in this patch?  If you are going to do that,
> perhaps putting the hypercall magic in that page is a good idea?
>   

para_state is the shared page.  The address is passed to the KVM via the 
MSR (so it's a shared page owned by the guest).

>> +extern unsigned char hypercall_addr[4];
>>     
>
> Perhaps in a header?
>
>   
>> +asm (
>> +       ".globl hypercall_addr\n"
>> +       ".align 4\n"
>> +       "hypercall_addr:\n"
>> +       "movl $-38, %eax\n"
>> +       "ret\n"
>> +);
>>     
>
> I don't think we want the hypercall returning Linux error numbers, and
> magic numbers are bad too.  ud2 here I think.
>   

Yeah, you're not the first one to suggest this.  The thing is, KVM 
already has host-side support for a hypercall API.  I didn't want to 
change that unless I had to.  However, based on the prior feedback re: 
using CPUID, I will be changing it so I'll update this too.

>> +       para_state->guest_version = KVM_PARA_API_VERSION;
>> +       para_state->host_version = -1;
>> +       para_state->size = sizeof(*para_state);
>> +       para_state->ret = 0;
>> +       para_state->hypercall_gpa = __pa(hypercall_addr);
>>     
>
> Two versions, size *and* ret?  This seems like overkill...
>   

Yeah, I agree :-)  I actually am not a huge fan of using version 
numbers.  I think I'm going to try the next patch using a single version 
number and a feature bitmap.  Some of the optimizations (like MMU 
batching) don't make sense in a NPT/EPT environment but the guest 
shouldn't have to be aware of that.

>> +       if (wrmsr_safe(MSR_KVM_API_MAGIC, __pa(para_state), 0)) {
>> +               printk(KERN_INFO "KVM guest: WRMSR probe failed.\n");
>> +               return -ENOENT;
>> +       }
>>     
>
> How about printk(KERN_INFO "I am not a KVM guest\n");?
>
>   
>> +static int __init kvm_guest_init(void)
>> +{
>> +       int rc;
>> +
>> +       rc = kvm_guest_register_para(smp_processor_id());
>> +       if (rc) {
>> +               printk(KERN_INFO "paravirt KVM unavailable\n");
>>     
>
> Double-printk when KVM isn't detected seems overkill.  Perhaps you could
> just fold this all into one function...
>   

Already have.

> (Personal gripe: I consider a variable named "rc" to be an admission of
> semantic defeat... "err" would be better here...)
>   

I'm not sure I agree that's one's better than the other.  Although I 
guess if (err) { reads a little better...

Regards,

Anthony Liguori

> Thanks!
> Rusty.
>
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by DB2 Express
> Download DB2 Express C - the FREE version of DB2 express and take
> control of your XML. No limits. Just data. Click to get it now.
> http://sourceforge.net/powerbar/db2/
> _______________________________________________
> kvm-devel mailing list
> kvm-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/kvm-devel
>
>   

Re: [PATCH 1/3] KVM paravirt_ops infrastructure

[Avi Kivity]

Rusty Russell wrote:
> Two versions, size *and* ret?  This seems like overkill...
>
>   

I think we ought to move away from version numbers and use feature 
availability flags instead.

>> +       if (rc) {
>> +               printk(KERN_INFO "paravirt KVM unavailable\n");
>>     
>
> Double-printk when KVM isn't detected seems overkill.  Perhaps you could
> just fold this all into one function...
>
> (Personal gripe: I consider a variable named "rc" to be an admission of
> semantic defeat... "err" would be better here...)
>   

I just use 'r' (short for 'argh').


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


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH 1/3] KVM paravirt_ops infrastructure

[Andi Kleen]

> > +       if (wrmsr_safe(MSR_KVM_API_MAGIC, __pa(para_state), 0)) {
> > +               printk(KERN_INFO "KVM guest: WRMSR probe failed.\n");
> > +               return -ENOENT;
> > +       }
> 
> How about printk(KERN_INFO "I am not a KVM guest\n");?

Actually paravirt probes that fail should be silent; similar like
drivers that don't find their hardware should do the same. Otherwise
if there are later distro kernels with various of those compiled
in the boot log would become quite noisy

-andi

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH 1/3] KVM paravirt_ops infrastructure

[Ingo Molnar]

* Andi Kleen <ak-l3A5Bk7waGM@public.gmane.org> wrote:

> 
> > > +       if (wrmsr_safe(MSR_KVM_API_MAGIC, __pa(para_state), 0)) {
> > > +               printk(KERN_INFO "KVM guest: WRMSR probe failed.\n");
> > > +               return -ENOENT;
> > > +       }
> > 
> > How about printk(KERN_INFO "I am not a KVM guest\n");?
> 
> Actually paravirt probes that fail should be silent; similar like 
> drivers that don't find their hardware should do the same. Otherwise 
> if there are later distro kernels with various of those compiled in 
> the boot log would become quite noisy

yeah. I suspect printing that it's executing in native mode is OK.

	Ingo

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH 1/3] KVM paravirt_ops infrastructure

[Andi Kleen]

On Thursday 31 May 2007 12:11:16 Ingo Molnar wrote:
> 
> * Andi Kleen <ak-l3A5Bk7waGM@public.gmane.org> wrote:
> 
> > 
> > > > +       if (wrmsr_safe(MSR_KVM_API_MAGIC, __pa(para_state), 0)) {
> > > > +               printk(KERN_INFO "KVM guest: WRMSR probe failed.\n");
> > > > +               return -ENOENT;
> > > > +       }
> > > 
> > > How about printk(KERN_INFO "I am not a KVM guest\n");?
> > 
> > Actually paravirt probes that fail should be silent; similar like 
> > drivers that don't find their hardware should do the same. Otherwise 
> > if there are later distro kernels with various of those compiled in 
> > the boot log would become quite noisy
> 
> yeah. I suspect printing that it's executing in native mode is OK.

But only a single printk for that please

-Andi

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH 1/3] KVM paravirt_ops infrastructure

[Rusty Russell]

On Thu, 2007-05-31 at 12:40 +0200, Andi Kleen wrote:
> On Thursday 31 May 2007 12:11:16 Ingo Molnar wrote:
> > 
> > * Andi Kleen <ak-l3A5Bk7waGM@public.gmane.org> wrote:
> > 
> > > 
> > > > > +       if (wrmsr_safe(MSR_KVM_API_MAGIC, __pa(para_state), 0)) {
> > > > > +               printk(KERN_INFO "KVM guest: WRMSR probe failed.\n");
> > > > > +               return -ENOENT;
> > > > > +       }
> > > > 
> > > > How about printk(KERN_INFO "I am not a KVM guest\n");?
> > > 
> > > Actually paravirt probes that fail should be silent; similar like 
> > > drivers that don't find their hardware should do the same. Otherwise 
> > > if there are later distro kernels with various of those compiled in 
> > > the boot log would become quite noisy
> > 
> > yeah. I suspect printing that it's executing in native mode is OK.
> 
> But only a single printk for that please

Which already exists in paravirt.c:

static void __init default_banner(void)
{
	printk(KERN_INFO "Booting paravirtualized kernel on %s\n",
	       paravirt_ops.name);
}
...
static int __init print_banner(void)
{
	paravirt_ops.banner();
	return 0;
}
core_initcall(print_banner);

Hmm, this will predate your kvm initcall tho, and making that a
pure_initcall is pretty gross...

Cheers,
Rusty.


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [kvm-devel] [PATCH 1/3] KVM paravirt_ops infrastructure

[Anthony Liguori]

Rusty Russell wrote:
> On Thu, 2007-05-31 at 12:40 +0200, Andi Kleen wrote:
>   
>> On Thursday 31 May 2007 12:11:16 Ingo Molnar wrote:
>>     
>>> * Andi Kleen <ak@suse.de> wrote:
>>>
>>>       
>>>>>> +       if (wrmsr_safe(MSR_KVM_API_MAGIC, __pa(para_state), 0)) {
>>>>>> +               printk(KERN_INFO "KVM guest: WRMSR probe failed.\n");
>>>>>> +               return -ENOENT;
>>>>>> +       }
>>>>>>             
>>>>> How about printk(KERN_INFO "I am not a KVM guest\n");?
>>>>>           
>>>> Actually paravirt probes that fail should be silent; similar like 
>>>> drivers that don't find their hardware should do the same. Otherwise 
>>>> if there are later distro kernels with various of those compiled in 
>>>> the boot log would become quite noisy
>>>>         
>>> yeah. I suspect printing that it's executing in native mode is OK.
>>>       
>> But only a single printk for that please
>>     
>
> Which already exists in paravirt.c:
>
> static void __init default_banner(void)
> {
> 	printk(KERN_INFO "Booting paravirtualized kernel on %s\n",
> 	       paravirt_ops.name);
> }
> ...
> static int __init print_banner(void)
> {
> 	paravirt_ops.banner();
> 	return 0;
> }
> core_initcall(print_banner);
>
> Hmm, this will predate your kvm initcall tho, and making that a
> pure_initcall is pretty gross...
>   

My current patch uses core_initcall() and presumably works by magic of 
linking order.

Any other suggestions over core_initcall()?

Regards,

Anthony Liguori

> Cheers,
> Rusty.
>
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by DB2 Express
> Download DB2 Express C - the FREE version of DB2 express and take
> control of your XML. No limits. Just data. Click to get it now.
> http://sourceforge.net/powerbar/db2/
> _______________________________________________
> kvm-devel mailing list
> kvm-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/kvm-devel
>
>   

Re: [PATCH 1/3] KVM paravirt_ops infrastructure

[Anthony Liguori]

Andi Kleen wrote:
> On Thursday 31 May 2007 12:11:16 Ingo Molnar wrote:
>   
>> yeah. I suspect printing that it's executing in native mode is OK.
>>     
>
> But only a single printk for that please
>   

This will be taken care of in the next round of patches.

Regards,

Anthony Liguori

> -Andi
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by DB2 Express
> Download DB2 Express C - the FREE version of DB2 express and take
> control of your XML. No limits. Just data. Click to get it now.
> http://sourceforge.net/powerbar/db2/
> _______________________________________________
> kvm-devel mailing list
> kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/kvm-devel
>
>   


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

[PATCH 2/3][PARAVIRT] Make IO delay a NOP

[Anthony Liguori]

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

Regards,

Anthony Liguori

[-- Attachment #2: paravirt-no-iodelay.diff --]
[-- Type: text/x-patch, Size: 874 bytes --]

Subject: [PATCH][PARAVIRT] Make IO delay a NOP for paravirt guests

No delay is required in between PIO operations under KVM guests so make IO
delay a NOP.  This was originally part of Ingo Molnar's paravirt series.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Index: kvm/arch/i386/kernel/kvm.c
===================================================================
--- kvm.orig/arch/i386/kernel/kvm.c	2007-05-30 09:30:42.000000000 -0500
+++ kvm/arch/i386/kernel/kvm.c	2007-05-30 09:31:46.000000000 -0500
@@ -28,9 +28,17 @@
 static DEFINE_PER_CPU(struct kvm_vcpu_para_state, para_state);
 extern unsigned char hypercall_addr[4];
 
+/*
+ * No need for any "IO delay" on KVM
+ */
+static void kvm_io_delay(void)
+{
+}
+
 static void kvm_guest_setup(void)
 {
 	paravirt_ops.name = "KVM";
+	paravirt_ops.io_delay = kvm_io_delay;
 	paravirt_ops.paravirt_enabled = 1;
 }
 

[-- Attachment #3: Type: text/plain, Size: 184 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

[PATCH 3/3] Eliminate read_cr3 on TLB flush

[Anthony Liguori]

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

Regards,

Anthony Liguori

[-- Attachment #2: paravirt-no-read-cr3-flush.diff --]
[-- Type: text/x-patch, Size: 1619 bytes --]

Subject: [PATCH][PARAVIRT] Eliminate unnecessary CR3 read in TLB flush

This patch eliminates the CR3 read (which would cause a VM exit) in the TLB
flush path.  The patch is based on Ingo Molnar's paravirt series.

Signed-off-by: Anthony Liguori <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Index: kvm/arch/i386/kernel/kvm.c
===================================================================
--- kvm.orig/arch/i386/kernel/kvm.c	2007-05-30 09:13:48.000000000 -0500
+++ kvm/arch/i386/kernel/kvm.c	2007-05-30 09:14:24.000000000 -0500
@@ -24,11 +24,35 @@
 #include <linux/kernel.h>
 #include <linux/kvm_para.h>
 #include <linux/cpu.h>
+#include <linux/mm.h>
+#include <asm/processor.h>
+#include <asm/tlbflush.h>
 
 static DEFINE_PER_CPU(struct kvm_vcpu_para_state, para_state);
 extern unsigned char hypercall_addr[4];
 
 /*
+ * Avoid the VM exit upon cr3 load by using the cached
+ * ->active_mm->pgd value:
+ */
+static void kvm_flush_tlb_user(void)
+{
+	write_cr3(__pa(current->active_mm->pgd));
+}
+
+/*
+ * Avoid VM exit for cr3 read by calling into kvm_flush_tlb_user
+ */
+static fastcall void kvm_flush_tlb_kernel(void)
+{
+	unsigned long orig_cr4 = read_cr4();
+
+	write_cr4(orig_cr4 & ~X86_CR4_PGE);
+	kvm_flush_tlb_user();
+	write_cr4(orig_cr4);
+}
+
+/*
  * No need for any "IO delay" on KVM
  */
 static void kvm_io_delay(void)
@@ -38,6 +62,8 @@
 static void kvm_guest_setup(void)
 {
 	paravirt_ops.name = "KVM";
+	paravirt_ops.flush_tlb_user = kvm_flush_tlb_user;
+	paravirt_ops.flush_tlb_kernel = kvm_flush_tlb_kernel;
 	paravirt_ops.io_delay = kvm_io_delay;
 	paravirt_ops.paravirt_enabled = 1;
 }

[-- Attachment #3: Type: text/plain, Size: 286 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH 3/3] Eliminate read_cr3 on TLB flush

[Andi Kleen]

On Wednesday 30 May 2007 16:53:41 Anthony Liguori wrote:
> Subject: [PATCH][PARAVIRT] Eliminate unnecessary CR3 read in TLB flush
> 
> This patch eliminates the CR3 read (which would cause a VM exit) in the TLB
> flush path.  The patch is based on Ingo Molnar's paravirt series.
> 

This change could be just done generically for the native architecture, couldn't 
it?

-Andi

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH 3/3] Eliminate read_cr3 on TLB flush

[Anthony Liguori]

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

Andi Kleen wrote:
> On Wednesday 30 May 2007 16:53:41 Anthony Liguori wrote:
>   
>> Subject: [PATCH][PARAVIRT] Eliminate unnecessary CR3 read in TLB flush
>>
>> This patch eliminates the CR3 read (which would cause a VM exit) in the TLB
>> flush path.  The patch is based on Ingo Molnar's paravirt series.
>>
>>     
>
> This change could be just done generically for the native architecture, couldn't 
> it?
>   

Sure.  It adds a few more cycles onto native though (two memory reads, 
and some math).  How does the following look?

Regards,

Anthony Liguori

> -Andi
>   


[-- Attachment #2: no-read-cr3-on-tlbflush.diff --]
[-- Type: text/x-patch, Size: 2122 bytes --]

Subject: [PATCH] Avoid reading CR3 on TLB flush
From: Anthony Liguori <aliguori@us.ibm.com>

In a virtualized environment, there is significant overhead in reading and
writing control registers.  Since we already have the value of CR3 in
current->active_mm->pgd, we can avoid taking the exit on CR3 read when
doing a TLB flush.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Index: kvm/include/asm-i386/tlbflush.h
===================================================================
--- kvm.orig/include/asm-i386/tlbflush.h	2007-05-30 10:22:48.000000000 -0500
+++ kvm/include/asm-i386/tlbflush.h	2007-05-30 10:22:54.000000000 -0500
@@ -14,13 +14,12 @@
 
 #define __native_flush_tlb()						\
 	do {								\
-		unsigned int tmpreg;					\
+		unsigned int cr3 = __pa(current->active_mm->pgd);	\
 									\
 		__asm__ __volatile__(					\
-			"movl %%cr3, %0;              \n"		\
 			"movl %0, %%cr3;  # flush TLB \n"		\
-			: "=r" (tmpreg)					\
-			:: "memory");					\
+			:: "r" (cr3)				       	\
+			: "memory");					\
 	} while (0)
 
 /*
@@ -29,18 +28,18 @@
  */
 #define __native_flush_tlb_global()					\
 	do {								\
-		unsigned int tmpreg, cr4, cr4_orig;			\
+		unsigned int cr3 = __pa(current->active_mm->pgd);	\
+		unsigned int cr4, cr4_orig;				\
 									\
 		__asm__ __volatile__(					\
-			"movl %%cr4, %2;  # turn off PGE     \n"	\
-			"movl %2, %1;                        \n"	\
-			"andl %3, %1;                        \n"	\
-			"movl %1, %%cr4;                     \n"	\
-			"movl %%cr3, %0;                     \n"	\
-			"movl %0, %%cr3;  # flush TLB        \n"	\
-			"movl %2, %%cr4;  # turn PGE back on \n"	\
-			: "=&r" (tmpreg), "=&r" (cr4), "=&r" (cr4_orig)	\
-			: "i" (~X86_CR4_PGE)				\
+			"movl %%cr4, %1;  # turn off PGE     \n"	\
+			"movl %1, %0;                        \n"	\
+			"andl %2, %0;                        \n"	\
+			"movl %0, %%cr4;                     \n"	\
+			"movl %3, %%cr3;  # flush TLB        \n"	\
+			"movl %1, %%cr4;  # turn PGE back on \n"	\
+			: "=&r" (cr4), "=&r" (cr4_orig)			\
+			: "i" (~X86_CR4_PGE), "r" (cr3)			\
 			: "memory");					\
 	} while (0)
 

[-- Attachment #3: Type: text/plain, Size: 184 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [PATCH 3/3] Eliminate read_cr3 on TLB flush

[Jeremy Fitzhardinge]

Anthony Liguori wrote:
> Sure.  It adds a few more cycles onto native though (two memory reads,
> and some math).

As opposed to a serializing control-register read?  I think that's
probably a win.

    J

RE: [kvm-devel] [PATCH 3/3] Eliminate read_cr3 on TLB flush

[Nakajima, Jun]

Jeremy Fitzhardinge wrote:
> Anthony Liguori wrote:
> > Sure.  It adds a few more cycles onto native though (two memory
reads,
> > and some math).
> 
> As opposed to a serializing control-register read?  I think that's
> probably a win.
> 
>     J
> 

And actually you don't need the write to CR3 to flush TLB because the
one to CR4 does it. Or does kvm_flush_tlb_kernel assume that CR3 is
updated at the same time?

Jun
---
Intel Open Source Technology Center