nVMX MSR load/store feature

nVMX MSR load/store feature

[Eugene Korenevsky]

Hi there,

Please DO NOT take v3 version of patchset in account. It contains bug
(missing check for MSR load/store area size in
`nested_vmx_check_msr_switch`). This bug has been fixed in v4 version
of patchset.

Now MSR load/store feature is partially covered with tests (see patch
to kvm-unit-tests in kvm-devel list).
Unfortunately the kvm-unit-tests framework cannot check for failed
VM-entry, so I skipped tests for incorrectly set MSR load/store areas
(16-byte alignment, address bits beyond address space etc). Maybe
method `vmfail_handler` should be added to `struct vmx_test`. Anyway
this can be done in the separate patch.

-- 
Eugene

Re: nVMX MSR load/store feature

[Paolo Bonzini]

On 14/12/2014 02:17, Eugene Korenevsky wrote:
> Hi there,
> 
> Please DO NOT take v3 version of patchset in account. It contains bug
> (missing check for MSR load/store area size in
> `nested_vmx_check_msr_switch`). This bug has been fixed in v4 version
> of patchset.
> 
> Now MSR load/store feature is partially covered with tests (see patch
> to kvm-unit-tests in kvm-devel list).
> Unfortunately the kvm-unit-tests framework cannot check for failed
> VM-entry, so I skipped tests for incorrectly set MSR load/store areas
> (16-byte alignment, address bits beyond address space etc). Maybe
> method `vmfail_handler` should be added to `struct vmx_test`. Anyway
> this can be done in the separate patch.

Thanks!

Paolo

Re: nVMX MSR load/store feature

[Paolo Bonzini]

On 14/12/2014 02:17, Eugene Korenevsky wrote:
> Hi there,
> 
> Please DO NOT take v3 version of patchset in account. It contains bug
> (missing check for MSR load/store area size in
> `nested_vmx_check_msr_switch`). This bug has been fixed in v4 version
> of patchset.

The diff is just

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index d6fe958a0403..09ccf6c09435 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -8305,6 +8305,8 @@ static int nested_vmx_check_msr_switch(struct kvm_vcpu *vcpu,
 		WARN_ON(1);
 		return -EINVAL;
 	}
+	if (count == 0)
+		return 0;
 	if (!IS_ALIGNED(addr, 16) || addr >> maxphyaddr ||
 	    (addr + count * sizeof(struct vmx_msr_entry) - 1) >> maxphyaddr) {
 		pr_warn_ratelimited(

right?

Paolo

Re: nVMX MSR load/store feature

[Eugene Korenevsky]

> The diff is just
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index d6fe958a0403..09ccf6c09435 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -8305,6 +8305,8 @@ static int nested_vmx_check_msr_switch(struct kvm_vcpu *vcpu,
>                 WARN_ON(1);
>                 return -EINVAL;
>         }
> +       if (count == 0)
> +               return 0;
>         if (!IS_ALIGNED(addr, 16) || addr >> maxphyaddr ||
>             (addr + count * sizeof(struct vmx_msr_entry) - 1) >> maxphyaddr) {
>                 pr_warn_ratelimited(
>
> right?

Yes. Without this check, `nested_vmx_check_msr_switch` returns -EINVAL
for count==0 and addr==0.


-- 
Eugene

Re: nVMX MSR load/store feature

[Paolo Bonzini]

On 15/12/2014 14:59, Eugene Korenevsky wrote:
>> The diff is just
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index d6fe958a0403..09ccf6c09435 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -8305,6 +8305,8 @@ static int nested_vmx_check_msr_switch(struct kvm_vcpu *vcpu,
>>                 WARN_ON(1);
>>                 return -EINVAL;
>>         }
>> +       if (count == 0)
>> +               return 0;
>>         if (!IS_ALIGNED(addr, 16) || addr >> maxphyaddr ||
>>             (addr + count * sizeof(struct vmx_msr_entry) - 1) >> maxphyaddr) {
>>                 pr_warn_ratelimited(
>>
>> right?
> 
> Yes. Without this check, `nested_vmx_check_msr_switch` returns -EINVAL
> for count==0 and addr==0.

Ok, thanks.  I'll push the patches to kvm/queue as soon as my testing
finishes.

Paolo