From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH 2/3] Add HYPER-V apic access MSRs. Date: Wed, 13 Jan 2010 16:40:59 +0200 Message-ID: <20100113144059.GC7549@redhat.com> References: <1263391197-9883-1-git-send-email-gleb@redhat.com> <1263391197-9883-3-git-send-email-gleb@redhat.com> <4B4DD850.7010808@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org To: Avi Kivity Return-path: Received: from mx1.redhat.com ([209.132.183.28]:5277 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755327Ab0AMOlC (ORCPT ); Wed, 13 Jan 2010 09:41:02 -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 o0DEf23L013914 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 13 Jan 2010 09:41:02 -0500 Content-Disposition: inline In-Reply-To: <4B4DD850.7010808@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, Jan 13, 2010 at 04:27:28PM +0200, Avi Kivity wrote: > 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. > I thought about it. It will bring apic internals into x86.c. If you are OK with that I'll do it like you say. > > #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. Forgot about checkpatch again :( > > >+ 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? Is says "in GPA space". Not sure how to interpret it. The guest I run uses address in real RAM. > > >+ memset((void*)addr, 0, PAGE_SIZE); > > clear_user_page()! > Ugh. > >+ 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 -- Gleb.