KVM updates for 2.6.20-rc2

Compatibility and stabilization fixes, many centered around msrs, but a few other corner cases as well.

error compiling committee.c: too many arguments to function

current_cpu_data invokes smp_processor_id(), which is inadvisable when preemption is enabled. Switch to boot_cpu_data instead.

Resolves sourceforge bug 1621401.

Index: linux-2.6/drivers/kvm/svm.c
===================================================================
@@ -246,7 +246,7 @@ static int has_svm(void)
 {
 	uint32_t eax, ebx, ecx, edx;
 
-	if (current_cpu_data.x86_vendor != X86_VENDOR_AMD) {
+	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD) {
 		printk(KERN_INFO "has_svm: not amd\n");
 		return 0;
 	}

Instead of doing tricky stuff with the arch dependent virtualization registers, take a peek at the guest's efer.

This simlifies some code, and fixes some confusion in the mmu branch.

Index: linux-2.6/drivers/kvm/mmu.c
===================================================================
@@ -578,7 +578,7 @@ static int init_kvm_mmu(struct kvm_vcpu 
 
 	if (!is_paging(vcpu))
 		return nonpaging_init_context(vcpu);
-	else if (kvm_arch_ops->is_long_mode(vcpu))
+	else if (is_long_mode(vcpu))
 		return paging64_init_context(vcpu);
 	else if (is_pae(vcpu))
 		return paging32E_init_context(vcpu);

Index: linux-2.6/drivers/kvm/kvm_main.c
===================================================================
@@ -398,7 +398,7 @@ void set_cr4(struct kvm_vcpu *vcpu, unsi
 		return;
 	}
 
-	if (kvm_arch_ops->is_long_mode(vcpu)) {
+	if (is_long_mode(vcpu)) {
 		if (!(cr4 & CR4_PAE_MASK)) {
 			printk(KERN_DEBUG "set_cr4: #GP, clearing PAE while "
 			       "in long mode\n");

@@ -425,7 +425,7 @@ EXPORT_SYMBOL_GPL(set_cr4);
 
 void set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
 {
-	if (kvm_arch_ops->is_long_mode(vcpu)) {
+	if (is_long_mode(vcpu)) {
 		if ( cr3 & CR3_L_MODE_RESEVED_BITS) {
 			printk(KERN_DEBUG "set_cr3: #GP, reserved bits\n");
 			inject_gp(vcpu);

Index: linux-2.6/drivers/kvm/kvm.h
===================================================================
@@ -278,7 +278,6 @@ struct kvm_arch_ops {
 			    struct kvm_segment *var, int seg);
 	void (*set_segment)(struct kvm_vcpu *vcpu,
 			    struct kvm_segment *var, int seg);
-	int (*is_long_mode)(struct kvm_vcpu *vcpu);
 	void (*get_cs_db_l_bits)(struct kvm_vcpu *vcpu, int *db, int *l);
 	void (*set_cr0)(struct kvm_vcpu *vcpu, unsigned long cr0);
 	void (*set_cr0_no_modeswitch)(struct kvm_vcpu *vcpu,

@@ -403,6 +402,15 @@ static inline struct page *_gfn_to_page(
 	return (slot) ? slot->phys_mem[gfn - slot->base_gfn] : NULL;
 }
 
+static inline int is_long_mode(struct kvm_vcpu *vcpu)
+{
+#ifdef CONFIG_X86_64
+	return vcpu->shadow_efer & EFER_LME;
+#else
+	return 0;
+#endif
+}
+
 static inline int is_pae(struct kvm_vcpu *vcpu)
 {
 	return vcpu->cr4 & CR4_PAE_MASK;

Index: linux-2.6/drivers/kvm/svm.c
===================================================================
@@ -166,11 +166,6 @@ static inline void write_dr7(unsigned lo
 	asm volatile ("mov %0, %%dr7" :: "r" (val));
 }
 
-static inline int svm_is_long_mode(struct kvm_vcpu *vcpu)
-{
-	return vcpu->svm->vmcb->save.efer & KVM_EFER_LMA;
-}
-
 static inline void force_new_asid(struct kvm_vcpu *vcpu)
 {
 	vcpu->svm->asid_generation--;

@@ -1609,7 +1604,6 @@ static struct kvm_arch_ops svm_arch_ops 
 	.get_segment_base = svm_get_segment_base,
 	.get_segment = svm_get_segment,
 	.set_segment = svm_set_segment,
-	.is_long_mode = svm_is_long_mode,
 	.get_cs_db_l_bits = svm_get_cs_db_l_bits,
 	.set_cr0 = svm_set_cr0,
 	.set_cr0_no_modeswitch = svm_set_cr0,

Index: linux-2.6/drivers/kvm/paging_tmpl.h
===================================================================
@@ -68,7 +68,7 @@ static void FNAME(init_walker)(struct gu
 	hpa = safe_gpa_to_hpa(vcpu, vcpu->cr3 & PT64_BASE_ADDR_MASK);
 	walker->table = kmap_atomic(pfn_to_page(hpa >> PAGE_SHIFT), KM_USER0);
 
-	ASSERT((!kvm_arch_ops->is_long_mode(vcpu) && is_pae(vcpu)) ||
+	ASSERT((!is_long_mode(vcpu) && is_pae(vcpu)) ||
 	       (vcpu->cr3 & ~(PAGE_MASK | CR3_FLAGS_MASK)) == 0);
 
 	walker->table = (pt_element_t *)( (unsigned long)walker->table |

@@ -131,7 +131,7 @@ static pt_element_t *FNAME(fetch_guest)(
 		     (walker->table[index] & PT_PAGE_SIZE_MASK) &&
 		     (PTTYPE == 64 || is_pse(vcpu))))
 			return &walker->table[index];
-		if (walker->level != 3 || kvm_arch_ops->is_long_mode(vcpu))
+		if (walker->level != 3 || is_long_mode(vcpu))
 			walker->inherited_ar &= walker->table[index];
 		paddr = safe_gpa_to_hpa(vcpu, walker->table[index] & PT_BASE_ADDR_MASK);
 		kunmap_atomic(walker->table, KM_USER0);

Index: linux-2.6/drivers/kvm/vmx.c
===================================================================
@@ -900,11 +900,6 @@ static void vmx_set_segment(struct kvm_v
 	vmcs_write32(sf->ar_bytes, ar);
 }
 
-static int vmx_is_long_mode(struct kvm_vcpu *vcpu)
-{
-	return vmcs_read32(VM_ENTRY_CONTROLS) & VM_ENTRY_CONTROLS_IA32E_MASK;
-}
-
 static void vmx_get_cs_db_l_bits(struct kvm_vcpu *vcpu, int *db, int *l)
 {
 	u32 ar = vmcs_read32(GUEST_CS_AR_BYTES);

@@ -1975,7 +1970,6 @@ static struct kvm_arch_ops vmx_arch_ops 
 	.get_segment_base = vmx_get_segment_base,
 	.get_segment = vmx_get_segment,
 	.set_segment = vmx_set_segment,
-	.is_long_mode = vmx_is_long_mode,
 	.get_cs_db_l_bits = vmx_get_cs_db_l_bits,
 	.set_cr0 = vmx_set_cr0,
 	.set_cr0_no_modeswitch = vmx_set_cr0_no_modeswitch,

From: Yoshimi Ichiyanagi <[email protected]>

The latest version of kvm doesn't initialize kvm_arch_ops in kvm_init(), which causes an error with the following sequence.

1. Load the supported arch's module.
2. Load the unsupported arch's module.$B!!(B(loading error)
3. Unload the unsupported arch's module.

You'll get the following error message after step 3. "BUG: unable to handle to handle kernel paging request at virtual address xxxxxxxx"

The problem here is that the unsupported arch's module overwrites kvm_arch_ops of the supported arch's module at step 2.

This patch initializes kvm_arch_ops upon loading architecture specific kvm module, and prevents overwriting kvm_arch_ops when kvm_arch_ops is already set correctly.

Index: linux-2.6/drivers/kvm/kvm_main.c
===================================================================
@@ -1865,6 +1865,11 @@ int kvm_init_arch(struct kvm_arch_ops *o
 {
 	int r;
 
+	if (kvm_arch_ops) {
+		printk(KERN_ERR "kvm: already loaded the other module\n");
+		return -EEXIST;
+	}
+
 	kvm_arch_ops = ops;
 
 	if (!kvm_arch_ops->cpu_has_kvm_support()) {

@@ -1907,6 +1912,7 @@ void kvm_exit_arch(void)
 	unregister_reboot_notifier(&kvm_reboot_notifier);
 	on_each_cpu(kvm_arch_ops->hardware_disable, 0, 0, 1);
 	kvm_arch_ops->hardware_unsetup();
+	kvm_arch_ops = NULL;
 }
 
 static __init int kvm_init(void)

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

btw., we've got a problem with KVM in -rt:

 BUG: scheduling while atomic: qemu/0x00000001/11445, CPU#0
  [<c0104fce>] dump_trace+0x63/0x1e8
  [<c010516c>] show_trace_log_lvl+0x19/0x2e
  [<c0105541>] show_trace+0x12/0x14
  [<c0105557>] dump_stack+0x14/0x16
  [<c0322662>] __schedule+0xae/0xd9f
  [<c0323597>] schedule+0xec/0x10f
  [<c0324017>] rt_spin_lock_slowlock+0xd2/0x15b
  [<c03244a6>] rt_spin_lock+0x1e/0x20
  [<c0169d94>] kunmap_high+0x11/0x99
  [<c0121139>] kunmap+0x40/0x42
  [<c0121171>] kunmap_virt+0x36/0x38
  [<f8f017b7>] emulator_read_std+0x92/0xaf [kvm]
  [<f8f05292>] x86_emulate_memop+0xc6/0x295c [kvm]
  [<f8f018c3>] emulate_instruction+0xef/0x202 [kvm]
  [<f8e0f2a7>] handle_exception+0x107/0x1c8 [kvm_intel]
  [<f8e0ef0c>] kvm_vmx_return+0x145/0x18d [kvm_intel]
  [<f8f0253c>] kvm_dev_ioctl+0x253/0xd76 [kvm]
  [<c01874f1>] do_ioctl+0x21/0x66
  [<c018778b>] vfs_ioctl+0x255/0x268
  [<c01877e6>] sys_ioctl+0x48/0x62
  [<c0103f8d>] syscall_call+0x7/0xb
  [<4a4538b2>] 0x4a4538b2

NOTE: this is not a worry for upstream kernel, it is caused by PREEMPT_RT scheduling in previously atomic APIs like kunmap(). But KVM used to work pretty nicely in -rt and this problem got introduced fairly recently, related to some big-page changes IIRC.

Any suggestions of how to best fix this in -rt? Basically, it would be nice to decouple KVM from get_cpu/preempt_disable type of interfaces as much as possible, to make it fully preemptible. Below are my current fixups for KVM, but the problem above looks harder to fix.

Ingo

---
 drivers/kvm/vmx.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux/drivers/kvm/vmx.c
===================================================================
@@ -117,7 +117,7 @@ static void vmcs_clear(struct vmcs *vmcs
 static void __vcpu_clear(void *arg)
 {
 	struct kvm_vcpu *vcpu = arg;
-	int cpu = smp_processor_id();
+	int cpu = raw_smp_processor_id();
 
 	if (vcpu->cpu == cpu)
 		vmcs_clear(vcpu->vmcs);

@@ -576,7 +576,7 @@ static struct vmcs *alloc_vmcs_cpu(int c
 
 static struct vmcs *alloc_vmcs(void)
 {
-	return alloc_vmcs_cpu(smp_processor_id());
+	return alloc_vmcs_cpu(raw_smp_processor_id());
 }
 
 static void free_vmcs(struct vmcs *vmcs)

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

This is not a recent change. The x86 emulator is fetching an instruction or a memory operand.

kvm on intel uses hidden cpu registers, so it can't be migrated as a normal process. We'd need a hook on context switch to check if we migrated to another cpu, issue an ipi to unload the registers on the previous cpu, and reload the registers on the current cpu. The hook would need to be protected from context switches. See vmx.c:vmx_vcpu_load() for the gory details. The actual launch would require a small non-preemptible section as well, on both intel and AMD.

Talking about the scheduler (and to the scheduler's author :), it would be nice to hook the migration weight algorithm too. kvm guests are considerably more expensive to migrate, at least on intel.

These make sense for mainline (especially the first hunk), so I'll apply them.

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

well, the fact that it's somehow ending up in highmem and thus the kmap becomes nontrivial could be a recent change maybe?

oh, granted. My suggestion would be to make it more atomic - not to break the physical attachment of a KVM context to a physical CPU (which is unbreakable, obviously).

actually, another, possibly better approach would be to attach a KVM context to tasks, automatically loaded/unloaded upon switch_to(). No need to do IPIs. Would this be fast enough?

Ingo

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Guest memory was always highmem. If a particular page is highmem is of course runtime dependent.

You don't want or need to unload on switching out, only on migration. Since migrations are hopefully few and far between, it's best to let them handle the heavy lifting and keep non-migrating context switches cheap.

If we touch the scheduler, maybe we can let the migration threads call a kvm callback to unload the vmx context.

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

i have worked the problem around via the change below. This affects 32-bit hosts only, and only on CONFIG_PREEMPT_RT.

with this and the other two patches kvm/vmx works on 32-bit -rt hosts.

Ingo

Index: linux/drivers/kvm/kvm_main.c
===================================================================
@@ -628,8 +628,12 @@ raced:
 
 		memset(new.phys_mem, 0, npages * sizeof(struct page *));
 		for (i = 0; i < npages; ++i) {
+#ifdef CONFIG_PREEMPT_RT
+			new.phys_mem[i] = alloc_page(GFP_USER | __GFP_ZERO);
+#else
 			new.phys_mem[i] = alloc_page(GFP_HIGHUSER
 						     | __GFP_ZERO);
+#endif
 			if (!new.phys_mem[i])
 				goto out_free;
 		}

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

what do you mean precisely, and where does the cost come from? To me it seems the cost of loading/saving a VMX context on Intel is the biggest cost, and that's roughly the same cost for VM exits/enters, regardless of what the scheduler does. What am i missing?

Ingo

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

There are three types of vmx context switches.

1. The most common switch is a vm entry/exit pair (vmresume instruction). The context is already loaded into the cpu (via a vmptrld, below), and we enter into a preloaded VM. In addition, register access implicitly addresses the loaded context.

2. The second most common switch is loading another context. If we have several VMs on a uniprocessor machine, the execution sequence looks like this:

vmptrld vm1

... access context registers (vmread/vmwrite)
vmresume
... access context registers (vmread/vmwrite)

... access context registers (vmread/vmwrite)
vmresume
... access context registers (vmread/vmwrite)

... access context registers (vmread/vmwrite)
vmresume
... access context registers (vmread/vmwrite)

Linux context switch to another kvm process

vmptrld vm2

... access context registers (vmread/vmwrite)
vmresume
... access context registers (vmread/vmwrite)

... access context registers (vmread/vmwrite)
vmresume
... access context registers (vmread/vmwrite)

Linux context switch to back to original process

vmptrld vm1

Linux context switches to non-kvm processes and back need no special handling.

The processor caches the current VM context, and possibly more than one.

3. The most expensive vmx context switch involves cpu migration:

cpu 0:  vmclear vm1 /* decache vmx context into memory */
cpu 1:  vmptrld vm1

... vm register access
vmlaunch

The vmlaunch instruction, like vmresume, causes a VM entry, but is documented to be significantly more expensive. It is required after a vmclear.

Currently, the vmclear is performed by an ipi, because we can only detect migration after the fact. However, if we enlist the migration threads, we can vmclear before the process has left the cpu.

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

ah. And you optimize this away if previously-used-CPU == curr-CPU and the vmx context loaded on this CPU is the same as we are trying to run now, right?

the most common type of migration isnt even in the migration threads but in simple try_to_wake_up(). And that call often does not run on the CPU that has the VMX state ...

i see no easy solution here - this really parallels all the lazy-FPU problems. Would it really be all that expensive to just save/load the VMX state in switch_to()? As SVN has shown it, we can rely on VMX state save/load to become faster in the future. So we definitely shouldnt design for a small-scale overhead in first-generation silicon.

Ingo

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Yes. If we changed cpu, we do a ipi vmclear and a vmptrld. If we just changed vm, we only do a vmptrld. If nothing changed, we do nothing.

The Intel documentation explicitly states that a vmlaunch (required after flushing the vmx state using a vmclear) is much more expensive than a vmresume. I'd expect the difference to become more important if they cache multiple VMs on one core, which seems to be the point of all this complexity.

In this case I think the documentation indicates their long term plans. However, the only real answer is to measure.

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

yeah. Would be nice to see some hard numbers about how many cycles all these context load/save variants take.

Ingo

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

PIO latency on AMD (including a trip to qemu and back) is 5500 cycles [1]. Intel is significantly higher.

[1] http://virt.kernelnewbies.org/KVM/Performance

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

ok. We need the IPI only on VMX, right? That's because it has cached VM state in the CPU that needs to be flushed out to the public VM area before it can be loaded into another CPU, correct? Is there no cheap way to do this flushing preemptively (say in vcpu_put()), to make the VM context potentially loadable into another CPU?

Ingo

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Correct.

Correct.

You could issue a vmclear, but that forces a vm entry on the _same_ cpu to use vmlaunch instead of vmresume, which is documented as expensive. If we assume that cpu migrations are much rarer than vm entries (a very safe assumption), then the ipi is better than forcing a vmclear on vcpu_put().

Here's the relevant text from scripture:

The following software usage is consistent with these limitations:
• VMCLEAR should be executed for a VMCS before it is used for VM entry.
• VMLAUNCH should be used for the first VM entry using a VMCS after VMCLEAR has
been executed for that VMCS.
• VMRESUME should be used for any subsequent VM entry using a VMCS (until the next
execution of VMCLEAR for the VMCS).
It is expected that, in general, VMRESUME will have lower latency than VMLAUNCH. Since
“migrating” a VMCS from one logical processor to another requires use of VMCLEAR (see
Section 20.10.1), which sets the launch state of the VMCS to “clear,” such migration requires
the next VM entry to be performed using VMLAUNCH. Software developers can avoid the
performance cost of increased VM-entry latency by avoiding unnecessary migration of a VMCS
from one logical processor to another.

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

hm. Could you try to just do a vmresume nevertheless - maybe it works but is undocumented? What would be nice is a 'vmflush' instruction that still keeps the state usable.

doh, very helpful ...

Ingo

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
kvm-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

I'd hate to do that. There are at least three implementations of VT (PD, Core, Core2) and maybe more when steppings and microcode updates are considered. Not to mention having kvm fail when a new cpu is introduced is not a pleasant scenario.

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

lets check whether it's even possible ... Then we could ask Intel to clarify their position on this. I can see no real reason why a vmclear should really make it impossible to re-use the already loaded context data /on the same CPU/. And that is the important case. When we migrate to another CPU we have to do a VMLAUNCH anyway, upon the first execution of that context.

Ingo

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

There's a documented error number, 5, for "VMRESUME with non-launched vmcs".

Furthermore, I checked, and it indeed doesn't work.

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

btw, there is a large number of checks in the vmx spec, and I know of none which are not implemented. It seems Intel is (understandably) wary of software exploiting undocumented behavior, which would force them to support it in future processors to maintain compatibility.

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Resolves sourceforge bug 1622229 (guest crashes running benchmark software).

Index: linux-2.6/drivers/kvm/svm.c
===================================================================
@@ -1068,6 +1068,9 @@ static int emulate_on_interception(struc
 static int svm_get_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 *data)
 {
 	switch (ecx) {
+	case 0xc0010010: /* SYSCFG */
+	case 0xc0010015: /* HWCR */
+	case MSR_IA32_PLATFORM_ID:
 	case MSR_IA32_P5_MC_ADDR:
 	case MSR_IA32_P5_MC_TYPE:
 	case MSR_IA32_MC0_CTL:

Index: linux-2.6/drivers/kvm/vmx.c
===================================================================
@@ -359,6 +359,9 @@ static int vmx_get_msr(struct kvm_vcpu *
 	case MSR_IA32_SYSENTER_ESP:
 		data = vmcs_read32(GUEST_SYSENTER_ESP);
 		break;
+	case 0xc0010010: /* SYSCFG */
+	case 0xc0010015: /* HWCR */
+	case MSR_IA32_PLATFORM_ID:
 	case MSR_IA32_P5_MC_ADDR:
 	case MSR_IA32_P5_MC_TYPE:
 	case MSR_IA32_MC0_CTL:

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Hi,

Yes, there are a few more of these as well. I'll clean them up once things quiet down a bit.

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

These msrs are referenced by benchmarking software when pretending to be an Intel cpu.

Index: linux-2.6/drivers/kvm/kvm_main.c
===================================================================
@@ -1122,11 +1122,15 @@ int kvm_get_msr_common(struct kvm_vcpu *
 	case MSR_IA32_MC0_MISC+12:
 	case MSR_IA32_MC0_MISC+16:
 	case MSR_IA32_UCODE_REV:
+	case MSR_IA32_PERF_STATUS:
 		/* MTRR registers */
 	case 0xfe:
 	case 0x200 ... 0x2ff:
 		data = 0;
 		break;
+	case 0xcd: /* fsb frequency */
+		data = 3;
+		break;
 	case MSR_IA32_APICBASE:
 		data = vcpu->apic_base;
 		break;

From: Nguyen Anh Quynh <[email protected]>

No need to append _MSR to msr names, a prefix should suffice.

Index: linux-2.6/drivers/kvm/vmx.c
===================================================================
@@ -26,7 +26,6 @@
 
 #include "segment_descriptor.h"
 
-#define MSR_IA32_FEATURE_CONTROL 		0x03a
 
 MODULE_AUTHOR("Qumranet");
 MODULE_LICENSE("GPL");

@@ -519,11 +518,11 @@ static __init void setup_vmcs_descriptor
 {
 	u32 vmx_msr_low, vmx_msr_high;
 
-	rdmsr(MSR_IA32_VMX_BASIC_MSR, vmx_msr_low, vmx_msr_high);
+	rdmsr(MSR_IA32_VMX_BASIC, vmx_msr_low, vmx_msr_high);
 	vmcs_descriptor.size = vmx_msr_high & 0x1fff;
 	vmcs_descriptor.order = get_order(vmcs_descriptor.size);
 	vmcs_descriptor.revision_id = vmx_msr_low;
-};
+}
 
 static struct vmcs *alloc_vmcs_cpu(int cpu)
 {

@@ -1039,12 +1038,12 @@ static int vmx_vcpu_setup(struct kvm_vcp
 	vmcs_write64(GUEST_IA32_DEBUGCTL, 0);
 
 	/* Control */
-	vmcs_write32_fixedbits(MSR_IA32_VMX_PINBASED_CTLS_MSR,
+	vmcs_write32_fixedbits(MSR_IA32_VMX_PINBASED_CTLS,
 			       PIN_BASED_VM_EXEC_CONTROL,
 			       PIN_BASED_EXT_INTR_MASK   /* 20.6.1 */
 			       | PIN_BASED_NMI_EXITING   /* 20.6.1 */
 			);
-	vmcs_write32_fixedbits(MSR_IA32_VMX_PROCBASED_CTLS_MSR,
+	vmcs_write32_fixedbits(MSR_IA32_VMX_PROCBASED_CTLS,
 			       CPU_BASED_VM_EXEC_CONTROL,
 			       CPU_BASED_HLT_EXITING         /* 20.6.2 */
 			       | CPU_BASED_CR8_LOAD_EXITING    /* 20.6.2 */

@@ -1127,7 +1126,7 @@ static int vmx_vcpu_setup(struct kvm_vcp
 		    virt_to_phys(vcpu->guest_msrs + NR_BAD_MSRS));
 	vmcs_writel(VM_EXIT_MSR_LOAD_ADDR,
 		    virt_to_phys(vcpu->host_msrs + NR_BAD_MSRS));
-	vmcs_write32_fixedbits(MSR_IA32_VMX_EXIT_CTLS_MSR, VM_EXIT_CONTROLS,
+	vmcs_write32_fixedbits(MSR_IA32_VMX_EXIT_CTLS, VM_EXIT_CONTROLS,
 		     	       (HOST_IS_64 << 9));  /* 22.2,1, 20.7.1 */
 	vmcs_write32(VM_EXIT_MSR_STORE_COUNT, nr_good_msrs); /* 22.2.2 */
 	vmcs_write32(VM_EXIT_MSR_LOAD_COUNT, nr_good_msrs);  /* 22.2.2 */

@@ -1135,7 +1134,7 @@ static int vmx_vcpu_setup(struct kvm_vcp
 
 
 	/* 22.2.1, 20.8.1 */
-	vmcs_write32_fixedbits(MSR_IA32_VMX_ENTRY_CTLS_MSR,
+	vmcs_write32_fixedbits(MSR_IA32_VMX_ENTRY_CTLS,
                                VM_ENTRY_CONTROLS, 0);
 	vmcs_write32(VM_ENTRY_INTR_INFO_FIELD, 0);  /* 22.2.1 */
 

Index: linux-2.6/drivers/kvm/vmx.h
===================================================================
@@ -286,11 +286,11 @@ enum vmcs_field {
 
 #define CR4_VMXE 0x2000
 
-#define MSR_IA32_VMX_BASIC_MSR   		0x480
+#define MSR_IA32_VMX_BASIC   		0x480
 #define MSR_IA32_FEATURE_CONTROL 		0x03a
-#define MSR_IA32_VMX_PINBASED_CTLS_MSR		0x481
-#define MSR_IA32_VMX_PROCBASED_CTLS_MSR		0x482
-#define MSR_IA32_VMX_EXIT_CTLS_MSR		0x483
-#define MSR_IA32_VMX_ENTRY_CTLS_MSR		0x484
+#define MSR_IA32_VMX_PINBASED_CTLS		0x481
+#define MSR_IA32_VMX_PROCBASED_CTLS		0x482
+#define MSR_IA32_VMX_EXIT_CTLS		0x483
+#define MSR_IA32_VMX_ENTRY_CTLS		0x484
 
 #endif

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

__free_page() doesn't like a NULL argument, so check before calling it. A NULL can only happen if memory is exhausted during allocation of a memory slot.

Index: linux-2.6/drivers/kvm/kvm_main.c
===================================================================
@@ -245,7 +245,8 @@ static void kvm_free_physmem_slot(struct
 	if (!dont || free->phys_mem != dont->phys_mem)
 		if (free->phys_mem) {
 			for (i = 0; i < free->npages; ++i)
-				__free_page(free->phys_mem[i]);
+				if (free->phys_mem[i])
+					__free_page(free->phys_mem[i]);
 			vfree(free->phys_mem);
 		}
 

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Subject: [patch] kvm: fix GFP_KERNEL allocation in atomic section in kvm_dev_ioctl_create_vcpu()
From: Ingo Molnar <[email protected]>

fix a GFP_KERNEL allocation in atomic section bug:
kvm_dev_ioctl_create_vcpu() called kvm_mmu_init(), which calls alloc_pages(), while holding the vcpu. The fix is to set up the MMU state earlier, it does not require a loaded CPU state.

(NOTE: free_vcpus does an kvm_mmu_destroy() call so there's no need
for any extra teardown branch on allocation failure here.)

found in 2.6.20-rc2-rt1.

---
 drivers/kvm/kvm_main.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Index: linux/drivers/kvm/kvm_main.c
===================================================================
@@ -522,12 +522,12 @@ static int kvm_dev_ioctl_create_vcpu(str
 	if (r < 0)
 		goto out_free_vcpus;
 
-	kvm_arch_ops->vcpu_load(vcpu);
+	r = kvm_mmu_init(vcpu);
+	if (r < 0)
+		goto out_free_vcpus;
 
+	kvm_arch_ops->vcpu_load(vcpu);
 	r = kvm_arch_ops->vcpu_setup(vcpu);
-	if (r >= 0)
-		r = kvm_mmu_init(vcpu);
-
 	vcpu_put(vcpu);
 
 	if (r < 0)

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Yes it does. It calls nonpaging_init_context() which calls vmx_set_cr3() which promptly trashes address space of the VM that previously ran on that vcpu (or, if there were none, logs a vmwrite error).

error compiling committee.c: too many arguments to function

ok, i missed that. Nevertheless the problem of the nonatomic alloc remains. I guess a kvm_mmu_init() needs to be split into kvm_mmu_create() and kvm_mmu_setup()?

Ingo

the patch below implements this fix. Lightly tested on 32-bit/VMX on 2.6.20-rc2-rt2 but seems to have done the trick.

Ingo

-------------------->
Subject: [patch] kvm: fix GFP_KERNEL allocation in atomic section in kvm_dev_ioctl_create_vcpu()
From: Ingo Molnar <[email protected]>

fix an GFP_KERNEL allocation in atomic section:
kvm_dev_ioctl_create_vcpu() called kvm_mmu_init(), which calls alloc_pages(), while holding the vcpu.

The fix is to set up the MMU state in two phases: kvm_mmu_create() and kvm_mmu_setup().

(NOTE: free_vcpus does an kvm_mmu_destroy() call so there's no need
for any extra teardown branch on allocation/init failure here.)

---
 drivers/kvm/kvm.h      |    3 ++-
 drivers/kvm/kvm_main.c |   10 ++++++----
 drivers/kvm/mmu.c      |   24 +++++++++---------------
 3 files changed, 17 insertions(+), 20 deletions(-)

Index: linux/drivers/kvm/kvm.h
===================================================================
@@ -319,7 +319,8 @@ int kvm_init_arch(struct kvm_arch_ops *o
 void kvm_exit_arch(void);
 
 void kvm_mmu_destroy(struct kvm_vcpu *vcpu);
-int kvm_mmu_init(struct kvm_vcpu *vcpu);
+int kvm_mmu_create(struct kvm_vcpu *vcpu);
+int kvm_mmu_setup(struct kvm_vcpu *vcpu);
 
 int kvm_mmu_reset_context(struct kvm_vcpu *vcpu);
 void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot);

Index: linux/drivers/kvm/kvm_main.c
===================================================================
@@ -522,12 +522,14 @@ static int kvm_dev_ioctl_create_vcpu(str
 	if (r < 0)
 		goto out_free_vcpus;
 
-	kvm_arch_ops->vcpu_load(vcpu);
+	r = kvm_mmu_create(vcpu);
+	if (r < 0)
+		goto out_free_vcpus;
 
-	r = kvm_arch_ops->vcpu_setup(vcpu);
+	kvm_arch_ops->vcpu_load(vcpu);
+	r = kvm_mmu_setup(vcpu);
 	if (r >= 0)
-		r = kvm_mmu_init(vcpu);
-
+		r = kvm_arch_ops->vcpu_setup(vcpu);
 	vcpu_put(vcpu);
 
 	if (r < 0)

Index: linux/drivers/kvm/mmu.c
===================================================================
@@ -639,28 +639,22 @@ error_1:
 	return -ENOMEM;
 }
 
-int kvm_mmu_init(struct kvm_vcpu *vcpu)
+int kvm_mmu_create(struct kvm_vcpu *vcpu)
 {
-	int r;
-
 	ASSERT(vcpu);
 	ASSERT(!VALID_PAGE(vcpu->mmu.root_hpa));
 	ASSERT(list_empty(&vcpu->free_pages));
 
-	r = alloc_mmu_pages(vcpu);
-	if (r)
-		goto out;
-
-	r = init_kvm_mmu(vcpu);
-	if (r)
-		goto out_free_pages;
+	return alloc_mmu_pages(vcpu);
+}
 
-	return 0;
+int kvm_mmu_setup(struct kvm_vcpu *vcpu)
+{
+	ASSERT(vcpu);
+	ASSERT(!VALID_PAGE(vcpu->mmu.root_hpa));
+	ASSERT(!list_empty(&vcpu->free_pages));
 
-out_free_pages:
-	free_mmu_pages(vcpu);
-out:
-	return r;
+	return init_kvm_mmu(vcpu);
 }
 
 void kvm_mmu_destroy(struct kvm_vcpu *vcpu)

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Applied, thanks.

error compiling committee.c: too many arguments to function

great!

I've got a security related question as well: vcpu_load() sets up a physical CPU's VM registers/state, and vcpu_put() drops that. But vcpu_put() only does a put_cpu() call - it does not tear down any VM state that has been loaded into the CPU. Is it guaranteed that (hostile) user-space cannot use that VM state in any unauthorized way? The state is still loaded while arbitrary tasks execute on the CPU. The next vcpu_load() will then override it, but the state lingers around forever.

The new x86 VM instructions: vmclear, vmlaunch, vmresume, vmptrld, vmread, vmwrite, vmxoff, vmxon are all privileged so i guess it should be mostly safe - i'm just wondering whether you thought about this attack angle.

ultimately we want to integrate VM state management into the scheduler and the context-switch lowlevel arch code, but right now CPU state management is done by the KVM 'driver' and there's nothing that isolates other tasks from possible side-effects of a loaded VMX/SVN state.

Ingo

Yes. Userspace cannot snoop on a VM state.

AFAICS in vmx root mode the vm state only affects vmx instructions; SVM has no architecturally hidden state.

error compiling committee.c: too many arguments to function