| Message ID | 20180922015639.12666-2-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] KVM: x86: replace KVM_PAGES_PER_HPAGE with KVM_HPAGE_GFN_SIZE | expand |
On 09/21/2018 06:56 PM, Wei Yang 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@gmail.com> > --- > 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; > vmx_set_cr0(vcpu, cr0); /* enter rmode */ > vmx_set_cr4(vcpu, 0); > vmx_set_efer(vcpu, 0); Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
On Sat, 2018-09-22 at 09:56 +0800, kvm-owner@vger.kernel.org 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@gmail.com> > --- > 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. > vmx_set_cr0(vcpu, cr0); /* enter rmode */ > vmx_set_cr4(vcpu, 0); > vmx_set_efer(vcpu, 0);
On Mon, 24 Sep 2018, Sean Christopherson wrote: > On Sat, 2018-09-22 at 09:56 +0800, kvm-owner@vger.kernel.org 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@gmail.com> > > --- > > 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. Thanks, tglx
at 1:02 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > On Mon, 24 Sep 2018, Sean Christopherson wrote: > >> On Sat, 2018-09-22 at 09:56 +0800, kvm-owner@vger.kernel.org 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@gmail.com> >>> --- >>> 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. Regards, Nadav
On Mon, Sep 24, 2018 at 02:43:55PM -0700, Nadav Amit wrote: >at 1:02 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > >> On Mon, 24 Sep 2018, Sean Christopherson wrote: >> >>> On Sat, 2018-09-22 at 09:56 +0800, kvm-owner@vger.kernel.org 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@gmail.com> >>>> --- >>>> 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
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; vmx_set_cr0(vcpu, cr0); /* enter rmode */ vmx_set_cr4(vcpu, 0); vmx_set_efer(vcpu, 0);
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@gmail.com> --- arch/x86/kvm/vmx.c | 1 - 1 file changed, 1 deletion(-)