[PATCH v2 0/4] KVM: x86: allow hotplug of VCPU with APIC ID over 0xff

[PATCH v2 0/4] KVM: x86: allow hotplug of VCPU with APIC ID over 0xff

[Radim Krčmář]

v2 removes the capability that let userspace know of changes.

v1: http://www.spinics.net/lists/kvm/msg141944.html
> The problem is described in [4/4].
> 
> [1/4] is a prerequisite to allow a cleanup in [2/4].
> [2/4] hopefully makes [4/4] easier to understand.
> [3/4] fixes mistakes from dealing with the mixed mode.
> [4/4] allows the hotplug and and add a capability for it.


Radim Krčmář (4):
  KVM: x86: use delivery to self in hyperv synic
  KVM: x86: replace kvm_apic_id with kvm_{x,x2}apic_id
  KVM: x86: make interrupt delivery fast and slow path behave the same
  KVM: x86: allow hotplug of VCPU with APIC ID over 0xff

 arch/x86/kvm/hyperv.c |  4 ++--
 arch/x86/kvm/lapic.c  | 62 ++++++++++++++++++++++++++++++++++++---------------
 arch/x86/kvm/lapic.h  | 11 ---------
 3 files changed, 46 insertions(+), 31 deletions(-)

-- 
2.11.0

[PATCH v2 1/4] KVM: x86: use delivery to self in hyperv synic

[Radim Krčmář]

Interrupt to self can be sent without knowing the APIC ID.

Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: r-b David

 arch/x86/kvm/hyperv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 99cde5220e07..313957ec9a9d 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -305,13 +305,13 @@ static int synic_set_irq(struct kvm_vcpu_hv_synic *synic, u32 sint)
 		return -ENOENT;
 
 	memset(&irq, 0, sizeof(irq));
-	irq.dest_id = kvm_apic_id(vcpu->arch.apic);
+	irq.shorthand = APIC_DEST_SELF;
 	irq.dest_mode = APIC_DEST_PHYSICAL;
 	irq.delivery_mode = APIC_DM_FIXED;
 	irq.vector = vector;
 	irq.level = 1;
 
-	ret = kvm_irq_delivery_to_apic(vcpu->kvm, NULL, &irq, NULL);
+	ret = kvm_irq_delivery_to_apic(vcpu->kvm, vcpu->arch.apic, &irq, NULL);
 	trace_kvm_hv_synic_set_irq(vcpu->vcpu_id, sint, irq.vector, ret);
 	return ret;
 }
-- 
2.11.0

[PATCH v2 2/4] KVM: x86: replace kvm_apic_id with kvm_{x,x2}apic_id

[Radim Krčmář]

There were three calls sites:
 - recalculate_apic_map and kvm_apic_match_physical_addr, where it would
   only complicate implementation of x2APIC hotplug;
 - in apic_debug, where it was still somewhat preserved, but keeping the
   old function just for apic_debug was not worth it

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/lapic.c | 41 ++++++++++++++++++++++++++++++-----------
 arch/x86/kvm/lapic.h | 11 -----------
 2 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 09edd32b8e42..e645b4bc6437 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -115,6 +115,16 @@ static inline int apic_enabled(struct kvm_lapic *apic)
 	(LVT_MASK | APIC_MODE_MASK | APIC_INPUT_POLARITY | \
 	 APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER)
 
