Re: [PATCH RFC v1 2/2] i386: reorder kvm_put_sregs2() and kvm_put_nested_state() when vCPU is reset

[Maxim Levitsky]

On Wed, 2022-08-10 at 16:00 +0200, Vitaly Kuznetsov wrote:
> Setting nested state upon migration needs to happen after kvm_put_sregs2()
> to e.g. have EFER.SVME set. This, however, doesn't work for vCPU reset:
> when vCPU is in VMX root operation, certain CR bits are locked and
> kvm_put_sregs2() may fail. As nested state is fully cleaned up upon
> vCPU reset (kvm_arch_reset_vcpu() -> kvm_init_nested_state()), calling
> kvm_put_nested_state() before kvm_put_sregs2() is OK, this will ensure
> that vCPU is *not* in VMX root opertaion.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  target/i386/kvm/kvm.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 4f8dacc1d4b5..73e3880fa57b 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -4529,18 +4529,34 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
>  
>      assert(cpu_is_stopped(cpu) || qemu_cpu_is_self(cpu));
>  
> -    /* must be before kvm_put_nested_state so that EFER.SVME is set */
> +    /*
> +     * When resetting a vCPU, make sure to reset nested state first to
> +     * e.g clear VMXON state and unlock certain CR4 bits.
> +     */
> +    if (level == KVM_PUT_RESET_STATE) {
> +        ret = kvm_put_nested_state(x86_cpu);
> +        if (ret < 0) {
> +            return ret;
> +        }

I should have mentioned this, I actually already debugged the same issue while
trying to reproduce the smm int window bug.
100% my fault.

I also share the same feeling that this might be yet another 'whack a mole' and
break somewhere else, but overall it does make sense.


Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>


Best regards,
        Maxim Levitsky

> +    }
> +
>      ret = has_sregs2 ? kvm_put_sregs2(x86_cpu) : kvm_put_sregs(x86_cpu);
>      if (ret < 0) {
>          return ret;
>      }
>  
> -    if (level >= KVM_PUT_RESET_STATE) {
> +    /*
> +     * When putting full CPU state, kvm_put_nested_state() must happen after
> +     * kvm_put_sregs{,2} so that e.g. EFER.SVME is already set.
> +     */
> +    if (level == KVM_PUT_FULL_STATE) {
>          ret = kvm_put_nested_state(x86_cpu);
>          if (ret < 0) {
>              return ret;
>          }
> +    }
>  
> +    if (level >= KVM_PUT_RESET_STATE) {
>          ret = kvm_put_msr_feature_control(x86_cpu);
>          if (ret < 0) {
>              return ret;