Re: [PATCHv2 2/4] Implement bare minimum of HYPER-V MSRs.

[Avi Kivity <avi@redhat.com>]

On 01/17/2010 02:44 PM, Gleb Natapov wrote:
> On Sun, Jan 17, 2010 at 02:10:45PM +0200, Avi Kivity wrote:
>    
>> On 01/17/2010 11:03 AM, Gleb Natapov wrote:
>>      
>>> Minimum HYPER-V implementation should have GUEST_OS_ID, HYPERCALL and
>>> VP_INDEX MSRs.
>>>
>>>
>>>   TRACE_EVENT(kvm_pio,
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 4d835b6..db0b2b1 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -630,7 +630,8 @@ static u32 msrs_to_save[] = {
>>>   #ifdef CONFIG_X86_64
>>>   	MSR_CSTAR, MSR_KERNEL_GS_BASE, MSR_SYSCALL_MASK, MSR_LSTAR,
>>>   #endif
>>> -	MSR_IA32_TSC, MSR_IA32_PERF_STATUS, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA
>>> +	MSR_IA32_TSC, MSR_IA32_PERF_STATUS, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
>>> +	HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,
>>>   };
>>>        
>> These will be disabled since the msrs don't exist on the host.  See
>> the comment above and KVM_SAVE_MSRS_BEGIN.
>>
>>      
> I see. Why not have two arrays?
>    

Clearly better.

>>> +	case HV_X64_MSR_HYPERCALL: {
>>> +		u64 gfn;
>>> +		unsigned long addr;
>>> +		/* if guest os id is not set hypercall should remain disabled */
>>> +		if (!kvm->arch.hv_guest_os_id&&   data)
>>> +			break;
>>> +		kvm->arch.hv_hypercall = data;
>>> +		if (!kvm_hv_hypercall_enabled(kvm))
>>> +			break;
>>> +		gfn = kvm->arch.hv_hypercall>>
>>> +			HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT;
>>> +		addr = gfn_to_hva(kvm, gfn);
>>> +		if (kvm_is_error_hva(addr))
>>> +			return 1;
>>>        
>> Should di the error check before assigning, perhaps.
>>
>>      
> Spec doesn't tell. And guest will get #GP and BSOD anyway.
>    

Well, all msrs I know of either #GP, or store the value and do what 
they're supposed to do, never both.


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