linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] KVM: SVM: delay svm_vcpu_init_msrpm after svm->vmcb is initialized
@ 2021-07-26 16:58 Paolo Bonzini
  2021-07-27  0:41 ` Vineeth Pillai
  2021-07-27 15:23 ` Vitaly Kuznetsov
  0 siblings, 2 replies; 6+ messages in thread
From: Paolo Bonzini @ 2021-07-26 16:58 UTC (permalink / raw)
  To: linux-kernel, kvm; +Cc: Dan Carpenter, Vineeth Pillai

Right now, svm_hv_vmcb_dirty_nested_enlightenments has an incorrect
dereference of vmcb->control.reserved_sw before the vmcb is checked
for being non-NULL.  The compiler is usually sinking the dereference
after the check; instead of doing this ourselves in the source,
ensure that svm_hv_vmcb_dirty_nested_enlightenments is only called
with a non-NULL VMCB.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Vineeth Pillai <viremana@linux.microsoft.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
[Untested for now due to issues with my AMD machine. - Paolo]
---
 arch/x86/kvm/svm/svm.c          | 4 ++--
 arch/x86/kvm/svm/svm_onhyperv.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 9a6987549e1b..4bcb95bb8ed7 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1406,8 +1406,6 @@ static int svm_create_vcpu(struct kvm_vcpu *vcpu)
 		goto error_free_vmsa_page;
 	}
 
-	svm_vcpu_init_msrpm(vcpu, svm->msrpm);
-
 	svm->vmcb01.ptr = page_address(vmcb01_page);
 	svm->vmcb01.pa = __sme_set(page_to_pfn(vmcb01_page) << PAGE_SHIFT);
 
@@ -1419,6 +1417,8 @@ static int svm_create_vcpu(struct kvm_vcpu *vcpu)
 	svm_switch_vmcb(svm, &svm->vmcb01);
 	init_vmcb(vcpu);
 
+	svm_vcpu_init_msrpm(vcpu, svm->msrpm);
+
 	svm_init_osvw(vcpu);
 	vcpu->arch.microcode_version = 0x01000065;
 
diff --git a/arch/x86/kvm/svm/svm_onhyperv.h b/arch/x86/kvm/svm/svm_onhyperv.h
index 9b9a55abc29f..c53b8bf8d013 100644
--- a/arch/x86/kvm/svm/svm_onhyperv.h
+++ b/arch/x86/kvm/svm/svm_onhyperv.h
@@ -89,7 +89,7 @@ static inline void svm_hv_vmcb_dirty_nested_enlightenments(
 	 * as we mark it dirty unconditionally towards end of vcpu
 	 * init phase.
 	 */
-	if (vmcb && vmcb_is_clean(vmcb, VMCB_HV_NESTED_ENLIGHTENMENTS) &&
+	if (vmcb_is_clean(vmcb, VMCB_HV_NESTED_ENLIGHTENMENTS) &&
 	    hve->hv_enlightenments_control.msr_bitmap)
 		vmcb_mark_dirty(vmcb, VMCB_HV_NESTED_ENLIGHTENMENTS);
 }
-- 
2.27.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] KVM: SVM: delay svm_vcpu_init_msrpm after svm->vmcb is initialized
  2021-07-26 16:58 [PATCH] KVM: SVM: delay svm_vcpu_init_msrpm after svm->vmcb is initialized Paolo Bonzini
@ 2021-07-27  0:41 ` Vineeth Pillai
  2021-07-27 15:23 ` Vitaly Kuznetsov
  1 sibling, 0 replies; 6+ messages in thread
From: Vineeth Pillai @ 2021-07-27  0:41 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Dan Carpenter, linux-kernel, kvm

Hi Paulo,

Thanks a lot for fixing this.

