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

[Sean Christopherson <seanjc@google.com>]

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);
> > +		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?  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