Re: [RFC PATCH 1/2] KVM: x86: update APIC_ID also when disabling x2APIC in kvm_lapic_set_base

[Emanuele Giuseppe Esposito]

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
>