From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Xu Subject: Re: [PATCH 1/9] KVM: x86: add kvm_apic_map_get_dest_lapic Date: Thu, 19 May 2016 14:36:06 +0800 Message-ID: <20160519063606.GA8574@pxdev.xzpeter.org> References: <1462568045-31085-1-git-send-email-rkrcmar@redhat.com> <1462568045-31085-2-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, Paolo Bonzini , "Lan, Tianyu" , Igor Mammedov , Jan Kiszka To: Radim =?utf-8?B?S3LEjW3DocWZ?= Return-path: Received: from mx1.redhat.com ([209.132.183.28]:51330 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753459AbcESGgL (ORCPT ); Thu, 19 May 2016 02:36:11 -0400 Content-Disposition: inline In-Reply-To: <1462568045-31085-2-git-send-email-rkrcmar@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Fri, May 06, 2016 at 10:53:57PM +0200, Radim Kr=C4=8Dm=C3=A1=C5=99 w= rote: > 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, because= 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(struc= t kvm *kvm) > } > } > =20 > +/* If kvm_apic_map_get_dest_lapic returns true, then *bitmap encodes= accessible > + * elements in the *dst array. Those are destinations for the inter= rupt. Here, we mentioned that "*bitmap" encodes accessible elements" if returns true, while... > + */ > +static inline bool kvm_apic_map_get_dest_lapic(struct kvm *kvm, stru= ct 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_BROAD= CAST)) > + 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; =2E.. 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. [...] > @@ -821,69 +835,16 @@ bool kvm_intr_is_single_vcpu_fast(struct kvm *k= vm, 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, &bitmap)= && > + hweight16(bitmap) =3D=3D 1) { 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. Thanks, -- peterx