[PATCH 0/2] KVM: VMX: Clean up of vmx_set_cr4()

[PATCH 0/2] KVM: VMX: Clean up of vmx_set_cr4()

[Xiaoyao Li]

Two minor patches of vmx_set_cr4() during code inspection.

Patch 1 gets rid of the direct accessing of vcpu->arch.cr4 to avoid
stale value.

Patch 2 moves the code comment to correct place.

Xiaoyao Li (2):
  KVM: VMX: Use kvm_read_cr4() to get cr4 value
  KVM: VMX: Move the comment of CR4.MCE handling right above the code

 arch/x86/kvm/vmx/vmx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)


base-commit: d8708b80fa0e6e21bc0c9e7276ad0bccef73b6e7
prerequisite-patch-id: 5c516b453b538845ceb91a76678803ec123834ba
prerequisite-patch-id: 022904226ae3cb6766bba71c3bf407749ab5b5b2
prerequisite-patch-id: cf5655ce89a2390cd29f33c57a4fc307a6045f62
-- 
2.34.1

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

[Xiaoyao Li]

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);
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	/*
 	 * Pass through host's Machine Check Enable value to hw_cr4, which
-- 
2.34.1

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

[Sean Christopherson]

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.

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

[Xiaoyao Li]

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?

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

[Sean Christopherson]

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?

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

[Xiaoyao Li]

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

[PATCH 2/2] KVM: VMX: Move the comment of CR4.MCE handling right above the code

[Xiaoyao Li]

... to improve the code readability.

No functional change indented.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 arch/x86/kvm/vmx/vmx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index befa2486836b..c9421eb25666 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -3433,13 +3433,13 @@ void vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 {
 	unsigned long old_cr4 = kvm_read_cr4(vcpu);
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
+	unsigned long hw_cr4;
+
 	/*
 	 * Pass through host's Machine Check Enable value to hw_cr4, which
 	 * is in force while we are in guest mode.  Do not let guests control
 	 * this bit, even if host CR4.MCE == 0.
 	 */
-	unsigned long hw_cr4;
-
 	hw_cr4 = (cr4_read_shadow() & X86_CR4_MCE) | (cr4 & ~X86_CR4_MCE);
 	if (is_unrestricted_guest(vcpu))
 		hw_cr4 |= KVM_VM_CR4_ALWAYS_ON_UNRESTRICTED_GUEST;
-- 
2.34.1

Re: [PATCH 0/2] KVM: VMX: Clean up of vmx_set_cr4()

[Sean Christopherson]

On Mon, 10 Apr 2023 08:50:15 -0400, Xiaoyao Li wrote:
> Two minor patches of vmx_set_cr4() during code inspection.
> 
> Patch 1 gets rid of the direct accessing of vcpu->arch.cr4 to avoid
> stale value.
> 
> Patch 2 moves the code comment to correct place.
> 
> [...]

Applied to kvm-x86 vmx, thanks!

[1/2] KVM: VMX: Use kvm_read_cr4() to get cr4 value
      https://github.com/kvm-x86/linux/commit/334006b78ca8
[2/2] KVM: VMX: Move the comment of CR4.MCE handling right above the code
      https://github.com/kvm-x86/linux/commit/82dc11b82b00

--
https://github.com/kvm-x86/linux/tree/next
https://github.com/kvm-x86/linux/tree/fixes