From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PATCH] KVM: x86: x2apic broadcast with physical mode does not work Date: Wed, 1 Oct 2014 15:35:06 +0200 Message-ID: <20141001133506.GA12083@potion.brq.redhat.com> References: <1412014558-16970-1-git-send-email-namit@cs.technion.ac.il> <20141001000956.GA7948@potion.redhat.com> <4A5CF035-AF20-465D-886F-449A85E8DB1D@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Nadav Amit , Paolo Bonzini , kvm@vger.kernel.org To: Nadav Amit Return-path: Received: from mx1.redhat.com ([209.132.183.28]:14385 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751834AbaJANfQ (ORCPT ); Wed, 1 Oct 2014 09:35:16 -0400 Content-Disposition: inline In-Reply-To: <4A5CF035-AF20-465D-886F-449A85E8DB1D@gmail.com> Sender: kvm-owner@vger.kernel.org List-ID: 2014-10-01 10:40+0300, Nadav Amit: > On Oct 1, 2014, at 3:09 AM, Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: > > 2014-09-29 21:15+0300, Nadav Amit: > >> KVM does not deliver x2APIC broadcast messages with physical mode.= Intel SDM > >> (10.12.9 ICR Operation in x2APIC Mode) states: "A destination ID v= alue of > >> FFFF_FFFFH is used for broadcast of interrupts in both logical des= tination and > >> physical destination modes." > >=20 > > Btw. have you hit it in the wild? (It was there so long that I sup= pose > > that all OSes use a shorthand =E2=80=A6) > Not in real OSes, but in some tests I run. Great, thanks. (Would be a -stable material otherwise.) > >> The fix tries to combine broadcast in different modes. Broadcast = can be done > >> in the fast-path if the delivery-mode is logical and there is only= a single > >> cluster. Otherwise, do it slowly. > >=20 > > (Flat logical xapic or current logical x2apic; it doesn't have to d= o > > much with clusters from Intel's vocabulary =E2=80=A6) > I meant if the ICR destination mode is logical (not the delivery-mode= ) and there is no more than one cluster (if clusters exist in this mode= ). > [It also applies to logical cluster-mode.] Does this phrasing makes s= ense to you? 'irq->dest_mode =3D=3D 0' means that it is logical destination, but 'map->cid_mask !=3D 0' is a variable that doesn't have a well defined relationship to the number of clusters -- it just works for the two cases right now. (Flat logical xapic has 0 clusters :) The original wording is better (xapic logical cluster has cid_mask=3D0x= f), and I was fine with it (just pointing out that it is confusing), which is why I put my comment into parentheses. It would be best to drop the condition and comment IMO. > > xapic broadcast is 0xff in flat and cluster mode, so it would actua= lly > > be another bugfix -- we miss the latter one right now; not that it = has > > any users. > I actually submitted a fix for cluster mode - http://www.spinics.net/= lists/kvm/msg106351.html > I now see Paolo did not apply it or responded. I will combine the two= patches to one patch-set for v2. Oh, I didn't see that, please do. > >> @@ -657,9 +663,13 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm= *kvm, struct kvm_lapic *src, > >> if (!map) > >> goto out; > >>=20 > >> + /* broadcast on physical or multiple clusters is done slowly */ > >> + if (irq->dest_id =3D=3D map->broadcast && > >=20 > > (If we dropped the second && condition, broadcast check could be mo= ved > > before we rcu_dereference the map. Using kvm_apic_broadcast instea= d.) > >=20 > >> + (irq->dest_mode =3D=3D 0 || map->cid_mask !=3D 0)) > >=20 > > It's a clever and pretty hacky use of cid_mask[1] ... I think it wo= uld > > be better to have a broadcast fast-path for all modes, so we would = do > > something like this > >=20 > > if (irq->dest =3D=3D kvm_apic_broadcast(apic)) > > return HANDLE_BROADCAST(=E2=80=A6); > >=20 > > in the end and having 'return 0' fallback for now is closer to our = goal. > I agree that broadcast may be done more efficiently, but broadcast us= ing shorthand is also done slowly today. True, we would probably completely rewrite both variants. > So I think it should go into different patch. (The rework, definitely, but there isn't much difference between 'goto out' and 'return 0' after our broadcast check.) > >=20 > >> + goto out; > >=20 > >=20 > > --- > > 1: When we fix logical x2apic, it will have cid_mask > 0 and hopefu= lly > > no-one uses it now, broken and capped at one 16 CPU cluster. > > That would leave us with flat logical mode that supports up to 8 > > CPUs, which is a hard performance decision -- I think that most > > guests will use other modes and the universe will lose more cycle= s on > > checking for this than it would on broadcasting through slow path= :) >=20 >=20 > Thanks for the feedback. >=20 > I completely agree with your revisions, yet I think they are orthogon= al and should go into different patches. > Since my current project focus on validation of hypervisors, I am not= sure I will have the time to do it well soon. Sure, what you are doing seems much more important. > Regardless, I am not sure whether the performance impact of taking th= e slow-path is significant. I agree, so there is little reason to do the extra check for performanc= e reasons. The check makes the code worse, which makes performance gain the only possible excuse for it.