Re: [PATCH 1/9] KVM: x86: add kvm_apic_map_get_dest_lapic

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

2016-05-19 14:36+0800, Peter Xu:
> On Fri, May 06, 2016 at 10:53:57PM +0200, Radim Krčmář wrote:
> > kvm_irq_delivery_to_apic_fast and kvm_intr_is_single_vcpu_fast both
> > compute the interrupt destination.  Factor the code.
> > 
> > 'struct kvm_lapic **dst = 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.
> > 
> > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> > ---
> >  arch/x86/kvm/lapic.c | 237 +++++++++++++++++++++------------------------------
> >  1 file changed, 99 insertions(+), 138 deletions(-)
> > 
> > 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(struct kvm *kvm)
> >  	}
> >  }
> >  
> > +/* If kvm_apic_map_get_dest_lapic returns true, then *bitmap encodes accessible
> > + * elements in the *dst array.  Those are destinations for the interrupt.
> 
> Here, we mentioned that "*bitmap" encodes accessible elements" if
> returns true, while...
> 
>> + */
>> +static inline bool kvm_apic_map_get_dest_lapic(struct kvm *kvm, struct 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 == APIC_DEST_SELF) {
>> +		*dst = &src;
>> +		*bitmap = 1;
>> +		return true;
>> +	} else if (irq->shorthand)
>> +		return false;
>> +
>> +	x2apic_ipi = src && apic_x2apic_mode(src);
>> +	if (irq->dest_id == (x2apic_ipi ? X2APIC_BROADCAST : APIC_BROADCAST))
>> +		return false;
>> +
>> +	if (!map)
>> +		return false;
>> +
>> +	if (irq->dest_mode == APIC_DEST_PHYSICAL) {
>> +		if (irq->dest_id >= ARRAY_SIZE(map->phys_map)) {
>> +			*bitmap = 0;
> 
> ... 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 >= ARRAY_SIZE(map->phys_map)' is a case where we know that
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 ...

> [...]
> 
> > @@ -821,69 +835,16 @@ bool kvm_intr_is_single_vcpu_fast(struct kvm *kvm, struct kvm_lapic_irq *irq,
> >  	rcu_read_lock();
> >  	map = rcu_dereference(kvm->arch.apic_map);
> >  
> > -	if (!map)
> > -		goto out;
> > +	if (kvm_apic_map_get_dest_lapic(kvm, NULL, irq, map, &dst, &bitmap) &&
> > +			hweight16(bitmap) == 1) {
> 
> If we follow the rule in comment above (return true only if there
> are valid candidates), we may avoid the hweight16(bitmap) == 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 = 0b100101, the interrupt is directed to dst[0], dst[2]
and dst[5].