Heads-up: Nested VMX got broken by commit

Heads-up: Nested VMX got broken by commit

[Nadav Har'El]

Hi,

I just noticed that Nested VMX got broken (at least in my tests) by commit
46199f33c29533e7ad2a7d2128dc30175d1d4157.

The specific change causing the problem was:

@@ -2220,7 +2216,6 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, u32
msr_index, u64 data)
                        break;
                msr = find_msr_entry(vmx, msr_index);
                if (msr) {
-                       vmx_load_host_state(vmx);
                        msr->data = data;
                        break;
                }

And if anyone wants a quick workaround to making nested VMX work again,
returning this line fixes the problem.

I'm still trying to figure out why this line, which indeed seems unrelated
and unnecessary, is necessary for the correct functioning of nested VMX.
My (unsubstantiated) guess is that it isn't that it is actually necessary
in this point - it's just that it does something that should have been more
properly done in another place, but I've yet to figure out exactly what.
I'll send a patch when I have this figured out. If anybody else has any
guess, I'd love to hear.

Nadav.


-- 
Nadav Har'El                        |                    Tuesday, Mar 6 2012, 
nyh@math.technion.ac.il             |-----------------------------------------
Phone +972-523-790466, ICQ 13349191 |Sign in pool: "Welcome to our OOL. Notice
http://nadav.harel.org.il           |there is no P, please keep it that way."

Re: Heads-up: Nested VMX got broken by commit

[Avi Kivity]

On 03/06/2012 10:33 AM, Nadav Har'El wrote:
> Hi,
>
> I just noticed that Nested VMX got broken (at least in my tests) by commit
> 46199f33c29533e7ad2a7d2128dc30175d1d4157.
>
> The specific change causing the problem was:
>
> @@ -2220,7 +2216,6 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, u32
> msr_index, u64 data)
>                         break;
>                 msr = find_msr_entry(vmx, msr_index);
>                 if (msr) {
> -                       vmx_load_host_state(vmx);
>                         msr->data = data;
>                         break;
>                 }
>
> And if anyone wants a quick workaround to making nested VMX work again,
> returning this line fixes the problem.
>
> I'm still trying to figure out why this line, which indeed seems unrelated
> and unnecessary, is necessary for the correct functioning of nested VMX.
> My (unsubstantiated) guess is that it isn't that it is actually necessary
> in this point - it's just that it does something that should have been more
> properly done in another place, but I've yet to figure out exactly what.
> I'll send a patch when I have this figured out. If anybody else has any
> guess, I'd love to hear.

A side effect of vmx_load_host_state() is that it schedules a
vmx_save_host_state() for the next entry.  Maybe you're missing
something there.

Ah, it's probably

    for (i = 0; i < vmx->save_nmsrs; ++i)
        kvm_set_shared_msr(vmx->guest_msrs[i].index,
                   vmx->guest_msrs[i].data,
                   vmx->guest_msrs[i].mask);

What happened is that nvmx workloads write a lot to MSR_*STAR and
friends, and these are stored both in vmx->guest_msrs[] and on the cpu. 
Without the call to vmx_save_host_state(), the values were written only
to memory, not to the cpu register, and the guest saw a corrupted value.

I'll post a patch soon.

-- 
error compiling committee.c: too many arguments to function