All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Don't map nested_vmcb on INTERCEPT_MSR_PROT
@ 2009-09-03 14:12 Alexander Graf
  0 siblings, 0 replies; 6+ messages in thread
From: Alexander Graf @ 2009-09-03 14:12 UTC (permalink / raw)
  To: kvm; +Cc: Joerg Roedel

Thanks to Joerg's previous series of cleanups, we now have almost all
information we need to decide what to do on #VMEXIT because we get
the variables from the VMCB on VMRUN.

Unfortunately there's one piece that slipped through the conversion,
namely the MSR intercept which still tries to map the nested VMCB
to find out if MSRs are intercepted.

So let's use the cached value, removing the need for two atomic maps
(which breaks anyways) and fix an oops along the way.

CC: Joerg Roedel <joerg.roedel@amd.com>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/x86/kvm/svm.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 2df9b45..e597961 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1427,18 +1427,16 @@ static bool nested_svm_exit_handled_msr(struct vcpu_svm *svm)
 {
 	u32 param = svm->vmcb->control.exit_info_1 & 1;
 	u32 msr = svm->vcpu.arch.regs[VCPU_REGS_RCX];
-	struct vmcb *nested_vmcb;
 	bool ret = false;
 	u32 t0, t1;
 	u8 *msrpm;
 
-	nested_vmcb = nested_svm_map(svm, svm->nested.vmcb, KM_USER0);
-	msrpm       = nested_svm_map(svm, svm->nested.vmcb_msrpm, KM_USER1);
+	msrpm       = nested_svm_map(svm, svm->nested.vmcb_msrpm, KM_USER0);
 
-	if (!nested_vmcb || !msrpm)
+	if (!msrpm)
 		goto out;
 
-	if (!(nested_vmcb->control.intercept & (1ULL << INTERCEPT_MSR_PROT)))
+	if (!(svm->nested.intercept & (1ULL << INTERCEPT_MSR_PROT)))
 		return 0;
 
 	switch (msr) {
@@ -1464,8 +1462,7 @@ static bool nested_svm_exit_handled_msr(struct vcpu_svm *svm)
 	ret = msrpm[t1] & ((1 << param) << t0);
 
 out:
-	nested_svm_unmap(nested_vmcb, KM_USER0);
-	nested_svm_unmap(msrpm, KM_USER1);
+	nested_svm_unmap(msrpm, KM_USER0);
 
 	return ret;
 }
-- 
1.6.0.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] Don't map nested_vmcb on INTERCEPT_MSR_PROT
  2009-09-04 22:59   ` Alexander Graf
@ 2009-09-05  0:29     ` Marcelo Tosatti
  0 siblings, 0 replies; 6+ messages in thread
From: Marcelo Tosatti @ 2009-09-05  0:29 UTC (permalink / raw)
  To: Alexander Graf; +Cc: kvm, Joerg Roedel

On Sat, Sep 05, 2009 at 12:59:26AM +0200, Alexander Graf wrote:
>
> Am 04.09.2009 um 19:05 schrieb Marcelo Tosatti <mtosatti@redhat.com>:
>
>> On Thu, Sep 03, 2009 at 04:51:52PM +0200, Alexander Graf wrote:
>>> Thanks to Joerg's previous series of cleanups, we now have almost all
>>> information we need to decide what to do on #VMEXIT because we get
>>> the variables from the VMCB on VMRUN.
>>>
>>> Unfortunately there's one piece that slipped through the conversion,
>>> namely the MSR intercept which still tries to map the nested VMCB
>>> to find out if MSRs are intercepted.
>>>
>>> So let's use the cached value, removing the need for two atomic maps
>>> (which breaks anyways) and fix an oops along the way.
>>>
>>> CC: Joerg Roedel <joerg.roedel@amd.com>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>
>> Applied, thanks.
>>
>> BTW, why nested_svm_map takes mmap_sem? Thats looks wrong.
>
> Hm, good question.
>
> As long as it's an atomic section that only touches vcpu local data  
> there's no need to lock at all, right?

