From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 1/1] x2apic interface to lapic Date: Sun, 31 May 2009 15:44:39 +0300 Message-ID: <4A227BB7.8080003@redhat.com> References: <1242927475-6140-1-git-send-email-gleb@redhat.com> <1242927475-6140-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 mx2.redhat.com ([66.187.237.31]:48130 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752120AbZEaMoj (ORCPT ); Sun, 31 May 2009 08:44:39 -0400 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n4VCifLb029464 for ; Sun, 31 May 2009 08:44:41 -0400 In-Reply-To: <1242927475-6140-2-git-send-email-gleb@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Gleb Natapov wrote: > static unsigned int apic_lvt_mask[APIC_LVT_NUM] = { > LVT_MASK | APIC_LVT_TIMER_PERIODIC, /* LVTT */ > LVT_MASK | APIC_MODE_MASK, /* LVTTHMR */ > @@ -251,7 +257,12 @@ int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u16 dest) > int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda) > { > int result = 0; > - u8 logical_id; > + u32 logical_id; > + > + if (apic_x2apic_mode(apic)) { > + logical_id = apic_get_reg(apic, APIC_LDR); > + return !!(logical_id & (uint16_t)mda & 0xffff); > + } > !! unnecessary. And one of the (cast) and (& 0xffff) is unnecessary. > > logical_id = GET_APIC_LOGICAL_ID(apic_get_reg(apic, APIC_LDR)); > > @@ -440,7 +451,8 @@ static void apic_send_ipi(struct kvm_lapic *apic) > irq.level = icr_low & APIC_INT_ASSERT; > irq.trig_mode = icr_low & APIC_INT_LEVELTRIG; > irq.shorthand = icr_low & APIC_SHORT_MASK; > - irq.dest_id = GET_APIC_DEST_FIELD(icr_high); > + irq.dest_id = apic_x2apic_mode(apic) ? icr_high : > + GET_APIC_DEST_FIELD(icr_high); > Please replace the ?: (here and elsewhere) with explicit if statements. ?: is unreadable when split over two lines like this. (I find it more readable to do blah = predicate ? iftrue : iffalse; but that's almost the same as an if) > > -static void apic_mmio_read(struct kvm_io_device *this, > - gpa_t address, int len, void *data) > +static int apic_reg_read(struct kvm_lapic *apic, u32 offset, int len, > + void *data) > { > - struct kvm_lapic *apic = (struct kvm_lapic *)this->private; > - unsigned int offset = address - apic->base_address; > unsigned char alignment = offset & 0xf; > u32 result; > + static const uint64_t rmask = 0x43ff01ffffffe70c; > Wow. A comment perhaps? > > if ((alignment + len) > 4) { > - printk(KERN_ERR "KVM_APIC_READ: alignment error %lx %d", > - (unsigned long)address, len); > - return; > + printk(KERN_ERR "KVM_APIC_READ: alignment error %x %d\n", > + offset, len); > + return 1; > } > + > + if (!(rmask & (1ULL << (offset >> 4)))) { > + printk(KERN_ERR "KVM_APIC_READ: read reserved register %x\n", > + offset); > + return 1; > + } > + > (offset >> 4) can still be 255, yielding unspecified results for the shift. > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 98c2434..d3df59a 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -800,6 +800,8 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data) > case MSR_IA32_APICBASE: > kvm_set_apic_base(vcpu, data); > break; > + case 0x800 ... 0xbff: > + return kvm_x2apic_msr_write(vcpu, msr, data); > case MSR_IA32_MISC_ENABLE: > vcpu->arch.ia32_misc_enable_msr = data; > break; > @@ -958,6 +960,9 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata) > case MSR_IA32_APICBASE: > data = kvm_get_apic_base(vcpu); > break; > + case 0x800 ... 0xbff: > + return kvm_x2apic_msr_read(vcpu, msr, pdata); > + break; > case MSR_IA32_MISC_ENABLE: > data = vcpu->arch.ia32_misc_enable_msr; > break; > Whitespace... -- error compiling committee.c: too many arguments to function