From: Tao Xu
Date: Mon Jun 17 2019 - 22:45:28 EST

On 6/17/2019 11:50 PM, Radim KrÄmÃÅ wrote:
2019-06-17 14:31+0800, Xiaoyao Li:
On 6/17/2019 11:32 AM, Xiaoyao Li wrote:
On 6/16/2019 5:55 PM, Tao Xu wrote:
+ÂÂÂ if (vmx->msr_ia32_umwait_control != host_umwait_control)
+ÂÂÂÂÂÂÂ add_atomic_switch_msr(vmx, MSR_IA32_UMWAIT_CONTROL,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ vmx->msr_ia32_umwait_control,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ host_umwait_control, false);

The bit 1 is reserved, at least, we need to do below to ensure not
modifying the reserved bit:

ÂÂÂÂguest_val = (vmx->msr_ia32_umwait_control & ~BIT_ULL(1)) |
ÂÂÂÂÂÂÂÂÂÂÂ (host_val & BIT_ULL(1))

I find a better solution to ensure reserved bit 1 not being modified in
vmx_set_msr() as below:

if((data ^ umwait_control_cached) & BIT_ULL(1))
return 1;

We could just be checking

if (data & BIT_ULL(1))

because the guest cannot change its visible reserved value and KVM
currently initializes the value to 0.

The arch/x86/kernel/cpu/umwait.c series assumes that the reserved bit
is 0 (hopefully deliberately) and I would do the same in KVM as it
simplifies the logic. (We don't have to even think about migrations
between machines with a different reserved value and making it play
nicely with possible future implementations of that bit.)


Thank you Radim and xiaoyao's review, I will improve it in the next version. Xiaoyao's suggestion remind me another thing. And I am wondering if we need to initialize the value of MSR_IA32_UMWAIT_CONTROL in KVM to 0x186a0(umwait_control = 100000, as host does).

Although the guest with new kernel(has umwait host patch)can initialize the value to 0x186a0. But there is a case that a guest with a old kernel and the host with the new kernel and has the cpuid of WAITPKG. Because the msr value is 0, the guest umwait will have no max time by default.