KVM paravirt_ops implementation

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.

Quoted 6 lines · 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

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

Regards,

Anthony Liguori

Quoted 1 line · Anthony Liguori

Sure. It adds a few more cycles onto native though (two memory

Quoted 1 line

and some math).

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

J

Quoted 1 line · Anthony Liguori

Sure. It adds a few more cycles onto native though (two memory

Quoted 1 line

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

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

Quoted 4 lines · Anthony Liguori

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

Quoted 8 lines · Jun

Quoted 4 lines · Anthony Liguori

Regards,

Anthony Liguori

I think we should use the CPUID instruction (leaf 0x40000000) to

Quoted 2 lines

the hypervosor as we are doing in Xen.

Is that leaf reserved for such use by Intel?

Regards,

Anthony Liguori

Quoted 1 line · Jun

And actually you don't need the write to CR3 to flush TLB because

Quoted 1 line

one to CR4 does it. Or does kvm_flush_tlb_kernel assume that CR3 is

Quoted 3 lines

at the same time?

Jun

It should not be necessary, but I believe this was added as a

to a PII erratum. I can't find the erratum, however, and the history

using G bits in Linux is complicated (several bugs introduced and many intermediate versions of this code). Since this is not performance critical, I think it is probably best to leave the CR3 reload.

However, being unnecessary on modern processors, I already submitted a patch to eliminate it on 64-bit (or maybe just told Andi about it, I can't recall).

Zach

Quoted 1 line · Jun

And actually you don't need the write to CR3 to flush TLB because

Quoted 1 line

one to CR4 does it. Or does kvm_flush_tlb_kernel assume that CR3 is

Quoted 3 lines

at the same time?

Jun

It should not be necessary, but I believe this was added as a

to a PII erratum. I can't find the erratum, however, and the history

using G bits in Linux is complicated (several bugs introduced and many intermediate versions of this code). Since this is not performance critical, I think it is probably best to leave the CR3 reload.

However, being unnecessary on modern processors, I already submitted a patch to eliminate it on 64-bit (or maybe just told Andi about it, I can't recall).

Zach

Quoted 4 lines · Zachary Amsden

Quoted 2 lines · Jun

And actually you don't need the write to CR3 to flush TLB because

the

Quoted 2 lines

Quoted 2 lines

one to CR4 does it. Or does kvm_flush_tlb_kernel assume that CR3 is

updated

Quoted 6 lines

Quoted 4 lines

at the same time?

Jun

It should not be necessary, but I believe this was added as a

workaround

Quoted 2 lines

to a PII erratum. I can't find the erratum, however, and the history

of

Quoted 4 lines

using G bits in Linux is complicated (several bugs introduced and many intermediate versions of this code). Since this is not performance critical, I think it is probably best to leave the CR3 reload.

I don't recommend this for old processors.

Quoted 4 lines

However, being unnecessary on modern processors, I already submitted a patch to eliminate it on 64-bit (or maybe just told Andi about it, I can't recall).

Quoted 2 lines

Zach

For KVM, it should be okay as well. But we can replace two CR4 accesses with just one hypercall.

Jun
---
Intel Open Source Technology Center

Nakajima, Jun wrote:

Quoted 1 line

For KVM, it should be okay as well. But we can replace two CR4

Quoted 2 lines

with just one hypercall.

I was thinking the same thing :-)

I was actually thinking about adding a hypercall to set/clear a bit in

control register. The thought here is that it would be useful not

for the global bit but also for CR0.TS although we would need another paravirt_op hook for stts.

Regards,

Anthony Liguori

Nakajima, Jun wrote:

Quoted 1 line

For KVM, it should be okay as well. But we can replace two CR4

Quoted 2 lines

with just one hypercall.

I was thinking the same thing :-)

I was actually thinking about adding a hypercall to set/clear a bit in

control register. The thought here is that it would be useful not

for the global bit but also for CR0.TS although we would need another paravirt_op hook for stts.

Regards,

Anthony Liguori

I was thinking the same thing :-)

I was actually thinking about adding a hypercall to set/clear a bit in a control register. The thought here is that it would be useful not just for the global bit but also for CR0.TS although we would need another paravirt_op hook for stts.

I was thinking the same thing :-)

I was actually thinking about adding a hypercall to set/clear a bit in a control register. The thought here is that it would be useful not just for the global bit but also for CR0.TS although we would need another paravirt_op hook for stts.

Anthony Liguori wrote:

Given the optimizations for CPU virtualization in the current H/W, I'm not sure if such hooks are useful. Do you have any performance data that justify such hooks?

