From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 2/3] Add HYPER-V apic access MSRs. Date: Wed, 13 Jan 2010 16:27:28 +0200 Message-ID: <4B4DD850.7010808@redhat.com> References: <1263391197-9883-1-git-send-email-gleb@redhat.com> <1263391197-9883-3-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]:38837 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754903Ab0AMO13 (ORCPT ); Wed, 13 Jan 2010 09:27:29 -0500 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o0DERT3X024698 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 13 Jan 2010 09:27:29 -0500 In-Reply-To: <1263391197-9883-3-git-send-email-gleb@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 01/13/2010 03:59 PM, Gleb Natapov wrote: > Signed-off-by: Gleb Natapov > Signed-off-by: Vadim Rozenfeld > --- > arch/x86/include/asm/kvm_host.h | 2 + > arch/x86/kvm/lapic.c | 53 +++++++++++++++++++++++++++++++++++++++ > arch/x86/kvm/lapic.h | 8 ++++++ > arch/x86/kvm/x86.c | 34 ++++++++++++++++++++++--- > include/linux/kvm.h | 1 + > 5 files changed, 94 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 7bfd468..4d82c52 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -362,6 +362,8 @@ struct kvm_vcpu_arch { > /* used for guest single stepping over the given code position */ > u16 singlestep_cs; > unsigned long singlestep_rip; > + /* fields used by HYPER-V emulation */ > + u64 hv_vapic; > }; > > struct kvm_mem_alias { > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index ba8c045..063963f 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -1246,3 +1246,56 @@ int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data) > > return 0; > } > + > +static u32 kvm_hv_vapic_msr2reg(u32 msr) > +{ > + u32 reg; > + switch (msr) { > + case HV_X64_MSR_EOI: > + reg = APIC_EOI; > + break; > + case HV_X64_MSR_ICR: > + reg = APIC_ICR; > + break; > + case HV_X64_MSR_TPR: > + reg = APIC_TASKPRI; > + break; > + default: > + reg = -1; > + break; > + } > + > + return reg; > +} > + > +int kvm_hv_vapic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data) > +{ > + struct kvm_lapic *apic = vcpu->arch.apic; > + u32 reg = kvm_hv_vapic_msr2reg(msr); > + > + if (!irqchip_in_kernel(vcpu->kvm)) > + return 1; > + > + /* if this is ICR write vector before command */ > + if (reg == APIC_ICR) > + apic_reg_write(apic, APIC_ICR2, (u32)(data>> 32)); > + return apic_reg_write(apic, reg, (u32)data); > +} > + > +int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data) > +{ > + struct kvm_lapic *apic = vcpu->arch.apic; > + u32 reg = kvm_hv_vapic_msr2reg(msr), low, high = 0; > + > + if (!irqchip_in_kernel(vcpu->kvm)) > + return 1; > + > + if (apic_reg_read(apic, reg, 4,&low)) > + return 1; > + if (reg == APIC_ICR) > + apic_reg_read(apic, APIC_ICR2, 4,&high); > + > + *data = (((u64)high)<< 32) | low; > + > + return 0; > +} > I think it's cleaner to put all this into the (set_msr and get_msr). You're just converting register numbers back and forth, while they are known at the call site. > #endif > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 6972b2b..e058898 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -629,7 +629,8 @@ static u32 msrs_to_save[] = { > 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, > - HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL > + HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL, > + HV_X64_MSR_APIC_ASSIST_PAGE > }; > , at the end. > > static unsigned num_msrs_to_save; > @@ -1061,10 +1062,30 @@ static int set_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data) > > static int set_msr_hyperv(struct kvm_vcpu *vcpu, u32 msr, u64 data) > { > - pr_unimpl(vcpu, "HYPER-V unimplemented wrmsr: 0x%x data 0x%llx\n", > - msr, data); > + switch (msr) { > + case HV_X64_MSR_APIC_ASSIST_PAGE: { > + unsigned long addr; > + vcpu->arch.hv_vapic = data; > + if(!kvm_hv_vapic_enabled(vcpu)) > + break; > Space after if. > + addr = gfn_to_hva(vcpu->kvm, data>> > + HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT); > + if (kvm_is_error_hva(addr)) > + return 1; > But the spec actually permits addresses outside RAM? > + memset((void*)addr, 0, PAGE_SIZE); > clear_user_page()! > + break; > + } > + case HV_X64_MSR_EOI: > + case HV_X64_MSR_ICR: > + case HV_X64_MSR_TPR: > + return kvm_hv_vapic_msr_write(vcpu, msr, data); > Here, just call the apic function with the known apic register number. -- error compiling committee.c: too many arguments to function