Re: [PATCH 5/8] KVM: x86: use MDA for interrupt matching

[Paolo Bonzini <pbonzini@redhat.com>]

On 29/01/2015 22:48, Radim Krčmář wrote:
> In mixed modes, we musn't deliver xAPIC IPIs like x2APIC and vice versa.
> Instead of preserving the information in apic_send_ipi(), we regain it
> by converting all destinations into correct APIC MDA in the slow path.
> This allows easier reasoning about subsequent matching.
> 
> kvm_apic_broadcast() had an interesting design decision:
> it didn't consider IOxAPIC 0xff as broadcast in x2APIC mode ...
> everything worked because IOxAPIC can't set that in physical mode and
> logical mode considered it as a message for first 8 VCPUs.
> This patch interprets IOxAPIC 0xff as x2APIC broadcast.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arch/x86/kvm/lapic.c | 40 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 33 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index aae043f38548..871ce6a2485b 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -581,15 +581,24 @@ static void apic_set_tpr(struct kvm_lapic *apic, u32 tpr)
>  	apic_update_ppr(apic);
>  }
>  
> -static bool kvm_apic_broadcast(struct kvm_lapic *apic, u32 dest)
> +static bool kvm_apic_broadcast(struct kvm_lapic *apic, u32 mda)
>  {
> -	return dest == (apic_x2apic_mode(apic) ?
> -			X2APIC_BROADCAST : APIC_BROADCAST);
> +	if (apic_x2apic_mode(apic))
> +		return mda == X2APIC_BROADCAST;
> +
> +	/* XXX: verify that xAPIC accepts x2APIC broadcast */
> +	return GET_APIC_DEST_FIELD(mda) == APIC_BROADCAST;
>  }
>  
> -static bool kvm_apic_match_physical_addr(struct kvm_lapic *apic, u32 dest)
> +static bool kvm_apic_match_physical_addr(struct kvm_lapic *apic, u32 mda)
>  {
> -	return kvm_apic_id(apic) == dest || kvm_apic_broadcast(apic, dest);
> +	if (kvm_apic_broadcast(apic, mda))
> +		return true;
> +
> +	if (apic_x2apic_mode(apic))
> +		return mda == kvm_apic_id(apic);
> +
> +	return mda == SET_APIC_DEST_FIELD(kvm_apic_id(apic));
>  }
>  
>  static bool kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
> @@ -606,6 +615,7 @@ static bool kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
>  		       && (logical_id & mda & 0xffff);
>  
>  	logical_id = GET_APIC_LOGICAL_ID(logical_id);
> +	mda = GET_APIC_DEST_FIELD(mda);
>  
>  	switch (kvm_apic_get_reg(apic, APIC_DFR)) {
>  	case APIC_DFR_FLAT:
> @@ -620,10 +630,26 @@ static bool kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
>  	}
>  }
>  
> +/* KVM APIC implementation has two quirks
> + *  - dest always begins at 0 while xAPIC MDA has offset 24,
> + *  - IOxAPIC messages have to be delivered (directly) to x2APIC.
> + */
> +static u32
> +kvm_apic_mda(unsigned int dest, struct kvm_lapic *ipi, bool x2apic_dest)

Please pass two struct kvm_lapic, so that you can write

	bool ipi = source != NULL;
	bool x2apic_mda = apic_x2apic_mode(ipi ? source : dest);

Looks a little nicer to me at least.

> +{
> +	bool x2apic_mda = ipi ? apic_x2apic_mode(ipi) : x2apic_dest;
> +
> +	if (!ipi && dest == APIC_BROADCAST)
> +		dest = X2APIC_BROADCAST;

This works, but it is not super-clear that you are shifting left by 24
here, and right in kvm_apic_broadcast().  What if you just make it

	if (!ipi && dest == APIC_BROADCAST && x2apic_mda)
		return X2APIC_BROADCAST.

?

Paolo

> +
> +	return x2apic_mda ? dest : SET_APIC_DEST_FIELD(dest);
> +}
> +
>  bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>  			   int short_hand, unsigned int dest, int dest_mode)
>  {
>  	struct kvm_lapic *target = vcpu->arch.apic;
> +	u32 mda = kvm_apic_mda(dest, source, apic_x2apic_mode(target));
>  
>  	apic_debug("target %p, source %p, dest 0x%x, "
>  		   "dest_mode 0x%x, short_hand 0x%x\n",
> @@ -633,9 +659,9 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>  	switch (short_hand) {
>  	case APIC_DEST_NOSHORT:
>  		if (dest_mode == APIC_DEST_PHYSICAL)
> -			return kvm_apic_match_physical_addr(target, dest);
> +			return kvm_apic_match_physical_addr(target, mda);
>  		else
> -			return kvm_apic_match_logical_addr(target, dest);
> +			return kvm_apic_match_logical_addr(target, mda);
>  	case APIC_DEST_SELF:
>  		return target == source;
>  	case APIC_DEST_ALLINC:
>