+static inline u8 kvm_xapic_id(struct kvm_lapic *apic)
+{
+	return kvm_lapic_get_reg(apic, APIC_ID) >> 24;
+}
+
+static inline u32 kvm_x2apic_id(struct kvm_lapic *apic)
+{
+	return apic->vcpu->vcpu_id;
+}
+
 static inline bool kvm_apic_map_get_logical_dest(struct kvm_apic_map *map,
 		u32 dest_id, struct kvm_lapic ***cluster, u16 *mask) {
 	switch (map->mode) {
@@ -159,13 +169,13 @@ static void recalculate_apic_map(struct kvm *kvm)
 	struct kvm_apic_map *new, *old = NULL;
 	struct kvm_vcpu *vcpu;
 	int i;
-	u32 max_id = 255;
+	u32 max_id = 256; /* enough space for any xAPIC ID */
 
 	mutex_lock(&kvm->arch.apic_map_lock);
 
 	kvm_for_each_vcpu(i, vcpu, kvm)
 		if (kvm_apic_present(vcpu))
-			max_id = max(max_id, kvm_apic_id(vcpu->arch.apic));
+			max_id = max(max_id, kvm_x2apic_id(vcpu->arch.apic));
 
 	new = kvm_kvzalloc(sizeof(struct kvm_apic_map) +
 	                   sizeof(struct kvm_lapic *) * ((u64)max_id + 1));
@@ -179,16 +189,23 @@ static void recalculate_apic_map(struct kvm *kvm)
 		struct kvm_lapic *apic = vcpu->arch.apic;
 		struct kvm_lapic **cluster;
 		u16 mask;
-		u32 ldr, aid;
+		u32 ldr;
+		u8 xapic_id;
+		u32 x2apic_id;
 
 		if (!kvm_apic_present(vcpu))
 			continue;
 
-		aid = kvm_apic_id(apic);
-		ldr = kvm_lapic_get_reg(apic, APIC_LDR);
+		xapic_id = kvm_xapic_id(apic);
+		x2apic_id = kvm_x2apic_id(apic);
 
-		if (aid <= new->max_apic_id)
-			new->phys_map[aid] = apic;
+		if (apic_x2apic_mode(apic) &&
+				x2apic_id <= new->max_apic_id)
+			new->phys_map[x2apic_id] = apic;
+		else if (!apic_x2apic_mode(apic))
+			new->phys_map[xapic_id] = apic;
+
+		ldr = kvm_lapic_get_reg(apic, APIC_LDR);
 
 		if (apic_x2apic_mode(apic)) {
 			new->mode |= KVM_APIC_MODE_X2APIC;
@@ -250,6 +267,8 @@ static inline void kvm_apic_set_x2apic_id(struct kvm_lapic *apic, u32 id)
 {
 	u32 ldr = ((id >> 4) << 16) | (1 << (id & 0xf));
 
+	WARN_ON_ONCE(id != apic->vcpu->vcpu_id);
+
 	kvm_lapic_set_reg(apic, APIC_ID, id);
 	kvm_lapic_set_reg(apic, APIC_LDR, ldr);
 	recalculate_apic_map(apic->vcpu->kvm);
@@ -591,9 +610,9 @@ static bool kvm_apic_match_physical_addr(struct kvm_lapic *apic, u32 mda)
 		return true;
 
 	if (apic_x2apic_mode(apic))
-		return mda == kvm_apic_id(apic);
+		return mda == kvm_x2apic_id(apic);
 
-	return mda == SET_APIC_DEST_FIELD(kvm_apic_id(apic));
+	return mda == SET_APIC_DEST_FIELD(kvm_xapic_id(apic));
 }
 
 static bool kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
@@ -1907,9 +1926,9 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event)
 	vcpu->arch.apic_arb_prio = 0;
 	vcpu->arch.apic_attention = 0;
 
-	apic_debug("%s: vcpu=%p, id=%d, base_msr="
+	apic_debug("%s: vcpu=%p, id=0x%x, base_msr="
 		   "0x%016" PRIx64 ", base_address=0x%0lx.\n", __func__,
-		   vcpu, kvm_apic_id(apic),
+		   vcpu, kvm_lapic_get_reg(apic, APIC_ID),
 		   vcpu->arch.apic_base, apic->base_address);
 }
 
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index e0c80233b3e1..cb16e6fd2330 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -202,17 +202,6 @@ static inline int kvm_lapic_latched_init(struct kvm_vcpu *vcpu)
 	return lapic_in_kernel(vcpu) && test_bit(KVM_APIC_INIT, &vcpu->arch.apic->pending_events);
 }
 
-static inline u32 kvm_apic_id(struct kvm_lapic *apic)
-{
-	/* To avoid a race between apic_base and following APIC_ID update when
-	 * switching to x2apic_mode, the x2apic mode returns initial x2apic id.
-	 */
-	if (apic_x2apic_mode(apic))
-		return apic->vcpu->vcpu_id;
-
-	return kvm_lapic_get_reg(apic, APIC_ID) >> 24;
-}
-
 bool kvm_apic_pending_eoi(struct kvm_vcpu *vcpu, int vector);
 
 void wait_lapic_expire(struct kvm_vcpu *vcpu);
-- 
2.11.0

[PATCH v2 3/4] KVM: x86: make interrupt delivery fast and slow path behave the same

[Radim Krčmář]

Slow path tried to prevent IPIs from x2APIC VCPUs from being delivered
to xAPIC VCPUs and vice-versa.  Make slow path behave like fast path,
which never distinguished that.

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

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index e645b4bc6437..bc8df7ce2766 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -612,7 +612,7 @@ static bool kvm_apic_match_physical_addr(struct kvm_lapic *apic, u32 mda)
 	if (apic_x2apic_mode(apic))
 		return mda == kvm_x2apic_id(apic);
 
-	return mda == SET_APIC_DEST_FIELD(kvm_xapic_id(apic));
+	return mda == kvm_xapic_id(apic);
 }
 
 static bool kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
