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.)
Thanks.
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.
