[PATCH 0/8] KVM: minor APIC fixes and cleanups

[PATCH 0/8] KVM: minor APIC fixes and cleanups

[Radim Krčmář]

This patch series is made our of three logical parts,

[1-3/8] are just a cleanup and could be omitted
[4-6/8] improve broadcast detection and unoptimized delivery
[7-8/8] handle mixed mode (by falling back to improvements from [5-6/9])

Radim Krčmář (8):
  KVM: x86: return bool from kvm_apic_match*()
  KVM: x86: cleanup kvm_apic_match_*()
  KVM: x86: replace 0 with APIC_DEST_PHYSICAL
  KVM: x86: fix x2apic logical address matching
  KVM: x86: use MDA for interrupt matching
  KVM: x86: allow mixed APIC mode broadcast
  KVM: x86: avoid logical_map when it is invalid
  KVM: x86: simplify kvm_apic_map

 arch/x86/include/asm/kvm_host.h |   4 +-
 arch/x86/kvm/ioapic.h           |   2 +-
 arch/x86/kvm/lapic.c            | 176 +++++++++++++++++++---------------------
 arch/x86/kvm/lapic.h            |  35 ++++----
 4 files changed, 106 insertions(+), 111 deletions(-)

-- 
2.2.2

[PATCH 1/8] KVM: x86: return bool from kvm_apic_match*()

[Radim Krčmář]

And don't export the internal ones while at it.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/ioapic.h | 2 +-
 arch/x86/kvm/lapic.c  | 8 ++++----
 arch/x86/kvm/lapic.h  | 2 --
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h
index 3c9195535ffc..c2e36d934af4 100644
--- a/arch/x86/kvm/ioapic.h
+++ b/arch/x86/kvm/ioapic.h
@@ -98,7 +98,7 @@ static inline struct kvm_ioapic *ioapic_irqchip(struct kvm *kvm)
 }
 
 void kvm_rtc_eoi_tracking_restore_one(struct kvm_vcpu *vcpu);
-int kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
+bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
 		int short_hand, unsigned int dest, int dest_mode);
 int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2);
 void kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu, int vector,
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index d1b7d42c3af0..478dd8bd653b 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -581,18 +581,18 @@ static void apic_set_tpr(struct kvm_lapic *apic, u32 tpr)
 	apic_update_ppr(apic);
 }
 
-static int kvm_apic_broadcast(struct kvm_lapic *apic, u32 dest)
+static bool kvm_apic_broadcast(struct kvm_lapic *apic, u32 dest)
 {
 	return dest == (apic_x2apic_mode(apic) ?
 			X2APIC_BROADCAST : APIC_BROADCAST);
 }
 
-int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u32 dest)
+static bool kvm_apic_match_physical_addr(struct kvm_lapic *apic, u32 dest)
 {
 	return kvm_apic_id(apic) == dest || kvm_apic_broadcast(apic, dest);
 }
 
-int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
+static bool kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
 {
 	int result = 0;
 	u32 logical_id;
@@ -626,7 +626,7 @@ int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
 	return result;
 }
 
-int kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
+bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
 			   int short_hand, unsigned int dest, int dest_mode)
 {
 	int result = 0;
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 7054437944cd..c1ef25c89508 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -58,8 +58,6 @@ void kvm_apic_set_version(struct kvm_vcpu *vcpu);
 
 void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr);
 void kvm_apic_update_irr(struct kvm_vcpu *vcpu, u32 *pir);
-int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u32 dest);
-int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda);
 int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
 		unsigned long *dest_map);
 int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type);
-- 
2.2.2

Re: [PATCH 1/8] KVM: x86: return bool from kvm_apic_match*()

[Joe Perches]

On Thu, 2015-01-29 at 22:48 +0100, Radim Krčmář wrote:
> And don't export the internal ones while at it.
[]
> -int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
> +static bool kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
>  {
>  	int result = 0;
>  	u32 logical_id;
> @@ -626,7 +626,7 @@ int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
>  	return result;
>  }
>  
> -int kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
> +bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>  			   int short_hand, unsigned int dest, int dest_mode)
>  {
>  	int result = 0;

Perhaps these result variables should be bool.

Re: [PATCH 1/8] KVM: x86: return bool from kvm_apic_match*()

[Paolo Bonzini]

On 29/01/2015 23:10, Joe Perches wrote:
> On Thu, 2015-01-29 at 22:48 +0100, Radim Krčmář wrote:
>> And don't export the internal ones while at it.
> []
>> -int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
>> +static bool kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
>>  {
>>  	int result = 0;
>>  	u32 logical_id;
>> @@ -626,7 +626,7 @@ int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
>>  	return result;
>>  }
>>  
>> -int kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>> +bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>>  			   int short_hand, unsigned int dest, int dest_mode)
>>  {
>>  	int result = 0;
> 
> Perhaps these result variables should be bool.

See patch 2.

Paolo

[PATCH 2/8] KVM: x86: cleanup kvm_apic_match_*()

[Radim Krčmář]

The majority of this patch turns
  result = 0; if (CODE) result = 1; return result;
into
  return CODE;
because we return bool now.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/lapic.c | 44 +++++++++++++++-----------------------------
 1 file changed, 15 insertions(+), 29 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 478dd8bd653b..271e7d076879 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -594,42 +594,34 @@ static bool kvm_apic_match_physical_addr(struct kvm_lapic *apic, u32 dest)
 
 static bool kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
 {
-	int result = 0;
 	u32 logical_id;
 
 	if (kvm_apic_broadcast(apic, mda))
-		return 1;
+		return true;
 
-	if (apic_x2apic_mode(apic)) {
-		logical_id = kvm_apic_get_reg(apic, APIC_LDR);
+	logical_id = kvm_apic_get_reg(apic, APIC_LDR);
+
+	if (apic_x2apic_mode(apic))
 		return logical_id & mda;
-	}
 
-	logical_id = GET_APIC_LOGICAL_ID(kvm_apic_get_reg(apic, APIC_LDR));
+	logical_id = GET_APIC_LOGICAL_ID(logical_id);
 
 	switch (kvm_apic_get_reg(apic, APIC_DFR)) {
 	case APIC_DFR_FLAT:
-		if (logical_id & mda)
-			result = 1;
-		break;
+		return logical_id & mda;
 	case APIC_DFR_CLUSTER:
-		if (((logical_id >> 4) == (mda >> 0x4))
-		    && (logical_id & mda & 0xf))
-			result = 1;
-		break;
+		return ((logical_id >> 4) == (mda >> 4))
+		       && (logical_id & mda & 0xf);
 	default:
 		apic_debug("Bad DFR vcpu %d: %08x\n",
 			   apic->vcpu->vcpu_id, kvm_apic_get_reg(apic, APIC_DFR));
-		break;
+		return false;
 	}
-
-	return result;
 }
 
 bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
 			   int short_hand, unsigned int dest, int dest_mode)
 {
-	int result = 0;
 	struct kvm_lapic *target = vcpu->arch.apic;
 
 	apic_debug("target %p, source %p, dest 0x%x, "
@@ -641,27 +633,21 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
 	case APIC_DEST_NOSHORT:
 		if (dest_mode == 0)
 			/* Physical mode. */
-			result = kvm_apic_match_physical_addr(target, dest);
+			return kvm_apic_match_physical_addr(target, dest);
 		else
 			/* Logical mode. */
-			result = kvm_apic_match_logical_addr(target, dest);
-		break;
+			return kvm_apic_match_logical_addr(target, dest);
 	case APIC_DEST_SELF:
-		result = (target == source);
-		break;
+		return target == source;
 	case APIC_DEST_ALLINC:
-		result = 1;
-		break;
+		return true;
 	case APIC_DEST_ALLBUT:
-		result = (target != source);
-		break;
+		return target != source;
 	default:
 		apic_debug("kvm: apic: Bad dest shorthand value %x\n",
 			   short_hand);
-		break;
+		return false;
 	}
-
-	return result;
 }
 
 bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
