• Sean Christopherson's avatar
    Revert "KVM: x86: Open code necessary bits of kvm_lapic_set_base() at vCPU RESET" · f7d8a19f
    Sean Christopherson authored
    Revert a change to open code bits of kvm_lapic_set_base() when emulating
    APIC RESET to fix an apic_hw_disabled underflow bug due to arch.apic_base
    and apic_hw_disabled being unsyncrhonized when the APIC is created.  If
    kvm_arch_vcpu_create() fails after creating the APIC, kvm_free_lapic()
    will see the initialized-to-zero vcpu->arch.apic_base and decrement
    apic_hw_disabled without KVM ever having incremented apic_hw_disabled.
    
    Using kvm_lapic_set_base() in kvm_lapic_reset() is also desirable for a
    potential future where KVM supports RESET outside of vCPU creation, in
    which case all the side effects of kvm_lapic_set_base() are needed, e.g.
    to handle the transition from x2APIC => xAPIC.
    
    Alternatively, KVM could temporarily increment apic_hw_disabled (and call
    kvm_lapic_set_base() at RESET), but that's a waste of cycles and would
    impact the performance of other vCPUs and VMs.  The other subtle side
    effect is that updating the xAPIC ID needs to be done at RESET regardless
    of whether the APIC was previously enabled, i.e. kvm_lapic_reset() needs
    an explicit call to kvm_apic_set_xapic_id() regardless of whether or not
    kvm_lapic_set_base() also performs the update.  That makes stuffing the
    enable bit at vCPU creation slightly more palatable, as doing so affects
    only the apic_hw_disabled key.
    
    Opportunistically tweak the comment to explicitly call out the connection
    between vcpu->arch.apic_base and apic_hw_disabled, and add a comment to
    call out the need to always do kvm_apic_set_xapic_id() at RESET.
    
    Underflow scenario:
    
      kvm_vm_ioctl() {
        kvm_vm_ioctl_create_vcpu() {
          kvm_arch_vcpu_create() {
            if (something_went_wrong)
              goto fail_free_lapic;
            /* vcpu->arch.apic_base is initialized when something_went_wrong is false. */
            kvm_vcpu_reset() {
              kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event) {
                vcpu->arch.apic_base = APIC_DEFAULT_PHYS_BASE | MSR_IA32_APICBASE_ENABLE;
              }
            }
            return 0;
          fail_free_lapic:
            kvm_free_lapic() {
              /* vcpu->arch.apic_base is not yet initialized when something_went_wrong is true. */
              if (!(vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE))
                static_branch_slow_dec_deferred(&apic_hw_disabled); // <= underflow bug.
            }
            return r;
          }
        }
      }
    
    This (mostly) reverts commit 42122123.
    
    Fixes: 42122123 ("KVM: x86: Open code necessary bits of kvm_lapic_set_base() at vCPU RESET")
    Reported-by: syzbot+9fc046ab2b0cf295a063@syzkaller.appspotmail.com
    Debugged-by: default avatarTetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
    Signed-off-by: default avatarSean Christopherson <seanjc@google.com>
    Message-Id: <20211013003554.47705-2-seanjc@google.com>
    Signed-off-by: default avatarPaolo Bonzini <pbonzini@redhat.com>
    f7d8a19f
lapic.c 74.9 KB