Re: [PATCH] KVM: VMX: Make setup_vmcs_config() preemption disabled

[Huang, Kai]

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.