On Thu, 2023-03-02 at 13:36 +0800, Gao, Chao wrote:
> On Wed, Mar 01, 2023 at 11:54:38PM +1300, Kai Huang wrote:
> > Make setup_vmcs_config() preemption disabled so it always performs on
> > the same local cpu.
> >
> > During module loading time, KVM intends to call setup_vmcs_config() to
> > set up the global VMCS configurations on _one_ cpu in hardware_setup(),
> > and then calls setup_vmcs_config() on all other online cpus via sending
>
> *all other* is misleading. The compatibility check is actually done on
> *all* online cpus.
>
> for_each_online_cpu(cpu) {
> smp_call_function_single(cpu, kvm_x86_check_cpu_compat, &r, 1);
> if (r < 0)
> goto out_unwind_ops;
> }
>
> Given this, it probably is ok to not disable preemption because all CPUs
> are guaranteed to be compatible later in the flow in terms of VMCS
> capabilities. But we don't want to have such a subtle dependency.
>
> Do you see any real problem with preemption enabled?
No. Just thought it is the right thing to do.
>
> > IPI to perform VMX compatibility check. Further more, KVM has CPU
> > hotplug callback to call setup_vmcs_config() to do compatibility check
> > on the "new-online" cpu to make sure it is compatible too.
> >
> > setup_vmcs_config() is supposed to be done on the same cpu. This is
> > true in the compatibility check code path as setup_vmcs_config() is
> > called either via IPI or in per-cpu CPU hotplug thread. However, the
> > first call from hardware_setup() isn't as it is called when preemption
> > is enabled.
> >
> > Change the existing setup_vmcs_config() to __setup_vmcs_config() and
> > call the latter directly in the compatibility check code path. Change
> > setup_vmcs_config() to call __setup_vmcs_config() with preemption
> > disabled so __setup_vmcs_config() is always done on the same cpu.
>
> Maybe you can simply disable preemption in hardware_setup() although I
> don't have a strong preference.
>
> nested_vmx_setup_ctls_msrs() also reads some MSRs and sets up part of
> vmcs_conf, should it be called on the same CPU as setup_vmcs_config()?
Yes I think so. I missed this :)
Not sure whether there are other similar places too even outside of
hardware_setup().
But compatibility check only checks things calculated via setup_vmcs_config()
and nested_vmx_setup_ctls_msrs(), so I think it's fair to only put
hardware_setup() inside preemption disabled.
