KVM paravirt_ops implementation

(quoted diff) +2 -3

-		nr = vcpu->regs[VCPU_REGS_RBX] & -1u;
-		a0 = vcpu->regs[VCPU_REGS_RAX] & -1u;
+		nr = vcpu->regs[VCPU_REGS_RAX] & -1u;
+		a0 = vcpu->regs[VCPU_REGS_RBX] & -1u;
 
 
- * Each hypercall may have 0-6 parameters.

Quoted 11 lines · Anthony Liguori

(quoted diff) +2 -7

-        nr = vcpu->regs[VCPU_REGS_RBX] & -1u;
-        a0 = vcpu->regs[VCPU_REGS_RAX] & -1u;
+        nr = vcpu->regs[VCPU_REGS_RAX] & -1u;
+        a0 = vcpu->regs[VCPU_REGS_RBX] & -1u;
 
 
- * Each hypercall may have 0-6 parameters.
- *
- * 64-bit hypercall index is in RAX, goes from 0 to __NR_hypercalls-1
- *
- * 64-bit parameters 1-6 are in the standard gcc x86_64 calling 

Quoted 7 lines

convention
- * order: RDI, RSI, RDX, RCX, R8, R9.
+ * Each hypercall may have 0-4 parameters.
*
- * 32-bit index is EBX, parameters are: EAX, ECX, EDX, ESI, EDI, EBP.
- * (the first 3 are according to the gcc regparm calling convention)
+ * 32-bit index is EAX, parameters are: EBX, ECX, EDX, ESI.

What's the motivation for these changes?

Quoted 2 lines

What's the motivation for these changes?

If we're queuing hypercalls, then having 4 arguments verses 6 means that we can queue 50% more hypercalls in a single page. Using all six arguments clobbers all the GP registers in 32-bit mode too.

...

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

