Re: [PATCH] KVM: x86: x2apic broadcast with physical mode does not work

["Radim Krčmář" <rkrcmar@redhat.com>]

2014-10-01 10:40+0300, Nadav Amit:
> On Oct 1, 2014, at 3:09 AM, Radim Krčmář <rkrcmar@redhat.com> 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 value of
> >> FFFF_FFFFH is used for broadcast of interrupts in both logical destination and
> >> physical destination modes."
> > 
> > Btw. have you hit it in the wild?  (It was there so long that I suppose
> > that all OSes use a shorthand …)
> 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.
> > 
> > (Flat logical xapic or current logical x2apic; it doesn't have to do
> > much with clusters from Intel's vocabulary …)
> 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 sense to you?

'irq->dest_mode == 0' means that it is logical destination, but
'map->cid_mask != 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=0xf),
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 actually
> > 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;
> >> 
> >> +	/* broadcast on physical or multiple clusters is done slowly */
> >> +	if (irq->dest_id == map->broadcast &&
> > 
> > (If we dropped the second && condition, broadcast check could be moved
> > before we rcu_dereference the map.  Using kvm_apic_broadcast instead.)
> > 
> >> +	    (irq->dest_mode == 0 || map->cid_mask != 0))
> > 
> > It's a clever and pretty hacky use of cid_mask[1] ... I think it would
> > be better to have a broadcast fast-path for all modes, so we would do
> > something like this
> > 
> >  if (irq->dest == kvm_apic_broadcast(apic))
> >  	return HANDLE_BROADCAST(…);
> > 
> > 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 using 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.)

> > 
> >> +		goto out;
> > 
> > 
> > ---
> > 1: When we fix logical x2apic, it will have cid_mask > 0 and hopefully
> >   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 cycles on
> >   checking for this than it would on broadcasting through slow path :)
> 
> 
> Thanks for the feedback.
> 
> I completely agree with your revisions, yet I think they are orthogonal 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 the slow-path is significant.

I agree, so there is little reason to do the extra check for performance
reasons.  The check makes the code worse, which makes performance gain
the only possible excuse for it.