From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PATCH 1/9] KVM: x86: add kvm_apic_map_get_dest_lapic Date: Wed, 25 May 2016 18:02:46 +0200 Message-ID: <20160525160245.GE14795@potion> References: <1462568045-31085-1-git-send-email-rkrcmar@redhat.com> <1462568045-31085-2-git-send-email-rkrcmar@redhat.com> <20160519063606.GA8574@pxdev.xzpeter.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: kvm@vger.kernel.org, Paolo Bonzini , "Lan, Tianyu" , Igor Mammedov , Jan Kiszka To: Peter Xu Return-path: Received: from mx1.redhat.com ([209.132.183.28]:52655 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751907AbcEYQCu (ORCPT ); Wed, 25 May 2016 12:02:50 -0400 Content-Disposition: inline In-Reply-To: <20160519063606.GA8574@pxdev.xzpeter.org> Sender: kvm-owner@vger.kernel.org List-ID: 2016-05-19 14:36+0800, Peter Xu: > On Fri, May 06, 2016 at 10:53:57PM +0200, Radim Kr=C4=8Dm=C3=A1=C5=99= wrote: > > kvm_irq_delivery_to_apic_fast and kvm_intr_is_single_vcpu_fast both > > compute the interrupt destination. Factor the code. > >=20 > > 'struct kvm_lapic **dst =3D NULL' had to be added to silence GCC. > > GCC might complain about potential NULL access in the future, becau= se it > > missed conditions that avoided uninitialized uses of dst. > >=20 > > Signed-off-by: Radim Kr=C4=8Dm=C3=A1=C5=99 > > --- > > arch/x86/kvm/lapic.c | 237 +++++++++++++++++++++------------------= ------------ > > 1 file changed, 99 insertions(+), 138 deletions(-) > >=20 > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > > index 1a2da0e5a373..6812e61c12d4 100644 > > --- a/arch/x86/kvm/lapic.c > > +++ b/arch/x86/kvm/lapic.c > > @@ -694,14 +694,94 @@ static void kvm_apic_disabled_lapic_found(str= uct kvm *kvm) > > } > > } > > =20 > > +/* If kvm_apic_map_get_dest_lapic returns true, then *bitmap encod= es accessible > > + * elements in the *dst array. Those are destinations for the int= errupt. >=20 > Here, we mentioned that "*bitmap" encodes accessible elements" if > returns true, while... >=20 >> + */ >> +static inline bool kvm_apic_map_get_dest_lapic(struct kvm *kvm, str= uct kvm_lapic *src, >> + struct kvm_lapic_irq *irq, struct kvm_apic_map *map, >> + struct kvm_lapic ***dst, unsigned long *bitmap) >> +{ >> + int i, lowest; >> + bool x2apic_ipi; >> + u16 cid; >> + >> + if (irq->shorthand =3D=3D APIC_DEST_SELF) { >> + *dst =3D &src; >> + *bitmap =3D 1; >> + return true; >> + } else if (irq->shorthand) >> + return false; >> + >> + x2apic_ipi =3D src && apic_x2apic_mode(src); >> + if (irq->dest_id =3D=3D (x2apic_ipi ? X2APIC_BROADCAST : APIC_BROA= DCAST)) >> + return false; >> + >> + if (!map) >> + return false; >> + >> + if (irq->dest_mode =3D=3D APIC_DEST_PHYSICAL) { >> + if (irq->dest_id >=3D ARRAY_SIZE(map->phys_map)) { >> + *bitmap =3D 0; >=20 > ... here we returned true, but actually we found no real candidates. > Would it be better that we directly return false in this case? There > are several similar places in this function as well. Not sure, so > asked. kvm_apic_map_get_dest_lapic() returns false if it doesn't know how to handle the irq, so another function has to be used to get destinations of the interrupt in question. 'irq->dest_id >=3D ARRAY_SIZE(map->phys_map)' is a case where we know t= hat the destination doesn't exist, so asking another function to get destinations would be pure waste. but the destination doesn't exist, so *bitmap has to be 0 to avoid invalid accesses. "return true;" means that **dst offsets encoded in *bitmap are correct destination(s). Ah, it is horribly convoluted. I am not sure if I can improve the comment, though ... > [...] >=20 > > @@ -821,69 +835,16 @@ bool kvm_intr_is_single_vcpu_fast(struct kvm = *kvm, struct kvm_lapic_irq *irq, > > rcu_read_lock(); > > map =3D rcu_dereference(kvm->arch.apic_map); > > =20 > > - if (!map) > > - goto out; > > + if (kvm_apic_map_get_dest_lapic(kvm, NULL, irq, map, &dst, &bitma= p) && > > + hweight16(bitmap) =3D=3D 1) { >=20 > If we follow the rule in comment above (return true only if there > are valid candidates), we may avoid the hweight16(bitmap) =3D=3D 1 > check as well. hweight16(bitmap) ranges from 0 to 16. Logical destination mode has multicast (the maximal addressible unit is one cluster, which has 16 LAPICs), but multicast cannot be optimized by hardware, so we have a special handling of cases where the destination is just one VCPU. e.g. for bitmap =3D 0b100101, the interrupt is directed to dst[0], dst[= 2] and dst[5].