Re: [PATCH 7/8] KVM: x86: avoid logical_map when it is invalid

[Paolo Bonzini <pbonzini@redhat.com>]

On 29/01/2015 22:48, Radim Krčmář wrote:
> We want to support mixed modes and the easiest solution is to avoid
> optimizing those weird and unlikely scenarios.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  1 +
>  arch/x86/kvm/lapic.c            | 16 ++++++++++++++++
>  arch/x86/kvm/lapic.h            |  4 ++++
>  3 files changed, 21 insertions(+)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 26d0f0f646d3..fec3188cabbb 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -554,6 +554,7 @@ struct kvm_arch_memory_slot {
>  
>  struct kvm_apic_map {
>  	struct rcu_head rcu;
> +	u8 mode;
>  	u8 ldr_bits;
>  	/* fields bellow are used to decode ldr values in different modes */
>  	u32 cid_shift, cid_mask, lid_mask;
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index fab007509047..621d9df6ac63 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -162,16 +162,19 @@ static void recalculate_apic_map(struct kvm *kvm)
>  			new->ldr_bits = 32;
>  			new->cid_shift = 16;
>  			new->cid_mask = new->lid_mask = 0xffff;
> +			new->mode |= KVM_APIC_MODE_X2APIC;
>  		} else if (kvm_apic_get_reg(apic, APIC_LDR)) {
>  			if (kvm_apic_get_reg(apic, APIC_DFR) ==
>  							APIC_DFR_CLUSTER) {
>  				new->cid_shift = 4;
>  				new->cid_mask = 0xf;
>  				new->lid_mask = 0xf;
> +				new->mode |= KVM_APIC_MODE_XAPIC_CLUSTER;
>  			} else {
>  				new->cid_shift = 8;
>  				new->cid_mask = 0;
>  				new->lid_mask = 0xff;
> +				new->mode |= KVM_APIC_MODE_XAPIC_FLAT;
>  			}
>  		}
>  
> @@ -201,6 +204,13 @@ static void recalculate_apic_map(struct kvm *kvm)
>  
>  		if (aid < ARRAY_SIZE(new->phys_map))
>  			new->phys_map[aid] = apic;
> +
> +		/* The logical map is definitely wrong if we have multiple
> +		 * modes at the same time.  Physical is still right though.
> +		 */
> +		if (hweight8(new->mode) != 1)

Better (more optimized):

	if (new->mode & (new->mode - 1))

Please add a comment to kvm_irq_delivery_to_apic_fast to explain what
you are doing.

> +			continue;
> +
>  		if (lid && cid < ARRAY_SIZE(new->logical_map))
>  			new->logical_map[cid][ffs(lid) - 1] = apic;
>  	}
> @@ -720,6 +730,12 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
>  		if (cid >= ARRAY_SIZE(map->logical_map))
>  			goto out;
>  
> +		if (hweight8(map->mode) != 1) {
> +			/* Not deliverable with optimized map. */
> +			ret = false;
> +			goto out;
> +		}

Put this before the computation of cid and mda.  The cid and mda are all
wrong with a mixed map, and the result of the "if" before is influenced
by the wrong cid.  Fixed by patch 8, but better get it right here.

Paolo

>  		dst = map->logical_map[cid];
>  
>  		bitmap = apic_logical_id(map, mda);
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index c1ef25c89508..fd0197a93862 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -8,6 +8,10 @@
>  #define KVM_APIC_INIT		0
>  #define KVM_APIC_SIPI		1
>  
> +#define KVM_APIC_MODE_XAPIC_FLAT            (1 << 0)
> +#define KVM_APIC_MODE_XAPIC_CLUSTER         (1 << 1)
> +#define KVM_APIC_MODE_X2APIC                (1 << 2)
> +
>  struct kvm_timer {
>  	struct hrtimer timer;
>  	s64 period; 				/* unit: ns */
>