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

[Gleb Natapov <gleb@redhat.com>]

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?

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

> >+		kvm_x86_ops->patch_hypercall(vcpu, (unsigned char *)addr);
> >+		((unsigned char *)addr)[3] = 0xc3; /* ret */
> 
> kvm_write_guest(), this can fault.
> 
> >+int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
> >+{
> >+	u64 param, ingpa, outgpa, ret;
> >+	uint16_t code, rep_idx, rep_cnt, res = HV_STATUS_SUCCESS, rep_done = 0;
> >+	bool fast, longmode;
> >+	int cs_db, cs_l;
> >+
> >+	/*
> >+	 * hypercall generates UD from non zero cpl and real mode
> >+	 * per HYPER-V spec
> >+	 */
> >+	if (kvm_x86_ops->get_cpl(vcpu) != 0 || !(vcpu->arch.cr0&  X86_CR0_PE)) {
> 
> Use kvm_read_cr0_bits() to avoid caching.
> 
> 
> -- 
> error compiling committee.c: too many arguments to function

--
			Gleb.