Yes. gfn_to_page / get_user_pages will grab mmap_sem if required. Can
you please send a patch to drop it from nested_svm_map?


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] Don't map nested_vmcb on INTERCEPT_MSR_PROT
  2009-09-04 17:05 ` Marcelo Tosatti
@ 2009-09-04 22:59   ` Alexander Graf
  2009-09-05  0:29     ` Marcelo Tosatti
  0 siblings, 1 reply; 6+ messages in thread
From: Alexander Graf @ 2009-09-04 22:59 UTC (permalink / raw)
  To: Marcelo Tosatti; +Cc: kvm, Joerg Roedel


Am 04.09.2009 um 19:05 schrieb Marcelo Tosatti <mtosatti@redhat.com>:

> On Thu, Sep 03, 2009 at 04:51:52PM +0200, Alexander Graf wrote:
>> Thanks to Joerg's previous series of cleanups, we now have almost all
>> information we need to decide what to do on #VMEXIT because we get
>> the variables from the VMCB on VMRUN.
>>
>> Unfortunately there's one piece that slipped through the conversion,
>> namely the MSR intercept which still tries to map the nested VMCB
>> to find out if MSRs are intercepted.
>>
>> So let's use the cached value, removing the need for two atomic maps
>> (which breaks anyways) and fix an oops along the way.
>>
>> CC: Joerg Roedel <joerg.roedel@amd.com>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>
> Applied, thanks.
>
> BTW, why nested_svm_map takes mmap_sem? Thats looks wrong.

Hm, good question.

As long as it's an atomic section that only touches vcpu local data  
there's no need to lock at all, right?

Alex

>
>>
>> ---
>>
>> v1 -> v2:
>>
>>  - Don't break when MSR is not intercepted
>> ---
>> arch/x86/kvm/svm.c |   15 ++++++---------
>> 1 files changed, 6 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 2df9b45..a5f90c7 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -1427,19 +1427,17 @@ static bool nested_svm_exit_handled_msr 
>> (struct vcpu_svm *svm)
>> {
>>    u32 param = svm->vmcb->control.exit_info_1 & 1;
>>    u32 msr = svm->vcpu.arch.regs[VCPU_REGS_RCX];
>> -    struct vmcb *nested_vmcb;
>>    bool ret = false;
>>    u32 t0, t1;
>>    u8 *msrpm;
>>
>> -    nested_vmcb = nested_svm_map(svm, svm->nested.vmcb, KM_USER0);
>> -    msrpm       = nested_svm_map(svm, svm->nested.vmcb_msrpm,  
>> KM_USER1);
>> +    if (!(svm->nested.intercept & (1ULL << INTERCEPT_MSR_PROT)))
>> +        return false;
>>
>> -    if (!nested_vmcb || !msrpm)
>> -        goto out;
>> +    msrpm = nested_svm_map(svm, svm->nested.vmcb_msrpm, KM_USER0);
>>
>> -    if (!(nested_vmcb->control.intercept & (1ULL <<  
>> INTERCEPT_MSR_PROT)))
>> -        return 0;
>> +    if (!msrpm)
>> +        goto out;
>>
>>    switch (msr) {
>>    case 0 ... 0x1fff:
>> @@ -1464,8 +1462,7 @@ static bool nested_svm_exit_handled_msr 
>> (struct vcpu_svm *svm)
>>    ret = msrpm[t1] & ((1 << param) << t0);
>>
>> out:
>> -    nested_svm_unmap(nested_vmcb, KM_USER0);
>> -    nested_svm_unmap(msrpm, KM_USER1);
>> +    nested_svm_unmap(msrpm, KM_USER0);
>>
>>    return ret;
>> }
>> -- 
>> 1.6.0.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] Don't map nested_vmcb on INTERCEPT_MSR_PROT
  2009-09-03 14:51 Alexander Graf
  2009-09-03 16:00 ` Joerg Roedel
@ 2009-09-04 17:05 ` Marcelo Tosatti
  2009-09-04 22:59   ` Alexander Graf
  1 sibling, 1 reply; 6+ messages in thread
From: Marcelo Tosatti @ 2009-09-04 17:05 UTC (permalink / raw)
  To: Alexander Graf; +Cc: kvm, Joerg Roedel

On Thu, Sep 03, 2009 at 04:51:52PM +0200, Alexander Graf wrote:
> Thanks to Joerg's previous series of cleanups, we now have almost all
> information we need to decide what to do on #VMEXIT because we get
> the variables from the VMCB on VMRUN.
> 
> Unfortunately there's one piece that slipped through the conversion,
> namely the MSR intercept which still tries to map the nested VMCB
> to find out if MSRs are intercepted.
> 
> So let's use the cached value, removing the need for two atomic maps
> (which breaks anyways) and fix an oops along the way.
> 
> CC: Joerg Roedel <joerg.roedel@amd.com>
> Signed-off-by: Alexander Graf <agraf@suse.de>