Quoted 5 lines

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 [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Anthony Liguori wrote:

Given the optimizations for CPU virtualization in the current H/W, I'm not sure if such hooks are useful. Do you have any performance data that justify such hooks?

Quoted 5 lines

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
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

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

+extern unsigned char hypercall_addr[4];

I was actually thinking about adding a hypercall to set/clear a bit in a control register. The thought here is that it would be useful not just for the global bit but also for CR0.TS although we would need another paravirt_op hook for stts.

Quoted 3 lines · Anthony Liguori

This patch adds the basic infrastructure for paravirtualizing a KVM guest.

Hi Anthony!

Nice patch, comments below.

Quoted 4 lines

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?

Quoted 2 lines

+extern unsigned char hypercall_addr[4];

Perhaps in a header?

arch/i386/kernel/kvm.c line 47

Quoted 8 lines

arch/i386/kernel/kvm.c new file +7 -0

+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.

arch/i386/kernel/kvm.c line 65

Quoted 6 lines

arch/i386/kernel/kvm.c new file +5 -0

+       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...

arch/i386/kernel/kvm.c line 70

Quoted 5 lines

arch/i386/kernel/kvm.c new file +4 -0

+       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");?

arch/i386/kernel/kvm.c line 92

Quoted 8 lines

arch/i386/kernel/kvm.c new file +7 -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");

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.

-------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

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

arch/i386/kernel/kvm.c line 92

Quoted 3 lines

arch/i386/kernel/kvm.c new file +2 -0

+       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...)

Quoted 6 lines

Quoted 2 lines

For KVM, it should be okay as well. But we can replace two CR4 accesses with just one hypercall.

I was thinking the same thing :-)

I was actually thinking about adding a hypercall to set/clear a bit in a control register. The thought here is that it would be useful not just for the global bit but also for CR0.TS although we would need another paravirt_op hook for stts.

Quoted 6 lines

Quoted 2 lines

For KVM, it should be okay as well. But we can replace two CR4 accesses with just one hypercall.

I was thinking the same thing :-)

I was actually thinking about adding a hypercall to set/clear a bit in a control register. The thought here is that it would be useful not just for the global bit but also for CR0.TS although we would need another paravirt_op hook for stts.

Quoted 7 lines · Anthony Liguori

Quoted 4 lines

Quoted 1 line

For KVM, it should be okay as well. But we can replace two CR4 accesses with just one hypercall.

I was thinking the same thing :-)

Quoted 5 lines

I was actually thinking about adding a hypercall to set/clear a bit in a control register. The thought here is that it would be useful not just for the global bit but also for CR0.TS although we would need another paravirt_op hook for stts.

Are global tlb flushes frequent enough to warrant optimization?

arch/i386/kernel/kvm.c line 70

Quoted 5 lines

arch/i386/kernel/kvm.c new file +4 -0

+       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");?

Quoted 6 lines

Quoted 5 lines

arch/i386/kernel/kvm.c new file +4 -0

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

Quoted 1 line

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

Quoted 6 lines · Andi Kleen

Quoted 6 lines

Quoted 5 lines

(quoted diff) +4 -0

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

Quoted 6 lines

Quoted 1 line

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.

Quoted 11 lines · On Thursday 31 May 2007 12:11:16 Ingo Molnar wrote:

Quoted 8 lines · Andi Kleen

Quoted 7 lines

Review

Quoted 5 lines

(quoted diff) +4 -0

+       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");?

Quoted 6 lines

Quoted 4 lines

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

Quoted 10 lines · Andi Kleen

Quoted 8 lines · On Thursday 31 May 2007 12:11:16 Ingo Molnar wrote:

Quoted 5 lines · Andi Kleen

Quoted 5 lines

Quoted 5 lines

(quoted diff) +4 -0

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

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

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/
_______________________________________________
kvm-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Quoted 2 lines · 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

-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 [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Quoted 10 lines · Anthony Liguori

Quoted 8 lines

Quoted 3 lines

For KVM, it should be okay as well. But we can replace two CR4 accesses with just one hypercall.

I was thinking the same thing :-)

Quoted 6 lines

I was actually thinking about adding a hypercall to set/clear a bit in a control register. The thought here is that it would be useful not just for the global bit but also for CR0.TS although we would need another paravirt_op hook for stts.

Are global tlb flushes frequent enough to warrant optimization?

Quoted 8 lines · Anthony Liguori

Quoted 8 lines

Quoted 3 lines

For KVM, it should be okay as well. But we can replace two CR4 accesses with just one hypercall.

Quoted 5 lines

I was thinking the same thing :-)

I was actually thinking about adding a hypercall to set/clear a bit in a control register. The thought here is that it would be useful not just for the global bit but also for CR0.TS although we would need another paravirt_op hook for stts.

Are global tlb flushes frequent enough to warrant optimization?

Quoted 5 lines · Anthony Liguori

Quoted 3 lines · Jun

Anthony Liguori wrote:

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

Regards,

Anthony Liguori

I think we should use the CPUID instruction (leaf 0x40000000) to

detect

Quoted 3 lines

Quoted 3 lines

the hypervosor as we are doing in Xen.

Quoted 3 lines

Is that leaf reserved for such use by Intel?

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

Quoted 5 lines

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 [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Quoted 1 line · Jun

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

Quoted 1 line

I've switched to using CPUID. I noticed that Xen also uses

so I'm presuming that that is okay too?

Regards,

Anthony Liguori