>   
> diff --git a/arch/x86/kvm/svm/svm_onhyperv.h b/arch/x86/kvm/svm/svm_onhyperv.h
> index 9b9a55abc29f..c53b8bf8d013 100644
> --- a/arch/x86/kvm/svm/svm_onhyperv.h
> +++ b/arch/x86/kvm/svm/svm_onhyperv.h
> @@ -89,7 +89,7 @@ static inline void svm_hv_vmcb_dirty_nested_enlightenments(
>   	 * as we mark it dirty unconditionally towards end of vcpu
>   	 * init phase.
>   	 */
> -	if (vmcb && vmcb_is_clean(vmcb, VMCB_HV_NESTED_ENLIGHTENMENTS) &&
> +	if (vmcb_is_clean(vmcb, VMCB_HV_NESTED_ENLIGHTENMENTS) &&
>   	    hve->hv_enlightenments_control.msr_bitmap)
>   		vmcb_mark_dirty(vmcb, VMCB_HV_NESTED_ENLIGHTENMENTS);
>   }
The changes looks good to me. Could you please remove the above comment
as well while you are at it.

Many Thanks,
Vineeth

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] KVM: SVM: delay svm_vcpu_init_msrpm after svm->vmcb is initialized
  2021-07-26 16:58 [PATCH] KVM: SVM: delay svm_vcpu_init_msrpm after svm->vmcb is initialized Paolo Bonzini
  2021-07-27  0:41 ` Vineeth Pillai
@ 2021-07-27 15:23 ` Vitaly Kuznetsov
  2021-07-28 20:18   ` Vineeth Pillai
  1 sibling, 1 reply; 6+ messages in thread
From: Vitaly Kuznetsov @ 2021-07-27 15:23 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Dan Carpenter, Vineeth Pillai, linux-kernel, kvm

Paolo Bonzini <pbonzini@redhat.com> writes:

> Right now, svm_hv_vmcb_dirty_nested_enlightenments has an incorrect
> dereference of vmcb->control.reserved_sw before the vmcb is checked
> for being non-NULL.  The compiler is usually sinking the dereference
> after the check; instead of doing this ourselves in the source,
> ensure that svm_hv_vmcb_dirty_nested_enlightenments is only called
> with a non-NULL VMCB.
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: Vineeth Pillai <viremana@linux.microsoft.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> [Untested for now due to issues with my AMD machine. - Paolo]

At least this doesn't seem to break kvm-amd on bare metal, so

Tested-by: Vitaly Kuznetsov <vkuznets@redhat.com>

> ---
>  arch/x86/kvm/svm/svm.c          | 4 ++--
>  arch/x86/kvm/svm/svm_onhyperv.h | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 9a6987549e1b..4bcb95bb8ed7 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1406,8 +1406,6 @@ static int svm_create_vcpu(struct kvm_vcpu *vcpu)
>  		goto error_free_vmsa_page;
>  	}
>  
> -	svm_vcpu_init_msrpm(vcpu, svm->msrpm);
> -
>  	svm->vmcb01.ptr = page_address(vmcb01_page);
>  	svm->vmcb01.pa = __sme_set(page_to_pfn(vmcb01_page) << PAGE_SHIFT);
>  
> @@ -1419,6 +1417,8 @@ static int svm_create_vcpu(struct kvm_vcpu *vcpu)
>  	svm_switch_vmcb(svm, &svm->vmcb01);
>  	init_vmcb(vcpu);
>  
> +	svm_vcpu_init_msrpm(vcpu, svm->msrpm);
> +
>  	svm_init_osvw(vcpu);
>  	vcpu->arch.microcode_version = 0x01000065;
>  
> diff --git a/arch/x86/kvm/svm/svm_onhyperv.h b/arch/x86/kvm/svm/svm_onhyperv.h
> index 9b9a55abc29f..c53b8bf8d013 100644
> --- a/arch/x86/kvm/svm/svm_onhyperv.h
> +++ b/arch/x86/kvm/svm/svm_onhyperv.h
> @@ -89,7 +89,7 @@ static inline void svm_hv_vmcb_dirty_nested_enlightenments(
>  	 * as we mark it dirty unconditionally towards end of vcpu
>  	 * init phase.
>  	 */
> -	if (vmcb && vmcb_is_clean(vmcb, VMCB_HV_NESTED_ENLIGHTENMENTS) &&
> +	if (vmcb_is_clean(vmcb, VMCB_HV_NESTED_ENLIGHTENMENTS) &&
>  	    hve->hv_enlightenments_control.msr_bitmap)
>  		vmcb_mark_dirty(vmcb, VMCB_HV_NESTED_ENLIGHTENMENTS);
>  }

-- 
Vitaly


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] KVM: SVM: delay svm_vcpu_init_msrpm after svm->vmcb is initialized
  2021-07-27 15:23 ` Vitaly Kuznetsov
@ 2021-07-28 20:18   ` Vineeth Pillai
  2021-07-29 16:30     ` Paolo Bonzini
  0 siblings, 1 reply; 6+ messages in thread
From: Vineeth Pillai @ 2021-07-28 20:18 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Dan Carpenter, linux-kernel, kvm, Vitaly Kuznetsov


On 7/27/2021 11:23 AM, Vitaly Kuznetsov wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> Right now, svm_hv_vmcb_dirty_nested_enlightenments has an incorrect
>> dereference of vmcb->control.reserved_sw before the vmcb is checked
>> for being non-NULL.  The compiler is usually sinking the dereference
>> after the check; instead of doing this ourselves in the source,
>> ensure that svm_hv_vmcb_dirty_nested_enlightenments is only called
>> with a non-NULL VMCB.
>>
>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>> Cc: Vineeth Pillai <viremana@linux.microsoft.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> [Untested for now due to issues with my AMD machine. - Paolo]
Finally got hold of an AMD machine and tested nested virt: windows on 
linux on
windows with the patches applied. Did basic boot and minimal verification.

Tested-by: Vineeth Pillai <viremana@linux.microsoft.com>

