* [PATCH 0/2] Lapic LDR fixup
@ 2017-11-17 11:52 Dr. David Alan Gilbert (git)
2017-11-17 11:52 ` [PATCH 1/2] KVM: lapic: Split out x2apic ldr calculation Dr. David Alan Gilbert (git)
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2017-11-17 11:52 UTC (permalink / raw)
To: kvm; +Cc: pbonzini
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
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(-)
--
2.14.3
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] KVM: lapic: Split out x2apic ldr calculation
2017-11-17 11:52 [PATCH 0/2] Lapic LDR fixup Dr. David Alan Gilbert (git)
@ 2017-11-17 11:52 ` Dr. David Alan Gilbert (git)
2017-11-23 15:31 ` David Hildenbrand
2017-11-17 11:52 ` [PATCH 2/2] KVM: lapic: Fixup LDR on load in x2apic Dr. David Alan Gilbert (git)
2017-11-17 12:29 ` [PATCH 0/2] Lapic LDR fixup Paolo Bonzini
2 siblings, 1 reply; 13+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2017-11-17 11:52 UTC (permalink / raw)
To: kvm; +Cc: pbonzini
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
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 <dgilbert@redhat.com>
---
arch/x86/kvm/lapic.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 36c90d631096..4991e9e51611 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -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);
--
2.14.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] KVM: lapic: Fixup LDR on load in x2apic
2017-11-17 11:52 [PATCH 0/2] Lapic LDR fixup Dr. David Alan Gilbert (git)
2017-11-17 11:52 ` [PATCH 1/2] KVM: lapic: Split out x2apic ldr calculation Dr. David Alan Gilbert (git)
@ 2017-11-17 11:52 ` Dr. David Alan Gilbert (git)
2017-11-17 21:01 ` Liran Alon
2017-11-17 12:29 ` [PATCH 0/2] Lapic LDR fixup Paolo Bonzini
2 siblings, 1 reply; 13+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2017-11-17 11:52 UTC (permalink / raw)
To: kvm; +Cc: pbonzini
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
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 <yiwei@redhat.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
arch/x86/kvm/lapic.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 4991e9e51611..cff55beb0263 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -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;
--
2.14.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] Lapic LDR fixup
2017-11-17 11:52 [PATCH 0/2] Lapic LDR fixup Dr. David Alan Gilbert (git)
2017-11-17 11:52 ` [PATCH 1/2] KVM: lapic: Split out x2apic ldr calculation Dr. David Alan Gilbert (git)
2017-11-17 11:52 ` [PATCH 2/2] KVM: lapic: Fixup LDR on load in x2apic Dr. David Alan Gilbert (git)
@ 2017-11-17 12:29 ` Paolo Bonzini
2017-11-17 19:37 ` Dr. David Alan Gilbert
2 siblings, 1 reply; 13+ messages in thread
From: Paolo Bonzini @ 2017-11-17 12:29 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git), kvm
On 17/11/2017 12:52, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> 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(-)
>
Queued, thanks.
Paolo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] Lapic LDR fixup
2017-11-17 12:29 ` [PATCH 0/2] Lapic LDR fixup Paolo Bonzini
@ 2017-11-17 19:37 ` Dr. David Alan Gilbert
2017-11-17 20:27 ` Paolo Bonzini
0 siblings, 1 reply; 13+ messages in thread
From: Dr. David Alan Gilbert @ 2017-11-17 19:37 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kvm
* Paolo Bonzini (pbonzini@redhat.com) wrote:
> On 17/11/2017 12:52, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > 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(-)
> >
>
> 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
> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] Lapic LDR fixup
2017-11-17 19:37 ` Dr. David Alan Gilbert
@ 2017-11-17 20:27 ` Paolo Bonzini
0 siblings, 0 replies; 13+ messages in thread
From: Paolo Bonzini @ 2017-11-17 20:27 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: kvm
On 17/11/2017 20:37, Dr. David Alan Gilbert wrote:
> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>> On 17/11/2017 12:52, Dr. David Alan Gilbert (git) wrote:
>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>
>>> 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(-)
>>>
>>
>> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] KVM: lapic: Fixup LDR on load in x2apic
2017-11-17 11:52 ` [PATCH 2/2] KVM: lapic: Fixup LDR on load in x2apic Dr. David Alan Gilbert (git)
@ 2017-11-17 21:01 ` Liran Alon
2017-11-17 21:03 ` Liran Alon
0 siblings, 1 reply; 13+ messages in thread
From: Liran Alon @ 2017-11-17 21:01 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git), kvm; +Cc: pbonzini
On 17/11/17 13:52, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> 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 <yiwei@redhat.com>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> arch/x86/kvm/lapic.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 4991e9e51611..cff55beb0263 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -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;
>
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] KVM: lapic: Fixup LDR on load in x2apic
2017-11-17 21:01 ` Liran Alon
@ 2017-11-17 21:03 ` Liran Alon
2017-11-20 0:53 ` Wanpeng Li
2017-11-20 10:48 ` Dr. David Alan Gilbert
0 siblings, 2 replies; 13+ messages in thread
From: Liran Alon @ 2017-11-17 21:03 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git), kvm; +Cc: pbonzini
On 17/11/17 23:01, Liran Alon wrote:
>
>
> On 17/11/17 13:52, Dr. David Alan Gilbert (git) wrote:
>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>
>> 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 <yiwei@redhat.com>
>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> ---
>> arch/x86/kvm/lapic.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 4991e9e51611..cff55beb0263 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -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;
>>
>
> 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
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;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] KVM: lapic: Fixup LDR on load in x2apic
2017-11-17 21:03 ` Liran Alon
@ 2017-11-20 0:53 ` Wanpeng Li
2017-11-20 1:04 ` Liran Alon
2017-11-20 10:48 ` Dr. David Alan Gilbert
1 sibling, 1 reply; 13+ messages in thread
From: Wanpeng Li @ 2017-11-20 0:53 UTC (permalink / raw)
To: Liran Alon; +Cc: Dr. David Alan Gilbert (git), kvm, Paolo Bonzini
2017-11-18 5:03 GMT+08:00 Liran Alon <LIRAN.ALON@oracle.com>:
>
>
> On 17/11/17 23:01, Liran Alon wrote:
>>
>>
>>
>> On 17/11/17 13:52, Dr. David Alan Gilbert (git) wrote:
>>>
>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>
>>> 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 <yiwei@redhat.com>
>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>> ---
>>> arch/x86/kvm/lapic.c | 5 +++++
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>>> index 4991e9e51611..cff55beb0263 100644
>>> --- a/arch/x86/kvm/lapic.c
>>> +++ b/arch/x86/kvm/lapic.c
>>> @@ -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;
>>>
>>
>> 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
>
>
> 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);
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
> }
>
> return 0;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] KVM: lapic: Fixup LDR on load in x2apic
2017-11-20 0:53 ` Wanpeng Li
@ 2017-11-20 1:04 ` Liran Alon
2017-11-20 20:48 ` Paolo Bonzini
0 siblings, 1 reply; 13+ messages in thread
From: Liran Alon @ 2017-11-20 1:04 UTC (permalink / raw)
To: Wanpeng Li; +Cc: Dr. David Alan Gilbert (git), kvm, Paolo Bonzini
On 20/11/17 02:53, Wanpeng Li wrote:
> 2017-11-18 5:03 GMT+08:00 Liran Alon <LIRAN.ALON@oracle.com>:
>>
>>
>> On 17/11/17 23:01, Liran Alon wrote:
>>>
>>>
>>>
>>> On 17/11/17 13:52, Dr. David Alan Gilbert (git) wrote:
>>>>
>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>>
>>>> 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 <yiwei@redhat.com>
>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>> ---
>>>> arch/x86/kvm/lapic.c | 5 +++++
>>>> 1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>>>> index 4991e9e51611..cff55beb0263 100644
>>>> --- a/arch/x86/kvm/lapic.c
>>>> +++ b/arch/x86/kvm/lapic.c
>>>> @@ -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;
>>>>
>>>
>>> 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
>>
>>
>> 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);
>
> 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
>
> Regards,
> Wanpeng Li
>
>> }
>>
>> return 0;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] KVM: lapic: Fixup LDR on load in x2apic
2017-11-17 21:03 ` Liran Alon
2017-11-20 0:53 ` Wanpeng Li
@ 2017-11-20 10:48 ` Dr. David Alan Gilbert
1 sibling, 0 replies; 13+ messages in thread
From: Dr. David Alan Gilbert @ 2017-11-20 10:48 UTC (permalink / raw)
To: Liran Alon; +Cc: kvm, pbonzini
* Liran Alon (LIRAN.ALON@ORACLE.COM) wrote:
>
>
> On 17/11/17 23:01, Liran Alon wrote:
> >
> >
> > On 17/11/17 13:52, Dr. David Alan Gilbert (git) wrote:
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > >
> > > 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 <yiwei@redhat.com>
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > ---
> > > arch/x86/kvm/lapic.c | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > index 4991e9e51611..cff55beb0263 100644
> > > --- a/arch/x86/kvm/lapic.c
> > > +++ b/arch/x86/kvm/lapic.c
> > > @@ -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;
> > >
> >
> > 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
>
> 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);
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
> }
>
> return 0;
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] KVM: lapic: Fixup LDR on load in x2apic
2017-11-20 1:04 ` Liran Alon
@ 2017-11-20 20:48 ` Paolo Bonzini
0 siblings, 0 replies; 13+ messages in thread
From: Paolo Bonzini @ 2017-11-20 20:48 UTC (permalink / raw)
To: Liran Alon, Wanpeng Li; +Cc: Dr. David Alan Gilbert (git), kvm
On 20/11/2017 02:04, Liran Alon wrote:
>>
> 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.
I'm not sure you can do that for backwards-compatibility reasons. I'm
applying your fixup, thanks.
Paolo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] KVM: lapic: Split out x2apic ldr calculation
2017-11-17 11:52 ` [PATCH 1/2] KVM: lapic: Split out x2apic ldr calculation Dr. David Alan Gilbert (git)
@ 2017-11-23 15:31 ` David Hildenbrand
0 siblings, 0 replies; 13+ messages in thread
From: David Hildenbrand @ 2017-11-23 15:31 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git), kvm; +Cc: pbonzini
On 17.11.2017 12:52, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> 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 <dgilbert@redhat.com>
> ---
> arch/x86/kvm/lapic.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 36c90d631096..4991e9e51611 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -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);
>
>
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-11-23 15:31 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-17 11:52 [PATCH 0/2] Lapic LDR fixup Dr. David Alan Gilbert (git)
2017-11-17 11:52 ` [PATCH 1/2] KVM: lapic: Split out x2apic ldr calculation Dr. David Alan Gilbert (git)
2017-11-23 15:31 ` David Hildenbrand
2017-11-17 11:52 ` [PATCH 2/2] KVM: lapic: Fixup LDR on load in x2apic Dr. David Alan Gilbert (git)
2017-11-17 21:01 ` Liran Alon
2017-11-17 21:03 ` Liran Alon
2017-11-20 0:53 ` Wanpeng Li
2017-11-20 1:04 ` Liran Alon
2017-11-20 20:48 ` Paolo Bonzini
2017-11-20 10:48 ` Dr. David Alan Gilbert
2017-11-17 12:29 ` [PATCH 0/2] Lapic LDR fixup Paolo Bonzini
2017-11-17 19:37 ` Dr. David Alan Gilbert
2017-11-17 20:27 ` Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.