Re: BUG with Win7 and user-return-notifier

[Jan Kiszka <jan.kiszka@siemens.com>]

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