linux-kernel - Re: [PATCH RESEND v3 2/3] KVM: vmx: Emulate MSR IA32_UMWAIT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 18 Jun 2019 10:40:18 +0800
From:   Tao Xu <tao3.xu@...el.com>
To:     Radim Krčmář <rkrcmar@...hat.com>,
        Xiaoyao Li <xiaoyao.li@...ux.intel.com>
Cc:     pbonzini@...hat.com, corbet@....net, tglx@...utronix.de,
        mingo@...hat.com, bp@...en8.de, hpa@...or.com,
        sean.j.christopherson@...el.com, fenghua.yu@...el.com,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        jingqi.liu@...el.com
Subject: Re: [PATCH RESEND v3 2/3] KVM: vmx: Emulate MSR IA32_UMWAIT_CONTROL

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.