-- 
2.2.2

Re: [PATCH 2/8] KVM: x86: cleanup kvm_apic_match_*()

[Paolo Bonzini]

On 29/01/2015 22:48, Radim Krčmář wrote:
> The majority of this patch turns
>   result = 0; if (CODE) result = 1; return result;
> into
>   return CODE;
> because we return bool now.

Added a bunch of "!= 0" to avoid automagic conversion to bool.

Paolo

> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arch/x86/kvm/lapic.c | 44 +++++++++++++++-----------------------------
>  1 file changed, 15 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 478dd8bd653b..271e7d076879 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -594,42 +594,34 @@ static bool kvm_apic_match_physical_addr(struct kvm_lapic *apic, u32 dest)
>  
>  static bool kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
>  {
> -	int result = 0;
>  	u32 logical_id;
>  
>  	if (kvm_apic_broadcast(apic, mda))
> -		return 1;
> +		return true;
>  
> -	if (apic_x2apic_mode(apic)) {
> -		logical_id = kvm_apic_get_reg(apic, APIC_LDR);
> +	logical_id = kvm_apic_get_reg(apic, APIC_LDR);
> +
> +	if (apic_x2apic_mode(apic))
>  		return logical_id & mda;
> -	}
>  
> -	logical_id = GET_APIC_LOGICAL_ID(kvm_apic_get_reg(apic, APIC_LDR));
> +	logical_id = GET_APIC_LOGICAL_ID(logical_id);
>  
>  	switch (kvm_apic_get_reg(apic, APIC_DFR)) {
>  	case APIC_DFR_FLAT:
> -		if (logical_id & mda)
> -			result = 1;
> -		break;
> +		return logical_id & mda;
>  	case APIC_DFR_CLUSTER:
> -		if (((logical_id >> 4) == (mda >> 0x4))
> -		    && (logical_id & mda & 0xf))
> -			result = 1;
> -		break;
> +		return ((logical_id >> 4) == (mda >> 4))
> +		       && (logical_id & mda & 0xf);
>  	default:
>  		apic_debug("Bad DFR vcpu %d: %08x\n",
>  			   apic->vcpu->vcpu_id, kvm_apic_get_reg(apic, APIC_DFR));
> -		break;
> +		return false;
>  	}
> -
> -	return result;
>  }
>  
>  bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>  			   int short_hand, unsigned int dest, int dest_mode)
>  {
> -	int result = 0;
>  	struct kvm_lapic *target = vcpu->arch.apic;
>  
>  	apic_debug("target %p, source %p, dest 0x%x, "
> @@ -641,27 +633,21 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>  	case APIC_DEST_NOSHORT:
>  		if (dest_mode == 0)
>  			/* Physical mode. */
> -			result = kvm_apic_match_physical_addr(target, dest);
> +			return kvm_apic_match_physical_addr(target, dest);
>  		else
>  			/* Logical mode. */
> -			result = kvm_apic_match_logical_addr(target, dest);
> -		break;
> +			return kvm_apic_match_logical_addr(target, dest);
>  	case APIC_DEST_SELF:
> -		result = (target == source);
> -		break;
> +		return target == source;
>  	case APIC_DEST_ALLINC:
> -		result = 1;
> -		break;
> +		return true;
>  	case APIC_DEST_ALLBUT:
> -		result = (target != source);
> -		break;
> +		return target != source;
>  	default:
>  		apic_debug("kvm: apic: Bad dest shorthand value %x\n",
>  			   short_hand);
> -		break;
> +		return false;
>  	}
> -
> -	return result;
>  }
>  
>  bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
> 

Re: [PATCH 2/8] KVM: x86: cleanup kvm_apic_match_*()

[Radim Krčmář]

2015-01-30 09:52+0100, Paolo Bonzini:
> On 29/01/2015 22:48, Radim Krčmář wrote:
> > The majority of this patch turns
> >   result = 0; if (CODE) result = 1; return result;
> > into
> >   return CODE;
> > because we return bool now.
> 
> Added a bunch of "!= 0" to avoid automagic conversion to bool.

