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@xxxxxxxxxx>
>>> ---
>>> 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);
>>> + else
>>> + kvm_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@xxxxxxxxxx
>
