[1/2] KVM: VMX: Use kvm_read_cr4() to get cr4 value

  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(-)
  

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. :)