Skip to contentSkip to conversation

Lapic LDR fixup

PATCH Applied Dr. David Alan Gilbert (git) 5 participants 13 messages Nov 17, 2017 – Nov 23, 2017

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Dr. David Alan Gilbert (git)

author
#0 lore raw

cover Lapic LDR fixup

From: "Dr. David Alan Gilbert" <[email protected]>

In x2apic mode the LDR should be fixed, however it gets overwritten when the lapic state is loaded after a migration. Change the fixup to keep LDR constant.

Dave

Dr. David Alan Gilbert (2):
KVM: lapic: Split out x2apic ldr calculation
KVM: lapic: Fixup LDR on load in x2apic

 arch/x86/kvm/lapic.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
Signature · 1 line
2.14.3
author
#1 lore raw

PATCH 1/2 KVM: lapic: Split out x2apic ldr calculation

From: "Dr. David Alan Gilbert" <[email protected]>

Split out the ldr calculation from kvm_apic_set_x2apic_id since we're about to reuse it in the following patch.

Signed-off-by Dr. David Alan Gilbert <[email protected]>
---
 arch/x86/kvm/lapic.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
arch/x86/kvm/lapic.c +6 -1
index 36c90d631096..4991e9e51611 100644
@@ -266,9 +266,14 @@ static inline void kvm_apic_set_ldr(struct kvm_lapic *apic, u32 id)
 	recalculate_apic_map(apic->vcpu->kvm);
 }
 
+static inline u32 kvm_apic_calc_x2apic_ldr(u32 id)
+{
+	return ((id >> 4) << 16) | (1 << (id & 0xf));
+}
+
 static inline void kvm_apic_set_x2apic_id(struct kvm_lapic *apic, u32 id)
 {
-	u32 ldr = ((id >> 4) << 16) | (1 << (id & 0xf));
+	u32 ldr = kvm_apic_calc_x2apic_ldr(id);
 
 	WARN_ON_ONCE(id != apic->vcpu->vcpu_id);
Signature · 1 line
2.14.3
Reviewed-by on v1 1/2
#2 lore raw

Re: [PATCH 1/2] KVM: lapic: Split out x2apic ldr calculation

Quoted 21 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

Split out the ldr calculation from kvm_apic_set_x2apic_id since we're about to reuse it in the following patch.

Signed-off-by Dr. David Alan Gilbert <[email protected]>
---
 arch/x86/kvm/lapic.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
arch/x86/kvm/lapic.c +2 -0
index 36c90d631096..4991e9e51611 100644
@@ -266,9 +266,14 @@ static inline void kvm_apic_set_ldr(struct kvm_lapic *apic, u32 id)
 	recalculate_apic_map(apic->vcpu->kvm);
 }
 
