BUG with Win7 and user-return-notifier

BUG with Win7 and user-return-notifier

[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:

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

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

Re: BUG with Win7 and user-return-notifier

[Avi Kivity]

On 10/27/2009 02:52 PM, Jan Kiszka wrote:
> 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?

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

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

Re: BUG with Win7 and user-return-notifier

[Jan Kiszka]

Avi Kivity wrote:
> On 10/27/2009 02:52 PM, Jan Kiszka wrote:
>> 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.

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

Jan

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

Re: BUG with Win7 and user-return-notifier

[Avi Kivity]

On 10/27/2009 03:13 PM, Jan Kiszka wrote:
> Avi Kivity wrote:
>    
>> On 10/27/2009 02:52 PM, Jan Kiszka wrote:
>>      
>>> 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.
>    

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

    CONFIG_USER_RETURN_NOTIFIER=y

in your .config?

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

Re: BUG with Win7 and user-return-notifier

[Avi Kivity]

On 10/27/2009 03:24 PM, Avi Kivity wrote:
>
> 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.

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

Re: BUG with Win7 and user-return-notifier

[Avi Kivity]

On 10/27/2009 03:25 PM, Avi Kivity wrote:
> On 10/27/2009 03:24 PM, Avi Kivity wrote:
>>
>> 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.

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

Re: BUG with Win7 and user-return-notifier

[Jan Kiszka]

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

Avi Kivity wrote:
> On 10/27/2009 03:25 PM, Avi Kivity wrote:
>> On 10/27/2009 03:24 PM, Avi Kivity wrote:
>>> 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.
> 

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

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

[-- Attachment #2: msr-crash.bz2 --]
[-- Type: application/x-bzip, Size: 6224 bytes --]

Re: BUG with Win7 and user-return-notifier

[Avi Kivity]

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

On 10/28/2009 04:01 PM, Jan Kiszka wrote:
> Avi Kivity wrote:
>    
>> On 10/27/2009 03:25 PM, Avi Kivity wrote:
>>      
>>> On 10/27/2009 03:24 PM, Avi Kivity wrote:
>>>        
>>>> 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.
>>
>>      
> 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?

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


[-- Attachment #2: efer-debug.patch --]
[-- Type: text/x-patch, Size: 1346 bytes --]

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 85f97d1..6bd6d2c 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -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;
 

Re: BUG with Win7 and user-return-notifier

[Jan Kiszka]

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

Avi Kivity wrote:
> On 10/28/2009 04:01 PM, Jan Kiszka wrote:
>> Avi Kivity wrote:
>>   
>>> On 10/27/2009 03:25 PM, Avi Kivity wrote:
>>>     
>>>> On 10/27/2009 03:24 PM, Avi Kivity wrote:
>>>>       
>>>>> 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.
>>>
>>>      
>> 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?
> 

Currently building, expect results soon.

But while we are at it:

> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 85f97d1..6bd6d2c 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c

...

> @@ -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++);

The last line breaks x86-32 builds.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: BUG with Win7 and user-return-notifier

[Avi Kivity]

On 10/28/2009 10:40 PM, Jan Kiszka wrote:
>
>> [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?
>>
>>      
> 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?

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

Re: BUG with Win7 and user-return-notifier

[Jan Kiszka]

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

Avi Kivity wrote:
> On 10/28/2009 10:40 PM, Jan Kiszka wrote:
>>
>>> [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?
>>>
>>>      
>> 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?
> 

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

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: BUG with Win7 and user-return-notifier

[Jan Kiszka]

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

Jan Kiszka wrote:
> Avi Kivity wrote:
>> On 10/28/2009 10:40 PM, Jan Kiszka wrote:
>>>> [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?
>>>>
>>>>      
>>> 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?
>>
> 
> Without understanding the code completely yet: When you push the slot
> containing EFER around, do you also update msr_offset_efer?

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 4264e09..0b1f461 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: BUG with Win7 and user-return-notifier

[Avi Kivity]

On 10/29/2009 10:03 AM, Jan Kiszka wrote:
> Avi Kivity wrote:
>    
>> On 10/28/2009 10:40 PM, Jan Kiszka wrote:
>>      
>>>        
>>>> [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?
>>>>
>>>>
>>>>          
>>> 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?
>>
>>      
> 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.

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

Re: BUG with Win7 and user-return-notifier

[Jan Kiszka]

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

Avi Kivity wrote:
> On 10/29/2009 10:03 AM, Jan Kiszka wrote:
>> Avi Kivity wrote:
>>   
>>> On 10/28/2009 10:40 PM, Jan Kiszka wrote:
>>>     
>>>>       
>>>>> [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?
>>>>>
>>>>>
>>>>>          
>>>> 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?
>>>
>>>      
>> 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.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: BUG with Win7 and user-return-notifier

[Jan Kiszka]

Jan Kiszka wrote:
> Avi Kivity wrote:
>> On 10/29/2009 10:03 AM, Jan Kiszka wrote:
>>> Avi Kivity wrote:
>>>   
>>>> On 10/28/2009 10:40 PM, Jan Kiszka wrote:
>>>>     
>>>>>       
>>>>>> [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?
>>>>>>
>>>>>>
>>>>>>          
>>>>> 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?
>>>>
>>>>      
>>> 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.
> 
>> 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:

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

> 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

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

Re: BUG with Win7 and user-return-notifier

[Avi Kivity]

On 10/29/2009 05:45 PM, Jan Kiszka wrote:
>
>> 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".
>    

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.

>> 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?
>    

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.

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

Re: BUG with Win7 and user-return-notifier

[Jan Kiszka]

Avi Kivity wrote:
> On 10/28/2009 10:40 PM, Jan Kiszka wrote:
>>
>>> [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?
>>>
>>>      
>> 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?
> 

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

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

Re: BUG with Win7 and user-return-notifier

[Jan Kiszka]

Jan Kiszka wrote:
> Avi Kivity wrote:
>> On 10/28/2009 10:40 PM, Jan Kiszka wrote:
>>>> [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?
>>>>
>>>>      
>>> 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?
>>
> 
> 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

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

Re: BUG with Win7 and user-return-notifier

[Avi Kivity]

On 10/29/2009 06:14 PM, Jan Kiszka wrote:
>
> 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.

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

Re: BUG with Win7 and user-return-notifier

[Avi Kivity]

On 10/29/2009 06:07 PM, Jan Kiszka wrote:
>
> 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().

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

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