| Message ID | 1462568045-31085-2-git-send-email-rkrcmar@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. [...] > @@ -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. Thanks, -- peterx -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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]. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 25, 2016 at 06:02:46PM +0200, Radim Krčmář wrote: [...] > 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 ... Thanks for the explanation. How about: "Return true if the interrupt can be handled by fast path, false otherwise (in which case we still need to go through the slow path). Note: we may have zero kvm_lapic candidate even when we return true, which means the interrupt should be dropped. In this case, *bitmap would be zero." > > > [...] > > > > > @@ -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]. Yes, thanks! -- peterx -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. + */ +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; + } else { + *dst = &map->phys_map[irq->dest_id]; + *bitmap = 1; + } + return true; + } + + if (!kvm_apic_logical_map_valid(map)) + return false; + + apic_logical_id(map, irq->dest_id, &cid, (u16 *)bitmap); + + if (cid >= ARRAY_SIZE(map->logical_map)) { + *bitmap = 0; + return true; + } + + *dst = map->logical_map[cid]; + + if (!kvm_lowest_prio_delivery(irq)) + return true; + + if (!kvm_vector_hashing_enabled()) { + lowest = -1; + for_each_set_bit(i, bitmap, 16) { + if (!(*dst)[i]) + continue; + if (lowest < 0) + lowest = i; + else if (kvm_apic_compare_prio((*dst)[i]->vcpu, + (*dst)[lowest]->vcpu) < 0) + lowest = i; + } + } else { + if (!*bitmap) + return true; + + lowest = kvm_vector_to_index(irq->vector, hweight16(*bitmap), + bitmap, 16); + + if (!(*dst)[lowest]) { + kvm_apic_disabled_lapic_found(kvm); + *bitmap = 0; + return true; + } + } + + *bitmap = (lowest >= 0) ? 1 << lowest : 0; + + return true; +} + bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src, struct kvm_lapic_irq *irq, int *r, struct dest_map *dest_map) { struct kvm_apic_map *map; - unsigned long bitmap = 1; - struct kvm_lapic **dst; + unsigned long bitmap; + struct kvm_lapic **dst = NULL; int i; - bool ret, x2apic_ipi; + bool ret; *r = -1; @@ -710,86 +790,19 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src, return true; } - if (irq->shorthand) - return false; - - x2apic_ipi = src && apic_x2apic_mode(src); - if (irq->dest_id == (x2apic_ipi ? X2APIC_BROADCAST : APIC_BROADCAST)) - return false; - - ret = true; rcu_read_lock(); map = rcu_dereference(kvm->arch.apic_map); - if (!map) { - ret = false; - goto out; - } - - if (irq->dest_mode == APIC_DEST_PHYSICAL) { - if (irq->dest_id >= ARRAY_SIZE(map->phys_map)) - goto out; - - dst = &map->phys_map[irq->dest_id]; - } else { - u16 cid; - - if (!kvm_apic_logical_map_valid(map)) { - ret = false; - goto out; + ret = kvm_apic_map_get_dest_lapic(kvm, src, irq, map, &dst, &bitmap); + if (ret) + for_each_set_bit(i, &bitmap, 16) { + if (!dst[i]) + continue; + if (*r < 0) + *r = 0; + *r += kvm_apic_set_irq(dst[i]->vcpu, irq, dest_map); } - apic_logical_id(map, irq->dest_id, &cid, (u16 *)&bitmap); - - if (cid >= ARRAY_SIZE(map->logical_map)) - goto out; - - dst = map->logical_map[cid]; - - if (!kvm_lowest_prio_delivery(irq)) - goto set_irq; - - if (!kvm_vector_hashing_enabled()) { - int l = -1; - for_each_set_bit(i, &bitmap, 16) { - if (!dst[i]) - continue; - if (l < 0) - l = i; - else if (kvm_apic_compare_prio(dst[i]->vcpu, - dst[l]->vcpu) < 0) - l = i; - } - bitmap = (l >= 0) ? 1 << l : 0; - } else { - int idx; - unsigned int dest_vcpus; - - dest_vcpus = hweight16(bitmap); - if (dest_vcpus == 0) - goto out; - - idx = kvm_vector_to_index(irq->vector, - dest_vcpus, &bitmap, 16); - - if (!dst[idx]) { - kvm_apic_disabled_lapic_found(kvm); - goto out; - } - - bitmap = (idx >= 0) ? 1 << idx : 0; - } - } - -set_irq: - for_each_set_bit(i, &bitmap, 16) { - if (!dst[i]) - continue; - if (*r < 0) - *r = 0; - *r += kvm_apic_set_irq(dst[i]->vcpu, irq, dest_map); - } -out: rcu_read_unlock(); return ret; } @@ -812,8 +825,9 @@ bool kvm_intr_is_single_vcpu_fast(struct kvm *kvm, struct kvm_lapic_irq *irq, struct kvm_vcpu **dest_vcpu) { struct kvm_apic_map *map; + unsigned long bitmap; + struct kvm_lapic **dst = NULL; bool ret = false; - struct kvm_lapic *dst = NULL; if (irq->shorthand) return false; @@ -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) { + unsigned long i = find_first_bit(&bitmap, 16); - if (irq->dest_mode == APIC_DEST_PHYSICAL) { - if (irq->dest_id == 0xFF) - goto out; - - if (irq->dest_id >= ARRAY_SIZE(map->phys_map)) - goto out; - - dst = map->phys_map[irq->dest_id]; - if (dst && kvm_apic_present(dst->vcpu)) - *dest_vcpu = dst->vcpu; - else - goto out; - } else { - u16 cid; - unsigned long bitmap = 1; - int i, r = 0; - - if (!kvm_apic_logical_map_valid(map)) - goto out; - - apic_logical_id(map, irq->dest_id, &cid, (u16 *)&bitmap); - - if (cid >= ARRAY_SIZE(map->logical_map)) - goto out; - - if (kvm_vector_hashing_enabled() && - kvm_lowest_prio_delivery(irq)) { - int idx; - unsigned int dest_vcpus; - - dest_vcpus = hweight16(bitmap); - if (dest_vcpus == 0) - goto out; - - idx = kvm_vector_to_index(irq->vector, dest_vcpus, - &bitmap, 16); - - dst = map->logical_map[cid][idx]; - if (!dst) { - kvm_apic_disabled_lapic_found(kvm); - goto out; - } - - *dest_vcpu = dst->vcpu; - } else { - for_each_set_bit(i, &bitmap, 16) { - dst = map->logical_map[cid][i]; - if (++r == 2) - goto out; - } - - if (dst && kvm_apic_present(dst->vcpu)) - *dest_vcpu = dst->vcpu; - else - goto out; + if (dst[i]) { + *dest_vcpu = dst[i]->vcpu; + ret = true; } } - ret = true; -out: rcu_read_unlock(); return ret; }
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(-)