Re: [PATCH RESEND v3 2/3] KVM: vmx: Emulate MSR IA32_UMWAIT_CONTROL

[Tao Xu]

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.