On Mon, Sep 24, 2018 at 02:43:55PM -0700, Nadav Amit wrote:
>at 1:02 PM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>
>> On Mon, 24 Sep 2018, Sean Christopherson wrote:
>>
>>> On Sat, 2018-09-22 at 09:56 +0800, kvm-owner@xxxxxxxxxxxxxxx wrote:
>>>> vmx->vcpu.arch.cr0 will be set in vmx_set_cr0().
>>>>
>>>> This patch removes duplicate cr0 set in vmx_vcpu_reset().
>>>>
>>>> Signed-off-by: Wei Yang <richard.weiyang@xxxxxxxxx>
>>>> ---
>>>> arch/x86/kvm/vmx.c | 1 -
>>>> 1 file changed, 1 deletion(-)
>>>>
>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>> index 1519f030fd73..b1e1d63a4970 100644
>>>> --- a/arch/x86/kvm/vmx.c
>>>> +++ b/arch/x86/kvm/vmx.c
>>>> @@ -6734,7 +6734,6 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>>>> vmcs_write16(VIRTUAL_PROCESSOR_ID, vmx->vpid);
>>>>
>>>> cr0 = X86_CR0_NW | X86_CR0_CD | X86_CR0_ET;
>>>> - vmx->vcpu.arch.cr0 = cr0;
>>>
>>> Initializing arch.cr0 prior to vmx_set_cr0() is necessary because it
>>> might be queried by vmx_set_cr0(), e.g. via is_paging(). A stale cr0
>>> could trigger side effects in vmx_set_cr0() related to toggling cr0
>>> bits, which we don't want.
>>
>> And these side effects are completely undocumented. So if that store needs
>> to happen before calling vmx_set_cr0() then this really wants a comment.
>
>Whoever is going to deal with it - you may want to have a look at the commit
>message of f24632475d4f ("KVM: x86: fix ordering of cr0 initialization code
>in vmx_cpu_reset???), which fixed a bug that I caused by reversing the order
>of the cr0 initialization.
>
Thanks for your comments and seems this change is proven to be wrong.
We should keep it.
>Regards,
>Nadav
--
Wei Yang
Help you, Help me
