BUG with Win7 and user-return-notifier

DISCUSSION Jan Kiszka 3 participants 20 messages Oct 27, 2009 – Oct 29, 2009 1 message not in the archive

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Jan Kiszka

author

2009-10-27 12:52 UTC #0 lore raw

Hi Avi,

just booted kvm.git master (974ae8d7ff) as host and re-ran my boot test of Windows 7. Already during "Starting Windows" I get this:

...

general protection fault: 0000 [#1] PREEMPT SMP
last sysfs file: /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0A:01/power_supply/CMB2/charge_full
CPU 0
Modules linked in: kvm_intel kvm ip6t_LOG xt_pkttype xt_TCPMSS xt_tcpudp ipt_LOG xt_limit iptable_nat nf_nat xt_physdev sco bridge stp bnep rfcomm l2cap snd_pcm_oss snd_mixer_oss crc16 snd_seq coretemp snd_seq_device i915 drm_kms_helper drm i2c_algo_bit af_packet ip6t_REJECT nf_conntrack_ipv6 ip6table_raw xt_NOTRACK ipt_REJECT xt_state iptable_raw iptable_filter ip6table_mangle nf_conntrack_netbios_ns nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 ip_tables ip6table_filter ip6_tables x_tables ipv6 microcode fuse ohci_hcd loop snd_hda_codec_realtek arc4 ecb snd_hda_intel ath5k snd_hda_codec mac80211 ath snd_hwdep btusb pcmcia snd_pcm sdhci_pci video snd_timer cfg80211 iTCO_wdt rtc_cmos ppdev bluetooth sdhci yenta_socket iTCO_vendor_support i2c_i801 snd rsrc_nonstatic fujitsu_laptop rtc_core 
 soundcore mmc_core output ohci1394 parport_pc battery rfkill ac rtc_lib i2c_core snd_page_alloc pcmcia_core intel_agp parport joydev serio_raw led_class ieee1394 pcspkr button sky2 sg sha256
_generic aes_generic cbc dm_crypt usbhid hid ehci_hcd uhci_hcd sd_mod crc_t10dif usbcore dm_snapshot dm_mod edd ext3 mbcache jbd fan thermal_sys hwmon ata_piix ahci libata scsi_mod
Pid: 7404, comm: qemu-system-x86 Not tainted 2.6.32-rc5 #8 LIFEBOOK E8110
RIP: 0010:[<ffffffffa05853ad>]  [<ffffffffa05853ad>] kvm_set_shared_msr+0x51/0x7a [kvm]
RSP: 0018:ffff880060947cc8  EFLAGS: 00010246
RAX: 0000000000000000 RBX: 0000000000000003 RCX: 00000000c0000080
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000003
RBP: ffff880060947ce8 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000012 R11: ffff880062b01158 R12: ffff88000601dad0
R13: 0000000000000000 R14: ffff880062b00058 R15: ffff880062b00059
FS:  00007f91606df950(0000) GS:ffff880006000000(0000) knlGS:0000000000000000
CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
CR2: 0000000000000000 CR3: 00000000665ab000 CR4: 00000000000026f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process qemu-system-x86 (pid: 7404, threadinfo ffff880060946000, task ffff880062989950)
Stack:
 0000000060947cf8 0000000000000004 ffff880062b00000 0000000000000000
<0> ffff880060947d08 ffffffffa05d1ef9 ffff880060991f80 ffff880062b00000
<0> ffff880060947dd8 ffffffffa0582e40 ffff880060947d38 000000000001db70
Call Trace:
 [<ffffffffa05d1ef9>] vmx_save_host_state+0x141/0x150 [kvm_intel]
 [<ffffffffa0582e40>] kvm_arch_vcpu_ioctl_run+0x510/0xb25 [kvm]
 [<ffffffffa0571dda>] kvm_vcpu_ioctl+0xfb/0x722 [kvm]
 [<ffffffffa05743de>] ? kvm_vm_ioctl+0x33a/0x36b [kvm]
 [<ffffffff81330022>] ? sub_preempt_count+0x9/0x83
 [<ffffffff810cb698>] ? fire_user_return_notifiers+0x50/0x6b
 [<ffffffff81122bf4>] vfs_ioctl+0x2f/0x7d
 [<ffffffff81123112>] do_vfs_ioctl+0x450/0x48d
 [<ffffffff81330022>] ? sub_preempt_count+0x9/0x83
 [<ffffffff811231a9>] sys_ioctl+0x5a/0x7c
 [<ffffffff8100bc5b>] system_call_fastpath+0x16/0x1b
Code: 03 24 c5 60 a8 6f 81 89 d8 48 8d 50 04 4d 3b 2c d4 74 38 4d 89 2c d4 48 c1 e0 04 4c 89 ea 8b 88 f8 50 5a a0 48 c1 ea 20 44 89 e8 <0f> 30 41 80 7c 24 18 00 75 16 49 c7 04 24 d6 53 58 a0 4c 89 e7
RIP  [<ffffffffa05853ad>] kvm_set_shared_msr+0x51/0x7a [kvm]
 RSP <ffff880060947cc8>
---[ end trace 44d1410c7cb2d885 ]---

note: qemu-system-x86[7404] exited with preempt_count 1

So the problem is not kvm-kmod related. Any ideas? Need .config or other additional information?

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Avi Kivity

2009-10-27 13:11 UTC #1 lore raw

Quoted 5 lines · Jan Kiszka

Hi Avi,
just booted kvm.git master (974ae8d7ff) as host and re-ran my boot test of Windows 7. Already during "Starting Windows" I get this:

x86 or x64 7?

Quoted 18 lines

RAX: 0000000000000000 RBX: 0000000000000003 RCX: 00000000c0000080
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000003

Call Trace:
  [<ffffffffa05d1ef9>] vmx_save_host_state+0x141/0x150 [kvm_intel]
  [<ffffffffa0582e40>] kvm_arch_vcpu_ioctl_run+0x510/0xb25 [kvm]
  [<ffffffffa0571dda>] kvm_vcpu_ioctl+0xfb/0x722 [kvm]
  [<ffffffffa05743de>] ? kvm_vm_ioctl+0x33a/0x36b [kvm]
  [<ffffffff81330022>] ? sub_preempt_count+0x9/0x83
  [<ffffffff810cb698>] ? fire_user_return_notifiers+0x50/0x6b
  [<ffffffff81122bf4>] vfs_ioctl+0x2f/0x7d
  [<ffffffff81123112>] do_vfs_ioctl+0x450/0x48d
  [<ffffffff81330022>] ? sub_preempt_count+0x9/0x83
  [<ffffffff811231a9>] sys_ioctl+0x5a/0x7c
  [<ffffffff8100bc5b>] system_call_fastpath+0x16/0x1b
Code: 03 24 c5 60 a8 6f 81 89 d8 48 8d 50 04 4d 3b 2c d4 74 38 4d 89 2c d4 48 c1 e0 04 4c 89 ea 8b 88 f8 50 5a a0 48 c1 ea 20 44 89 e8<0f>  30 41 80 7c 24 18 00 75 16 49 c7 04 24 d6 53 58 a0 4c 89 e7
RIP  [<ffffffffa05853ad>] kvm_set_shared_msr+0x51/0x7a [kvm]

That's a wrmsr(EFER, 0), which should definitely fault. update_transition_efer() should have taken care of it.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2009-10-27 13:13 UTC #2 lore raw

Quoted 8 lines · Avi Kivity

Quoted 5 lines · Jan Kiszka
Hi Avi,
just booted kvm.git master (974ae8d7ff) as host and re-ran my boot test of Windows 7. Already during "Starting Windows" I get this:
x86 or x64 7?

x64.

Quoted 20 lines

Quoted 4 lines

RAX: 0000000000000000 RBX: 0000000000000003 RCX: 00000000c0000080
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000003

Quoted 15 lines

Call Trace:
  [<ffffffffa05d1ef9>] vmx_save_host_state+0x141/0x150 [kvm_intel]
  [<ffffffffa0582e40>] kvm_arch_vcpu_ioctl_run+0x510/0xb25 [kvm]
  [<ffffffffa0571dda>] kvm_vcpu_ioctl+0xfb/0x722 [kvm]
  [<ffffffffa05743de>] ? kvm_vm_ioctl+0x33a/0x36b [kvm]
  [<ffffffff81330022>] ? sub_preempt_count+0x9/0x83
  [<ffffffff810cb698>] ? fire_user_return_notifiers+0x50/0x6b
  [<ffffffff81122bf4>] vfs_ioctl+0x2f/0x7d
  [<ffffffff81123112>] do_vfs_ioctl+0x450/0x48d
  [<ffffffff81330022>] ? sub_preempt_count+0x9/0x83
  [<ffffffff811231a9>] sys_ioctl+0x5a/0x7c
  [<ffffffff8100bc5b>] system_call_fastpath+0x16/0x1b
Code: 03 24 c5 60 a8 6f 81 89 d8 48 8d 50 04 4d 3b 2c d4 74 38 4d 89 2c d4 48 c1 e0 04 4c 89 ea 8b 88 f8 50 5a a0 48 c1 ea 20 44 89 e8<0f>  30 41 80 7c 24 18 00 75 16 49 c7 04 24 d6 53 58 a0 4c 89 e7
RIP  [<ffffffffa05853ad>] kvm_set_shared_msr+0x51/0x7a [kvm]

Quoted 3 lines

That's a wrmsr(EFER, 0), which should definitely fault. update_transition_efer() should have taken care of it.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Avi Kivity

2009-10-27 13:24 UTC #3 lore raw

Quoted 6 lines · Jan Kiszka

Quoted 4 lines · Avi Kivity
Quoted 2 lines · Jan Kiszka
Hi Avi,

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
just booted kvm.git master (974ae8d7ff) as host and re-ran my boot test of Windows 7. Already during "Starting Windows" I get this:
x86 or x64 7?
x64.

Worked for me - getting to the initial prompt. Do you have

CONFIG_USER_RETURN_NOTIFIER=y

in your .config?

Signature · 1 line

error compiling committee.c: too many arguments to function

Avi Kivity

2009-10-27 13:25 UTC #4 lore raw

Quoted 7 lines · Avi Kivity

Worked for me - getting to the initial prompt. Do you have
CONFIG_USER_RETURN_NOTIFIER=y
in your .config?

If you do, send your own .config, will try to reproduce.

Signature · 1 line

error compiling committee.c: too many arguments to function

Avi Kivity

2009-10-28 08:18 UTC #5 lore raw

Quoted 11 lines · Avi Kivity

Quoted 7 lines · Avi Kivity
Worked for me - getting to the initial prompt. Do you have
CONFIG_USER_RETURN_NOTIFIER=y
in your .config?
If you do, send your own .config, will try to reproduce.

As I can't reproduce it, can you send a trace of what's going on?

The kvm:kvm_msr and kvm:kvm_cr events should suffice to understand what's going on. Please enlarge your buffer size (buffer_size_kb) so we don't drop events.

Signature · 1 line

Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Jan Kiszka

author

2009-10-28 14:01 UTC #6 lore raw

Quoted 8 lines · Avi Kivity

Quoted 7 lines · Avi Kivity
Quoted 6 lines · Avi Kivity
Worked for me - getting to the initial prompt. Do you have
CONFIG_USER_RETURN_NOTIFIER=y
in your .config?

Quoted 9 lines

Quoted 2 lines
If you do, send your own .config, will try to reproduce.
As I can't reproduce it, can you send a trace of what's going on?
The kvm:kvm_msr and kvm:kvm_cr events should suffice to understand what's going on. Please enlarge your buffer size (buffer_size_kb) so we don't drop events.

Find such a trace attached. I hope I caught all important events (there were tons of identical kvm_cr events before them which I cut off).

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Avi Kivity

2009-10-28 16:00 UTC #7 lore raw

Quoted 18 lines · Jan Kiszka

Quoted 16 lines · Avi Kivity
Quoted 12 lines · Avi Kivity
Quoted 7 lines · Avi Kivity
Worked for me - getting to the initial prompt. Do you have
CONFIG_USER_RETURN_NOTIFIER=y
in your .config?
If you do, send your own .config, will try to reproduce.
As I can't reproduce it, can you send a trace of what's going on?

Quoted 8 lines

Quoted 5 lines
The kvm:kvm_msr and kvm:kvm_cr events should suffice to understand what's going on. Please enlarge your buffer size (buffer_size_kb) so we don't drop events.
Find such a trace attached. I hope I caught all important events (there were tons of identical kvm_cr events before them which I cut off).

[you can get longer, more detailed traces by using /sys/kernel/debug/tracing/trace instead of dmesg]

Oct 28 14:29:56 mchn012c kernel: qemu-sys-7200    0...1. 676996395us : kvm_msr: msr_read c0000080 = 0x500
Oct 28 14:29:56 mchn012c kernel: qemu-sys-7200    0...1. 676996403us : kvm_msr: msr_write c0000080 = 0xd01

So Windows is setting EFER.SCE and EFER.NX while in long mode - perfectly reasonable. Can you rerun with the attached debug patch?

Signature · 1 line

error compiling committee.c: too many arguments to function

arch/x86/kvm/vmx.c +8 -2

index 85f97d1..6bd6d2c 100644
@@ -590,6 +590,8 @@ static bool update_transition_efer(struct vcpu_vmx *vmx)
 	u64 guest_efer;
 	u64 ignore_bits;
 
+	printk("%s: efer_offset %d efer %llx\n",
+	       __func__, efer_offset, vmx->vcpu.arch.shadow_efer);
 	if (efer_offset < 0)
 		return false;
 	guest_efer = vmx->vcpu.arch.shadow_efer;

@@ -606,10 +608,11 @@ static bool update_transition_efer(struct vcpu_vmx *vmx)
 		ignore_bits &= ~(u64)EFER_SCE;
 #endif
 	if ((guest_efer & ~ignore_bits) == (host_efer & ~ignore_bits))
-		return false;
+		return printk("%s: ignoring all bits\n", __func__), false;
 
 	guest_efer &= ~ignore_bits;
 	guest_efer |= host_efer & ignore_bits;
+	printk("%s: transition efer %llx\n", __func__, guest_efer);
 	vmx->guest_msrs[efer_offset].data = guest_efer;
 	return true;
 }

