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@xxxxxxxxxx>
> > ---
> > 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].
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
