lkml.org 
[lkml]   [2023]   [Jan]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [RFC PATCH 1/2] KVM: x86: update APIC_ID also when disabling x2APIC in kvm_lapic_set_base
On 1/10/23 13:16, Emanuele Giuseppe Esposito wrote:
> I think the test in patch 2 I wrote gives a better idea on what I am
> trying to fix: if we are transitioning from x2APIC to xAPIC (RESET I
> would say, even though I am not sure if userspace really does it in the
> way I do it in the test, ie through KVM_SET_MSRS), the APIC_ID is not
> updated back in the right bits, and we can see that by querying the ID
> with KVM_GET_LAPIC after disabling x2APIC.
>
> Now, if the way I reproduce this issue is correct, it is indeed a bug
> and needs to be fixed with the fix in patch 1 or something similar.
> I think it won't really make any difference if instead following what
> the doc says (x2APIC -> disabled -> xAPIC) we directly do x2APIC -> xAPIC.

Yes, the default value at reset is xAPIC mode, so a reset will do a
KVM_SET_MSRS that clears X2APIC_ENABLE but leaves
MSR_IA32_APICBASE_ENABLE set.

So, if I understand correctly...

> The test in patch 2 started being developed to test ef40757743b47 ("KVM:
> x86: fix APICv/x2AVIC disabled when vm reboot by itself") even though I
> honestly didn't really understand how to replicate that bug (see cover
> letter) and instead I found this other possibility that still manages to
> screw APIC_ID.

... what you're saying is that there were two different bugs, but one
fixing any one of them was enough to prevent the symptoms shown by
commit ef40757743b47? That is:

- the APICv inhibit was set by KVM_GET_LAPIC because it called
kvm_lapic_xapic_id_updated(), and the call was unnecessary as fixed in
commit ef40757743b47;

- however, there is no reason for the vCPU ID to be mismatched. It
happened because the code didn't handle the host-initiated x2APIC->xAPIC
case and thus lacked a call to kvm_apic_set_xapic_id().

If so, I think the idea of the patch is fine.

Just one thing: your patch also changes the APIC_ID on the
x2APIC->disabled transition, not just the "forbidden" (i.e. host-
initiated only) x2APIC->xAPIC transition. I think this is okay too: the
manual says:

10.4.3 Enabling or Disabling the Local APIC

When IA32_APIC_BASE[11] is set to 0, prior initialization to the APIC
may be lost and the APIC may return to the state described in Section
10.4.7.1, “Local APIC State After Power-Up or Reset.”

10.4.7.1 Local APIC State After Power-Up or Reset

... The local APIC ID register is set to a unique APIC ID. ...

(which must be an xAPIC ID) and this is what your patch does.

In fact perhaps you can change the code further to invoke
kvm_lapic_reset() after static_branch_inc(&apic_hw_disabled.key)? It's
just a bit messy that you have a call back to kvm_lapic_set_base() in
there, so perhaps something like this can help:

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 4efdb4a4d72c..24e5df23a4d9 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2433,9 +2436,7 @@ void kvm_apic_update_apicv(struct kvm_vcpu *vcpu)

void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event)
{
- struct kvm_lapic *apic = vcpu->arch.apic;
u64 msr_val;
- int i;

if (!init_event) {
msr_val = APIC_DEFAULT_PHYS_BASE | MSR_IA32_APICBASE_ENABLE;
@@ -2444,8 +2445,14 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event)
kvm_lapic_set_base(vcpu, msr_val);
}

- if (!apic)
- return;
+ if (vcpu->arch.apic)
+ __kvm_lapic_reset(vcpu, init_event);
+}
+
+static void __kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event)
+{
+ struct kvm_lapic *apic = vcpu->arch.apic;
+ int i;

/* Stop the timer in case it's a reset to an active apic */
hrtimer_cancel(&apic->lapic_timer.timer);

(just a sketch to show the idea, of course __kvm_lapic_reset would have to
go first).

Paolo

\
 
 \ /
  Last update: 2023-03-26 23:34    [W:0.109 / U:0.524 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site