@@ -928,8 +931,11 @@ static void setup_msrs(struct vcpu_vmx *vmx)
 	}
 #endif
 	vmx->msr_offset_efer = index = __find_msr_index(vmx, MSR_EFER);
-	if (index >= 0 && update_transition_efer(vmx))
+	if (index >= 0 && update_transition_efer(vmx)) {
+		printk("%s: marking efer for reload\n", __func__);
 		move_msr_up(vmx, index, save_nmsrs++);
+	} else
+		printk("%s: marking efer for no reload\n", __func__);
 
 	vmx->save_nmsrs = save_nmsrs;

Avi Kivity

2009-10-29 07:37 UTC #10 lore raw

In reply to a message not in the archive

Quoted 9 lines · Jan Kiszka

Quoted 9 lines
[you can get longer, more detailed traces by using /sys/kernel/debug/tracing/trace instead of dmesg]
Oct 28 14:29:56 mchn012c kernel: qemu-sys-7200    0...1. 676996395us :
kvm_msr: msr_read c0000080 = 0x500
Oct 28 14:29:56 mchn012c kernel: qemu-sys-7200    0...1. 676996403us :
kvm_msr: msr_write c0000080 = 0xd01

Quoted 6 lines

Quoted 4 lines
So Windows is setting EFER.SCE and EFER.NX while in long mode - perfectly reasonable. Can you rerun with the attached debug patch?
Log attached.

