From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vadim Rozenfeld Subject: Re: [PATCHv2 3/4] Add HYPER-V apic access MSRs. Date: Sun, 17 Jan 2010 14:34:49 +0200 Message-ID: <1263731689.14712.14.camel@localhost> References: <1263719028-24765-1-git-send-email-gleb@redhat.com> <1263719028-24765-4-git-send-email-gleb@redhat.com> <4B530090.50506@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: Gleb Natapov , mtosatti@redhat.com, kvm@vger.kernel.org To: Avi Kivity Return-path: Received: from mx1.redhat.com ([209.132.183.28]:43516 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753339Ab0AQMfN (ORCPT ); Sun, 17 Jan 2010 07:35:13 -0500 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o0HCZCs1009684 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sun, 17 Jan 2010 07:35:12 -0500 In-Reply-To: <4B530090.50506@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Sun, 2010-01-17 at 14:20 +0200, Avi Kivity wrote: > On 01/17/2010 11:03 AM, Gleb Natapov wrote: > > > Signed-off-by: Gleb Natapov > > Signed-off-by: Vadim Rozenfeld > > > > Changelog entry. > > > > > struct kvm_mem_alias { > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > > index ba8c045..4b224f9 100644 > > --- a/arch/x86/kvm/lapic.c > > +++ b/arch/x86/kvm/lapic.c > > @@ -1246,3 +1246,34 @@ int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data) > > > > return 0; > > } > > + > > +int kvm_hv_vapic_msr_write(struct kvm_vcpu *vcpu, u32 reg, u64 data) > > +{ > > + struct kvm_lapic *apic = vcpu->arch.apic; > > + > > + 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 reg, u64 *data) > > +{ > > + struct kvm_lapic *apic = vcpu->arch.apic; > > + u32 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 prefer putting this in x86.c (maybe later split into hyperv.c). > > > diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h > > index 40010b0..d081cb6 100644 > > --- a/arch/x86/kvm/lapic.h > > +++ b/arch/x86/kvm/lapic.h > > @@ -48,4 +48,12 @@ void kvm_lapic_sync_to_vapic(struct kvm_vcpu *vcpu); > > > > int kvm_x2apic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data); > > int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data); > > + > > +int kvm_hv_vapic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data); > > +int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data); > > + > > +static inline bool kvm_hv_vapic_enabled(struct kvm_vcpu *vcpu) > > +{ > > + return !!(vcpu->arch.hv_vapic& HV_X64_MSR_APIC_ASSIST_PAGE_ENABLE); > > +} > > > > Are you sure that vapic enabled is equivalent to apic assist page enable? At least, when it is disable, the EOI interception mechanism won't work. > > !! not required. > > > #endif > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index db0b2b1..2fe555c 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -632,6 +632,7 @@ static u32 msrs_to_save[] = { > > #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_APIC_ASSIST_PAGE, > > }; > > > > > > Will be dropped by msr validation. > > > static unsigned num_msrs_to_save; > > @@ -1063,10 +1064,37 @@ 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 vaddr; > > + void *addr; > > + struct page *page; > > + vcpu->arch.hv_vapic = data; > > + if (!kvm_hv_vapic_enabled(vcpu)) > > + break; > > + vaddr = gfn_to_hva(vcpu->kvm, data>> > > + HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT); > > + if (kvm_is_error_hva(vaddr)) > > + return 1; > > + page = virt_to_page(vaddr); > > > > virt_to_page() takes a kernel address, not a user address. This is > get_user_pages(). But I think the whole thing is done better with > put_user(). > > > + addr = kmap_atomic(page, KM_USER0); > > + clear_user_page(addr, vaddr, page); > > + kunmap_atomic(addr, KM_USER0); > > > > Surprising that clear_user_page needs kmap_atomic() (but true). >