@@ -629,7 +629,6 @@ static bool kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
 		       && (logical_id & mda & 0xffff) != 0;
 
 	logical_id = GET_APIC_LOGICAL_ID(logical_id);
-	mda = GET_APIC_DEST_FIELD(mda);
 
 	switch (kvm_lapic_get_reg(apic, APIC_DFR)) {
 	case APIC_DFR_FLAT:
@@ -646,9 +645,9 @@ static bool kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
 
 /* The KVM local APIC implementation has two quirks:
  *
- *  - the xAPIC MDA stores the destination at bits 24-31, while this
- *    is not true of struct kvm_lapic_irq's dest_id field.  This is
- *    just a quirk in the API and is not problematic.
+ *  - Real hardware delivers interrupts destined to x2APIC ID > 0xff to LAPICs
+ *    in xAPIC mode if the "destination & 0xff" matches its xAPIC ID.
+ *    KVM doesn't do that aliasing.
  *
  *  - in-kernel IOAPIC messages have to be delivered directly to
  *    x2APIC, because the kernel does not support interrupt remapping.
@@ -664,13 +663,12 @@ static u32 kvm_apic_mda(struct kvm_vcpu *vcpu, unsigned int dest_id,
 		struct kvm_lapic *source, struct kvm_lapic *target)
 {
 	bool ipi = source != NULL;
-	bool x2apic_mda = apic_x2apic_mode(ipi ? source : target);
 
 	if (!vcpu->kvm->arch.x2apic_broadcast_quirk_disabled &&
-	    !ipi && dest_id == APIC_BROADCAST && x2apic_mda)
+	    !ipi && dest_id == APIC_BROADCAST && apic_x2apic_mode(target))
 		return X2APIC_BROADCAST;
 
-	return x2apic_mda ? dest_id : SET_APIC_DEST_FIELD(dest_id);
+	return dest_id;
 }
 
 bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
-- 
2.11.0

[PATCH v2 4/4] KVM: x86: allow hotplug of VCPU with APIC ID over 0xff

[Radim Krčmář]

LAPIC after reset is in xAPIC mode, which poses a problem for hotplug of
VCPUs with high APIC ID, because reset VCPU is waiting for INIT/SIPI,
but there is no way to uniquely address it using xAPIC.

>From many possible options, we chose the one that also works on real
hardware: accepting interrupts addressed to LAPIC's x2APIC ID even in
xAPIC mode.

KVM intentionally differs from real hardware, because real hardware
(Knights Landing) does just "x2apic_id & 0xff" to decide whether to
accept the interrupt in xAPIC mode and it can deliver one interrupt to
more than one physical destination, e.g. 0x123 to 0x123 and 0x23.

Fixes: 682f732ecf73 ("KVM: x86: bump MAX_VCPUS to 288")
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2:
 * Do not add capability [David, Paolo]
 * Tag it as a fix [David]
---
 arch/x86/kvm/lapic.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index bc8df7ce2766..f1b22bb76a4a 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -199,10 +199,15 @@ static void recalculate_apic_map(struct kvm *kvm)
 		xapic_id = kvm_xapic_id(apic);
 		x2apic_id = kvm_x2apic_id(apic);
 
-		if (apic_x2apic_mode(apic) &&
+		/* Hotplug hack: see kvm_apic_match_physical_addr(), ... */
+		if ((apic_x2apic_mode(apic) || x2apic_id > 0xff) &&
 				x2apic_id <= new->max_apic_id)
 			new->phys_map[x2apic_id] = apic;
-		else if (!apic_x2apic_mode(apic))
+		/*
+		 * ... xAPIC ID of VCPUs with APIC ID > 0xff will wrap-around,
+		 * prevent them from masking VCPUs with APIC ID <= 0xff.
+		 */
+		if (!apic_x2apic_mode(apic) && !new->phys_map[xapic_id])
 			new->phys_map[xapic_id] = apic;
 
 		ldr = kvm_lapic_get_reg(apic, APIC_LDR);
@@ -612,6 +617,10 @@ static bool kvm_apic_match_physical_addr(struct kvm_lapic *apic, u32 mda)
 	if (apic_x2apic_mode(apic))
 		return mda == kvm_x2apic_id(apic);
 
+	/* Hotplug hack: LAPIC in xAPIC mode also accepts x2APIC. */
+	if (kvm_x2apic_id(apic) > 0xff && mda == kvm_x2apic_id(apic))
+		return true;
+
 	return mda == kvm_xapic_id(apic);
 }
 
