From:   Thomas Gleixner <tglx@linutronix.de>
To:     "Tian, Kevin" <kevin.tian@intel.com>,
        Paolo Bonzini <pbonzini@redhat.com>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "Yamahata, Isaku" <isaku.yamahata@intel.com>,
        "Huang, Kai" <kai.huang@intel.com>,
        "Nakajima, Jun" <jun.nakajima@intel.com>,
        "Hansen, Dave" <dave.hansen@intel.com>,
        "Gao, Chao" <chao.gao@intel.com>
Subject: RE: Q. about KVM and CPU hotplug
In-Reply-To: <BN9PR11MB54333C976289C4AA42D7B1AA8C689@BN9PR11MB5433.namprd11.prod.outlook.com>
References: <BL1PR11MB54295ADE4D7A81523EA50B2D8C679@BL1PR11MB5429.namprd11.prod.outlook.com>
 <3d3296f0-9245-40f9-1b5a-efffdb082de9@redhat.com>
 <BN9PR11MB54333C976289C4AA42D7B1AA8C689@BN9PR11MB5433.namprd11.prod.outlook.com>
Date:   Wed, 01 Dec 2021 11:30:57 +0100
Message-ID: <87bl20aa72.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Wed, Dec 01 2021 at 06:59, Kevin Tian wrote:
>> From: Paolo Bonzini <paolo.bonzini@gmail.com>
>> It should fail the first vmptrld instruction.  It will result in a few
>> WARN_ONCE and pr_warn_ratelimited (see vmx_insn_failed).  For VMX this
>> should be a pretty bad firmware bug, and it has never been reported.
>> KVM did find some undocumented errata but not this one!
>> 
>
> or it may be caused by incompatible CPU capabilities, which is currently
> missing a check in kvm_starting_cpu(). So far the compatibility check is
> done only once before registering cpu hotplug state machine:
>
>         for_each_online_cpu(cpu) {
>                 smp_call_function_single(cpu, check_processor_compat, &c, 1);
>                 if (r < 0)
>                         goto out_free_2;
>         }
>
>         r = cpuhp_setup_state_nocalls(CPUHP_AP_KVM_STARTING, "kvm/cpu:starting",
>                                       kvm_starting_cpu, kvm_dying_cpu);

Duh. This is silly _and_ broken.

Using for_each_inline_cpu() without holding cpus_read_lock() is racy
against concurrent hotplug. But even if the locking is added then
nothing prevents a CPU from being plugged _after_ the lock is dropped.

The right solution is to move the hotplug state into the threaded
section as I pointed out and do:

         r = cpuhp_setup_state(CPUHP_AP_KVM_STARTING, "kvm/cpu:starting",
                               kvm_starting_cpu, kvm_dying_cpu);

which will do the right thing automatically. Checking for compatibility
would just be part of the kvm_starting_cpu() callback.

Thanks,

        tglx