[PATCH] KVM: x86: remove vcpu's CPL check in host invoked vcpu's xcr set process

[PATCH] KVM: x86: remove vcpu's CPL check in host invoked vcpu's xcr set process

[Zhanghaoyu (A)]

__kvm_set_xcr function does the CPL check when set xcr. __kvm_set_xcr is called in two flows,
one is invoked by guest, call stack shown as below,
handle_xsetbv(or xsetbv_interception)
  kvm_set_xcr
    __kvm_set_xcr
the other one is invoked by host(QEMU), call stack shown as below,
kvm_arch_vcpu_ioctl
  kvm_vcpu_ioctl_x86_set_xcrs
    __kvm_set_xcr

The former does need the CPL check, but the latter does not.

Signed-off-by: Zhang Haoyu <haoyu.zhang@huawei.com>
---
 arch/x86/kvm/x86.c                            |    5 +-
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 094b5d9..16ff44e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -582,8 +582,6 @@ int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
 		 if (index != XCR_XFEATURE_ENABLED_MASK)
 		 		 return 1;
 		 xcr0 = xcr;
-		 if (kvm_x86_ops->get_cpl(vcpu) != 0)
-		 		 return 1;
 		 if (!(xcr0 & XSTATE_FP))
 		 		 return 1;
 		 if ((xcr0 & XSTATE_YMM) && !(xcr0 & XSTATE_SSE))