-- 
2.11.0

Re: [PATCH v2 2/4] KVM: x86: replace kvm_apic_id with kvm_{x,x2}apic_id

[David Hildenbrand]

Am 13.12.2016 um 17:29 schrieb Radim Krčmář:
> There were three calls sites:
>  - recalculate_apic_map and kvm_apic_match_physical_addr, where it would
>    only complicate implementation of x2APIC hotplug;
>  - in apic_debug, where it was still somewhat preserved, but keeping the
>    old function just for apic_debug was not worth it
>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arch/x86/kvm/lapic.c | 41 ++++++++++++++++++++++++++++++-----------
>  arch/x86/kvm/lapic.h | 11 -----------
>  2 files changed, 30 insertions(+), 22 deletions(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 09edd32b8e42..e645b4bc6437 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -115,6 +115,16 @@ static inline int apic_enabled(struct kvm_lapic *apic)
>  	(LVT_MASK | APIC_MODE_MASK | APIC_INPUT_POLARITY | \
>  	 APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER)
>
> +static inline u8 kvm_xapic_id(struct kvm_lapic *apic)
> +{
> +	return kvm_lapic_get_reg(apic, APIC_ID) >> 24;
> +}
> +
> +static inline u32 kvm_x2apic_id(struct kvm_lapic *apic)
> +{
> +	return apic->vcpu->vcpu_id;
> +}
> +
>  static inline bool kvm_apic_map_get_logical_dest(struct kvm_apic_map *map,
>  		u32 dest_id, struct kvm_lapic ***cluster, u16 *mask) {
>  	switch (map->mode) {
> @@ -159,13 +169,13 @@ static void recalculate_apic_map(struct kvm *kvm)
>  	struct kvm_apic_map *new, *old = NULL;
>  	struct kvm_vcpu *vcpu;
>  	int i;
> -	u32 max_id = 255;
> +	u32 max_id = 256; /* enough space for any xAPIC ID */

Why exactly do we have to increase this?

I thought the maximum xapic id is 255, and we correctly allocate
255+1 slots.

Now, we would allocate 257 slots and set max_apic_id to 256.

Can you elaborate?


-- 

David

Re: [PATCH v2 2/4] KVM: x86: replace kvm_apic_id with kvm_{x,x2}apic_id

[Radim Krčmář]

2016-12-14 13:39+0100, David Hildenbrand:
> Am 13.12.2016 um 17:29 schrieb Radim Krčmář:
>> There were three calls sites:
>>  - recalculate_apic_map and kvm_apic_match_physical_addr, where it would
>>    only complicate implementation of x2APIC hotplug;
>>  - in apic_debug, where it was still somewhat preserved, but keeping the
>>    old function just for apic_debug was not worth it
>> 
>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>> ---
>>  arch/x86/kvm/lapic.c | 41 ++++++++++++++++++++++++++++++-----------
>>  arch/x86/kvm/lapic.h | 11 -----------
>>  2 files changed, 30 insertions(+), 22 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 09edd32b8e42..e645b4bc6437 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -115,6 +115,16 @@ static inline int apic_enabled(struct kvm_lapic *apic)
>>  	(LVT_MASK | APIC_MODE_MASK | APIC_INPUT_POLARITY | \
>>  	 APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER)
>> 
>> +static inline u8 kvm_xapic_id(struct kvm_lapic *apic)
>> +{
>> +	return kvm_lapic_get_reg(apic, APIC_ID) >> 24;
>> +}
>> +
>> +static inline u32 kvm_x2apic_id(struct kvm_lapic *apic)
>> +{
>> +	return apic->vcpu->vcpu_id;
>> +}
>> +
>>  static inline bool kvm_apic_map_get_logical_dest(struct kvm_apic_map *map,
>>  		u32 dest_id, struct kvm_lapic ***cluster, u16 *mask) {
>>  	switch (map->mode) {
>> @@ -159,13 +169,13 @@ static void recalculate_apic_map(struct kvm *kvm)
>>  	struct kvm_apic_map *new, *old = NULL;
>>  	struct kvm_vcpu *vcpu;
>>  	int i;
>> -	u32 max_id = 255;
>> +	u32 max_id = 256; /* enough space for any xAPIC ID */
> 
> Why exactly do we have to increase this?
> 
> I thought the maximum xapic id is 255, and we correctly allocate
> 255+1 slots.
> 
> Now, we would allocate 257 slots and set max_apic_id to 256.

Right, it is a typical off-by-one error.  The comment is sufficient ...

Queued for v3,
Thanks.

Re: [PATCH v2 2/4] KVM: x86: replace kvm_apic_id with kvm_{x,x2}apic_id

[Paolo Bonzini]

On 14/12/2016 14:41, Radim Krčmář wrote:
> 2016-12-14 13:39+0100, David Hildenbrand:
>> Am 13.12.2016 um 17:29 schrieb Radim Krčmář:
>>> There were three calls sites:
>>>  - recalculate_apic_map and kvm_apic_match_physical_addr, where it would
>>>    only complicate implementation of x2APIC hotplug;
>>>  - in apic_debug, where it was still somewhat preserved, but keeping the
>>>    old function just for apic_debug was not worth it
>>>
>>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>>> ---
>>>  arch/x86/kvm/lapic.c | 41 ++++++++++++++++++++++++++++++-----------
>>>  arch/x86/kvm/lapic.h | 11 -----------
>>>  2 files changed, 30 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>>> index 09edd32b8e42..e645b4bc6437 100644
>>> --- a/arch/x86/kvm/lapic.c
>>> +++ b/arch/x86/kvm/lapic.c
>>> @@ -115,6 +115,16 @@ static inline int apic_enabled(struct kvm_lapic *apic)
>>>  	(LVT_MASK | APIC_MODE_MASK | APIC_INPUT_POLARITY | \
>>>  	 APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER)
>>>
>>> +static inline u8 kvm_xapic_id(struct kvm_lapic *apic)
>>> +{
>>> +	return kvm_lapic_get_reg(apic, APIC_ID) >> 24;
>>> +}
>>> +
>>> +static inline u32 kvm_x2apic_id(struct kvm_lapic *apic)
>>> +{
>>> +	return apic->vcpu->vcpu_id;
>>> +}
>>> +
>>>  static inline bool kvm_apic_map_get_logical_dest(struct kvm_apic_map *map,
>>>  		u32 dest_id, struct kvm_lapic ***cluster, u16 *mask) {
>>>  	switch (map->mode) {
>>> @@ -159,13 +169,13 @@ static void recalculate_apic_map(struct kvm *kvm)
>>>  	struct kvm_apic_map *new, *old = NULL;
>>>  	struct kvm_vcpu *vcpu;
>>>  	int i;
>>> -	u32 max_id = 255;
>>> +	u32 max_id = 256; /* enough space for any xAPIC ID */
>>
>> Why exactly do we have to increase this?
>>
>> I thought the maximum xapic id is 255, and we correctly allocate
>> 255+1 slots.
>>
>> Now, we would allocate 257 slots and set max_apic_id to 256.
> 
> Right, it is a typical off-by-one error.  The comment is sufficient ...

I can change it myself too.

Paolo

Re: [PATCH v2 2/4] KVM: x86: replace kvm_apic_id with kvm_{x,x2}apic_id

[David Hildenbrand]

>  	kvm_for_each_vcpu(i, vcpu, kvm)
>  		if (kvm_apic_present(vcpu))
> -			max_id = max(max_id, kvm_apic_id(vcpu->arch.apic));
> +			max_id = max(max_id, kvm_x2apic_id(vcpu->arch.apic));
>
>  	new = kvm_kvzalloc(sizeof(struct kvm_apic_map) +
>  	                   sizeof(struct kvm_lapic *) * ((u64)max_id + 1));
> @@ -179,16 +189,23 @@ static void recalculate_apic_map(struct kvm *kvm)
>  		struct kvm_lapic *apic = vcpu->arch.apic;
>  		struct kvm_lapic **cluster;
>  		u16 mask;
> -		u32 ldr, aid;
> +		u32 ldr;
> +		u8 xapic_id;
> +		u32 x2apic_id;
>
>  		if (!kvm_apic_present(vcpu))
>  			continue;
>
> -		aid = kvm_apic_id(apic);

think I'd even prefer here a simple

aid = kvm_xapic_id(apic);
if (apic_x2apic_mode(apic))
	aid = kvm_x2apic_id(apic);

that would keep changes minimal and I don't really see any benefit in 
the code when splitting handling up.

Patch 4 then simply can fixup setting code

if (aid <= new->max_apic_id && !new->phys_map[aid])
	new->phys_map[aid] = apic;

(if I am not missing some important corner case here)


> -		ldr = kvm_lapic_get_reg(apic, APIC_LDR);
> +		xapic_id = kvm_xapic_id(apic);
> +		x2apic_id = kvm_x2apic_id(apic);
>
> -		if (aid <= new->max_apic_id)
> -			new->phys_map[aid] = apic;
> +		if (apic_x2apic_mode(apic) &&
> +				x2apic_id <= new->max_apic_id)
> +			new->phys_map[x2apic_id] = apic;
> +		else if (!apic_x2apic_mode(apic))


This looks good to me.

-- 

David

Re: [PATCH v2 2/4] KVM: x86: replace kvm_apic_id with kvm_{x,x2}apic_id

[Paolo Bonzini]

On 14/12/2016 17:15, David Hildenbrand wrote:
> 
>>      kvm_for_each_vcpu(i, vcpu, kvm)
>>          if (kvm_apic_present(vcpu))
>> -            max_id = max(max_id, kvm_apic_id(vcpu->arch.apic));
>> +            max_id = max(max_id, kvm_x2apic_id(vcpu->arch.apic));
>>
>>      new = kvm_kvzalloc(sizeof(struct kvm_apic_map) +
>>                         sizeof(struct kvm_lapic *) * ((u64)max_id + 1));
>> @@ -179,16 +189,23 @@ static void recalculate_apic_map(struct kvm *kvm)
>>          struct kvm_lapic *apic = vcpu->arch.apic;
>>          struct kvm_lapic **cluster;
>>          u16 mask;
>> -        u32 ldr, aid;
>> +        u32 ldr;
>> +        u8 xapic_id;
>> +        u32 x2apic_id;
>>
>>          if (!kvm_apic_present(vcpu))
>>              continue;
>>
>> -        aid = kvm_apic_id(apic);
> 
> think I'd even prefer here a simple
> 
> aid = kvm_xapic_id(apic);
> if (apic_x2apic_mode(apic))
>     aid = kvm_x2apic_id(apic);
> 
> that would keep changes minimal and I don't really see any benefit in
> the code when splitting handling up.
> 
> Patch 4 then simply can fixup setting code
> 
> if (aid <= new->max_apic_id && !new->phys_map[aid])
>     new->phys_map[aid] = apic;
> 
> (if I am not missing some important corner case here)

Radim, what do you think?  I wanted to get these in before Christmas,
but it's your call.

Paolo

> 
>> -        ldr = kvm_lapic_get_reg(apic, APIC_LDR);
>> +        xapic_id = kvm_xapic_id(apic);
>> +        x2apic_id = kvm_x2apic_id(apic);
>>
>> -        if (aid <= new->max_apic_id)
>> -            new->phys_map[aid] = apic;
>> +        if (apic_x2apic_mode(apic) &&
>> +                x2apic_id <= new->max_apic_id)
>> +            new->phys_map[x2apic_id] = apic;
>> +        else if (!apic_x2apic_mode(apic))
> 
> 
> This looks good to me.
> 

Re: [PATCH v2 2/4] KVM: x86: replace kvm_apic_id with kvm_{x,x2}apic_id

[Radim Krčmář]

2016-12-14 17:15+0100, David Hildenbrand:
>>  	kvm_for_each_vcpu(i, vcpu, kvm)
>>  		if (kvm_apic_present(vcpu))
>> -			max_id = max(max_id, kvm_apic_id(vcpu->arch.apic));
>> +			max_id = max(max_id, kvm_x2apic_id(vcpu->arch.apic));
>> 
>>  	new = kvm_kvzalloc(sizeof(struct kvm_apic_map) +
>>  	                   sizeof(struct kvm_lapic *) * ((u64)max_id + 1));
>> @@ -179,16 +189,23 @@ static void recalculate_apic_map(struct kvm *kvm)
>>  		struct kvm_lapic *apic = vcpu->arch.apic;
>>  		struct kvm_lapic **cluster;
>>  		u16 mask;
>> -		u32 ldr, aid;
>> +		u32 ldr;
>> +		u8 xapic_id;
>> +		u32 x2apic_id;
>> 
>>  		if (!kvm_apic_present(vcpu))
>>  			continue;
>> 
>> -		aid = kvm_apic_id(apic);
> 
> think I'd even prefer here a simple
> 
> aid = kvm_xapic_id(apic);
> if (apic_x2apic_mode(apic))
> 	aid = kvm_x2apic_id(apic);
> 
> that would keep changes minimal and I don't really see any benefit in the
> code when splitting handling up.

It is neccesassary to write an entry for both IDs and I wanted to split
it before [4/4], because doing both changes at once seemed hard to
grasp.

Putting it here didn't work well either ... is a separate patch for the
hunk below better, or would you prefer to have it in [4/4]?

> Patch 4 then simply can fixup setting code
> 
> if (aid <= new->max_apic_id && !new->phys_map[aid])
> 	new->phys_map[aid] = apic;
> 
> (if I am not missing some important corner case here)

The trick is that we want to do the following even in xAPIC mode:

  new->phys_map[kvm_x2apic_id(apic)] = apic;

This is the main idea of the hotplug hack -- to allow unique addressing
of processors that were reset in xAPIC mode.  (And I add a disgusting
"x2apic_id > 0xff" condition in [4/4], because we still allow guests to
change xAPIC IDs, which wouldn't play nice with this.)

Hardware does a superset of this, because it only looks at lower 8 bits
of the desination ID when delivering to xAPIC.

When kvm_x2apic_id(apic) != kvm_xapic_id(apic), then the APIC is in
xAPIC mode so we definitely want to keep xAPIC working, hence

  if (!apic_x2apic_mode(apic))
  	new->phys_map[kvm_xapic_id(apic)] = apic;

Two writes are necessary.

And there can already be another_apic "kvm_x2apic_id(another_apic) ==
kvm_xapic_id(apic)" so we prevent hotplug from breaking existing x2APIC
setups by doing "!new->phys_map[aid]" when setting xAPIC ID.

I hope we get a better solution in the future, but it would have to be
done at hardware (QEMU) level, because even firmware (seabios) doesn't
have standard ways to deal with this situation ...

>> -		ldr = kvm_lapic_get_reg(apic, APIC_LDR);
>> +		xapic_id = kvm_xapic_id(apic);
>> +		x2apic_id = kvm_x2apic_id(apic);
>> 
>> -		if (aid <= new->max_apic_id)
>> -			new->phys_map[aid] = apic;
>> +		if (apic_x2apic_mode(apic) &&
>> +				x2apic_id <= new->max_apic_id)
>> +			new->phys_map[x2apic_id] = apic;
>> +		else if (!apic_x2apic_mode(apic))
> 
> 
> This looks good to me.

Re: [PATCH v2 2/4] KVM: x86: replace kvm_apic_id with kvm_{x,x2}apic_id

[Radim Krčmář]

2016-12-14 17:59+0100, Paolo Bonzini:
> On 14/12/2016 17:15, David Hildenbrand wrote:
>> 
>>>      kvm_for_each_vcpu(i, vcpu, kvm)
>>>          if (kvm_apic_present(vcpu))
>>> -            max_id = max(max_id, kvm_apic_id(vcpu->arch.apic));
>>> +            max_id = max(max_id, kvm_x2apic_id(vcpu->arch.apic));
>>>
>>>      new = kvm_kvzalloc(sizeof(struct kvm_apic_map) +
>>>                         sizeof(struct kvm_lapic *) * ((u64)max_id + 1));
>>> @@ -179,16 +189,23 @@ static void recalculate_apic_map(struct kvm *kvm)
>>>          struct kvm_lapic *apic = vcpu->arch.apic;
>>>          struct kvm_lapic **cluster;
>>>          u16 mask;
>>> -        u32 ldr, aid;
>>> +        u32 ldr;
>>> +        u8 xapic_id;
>>> +        u32 x2apic_id;
>>>
>>>          if (!kvm_apic_present(vcpu))
>>>              continue;
>>>
>>> -        aid = kvm_apic_id(apic);
>> 
>> think I'd even prefer here a simple
>> 
>> aid = kvm_xapic_id(apic);
>> if (apic_x2apic_mode(apic))
>>     aid = kvm_x2apic_id(apic);
>> 
>> that would keep changes minimal and I don't really see any benefit in
>> the code when splitting handling up.
>> 
>> Patch 4 then simply can fixup setting code
>> 
>> if (aid <= new->max_apic_id && !new->phys_map[aid])
>>     new->phys_map[aid] = apic;
>> 
>> (if I am not missing some important corner case here)
> 
> Radim, what do you think?  I wanted to get these in before Christmas,
> but it's your call.

There was a reason why it was so ugly ... it's not a hack for nothing.

I can hope to make the patches/code more understandable, but the
function shouldn't change, unless we want to take a different approach.

Re: [PATCH v2 2/4] KVM: x86: replace kvm_apic_id with kvm_{x,x2}apic_id

[David Hildenbrand]

>> think I'd even prefer here a simple
>>
>> aid = kvm_xapic_id(apic);
>> if (apic_x2apic_mode(apic))
>> 	aid = kvm_x2apic_id(apic);
>>
>> that would keep changes minimal and I don't really see any benefit in the
>> code when splitting handling up.
>
> It is neccesassary to write an entry for both IDs and I wanted to split
> it before [4/4], because doing both changes at once seemed hard to
> grasp.
>
> Putting it here didn't work well either ... is a separate patch for the
> hunk below better, or would you prefer to have it in [4/4]?

I actually would prefer to have it in 4/4, but not sure if it is worth 
yet another round. Anyhow,

Reviewed-by: David Hildenbrand <david@redhat.com>

for this patch (with the 256 fixed)

>
>> Patch 4 then simply can fixup setting code
>>
>> if (aid <= new->max_apic_id && !new->phys_map[aid])
>> 	new->phys_map[aid] = apic;
>>
>> (if I am not missing some important corner case here)
>
> The trick is that we want to do the following even in xAPIC mode:
>
>   new->phys_map[kvm_x2apic_id(apic)] = apic;
>
> This is the main idea of the hotplug hack -- to allow unique addressing
> of processors that were reset in xAPIC mode.  (And I add a disgusting
> "x2apic_id > 0xff" condition in [4/4], because we still allow guests to
> change xAPIC IDs, which wouldn't play nice with this.)
>
> Hardware does a superset of this, because it only looks at lower 8 bits
> of the desination ID when delivering to xAPIC.
>
> When kvm_x2apic_id(apic) != kvm_xapic_id(apic), then the APIC is in
> xAPIC mode so we definitely want to keep xAPIC working, hence
>
>   if (!apic_x2apic_mode(apic))
>   	new->phys_map[kvm_xapic_id(apic)] = apic;

Okay, so this is is the case I missed in patch 4 :)

Thanks for the explanation and sorry for the noise.

-- 

David