| Message ID | 20230410125017.1305238-2-xiaoyao.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: VMX: Clean up of vmx_set_cr4() | expand |
On Mon, Apr 10, 2023, Xiaoyao Li wrote: > Directly use vcpu->arch.cr4 is not recommended since it gets stale value > if the cr4 is not available. > > Use kvm_read_cr4() instead to ensure correct value. > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > --- > arch/x86/kvm/vmx/vmx.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index d7bf14abdba1..befa2486836b 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -3431,7 +3431,7 @@ static bool vmx_is_valid_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) > > void vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) > { > - unsigned long old_cr4 = vcpu->arch.cr4; > + unsigned long old_cr4 = kvm_read_cr4(vcpu); Ha! I've been tempted to change this multiple times, but always thought I was just being a bit obsessive :-) Patches look good, but I'm going to hold them for 6.5 just in case this somehow causes a problem, e.g. if there's a bizzaro nested path that "works" because KVM _doesn't_ decache info from the current VMCS.
On 4/11/2023 1:11 AM, Sean Christopherson wrote: > On Mon, Apr 10, 2023, Xiaoyao Li wrote: >> Directly use vcpu->arch.cr4 is not recommended since it gets stale value >> if the cr4 is not available. >> >> Use kvm_read_cr4() instead to ensure correct value. >> >> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> >> --- >> arch/x86/kvm/vmx/vmx.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c >> index d7bf14abdba1..befa2486836b 100644 >> --- a/arch/x86/kvm/vmx/vmx.c >> +++ b/arch/x86/kvm/vmx/vmx.c >> @@ -3431,7 +3431,7 @@ static bool vmx_is_valid_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) >> >> void vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) >> { >> - unsigned long old_cr4 = vcpu->arch.cr4; >> + unsigned long old_cr4 = kvm_read_cr4(vcpu); > > Ha! I've been tempted to change this multiple times, but always thought I was > just being a bit obsessive :-) > > Patches look good, but I'm going to hold them for 6.5 just in case this somehow > causes a problem, e.g. if there's a bizzaro nested path that "works" because KVM > _doesn't_ decache info from the current VMCS. so you will put it in kvm-next after 6.4 merge windows?
On Wed, Apr 12, 2023, Xiaoyao Li wrote: > On 4/11/2023 1:11 AM, Sean Christopherson wrote: > > On Mon, Apr 10, 2023, Xiaoyao Li wrote: > > > Directly use vcpu->arch.cr4 is not recommended since it gets stale value > > > if the cr4 is not available. > > > > > > Use kvm_read_cr4() instead to ensure correct value. > > > > > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > > > --- > > > arch/x86/kvm/vmx/vmx.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > > index d7bf14abdba1..befa2486836b 100644 > > > --- a/arch/x86/kvm/vmx/vmx.c > > > +++ b/arch/x86/kvm/vmx/vmx.c > > > @@ -3431,7 +3431,7 @@ static bool vmx_is_valid_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) > > > void vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) > > > { > > > - unsigned long old_cr4 = vcpu->arch.cr4; > > > + unsigned long old_cr4 = kvm_read_cr4(vcpu); > > > > Ha! I've been tempted to change this multiple times, but always thought I was > > just being a bit obsessive :-) > > > > Patches look good, but I'm going to hold them for 6.5 just in case this somehow > > causes a problem, e.g. if there's a bizzaro nested path that "works" because KVM > > _doesn't_ decache info from the current VMCS. > > so you will put it in kvm-next after 6.4 merge windows? The likely candidate is "kvm-x86 vmx", and I probably won't apply the patches until after v6.4-rc2 (rc2 being my preferred base for the next cycle). But yes, the plan is to apply the patches after the 6.4 merge window. Are you asking because you want to know if you need to resend for 6.5? Or does the timing/location matter for some other reason, e.g. a dependency from another patch/series?
On 4/12/2023 11:03 PM, Sean Christopherson wrote: > On Wed, Apr 12, 2023, Xiaoyao Li wrote: >> On 4/11/2023 1:11 AM, Sean Christopherson wrote: >>> On Mon, Apr 10, 2023, Xiaoyao Li wrote: >>>> Directly use vcpu->arch.cr4 is not recommended since it gets stale value >>>> if the cr4 is not available. >>>> >>>> Use kvm_read_cr4() instead to ensure correct value. >>>> >>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> >>>> --- >>>> arch/x86/kvm/vmx/vmx.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c >>>> index d7bf14abdba1..befa2486836b 100644 >>>> --- a/arch/x86/kvm/vmx/vmx.c >>>> +++ b/arch/x86/kvm/vmx/vmx.c >>>> @@ -3431,7 +3431,7 @@ static bool vmx_is_valid_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) >>>> void vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) >>>> { >>>> - unsigned long old_cr4 = vcpu->arch.cr4; >>>> + unsigned long old_cr4 = kvm_read_cr4(vcpu); >>> >>> Ha! I've been tempted to change this multiple times, but always thought I was >>> just being a bit obsessive :-) >>> >>> Patches look good, but I'm going to hold them for 6.5 just in case this somehow >>> causes a problem, e.g. if there's a bizzaro nested path that "works" because KVM >>> _doesn't_ decache info from the current VMCS. >> >> so you will put it in kvm-next after 6.4 merge windows? > > The likely candidate is "kvm-x86 vmx", and I probably won't apply the patches until > after v6.4-rc2 (rc2 being my preferred base for the next cycle). But yes, the plan > is to apply the patches after the 6.4 merge window. > > Are you asking because you want to know if you need to resend for 6.5? Or does > the timing/location matter for some other reason, e.g. a dependency from another > patch/series? none of it. Just to confirm I get it. :)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index d7bf14abdba1..befa2486836b 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -3431,7 +3431,7 @@ static bool vmx_is_valid_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) void vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) { - unsigned long old_cr4 = vcpu->arch.cr4; + unsigned long old_cr4 = kvm_read_cr4(vcpu); struct vcpu_vmx *vmx = to_vmx(vcpu); /* * Pass through host's Machine Check Enable value to hw_cr4, which
Directly use vcpu->arch.cr4 is not recommended since it gets stale value if the cr4 is not available. Use kvm_read_cr4() instead to ensure correct value. Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> --- arch/x86/kvm/vmx/vmx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)