Re: [PATCH 10/44] KVM: VMX: Clean up eVMCS enabling if KVM initialization fails

Sean Christopherson [off-list ref] writes:

On Thu, Nov 03, 2022, Vitaly Kuznetsov wrote:

quoted

Sean Christopherson [off-list ref] writes:

quoted

+	/*
+	 * Reset everything to support using non-enlightened VMCS access later
+	 * (e.g. when we reload the module with enlightened_vmcs=0)
+	 */
+	for_each_online_cpu(cpu) {
+		vp_ap =	hv_get_vp_assist_page(cpu);
+
+		if (!vp_ap)
+			continue;
+
+		vp_ap->nested_control.features.directhypercall = 0;
+		vp_ap->current_nested_vmcs = 0;
+		vp_ap->enlighten_vmentry = 0;
+	}

Unrelated to your patch but while looking at this code I got curious
about why don't we need a protection against CPU offlining here. Turns
out that even when we offline a CPU, its VP assist page remains
allocated (see hv_cpu_die()), we just write '0' to the MSR and thus

Heh, "die".  Hyper-V is quite dramatic.

quoted

accessing the page is safe. The consequent hv_cpu_init(), however, does
not restore VP assist page when it's already allocated:

# rdmsr -p 24 0x40000073
10212f001
# echo 0 > /sys/devices/system/cpu/cpu24/online 
# echo 1 > /sys/devices/system/cpu/cpu24/online 
# rdmsr -p 24 0x40000073
0

The culprit is commit e5d9b714fe402 ("x86/hyperv: fix root partition
faults when writing to VP assist page MSR"). A patch is inbound.

'hv_root_partition' case is different though. We do memunmap() and reset
VP assist page to zero so it is theoretically possible we're going to
clash. Unless I'm missing some obvious reason why module unload can't
coincide with CPU offlining, we may be better off surrounding this with
cpus_read_lock()/cpus_read_unlock(). 

I finally see what you're concerned about.  If a CPU goes offline and its assist
page is unmapped, zeroing out the nested/eVMCS stuff will fault.

I think the real problem is that the purging of the eVMCS is in the wrong place.
Move the clearing to vmx_hardware_disable() and then the CPU hotplug bug goes
away once KVM disables hotplug during hardware enabling/disable later in the series.
There's no need to wait until module exit, e.g. it's not like it costs much to
clear a few variables, and IIUC the state is used only when KVM is actively using
VMX/eVMCS.

However, I believe there's a second bug.  KVM's CPU online hook is called before
Hyper-V's online hook (CPUHP_AP_ONLINE_DYN).  Before this series, which moves KVM's
hook from STARTING to ONLINE, KVM's hook is waaaay before Hyper-V's.  That means
that hv_cpu_init()'s allocation of the VP assist page will come _after_ KVM's
check in vmx_hardware_enable()

	/*
	 * This can happen if we hot-added a CPU but failed to allocate
	 * VP assist page for it.
	 */
	if (static_branch_unlikely(&enable_evmcs) &&
	    !hv_get_vp_assist_page(cpu))
		return -EFAULT;

I.e. CPU hotplug will never work if KVM is running VMs as a Hyper-V guest.  I bet
you can repro by doing a SUSPEND+RESUME.

Can you try to see if that's actually a bug?  If so, the only sane fix seems to
be to add a dedicated ONLINE action for Hyper-V.  

It seems we can't get away without a dedicated stage for Hyper-V anyway,
e.g. see our discussion with Michael:

https://lore.kernel.org/linux-hyperv/878rkqr7ku.fsf@ovpn-192-136.brq.redhat.com/ (local)

All these issues are more or less "theoretical" as there's no real CPU
hotplug on Hyper-V/Azure. Yes, it is possible to trigger problems by
doing CPU offline/online but I don't see how this may come handy outside
of testing envs.

Per patch

  KVM: Rename and move CPUHP_AP_KVM_STARTING to ONLINE section

from this series, CPUHP_AP_KVM_ONLINE needs to be before CPUHP_AP_SCHED_WAIT_EMPTY
to ensure there are no tasks, i.e. no vCPUs, running on the to-be-unplugged CPU.

Back to the original bug, proposed fix is below.  The other advantage of moving
the reset to hardware disabling is that the "cleanup" is just disabling the static
key, and at that point can simply be deleted as there's no need to disable the
static key when kvm-intel is unloaded since kvm-intel owns the key.  I.e. this
patch (that we're replying to) would get replaced with a patch to delete the
disabling of the static key.

From a quick glance looks good to me, I'll try to find some time to work
on this issue. I will likely end up proposing a dedicated CPU hotplug
stage for Hyper-V (which needs to happen before KVM's
CPUHP_AP_KVM_ONLINE on CPU hotplug and after on unplug) anyway.

Thanks for looking into this!

-- 
Vitaly