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

[Peter Xu <peterx@redhat.com>]

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