Applied, thanks.

BTW, why nested_svm_map takes mmap_sem? Thats looks wrong.

> 
> ---
> 
> v1 -> v2:
> 
>   - Don't break when MSR is not intercepted
> ---
>  arch/x86/kvm/svm.c |   15 ++++++---------
>  1 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 2df9b45..a5f90c7 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1427,19 +1427,17 @@ static bool nested_svm_exit_handled_msr(struct vcpu_svm *svm)
>  {
>  	u32 param = svm->vmcb->control.exit_info_1 & 1;
>  	u32 msr = svm->vcpu.arch.regs[VCPU_REGS_RCX];
> -	struct vmcb *nested_vmcb;
>  	bool ret = false;
>  	u32 t0, t1;
>  	u8 *msrpm;
>  
> -	nested_vmcb = nested_svm_map(svm, svm->nested.vmcb, KM_USER0);
> -	msrpm       = nested_svm_map(svm, svm->nested.vmcb_msrpm, KM_USER1);
> +	if (!(svm->nested.intercept & (1ULL << INTERCEPT_MSR_PROT)))
> +		return false;
>  
> -	if (!nested_vmcb || !msrpm)
> -		goto out;
> +	msrpm = nested_svm_map(svm, svm->nested.vmcb_msrpm, KM_USER0);
>  
> -	if (!(nested_vmcb->control.intercept & (1ULL << INTERCEPT_MSR_PROT)))
> -		return 0;
> +	if (!msrpm)
> +		goto out;
>  
>  	switch (msr) {
>  	case 0 ... 0x1fff:
> @@ -1464,8 +1462,7 @@ static bool nested_svm_exit_handled_msr(struct vcpu_svm *svm)
>  	ret = msrpm[t1] & ((1 << param) << t0);
>  
>  out:
> -	nested_svm_unmap(nested_vmcb, KM_USER0);
> -	nested_svm_unmap(msrpm, KM_USER1);
> +	nested_svm_unmap(msrpm, KM_USER0);
>  
>  	return ret;
>  }
> -- 
> 1.6.0.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] Don't map nested_vmcb on INTERCEPT_MSR_PROT
  2009-09-03 14:51 Alexander Graf
@ 2009-09-03 16:00 ` Joerg Roedel
  2009-09-04 17:05 ` Marcelo Tosatti
  1 sibling, 0 replies; 6+ messages in thread
From: Joerg Roedel @ 2009-09-03 16:00 UTC (permalink / raw)
  To: Alexander Graf; +Cc: kvm

Indeed. Good catch. Thanks :-)

On Thu, Sep 03, 2009 at 04:51:52PM +0200, Alexander Graf wrote:
> Thanks to Joerg's previous series of cleanups, we now have almost all
> information we need to decide what to do on #VMEXIT because we get
> the variables from the VMCB on VMRUN.
> 
> Unfortunately there's one piece that slipped through the conversion,
> namely the MSR intercept which still tries to map the nested VMCB
> to find out if MSRs are intercepted.
> 
> So let's use the cached value, removing the need for two atomic maps
> (which breaks anyways) and fix an oops along the way.
> 
> CC: Joerg Roedel <joerg.roedel@amd.com>
> Signed-off-by: Alexander Graf <agraf@suse.de>

Acked-by: Joerg Roedel <joerg.roedel@amd.com>