+static inline u32 kvm_apic_calc_x2apic_ldr(u32 id)
+{

arch/x86/kvm/lapic.c static inline void kvm_apic_set_ldr(struct kvm_lapic *apic, u32 id) · line 279

Quoted 10 lines
arch/x86/kvm/lapic.c +4 -1
+	return ((id >> 4) << 16) | (1 << (id & 0xf));
+}
+
 static inline void kvm_apic_set_x2apic_id(struct kvm_lapic *apic, u32 id)
 {
-	u32 ldr = ((id >> 4) << 16) | (1 << (id & 0xf));
+	u32 ldr = kvm_apic_calc_x2apic_ldr(id);
 
 	WARN_ON_ONCE(id != apic->vcpu->vcpu_id);
Reviewed-by David Hildenbrand <[email protected]>
Signature · 3 lines
Thanks,

David / dhildenb
author
#3 lore raw

PATCH 2/2 KVM: lapic: Fixup LDR on load in x2apic

In reply to Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

In x2apic mode the LDR is fixed based on the ID rather than separately loadable like it was before x2. When kvm_apic_set_state is called, the base is set, and if it has the X2APIC_ENABLE flag set then the LDR is calculated; however that value gets overwritten by the memcpy a few lines below overwriting it with the value that came from userland.

The symptom is a lack of EOI after loading the state (e.g. after a QEMU migration) and is due to the EOI bitmap being wrong due to the incorrect LDR. This was seen with a Win2016 guest under Qemu with irqchip=split whose USB mouse didn't work after a VM migration.

This corresponds to RH bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1502591

Reported-by Yiqian Wei <[email protected]>
Signed-off-by Dr. David Alan Gilbert <[email protected]>
---
 arch/x86/kvm/lapic.c | 5 +++++
 1 file changed, 5 insertions(+)
arch/x86/kvm/lapic.c +5 -0
index 4991e9e51611..cff55beb0263 100644
@@ -2201,7 +2201,12 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu,
 {
 	if (apic_x2apic_mode(vcpu->arch.apic)) {
 		u32 *id = (u32 *)(s->regs + APIC_ID);
+		u32 *ldr = (u32 *)(s->regs + APIC_LDR);
 
+		if (set) {
+			/* In x2 the LDR is fixed based on the id */
+			*ldr = kvm_apic_calc_x2apic_ldr(*id);
+		}
 		if (vcpu->kvm->arch.x2apic_format) {
 			if (*id != vcpu->vcpu_id)
 				return -EINVAL;
Signature · 1 line
2.14.3
#4 lore raw

Re: [PATCH 2/2] KVM: lapic: Fixup LDR on load in x2apic

Quoted 32 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

In x2apic mode the LDR is fixed based on the ID rather than separately loadable like it was before x2. When kvm_apic_set_state is called, the base is set, and if it has the X2APIC_ENABLE flag set then the LDR is calculated; however that value gets overwritten by the memcpy a few lines below overwriting it with the value that came from userland.

The symptom is a lack of EOI after loading the state (e.g. after a QEMU migration) and is due to the EOI bitmap being wrong due to the incorrect LDR. This was seen with a Win2016 guest under Qemu with irqchip=split whose USB mouse didn't work after a VM migration.

This corresponds to RH bug:

   https://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.redhat.com_show-5Fbug.cgi-3Fid-3D1502591&d=DwIBaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=BDEsclkj9SNWbZXiuKgX07QVY0LqtwHA13yqtK4wreE&s=MJS_JxKV0dJS6T8qobO29j530xNJLFqgSuRMP8oEiwI&e=
Reported-by Yiqian Wei <[email protected]>
Signed-off-by Dr. David Alan Gilbert <[email protected]>
---
  arch/x86/kvm/lapic.c | 5 +++++
  1 file changed, 5 insertions(+)
arch/x86/kvm/lapic.c +0 -0
index 4991e9e51611..cff55beb0263 100644
@@ -2201,7 +2201,12 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu,
  {
  	if (apic_x2apic_mode(vcpu->arch.apic)) {

arch/x86/kvm/lapic.c static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu, · line 2212

Quoted 10 lines
arch/x86/kvm/lapic.c +5 -0
  		u32 *id = (u32 *)(s->regs + APIC_ID);
+		u32 *ldr = (u32 *)(s->regs + APIC_LDR);
 
+		if (set) {
+			/* In x2 the LDR is fixed based on the id */
+			*ldr = kvm_apic_calc_x2apic_ldr(*id);
+		}
  		if (vcpu->kvm->arch.x2apic_format) {
  			if (*id != vcpu->vcpu_id)
  				return -EINVAL;

I think there is a bug here of not adding the new code in the right place. I think diff should be instead:

@@ -2245,6 +2245,7 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu,

 {
        if (apic_x2apic_mode(vcpu->arch.apic)) {
                u32 *id = (u32 *)(s->regs + APIC_ID);

+ u32 *ldr = (u32 *)(s->regs + APIC_LDR);

                if (vcpu->kvm->arch.x2apic_format) {
                        if (*id != vcpu->vcpu_id)
@@ -2255,6 +2256,8 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu,
                        else
                                *id <<= 24;
                }
+
+               *ldr = kvm_apic_calc_x2apic_ldr(*id);
        }

	return 0;

This is because of the x2apic_format hack.
(Fore more info, see commit 3713131345fb ("KVM: x86: add KVM_CAP_X2APIC_API")). Otherwise, you will use a value which can be shifted-left by 24.

-Liran

#5 lore raw

Re: [PATCH 2/2] KVM: lapic: Fixup LDR on load in x2apic

Quoted 72 lines · Liran Alon
Quoted 43 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

In x2apic mode the LDR is fixed based on the ID rather than separately loadable like it was before x2. When kvm_apic_set_state is called, the base is set, and if it has the X2APIC_ENABLE flag set then the LDR is calculated; however that value gets overwritten by the memcpy a few lines below overwriting it with the value that came from userland.

The symptom is a lack of EOI after loading the state (e.g. after a QEMU migration) and is due to the EOI bitmap being wrong due to the incorrect LDR. This was seen with a Win2016 guest under Qemu with irqchip=split whose USB mouse didn't work after a VM migration.

This corresponds to RH bug:

https://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.redhat.com_show-5Fbug.cgi-3Fid-3D1502591&d=DwIBaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=BDEsclkj9SNWbZXiuKgX07QVY0LqtwHA13yqtK4wreE&s=MJS_JxKV0dJS6T8qobO29j530xNJLFqgSuRMP8oEiwI&e=

Reported-by Yiqian Wei <[email protected]>
Signed-off-by Dr. David Alan Gilbert <[email protected]>
---
  arch/x86/kvm/lapic.c | 5 +++++
  1 file changed, 5 insertions(+)
arch/x86/kvm/lapic.c +5 -0
index 4991e9e51611..cff55beb0263 100644
@@ -2201,7 +2201,12 @@ static int kvm_apic_state_fixup(struct kvm_vcpu
 *vcpu,
  {
      if (apic_x2apic_mode(vcpu->arch.apic)) {
          u32 *id = (u32 *)(s->regs + APIC_ID);
+        u32 *ldr = (u32 *)(s->regs + APIC_LDR);
 
+        if (set) {
+            /* In x2 the LDR is fixed based on the id */
+            *ldr = kvm_apic_calc_x2apic_ldr(*id);
+        }
          if (vcpu->kvm->arch.x2apic_format) {
              if (*id != vcpu->vcpu_id)
Quoted 2 lines
                  return -EINVAL;

I think there is a bug here of not adding the new code in the right place. I think diff should be instead:

arch/x86/kvm/lapic.c +1 -0
@@ -2245,6 +2245,7 @@ static int kvm_apic_state_fixup(struct kvm_vcpu
 *vcpu,
  {
         if (apic_x2apic_mode(vcpu->arch.apic)) {
                 u32 *id = (u32 *)(s->regs + APIC_ID);
+               u32 *ldr = (u32 *)(s->regs + APIC_LDR);
 
                 if (vcpu->kvm->arch.x2apic_format) {

                         if (*id != vcpu->vcpu_id)
(quoted diff) +2 -0
@@ -2255,6 +2256,8 @@ static int kvm_apic_state_fixup(struct kvm_vcpu
 *vcpu,
                         else
                                 *id <<= 24;
                 }
+
+               *ldr = kvm_apic_calc_x2apic_ldr(*id);
         }
 

     return 0;
Quoted 6 lines

This is because of the x2apic_format hack.
(Fore more info, see commit 3713131345fb ("KVM: x86: add KVM_CAP_X2APIC_API")).
Otherwise, you will use a value which can be shifted-left by 24.

-Liran

Sorry I meant diff should be:
@@ -2245,6 +2245,7 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu,

 {
        if (apic_x2apic_mode(vcpu->arch.apic)) {
                u32 *id = (u32 *)(s->regs + APIC_ID);

+ u32 *ldr = (u32 *)(s->regs + APIC_LDR);

                if (vcpu->kvm->arch.x2apic_format) {
                        if (*id != vcpu->vcpu_id)
@@ -2255,6 +2256,9 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu,
                        else
                                *id <<= 24;
                }
+
+               if (set)
+                       *ldr = kvm_apic_calc_x2apic_ldr(*id);
        }

        return 0;
#6 lore raw

Re: [PATCH 2/2] KVM: lapic: Fixup LDR on load in x2apic

2017-11-18 5:03 GMT+08:00 Liran Alon <[email protected]>:

Quoted 96 lines · Liran Alon
Quoted 75 lines · Dr. David Alan Gilbert (git)
Quoted 45 lines

From: "Dr. David Alan Gilbert" <[email protected]>

In x2apic mode the LDR is fixed based on the ID rather than separately loadable like it was before x2. When kvm_apic_set_state is called, the base is set, and if it has the X2APIC_ENABLE flag set then the LDR is calculated; however that value gets overwritten by the memcpy a few lines below overwriting it with the value that came from userland.

The symptom is a lack of EOI after loading the state (e.g. after a QEMU migration) and is due to the EOI bitmap being wrong due to the incorrect LDR. This was seen with a Win2016 guest under Qemu with irqchip=split whose USB mouse didn't work after a VM migration.

This corresponds to RH bug:

https://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.redhat.com_show-5Fbug.cgi-3Fid-3D1502591&d=DwIBaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=BDEsclkj9SNWbZXiuKgX07QVY0LqtwHA13yqtK4wreE&s=MJS_JxKV0dJS6T8qobO29j530xNJLFqgSuRMP8oEiwI&e=

Reported-by Yiqian Wei <[email protected]>
Signed-off-by Dr. David Alan Gilbert <[email protected]>
---
  arch/x86/kvm/lapic.c | 5 +++++
  1 file changed, 5 insertions(+)
arch/x86/kvm/lapic.c +5 -0
index 4991e9e51611..cff55beb0263 100644
@@ -2201,7 +2201,12 @@ static int kvm_apic_state_fixup(struct kvm_vcpu
 *vcpu,
  {
      if (apic_x2apic_mode(vcpu->arch.apic)) {
          u32 *id = (u32 *)(s->regs + APIC_ID);
+        u32 *ldr = (u32 *)(s->regs + APIC_LDR);
 
+        if (set) {
+            /* In x2 the LDR is fixed based on the id */
+            *ldr = kvm_apic_calc_x2apic_ldr(*id);
+        }
          if (vcpu->kvm->arch.x2apic_format) {
              if (*id != vcpu->vcpu_id)
Quoted 2 lines
                  return -EINVAL;

I think there is a bug here of not adding the new code in the right place. I think diff should be instead:

arch/x86/kvm/lapic.c +1 -0
@@ -2245,6 +2245,7 @@ static int kvm_apic_state_fixup(struct kvm_vcpu
 *vcpu,
  {
         if (apic_x2apic_mode(vcpu->arch.apic)) {
                 u32 *id = (u32 *)(s->regs + APIC_ID);
+               u32 *ldr = (u32 *)(s->regs + APIC_LDR);
 
                 if (vcpu->kvm->arch.x2apic_format) {

                         if (*id != vcpu->vcpu_id)
(quoted diff) +2 -0
@@ -2255,6 +2256,8 @@ static int kvm_apic_state_fixup(struct kvm_vcpu
 *vcpu,
                         else
                                 *id <<= 24;
                 }
+
+               *ldr = kvm_apic_calc_x2apic_ldr(*id);
         }
 

     return 0;
Quoted 6 lines

This is because of the x2apic_format hack.
(Fore more info, see commit 3713131345fb ("KVM: x86: add KVM_CAP_X2APIC_API")).
Otherwise, you will use a value which can be shifted-left by 24.

-Liran

Sorry I meant diff should be:

arch/x86/kvm/lapic.c +1 -0
@@ -2245,6 +2245,7 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu,
 {
        if (apic_x2apic_mode(vcpu->arch.apic)) {
                u32 *id = (u32 *)(s->regs + APIC_ID);
+               u32 *ldr = (u32 *)(s->regs + APIC_LDR);
 
                if (vcpu->kvm->arch.x2apic_format) {
                        if (*id != vcpu->vcpu_id)

arch/x86/kvm/lapic.c static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu, · line 2262

Quoted 7 lines
arch/x86/kvm/lapic.c +3 -0
@@ -2255,6 +2256,9 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu,
                        else
                                *id <<= 24;
                }
+
+               if (set)
 
+                       *ldr = kvm_apic_calc_x2apic_ldr(*id);

Why not move this to the else branch of vcpu->kvm->arch.x2apic_format since LDR has already stored 32-bit logical x2APIC ID in x2apic mode?

Regards,
Wanpeng Li

Quoted 3 lines
(quoted diff) +0 -0
        }
 

        return 0;
#7 lore raw

Re: [PATCH 2/2] KVM: lapic: Fixup LDR on load in x2apic

Quoted 103 lines · Wanpeng Li

2017-11-18 5:03 GMT+08:00 Liran Alon <[email protected]>:

Quoted 102 lines · Liran Alon
Quoted 75 lines · Dr. David Alan Gilbert (git)
Quoted 45 lines

From: "Dr. David Alan Gilbert" <[email protected]>

In x2apic mode the LDR is fixed based on the ID rather than separately loadable like it was before x2. When kvm_apic_set_state is called, the base is set, and if it has the X2APIC_ENABLE flag set then the LDR is calculated; however that value gets overwritten by the memcpy a few lines below overwriting it with the value that came from userland.

The symptom is a lack of EOI after loading the state (e.g. after a QEMU migration) and is due to the EOI bitmap being wrong due to the incorrect LDR. This was seen with a Win2016 guest under Qemu with irqchip=split whose USB mouse didn't work after a VM migration.

This corresponds to RH bug:

https://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.redhat.com_show-5Fbug.cgi-3Fid-3D1502591&d=DwIBaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=BDEsclkj9SNWbZXiuKgX07QVY0LqtwHA13yqtK4wreE&s=MJS_JxKV0dJS6T8qobO29j530xNJLFqgSuRMP8oEiwI&e=

Reported-by Yiqian Wei <[email protected]>
Signed-off-by Dr. David Alan Gilbert <[email protected]>
---
   arch/x86/kvm/lapic.c | 5 +++++
   1 file changed, 5 insertions(+)
arch/x86/kvm/lapic.c +5 -0
index 4991e9e51611..cff55beb0263 100644
@@ -2201,7 +2201,12 @@ static int kvm_apic_state_fixup(struct kvm_vcpu
 *vcpu,
   {
       if (apic_x2apic_mode(vcpu->arch.apic)) {
           u32 *id = (u32 *)(s->regs + APIC_ID);
+        u32 *ldr = (u32 *)(s->regs + APIC_LDR);
 
+        if (set) {
+            /* In x2 the LDR is fixed based on the id */
+            *ldr = kvm_apic_calc_x2apic_ldr(*id);
+        }
           if (vcpu->kvm->arch.x2apic_format) {
               if (*id != vcpu->vcpu_id)
Quoted 2 lines
                   return -EINVAL;

I think there is a bug here of not adding the new code in the right place. I think diff should be instead:

arch/x86/kvm/lapic.c +1 -0
@@ -2245,6 +2245,7 @@ static int kvm_apic_state_fixup(struct kvm_vcpu
 *vcpu,
   {
          if (apic_x2apic_mode(vcpu->arch.apic)) {
                  u32 *id = (u32 *)(s->regs + APIC_ID);
+               u32 *ldr = (u32 *)(s->regs + APIC_LDR);
 
                  if (vcpu->kvm->arch.x2apic_format) {

                          if (*id != vcpu->vcpu_id)
(quoted diff) +2 -0
@@ -2255,6 +2256,8 @@ static int kvm_apic_state_fixup(struct kvm_vcpu
 *vcpu,
                          else
                                  *id <<= 24;
                  }
+
+               *ldr = kvm_apic_calc_x2apic_ldr(*id);
          }
 

      return 0;
Quoted 6 lines

This is because of the x2apic_format hack.
(Fore more info, see commit 3713131345fb ("KVM: x86: add KVM_CAP_X2APIC_API")).
Otherwise, you will use a value which can be shifted-left by 24.

-Liran

Sorry I meant diff should be:

arch/x86/kvm/lapic.c +3 -0
@@ -2245,6 +2245,7 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu,
  {
         if (apic_x2apic_mode(vcpu->arch.apic)) {
                 u32 *id = (u32 *)(s->regs + APIC_ID);
+               u32 *ldr = (u32 *)(s->regs + APIC_LDR);
 
                 if (vcpu->kvm->arch.x2apic_format) {
                         if (*id != vcpu->vcpu_id)

@@ -2255,6 +2256,9 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu,
                         else
                                 *id <<= 24;
                 }
+
+               if (set)
Quoted 4 lines

arch/x86/kvm/lapic.c static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu, · line 2208

Quoted 1 line
arch/x86/kvm/lapic.c +1 -0
+                       *ldr = kvm_apic_calc_x2apic_ldr(*id);

Why not move this to the else branch of vcpu->kvm->arch.x2apic_format since LDR has already stored 32-bit logical x2APIC ID in x2apic mode?

First of all, I understand we agree that what I mentioned is indeed a bug in original commit?

Second, if I understood original commit purpose correctly, you want to make sure s->regs APIC_LDR value is correct and matches the s->regs APIC_ID value. Instead of trusting userspace providing these values synced correctly. Therefore, you should re-calc LDR regardless of if x2apic_format was used by userspace or not.

BTW, I would expect that if this is the intent, there should also be a commit which adds the "if (*id != vcpu->vcpu_id)" check to the case of the non-x2apic_format after value was shifted-right by 24.

Regards,
-Liran

Quoted 7 lines

Regards,
Wanpeng Li

Quoted 3 lines
(quoted diff) +0 -0
         }
 

         return 0;
maintainer
#8 lore raw

Re: [PATCH 2/2] KVM: lapic: Fixup LDR on load in x2apic

Quoted 10 lines · Liran Alon
Quoted 1 line

First of all, I understand we agree that what I mentioned is indeed a bug in original commit?

Second, if I understood original commit purpose correctly, you want to make sure s->regs APIC_LDR value is correct and matches the s->regs APIC_ID value. Instead of trusting userspace providing these values synced correctly. Therefore, you should re-calc LDR regardless of if x2apic_format was used by userspace or not.

Quoted 3 lines

BTW, I would expect that if this is the intent, there should also be a commit which adds the "if (*id != vcpu->vcpu_id)" check to the case of the non-x2apic_format after value was shifted-right by 24.

I'm not sure you can do that for backwards-compatibility reasons. I'm applying your fixup, thanks.

Paolo

author
#9 lore raw

Re: [PATCH 2/2] KVM: lapic: Fixup LDR on load in x2apic

In reply to Liran Alon
Quoted 92 lines · * Liran Alon ([email protected]) wrote:
Quoted 72 lines · Liran Alon
Quoted 43 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

In x2apic mode the LDR is fixed based on the ID rather than separately loadable like it was before x2. When kvm_apic_set_state is called, the base is set, and if it has the X2APIC_ENABLE flag set then the LDR is calculated; however that value gets overwritten by the memcpy a few lines below overwriting it with the value that came from userland.

The symptom is a lack of EOI after loading the state (e.g. after a QEMU migration) and is due to the EOI bitmap being wrong due to the incorrect LDR. This was seen with a Win2016 guest under Qemu with irqchip=split whose USB mouse didn't work after a VM migration.

This corresponds to RH bug:

https://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.redhat.com_show-5Fbug.cgi-3Fid-3D1502591&d=DwIBaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=BDEsclkj9SNWbZXiuKgX07QVY0LqtwHA13yqtK4wreE&s=MJS_JxKV0dJS6T8qobO29j530xNJLFqgSuRMP8oEiwI&e=

Reported-by Yiqian Wei <[email protected]>
Signed-off-by Dr. David Alan Gilbert <[email protected]>
---
  arch/x86/kvm/lapic.c | 5 +++++
  1 file changed, 5 insertions(+)
arch/x86/kvm/lapic.c +5 -0
index 4991e9e51611..cff55beb0263 100644
@@ -2201,7 +2201,12 @@ static int kvm_apic_state_fixup(struct kvm_vcpu
 *vcpu,
  {
      if (apic_x2apic_mode(vcpu->arch.apic)) {
          u32 *id = (u32 *)(s->regs + APIC_ID);
+        u32 *ldr = (u32 *)(s->regs + APIC_LDR);
 
+        if (set) {
+            /* In x2 the LDR is fixed based on the id */
+            *ldr = kvm_apic_calc_x2apic_ldr(*id);
+        }
          if (vcpu->kvm->arch.x2apic_format) {
              if (*id != vcpu->vcpu_id)
Quoted 2 lines
                  return -EINVAL;

I think there is a bug here of not adding the new code in the right place. I think diff should be instead:

arch/x86/kvm/lapic.c +1 -0
@@ -2245,6 +2245,7 @@ static int kvm_apic_state_fixup(struct kvm_vcpu
 *vcpu,
  {
         if (apic_x2apic_mode(vcpu->arch.apic)) {
                 u32 *id = (u32 *)(s->regs + APIC_ID);
+               u32 *ldr = (u32 *)(s->regs + APIC_LDR);
 
                 if (vcpu->kvm->arch.x2apic_format) {

                         if (*id != vcpu->vcpu_id)
(quoted diff) +2 -0
@@ -2255,6 +2256,8 @@ static int kvm_apic_state_fixup(struct kvm_vcpu
 *vcpu,
                         else
                                 *id <<= 24;
                 }
+
+               *ldr = kvm_apic_calc_x2apic_ldr(*id);
         }
 

     return 0;
Quoted 6 lines

This is because of the x2apic_format hack.
(Fore more info, see commit 3713131345fb ("KVM: x86: add KVM_CAP_X2APIC_API")).
Otherwise, you will use a value which can be shifted-left by 24.

-Liran

Sorry I meant diff should be:

arch/x86/kvm/lapic.c +1 -0
@@ -2245,6 +2245,7 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu,
 {
        if (apic_x2apic_mode(vcpu->arch.apic)) {
                u32 *id = (u32 *)(s->regs + APIC_ID);
+               u32 *ldr = (u32 *)(s->regs + APIC_LDR);
 
                if (vcpu->kvm->arch.x2apic_format) {
                        if (*id != vcpu->vcpu_id)

arch/x86/kvm/lapic.c static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu, · line 2261

Quoted 6 lines
arch/x86/kvm/lapic.c +3 -0
@@ -2255,6 +2256,9 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu,
                        else
                                *id <<= 24;
                }
+
+               if (set)
+                       *ldr = kvm_apic_calc_x2apic_ldr(*id);

Yes thank you; you're right, and this fixes the problem I'd spotted after I'd posted it with a Linux guest.

Windows:

[ 1539.833110] kvm_apic_state_fixup: x2apic set id=0 x2apic_format=1 ldr=1
[ 1539.833204] kvm_apic_state_fixup: x2apic set id=1 x2apic_format=1 ldr=2

Linux:

[ 1926.724608] kvm_apic_state_fixup: x2apic set id=0 x2apic_format=0 ldr=1
[ 1926.724734] kvm_apic_state_fixup: x2apic set id=1000000 x2apic_format=0 ldr=1
[ 1926.724854] kvm_apic_state_fixup: x2apic set id=2000000 x2apic_format=0 ldr=1
[ 1926.724988] kvm_apic_state_fixup: x2apic set id=3000000 x2apic_format=0 ldr=1

and you see my original patch messed up the LDR in that case.

Moving the *ldr to the bottom we get:

Linux:

[ 2296.390962] kvm_apic_state_fixup: x2apic set id=0 x2apic_format=0 ldr=1
[ 2296.391134] kvm_apic_state_fixup: x2apic set id=1 x2apic_format=0 ldr=2
[ 2296.391254] kvm_apic_state_fixup: x2apic set id=2 x2apic_format=0 ldr=4
[ 2296.391374] kvm_apic_state_fixup: x2apic set id=3 x2apic_format=0 ldr=8

Windows:

[ 2431.062751] kvm_apic_state_fixup: x2apic set id=0 x2apic_format=1 ldr=1
[ 2431.062903] kvm_apic_state_fixup: x2apic set id=1 x2apic_format=1 ldr=2

Dave

Review static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu, · line 2264

Quoted 3 lines
(quoted diff) +0 -0
        }
 
        return 0;

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

maintainer Applied
#10 lore raw
In reply to Dr. David Alan Gilbert (git)
Quoted 6 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

In x2apic mode the LDR should be fixed, however it gets overwritten when the lapic state is loaded after a migration. Change the fixup to keep LDR constant.

Quoted 9 lines

Dave

Dr. David Alan Gilbert (2):
KVM: lapic: Split out x2apic ldr calculation
KVM: lapic: Fixup LDR on load in x2apic

 arch/x86/kvm/lapic.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Queued, thanks.

Paolo

author
#11 lore raw
Quoted 13 lines · * Paolo Bonzini ([email protected]) wrote:
Quoted 12 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

In x2apic mode the LDR should be fixed, however it gets overwritten when the lapic state is loaded after a migration. Change the fixup to keep LDR constant.

Dave

Dr. David Alan Gilbert (2):
KVM: lapic: Split out x2apic ldr calculation
KVM: lapic: Fixup LDR on load in x2apic

Quoted 5 lines
Quoted 3 lines
 arch/x86/kvm/lapic.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Queued, thanks.

Hang on, there's something not right here.
I've got a Linux guest migration that's unstable with this patch.
(Tasks blocked for >22seconds, generally memory related, so I'm wondering if it's IPIs not happening)

I'll get back to you next week.

Dave

Quoted 1 line

Paolo

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

maintainer
#12 lore raw
Quoted 18 lines · Dr. David Alan Gilbert
Quoted 17 lines · * Paolo Bonzini ([email protected]) wrote:
Quoted 15 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

In x2apic mode the LDR should be fixed, however it gets overwritten when the lapic state is loaded after a migration. Change the fixup to keep LDR constant.

Dave

Dr. David Alan Gilbert (2):
KVM: lapic: Split out x2apic ldr calculation
KVM: lapic: Fixup LDR on load in x2apic

 arch/x86/kvm/lapic.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
Quoted 7 lines
Quoted 1 line

Queued, thanks.

Hang on, there's something not right here.
I've got a Linux guest migration that's unstable with this patch.
(Tasks blocked for >22seconds, generally memory related, so I'm wondering if it's IPIs not happening)

Too late, but I'll take a look as well on Monday, in case I can spot something obvious.

Paolo