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? 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