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

[Avi Kivity <avi@redhat.com>]

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