> 
> ---
> 
> v1 -> v2:
> 
>   - Don't break when MSR is not intercepted
> ---
>  arch/x86/kvm/svm.c |   15 ++++++---------
>  1 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 2df9b45..a5f90c7 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1427,19 +1427,17 @@ static bool nested_svm_exit_handled_msr(struct vcpu_svm *svm)
>  {
>  	u32 param = svm->vmcb->control.exit_info_1 & 1;
>  	u32 msr = svm->vcpu.arch.regs[VCPU_REGS_RCX];
> -	struct vmcb *nested_vmcb;
>  	bool ret = false;
>  	u32 t0, t1;
>  	u8 *msrpm;
>  
> -	nested_vmcb = nested_svm_map(svm, svm->nested.vmcb, KM_USER0);
> -	msrpm       = nested_svm_map(svm, svm->nested.vmcb_msrpm, KM_USER1);
> +	if (!(svm->nested.intercept & (1ULL << INTERCEPT_MSR_PROT)))
> +		return false;
>  
> -	if (!nested_vmcb || !msrpm)
> -		goto out;
> +	msrpm = nested_svm_map(svm, svm->nested.vmcb_msrpm, KM_USER0);
>  
> -	if (!(nested_vmcb->control.intercept & (1ULL << INTERCEPT_MSR_PROT)))
> -		return 0;
> +	if (!msrpm)
> +		goto out;
>  
>  	switch (msr) {
>  	case 0 ... 0x1fff:
> @@ -1464,8 +1462,7 @@ static bool nested_svm_exit_handled_msr(struct vcpu_svm *svm)
>  	ret = msrpm[t1] & ((1 << param) << t0);
>  
>  out:
> -	nested_svm_unmap(nested_vmcb, KM_USER0);
> -	nested_svm_unmap(msrpm, KM_USER1);
> +	nested_svm_unmap(msrpm, KM_USER0);
>  
>  	return ret;
>  }
> -- 
> 1.6.0.2
> 
> 

-- 


^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] Don't map nested_vmcb on INTERCEPT_MSR_PROT
@ 2009-09-03 14:51 Alexander Graf
  2009-09-03 16:00 ` Joerg Roedel
  2009-09-04 17:05 ` Marcelo Tosatti
  0 siblings, 2 replies; 6+ messages in thread
From: Alexander Graf @ 2009-09-03 14:51 UTC (permalink / raw)
  To: kvm; +Cc: Joerg Roedel

Thanks to Joerg's previous series of cleanups, we now have almost all
information we need to decide what to do on #VMEXIT because we get
the variables from the VMCB on VMRUN.

Unfortunately there's one piece that slipped through the conversion,
namely the MSR intercept which still tries to map the nested VMCB
to find out if MSRs are intercepted.

So let's use the cached value, removing the need for two atomic maps
(which breaks anyways) and fix an oops along the way.

CC: Joerg Roedel <joerg.roedel@amd.com>
Signed-off-by: Alexander Graf <agraf@suse.de>

---

v1 -> v2:

  - Don't break when MSR is not intercepted
---
 arch/x86/kvm/svm.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 2df9b45..a5f90c7 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1427,19 +1427,17 @@ static bool nested_svm_exit_handled_msr(struct vcpu_svm *svm)
 {
 	u32 param = svm->vmcb->control.exit_info_1 & 1;
 	u32 msr = svm->vcpu.arch.regs[VCPU_REGS_RCX];
-	struct vmcb *nested_vmcb;
 	bool ret = false;
 	u32 t0, t1;
 	u8 *msrpm;
 
-	nested_vmcb = nested_svm_map(svm, svm->nested.vmcb, KM_USER0);
-	msrpm       = nested_svm_map(svm, svm->nested.vmcb_msrpm, KM_USER1);
+	if (!(svm->nested.intercept & (1ULL << INTERCEPT_MSR_PROT)))
+		return false;
 
-	if (!nested_vmcb || !msrpm)
-		goto out;
+	msrpm = nested_svm_map(svm, svm->nested.vmcb_msrpm, KM_USER0);
 
-	if (!(nested_vmcb->control.intercept & (1ULL << INTERCEPT_MSR_PROT)))
-		return 0;
+	if (!msrpm)
+		goto out;
 
 	switch (msr) {
 	case 0 ... 0x1fff:
@@ -1464,8 +1462,7 @@ static bool nested_svm_exit_handled_msr(struct vcpu_svm *svm)
 	ret = msrpm[t1] & ((1 << param) << t0);
 
 out:
-	nested_svm_unmap(nested_vmcb, KM_USER0);
-	nested_svm_unmap(msrpm, KM_USER1);
+	nested_svm_unmap(msrpm, KM_USER0);
 
 	return ret;
 }
-- 
1.6.0.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-09-05  0:30 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-09-03 14:12 [PATCH] Don't map nested_vmcb on INTERCEPT_MSR_PROT Alexander Graf
2009-09-03 14:51 Alexander Graf
2009-09-03 16:00 ` Joerg Roedel
2009-09-04 17:05 ` Marcelo Tosatti
2009-09-04 22:59   ` Alexander Graf
2009-09-05  0:29     ` Marcelo Tosatti

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.