Re: [PATCH 1/1] x2apic interface to lapic

From: Avi Kivity <avi@redhat.com>
To: Gleb Natapov <gleb@redhat.com>
Cc: kvm@vger.kernel.org
Subject: Re: [PATCH 1/1] x2apic interface to lapic
Date: Sun, 31 May 2009 15:44:39 +0300	[thread overview]
Message-ID: <4A227BB7.8080003@redhat.com> (raw)
In-Reply-To: <1242927475-6140-2-git-send-email-gleb@redhat.com>

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