Makes sense, (valid code isn't automatically desirable)
thank you.

Re: [PATCH 2/8] KVM: x86: cleanup kvm_apic_match_*()

[Radim Krčmář]

2015-01-30 09:52+0100, Paolo Bonzini:
> On 29/01/2015 22:48, Radim Krčmář wrote:
> > The majority of this patch turns
> >   result = 0; if (CODE) result = 1; return result;
> > into
> >   return CODE;
> > because we return bool now.
> 
> Added a bunch of "!= 0" to avoid automagic conversion to bool.

(I haven't checked it earlier ... sorry.)

> > -		if (((logical_id >> 4) == (mda >> 0x4))
> > -		    && (logical_id & mda & 0xf))
> > -			result = 1;
> > -		break;
> > +		return ((logical_id >> 4) == (mda >> 4))
> > +		       && (logical_id & mda & 0xf);

was merged as

+		return ((logical_id >> 4) == (mda >> 4))
+		       && (logical_id & mda & 0xf) != 0;

but it has to be parenthesized ('&&' has lower precedence than '!=').

Re: [PATCH 2/8] KVM: x86: cleanup kvm_apic_match_*()

[Paolo Bonzini]

On 02/02/2015 15:26, Radim Krčmář wrote:
>>> > > +		return ((logical_id >> 4) == (mda >> 4))
>>> > > +		       && (logical_id & mda & 0xf);
> was merged as
> 
> +		return ((logical_id >> 4) == (mda >> 4))
> +		       && (logical_id & mda & 0xf) != 0;
> 
> but it has to be parenthesized ('&&' has lower precedence than '!=').

Lower precedence means that the merged version is right (unless my brain
went bonkers, which I cannot exclude).  "!=" has higher precedence and
thus it is implicitly parenthesized.

In fact the first comparison could have its parentheses removed as well.

Paolo

Re: [PATCH 2/8] KVM: x86: cleanup kvm_apic_match_*()

[Radim Krčmář]

2015-02-02 15:28+0100, Paolo Bonzini:
> 
> 
> On 02/02/2015 15:26, Radim Krčmář wrote:
> >>> > > +		return ((logical_id >> 4) == (mda >> 4))
> >>> > > +		       && (logical_id & mda & 0xf);
> > was merged as
> > 
> > +		return ((logical_id >> 4) == (mda >> 4))
> > +		       && (logical_id & mda & 0xf) != 0;
> > 
> > but it has to be parenthesized ('&&' has lower precedence than '!=').
> 
> Lower precedence means that the merged version is right (unless my brain
> went bonkers, which I cannot exclude).  "!=" has higher precedence and
> thus it is implicitly parenthesized.
> 
> In fact the first comparison could have its parentheses removed as well.

Yes, it could be,
  logical_id >> 4 == mda >> 4 && (logical_id & mda & 0xf) != 0

I missed the point and thought that you wanted to turn the whole
expression into bool, when it already was one.

Re: [PATCH 2/8] KVM: x86: cleanup kvm_apic_match_*()

[Radim Krčmář]

2015-02-02 15:26+0100, Radim Krčmář:
> 2015-01-30 09:52+0100, Paolo Bonzini:
> +		return ((logical_id >> 4) == (mda >> 4))
> +		       && (logical_id & mda & 0xf) != 0;
> 
> but it has to be parenthesized ('&&' has lower precedence than '!=').

No, my bad, I understood it now.

[PATCH 3/8] KVM: x86: replace 0 with APIC_DEST_PHYSICAL

[Radim Krčmář]

To make the code self-documenting.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/lapic.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 271e7d076879..0ee743c6b4f1 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -631,11 +631,9 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
 	ASSERT(target);
 	switch (short_hand) {
 	case APIC_DEST_NOSHORT:
-		if (dest_mode == 0)
-			/* Physical mode. */
+		if (dest_mode == APIC_DEST_PHYSICAL)
 			return kvm_apic_match_physical_addr(target, dest);
 		else
-			/* Logical mode. */
 			return kvm_apic_match_logical_addr(target, dest);
 	case APIC_DEST_SELF:
 		return target == source;
@@ -680,7 +678,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
 
 	ret = true;
 
-	if (irq->dest_mode == 0) { /* physical mode */
+	if (irq->dest_mode == APIC_DEST_PHYSICAL) {
 		if (irq->dest_id >= ARRAY_SIZE(map->phys_map))
 			goto out;
 
-- 
2.2.2

[PATCH 4/8] KVM: x86: fix x2apic logical address matching

[Radim Krčmář]

We cannot hit the bug now, but future patches will expose this path.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/lapic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 0ee743c6b4f1..aae043f38548 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -602,7 +602,8 @@ static bool kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
 	logical_id = kvm_apic_get_reg(apic, APIC_LDR);
 
 	if (apic_x2apic_mode(apic))
-		return logical_id & mda;
+		return ((logical_id >> 16) == (mda >> 16))
+		       && (logical_id & mda & 0xffff);
 
 	logical_id = GET_APIC_LOGICAL_ID(logical_id);
 
-- 
2.2.2

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

[Radim Krčmář]

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)
+{
+	bool x2apic_mda = ipi ? apic_x2apic_mode(ipi) : x2apic_dest;
+
+	if (!ipi && dest == APIC_BROADCAST)
+		dest = X2APIC_BROADCAST;
+
+	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:
-- 
2.2.2

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

[Paolo Bonzini]

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:
> 

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

[Radim Krčmář]

2015-01-30 10:03+0100, Paolo Bonzini:
> On 29/01/2015 22:48, Radim Krčmář wrote:
> > +/* 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.

Definitely.

> > +{
> > +	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.

It's better, it will be that way in v2 of [5-8/8], thanks.

(I was using it twice in earlier iterations and didn't think again.)

[PATCH 6/8] KVM: x86: allow mixed APIC mode broadcast

[Radim Krčmář]

Broadcast allowed only one global APIC mode, but mixed modes are
theoretically possible.  x2APIC IPI doesn't mean 0xff as broadcast,
the rest does.

(We also save one rcu dereference with broadcasts.)

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/kvm/lapic.c            | 13 ++++++++-----
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 843bea0e70fd..26d0f0f646d3 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -556,7 +556,7 @@ struct kvm_apic_map {
 	struct rcu_head rcu;
 	u8 ldr_bits;
 	/* fields bellow are used to decode ldr values in different modes */
-	u32 cid_shift, cid_mask, lid_mask, broadcast;
+	u32 cid_shift, cid_mask, lid_mask;
 	struct kvm_lapic *phys_map[256];
 	/* first index is cluster id second is cpu id in a cluster */
 	struct kvm_lapic *logical_map[16][16];
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 871ce6a2485b..fab007509047 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -151,7 +151,6 @@ static void recalculate_apic_map(struct kvm *kvm)
 	new->cid_shift = 8;
 	new->cid_mask = 0;
 	new->lid_mask = 0xff;
-	new->broadcast = APIC_BROADCAST;
 
 	kvm_for_each_vcpu(i, vcpu, kvm) {
 		struct kvm_lapic *apic = vcpu->arch.apic;
@@ -163,7 +162,6 @@ static void recalculate_apic_map(struct kvm *kvm)
 			new->ldr_bits = 32;
 			new->cid_shift = 16;
 			new->cid_mask = new->lid_mask = 0xffff;
-			new->broadcast = X2APIC_BROADCAST;
 		} else if (kvm_apic_get_reg(apic, APIC_LDR)) {
 			if (kvm_apic_get_reg(apic, APIC_DFR) ==
 							APIC_DFR_CLUSTER) {
@@ -683,6 +681,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
 	struct kvm_lapic **dst;
 	int i;
 	bool ret = false;
+	bool x2apic_ipi = src && apic_x2apic_mode(src);
 
 	*r = -1;
 
@@ -694,15 +693,19 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
 	if (irq->shorthand)
 		return false;
 
+	/* XXX: A superset of x2APIC broadcasts is fine in practice as long as
+	 * we don't support APIC ID > 0xfeffffff.
+	 */
+	if ((x2apic_ipi ? GET_APIC_DEST_FIELD(irq->dest_id) : irq->dest_id)
+	    == APIC_BROADCAST)
+		return false;
+
 	rcu_read_lock();
 	map = rcu_dereference(kvm->arch.apic_map);
 
 	if (!map)
 		goto out;
 
-	if (irq->dest_id == map->broadcast)
-		goto out;
-
 	ret = true;
 
 	if (irq->dest_mode == APIC_DEST_PHYSICAL) {
-- 
2.2.2

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

[Radim Krčmář]

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)
+			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;
+		}
+
 		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 */
-- 
2.2.2

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

[Paolo Bonzini]

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.

Hmm, what happens if use maxcpus to disable some CPUs at startup?  Could
the disabled CPUs remain in xapic mode while the others move to x2apic?

Paolo

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

[Radim Krčmář]

2015-01-30 10:19+0100, Paolo Bonzini:
> 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.
> 
> Hmm, what happens if use maxcpus to disable some CPUs at startup?

Their APICs are kept sw-disabled in xAPIC mode.

>                                                                    Could
> the disabled CPUs remain in xapic mode while the others move to x2apic?

Yes, but it would only matter if the OS set LDR on them.
I mark x2APIC with perfectly configured xAPIC on unused processors as
"weird" ... (Linux 3.18 and 2.6.32 doesn't do it.)

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

[Paolo Bonzini]

On 30/01/2015 15:21, Radim Krčmář wrote:
> Yes, but it would only matter if the OS set LDR on them.
> I mark x2APIC with perfectly configured xAPIC on unused processors as
> "weird" ... (Linux 3.18 and 2.6.32 doesn't do it.)

Ok.

Paolo

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

[Paolo Bonzini]

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 */
> 

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

[Radim Krčmář]

2015-01-30 10:38+0100, Paolo Bonzini:
> On 29/01/2015 22:48, Radim Krčmář wrote:
> > +		if (hweight8(new->mode) != 1)
> 
> Better (more optimized):
> 
> 	if (new->mode & (new->mode - 1))

True, hweight needs to have X86_FEATURE_POPCNT to be efficient ...

Do you know of a difference with it?
  new->mode & (new->mode - 1)   |  hweight8(new->mode) != 1
    lea    -0x1(%rax),%edi      |    popcnt %edi,%eax
    test   %eax,%edi            |    cmp    $1,%eax

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

Would naming it kvm_apic_need_slow_delivery(), or something, be enough?

> > +		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.

Will do,

thanks.

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

[Paolo Bonzini]

On 30/01/2015 15:56, Radim Krčmář wrote:
> 2015-01-30 10:38+0100, Paolo Bonzini:
>> On 29/01/2015 22:48, Radim Krčmář wrote:
>>> +		if (hweight8(new->mode) != 1)
>>
>> Better (more optimized):
>>
>> 	if (new->mode & (new->mode - 1))
> 
> True, hweight needs to have X86_FEATURE_POPCNT to be efficient ...
> 
> Do you know of a difference with it?
>   new->mode & (new->mode - 1)   |  hweight8(new->mode) != 1
>     lea    -0x1(%rax),%edi      |    popcnt %edi,%eax
>     test   %eax,%edi            |    cmp    $1,%eax

x & (x - 1) is really hweight8(new->mode) > 1.  So if new->mode == 0 it
would have a different result.

>> Please add a comment to kvm_irq_delivery_to_apic_fast to explain what
>> you are doing.
> 
> Would naming it kvm_apic_need_slow_delivery(), or something, be enough?

Or kvm_apic_map_valid() perhaps?

Paolo

>>> +		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.
> 
> Will do,
> 
> thanks.
> 

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

[Radim Krčmář]

2015-01-30 16:10+0100, Paolo Bonzini:
> On 30/01/2015 15:56, Radim Krčmář wrote:
> > Do you know of a difference with it?
> >   new->mode & (new->mode - 1)   |  hweight8(new->mode) != 1
> >     lea    -0x1(%rax),%edi      |    popcnt %edi,%eax
> >     test   %eax,%edi            |    cmp    $1,%eax
> 
> x & (x - 1) is really hweight8(new->mode) > 1.  So if new->mode == 0 it
> would have a different result.

(I was thinking if execution profile of those two instructions isn't
 vastly different.

 ">" in this check works too ... later it seemed like it was pushing
 lucky coincidence too far, so it ended as "!=" :)

> >> Please add a comment to kvm_irq_delivery_to_apic_fast to explain what
> >> you are doing.
> > 
> > Would naming it kvm_apic_need_slow_delivery(), or something, be enough?
> 
> Or kvm_apic_map_valid() perhaps?

Sure, thanks.

[PATCH 8/8] KVM: x86: simplify kvm_apic_map

[Radim Krčmář]

recalculate_apic_map() uses two passes over all VCPUs.  This is a relic
from time when we selected a global mode in the first pass and set up
the optimized table in the second pass (to have a consistent mode).

Recent changes made mixed mode unoptimized and we can do it in one pass.
Format of logical MDA is a function of the mode, so we encode it in
apic_logical_id() and drop obsoleted variables from the struct.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |  3 --
 arch/x86/kvm/lapic.c            | 78 ++++++++++-------------------------------
 arch/x86/kvm/lapic.h            | 29 ++++++++-------
 3 files changed, 36 insertions(+), 74 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index fec3188cabbb..14b6b0fd17b1 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -555,9 +555,6 @@ 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;
 	struct kvm_lapic *phys_map[256];
 	/* first index is cluster id second is cpu id in a cluster */
 	struct kvm_lapic *logical_map[16][16];
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 621d9df6ac63..f74557791a77 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -146,49 +146,6 @@ static void recalculate_apic_map(struct kvm *kvm)
 	if (!new)
 		goto out;
 
-	new->ldr_bits = 8;
-	/* flat mode is default */
-	new->cid_shift = 8;
-	new->cid_mask = 0;
-	new->lid_mask = 0xff;
-
-	kvm_for_each_vcpu(i, vcpu, kvm) {
-		struct kvm_lapic *apic = vcpu->arch.apic;
-
-		if (!kvm_apic_present(vcpu))
-			continue;
-
-		if (apic_x2apic_mode(apic)) {
-			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;
-			}
-		}
-
-		/*
-		 * All APICs have to be configured in the same mode by an OS.
-		 * We take advatage of this while building logical id loockup
-		 * table. After reset APICs are in software disabled mode, so if
-		 * we find apic with different setting we assume this is the mode
-		 * OS wants all apics to be in; build lookup table accordingly.
-		 */
-		if (kvm_apic_sw_enabled(apic))
-			break;
-	}
-
 	kvm_for_each_vcpu(i, vcpu, kvm) {
 		struct kvm_lapic *apic = vcpu->arch.apic;
 		u16 cid, lid;
@@ -198,17 +155,22 @@ static void recalculate_apic_map(struct kvm *kvm)
 			continue;
 
 		aid = kvm_apic_id(apic);
-		ldr = kvm_apic_get_reg(apic, APIC_LDR);
-		cid = apic_cluster_id(new, ldr);
-		lid = apic_logical_id(new, ldr);
-
 		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)
+		ldr = kvm_apic_get_reg(apic, APIC_LDR);
+
+		if (apic_x2apic_mode(apic)) {
+			new->mode |= KVM_APIC_MODE_X2APIC;
+		} else if (ldr) {
+			ldr = GET_APIC_LOGICAL_ID(ldr);
+			if (kvm_apic_get_reg(apic, APIC_DFR) == APIC_DFR_FLAT)
+				new->mode |= KVM_APIC_MODE_XAPIC_FLAT;
+			else
+				new->mode |= KVM_APIC_MODE_XAPIC_CLUSTER;
+		}
+
+		if (!apic_logical_id(new, ldr, &cid, &lid))
 			continue;
 
 		if (lid && cid < ARRAY_SIZE(new->logical_map))
@@ -724,22 +686,20 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
 
 		dst = &map->phys_map[irq->dest_id];
 	} else {
-		u32 mda = irq->dest_id << (32 - map->ldr_bits);
-		u16 cid = apic_cluster_id(map, mda);
+		u16 cid;
 
-		if (cid >= ARRAY_SIZE(map->logical_map))
-			goto out;
-
-		if (hweight8(map->mode) != 1) {
+		if (!apic_logical_id(map, irq->dest_id, &cid, (u16 *)&bitmap))
+		{
 			/* Not deliverable with optimized map. */
 			ret = false;
 			goto out;
 		}
 
+		if (cid >= ARRAY_SIZE(map->logical_map))
+			goto out;
+
 		dst = map->logical_map[cid];
 
-		bitmap = apic_logical_id(map, mda);
-
 		if (irq->delivery_mode == APIC_DM_LOWEST) {
 			int l = -1;
 			for_each_set_bit(i, &bitmap, 16) {
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index fd0197a93862..320716e301a8 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -151,19 +151,24 @@ static inline bool kvm_apic_vid_enabled(struct kvm *kvm)
 	return kvm_x86_ops->vm_has_apicv(kvm);
 }
 
-static inline u16 apic_cluster_id(struct kvm_apic_map *map, u32 ldr)
+static inline bool
+apic_logical_id(struct kvm_apic_map *map, u32 ldr, u16 *cid, u16 *lid)
 {
-	u16 cid;
-	ldr >>= 32 - map->ldr_bits;
-	cid = (ldr >> map->cid_shift) & map->cid_mask;
-
-	return cid;
-}
-
-static inline u16 apic_logical_id(struct kvm_apic_map *map, u32 ldr)
-{
-	ldr >>= (32 - map->ldr_bits);
-	return ldr & map->lid_mask;
+	switch (map->mode) {
+	case KVM_APIC_MODE_XAPIC_FLAT:
+		*cid = 0;
+		*lid = ldr & 0xff;
+		return true;
+	case KVM_APIC_MODE_XAPIC_CLUSTER:
+		*cid = (ldr >> 4) & 0xf;
+		*lid = ldr & 0xf;
+		return true;
+	case KVM_APIC_MODE_X2APIC:
+		*cid = ldr >> 16;
+		*lid = ldr & 0xffff;
+		return true;
+	}
+	return false;
 }
 
 static inline bool kvm_apic_has_events(struct kvm_vcpu *vcpu)
-- 
2.2.2

Re: [PATCH 8/8] KVM: x86: simplify kvm_apic_map

[Paolo Bonzini]

On 29/01/2015 22:48, Radim Krčmář wrote:
> recalculate_apic_map() uses two passes over all VCPUs.  This is a relic
> from time when we selected a global mode in the first pass and set up
> the optimized table in the second pass (to have a consistent mode).
> 
> Recent changes made mixed mode unoptimized and we can do it in one pass.
> Format of logical MDA is a function of the mode, so we encode it in
> apic_logical_id() and drop obsoleted variables from the struct.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  3 --
>  arch/x86/kvm/lapic.c            | 78 ++++++++++-------------------------------
>  arch/x86/kvm/lapic.h            | 29 ++++++++-------
>  3 files changed, 36 insertions(+), 74 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index fec3188cabbb..14b6b0fd17b1 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -555,9 +555,6 @@ 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;
>  	struct kvm_lapic *phys_map[256];
>  	/* first index is cluster id second is cpu id in a cluster */
>  	struct kvm_lapic *logical_map[16][16];
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 621d9df6ac63..f74557791a77 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -146,49 +146,6 @@ static void recalculate_apic_map(struct kvm *kvm)
>  	if (!new)
>  		goto out;
>  
> -	new->ldr_bits = 8;
> -	/* flat mode is default */
> -	new->cid_shift = 8;
> -	new->cid_mask = 0;
> -	new->lid_mask = 0xff;
> -
> -	kvm_for_each_vcpu(i, vcpu, kvm) {
> -		struct kvm_lapic *apic = vcpu->arch.apic;
> -
> -		if (!kvm_apic_present(vcpu))
> -			continue;
> -
> -		if (apic_x2apic_mode(apic)) {
> -			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;
> -			}
> -		}
> -
> -		/*
> -		 * All APICs have to be configured in the same mode by an OS.
> -		 * We take advatage of this while building logical id loockup
> -		 * table. After reset APICs are in software disabled mode, so if
> -		 * we find apic with different setting we assume this is the mode
> -		 * OS wants all apics to be in; build lookup table accordingly.
> -		 */
> -		if (kvm_apic_sw_enabled(apic))
> -			break;
> -	}
> -
>  	kvm_for_each_vcpu(i, vcpu, kvm) {
>  		struct kvm_lapic *apic = vcpu->arch.apic;
>  		u16 cid, lid;
> @@ -198,17 +155,22 @@ static void recalculate_apic_map(struct kvm *kvm)
>  			continue;
>  
>  		aid = kvm_apic_id(apic);
> -		ldr = kvm_apic_get_reg(apic, APIC_LDR);
> -		cid = apic_cluster_id(new, ldr);
> -		lid = apic_logical_id(new, ldr);
> -
>  		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)
> +		ldr = kvm_apic_get_reg(apic, APIC_LDR);
> +
> +		if (apic_x2apic_mode(apic)) {
> +			new->mode |= KVM_APIC_MODE_X2APIC;
> +		} else if (ldr) {
> +			ldr = GET_APIC_LOGICAL_ID(ldr);
> +			if (kvm_apic_get_reg(apic, APIC_DFR) == APIC_DFR_FLAT)
> +				new->mode |= KVM_APIC_MODE_XAPIC_FLAT;
> +			else
> +				new->mode |= KVM_APIC_MODE_XAPIC_CLUSTER;
> +		}
> +
> +		if (!apic_logical_id(new, ldr, &cid, &lid))
>  			continue;
>  
>  		if (lid && cid < ARRAY_SIZE(new->logical_map))
> @@ -724,22 +686,20 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
>  
>  		dst = &map->phys_map[irq->dest_id];
>  	} else {
> -		u32 mda = irq->dest_id << (32 - map->ldr_bits);
> -		u16 cid = apic_cluster_id(map, mda);
> +		u16 cid;
>  
> -		if (cid >= ARRAY_SIZE(map->logical_map))
> -			goto out;
> -
> -		if (hweight8(map->mode) != 1) {
> +		if (!apic_logical_id(map, irq->dest_id, &cid, (u16 *)&bitmap))
> +		{
>  			/* Not deliverable with optimized map. */
>  			ret = false;
>  			goto out;
>  		}
>  
> +		if (cid >= ARRAY_SIZE(map->logical_map))
> +			goto out;
> +
>  		dst = map->logical_map[cid];
>  
> -		bitmap = apic_logical_id(map, mda);
> -
>  		if (irq->delivery_mode == APIC_DM_LOWEST) {
>  			int l = -1;
>  			for_each_set_bit(i, &bitmap, 16) {
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index fd0197a93862..320716e301a8 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -151,19 +151,24 @@ static inline bool kvm_apic_vid_enabled(struct kvm *kvm)
>  	return kvm_x86_ops->vm_has_apicv(kvm);
>  }
>  
> -static inline u16 apic_cluster_id(struct kvm_apic_map *map, u32 ldr)
> +static inline bool
> +apic_logical_id(struct kvm_apic_map *map, u32 ldr, u16 *cid, u16 *lid)
>  {
> -	u16 cid;
> -	ldr >>= 32 - map->ldr_bits;
> -	cid = (ldr >> map->cid_shift) & map->cid_mask;
> -
> -	return cid;
> -}
> -
> -static inline u16 apic_logical_id(struct kvm_apic_map *map, u32 ldr)
> -{
> -	ldr >>= (32 - map->ldr_bits);
> -	return ldr & map->lid_mask;
> +	switch (map->mode) {
> +	case KVM_APIC_MODE_XAPIC_FLAT:
> +		*cid = 0;
> +		*lid = ldr & 0xff;
> +		return true;
> +	case KVM_APIC_MODE_XAPIC_CLUSTER:
> +		*cid = (ldr >> 4) & 0xf;
> +		*lid = ldr & 0xf;
> +		return true;
> +	case KVM_APIC_MODE_X2APIC:
> +		*cid = ldr >> 16;
> +		*lid = ldr & 0xffff;
> +		return true;
> +	}

We need some optimization here.  You can make the defines equal to the
size of the lid: CLUSTER = 1 << 3, FLAT = 1 << 2, X2APIC = 1 << 4:

	BUILD_BUG_ON(...FLAT != 4);
	BUILD_BUG_ON(...CLUSTER != 8);
	BUILD_BUG_ON(...X2APIC != 16);

	lid_bits = mode;
	cid_bits = mode & (16 | 4);
	lid_mask = (1 << lid_bits) - 1;
	cid_mask = (1 << cid_bits) - 1;

	*cid = (ldr >> lid_bits) & cid_mask;
	*lid = ldr & lid_mask;

Please move this to lapic.c since you are at it.

Paolo

> +	return false;
>  }
>  
>  static inline bool kvm_apic_has_events(struct kvm_vcpu *vcpu)
> 

Re: [PATCH 8/8] KVM: x86: simplify kvm_apic_map

[Radim Krčmář]

2015-01-30 10:18+0100, Paolo Bonzini:
> On 29/01/2015 22:48, Radim Krčmář wrote:
> > +static inline bool
> > +apic_logical_id(struct kvm_apic_map *map, u32 ldr, u16 *cid, u16 *lid)
> >  {
> > +	switch (map->mode) {
> > +	case KVM_APIC_MODE_XAPIC_FLAT:
> > +		*cid = 0;
> > +		*lid = ldr & 0xff;
> > +		return true;
> > +	case KVM_APIC_MODE_XAPIC_CLUSTER:
> > +		*cid = (ldr >> 4) & 0xf;
> > +		*lid = ldr & 0xf;
> > +		return true;
> > +	case KVM_APIC_MODE_X2APIC:
> > +		*cid = ldr >> 16;
> > +		*lid = ldr & 0xffff;
> > +		return true;
> > +	}
> 
> We need some optimization here.  You can make the defines equal to the
> size of the lid: CLUSTER = 1 << 3, FLAT = 1 << 2, X2APIC = 1 << 4:
> 
> 	BUILD_BUG_ON(...FLAT != 4);
> 	BUILD_BUG_ON(...CLUSTER != 8);

(Swapped.)

> 	BUILD_BUG_ON(...X2APIC != 16);

(Check the mode and return false here.)

> 	lid_bits = mode;
> 	cid_bits = mode & (16 | 4);
> 	lid_mask = (1 << lid_bits) - 1;
> 	cid_mask = (1 << cid_bits) - 1;
> 
> 	*cid = (ldr >> lid_bits) & cid_mask;
> 	*lid = ldr & lid_mask;

Would jump predictor fail on the switch?  Or is size of the code that
important?  This code is shorter, but is going to execute far more
operations, so I think it would be slower ... (And harder to read.)

> Please move this to lapic.c since you are at it.

Ok.

Re: [PATCH 8/8] KVM: x86: simplify kvm_apic_map

[Paolo Bonzini]

On 30/01/2015 16:14, Radim Krčmář wrote:
> > > +	case KVM_APIC_MODE_XAPIC_FLAT:
> > > +		*cid = 0;
> > > +		*lid = ldr & 0xff;
> > > +		return true;
> > > +	case KVM_APIC_MODE_XAPIC_CLUSTER:
> > > +		*cid = (ldr >> 4) & 0xf;
> > > +		*lid = ldr & 0xf;
> > > +		return true;
> > > +	case KVM_APIC_MODE_X2APIC:
> > > +		*cid = ldr >> 16;
> > > +		*lid = ldr & 0xffff;
> > > +		return true;
> > > +	}
> 
>> > 	lid_bits = mode;
>> > 	cid_bits = mode & (16 | 4);
>> > 	lid_mask = (1 << lid_bits) - 1;
>> > 	cid_mask = (1 << cid_bits) - 1;
>> > 
>> > 	*cid = (ldr >> lid_bits) & cid_mask;
>> > 	*lid = ldr & lid_mask;
> Would jump predictor fail on the switch?  Or is size of the code that
> important?  This code is shorter, but is going to execute far more
> operations, so I think it would be slower ... (And harder to read.)

Considering the additional comparisons for the switch, I don't think
it's going to execute far more operations...

Paolo

Re: [PATCH 8/8] KVM: x86: simplify kvm_apic_map

[Radim Krčmář]

2015-01-30 16:23+0100, Paolo Bonzini:
> On 30/01/2015 16:14, Radim Krčmář wrote:
> > > > +	case KVM_APIC_MODE_XAPIC_FLAT:
> > > > +		*cid = 0;
> > > > +		*lid = ldr & 0xff;
> > > > +		return true;
> > > > +	case KVM_APIC_MODE_XAPIC_CLUSTER:
> > > > +		*cid = (ldr >> 4) & 0xf;
> > > > +		*lid = ldr & 0xf;
> > > > +		return true;
> > > > +	case KVM_APIC_MODE_X2APIC:
> > > > +		*cid = ldr >> 16;
> > > > +		*lid = ldr & 0xffff;
> > > > +		return true;
> > > > +	}
> > 
> >> > 	lid_bits = mode;
> >> > 	cid_bits = mode & (16 | 4);
> >> > 	lid_mask = (1 << lid_bits) - 1;
> >> > 	cid_mask = (1 << cid_bits) - 1;
> >> > 
> >> > 	*cid = (ldr >> lid_bits) & cid_mask;
> >> > 	*lid = ldr & lid_mask;
> > Would jump predictor fail on the switch?  Or is size of the code that
> > important?  This code is shorter, but is going to execute far more
> > operations, so I think it would be slower ... (And harder to read.)
> 
> Considering the additional comparisons for the switch, I don't think
> it's going to execute far more operations...

(A quick assembly comparison [1].)

As optimizations go, we could drop the "&" on cid as "cid < 16" is
checked later, so mode=4 practically does nothing ... Not the best for
future bugs, but still pretty safe -- only x2APIC can set a value that
would require the "&" and it can't have valid XAPIC mode, so u32 still
protects us enough.

  *cid = ldr >> mode;
  *lid = ldr & ((1 << mode) - 1);

Which is probably faster a solution where we don't shuffle switch cases
to jump to x2APIC first.  A comparison is at the very bottom [2].

Would that be ok in v2?


---
1:  To make it easier, it wasn't inlined, sorry.

    There are four parts, first one compares the whole functions and
    three subsequent compare each switch case, with common head/tail
    omitted.  (We don't care about performance when the map is invalid.)

    The optimized version is always first and next to it is the old one.

0000000000001700 <apic_logical_id>:  (ALI from now on)
  1700:	callq  1705 <ALI+0x5>     1700:	callq  1705 <ALI+0x5>
  1705:	push   %rbp               1705:	movzbl 0x10(%rdi),%eax
  1706:	movzbl 0x10(%rdi),%r8d    1709:	push   %rbp
  170b:	mov    %rcx,%r10          170a:	mov    %rsp,%rbp
  170e:	xor    %eax,%eax          170d:	cmp    $0x8,%al
  1710:	mov    %rsp,%rbp          170f:	je     1758 <ALI+0x58>
  1713:	lea    -0x1(%r8),%ecx     1711:	cmp    $0x10,%al
  1717:	test   %r8d,%ecx          1713:	je     1740 <ALI+0x40>
  171a:	jne    1752 <ALI+0x52>    1715:	cmp    $0x4,%al
  171c:	mov    %r8d,%edi          1717:	je     1720 <ALI+0x20>
  171f:	mov    $0x1,%eax          1719:	xor    %eax,%eax
  1724:	mov    %esi,%r8d          171b:	pop    %rbp
  1727:	mov    %edi,%ecx          171c:	retq   
  1729:	mov    %eax,%r9d          171d:	nopl   (%rax)
  172c:	shr    %cl,%r8d           1720:	mov    %esi,%eax
  172f:	and    $0x14,%ecx         1722:	and    $0xf,%esi
  1732:	shl    %cl,%r9d           1725:	shr    $0x4,%eax
  1735:	mov    %edi,%ecx          1728:	and    $0xf,%eax
  1737:	shl    %cl,%eax           172b:	mov    %ax,(%rdx)
  1739:	sub    $0x1,%r9d          172e:	mov    $0x1,%eax
  173d:	sub    $0x1,%eax          1733:	mov    %si,(%rcx)
  1740:	and    %r9d,%r8d          1736:	pop    %rbp
  1743:	and    %esi,%eax          1737:	retq   
  1745:	mov    %r8w,(%rdx)        1738:	nopl   0x0(%rax,%rax,1)
  1749:	mov    %ax,(%r10)         173f:	
  174d:	mov    $0x1,%eax          1740:	mov    %esi,%eax
  1752:	pop    %rbp               1742:	shr    $0x10,%eax
  1753:	retq                      1745:	mov    %ax,(%rdx)
       	                          1748:	mov    $0x1,%eax
       	                          174d:	mov    %si,(%rcx)
       	                          1750:	pop    %rbp
       	                          1751:	retq   
       	                          1752:	nopw   0x0(%rax,%rax,1)
       	                          1758:	xor    %eax,%eax
       	                          175a:	and    $0xff,%si
       	                          175f:	mov    %ax,(%rdx)
       	                          1762:	mov    $0x1,%eax
       	                          1767:	mov    %si,(%rcx)
       	                          176a:	pop    %rbp
       	                          176b:	retq   



  170b:	mov    %rcx,%r10          170a:	mov    %rsp,%rbp
  170e:	xor    %eax,%eax          170d:	cmp    $0x8,%al
  1710:	mov    %rsp,%rbp          170f:	je     1758 <ALI+0x58>
  1713:	lea    -0x1(%r8),%ecx         [jump]
  1717:	test   %r8d,%ecx          1758:	xor    %eax,%eax
  171a:	jne    1752 <ALI+0x52>    175a:	and    $0xff,%si
  171c:	mov    %r8d,%edi          175f:	mov    %ax,(%rdx)
  171f:	mov    $0x1,%eax          1762:	mov    $0x1,%eax
  1724:	mov    %esi,%r8d          1767:	mov    %si,(%rcx)
  1727:	mov    %edi,%ecx          
  1729:	mov    %eax,%r9d          
  172c:	shr    %cl,%r8d           
  172f:	and    $0x14,%ecx         
  1732:	shl    %cl,%r9d           
  1735:	mov    %edi,%ecx          
  1737:	shl    %cl,%eax           
  1739:	sub    $0x1,%r9d          
  173d:	sub    $0x1,%eax          
  1740:	and    %r9d,%r8d          
  1743:	and    %esi,%eax          
  1745:	mov    %r8w,(%rdx)        
  1749:	mov    %ax,(%r10)         
  174d:	mov    $0x1,%eax          


  170b:	mov    %rcx,%r10          170a:	mov    %rsp,%rbp
  170e:	xor    %eax,%eax          170d:	cmp    $0x8,%al
  1710:	mov    %rsp,%rbp          170f:	je     1758 <ALI+0x58>
  1713:	lea    -0x1(%r8),%ecx     1711:	cmp    $0x10,%al
  1717:	test   %r8d,%ecx          1713:	je     1740 <ALI+0x40>
  171a:	jne    1752 <ALI+0x52>        [jump]
  171c:	mov    %r8d,%edi          1740:	mov    %esi,%eax
  171f:	mov    $0x1,%eax          1742:	shr    $0x10,%eax
  1724:	mov    %esi,%r8d          1745:	mov    %ax,(%rdx)
  1727:	mov    %edi,%ecx          1748:	mov    $0x1,%eax
  1729:	mov    %eax,%r9d          174d:	mov    %si,(%rcx)
  172c:	shr    %cl,%r8d           
  172f:	and    $0x14,%ecx         
  1732:	shl    %cl,%r9d           
  1735:	mov    %edi,%ecx          
  1737:	shl    %cl,%eax           
  1739:	sub    $0x1,%r9d          
  173d:	sub    $0x1,%eax          
  1740:	and    %r9d,%r8d          
  1743:	and    %esi,%eax          
  1745:	mov    %r8w,(%rdx)        
  1749:	mov    %ax,(%r10)         
  174d:	mov    $0x1,%eax          


  170b:	mov    %rcx,%r10          170a:	mov    %rsp,%rbp        
  170e:	xor    %eax,%eax          170d:	cmp    $0x8,%al
  1710:	mov    %rsp,%rbp          170f:	je     1758 <ALI+0x58>
  1713:	lea    -0x1(%r8),%ecx     1711:	cmp    $0x10,%al
  1717:	test   %r8d,%ecx          1713:	je     1740 <ALI+0x40>
  171a:	jne    1752 <ALI+0x52>    1715:	cmp    $0x4,%al
  171c:	mov    %r8d,%edi          1717:	je     1720 <ALI+0x20>
  171f:	mov    $0x1,%eax              [jump]
  1724:	mov    %esi,%r8d          1720:	mov    %esi,%eax
  1727:	mov    %edi,%ecx          1722:	and    $0xf,%esi
  1729:	mov    %eax,%r9d          1725:	shr    $0x4,%eax
  172c:	shr    %cl,%r8d           1728:	and    $0xf,%eax
  172f:	and    $0x14,%ecx         172b:	mov    %ax,(%rdx)
  1732:	shl    %cl,%r9d           172e:	mov    $0x1,%eax
  1735:	mov    %edi,%ecx          1733:	mov    %si,(%rcx)
  1737:	shl    %cl,%eax           
  1739:	sub    $0x1,%r9d          
  173d:	sub    $0x1,%eax          
  1740:	and    %r9d,%r8d          
  1743:	and    %esi,%eax          
  1745:	mov    %r8w,(%rdx)        
  1749:	mov    %ax,(%r10)         
  174d:	mov    $0x1,%eax          


2: A comparison of aggressively optimized ALI with the worst case
   scenario, where x2APIC is the third jump.

  16e5:	mov    %rcx,%r9           170a:	mov    %rsp,%rbp        
  16ed:	xor    %eax,%eax          170d:	cmp    $0x8,%al
  16ef:	mov    %rsp,%rbp          170f:	je     1758 <ALI+0x58>
  16f2:	lea    -0x1(%rcx),%r8d    1711:	cmp    $0x10,%al
  16f6:	test   %ecx,%r8d          1713:	je     1740 <ALI+0x40>
  16f9:	jne    1717 <ALI+0x37>    1715:	cmp    $0x4,%al
  16fb:	mov    %esi,%eax          1717:	je     1720 <ALI+0x20>
  16fd:	shr    %cl,%eax               [jump]
  16ff:	mov    %ax,(%rdx)         1720:	mov    %esi,%eax
  1702:	mov    $0x1,%eax          1722:	and    $0xf,%esi
  1707:	shl    %cl,%eax           1725:	shr    $0x4,%eax
  1709:	sub    $0x1,%eax          1728:	and    $0xf,%eax
  170c:	and    %esi,%eax          172b:	mov    %ax,(%rdx)
  170e:	mov    %ax,(%r9)          172e:	mov    $0x1,%eax
  1712:	mov    $0x1,%eax          1733:	mov    %si,(%rcx)

Re: [PATCH 8/8] KVM: x86: simplify kvm_apic_map

[Paolo Bonzini]

On 30/01/2015 17:57, Radim Krčmář wrote:
> As optimizations go, we could drop the "&" on cid as "cid < 16" is
> checked later, so mode=4 practically does nothing ... Not the best for
> future bugs, but still pretty safe -- only x2APIC can set a value that
> would require the "&" and it can't have valid XAPIC mode, so u32 still
> protects us enough.
> 
>   *cid = ldr >> mode;
>   *lid = ldr & ((1 << mode) - 1);
> 
> Which is probably faster a solution where we don't shuffle switch cases
> to jump to x2APIC first.  A comparison is at the very bottom [2].
> 
> Would that be ok in v2?

Yes, this is okay.  Thanks for looking at it!

Paolo

Re: [PATCH 0/8] KVM: minor APIC fixes and cleanups

[Paolo Bonzini]

On 29/01/2015 22:48, Radim Krčmář wrote:
> This patch series is made our of three logical parts,
> 
> [1-3/8] are just a cleanup and could be omitted
> [4-6/8] improve broadcast detection and unoptimized delivery
> [7-8/8] handle mixed mode (by falling back to improvements from [5-6/9])

I applied patches 1-4 and commented on patch 5.

I'm afraid that patches 7-8 could lead to missed optimizations in some cases.

Regarding patch 6, perhaps there's a way to avoid this:

+	/* XXX: A superset of x2APIC broadcasts is fine in practice as long as
+	 * we don't support APIC ID > 0xfeffffff.
+	 */

It's ugly. :)

Paolo

> 
> Radim Krčmář (8):
>   KVM: x86: return bool from kvm_apic_match*()
>   KVM: x86: cleanup kvm_apic_match_*()
>   KVM: x86: replace 0 with APIC_DEST_PHYSICAL
>   KVM: x86: fix x2apic logical address matching
>   KVM: x86: use MDA for interrupt matching
>   KVM: x86: allow mixed APIC mode broadcast
>   KVM: x86: avoid logical_map when it is invalid
>   KVM: x86: simplify kvm_apic_map
> 
>  arch/x86/include/asm/kvm_host.h |   4 +-
>  arch/x86/kvm/ioapic.h           |   2 +-
>  arch/x86/kvm/lapic.c            | 176 +++++++++++++++++++---------------------
>  arch/x86/kvm/lapic.h            |  35 ++++----
>  4 files changed, 106 insertions(+), 111 deletions(-)
> 

Re: [PATCH 0/8] KVM: minor APIC fixes and cleanups

[Radim Krčmář]

2015-01-30 10:22+0100, Paolo Bonzini:
> On 29/01/2015 22:48, Radim Krčmář wrote:
> > This patch series is made our of three logical parts,
> > 
> > [1-3/8] are just a cleanup and could be omitted
> > [4-6/8] improve broadcast detection and unoptimized delivery
> > [7-8/8] handle mixed mode (by falling back to improvements from [5-6/9])
> 
> I applied patches 1-4 and commented on patch 5.
> 
> I'm afraid that patches 7-8 could lead to missed optimizations in some cases.

True, I'll do more research on complexity of its assembly.

> Regarding patch 6, perhaps there's a way to avoid this:
> 
> +	/* XXX: A superset of x2APIC broadcasts is fine in practice as long as
> +	 * we don't support APIC ID > 0xfeffffff.
> +	 */
> 
> It's ugly. :)

Yeah, there is: don't deliver x2APIC broadcasts to xAPIC.
(I'm not even sure if it is correct ...)

Without that delivery, we could do something like
  if (irq->dest_id == (x2apic_ipi ? X2APIC_BROADCAST : APIC_BROADCAST))

Re: [PATCH 0/8] KVM: minor APIC fixes and cleanups

[Paolo Bonzini]

On 30/01/2015 16:20, Radim Krčmář wrote:
>> > +	/* XXX: A superset of x2APIC broadcasts is fine in practice as long as
>> > +	 * we don't support APIC ID > 0xfeffffff.
>> > +	 */
>> > 
>> > It's ugly. :)
> Yeah, there is: don't deliver x2APIC broadcasts to xAPIC.
> (I'm not even sure if it is correct ...)
> 
> Without that delivery, we could do something like
>   if (irq->dest_id == (x2apic_ipi ? X2APIC_BROADCAST : APIC_BROADCAST))

Nadav, do you know what real hardware does?

Paolo