From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 1/3] Implement bare minimum of HYPER-V MSRs. Date: Wed, 13 Jan 2010 16:21:33 +0200 Message-ID: <4B4DD6ED.8060101@redhat.com> References: <1263391197-9883-1-git-send-email-gleb@redhat.com> <1263391197-9883-2-git-send-email-gleb@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org To: Gleb Natapov Return-path: Received: from mx1.redhat.com ([209.132.183.28]:7610 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754842Ab0AMOVf (ORCPT ); Wed, 13 Jan 2010 09:21:35 -0500 Received: from int-mx05.intmail.prod.int.phx2.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.18]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o0DELYZm029808 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 13 Jan 2010 09:21:34 -0500 In-Reply-To: <1263391197-9883-2-git-send-email-gleb@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 01/13/2010 03:59 PM, Gleb Natapov wrote: > Minimum HYPER-V implementation should have GUEST_OS_ID, HYPERCALL and > VP_INDEX MSRs. > > > diff --git a/arch/x86/include/asm/kvm_hyperv.h b/arch/x86/include/asm/kvm_hyperv.h > new file mode 100644 > index 0000000..91211f3 > --- /dev/null > +++ b/arch/x86/include/asm/kvm_hyperv.h > Please name this asm/hyperv.h, so it can be used for Linux-as-Hyper-V-guest, not just Linux-as-host-impersonating-Hyper-V.+ Also put this in a separate patch. > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 6811e5e..6972b2b 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -628,7 +628,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 > End with trailing comma so future patches are nicer. > > +static bool kvm_hv_hypercall_enabled(struct kvm *kvm) > +{ > + return !!(kvm->arch.hv_hypercall& HV_X64_MSR_HYPERCALL_ENABLE); > +} > !! is unnecessary for bool: _Bool and(unsigned x) { return x & 16; } 0000000000000010 : 10: c1 ef 04 shr $0x4,%edi 13: 89 f8 mov %edi,%eax 15: 83 e0 01 and $0x1,%eax 18: c3 retq > + > +static int set_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data) > +{ > + struct kvm *kvm = vcpu->kvm; > + > + switch (msr) { > + case HV_X64_MSR_GUEST_OS_ID: > + kvm->arch.hv_guest_os_id = data; > + /* setting guest os id to zero disables hypercall page */ > + if (!kvm->arch.hv_guest_os_id) > + kvm->arch.hv_hypercall&= ~HV_X64_MSR_HYPERCALL_ENABLE; > + break; > + 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; > + kvm_x86_ops->patch_hypercall(vcpu, (unsigned char*)addr); > + ((unsigned char*)addr)[3] = 0xc3; /* ret */ > + break; > + } > + default: > + pr_unimpl(vcpu, "HYPER-V unimplemented wrmsr: 0x%x " > + "data 0x%llx\n", msr, data); > + return 1; > + } > + return 0; > +} > We need locking in case a malicious guest issues partition-wide msrs from multiple vcpus simultaneously. > int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data) > { > switch (msr) { > @@ -1117,6 +1181,16 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data) > pr_unimpl(vcpu, "unimplemented perfctr wrmsr: " > "0x%x data 0x%llx\n", msr, data); > break; > + case HV_X64_MSR_GUEST_OS_ID ... HV_X64_MSR_SINT15: > + if (kvm_hv_msr_partition_wide(msr)) { > + int r; > + mutex_lock(&vcpu->kvm->lock); > + r = set_msr_hyperv_pw(vcpu, msr, data); > + mutex_unlock(&vcpu->kvm->lock); > We do have locking. Any reason not to put it in set_msr_hyperv_pw? Seems cleaner. > +static int get_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata) > +{ > + u64 data = 0; > + struct kvm *kvm = vcpu->kvm; > + > + switch (msr) { > + case HV_X64_MSR_GUEST_OS_ID: > + data = kvm->arch.hv_guest_os_id; > + break; > + case HV_X64_MSR_HYPERCALL: > + data = kvm->arch.hv_hypercall; > + break; > This could be non-atomic on i386. I don't think it matters. > int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) > { > unsigned long nr, a0, a1, a2, a3, ret; > int r = 1; > > + if(kvm_hv_hypercall_enabled(vcpu->kvm)) > + return kvm_hv_hypercall(vcpu); > + > Space after if. -- error compiling committee.c: too many arguments to function