So the last bits are:

Oct 28 21:26:41 mchn012c kernel: update_transition_efer: efer_offset 4 efer d01
Oct 28 21:26:41 mchn012c kernel: update_transition_efer: ignoring all bits
Oct 28 21:26:41 mchn012c kernel: setup_msrs: marking efer for no reload

We're not reloading efer (correctly, as guest efer == host efer), yet vmx_save_host_state() fails while loading efer. I've looked at move_msr_up() (which is used by setup_msrs() to partition the msr space into reloaded and non-reloaded msrs), and it seems correct.

Can you see any way where update_transition_efer() returns false, yet efer turns up in the first save_nmsrs entries of vmx->guest_msrs?

Signature · 1 line

Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Jan Kiszka

2009-10-29 08:03 UTC #11 lore raw

Quoted 24 lines · Avi Kivity

Quoted 9 lines · Jan Kiszka
Quoted 9 lines
[you can get longer, more detailed traces by using /sys/kernel/debug/tracing/trace instead of dmesg]
Oct 28 14:29:56 mchn012c kernel: qemu-sys-7200    0...1. 676996395us :
kvm_msr: msr_read c0000080 = 0x500
Oct 28 14:29:56 mchn012c kernel: qemu-sys-7200    0...1. 676996403us :
kvm_msr: msr_write c0000080 = 0xd01
Quoted 6 lines
Quoted 4 lines
So Windows is setting EFER.SCE and EFER.NX while in long mode - perfectly reasonable. Can you rerun with the attached debug patch?
Log attached.
So the last bits are:
Oct 28 21:26:41 mchn012c kernel: update_transition_efer: efer_offset 4
efer d01
Oct 28 21:26:41 mchn012c kernel: update_transition_efer: ignoring all bits
Oct 28 21:26:41 mchn012c kernel: setup_msrs: marking efer for no reload