@@ -597,7 +595,8 @@ int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
 
 int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
 {
-		 if (__kvm_set_xcr(vcpu, index, xcr)) {
+		 if ((kvm_x86_ops->get_cpl(vcpu) != 0) ||
+		      __kvm_set_xcr(vcpu, index, xcr)) {
 		 		 kvm_inject_gp(vcpu, 0);
 		 		 return 1;
 		 }
1.7.3.1.msysgit.0

Re: [PATCH] KVM: x86: remove vcpu's CPL check in host invoked vcpu's xcr set process

[Paolo Bonzini]

Il 14/06/2013 09:36, Zhanghaoyu (A) ha scritto:
> __kvm_set_xcr function does the CPL check when set xcr. __kvm_set_xcr is called in two flows,
> one is invoked by guest, call stack shown as below,
> handle_xsetbv(or xsetbv_interception)
>   kvm_set_xcr
>     __kvm_set_xcr
> the other one is invoked by host(QEMU), call stack shown as below,
> kvm_arch_vcpu_ioctl
>   kvm_vcpu_ioctl_x86_set_xcrs
>     __kvm_set_xcr
> 
> The former does need the CPL check, but the latter does not.
> 
> Signed-off-by: Zhang Haoyu <haoyu.zhang@huawei.com>

What does this patch fix?  I suppose it is some kind of migration
problem since you mentioned QEMU, but I'd rather be sure.  I can fix the
commit message myself when applying.

Thanks,

Paolo

Re: [PATCH] KVM: x86: remove vcpu's CPL check in host invoked vcpu's xcr set process

[Zhanghaoyu (A)]

>> __kvm_set_xcr function does the CPL check when set xcr. __kvm_set_xcr 
>> is called in two flows, one is invoked by guest, call stack shown as 
>> below, handle_xsetbv(or xsetbv_interception)
>>   kvm_set_xcr
>>     __kvm_set_xcr
>> the other one is invoked by host(QEMU), call stack shown as below, 
>> kvm_arch_vcpu_ioctl
>>   kvm_vcpu_ioctl_x86_set_xcrs
>>     __kvm_set_xcr
>> 
>> The former does need the CPL check, but the latter does not.
>> 
>> Signed-off-by: Zhang Haoyu <haoyu.zhang@huawei.com>
>
>What does this patch fix?  I suppose it is some kind of migration problem since you mentioned QEMU, but I'd rather be sure.  I can fix the commit message myself when applying.
This patch tries to fix the problem of system_reset invoked by qemu monitor[system_reset] or virsh command[virsh reset domain].
QEMU will reset the domain on receiving the reset request from qemu monitor or libvirtd, the reset flow shown as below,
main_loop_should_exit
|- pause_all_vcpus
|- cpu_synchronize_all_states
|- qemu_system_reset
|-- cpu_synchronize_all_post_reset
|--- cpu_synchronize_post_reset
|---- kvm_cpu_synchronize_post_reset
|----- kvm_arch_put_registers
|------ kvm_put_xcrs
|------- kvm_vcpu_ioctl(CPU(cpu), KVM_SET_XCRS, &xcrs)
Above IOCTL syscall traps to kernel-space, KVM hypervisor then deal with the xcr set request,
kvm_arch_vcpu_ioctl
|- kvm_vcpu_ioctl_x86_set_xcrs
|-- __kvm_set_xcr
|--- if (kvm_x86_ops->get_cpl(vcpu) != 0)
          return 1;
It's quite probably happened that vcpu's CPL is 3 while QEMU monitor doing the reset operation, which is a normal case, so the CPL check should be removed.

But __kvm_set_xcr is also called in below flow, which is invoked by guest,
handle_xsetbv(or xsetbv_interception)
|- kvm_set_xcr
|-- __kvm_set_xcr
On this case, vcpu's CPL check must be performed, if vcpu's CPL!=0, then #GP exception should be injected to VM.

Thanks,
Zhang Haoyu

>
>Thanks,
>
>Paolo

Re: [PATCH] KVM: x86: remove vcpu's CPL check in host invoked vcpu's xcr set process

[Paolo Bonzini]

Il 14/06/2013 09:36, Zhanghaoyu (A) ha scritto:
> __kvm_set_xcr function does the CPL check when set xcr. __kvm_set_xcr is called in two flows,
> one is invoked by guest, call stack shown as below,
> handle_xsetbv(or xsetbv_interception)
>   kvm_set_xcr
>     __kvm_set_xcr
> the other one is invoked by host(QEMU), call stack shown as below,
> kvm_arch_vcpu_ioctl
>   kvm_vcpu_ioctl_x86_set_xcrs
>     __kvm_set_xcr
> 
> The former does need the CPL check, but the latter does not.
> 
> Signed-off-by: Zhang Haoyu <haoyu.zhang@huawei.com>
> ---
>  arch/x86/kvm/x86.c                            |    5 +-
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 094b5d9..16ff44e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -582,8 +582,6 @@ int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
>  		 if (index != XCR_XFEATURE_ENABLED_MASK)
>  		 		 return 1;
>  		 xcr0 = xcr;
> -		 if (kvm_x86_ops->get_cpl(vcpu) != 0)
> -		 		 return 1;
>  		 if (!(xcr0 & XSTATE_FP))
>  		 		 return 1;
>  		 if ((xcr0 & XSTATE_YMM) && !(xcr0 & XSTATE_SSE))
> @@ -597,7 +595,8 @@ int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
>  
>  int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
>  {
> -		 if (__kvm_set_xcr(vcpu, index, xcr)) {
> +		 if ((kvm_x86_ops->get_cpl(vcpu) != 0) ||
> +		      __kvm_set_xcr(vcpu, index, xcr)) {
>  		 		 kvm_inject_gp(vcpu, 0);
>  		 		 return 1;
>  		 }
> 1.7.3.1.msysgit.0
> 

Applied to master, but check your setup because I had to do so manually.
 Your patch has 2 tabs + 1 space for each indentation level (??), the
file only has one tab.

This time the patch was pretty simple and may even be worth having in
3.9, but next time I'd ask you to resubmit.

Can you write a testcase for kvm-unit-tests?

Paolo

Re: [PATCH] KVM: x86: remove vcpu's CPL check in host invoked vcpu's xcr set process

[Zhanghaoyu (A)]

>> __kvm_set_xcr function does the CPL check when set xcr. __kvm_set_xcr 
>> is called in two flows, one is invoked by guest, call stack shown as 
>> below, handle_xsetbv(or xsetbv_interception)
>>   kvm_set_xcr
>>     __kvm_set_xcr
>> the other one is invoked by host(QEMU), call stack shown as below, 
>> kvm_arch_vcpu_ioctl
>>   kvm_vcpu_ioctl_x86_set_xcrs
>>     __kvm_set_xcr
>> 
>> The former does need the CPL check, but the latter does not.
>> 
>> Signed-off-by: Zhang Haoyu <haoyu.zhang@huawei.com>
>> ---
>>  arch/x86/kvm/x86.c                            |    5 +-
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 
>> 094b5d9..16ff44e 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -582,8 +582,6 @@ int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
>>  		 if (index != XCR_XFEATURE_ENABLED_MASK)
>>  		 		 return 1;
>>  		 xcr0 = xcr;
>> -		 if (kvm_x86_ops->get_cpl(vcpu) != 0)
>> -		 		 return 1;
>>  		 if (!(xcr0 & XSTATE_FP))
>>  		 		 return 1;
>>  		 if ((xcr0 & XSTATE_YMM) && !(xcr0 & XSTATE_SSE)) @@ -597,7 +595,8 
>> @@ int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
>>  
>>  int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)  {
>> -		 if (__kvm_set_xcr(vcpu, index, xcr)) {
>> +		 if ((kvm_x86_ops->get_cpl(vcpu) != 0) ||
>> +		      __kvm_set_xcr(vcpu, index, xcr)) {
>>  		 		 kvm_inject_gp(vcpu, 0);
>>  		 		 return 1;
>>  		 }
>> 1.7.3.1.msysgit.0
>> 
>
>Applied to master, but check your setup because I had to do so manually.
> Your patch has 2 tabs + 1 space for each indentation level (??), the file only has one tab.
>
>This time the patch was pretty simple and may even be worth having in 3.9, but next time I'd ask you to resubmit.
>
>Can you write a testcase for kvm-unit-tests?
>
>Paolo

So sorry for above bad format patch, this time the format is OK, but I'm not sure could the format be changed in transmission?

Signed-off-by: Zhang Haoyu <haoyu.zhang@huawei.com>
---
 arch/x86/kvm/x86.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 094b5d9..aa7bda1 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -582,8 +582,6 @@ int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
 	if (index != XCR_XFEATURE_ENABLED_MASK)
 		return 1;
 	xcr0 = xcr;
-	if (kvm_x86_ops->get_cpl(vcpu) != 0)
-		return 1;
 	if (!(xcr0 & XSTATE_FP))
 		return 1;
 	if ((xcr0 & XSTATE_YMM) && !(xcr0 & XSTATE_SSE))
@@ -597,7 +595,8 @@ int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
 
 int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
 {
-	if (__kvm_set_xcr(vcpu, index, xcr)) {
+	if ((kvm_x86_ops->get_cpl(vcpu) != 0) ||
+	    __kvm_set_xcr(vcpu, index, xcr)) {
 		kvm_inject_gp(vcpu, 0);
 		return 1;
 	}
-- 
1.7.3.1.msysgit.0

Re: [PATCH] KVM: x86: remove vcpu's CPL check in host invoked vcpu's xcr set process

[Paolo Bonzini]

Il 18/06/2013 14:39, Zhanghaoyu (A) ha scritto:
>> Applied to master, but check your setup because I had to do so manually.
>> Your patch has 2 tabs + 1 space for each indentation level (??), the file only has one tab.
>>
>> This time the patch was pretty simple and may even be worth having in 3.9, but next time I'd ask you to resubmit.
>>
>> Can you write a testcase for kvm-unit-tests?
>>
>> Paolo
> 
> So sorry for above bad format patch, this time the format is OK, but
> I'm not sure could the format be changed in transmission?

This time it is okay.

Paolo

> 
> Signed-off-by: Zhang Haoyu <haoyu.zhang@huawei.com>
> ---
>  arch/x86/kvm/x86.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 094b5d9..aa7bda1 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -582,8 +582,6 @@ int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
>  	if (index != XCR_XFEATURE_ENABLED_MASK)
>  		return 1;
>  	xcr0 = xcr;
> -	if (kvm_x86_ops->get_cpl(vcpu) != 0)
> -		return 1;
>  	if (!(xcr0 & XSTATE_FP))
>  		return 1;
>  	if ((xcr0 & XSTATE_YMM) && !(xcr0 & XSTATE_SSE))
> @@ -597,7 +595,8 @@ int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
>  
>  int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
>  {
> -	if (__kvm_set_xcr(vcpu, index, xcr)) {
> +	if ((kvm_x86_ops->get_cpl(vcpu) != 0) ||
> +	    __kvm_set_xcr(vcpu, index, xcr)) {
>  		kvm_inject_gp(vcpu, 0);
>  		return 1;
>  	}
>