+static __init struct kvm_paravirt_state *paravirt_alloc_state(void)
+{
+       struct kvm_paravirt_state *state;
+
+       state = (void *)get_zeroed_page(GFP_KERNEL);
+       if (!state)
+               goto err;
+
+       state->vmca = (void *)get_zeroed_page(GFP_KERNEL);
+       if (!state->vmca)
+               goto err;
+
+       /* FIXME: what do I need for this to be executable on 64 bit? */
+       state->hypercall = (void *)get_zeroed_page(GFP_KERNEL);

Why do you alloc a page for the hypercall instead of using Ingo's code below? This way it can work for 64 bit too.

Ingo's code:
/*
* 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"

);

Quoted 25 lines · Dor Laor

...

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

+static __init struct kvm_paravirt_state *paravirt_alloc_state(void)
+{
+       struct kvm_paravirt_state *state;
+
+       state = (void *)get_zeroed_page(GFP_KERNEL);
+       if (!state)
+               goto err;
+
+       state->vmca = (void *)get_zeroed_page(GFP_KERNEL);
+       if (!state->vmca)
+               goto err;
+
+       /* FIXME: what do I need for this to be executable on 64 bit? */
+       state->hypercall = (void *)get_zeroed_page(GFP_KERNEL);

Why do you alloc a page for the hypercall instead of using Ingo's code below? This way it can work for 64 bit too.

Ingo's code:
/*
* This is the vm-syscall address - to be patched by the host to
* VMCALL (Intel) or VMMCALL (AMD), depending on the CPU model:
*/
asm (

Quoted 6 lines

        "       .globl hypercall_addr                   \n"
        "       .align 4                                \n"
        "       hypercall_addr:                         \n"
        "               movl $-38, %eax                 \n"
        "               ret                             \n"

);

The assembly code "movl $-38, %eax; \nret" is only a "reserved place", which is later overwritten by hypercall address from the host, isnt it?

If so, why dont we simply put 4 NOPs there?

Thanks,
Jun

...

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

+static __init struct kvm_paravirt_state *paravirt_alloc_state(void)
+{
+	struct kvm_paravirt_state *state;
+
+	state = (void *)get_zeroed_page(GFP_KERNEL);
+	if (!state)
+		goto err;
+
+	state->vmca = (void *)get_zeroed_page(GFP_KERNEL);
+	if (!state->vmca)
+		goto err;
+
+	/* FIXME: what do I need for this to be executable on 64 bit? */
+	state->hypercall = (void *)get_zeroed_page(GFP_KERNEL);

Why do you alloc a page for the hypercall instead of using Ingo's code below? This way it can work for 64 bit too.

Ingo's code:
/*
* 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"
);
extern unsigned char hypercall_addr[6];

And use it this way: (I used vmalloc_to_page since its compiles as a module)
hypercall_addr_page = vmalloc_to_page(hypercall_addr); para_state->hypercall_gpa = page_to_pfn(hypercall_addr_page) << PAGE_SHIFT |

				    offset_in_page(hypercall_addr);

Regards,
Dor.

With hardware virtualization, CR reads often times require a VMEXIT which is rather expensive. Instead of reading CR and taking the VMEXIT, maintain a copy of each CR and return that on CR reads.

Quoted 6 lines · Anthony Liguori

With hardware virtualization, CR reads often times require a VMEXIT which is
rather expensive. Instead of reading CR and taking the VMEXIT, maintain a
copy of each CR and return that on CR reads.

Looks good. Any measurable performance impact?

arch/i386/kernel/kvm.c +13 -0

+/*
+ * Control register reads can be trapped.  Since trapping is relatively
+ * expensive, we can avoid paying the cost by caching logically.
+ */
+static unsigned long kvm_read_cr(int reg)
+{
+	struct kvm_paravirt_state *state
+		= per_cpu(paravirt_state, smp_processor_id());
+
+	if (unlikely(!state->cr_valid[reg])) {
+		if (reg == 0)
+			state->cached_cr[reg] = native_read_cr0();
+		else if (reg == 3)

Quoted 20 lines · Anthony Liguori

arch/i386/kernel/kvm.c +20 -0

+/*
+ * Control register reads can be trapped.  Since trapping is relatively
+ * expensive, we can avoid paying the cost by caching logically.
+ */
+static unsigned long kvm_read_cr(int reg)
+{
+    struct kvm_paravirt_state *state
+        = per_cpu(paravirt_state, smp_processor_id());
+
+    if (unlikely(!state->cr_valid[reg])) {
+        if (reg == 0)
+            state->cached_cr[reg] = native_read_cr0();
+        else if (reg == 3)
+            state->cached_cr[reg] = native_read_cr3();
+        else if (reg == 4)
+            state->cached_cr[reg] = native_read_cr4();
+        else
+            BUG();
+        state->cr_valid[reg] = 1;
+    }

arch/i386/kernel/kvm.c static void kvm_io_delay(void) · line 100

Quoted 3 lines

arch/i386/kernel/kvm.c +3 -0

+    return state->cached_cr[reg];
+}
+

It would be good to declare this (and kvm_write_cr) always_inline. These functions are never called with a non-constant reg parameters, and the unsightly if tree (more readable as a switch, IMO) will fold nicely when inlined.

drivers/kvm/kvm_main.c +11 -0

+static int kvm_hypercall_mmu_write(struct kvm_vcpu *vcpu, gva_t addr,
+				   unsigned long size, unsigned long a0,
+				   unsigned long a1)
+{
+	gpa_t gpa = vcpu->mmu.gva_to_gpa(vcpu, addr);
+	u64 value;
+
+	if (gpa == UNMAPPED_GVA)
+		return -EFAULT;
+	if (size == 1) {
+		if (!emulator_write_phys(vcpu, gpa, &a0, sizeof(a0)))

Quoted 18 lines · Anthony Liguori

drivers/kvm/kvm_main.c +18 -0

+static int kvm_hypercall_mmu_write(struct kvm_vcpu *vcpu, gva_t addr,
+                   unsigned long size, unsigned long a0,
+                   unsigned long a1)
+{
+    gpa_t gpa = vcpu->mmu.gva_to_gpa(vcpu, addr);
+    u64 value;
+
+    if (gpa == UNMAPPED_GVA)
+        return -EFAULT;
+    if (size == 1) {
+        if (!emulator_write_phys(vcpu, gpa, &a0, sizeof(a0)))
+            return -EFAULT;
+    } else if (size == 2) {
+        value = (u64)a1 << 32 | a0;
+        if (!emulator_write_phys(vcpu, gpa, &value, sizeof(value)))
+            return -EFAULT;
+    } else
+        return -E2BIG;

drivers/kvm/kvm_main.c int kvm_emulate_halt(struct kvm_vcpu *vcpu) · line 1370

Quoted 3 lines

drivers/kvm/kvm_main.c +3 -0

+
+    return 0;
+}

Hypercalls should return kvm-specific error codes (defined in include/linux/kvm_para.h), not Linux error codes, as they could be used in operating systems which have different values for E2BIG and friends.

arch/i386/kernel/kvm.c static void kvm_write_cr4(unsigned long value) · line 201

Quoted 5 lines

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

+static void kvm_pte_clear(struct mm_struct *mm,
+              unsigned long addr, pte_t *ptep)
+{
+    pte_t pte = {0};

Surely there's a nice macro for creating a pte from an int?

Any performance measurement?

Quoted 5 lines

Hypercalls should return kvm-specific error codes (defined in include/linux/kvm_para.h), not Linux error codes, as they could be used in operating systems which have different values for E2BIG and friends.

If Linux's errnos are stable, we could just use them and let a non-Linux guest define a set of KVM_E2BIG, etc.? It just seemed pretty ugly to add a bunch of these.

Quoted 9 lines · Anthony Liguori

Quoted 5 lines

Hypercalls should return kvm-specific error codes (defined in include/linux/kvm_para.h), not Linux error codes, as they could be used in operating systems which have different values for E2BIG and friends.

If Linux's errnos are stable, we could just use them and let a non-Linux guest define a set of KVM_E2BIG, etc.? It just seemed pretty ugly to add a bunch of these.

The ugliness will serve to remind us that this is a potentially non-Linux path.

In this particular case, the names are not present in guest-visible headers, but any other that is made visible must have a non-Linux-dependent name, so I don't see why error numbers should be treated specially.

arch/i386/kernel/kvm.c static void kvm_write_cr4(unsigned long value) · line 201

Quoted 5 lines

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

+static void kvm_pte_clear(struct mm_struct *mm,
+              unsigned long addr, pte_t *ptep)
+{
+    pte_t pte = {0};

Surely there's a nice macro for creating a pte from an int?

Any performance measurement?

Perhaps my grep'ing skills are weak, but I don't seem to see any. Were you thinking of something in particular?

Quoted 3 lines · Anthony Liguori

Perhaps my grep'ing skills are weak, but I don't seem to see any. Were you thinking of something in particular?

__pte(), of course. Sheesh. ;)

J

Regards,

Anthony Liguori
------------------------------------------------------------------------

Subject: [PATCH] KVM: Add hypercall queue for paravirt_ops implementation
Author: Anthony Liguori <aliguori-r/[email protected]>

Implemented a hypercall queue that can be used when paravirt_ops lazy mode is enabled. This patch enables queueing of MMU write operations and CR updates. This results in about a 50% bump in kernbench performance.

Signed-off-by Anthony Liguori <aliguori-r/[email protected]>

arch/i386/kernel/kvm.c +24 -6

index 07ce38e..4b323f1 100644
@@ -33,8 +33,10 @@ struct kvm_paravirt_state
 	unsigned long cached_cr[5];
 	int cr_valid[5];
 
-	struct kvm_vmca *vmca;
+	enum paravirt_lazy_mode mode;
 	struct kvm_hypercall_entry *queue;
+
+	struct kvm_vmca *vmca;
 	void (*hypercall)(void);
 
 	u64 vmca_gpa;

@@ -42,17 +44,17 @@ struct kvm_paravirt_state
 
 static DEFINE_PER_CPU(struct kvm_paravirt_state *, paravirt_state);
 
+static int do_hypercall_batching;
 static int do_mmu_write;
 static int do_cr_read_caching;
 static int do_nop_io_delay;
 static u64 msr_set_vmca;
 
-static long kvm_hypercall(unsigned int nr, unsigned long p1,
-			  unsigned long p2, unsigned long p3,
-			  unsigned long p4)
+static long _kvm_hypercall(struct kvm_paravirt_state *state,
+			   unsigned int nr, unsigned long p1,
+			   unsigned long p2, unsigned long p3,
+			   unsigned long p4)
 {
-	struct kvm_paravirt_state *state
-		= per_cpu(paravirt_state, smp_processor_id());
 	long ret;
 
 	asm volatile("call *(%6) \n\t"

@@ -69,6 +71,55 @@ static long kvm_hypercall(unsigned int nr, unsigned long p1,
 	return ret;
 }
 
+static int can_defer_hypercall(struct kvm_paravirt_state *state,
+			       unsigned int nr)
+{
+	if (state->mode == PARAVIRT_LAZY_MMU) {
+		if (nr == KVM_HYPERCALL_MMU_WRITE)
+			return 1;
+	} else if (state->mode == PARAVIRT_LAZY_CPU) {
+		if (nr == KVM_HYPERCALL_SET_CR)
+			return 1;
+	}
+
+	return 0;
+}
+
+static void _kvm_hypercall_defer(struct kvm_paravirt_state *state,
+				 unsigned int nr,

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

+	entry = &state->queue[state->vmca->queue_index++];
+	entry->nr = nr;
+	entry->p1 = p1;
+	entry->p2 = p2;
+	entry->p3 = p3;

arch/i386/kernel/kvm.c +10 -0

+	long ret = 0;
+
+	if (can_defer_hypercall(state, nr))
+		_kvm_hypercall_defer(state, nr, p1, p2, p3, p4);
+	else
+		ret = _kvm_hypercall(state, nr, p1, p2, p3, p4);
+
+	return ret;
+}
+
 /*

* No need for any "IO delay" on KVM
*/

arch/i386/kernel/kvm.c +4 -1

@@ -107,7 +158,9 @@ static void kvm_write_cr(int reg, unsigned long value)
 	state->cr_valid[reg] = 1;
 	state->cached_cr[reg] = value;
 
-	if (reg == 0)
+	if (state->mode == PARAVIRT_LAZY_CPU)
+		kvm_hypercall(KVM_HYPERCALL_SET_CR, reg, value, 0, 0);
+	else if (reg == 0)
 		native_write_cr0(value);
 	else if (reg == 3)
 		native_write_cr3(value);

@@ -218,6 +271,18 @@ static void kvm_pmd_clear(pmd_t *pmdp)
 	kvm_mmu_write(pmdp, &pmd, sizeof(pmd));
 }
 
+static void kvm_set_lazy_mode(enum paravirt_lazy_mode mode)

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

+}
+
 static void paravirt_ops_setup(void)
 {

arch/i386/kernel/kvm.c +16 -0

 	paravirt_ops.name = "KVM";

@@ -249,6 +314,9 @@ static void paravirt_ops_setup(void)
 		paravirt_ops.set_pud = kvm_set_pud;
 	}
 
+	if (do_hypercall_batching)
+		paravirt_ops.set_lazy_mode = kvm_set_lazy_mode;
+
 	paravirt_ops.paravirt_enabled = 1;
 
 	apply_paravirt(__parainstructions, __parainstructions_end);

@@ -293,6 +361,9 @@ static int paravirt_initialize(void)
 	if ((edx & KVM_FEATURE_MMU_WRITE))
 		do_mmu_write = 1;
 
+	if ((edx & KVM_FEATURE_HYPERCALL_BATCHING))
+		do_hypercall_batching = 1;
+
 	on_each_cpu(paravirt_activate, NULL, 0, 1);
 
 	return 0;

@@ -303,6 +374,9 @@ static __init void paravirt_free_state(struct kvm_paravirt_state *state)
 	if (!state)
 		return;
 
+	if (state->queue)
+		__free_page(pfn_to_page(__pa(state->queue) >> PAGE_SHIFT));
+
 	if (state->hypercall)
 		__free_page(pfn_to_page(__pa(state->hypercall) >> PAGE_SHIFT));
 

@@ -329,8 +403,15 @@ static __init struct kvm_paravirt_state *paravirt_alloc_state(void)
 	if (!state->hypercall)
 		goto err;
 
+	state->queue = (void *)get_zeroed_page(GFP_KERNEL);
+	if (!state->queue)
+		goto err;
+
 	state->vmca_gpa = __pa(state->vmca);
 	state->vmca->hypercall_gpa = __pa(state->hypercall);
+	state->vmca->queue_gpa = __pa(state->queue);
+	state->vmca->max_queue_index
+		= (PAGE_SIZE / sizeof(struct kvm_hypercall_entry));
 
 	return state;
 

drivers/kvm/kvm.h +1 -0

index b08272b..d531899 100644
@@ -291,6 +291,7 @@ struct kvm_vcpu {
 	gpa_t para_state_gpa;
 	struct page *para_state_page;
 	gpa_t hypercall_gpa;
+	struct page *queue_page;
 	unsigned long cr4;
 	unsigned long cr8;
 	u64 pdptrs[4]; /* pae */

drivers/kvm/kvm_main.c +22 -1

index 4f65729..79a2a64 100644
@@ -94,7 +94,8 @@ struct vfsmount *kvmfs_mnt;
 
 #define KVM_PARAVIRT_FEATURES \
 	(KVM_FEATURE_VMCA | KVM_FEATURE_NOP_IO_DELAY | \
-	 KVM_FEATURE_CR_READ_CACHE | KVM_FEATURE_MMU_WRITE)
+	 KVM_FEATURE_CR_READ_CACHE | KVM_FEATURE_MMU_WRITE | \
+	 KVM_FEATURE_HYPERCALL_BATCHING)
 
 #define KVM_MSR_SET_VMCA	0x87655678
 

@@ -1369,6 +1370,24 @@ static int kvm_hypercall_mmu_write(struct kvm_vcpu *vcpu, gva_t addr,
 	return 0;
 }
 
+static int kvm_hypercall_set_cr(struct kvm_vcpu *vcpu,
+				u32 reg, unsigned long value)
+{
+	switch (reg) {
+	case 0:
+		set_cr0(vcpu, value);
+		break;
+	case 3:
+		set_cr3(vcpu, value);
+		break;
+	case 4:
+		set_cr4(vcpu, value);
+		break;
+	}
+
+	return 0;
+}
+
 static int dispatch_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
 			      unsigned long p1, unsigned long p2,
 			      unsigned long p3, unsigned long p4)

@@ -1376,10 +1395,36 @@ static int dispatch_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
 	switch (nr) {
 	case KVM_HYPERCALL_MMU_WRITE:
 		return kvm_hypercall_mmu_write(vcpu, p1, p2, p3, p4);
+	case KVM_HYPERCALL_SET_CR:
+		return kvm_hypercall_set_cr(vcpu, p1, p2);
 	}
 	return -ENOSYS;
 }
 

Quoted 5 lines

drivers/kvm/kvm_main.c +5 -0

+static int kvm_hypercall_flush(struct kvm_vcpu *vcpu)
+{
+	struct kvm_hypercall_entry *queue;
+	struct kvm_vmca *vmca;
+	int ret = 0;

drivers/kvm/kvm_main.c static int dispatch_hypercall(struct kvm_vcpu *vcpu, unsigned long nr, · line 1412

Quoted 6 lines

drivers/kvm/kvm_main.c +4 -0

+	int i;
+
+	queue = kmap(vcpu->queue_page);
+	vmca = kmap(vcpu->para_state_page);

kmap_atomic? Or why not keep them mapped all the time?

Quoted 78 lines · Anthony Liguori

Regards,

Anthony Liguori
------------------------------------------------------------------------

Subject: [PATCH] KVM: Add hypercall queue for paravirt_ops implementation
Author: Anthony Liguori <aliguori-r/[email protected]>

Implemented a hypercall queue that can be used when paravirt_ops lazy mode is enabled. This patch enables queueing of MMU write operations and CR updates. This results in about a 50% bump in kernbench performance.

Signed-off-by Anthony Liguori <aliguori-r/[email protected]>

arch/i386/kernel/kvm.c +29 -6

index 07ce38e..4b323f1 100644
@@ -33,8 +33,10 @@ struct kvm_paravirt_state
 	unsigned long cached_cr[5];
 	int cr_valid[5];
 
-	struct kvm_vmca *vmca;
+	enum paravirt_lazy_mode mode;
 	struct kvm_hypercall_entry *queue;
+
+	struct kvm_vmca *vmca;
 	void (*hypercall)(void);
 
 	u64 vmca_gpa;

@@ -42,17 +44,17 @@ struct kvm_paravirt_state
 
 static DEFINE_PER_CPU(struct kvm_paravirt_state *, paravirt_state);
 
+static int do_hypercall_batching;
 static int do_mmu_write;
 static int do_cr_read_caching;
 static int do_nop_io_delay;
 static u64 msr_set_vmca;
 
-static long kvm_hypercall(unsigned int nr, unsigned long p1,
-			  unsigned long p2, unsigned long p3,
-			  unsigned long p4)
+static long _kvm_hypercall(struct kvm_paravirt_state *state,
+			   unsigned int nr, unsigned long p1,
+			   unsigned long p2, unsigned long p3,
+			   unsigned long p4)
 {
-	struct kvm_paravirt_state *state
-		= per_cpu(paravirt_state, smp_processor_id());
 	long ret;
 
 	asm volatile("call *(%6) \n\t"

@@ -69,6 +71,55 @@ static long kvm_hypercall(unsigned int nr, unsigned long p1,
 	return ret;
 }
 
+static int can_defer_hypercall(struct kvm_paravirt_state *state,
+			       unsigned int nr)
+{
+	if (state->mode == PARAVIRT_LAZY_MMU) {
+		if (nr == KVM_HYPERCALL_MMU_WRITE)
+			return 1;
+	} else if (state->mode == PARAVIRT_LAZY_CPU) {
+		if (nr == KVM_HYPERCALL_SET_CR)
+			return 1;
+	}
+
+	return 0;
+}
+
+static void _kvm_hypercall_defer(struct kvm_paravirt_state *state,
+				 unsigned int nr,
+				 unsigned long p1, unsigned long p2,
+				 unsigned long p3, unsigned long p4)
+{
+	struct kvm_hypercall_entry *entry;
+

arch/i386/kernel/kvm.c static long kvm_hypercall(unsigned int nr, unsigned long p1,

Quoted 6 lines

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

+	if (state->vmca->queue_index == state->vmca->max_queue_index)
+		_kvm_hypercall(state, KVM_HYPERCALL_FLUSH, 0, 0, 0, 0);
+
+	/* FIXME: are we preempt safe here? */
 
 

BUG_ON(preemptible()) would be a reasonable thing to put here to be sure.

Quoted 12 lines

arch/i386/kernel/kvm.c +12 -0

+	entry = &state->queue[state->vmca->queue_index++];
+	entry->nr = nr;
+	entry->p1 = p1;
+	entry->p2 = p2;
+	entry->p3 = p3;
+	entry->p4 = p4;
+}
+
+static long kvm_hypercall(unsigned int nr, unsigned long p1,
+			  unsigned long p2, unsigned long p3,
+			  unsigned long p4)
+{

arch/i386/kernel/kvm.c static long kvm_hypercall(unsigned int nr, unsigned long p1,

Quoted 4 lines

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

+	struct kvm_paravirt_state *state
+		= per_cpu(paravirt_state, smp_processor_id());
 

Rather than using this here and passing state around, you could use either x86_read/write_percpu, or get/put_cpu_var (or __get_vpu_var if you don't need the preempt-disable).

Quoted 28 lines

arch/i386/kernel/kvm.c +10 -0

+	long ret = 0;
+
+	if (can_defer_hypercall(state, nr))
+		_kvm_hypercall_defer(state, nr, p1, p2, p3, p4);
+	else
+		ret = _kvm_hypercall(state, nr, p1, p2, p3, p4);
+
+	return ret;
+}
+

/*
* No need for any "IO delay" on KVM
*/

arch/i386/kernel/kvm.c +3 -1

@@ -107,7 +158,9 @@ static void kvm_write_cr(int reg, unsigned long value)
 	state->cr_valid[reg] = 1;
 	state->cached_cr[reg] = value;
 
-	if (reg == 0)
+	if (state->mode == PARAVIRT_LAZY_CPU)
+		kvm_hypercall(KVM_HYPERCALL_SET_CR, reg, value, 0, 0);
+	else if (reg == 0)
 		native_write_cr0(value);
 	else if (reg == 3)
 		native_write_cr3(value);

@@ -218,6 +271,18 @@ static void kvm_pmd_clear(pmd_t *pmdp)
 	kvm_mmu_write(pmdp, &pmd, sizeof(pmd));
 }
 

arch/i386/kernel/kvm.c static void kvm_pmd_clear(pmd_t *pmdp)

Quoted 12 lines

arch/i386/kernel/kvm.c +10 -0

+static void kvm_set_lazy_mode(enum paravirt_lazy_mode mode)
+{
+	struct kvm_paravirt_state *state
+		= per_cpu(paravirt_state, smp_processor_id());
+
+	if (mode == PARAVIRT_LAZY_FLUSH || mode == PARAVIRT_LAZY_NONE) {
+		if (state->vmca->queue_index)
+			_kvm_hypercall(state, KVM_HYPERCALL_FLUSH, 0, 0, 0, 0);
+	}
+	state->mode = mode;
 
 

No, you don't want to set state->mode to LAZY_FLUSH (its not a mode, just a action which overloads the interface).

Quoted 112 lines

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

+}
+
 static void paravirt_ops_setup(void)

{

 	paravirt_ops.name = "KVM";
	@@ -249,6 +314,9 @@ static void paravirt_ops_setup(void)

arch/i386/kernel/kvm.c +16 -0

 		paravirt_ops.set_pud = kvm_set_pud;
 	}
 
+	if (do_hypercall_batching)
+		paravirt_ops.set_lazy_mode = kvm_set_lazy_mode;
+
 	paravirt_ops.paravirt_enabled = 1;
 
 	apply_paravirt(__parainstructions, __parainstructions_end);

@@ -293,6 +361,9 @@ static int paravirt_initialize(void)
 	if ((edx & KVM_FEATURE_MMU_WRITE))
 		do_mmu_write = 1;
 
+	if ((edx & KVM_FEATURE_HYPERCALL_BATCHING))
+		do_hypercall_batching = 1;
+
 	on_each_cpu(paravirt_activate, NULL, 0, 1);
 
 	return 0;

@@ -303,6 +374,9 @@ static __init void paravirt_free_state(struct kvm_paravirt_state *state)
 	if (!state)
 		return;
 
+	if (state->queue)
+		__free_page(pfn_to_page(__pa(state->queue) >> PAGE_SHIFT));
+
 	if (state->hypercall)
 		__free_page(pfn_to_page(__pa(state->hypercall) >> PAGE_SHIFT));
 

@@ -329,8 +403,15 @@ static __init struct kvm_paravirt_state *paravirt_alloc_state(void)
 	if (!state->hypercall)
 		goto err;
 
+	state->queue = (void *)get_zeroed_page(GFP_KERNEL);
+	if (!state->queue)
+		goto err;
+
 	state->vmca_gpa = __pa(state->vmca);
 	state->vmca->hypercall_gpa = __pa(state->hypercall);
+	state->vmca->queue_gpa = __pa(state->queue);
+	state->vmca->max_queue_index
+		= (PAGE_SIZE / sizeof(struct kvm_hypercall_entry));
 
 	return state;
 

drivers/kvm/kvm.h +1 -0

index b08272b..d531899 100644
@@ -291,6 +291,7 @@ struct kvm_vcpu {
 	gpa_t para_state_gpa;
 	struct page *para_state_page;
 	gpa_t hypercall_gpa;
+	struct page *queue_page;
 	unsigned long cr4;
 	unsigned long cr8;
 	u64 pdptrs[4]; /* pae */

drivers/kvm/kvm_main.c +22 -1

index 4f65729..79a2a64 100644
@@ -94,7 +94,8 @@ struct vfsmount *kvmfs_mnt;
 
 #define KVM_PARAVIRT_FEATURES \
 	(KVM_FEATURE_VMCA | KVM_FEATURE_NOP_IO_DELAY | \
-	 KVM_FEATURE_CR_READ_CACHE | KVM_FEATURE_MMU_WRITE)
+	 KVM_FEATURE_CR_READ_CACHE | KVM_FEATURE_MMU_WRITE | \
+	 KVM_FEATURE_HYPERCALL_BATCHING)
 
 #define KVM_MSR_SET_VMCA	0x87655678
 

@@ -1369,6 +1370,24 @@ static int kvm_hypercall_mmu_write(struct kvm_vcpu *vcpu, gva_t addr,
 	return 0;
 }
 
+static int kvm_hypercall_set_cr(struct kvm_vcpu *vcpu,
+				u32 reg, unsigned long value)
+{
+	switch (reg) {
+	case 0:
+		set_cr0(vcpu, value);
+		break;
+	case 3:
+		set_cr3(vcpu, value);
+		break;
+	case 4:
+		set_cr4(vcpu, value);
+		break;
+	}
+
+	return 0;
+}
+
 static int dispatch_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
 			      unsigned long p1, unsigned long p2,
 			      unsigned long p3, unsigned long p4)

@@ -1376,10 +1395,36 @@ static int dispatch_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
 	switch (nr) {
 	case KVM_HYPERCALL_MMU_WRITE:
 		return kvm_hypercall_mmu_write(vcpu, p1, p2, p3, p4);
+	case KVM_HYPERCALL_SET_CR:
+		return kvm_hypercall_set_cr(vcpu, p1, p2);
 	}
 	return -ENOSYS;
 }
 

drivers/kvm/kvm_main.c static int dispatch_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,

Quoted 11 lines

drivers/kvm/kvm_main.c +9 -0

+static int kvm_hypercall_flush(struct kvm_vcpu *vcpu)
+{
+	struct kvm_hypercall_entry *queue;
+	struct kvm_vmca *vmca;
+	int ret = 0;
+	int i;
+
+	queue = kmap(vcpu->queue_page);
+	vmca = kmap(vcpu->para_state_page);
 
 

kmap_atomic? Or why not keep them mapped all the time?

J

Implemented a hypercall queue that can be used when paravirt_ops lazy mode is enabled. This patch enables queueing of MMU write operations and CR updates. This results in about a 50% bump in kernbench performance.

Quoted 5 lines · Anthony Liguori

Implemented a hypercall queue that can be used when paravirt_ops lazy mode
is enabled. This patch enables queueing of MMU write operations and CR updates. This results in about a 50% bump in kernbench performance.

Nice! But 50%? a kernel build is at native-25%, so we're now 25% faster than native?

arch/i386/kernel/kvm.c static __init struct kvm_paravirt_state *paravirt_alloc_state(void) · line 414

Quoted 4 lines

arch/i386/kernel/kvm.c +3 -0

+    state->vmca->queue_gpa = __pa(state->queue);
+    state->vmca->max_queue_index
+        = (PAGE_SIZE / sizeof(struct kvm_hypercall_entry));

Why not pass the queue address as an argument to KVM_HYPERCALL_FLUSH? That reduces the amount of setup, and allows more flexibility (e.g. multiple queues).

I'm not thrilled with having queues of hypercalls; instead I'd prefer queues of mmu operations, but I guess it won't do any good to go against prevailing custom here.

Quoted 8 lines · Avi Kivity

Quoted 5 lines · Anthony Liguori

Implemented a hypercall queue that can be used when paravirt_ops lazy mode
is enabled. This patch enables queueing of MMU write operations and CR updates. This results in about a 50% bump in kernbench performance.

Quoted 2 lines

Nice! But 50%? a kernel build is at native-25%, so we're now 25% faster than native?

Well, I haven't measured KVM to be 25% of native with kernbench :-) On my LS21 (AMD), I get:

KVM
Elapsed Time 1054.39 (25.8237)
User Time 371.844 (8.57204)
System Time 682.61 (17.7778)
Percent CPU 99.8 (0.447214) Sleeps 50115 (475.693)

KVM PV
Elapsed Time 595.85 (13.7058) User Time 360.99 (9.56093) System Time 234.704 (4.21283) Percent CPU 99 (0)
Context Switches 46989.8 (328.277) Sleep 47882.8 (242.583)

NATIVE
Elapsed Time 328.602 (0.212415) User Time 304.364 (0.353171) System Time 23.99 (0.325192) Percent CPU 99 (0)
Context Switches 39785.2 (159.796) Sleeps 46398.6 (311.466)

With Intel, we're still only about 60% of native to start out with. The PV patches take us to about 72%.

These numbers are pretty bad. I'd like to improve them, even without PV.

Quoted 3 lines · Avi Kivity

These numbers are pretty bad. I'd like to improve them, even without PV.

There's a 20% speedup just waiting to be checked in in the lapic branch ;)

(This gain was observed using "make mrproper; make defconfig; time make" on Intel with a non-PV 2.6.21 guest when the lapic code was enabled).

Quoted 10 lines · Gregory Haskins

Quoted 3 lines · Avi Kivity

These numbers are pretty bad. I'd like to improve them, even without PV.

There's a 20% speedup just waiting to be checked in in the lapic branch ;)

Quoted 3 lines

(This gain was observed using "make mrproper; make defconfig; time make" on Intel with a non-PV 2.6.21 guest when the lapic code was enabled).

So what was the final number you were seeing? Were you seeing only 25% below native to start with?

Quoted 10 lines · Anthony Liguori

Quoted 9 lines · Avi Kivity

Quoted 6 lines · Anthony Liguori

Implemented a hypercall queue that can be used when paravirt_ops lazy mode
is enabled. This patch enables queueing of MMU write operations and CR
updates. This results in about a 50% bump in kernbench performance.

Quoted 5 lines

Quoted 2 lines

Nice! But 50%? a kernel build is at native-25%, so we're now 25% faster than native?

Well, I haven't measured KVM to be 25% of native with kernbench :-) On my LS21 (AMD), I get:

I did, but using kbuild (a simple 'make' with defconfig), not kernbench. I get (elapsed time) 308 sec for kvm and 243 sec for native.

Intel however is much faster than AMD due to the recent optimizations, and I guess we get some pagetable thrashing with kernbench vs. kbuild.

Quoted 23 lines

KVM
Elapsed Time 1054.39 (25.8237)
User Time 371.844 (8.57204)
System Time 682.61 (17.7778)
Percent CPU 99.8 (0.447214) Sleeps 50115 (475.693)

KVM PV
Elapsed Time 595.85 (13.7058) User Time 360.99 (9.56093) System Time 234.704 (4.21283) Percent CPU 99 (0)
Context Switches 46989.8 (328.277) Sleep 47882.8 (242.583)

NATIVE
Elapsed Time 328.602 (0.212415) User Time 304.364 (0.353171) System Time 23.99 (0.325192) Percent CPU 99 (0)
Context Switches 39785.2 (159.796) Sleeps 46398.6 (311.466)

Quoted 3 lines

With Intel, we're still only about 60% of native to start out with. The PV patches take us to about 72%.

These numbers are pretty bad. I'd like to improve them, even without PV.

Quoted 3 lines

I did, but using kbuild (a simple 'make' with defconfig), not kernbench. I get (elapsed time) 308 sec for kvm and 243 sec for native.

kernbench is a little different. It does a find over the kernel source tree to attempt to get as much of the kernel in the page cache as possible. It also uses -j4 by default.

Quoted 1 line

Quoted 2 lines

These numbers are pretty bad. I'd like to improve them, even without PV.

I agree. Do you know what's missing at this point? There isn't a whole lot of state saving going on for the light weight exit paths for SVM.

Quoted 6 lines

Quoted 2 lines

These numbers are pretty bad. I'd like to improve them, even without PV.

I agree. Do you know what's missing at this point? There isn't a whole lot of state saving going on for the light weight exit paths for SVM.

The SVM code doesn't even have a lightweight vmexit path.

For every vmexit, it does the entire thing, including vmload/vmsave

, fpu switch (if needed)

, segment reloading, and msr reloading.

It could use a lot of work.

For kbuild vs. kernbench, I suspect that -j4 causes the shadow page table cache to thrash. 1024 pages may be enough for a single instance but not -j4. Hopefully replacing the eviction algorithm (currently FIFO) will help. Otherwise we'll need to resize the cache again.

Quoted 8 lines · Avi Kivity

Quoted 6 lines

Quoted 2 lines

These numbers are pretty bad. I'd like to improve them, even without PV.

I agree. Do you know what's missing at this point? There isn't a whole lot of state saving going on for the light weight exit paths for SVM.

The SVM code doesn't even have a lightweight vmexit path.

Sure it does. Quite a lot is deferred to vcpu_{load,put}.

Quoted 2 lines

For every vmexit, it does the entire thing, including vmload/vmsave

I haven't had a lot of luck eliminating vmload/vmsave.

Quoted 1 line

, fpu switch (if needed)

The FPU switch can really be avoided? Is it safe to assume that the KVM code isn't going to use any FPU operations?

Quoted 6 lines

For kbuild vs. kernbench, I suspect that -j4 causes the shadow page table cache to thrash. 1024 pages may be enough for a single instance but not -j4. Hopefully replacing the eviction algorithm (currently FIFO) will help. Otherwise we'll need to resize the cache again.

I naively tried to bump it to 2048 and hit a kmalloc limitation.

Quoted 11 lines · Anthony Liguori

Quoted 8 lines · Avi Kivity

Quoted 6 lines

Quoted 2 lines

These numbers are pretty bad. I'd like to improve them, even without PV.

I agree. Do you know what's missing at this point? There isn't a whole lot of state saving going on for the light weight exit paths for SVM.

The SVM code doesn't even have a lightweight vmexit path.

Sure it does. Quite a lot is deferred to vcpu_{load,put}.

Ah, I forgot. Yes, the syscall msrs are deferred.

Quoted 5 lines

Quoted 2 lines

For every vmexit, it does the entire thing, including vmload/vmsave

I haven't had a lot of luck eliminating vmload/vmsave.

For x86_64, the only issue I see is with TR. Unfortunately, I don't see a way around it.

Quoted 4 lines

Quoted 1 line

, fpu switch (if needed)

The FPU switch can really be avoided? Is it safe to assume that the KVM code isn't going to use any FPU operations?

Generally, kernel code does not use the fpu (when it does, it calls kernel_fpu_begin() and kernel_fpu_end()). The vmx code avoids the switch.

Of course, if the guest doesn't use the fpu, the switch is avoided anyway.

Quoted 7 lines

Quoted 6 lines

For kbuild vs. kernbench, I suspect that -j4 causes the shadow page table cache to thrash. 1024 pages may be enough for a single instance but not -j4. Hopefully replacing the eviction algorithm (currently FIFO) will help. Otherwise we'll need to resize the cache again.

Quoted 2 lines

I naively tried to bump it to 2048 and hit a kmalloc limitation.

struct kvm is 22K on x86_64. Adding 1024 pointers makes it 30K. What error did you get?

We should probably make the hashtable a pointer, and allocate vcpus separately as well.

Quoted 3 lines

struct kvm is 22K on x86_64. Adding 1024 pointers makes it 30K. What error did you get?

With an older kvm, on a different system, I was getting:

Quoted 6 lines

Quoted 3 lines

For every vmexit, it does the entire thing, including vmload/vmsave

I haven't had a lot of luck eliminating vmload/vmsave.

For x86_64, the only issue I see is with TR. Unfortunately, I don't see a way around it.

Quoted 10 lines · Avi Kivity

Quoted 6 lines

Quoted 3 lines

For every vmexit, it does the entire thing, including vmload/vmsave

I haven't had a lot of luck eliminating vmload/vmsave.

For x86_64, the only issue I see is with TR. Unfortunately, I don't see a way around it.

I think we can avoid vmload (but not vmsave):

1. Allocate a host gdt entry for kvm's exclusive use.

2. The first entry into the guest needs vmload as usual. The second entry reuses already-loaded registers, except tr, gs.base, and kernelgsbase.

3. To load tr, copy the descriptor into our gdt entry, and execute ltr.

4. To load gs.base, load the saved value into MSR_KERNELGSBASE, and execute swapgs

5. To load kernelgsbase, use wrmsr()

However, I'm not at all sure it's worth it.

arch/i386/kernel/kvm.c static long kvm_hypercall(unsigned int nr, unsigned long p1, · line 142

Quoted 11 lines · Anthony Liguori

arch/i386/kernel/kvm.c +9 -0

+static cycle_t read_hyper(void)
+{
+	struct timespec now;
+	int ret;
+
+	ret = kvm_hypercall(KVM_HYPERCALL_GET_KTIME, (u32)&now, 0, 0, 0);
+	WARN_ON(ret);
+
+	return now.tv_nsec + now.tv_sec * (cycles_t)1e9;

Hm, use of FP looks pretty odd. I guess its OK to assume the compiler will completely remove all the FP stuff at compile time. Or you could use NSEC_PER_SEC.

arch/i386/kernel/kvm.c static long kvm_hypercall(unsigned int nr, unsigned long p1, · line 147

Quoted 7 lines

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

+}
+
+static struct clocksource clocksource_hyper = {
+	.name			= "hyper",
+	.rating			= 200,

We should probably standardize on this. I guess that if you're in a paravirt environment, and there's a paravirt clocksource, that would always be the best clocksource to use.

arch/i386/kernel/kvm.c static long kvm_hypercall(unsigned int nr, unsigned long p1, · line 151

Quoted 6 lines

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

+	.read			= read_hyper,
+	.mask			= CLOCKSOURCE_MASK(64),
+	.mult			= 1,
+	.shift			= 0,

It would be better to use a scale and shift here, so that adjtime has something to work with when warping time.

J

Okay. I may remove this patch from the patch series and attempt to sit down next week and work out something more complete that also implements stolen time accounting.

Quoted 4 lines · Anthony Liguori

Okay. I may remove this patch from the patch series and attempt to sit down next week and work out something more complete that also implements stolen time accounting.

Well, that's a separate problem. clocksource.read should always return real time passed, so stolen time doesn't come into it.

paravirt_ops.sched_clock should take stolen time into account, but that's almost completely orthogonal.

How are you doing clockevents?

J

Quoted 9 lines · Jeremy Fitzhardinge

Quoted 4 lines · Anthony Liguori

Okay. I may remove this patch from the patch series and attempt to sit down next week and work out something more complete that also implements stolen time accounting.

Well, that's a separate problem. clocksource.read should always return real time passed, so stolen time doesn't come into it.

Right.

Quoted 1 line

Quoted 3 lines

paravirt_ops.sched_clock should take stolen time into account, but that's almost completely orthogonal.

Except that I wanted to change the hypercall to allow querying of real time or "available" time as VMI puts it.

Right now, I'm relying on the PIT but it would be nice to eliminate that. I'd like to move to something PV so that I can make use of tickless guest kernels. I'm very open to suggestion and even more open to reusing other people's code :-)

Quoted 3 lines · Anthony Liguori

Except that I wanted to change the hypercall to allow querying of real time or "available" time as VMI puts it.

I see. You could have an interface like Xen's runstate interface, which gives you a breakdown of how long each vcpu spends in each state (running, runnable, paused or blocked). The sum of all of them gives you real time, or you can use a subset to work out stolen time (runnable+paused), busyness (blocked / (running+runnable+blocked)), etc.

Quoted 5 lines

Right now, I'm relying on the PIT but it would be nice to eliminate that. I'd like to move to something PV so that I can make use of tickless guest kernels. I'm very open to suggestion and even more open to reusing other people's code :-)

A simple hypercall interface which says "raise irq X after N ns" is probably the easiest way to go.

Try to avoid the pitfalls of VMI's
interface in this area, which tries to recycle existing architectural interrupt sources for hypervisor timers, and gets into a bit of mess. Xen is relatively clean, but I'm not sure how it would fit into the mostly-emulated kvm model. If you keep things simple, most of the Xen clockevent code could be reused with little change.

J

Quoted 4 lines

How are you doing clockevents?

Right now, I'm relying on the PIT but it would be nice to eliminate that. I'd like to move to something PV so that I can make use of tickless guest kernels. I'm very open to suggestion and even more open to reusing other people's code :-)

Quoted 5 lines · Anthony Liguori

Quoted 4 lines

How are you doing clockevents?

Quoted 5 lines

Right now, I'm relying on the PIT but it would be nice to eliminate that. I'd like to move to something PV so that I can make use of tickless guest kernels. I'm very open to suggestion and even more open to reusing other people's code :-)

You'll need an irqchip to inject irqs. I think Ingo's patchset had an irqchip implementation.

arch/i386/kernel/kvm.c static long kvm_hypercall(unsigned int nr, unsigned long p1, · line 151

Quoted 6 lines

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

+	.read			= read_hyper,
+	.mask			= CLOCKSOURCE_MASK(64),
+	.mult			= 1,
+	.shift			= 0,

It would be better to use a scale and shift here, so that adjtime has something to work with when warping time.

J

I've updated this patch and switched to using a scale/shift like Xen is doing, but I must admit, I don't understand how it helps adjtime. I poked around a bit and it wasn't obvious.

Why is having {mult=1<<22, shift=22} better for adjtime than {mult=1, shift=0}?

Quoted 4 lines · Anthony Liguori

I've updated this patch and switched to using a scale/shift like Xen is doing, but I must admit, I don't understand how it helps adjtime. I poked around a bit and it wasn't obvious.

Quoted 3 lines

Why is having {mult=1<<22, shift=22} better for adjtime than {mult=1, shift=0}?

I don't fully understand it myself, but I think its because adjtime plays with the mult factor to scale the rate at which time passes. If the scale is 1, then it can only scale time by integer amounts. By setting it to 2^22, then it can adjust time down to 1 part in 4 million.

J

I've updated this patch and switched to using a scale/shift like Xen is
doing, but I must admit, I don't understand how it helps adjtime. I poked around a bit and it wasn't obvious.

Why is having {mult=1<<22, shift=22} better for adjtime than {mult=1, shift=0}?

1) Not really sure what is needed for CONFIG_PREEMPT support. I'm not sure which paravirt_ops calls are actually re-entrant.

2) The paravirt_ops implementation is registered with core_initcall(). However, the paravirt_ops banner is also printed with core_initcall() so that fact that this works now is just the luck of build order. Need a better way to initialize the KVM paravirt_ops backend.

Quoted 3 lines · Anthony Liguori

1) Not really sure what is needed for CONFIG_PREEMPT support. I'm not sure which paravirt_ops calls are actually re-entrant.

I'm not sure that has specifically come up. The main issue is whether a particular call can be preempted and whether that matters. I guess the calls which affect a particular CPU's state will generally be called in a non-preemptable context, but I guess we can't assume that; the best approach is to assume that each call be atomic with respect to preemption.

Things like batching must be completed with preemption disabled over the whole batch. I check that with BUG_ON in the Xen code.

Quoted 6 lines

2) The paravirt_ops implementation is registered with core_initcall(). However, the paravirt_ops banner is also printed with core_initcall() so that fact that this works now is just the luck of build order. Need a better way to initialize the KVM paravirt_ops backend.

Hm. We could make the banner printing later; obviously its purely cosmetic. I put it as a core_initcall on the assumption that pv-ops would be set up very early as it is with Xen and lguest, and so the banner should print relatively early. But if your model is that you boot fully virtualized for a while, and then become paravirtualized later, then it would make sense to defer banner printing until then.

J

Hi Jeremy,

Quoted 6 lines · Jeremy Fitzhardinge

Quoted 3 lines · Anthony Liguori

1) Not really sure what is needed for CONFIG_PREEMPT support. I'm not sure which paravirt_ops calls are actually re-entrant.

Quoted 7 lines

I'm not sure that has specifically come up. The main issue is whether a particular call can be preempted and whether that matters. I guess the calls which affect a particular CPU's state will generally be called in a non-preemptable context, but I guess we can't assume that; the best approach is to assume that each call be atomic with respect to preemption.

So each call would need to disable preemption? I'm not sure that makes a whole lot of sense for something like CR reads/writes. In fact, without passing in a cpu parameter, I'm pretty sure that those operations *have* to require preemption to be disabled.

For something like MMU operations, preemption really doesn't have to be disabled.

Quoted 3 lines

Things like batching must be completed with preemption disabled over the whole batch. I check that with BUG_ON in the Xen code.

Right now the KVM batching requires preemption to be disabled for batching.

I don't think that's a hard requirement though since we could pass
the current batch PA as part of the flush hypercalls.

Things are a lot easier though if we can just assume preemption is disabled :-)

Are you aware of any paravirt_ops calls that are probably being called in the kernel with preemption enabled?

I don't see a compelling reason to paravirtualize earlier although I also don't see a compelling reason not too. I noticed that VMI hooks setup.c. It wasn't immediately obvious why it was hooking there but perhaps it worthwhile to have a common hook? I suspect VMI and KVM will have a similar model for startup.

Quoted 19 lines · Anthony Liguori

Hi Jeremy,

Quoted 7 lines · Jeremy Fitzhardinge

Quoted 4 lines · Anthony Liguori

1) Not really sure what is needed for CONFIG_PREEMPT support. I'm not sure which paravirt_ops calls are actually re-entrant.

Quoted 8 lines

I'm not sure that has specifically come up. The main issue is whether a particular call can be preempted and whether that matters. I guess the calls which affect a particular CPU's state will generally be called in a non-preemptable context, but I guess we can't assume that; the best approach is to assume that each call be atomic with respect to preemption.

Quoted 5 lines

So each call would need to disable preemption? I'm not sure that makes a whole lot of sense for something like CR reads/writes. In fact, without passing in a cpu parameter, I'm pretty sure that those operations *have* to require preemption to be disabled.

Yeah, its a little unclear to me. If you're poking at a control register, then one presumes you've got a specific CPU's CRx in mind. But in the Xen code I don't care about the preemption state for control register updates - except for write_cr3, which never makes any sense with preemption enabled.

Quoted 3 lines

For something like MMU operations, preemption really doesn't have to be disabled.

Unless you're batching, since the lazy_mode is inherently per-cpu state.

Quoted 7 lines

Quoted 4 lines

Things like batching must be completed with preemption disabled over the whole batch. I check that with BUG_ON in the Xen code.

Right now the KVM batching requires preemption to be disabled for batching.

I think that's probably overkill. I had to put a few explicit preempt_disable/enables in the Xen code, but mostly the preempt state is reasonable for a given operation (ie, disabled for per-cpu state updates, enabled for memory/global state updates).

Quoted 3 lines

I don't think that's a hard requirement though since we could pass
the current batch PA as part of the flush hypercalls.

PA?

Quoted 6 lines

Things are a lot easier though if we can just assume preemption is disabled :-)

Are you aware of any paravirt_ops calls that are probably being called in the kernel with preemption enabled?

Erm, I haven't made a breakdown, but many are. The descriptor updates generally are, for example. Pagetable updates could be, but are generally done under a pagetable lock, and so are not preemptible anyway.

Quoted 6 lines

I don't see a compelling reason to paravirtualize earlier although I also don't see a compelling reason not too. I noticed that VMI hooks setup.c. It wasn't immediately obvious why it was hooking there but perhaps it worthwhile to have a common hook? I suspect VMI and KVM will have a similar model for startup.

Well, I was suggesting we could print the banner later rather than forcing an earlier init.

The important part is that you set your pv_ops before patching occurs, since that will bake the function calls into the rest of the kernel, and it will ignore any further changes to the paravirt_ops structure.

I think Zach was originally thinking of initializing VMI much later (even as a module load), but the subtleties of inveigling its way into the kernel at that late stage got too complex.

J

Perhaps we can just print the banner before batching occurs? Then it's being printed at the last possible moment.

Quoted 3 lines · Anthony Liguori

Perhaps we can just print the banner before batching occurs? Then it's being printed at the last possible moment.

s/batching/patching/? Yes, that would work.

J

Well, I was suggesting we could print the banner later rather than forcing an earlier init.

The important part is that you set your pv_ops before patching occurs, since that will bake the function calls into the rest of the kernel, and it will ignore any further changes to the paravirt_ops structure.

I think Zach was originally thinking of initializing VMI much later (even as a module load), but the subtleties of inveigling its way into the kernel at that late stage got too complex.

For software-reliant paravirtualization, it is difficult to atomically switch from natural instructions to simulated para-instructions on the fly; you would need stop_machine_run that also holds off NMIs (so as to keep IF flag state intact across a window where non-virtualizable IRET instruction is not yet patched), and you would need to re-patch the kernel and modules dynamically. Another problem is unloading the module, which requires restoring the smashed native paravirt-ops - some of which may have been patched, some not. It is possible to do this from a module, just obtuse, and for 32-bit, not really worth the effort IMHO.

Definition: software-advisory paravirtualization is a guest-involved virtualization technique in which only advisory state is communicated to the hypervisor, thus making redirection of instruction flow at any particular point optional for more efficient virtualization (and non-virtualizability is eliminated by some other mechanism).

For software-advisory paravirtualization, it is totally possible to just switch over to new pv-ops at any time, and there need be no atomicity. This would make a paravirt-ops module rather easy to write; it simply needs to run some init code on each CPU and the patch paravirt-ops at leisure.

Now it is quite likely at least one developer is going to be assuming hardware virtualization capabilities for 64-bit paravirt, thus making an advisory method with module loading (and unloading) a more practical option than dissecting the 64-bit startup sequence.

In that case, perhaps having a paravirt_register function which
would check to make sure no conflicting paravirt-ops have already been installed, printing the banner on success would be the most logical. The paravirt_unregister function can then simply restore the native paravirt-ops.

More importantly, now device drivers for virtual devices would have a way to inquire into which set of paravirt-ops was loaded by having an official registered interface rather than an ad-hoc (if xxx_running ==
1) mess, and now the paravirt driver modules are nicely decoupled from the boot-strap code and can be loaded dynamically.

Zach

I don't agree that having paravirt_ops within a normal module is all that useful. By the time modules can be loaded, the kernel has completely booted. There should only be a handful of paravirt_ops implementations and they aren't large so I don't think there's a big size savings either.

Quoted 5 lines

More importantly, now device drivers for virtual devices would have a way to inquire into which set of paravirt-ops was loaded by having an official registered interface rather than an ad-hoc (if xxx_running == 1) mess, and now the paravirt driver modules are nicely decoupled from the boot-strap code and can be loaded dynamically.

I'm not familiar with the particular problem here, but I don't think that driver modules should be checking to see what paravirt_ops is active. Each VMM has it's own discovery mechanism (KVM and Xen are both based on CPUID) so that seems like a much better method to use.

Quoted 6 lines · Anthony Liguori

I don't agree that having paravirt_ops within a normal module is all that useful. By the time modules can be loaded, the kernel has completely booted. There should only be a handful of paravirt_ops implementations and they aren't large so I don't think there's a big size savings either.

It doesn't seem terribly valuable to me either. But Zach is talking about something very similar to the kvm case, where you have a fully virtualized environment (with hardware support), but then you load a module containing paravirtualized helpers at some late stage which makes things more efficient but isn't required for functional correctness.

Quoted 11 lines

Quoted 6 lines

More importantly, now device drivers for virtual devices would have a way to inquire into which set of paravirt-ops was loaded by having an official registered interface rather than an ad-hoc (if xxx_running == 1) mess, and now the paravirt driver modules are nicely decoupled from the boot-strap code and can be loaded dynamically.

I'm not familiar with the particular problem here, but I don't think that driver modules should be checking to see what paravirt_ops is active. Each VMM has it's own discovery mechanism (KVM and Xen are both based on CPUID) so that seems like a much better method to use.

I think he's referring to the xen/kvm/vmi paravirt implementation as a "driver" here. I think.

I don't know of any "if (xxx_running)" tests at present.

For a VMM which supports both full emulation and para-virtualization, testing CPUID leaves is not sufficient to determine applicability of a paravirt device driver. This only indicates the presence of the functionality, not the fact that the functionality has been activated. For 32-bit Xen, this might be an already assumed fact - but for VMI, KVM, and HV assisted Xen, which do support guests running without paravirt, you need a way to test whether the particular family of paravirt has been activated - for device drivers which assume paravirt semantics might well require this activation to work, or need to behave differently in an unactivated environment (emulate hypercalls with port I/O, for example).

Thus, all the paravirt drivers as modules would need to test if (xen_running) or (vmi_enabled) or (kvm_active), and then all these symbols need to be exported, and now you have an ad-hoc activation detection system for each brand of paravirt.

Better to have a standard interface, IMHO, where a paravirt-ops "parent" module gets registered and activated, and then well defined symbols to query that activation. You also have module dependencies between the parent and child which are nicely modeled with the module system (xenbus dependes on xen, vmitimer depends on vmi, etc..).

Quoted 9 lines · Jeremy Fitzhardinge

Quoted 6 lines · Anthony Liguori

I don't agree that having paravirt_ops within a normal module is all that useful. By the time modules can be loaded, the kernel has completely booted. There should only be a handful of paravirt_ops implementations and they aren't large so I don't think there's a big size savings either.

Quoted 6 lines

It doesn't seem terribly valuable to me either. But Zach is talking about something very similar to the kvm case, where you have a fully virtualized environment (with hardware support), but then you load a module containing paravirtualized helpers at some late stage which makes things more efficient but isn't required for functional correctness.

Yes, the value isn't the space savings - it's the ability to include paravirtualized driver support for Xen, KVM, VMI, lhype - which need not be compiled in, but can now be modules in a ramdisk. The goal is minimal effort for a single bootable image which works across native and all virtualization environments.

Quoted 12 lines

Quoted 11 lines

Quoted 6 lines

More importantly, now device drivers for virtual devices would have a way to inquire into which set of paravirt-ops was loaded by having an official registered interface rather than an ad-hoc (if xxx_running == 1) mess, and now the paravirt driver modules are nicely decoupled from the boot-strap code and can be loaded dynamically.

I'm not familiar with the particular problem here, but I don't think that driver modules should be checking to see what paravirt_ops is active. Each VMM has it's own discovery mechanism (KVM and Xen are both based on CPUID) so that seems like a much better method to use.

Quoted 5 lines

I think he's referring to the xen/kvm/vmi paravirt implementation as a "driver" here. I think.

I don't know of any "if (xxx_running)" tests at present.

For a VMM which supports both full emulation and para-virtualization, testing CPUID leaves is not sufficient to determine applicability of a paravirt device driver. This only indicates the presence of the functionality, not the fact that the functionality has been activated. For 32-bit Xen, this might be an already assumed fact - but for VMI, KVM, and HV assisted Xen, which do support guests running without paravirt, you need a way to test whether the particular family of paravirt has been activated - for device drivers which assume paravirt semantics might well require this activation to work, or need to behave differently in an unactivated environment (emulate hypercalls with port I/O, for example).

Thus, all the paravirt drivers as modules would need to test if (xen_running) or (vmi_enabled) or (kvm_active), and then all these symbols need to be exported, and now you have an ad-hoc activation detection system for each brand of paravirt.

Better to have a standard interface, IMHO, where a paravirt-ops "parent" module gets registered and activated, and then well defined symbols to query that activation. You also have module dependencies between the parent and child which are nicely modeled with the module system (xenbus dependes on xen, vmitimer depends on vmi, etc..).

Zach

Quoted 18 lines · Zachary Amsden

Quoted 9 lines · Jeremy Fitzhardinge

Quoted 6 lines · Anthony Liguori

I don't agree that having paravirt_ops within a normal module is all that useful. By the time modules can be loaded, the kernel has completely booted. There should only be a handful of paravirt_ops implementations and they aren't large so I don't think there's a big size savings either.

Quoted 7 lines

It doesn't seem terribly valuable to me either. But Zach is talking about something very similar to the kvm case, where you have a fully virtualized environment (with hardware support), but then you load a module containing paravirtualized helpers at some late stage which makes
things more efficient but isn't required for functional correctness.

Quoted 5 lines

Yes, the value isn't the space savings - it's the ability to include paravirtualized driver support for Xen, KVM, VMI, lhype - which need not be compiled in, but can now be modules in a ramdisk. The goal is minimal effort for a single bootable image which works across native and all virtualization environments.

But what's the value in having it not in the kernel? Let's take Xen and lhype out of the picture because it clearly has to be there for them. You have a little less in the kernel now but then your kernel boots more slowly. There's already a noticable difference in boot-time with the KVM paravirt_ops implementation. I imagine there is for VMI too.

In the case of KVM, the paravirt_ops implementation is orthogonal to paravirt device drivers. A PV device driver can happily exist even if the paravirt_ops backend isn't activated. This is assuming that hypercalls aren't used btw. If hypercalls are desirable to use, then the paravirt_ops backend would have to EXPORT_GPL the hypercall interface. I imagine returning a specific errno would suffice.

Basically, it just makes it easier on distributors and allows any old kernel with paravirt-ops module support to run on any modern, new hypervisor - that might not have even existed at the time the distro was created.

Quoted 6 lines

In the case of KVM, the paravirt_ops implementation is orthogonal to paravirt device drivers. A PV device driver can happily exist even if the paravirt_ops backend isn't activated. This is assuming that hypercalls aren't used btw. If hypercalls are desirable to use, then the paravirt_ops backend would have to EXPORT_GPL the hypercall interface. I imagine returning a specific errno would suffice.

I'm mostly in agreement on that - although making dual hypercall / I/O emulated drivers is a bit more work.

Quoted 4 lines · Zachary Amsden

Basically, it just makes it easier on distributors and allows any old kernel with paravirt-ops module support to run on any modern, new hypervisor - that might not have even existed at the time the distro was created.

Hey, isn't that what VMI's for? ;)

I'd been thinking about the possibility of allowing the domain builder to provide a new paravirt_ops implementation to the booting kernel. It would be akin to a kernel module, in that its built for a specific kernel, but obviously run a lot earlier. But at this point I think the idea is too crack-ridden to be taken seriously.

Quoted 7 lines

Quoted 6 lines

In the case of KVM, the paravirt_ops implementation is orthogonal to paravirt device drivers. A PV device driver can happily exist even if the paravirt_ops backend isn't activated. This is assuming that hypercalls aren't used btw. If hypercalls are desirable to use, then the paravirt_ops backend would have to EXPORT_GPL the hypercall interface. I imagine returning a specific errno would suffice.

Quoted 2 lines

I'm mostly in agreement on that - although making dual hypercall / I/O emulated drivers is a bit more work.

Semi-paravirtualized real-hardware drivers seems like a difficult mishmash. I would hope we could deal with it with a virtio-like thing.

J

I've been thinking about this wrt the hypercall page in KVM. The problem is that in a model like KVM (or presumably VMI), migration gets really difficult if you have anything but a trivial hypercall page since the hypercall page will change after migration.

If you cannot guarantee the guest isn't executing code within the hypercall page (or in your case, doing something with paravirt_ops), then you cannot safely migrate.

Hm, you need to quiesce the kernel in some way when you do a migrate, so making sure it isn't in a hypercall would be just part of that. In general you'd make sure all but one CPU is parked somewhere, and the remaining CPU is doing the suspend, right?

The tricky part for Xen in all this is how to make sure all mfn references are visible to the hypervisor/toolstack so they can be remapped; hypercall page contents are not a concern by comparison.

You only need to quiesce if you have guest-visible data-structures that have details about the underlying hardware. So Xen needs to quiesce, but I don't know of any other VMM that would.

VMI, KVM and lhype should be capable of transparent migration without guest involvement.

Quoted 5 lines · Anthony Liguori

I've been thinking about this wrt the hypercall page in KVM. The problem is that in a model like KVM (or presumably VMI), migration gets really difficult if you have anything but a trivial hypercall page since the hypercall page will change after migration.

Quoted 3 lines

If you cannot guarantee the guest isn't executing code within the hypercall page (or in your case, doing something with paravirt_ops), then you cannot safely migrate.

Hm, you need to quiesce the kernel in some way when you do a migrate, so making sure it isn't in a hypercall would be just part of that. In general you'd make sure all but one CPU is parked somewhere, and the remaining CPU is doing the suspend, right?

The tricky part for Xen in all this is how to make sure all mfn references are visible to the hypervisor/toolstack so they can be remapped; hypercall page contents are not a concern by comparison.

J

The real trick is doing it without the guest being involved at all. Right now, it won't be a problem in KVM since the hypercall page only differs by a single instruction across platforms. In the future, we'll have to be smarter and wait for all VCPUs to leave the hypercall page.

Quoted 3 lines

The tricky part for Xen in all this is how to make sure all mfn references are visible to the hypervisor/toolstack so they can be remapped; hypercall page contents are not a concern by comparison.

I don't know HVM save/resume all that well but I think it's a similar model where the guest isn't involved.

They may have a similar issue when using PV drivers.

I've been thinking about this wrt the hypercall page in KVM. The problem is that in a model like KVM (or presumably VMI), migration gets really difficult if you have anything but a trivial hypercall page since the hypercall page will change after migration.

If you cannot guarantee the guest isn't executing code within the hypercall page (or in your case, doing something with paravirt_ops), then you cannot safely migrate.

Unless you also migrate the hypercall page itself and impose migration restrictions on compatible hypercall pages.

Although I favor the guarantee that execution within the hypercall page is finished - it is important for protecting against non-reentrancy as well. Think about interrupts during batching / queueing operations.

Quoted 2 lines · Zachary Amsden

Unless you also migrate the hypercall page itself and impose migration restrictions on compatible hypercall pages.

Seems unreasonable, especially if you support migration between VT and SVM machines. The whole point of a hypercall page is to give you a point of indirection in order to hide these kinds of hardware differences; migrating it would defeat the purpose.

Quoted 4 lines

Although I favor the guarantee that execution within the hypercall page is finished - it is important for protecting against non-reentrancy as well. Think about interrupts during batching / queueing operations.

Not quite sure that's specifically relevant to migration, but yes, its important to disable interrupts while doing the setup for a batch of stuff unless you want to see some surprises in your queue (and not "Oh, yay, a puppy!" kind of surprises).

If I had a gentoo install,

I would probably go so far as to want to recompile everything after migration across CPU vendors; things like NMIs, MSRs, thermal controls and sleep states are also vendor dependent and either need to be emulated both ways, re-invented in a new way entirely, or just dropped.

I don't think cross-CPU vendor hot migration is particularly compelling, although it certainly is possible, the payoff doesn't seem worth the implementation cost and you will find a maze of brambly thorns blocking your path.

Quoted 5 lines · Anthony Liguori

I've been thinking about this wrt the hypercall page in KVM. The problem is that in a model like KVM (or presumably VMI), migration gets really difficult if you have anything but a trivial hypercall page since the hypercall page will change after migration.

Quoted 3 lines

If you cannot guarantee the guest isn't executing code within the hypercall page (or in your case, doing something with paravirt_ops), then you cannot safely migrate.

Unless you also migrate the hypercall page itself and impose migration restrictions on compatible hypercall pages.

Although I favor the guarantee that execution within the hypercall page is finished - it is important for protecting against non-reentrancy as well. Think about interrupts during batching / queueing operations.

Zach

Quoted 6 lines · Anthony Liguori

But what's the value in having it not in the kernel? Let's take Xen and lhype out of the picture because it clearly has to be there for them. You have a little less in the kernel now but then your kernel boots more slowly. There's already a noticable difference in boot-time with the KVM paravirt_ops implementation. I imagine there is for VMI too.

If it isn't compiled in the core kernel, then a distro need not do anything special to distribute VMI or KVM support - other than compile support for paravirt-ops. Then the paravirt-ops module can be installed along with the guest tools and drivers, but need not be on install media.

Basically, it just makes it easier on distributors and allows any old kernel with paravirt-ops module support to run on any modern, new hypervisor - that might not have even existed at the time the distro was created.

Quoted 7 lines

In the case of KVM, the paravirt_ops implementation is orthogonal to paravirt device drivers. A PV device driver can happily exist even if the paravirt_ops backend isn't activated. This is assuming that hypercalls aren't used btw. If hypercalls are desirable to use, then the paravirt_ops backend would have to EXPORT_GPL the hypercall interface. I imagine returning a specific errno would suffice.

I'm mostly in agreement on that - although making dual hypercall / I/O emulated drivers is a bit more work.

Zach

Quoted 8 lines · Zachary Amsden

Quoted 6 lines · Anthony Liguori

But what's the value in having it not in the kernel? Let's take Xen and lhype out of the picture because it clearly has to be there for them. You have a little less in the kernel now but then your kernel boots more slowly. There's already a noticable difference in boot-time with the KVM paravirt_ops implementation. I imagine there is for VMI too.

Quoted 5 lines

If it isn't compiled in the core kernel, then a distro need not do anything special to distribute VMI or KVM support - other than compile support for paravirt-ops. Then the paravirt-ops module can be installed along with the guest tools and drivers, but need not be on install media.

Typically, distros do not support third-party modules so that's not a very useful property. Further, that just encourages out-of-kernel modules and worst yet, binary modules.

In fact, the whole install "guest tools" is fundamentally broken in this respect. Guest tools always end up installing closed source drivers. Plus, these things aren't available during distro installation typically so you end up with a sucky user experience.

Quoted 5 lines

Basically, it just makes it easier on distributors and allows any old kernel with paravirt-ops module support to run on any modern, new hypervisor - that might not have even existed at the time the distro was created.

Yeah, I'm not buying it. Is it really that much easier to backport a module than it is to just roll out a new kernel for an older distro?

BTW, isn't this the whole point of the VMI ROM? :-)

Yes, but if we want to stay with that forward compatibility story, we need a way to allow paravirt device probing to be completely orthogonal to paravirt-ops probing. Either the VMware hypervisor needs to NOT implement a CPUID leaf, keeping the same ROM based detection, or other VMI client drivers (say, as a wild example, a KVM driver running on a VMI to KVM paravirt-ops backend) need not to check CPUID leaf as a condition of execution.

We at least would like to use a CPUID leaf for the core paravirt-ops on 64-bit and get rid of the need for ROM probing in that case, which would mean we either need a CPUID sub-leaf for the device model, a completely identical device model, or completely orthogonal device probing.

Since there hasn't been a formal specification for how the device
probing should work, or, at least, I don't know all the details of how device probing works for all the various hypervisors, I worry that weird ad-hoc tests could trample the compatibility effort.

The completely identical device model is of course ideal, but the implementation and consolidation of that is a long term prospect to move towards, not something that will happen immediately. We at least emulate physical hardware devices already, and will continue to need drivers compatible with those models for some time.

I'm not wildly happy about the idea of using PCI for probing for otherwise completely non-PCI devices,

Xen deals with it with Xenbus, but I
figure I'm unlikely to convince everyone to adopt that.

Yes. That's the thinking behind using PCI as a somewhat common mechanism for device discovery. s390 folks hate it, of course.

Quoted 6 lines

The completely identical device model is of course ideal, but the implementation and consolidation of that is a long term prospect to move towards, not something that will happen immediately. We at least emulate physical hardware devices already, and will continue to need drivers compatible with those models for some time.

Well, physical devices and completely emulated physical devices are fairly straightforward - do it like real hardware. Its the semi-virtual devices which pose problems. Either device emulations with a bit of performance paravirtualization sprinkled over them, or virtualization friendly devices which allow safe direct guest access, but need some paravirtual management interfaces as well.

I don't see a compelling reason to paravirtualize earlier although I also don't see a compelling reason not too. I noticed that VMI hooks setup.c. It wasn't immediately obvious why it was hooking there but perhaps it worthwhile to have a common hook? I suspect VMI and KVM will have a similar model for startup.