Quoted 8 lines

We're not reloading efer (correctly, as guest efer == host efer), yet vmx_save_host_state() fails while loading efer. I've looked at move_msr_up() (which is used by setup_msrs() to partition the msr space into reloaded and non-reloaded msrs), and it seems correct.
Can you see any way where update_transition_efer() returns false, yet efer turns up in the first save_nmsrs entries of vmx->guest_msrs?

Without understanding the code completely yet: When you push the slot containing EFER around, do you also update msr_offset_efer?

Jan

Jan Kiszka

2009-10-29 08:06 UTC #12 lore raw

Quoted 28 lines · Jan Kiszka

Quoted 27 lines · Avi Kivity
Quoted 8 lines · Jan Kiszka
Quoted 8 lines
[you can get longer, more detailed traces by using /sys/kernel/debug/tracing/trace instead of dmesg]
Oct 28 14:29:56 mchn012c kernel: qemu-sys-7200    0...1. 676996395us :
kvm_msr: msr_read c0000080 = 0x500
Oct 28 14:29:56 mchn012c kernel: qemu-sys-7200    0...1. 676996403us :
kvm_msr: msr_write c0000080 = 0xd01
Quoted 6 lines
Quoted 4 lines
So Windows is setting EFER.SCE and EFER.NX while in long mode - perfectly reasonable. Can you rerun with the attached debug patch?
Log attached.
So the last bits are:
Oct 28 21:26:41 mchn012c kernel: update_transition_efer: efer_offset 4
efer d01
Oct 28 21:26:41 mchn012c kernel: update_transition_efer: ignoring all bits
Oct 28 21:26:41 mchn012c kernel: setup_msrs: marking efer for no reload
We're not reloading efer (correctly, as guest efer == host efer), yet vmx_save_host_state() fails while loading efer. I've looked at move_msr_up() (which is used by setup_msrs() to partition the msr space into reloaded and non-reloaded msrs), and it seems correct.

