[PATCH] KVM: x86: no need to check CPL for XSETBV on VMX

[PATCH] KVM: x86: no need to check CPL for XSETBV on VMX

[Yang Zhang]

The CPL check for XSETBV instruction is done by hardware on VMX.
It only needs by SVM.

Signed-off-by: Yang Zhang <yang.zhang.wz@gmail.com>
---
  arch/x86/kvm/svm.c | 4 +++-
  arch/x86/kvm/x86.c | 3 +--
  2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 31346a3..14680f5 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2715,7 +2715,9 @@ static int xsetbv_interception(struct vcpu_svm *svm)
  	u64 new_bv = kvm_read_edx_eax(&svm->vcpu);
  	u32 index = kvm_register_read(&svm->vcpu, VCPU_REGS_RCX);

-	if (kvm_set_xcr(&svm->vcpu, index, new_bv) == 0) {
+	if (svm_get_cpl(&svm->vcpu) != 0)
+		kvm_inject_gp(&svm->vcpu, 0);
+	else if (kvm_set_xcr(&svm->vcpu, index, new_bv) == 0) {
  		svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
  		skip_emulated_instruction(&svm->vcpu);
  	}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8f57335..3cfc59f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -709,8 +709,7 @@ static 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_x86_ops->get_cpl(vcpu) != 0 ||
-	    __kvm_set_xcr(vcpu, index, xcr)) {
+	if (__kvm_set_xcr(vcpu, index, xcr)) {
  		kvm_inject_gp(vcpu, 0);
  		return 1;
  	}
-- 
1.8.3.1

Re: [PATCH] KVM: x86: no need to check CPL for XSETBV on VMX

[Paolo Bonzini]

On 15/04/2016 03:55, Yang Zhang wrote:
> The CPL check for XSETBV instruction is done by hardware on VMX.
> It only needs by SVM.

I don't think this is performance-sensitive, hence it's simpler to keep
the code as simple as possible.  For example, if one added XSETBV
support to the emulator, your patch would introduce a bug.

Paolo

> Signed-off-by: Yang Zhang <yang.zhang.wz@gmail.com>
> ---
>  arch/x86/kvm/svm.c | 4 +++-
>  arch/x86/kvm/x86.c | 3 +--
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 31346a3..14680f5 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2715,7 +2715,9 @@ static int xsetbv_interception(struct vcpu_svm *svm)
>      u64 new_bv = kvm_read_edx_eax(&svm->vcpu);
>      u32 index = kvm_register_read(&svm->vcpu, VCPU_REGS_RCX);
> 
> -    if (kvm_set_xcr(&svm->vcpu, index, new_bv) == 0) {
> +    if (svm_get_cpl(&svm->vcpu) != 0)
> +        kvm_inject_gp(&svm->vcpu, 0);
> +    else if (kvm_set_xcr(&svm->vcpu, index, new_bv) == 0) {
>          svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
>          skip_emulated_instruction(&svm->vcpu);
>      }
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 8f57335..3cfc59f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -709,8 +709,7 @@ static 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_x86_ops->get_cpl(vcpu) != 0 ||
> -        __kvm_set_xcr(vcpu, index, xcr)) {
> +    if (__kvm_set_xcr(vcpu, index, xcr)) {
>          kvm_inject_gp(vcpu, 0);
>          return 1;
>      }

Re: [PATCH] KVM: x86: no need to check CPL for XSETBV on VMX

[Yang Zhang]

On 2016/4/15 18:11, Paolo Bonzini wrote:
>
>
> On 15/04/2016 03:55, Yang Zhang wrote:
>> The CPL check for XSETBV instruction is done by hardware on VMX.
>> It only needs by SVM.
>
> I don't think this is performance-sensitive, hence it's simpler to keep
> the code as simple as possible.  For example, if one added XSETBV
> support to the emulator, your patch would introduce a bug.

In what case we need to decode XSETBV?

>
> Paolo
>
>> Signed-off-by: Yang Zhang <yang.zhang.wz@gmail.com>
>> ---
>>   arch/x86/kvm/svm.c | 4 +++-
>>   arch/x86/kvm/x86.c | 3 +--
>>   2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 31346a3..14680f5 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -2715,7 +2715,9 @@ static int xsetbv_interception(struct vcpu_svm *svm)
>>       u64 new_bv = kvm_read_edx_eax(&svm->vcpu);
>>       u32 index = kvm_register_read(&svm->vcpu, VCPU_REGS_RCX);
>>
>> -    if (kvm_set_xcr(&svm->vcpu, index, new_bv) == 0) {
>> +    if (svm_get_cpl(&svm->vcpu) != 0)
>> +        kvm_inject_gp(&svm->vcpu, 0);
>> +    else if (kvm_set_xcr(&svm->vcpu, index, new_bv) == 0) {
>>           svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
>>           skip_emulated_instruction(&svm->vcpu);
>>       }
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 8f57335..3cfc59f 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -709,8 +709,7 @@ static 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_x86_ops->get_cpl(vcpu) != 0 ||
>> -        __kvm_set_xcr(vcpu, index, xcr)) {
>> +    if (__kvm_set_xcr(vcpu, index, xcr)) {
>>           kvm_inject_gp(vcpu, 0);
>>           return 1;
>>       }


-- 
best regards
yang

Re: [PATCH] KVM: x86: no need to check CPL for XSETBV on VMX

[Paolo Bonzini]

On 15/04/2016 12:37, Yang Zhang wrote:
>> I don't think this is performance-sensitive, hence it's simpler to keep
>> the code as simple as possible.  For example, if one added XSETBV
>> support to the emulator, your patch would introduce a bug.
> 
> In what case we need to decode XSETBV?

The emulator can be a way to unify code between vmx and svm.  It is an
alternative to writing small wrapper functions such as kvm_set_xcr.

Thanks,

Paolo

Re: [PATCH] KVM: x86: no need to check CPL for XSETBV on VMX

[Yang Zhang]

On 2016/4/15 18:43, Paolo Bonzini wrote:
>
>
> On 15/04/2016 12:37, Yang Zhang wrote:
>>> I don't think this is performance-sensitive, hence it's simpler to keep
>>> the code as simple as possible.  For example, if one added XSETBV
>>> support to the emulator, your patch would introduce a bug.
>>
>> In what case we need to decode XSETBV?
>
> The emulator can be a way to unify code between vmx and svm.  It is an
> alternative to writing small wrapper functions such as kvm_set_xcr.

I still think the correctness is important. But it's ok to leave it 
there since it doesn't cause any real problem so far. :)

-- 
best regards
yang

Re: [PATCH] KVM: x86: no need to check CPL for XSETBV on VMX

[Paolo Bonzini]

On 15/04/2016 12:55, Yang Zhang wrote:
>>
>>>> I don't think this is performance-sensitive, hence it's simpler to keep
>>>> the code as simple as possible.  For example, if one added XSETBV
>>>> support to the emulator, your patch would introduce a bug.
>>>
>>> In what case we need to decode XSETBV?
>>
>> The emulator can be a way to unify code between vmx and svm.  It is an
>> alternative to writing small wrapper functions such as kvm_set_xcr.
> 
> I still think the correctness is important.

What correctness?  You said "the CPL check is done by hardware on VMX".
 Doing the check twice is not incorrect.

Unifying code between VMX and SVM is exactly useful because it makes it
easier to have correct code.

Paolo

Re: [PATCH] KVM: x86: no need to check CPL for XSETBV on VMX

[Yang Zhang]

On 2016/4/15 19:03, Paolo Bonzini wrote:
>
>
> On 15/04/2016 12:55, Yang Zhang wrote:
>>>
>>>>> I don't think this is performance-sensitive, hence it's simpler to keep
>>>>> the code as simple as possible.  For example, if one added XSETBV
>>>>> support to the emulator, your patch would introduce a bug.
>>>>
>>>> In what case we need to decode XSETBV?
>>>
>>> The emulator can be a way to unify code between vmx and svm.  It is an
>>> alternative to writing small wrapper functions such as kvm_set_xcr.
>>
>> I still think the correctness is important.
>
> What correctness?  You said "the CPL check is done by hardware on VMX".
>   Doing the check twice is not incorrect.

Yes, you are right. It cannot be considered as correctness issue.

>
> Unifying code between VMX and SVM is exactly useful because it makes it
> easier to have correct code.
>
> Paolo
>


-- 
best regards
yang