From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH 8/8] KVM: x86: simplify kvm_apic_map Date: Fri, 30 Jan 2015 10:18:20 +0100 Message-ID: <54CB4C5C.6090706@redhat.com> References: <1422568135-28402-1-git-send-email-rkrcmar@redhat.com> <1422568135-28402-9-git-send-email-rkrcmar@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: kvm@vger.kernel.org, Nadav Amit , Gleb Natapov To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , linux-kernel@vger.kernel.org Return-path: In-Reply-To: <1422568135-28402-9-git-send-email-rkrcmar@redhat.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On 29/01/2015 22:48, Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: > recalculate_apic_map() uses two passes over all VCPUs. This is a rel= ic > from time when we selected a global mode in the first pass and set up > the optimized table in the second pass (to have a consistent mode). >=20 > Recent changes made mixed mode unoptimized and we can do it in one pa= ss. > Format of logical MDA is a function of the mode, so we encode it in > apic_logical_id() and drop obsoleted variables from the struct. >=20 > Signed-off-by: Radim Kr=C4=8Dm=C3=A1=C5=99 > --- > arch/x86/include/asm/kvm_host.h | 3 -- > arch/x86/kvm/lapic.c | 78 ++++++++++---------------------= ---------- > arch/x86/kvm/lapic.h | 29 ++++++++------- > 3 files changed, 36 insertions(+), 74 deletions(-) >=20 > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/k= vm_host.h > index fec3188cabbb..14b6b0fd17b1 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -555,9 +555,6 @@ struct kvm_arch_memory_slot { > struct kvm_apic_map { > struct rcu_head rcu; > u8 mode; > - u8 ldr_bits; > - /* fields bellow are used to decode ldr values in different modes *= / > - u32 cid_shift, cid_mask, lid_mask; > struct kvm_lapic *phys_map[256]; > /* first index is cluster id second is cpu id in a cluster */ > struct kvm_lapic *logical_map[16][16]; > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 621d9df6ac63..f74557791a77 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -146,49 +146,6 @@ static void recalculate_apic_map(struct kvm *kvm= ) > if (!new) > goto out; > =20 > - new->ldr_bits =3D 8; > - /* flat mode is default */ > - new->cid_shift =3D 8; > - new->cid_mask =3D 0; > - new->lid_mask =3D 0xff; > - > - kvm_for_each_vcpu(i, vcpu, kvm) { > - struct kvm_lapic *apic =3D vcpu->arch.apic; > - > - if (!kvm_apic_present(vcpu)) > - continue; > - > - if (apic_x2apic_mode(apic)) { > - new->ldr_bits =3D 32; > - new->cid_shift =3D 16; > - new->cid_mask =3D new->lid_mask =3D 0xffff; > - new->mode |=3D KVM_APIC_MODE_X2APIC; > - } else if (kvm_apic_get_reg(apic, APIC_LDR)) { > - if (kvm_apic_get_reg(apic, APIC_DFR) =3D=3D > - APIC_DFR_CLUSTER) { > - new->cid_shift =3D 4; > - new->cid_mask =3D 0xf; > - new->lid_mask =3D 0xf; > - new->mode |=3D KVM_APIC_MODE_XAPIC_CLUSTER; > - } else { > - new->cid_shift =3D 8; > - new->cid_mask =3D 0; > - new->lid_mask =3D 0xff; > - new->mode |=3D KVM_APIC_MODE_XAPIC_FLAT; > - } > - } > - > - /* > - * All APICs have to be configured in the same mode by an OS. > - * We take advatage of this while building logical id loockup > - * table. After reset APICs are in software disabled mode, so if > - * we find apic with different setting we assume this is the mode > - * OS wants all apics to be in; build lookup table accordingly. > - */ > - if (kvm_apic_sw_enabled(apic)) > - break; > - } > - > kvm_for_each_vcpu(i, vcpu, kvm) { > struct kvm_lapic *apic =3D vcpu->arch.apic; > u16 cid, lid; > @@ -198,17 +155,22 @@ static void recalculate_apic_map(struct kvm *kv= m) > continue; > =20 > aid =3D kvm_apic_id(apic); > - ldr =3D kvm_apic_get_reg(apic, APIC_LDR); > - cid =3D apic_cluster_id(new, ldr); > - lid =3D apic_logical_id(new, ldr); > - > if (aid < ARRAY_SIZE(new->phys_map)) > new->phys_map[aid] =3D apic; > =20 > - /* The logical map is definitely wrong if we have multiple > - * modes at the same time. Physical is still right though. > - */ > - if (hweight8(new->mode) !=3D 1) > + ldr =3D kvm_apic_get_reg(apic, APIC_LDR); > + > + if (apic_x2apic_mode(apic)) { > + new->mode |=3D KVM_APIC_MODE_X2APIC; > + } else if (ldr) { > + ldr =3D GET_APIC_LOGICAL_ID(ldr); > + if (kvm_apic_get_reg(apic, APIC_DFR) =3D=3D APIC_DFR_FLAT) > + new->mode |=3D KVM_APIC_MODE_XAPIC_FLAT; > + else > + new->mode |=3D KVM_APIC_MODE_XAPIC_CLUSTER; > + } > + > + if (!apic_logical_id(new, ldr, &cid, &lid)) > continue; > =20 > if (lid && cid < ARRAY_SIZE(new->logical_map)) > @@ -724,22 +686,20 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *= kvm, struct kvm_lapic *src, > =20 > dst =3D &map->phys_map[irq->dest_id]; > } else { > - u32 mda =3D irq->dest_id << (32 - map->ldr_bits); > - u16 cid =3D apic_cluster_id(map, mda); > + u16 cid; > =20 > - if (cid >=3D ARRAY_SIZE(map->logical_map)) > - goto out; > - > - if (hweight8(map->mode) !=3D 1) { > + if (!apic_logical_id(map, irq->dest_id, &cid, (u16 *)&bitmap)) > + { > /* Not deliverable with optimized map. */ > ret =3D false; > goto out; > } > =20 > + if (cid >=3D ARRAY_SIZE(map->logical_map)) > + goto out; > + > dst =3D map->logical_map[cid]; > =20 > - bitmap =3D apic_logical_id(map, mda); > - > if (irq->delivery_mode =3D=3D APIC_DM_LOWEST) { > int l =3D -1; > for_each_set_bit(i, &bitmap, 16) { > diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h > index fd0197a93862..320716e301a8 100644 > --- a/arch/x86/kvm/lapic.h > +++ b/arch/x86/kvm/lapic.h > @@ -151,19 +151,24 @@ static inline bool kvm_apic_vid_enabled(struct = kvm *kvm) > return kvm_x86_ops->vm_has_apicv(kvm); > } > =20 > -static inline u16 apic_cluster_id(struct kvm_apic_map *map, u32 ldr) > +static inline bool > +apic_logical_id(struct kvm_apic_map *map, u32 ldr, u16 *cid, u16 *li= d) > { > - u16 cid; > - ldr >>=3D 32 - map->ldr_bits; > - cid =3D (ldr >> map->cid_shift) & map->cid_mask; > - > - return cid; > -} > - > -static inline u16 apic_logical_id(struct kvm_apic_map *map, u32 ldr) > -{ > - ldr >>=3D (32 - map->ldr_bits); > - return ldr & map->lid_mask; > + 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; > + } We need some optimization here. You can make the defines equal to the size of the lid: CLUSTER =3D 1 << 3, FLAT =3D 1 << 2, X2APIC =3D 1 << 4= : BUILD_BUG_ON(...FLAT !=3D 4); BUILD_BUG_ON(...CLUSTER !=3D 8); BUILD_BUG_ON(...X2APIC !=3D 16); lid_bits =3D mode; cid_bits =3D mode & (16 | 4); lid_mask =3D (1 << lid_bits) - 1; cid_mask =3D (1 << cid_bits) - 1; *cid =3D (ldr >> lid_bits) & cid_mask; *lid =3D ldr & lid_mask; Please move this to lapic.c since you are at it. Paolo > + return false; > } > =20 > static inline bool kvm_apic_has_events(struct kvm_vcpu *vcpu) >=20