Quoted 6 lines

Quoted 3 lines
Can you see any way where update_transition_efer() returns false, yet efer turns up in the first save_nmsrs entries of vmx->guest_msrs?
Without understanding the code completely yet: When you push the slot containing EFER around, do you also update msr_offset_efer?

arch/x86/kvm/vmx.c +3 -1

index 4264e09..0b1f461 100644
@@ -928,8 +928,10 @@ static void setup_msrs(struct vcpu_vmx *vmx)
 	}
 #endif
 	vmx->msr_offset_efer = index = __find_msr_index(vmx, MSR_EFER);
-	if (index >= 0 && update_transition_efer(vmx))
+	if (index >= 0 && update_transition_efer(vmx)) {
+		vmx->msr_offset_efer = save_nmsrs;
 		move_msr_up(vmx, index, save_nmsrs++);
+	}
 
 	vmx->save_nmsrs = save_nmsrs;

Untested as I don't want to crash my notebook ATM. :)

Jan

Avi Kivity

2009-10-29 08:07 UTC #13 lore raw

In reply to Jan Kiszka

Quoted 33 lines · Jan Kiszka

Quoted 31 lines · Avi Kivity
Quoted 9 lines · Jan Kiszka
Quoted 9 lines
[you can get longer, more detailed traces by using /sys/kernel/debug/tracing/trace instead of dmesg]
Oct 28 14:29:56 mchn012c kernel: qemu-sys-7200    0...1. 676996395us :
kvm_msr: msr_read c0000080 = 0x500
Oct 28 14:29:56 mchn012c kernel: qemu-sys-7200    0...1. 676996403us :
kvm_msr: msr_write c0000080 = 0xd01
Quoted 8 lines
Quoted 5 lines
So Windows is setting EFER.SCE and EFER.NX while in long mode - perfectly reasonable. Can you rerun with the attached debug patch?
Log attached.
So the last bits are:
Oct 28 21:26:41 mchn012c kernel: update_transition_efer: efer_offset 4
efer d01
Oct 28 21:26:41 mchn012c kernel: update_transition_efer: ignoring all bits
Oct 28 21:26:41 mchn012c kernel: setup_msrs: marking efer for no reload
We're not reloading efer (correctly, as guest efer == host efer), yet vmx_save_host_state() fails while loading efer. I've looked at move_msr_up() (which is used by setup_msrs() to partition the msr space into reloaded and non-reloaded msrs), and it seems correct.

