Am 09/01/2023 um 17:23 schrieb Sean Christopherson:
On Mon, Jan 09, 2023, Maxim Levitsky wrote:
On Mon, 2023-01-09 at 08:06 -0500, Emanuele Giuseppe Esposito wrote:
If KVM_SET_MSR firstly enables and then disables x2APIC, make sure APIC_ID is actually updated correctly, since bits and offset differ from xAPIC and x2APIC.
Currently this is not handled correctly, as kvm_set_apic_base() will have msr_info->host_initiated, so switching from x2APIC to xAPIC won't fail, but kvm_lapic_set_base() does not handle the case.
Fixes: 8d860bbeedef ("kvm: vmx: Basic APIC virtualization controls have three settings") Signed-off-by: Emanuele Giuseppe Esposito eesposit@redhat.com
arch/x86/kvm/lapic.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index 4efdb4a4d72c..df0a50099aa2 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -2394,8 +2394,12 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value) } }
- if (((old_value ^ value) & X2APIC_ENABLE) && (value & X2APIC_ENABLE))
kvm_apic_set_x2apic_id(apic, vcpu->vcpu_id);
- if ((old_value ^ value) & X2APIC_ENABLE) {
if (value & X2APIC_ENABLE)kvm_apic_set_x2apic_id(apic, vcpu->vcpu_id);elsekvm_apic_set_xapic_id(apic, vcpu->vcpu_id);- }
if ((old_value ^ value) & (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE)) { kvm_vcpu_update_apicv(vcpu);
I don't think that this patch is 100% needed in a strict sense, but I don't object to it either.
I'd prefer not to go this route, I assume/suspect there's a diffferent underlying issue that is the real problem.
The switch between x2apic and xapic mode is not allowed by X86 spec, while vise versa is allowed and I think that the spec says that in this case APIC ID is restored to its default value.
No, APIC ID is initialized on RESET, but AFAIK it's preserved for all other transitions. It's definitely preserved on INIT (doesn't touch the enable bit), and this snippet from the SDM more or less says the APIC ID is preserved when it's disabled in IA32_APIC_BASE.
From the disabled state, the only valid x2APIC transition using IA32_APIC_BASE is to the xAPIC mode (EN= 1, EXTD = 0). Thus the only means to transition from x2APIC mode to xAPIC mode is a two-step process:
- first transition from x2APIC mode to local APIC disabled mode (EN= 0, EXTD = 0),
- followed by another transition from disabled mode to xAPIC mode (EN= 1, EXTD= 0).
Consequently, all the APIC register states in the x2APIC, except for the x2APIC ID (32 bits), are not preserved across mode transitions.
And for RESET vs. INIT
A reset in this state places the x2APIC in xAPIC mode. All APIC registers (including the local APIC ID register) are initialized as described in Section 10.12.5.1.
An INIT in this state keeps the x2APIC in the x2APIC mode. The state of the local APIC ID register is preserved (all 32 bits). However, all the other APIC registers are initialized as a result of the INIT transition.
Emanuele, what is the actual issue you are trying to fix? E.g. is APICv left inihibited after an emulated RESET? Something else?
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.
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.
Hope this clarifies it a little, Emanuele
Stuffing APIC state from
userspace should do the right thing after commit ef40757743b4 ("KVM: x86: fix APICv/x2AVIC disabled when vm reboot by itself") and this patch:
https://lore.kernel.org/all/20230106011306.85230-33-seanjc@google.com