Thanks,
Vineeth


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] KVM: SVM: delay svm_vcpu_init_msrpm after svm->vmcb is initialized
  2021-07-28 20:18   ` Vineeth Pillai
@ 2021-07-29 16:30     ` Paolo Bonzini
  0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2021-07-29 16:30 UTC (permalink / raw)
  To: Vineeth Pillai; +Cc: Dan Carpenter, linux-kernel, kvm, Vitaly Kuznetsov

On 28/07/21 22:18, Vineeth Pillai wrote:
> 
> On 7/27/2021 11:23 AM, Vitaly Kuznetsov wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> Right now, svm_hv_vmcb_dirty_nested_enlightenments has an incorrect
>>> dereference of vmcb->control.reserved_sw before the vmcb is checked
>>> for being non-NULL.  The compiler is usually sinking the dereference
>>> after the check; instead of doing this ourselves in the source,
>>> ensure that svm_hv_vmcb_dirty_nested_enlightenments is only called
>>> with a non-NULL VMCB.
>>>
>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>>> Cc: Vineeth Pillai <viremana@linux.microsoft.com>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> [Untested for now due to issues with my AMD machine. - Paolo]
> Finally got hold of an AMD machine and tested nested virt: windows on 
> linux on
> windows with the patches applied. Did basic boot and minimal verification.
> 
> Tested-by: Vineeth Pillai <viremana@linux.microsoft.com>

Thanks!  In the meanwhile I had fixed my machine too. :)

Paolo


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] KVM: SVM: delay svm_vcpu_init_msrpm after svm->vmcb is initialized
@ 2021-07-26 16:49 Paolo Bonzini
  0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2021-07-26 16:49 UTC (permalink / raw)
  To: linux-kernel, kvm; +Cc: Dan Carpenter, Vineeth Pillai

Right now, svm_hv_vmcb_dirty_nested_enlightenments has an incorrect
dereference of vmcb->control.reserved_sw before the vmcb is checked
for being non-NULL.  The compiler is usually sinking the dereference
after the check; instead of doing this ourselves in the source,
ensure that svm_hv_vmcb_dirty_nested_enlightenments is only called
with a non-NULL VMCB.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Vineeth Pillai <viremana@linux.microsoft.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
[Untested for now due to issues with my AMD machine. - Paolo]
---
 arch/x86/kvm/svm/svm.c          | 4 ++--
 arch/x86/kvm/svm/svm_onhyperv.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 9a6987549e1b..4bcb95bb8ed7 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1406,8 +1406,6 @@ static int svm_create_vcpu(struct kvm_vcpu *vcpu)
 		goto error_free_vmsa_page;
 	}
 
-	svm_vcpu_init_msrpm(vcpu, svm->msrpm);
-
 	svm->vmcb01.ptr = page_address(vmcb01_page);
 	svm->vmcb01.pa = __sme_set(page_to_pfn(vmcb01_page) << PAGE_SHIFT);
 
@@ -1419,6 +1417,8 @@ static int svm_create_vcpu(struct kvm_vcpu *vcpu)
 	svm_switch_vmcb(svm, &svm->vmcb01);
 	init_vmcb(vcpu);
 
+	svm_vcpu_init_msrpm(vcpu, svm->msrpm);
+
 	svm_init_osvw(vcpu);
 	vcpu->arch.microcode_version = 0x01000065;
 
diff --git a/arch/x86/kvm/svm/svm_onhyperv.h b/arch/x86/kvm/svm/svm_onhyperv.h
index 9b9a55abc29f..c53b8bf8d013 100644
--- a/arch/x86/kvm/svm/svm_onhyperv.h
+++ b/arch/x86/kvm/svm/svm_onhyperv.h
@@ -89,7 +89,7 @@ static inline void svm_hv_vmcb_dirty_nested_enlightenments(
 	 * as we mark it dirty unconditionally towards end of vcpu
 	 * init phase.
 	 */
-	if (vmcb && vmcb_is_clean(vmcb, VMCB_HV_NESTED_ENLIGHTENMENTS) &&
+	if (vmcb_is_clean(vmcb, VMCB_HV_NESTED_ENLIGHTENMENTS) &&
 	    hve->hv_enlightenments_control.msr_bitmap)
 		vmcb_mark_dirty(vmcb, VMCB_HV_NESTED_ENLIGHTENMENTS);
 }
-- 
2.27.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-07-29 16:30 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-26 16:58 [PATCH] KVM: SVM: delay svm_vcpu_init_msrpm after svm->vmcb is initialized Paolo Bonzini
2021-07-27  0:41 ` Vineeth Pillai
2021-07-27 15:23 ` Vitaly Kuznetsov
2021-07-28 20:18   ` Vineeth Pillai
2021-07-29 16:30     ` Paolo Bonzini
  -- strict thread matches above, loose matches on Subject: below --
2021-07-26 16:49 Paolo Bonzini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).