Quoted 8 lines

Quoted 4 lines
Can you see any way where update_transition_efer() returns false, yet efer turns up in the first save_nmsrs entries of vmx->guest_msrs?
Without understanding the code completely yet: When you push the slot containing EFER around, do you also update msr_offset_efer?

We don't, but msr_offset_efer is only used from update_transition_efer(), which is only ever called from setup_msrs() immediately after updating msr_offset_efer.

Of course, it should be an argument to update_transition_efer(), I'll clean up this leftover.

Signature · 1 line

Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Jan Kiszka

2009-10-29 08:32 UTC #14 lore raw

Quoted 39 lines · Avi Kivity

Quoted 38 lines · Jan Kiszka
Quoted 36 lines · Avi Kivity
Quoted 9 lines · Jan Kiszka
Quoted 9 lines
[you can get longer, more detailed traces by using /sys/kernel/debug/tracing/trace instead of dmesg]
Oct 28 14:29:56 mchn012c kernel: qemu-sys-7200    0...1. 676996395us :
kvm_msr: msr_read c0000080 = 0x500
Oct 28 14:29:56 mchn012c kernel: qemu-sys-7200    0...1. 676996403us :
kvm_msr: msr_write c0000080 = 0xd01
Quoted 8 lines
Quoted 5 lines
So Windows is setting EFER.SCE and EFER.NX while in long mode - perfectly reasonable. Can you rerun with the attached debug patch?
Log attached.
So the last bits are:
Oct 28 21:26:41 mchn012c kernel: update_transition_efer: efer_offset 4
efer d01
Oct 28 21:26:41 mchn012c kernel: update_transition_efer: ignoring all
bits
Oct 28 21:26:41 mchn012c kernel: setup_msrs: marking efer for no reload
We're not reloading efer (correctly, as guest efer == host efer), yet vmx_save_host_state() fails while loading efer. I've looked at move_msr_up() (which is used by setup_msrs() to partition the msr space into reloaded and non-reloaded msrs), and it seems correct.
Can you see any way where update_transition_efer() returns false, yet efer turns up in the first save_nmsrs entries of vmx->guest_msrs?

Quoted 8 lines

Quoted 4 lines
Without understanding the code completely yet: When you push the slot containing EFER around, do you also update msr_offset_efer?
We don't, but msr_offset_efer is only used from update_transition_efer(), which is only ever called from setup_msrs() immediately after updating msr_offset_efer.

Indeed.

Quoted 4 lines

Of course, it should be an argument to update_transition_efer(), I'll clean up this leftover.

OK, will see that I can debug this later today.

Jan

Jan Kiszka

author

2009-10-29 15:45 UTC #15 lore raw

Quoted 48 lines · Jan Kiszka

Quoted 39 lines · Avi Kivity
Quoted 38 lines · Jan Kiszka
Quoted 36 lines · Avi Kivity
Quoted 9 lines · Jan Kiszka
Quoted 9 lines
[you can get longer, more detailed traces by using /sys/kernel/debug/tracing/trace instead of dmesg]
Oct 28 14:29:56 mchn012c kernel: qemu-sys-7200    0...1. 676996395us :
kvm_msr: msr_read c0000080 = 0x500
Oct 28 14:29:56 mchn012c kernel: qemu-sys-7200    0...1. 676996403us :
kvm_msr: msr_write c0000080 = 0xd01
Quoted 8 lines
Quoted 5 lines
So Windows is setting EFER.SCE and EFER.NX while in long mode - perfectly reasonable. Can you rerun with the attached debug patch?
Log attached.
So the last bits are:
Oct 28 21:26:41 mchn012c kernel: update_transition_efer: efer_offset 4
efer d01
Oct 28 21:26:41 mchn012c kernel: update_transition_efer: ignoring all
bits
Oct 28 21:26:41 mchn012c kernel: setup_msrs: marking efer for no reload
We're not reloading efer (correctly, as guest efer == host efer), yet vmx_save_host_state() fails while loading efer. I've looked at move_msr_up() (which is used by setup_msrs() to partition the msr space into reloaded and non-reloaded msrs), and it seems correct.
Can you see any way where update_transition_efer() returns false, yet efer turns up in the first save_nmsrs entries of vmx->guest_msrs?
Quoted 7 lines
Quoted 4 lines
Without understanding the code completely yet: When you push the slot containing EFER around, do you also update msr_offset_efer?
We don't, but msr_offset_efer is only used from update_transition_efer(), which is only ever called from setup_msrs() immediately after updating msr_offset_efer.

