From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1762631AbbA3POv (ORCPT <rfc822;w@1wt.eu>);
	Fri, 30 Jan 2015 10:14:51 -0500
Received: from mx1.redhat.com ([209.132.183.28]:47790 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1750852AbbA3POt (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Fri, 30 Jan 2015 10:14:49 -0500
Date: Fri, 30 Jan 2015 16:14:42 +0100
From: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        Nadav Amit <namit@cs.technion.ac.il>, Gleb Natapov <gleb@kernel.org>
Subject: Re: [PATCH 8/8] KVM: x86: simplify kvm_apic_map
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-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <54CB4C5C.6090706@redhat.com>
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

2015-01-30 10:18+0100, Paolo Bonzini:
> On 29/01/2015 22:48, Radim Krčmář 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 = 0;
> > +		*lid = ldr & 0xff;
> > +		return true;
> > +	case KVM_APIC_MODE_XAPIC_CLUSTER:
> > +		*cid = (ldr >> 4) & 0xf;
> > +		*lid = ldr & 0xf;
> > +		return true;
> > +	case KVM_APIC_MODE_X2APIC:
> > +		*cid = ldr >> 16;
> > +		*lid = ldr & 0xffff;
> > +		return true;
> > +	}
> 
> We need some optimization here.  You can make the defines equal to the
> size of the lid: CLUSTER = 1 << 3, FLAT = 1 << 2, X2APIC = 1 << 4:
> 
> 	BUILD_BUG_ON(...FLAT != 4);
> 	BUILD_BUG_ON(...CLUSTER != 8);

(Swapped.)

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

(Check the mode and return false here.)

> 	lid_bits = mode;
> 	cid_bits = mode & (16 | 4);
> 	lid_mask = (1 << lid_bits) - 1;
> 	cid_mask = (1 << cid_bits) - 1;
> 
> 	*cid = (ldr >> lid_bits) & cid_mask;
> 	*lid = 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.