From mboxrd@z Thu Jan  1 00:00:00 1970
From: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
Subject: Re: [PATCH 8/8] KVM: x86: simplify kvm_apic_map
Date: Fri, 30 Jan 2015 16:14:42 +0100
Message-ID: <20150130151442.GC27414@potion.redhat.com>
References: <1422568135-28402-1-git-send-email-rkrcmar@redhat.com>
 <1422568135-28402-9-git-send-email-rkrcmar@redhat.com>
 <54CB4C5C.6090706@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: QUOTED-PRINTABLE
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
	Nadav Amit <namit@cs.technion.ac.il>,
	Gleb Natapov <gleb@kernel.org>
To: Paolo Bonzini <pbonzini@redhat.com>
Return-path: <linux-kernel-owner@vger.kernel.org>
Content-Disposition: inline
In-Reply-To: <54CB4C5C.6090706@redhat.com>
Sender: linux-kernel-owner@vger.kernel.org
List-Id: kvm.vger.kernel.org

2015-01-30 10:18+0100, Paolo Bonzini:
> On 29/01/2015 22:48, Radim Kr=C4=8Dm=C3=A1=C5=99 wrote:
> > +static inline bool
> > +apic_logical_id(struct kvm_apic_map *map, u32 ldr, u16 *cid, u16 *=
lid)
> >  {
> > +	switch (map->mode) {
> > +	case KVM_APIC_MODE_XAPIC_FLAT:
> > +		*cid =3D 0;
> > +		*lid =3D ldr & 0xff;
> > +		return true;
> > +	case KVM_APIC_MODE_XAPIC_CLUSTER:
> > +		*cid =3D (ldr >> 4) & 0xf;
> > +		*lid =3D ldr & 0xf;
> > +		return true;
> > +	case KVM_APIC_MODE_X2APIC:
> > +		*cid =3D ldr >> 16;
> > +		*lid =3D ldr & 0xffff;
> > +		return true;
> > +	}
>=20
> We need some optimization here.  You can make the defines equal to th=
e
> size of the lid: CLUSTER =3D 1 << 3, FLAT =3D 1 << 2, X2APIC =3D 1 <<=
 4:
>=20
> 	BUILD_BUG_ON(...FLAT !=3D 4);
> 	BUILD_BUG_ON(...CLUSTER !=3D 8);

(Swapped.)

> 	BUILD_BUG_ON(...X2APIC !=3D 16);

(Check the mode and return false here.)

> 	lid_bits =3D mode;
> 	cid_bits =3D mode & (16 | 4);
> 	lid_mask =3D (1 << lid_bits) - 1;
> 	cid_mask =3D (1 << cid_bits) - 1;
>=20
> 	*cid =3D (ldr >> lid_bits) & cid_mask;
> 	*lid =3D ldr & lid_mask;

Would jump predictor fail on the switch?  Or is size of the code that
important?  This code is shorter, but is going to execute far more
operations, so I think it would be slower ... (And harder to read.)

> Please move this to lapic.c since you are at it.

Ok.