Quoted 8 lines

Indeed.
Quoted 3 lines
Of course, it should be an argument to update_transition_efer(), I'll clean up this leftover.
OK, will see that I can debug this later today.

Haven't found the actual problem yet, but some oddities:

Quoted 16 lines

static int vmx_vcpu_setup(struct vcpu_vmx *vmx)
{
...

	for (i = 0; i < NR_VMX_MSR; ++i) {
		u32 index = vmx_msr_index[i];
		u32 data_low, data_high;
		u64 data;
		int j = vmx->nmsrs;

		if (rdmsr_safe(index, &data_low, &data_high) < 0)
			continue;
		if (wrmsr_safe(index, data_low, data_high) < 0)
			continue;
		data = data_low | ((u64)data_high << 32);
		vmx->guest_msrs[j].index = i;
		vmx->guest_msrs[j].data = 0;

                                        ^^^^^

Local 'data' drops on the floor. Is that correct (then it deserves a cleanup)? Previous version did a "guest = host".

Quoted 10 lines

static void vmx_set_efer(struct kvm_vcpu *vcpu, u64 efer)

{
	struct vcpu_vmx *vmx = to_vmx(vcpu);
	struct shared_msr_entry *msr = find_msr_entry(vmx, MSR_EFER);

	if (!msr)
		return;
	vcpu->arch.shadow_efer = efer;
	if (!msr)
		return;

One "if (!msr)" too much - really the second one?

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Avi Kivity

2009-10-29 16:05 UTC #16 lore raw

Quoted 18 lines · Jan Kiszka

Quoted 4 lines

static int vmx_vcpu_setup(struct vcpu_vmx *vmx)
{
...

Quoted 14 lines

	for (i = 0; i<  NR_VMX_MSR; ++i) {
		u32 index = vmx_msr_index[i];
		u32 data_low, data_high;
		u64 data;
		int j = vmx->nmsrs;

		if (rdmsr_safe(index,&data_low,&data_high)<  0)
			continue;
		if (wrmsr_safe(index, data_low, data_high)<  0)
			continue;
		data = data_low | ((u64)data_high<<  32);
		vmx->guest_msrs[j].index = i;
		vmx->guest_msrs[j].data = 0;

Quoted 4 lines

                                         ^^^^^
Local 'data' drops on the floor. Is that correct (then it deserves a cleanup)? Previous version did a "guest = host".

Arguably, it's more correct than what we used to have. This code dates back to the day when kvm started in 64-bit mode and so we copied important MSRs from the host.

Shouldn't matter for our case.

Quoted 5 lines

Quoted 5 lines
static void vmx_set_efer(struct kvm_vcpu *vcpu, u64 efer)
{
	struct vcpu_vmx *vmx = to_vmx(vcpu);
	struct shared_msr_entry *msr = find_msr_entry(vmx, MSR_EFER);

Quoted 8 lines

Quoted 6 lines
	if (!msr)
		return;
	vcpu->arch.shadow_efer = efer;
	if (!msr)
		return;
One "if (!msr)" too much - really the second one?

Yes. (Either really - if there is no host EFER, the only legal value for efer is 0, so whether we update it or not doesn't matter).

Likely introduced by a bad merge. How code rots.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2009-10-29 16:07 UTC #17 lore raw

In reply to Avi Kivity

Quoted 24 lines · Avi Kivity

Quoted 9 lines · Jan Kiszka
Quoted 9 lines
[you can get longer, more detailed traces by using /sys/kernel/debug/tracing/trace instead of dmesg]
Oct 28 14:29:56 mchn012c kernel: qemu-sys-7200    0...1. 676996395us :
kvm_msr: msr_read c0000080 = 0x500
Oct 28 14:29:56 mchn012c kernel: qemu-sys-7200    0...1. 676996403us :
kvm_msr: msr_write c0000080 = 0xd01
Quoted 6 lines
Quoted 4 lines
So Windows is setting EFER.SCE and EFER.NX while in long mode - perfectly reasonable. Can you rerun with the attached debug patch?
Log attached.
So the last bits are:
Oct 28 21:26:41 mchn012c kernel: update_transition_efer: efer_offset 4
efer d01
Oct 28 21:26:41 mchn012c kernel: update_transition_efer: ignoring all bits
Oct 28 21:26:41 mchn012c kernel: setup_msrs: marking efer for no reload

Quoted 8 lines

We're not reloading efer (correctly, as guest efer == host efer), yet vmx_save_host_state() fails while loading efer. I've looked at move_msr_up() (which is used by setup_msrs() to partition the msr space into reloaded and non-reloaded msrs), and it seems correct.
Can you see any way where update_transition_efer() returns false, yet efer turns up in the first save_nmsrs entries of vmx->guest_msrs?

Question: When a VCPU migrates, what syncs the shared_msrs per-cpu vars before or after that, or why is this no problem?

I'm currently following the theory that guest_msrs contains some non-EFER entry with 0 value, but shared_msrs has a different index in the slot passed to kvm_set_shared_msr.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Jan Kiszka

author

2009-10-29 16:14 UTC #18 lore raw

Quoted 32 lines · Jan Kiszka

Quoted 22 lines · Avi Kivity
Quoted 8 lines · Jan Kiszka
Quoted 8 lines
[you can get longer, more detailed traces by using /sys/kernel/debug/tracing/trace instead of dmesg]
Oct 28 14:29:56 mchn012c kernel: qemu-sys-7200    0...1. 676996395us :
kvm_msr: msr_read c0000080 = 0x500
Oct 28 14:29:56 mchn012c kernel: qemu-sys-7200    0...1. 676996403us :
kvm_msr: msr_write c0000080 = 0xd01
Quoted 6 lines
Quoted 4 lines
So Windows is setting EFER.SCE and EFER.NX while in long mode - perfectly reasonable. Can you rerun with the attached debug patch?
Log attached.
So the last bits are:
Oct 28 21:26:41 mchn012c kernel: update_transition_efer: efer_offset 4
efer d01
Oct 28 21:26:41 mchn012c kernel: update_transition_efer: ignoring all bits
Oct 28 21:26:41 mchn012c kernel: setup_msrs: marking efer for no reload
Quoted 8 lines
We're not reloading efer (correctly, as guest efer == host efer), yet vmx_save_host_state() fails while loading efer. I've looked at move_msr_up() (which is used by setup_msrs() to partition the msr space into reloaded and non-reloaded msrs), and it seems correct.
Can you see any way where update_transition_efer() returns false, yet efer turns up in the first save_nmsrs entries of vmx->guest_msrs?

Quoted 7 lines

Question: When a VCPU migrates, what syncs the shared_msrs per-cpu vars before or after that, or why is this no problem?
I'm currently following the theory that guest_msrs contains some non-EFER entry with 0 value, but shared_msrs has a different index in the slot passed to kvm_set_shared_msr.

OK, EFER is a globally shared msr. But there still needs to be a consensus on the slot id used for guest_msrs and shared_msrs_global.msrs, right? move_msr_up works per-vcpu and is obviously decoupled...

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Avi Kivity

2009-10-29 16:49 UTC #20 lore raw

In reply to Jan Kiszka

Quoted 5 lines · Jan Kiszka

Question: When a VCPU migrates, what syncs the shared_msrs per-cpu vars before or after that, or why is this no problem?

The guest's msrs remain on the old cpu (until a new guest is switched in or we return to userspace). The guest msrs are loaded into the new cpu when vmx_save_host_state() is called as part or kvm_arch_vcpu_load().

Quoted 4 lines

I'm currently following the theory that guest_msrs contains some non-EFER entry with 0 value, but shared_msrs has a different index in the slot passed to kvm_set_shared_msr.

That's a global...

Signature · 1 line

Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Avi Kivity

2009-10-29 16:52 UTC #19 lore raw

In reply to Jan Kiszka

Quoted 7 lines · Jan Kiszka

OK, EFER is a globally shared msr. But there still needs to be a consensus on the slot id used for guest_msrs and shared_msrs_global.msrs, right? move_msr_up works per-vcpu and is obviously decoupled...

move_msr_up() moves a shared_msr_entry, which contains an index into the shared_msrs_global structure. Double indirection:

msr_index = kvm_shared_msrs_global.msrs[vmx->guest_msrs[index].index].msr

So guest_msrs can be rearranged at will. Except for your oops.

Signature · 1 line

Do not meddle in the internals of kernels, for they are subtle and quick to panic.