From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D1CA7C43613 for ; Thu, 20 Jun 2019 12:18:45 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id A37FA2080C for ; Thu, 20 Jun 2019 12:18:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731817AbfFTMSo (ORCPT ); Thu, 20 Jun 2019 08:18:44 -0400 Received: from mail-wr1-f67.google.com ([209.85.221.67]:35271 "EHLO mail-wr1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726874AbfFTMSn (ORCPT ); Thu, 20 Jun 2019 08:18:43 -0400 Received: by mail-wr1-f67.google.com with SMTP id m3so2829679wrv.2 for ; Thu, 20 Jun 2019 05:18:41 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:in-reply-to:references:date :message-id:mime-version; bh=4Enp7W0ESdkTTd9HBh/ANelF/DbInTnKEbgmPM5TYt4=; b=N82snS5sF9zuxOyEHFkfP5+elME+K3z0rlNSkLzkY7x4jXpQ1RtTuWiUsZuVBrOWC7 uf7O924YQQduIEcmGvkJ3GzSCz+IBjxGjYFAA7kXSz0rpZMjC+0hGWRTggsE0OfWseIX +at9v9oRYgbwbyR9qwmQqjSPsp6KiTlFtctJO6rp2i6dF6S9a2d1SB9ZG19FKuy3O57h wd0hRGwx65N3m73sd8n0FPI4eeSyttIUYD///Z1DhnvEGJQukPpAhpQI8+rYRBN5Ezg8 6EkGA7WZj6sQ22kUH+Kh3hgcuYCEEIqnuUy0lWm1yz3SBRfFpav0H4Eoz5TIkvF5IUYD FIwA== X-Gm-Message-State: APjAAAXmfClTKDJLyJvzF59FtSShtBBaWeMMGHigd+xgnd78fw7x5gTi Vzc8GqA1KiLikOCQtxahFwCxzQ== X-Google-Smtp-Source: APXvYqw0a0Atzq/xJ79+F6gAc8zeGL5JH3frFK3lbZdQKk6lxNUQWMXbkclgr9eyYNAX/Q6pZQKdcA== X-Received: by 2002:adf:eacd:: with SMTP id o13mr19533546wrn.91.1561033120821; Thu, 20 Jun 2019 05:18:40 -0700 (PDT) Received: from vitty.brq.redhat.com (nat-pool-brq-t.redhat.com. [213.175.37.10]) by smtp.gmail.com with ESMTPSA id r5sm30834313wrg.10.2019.06.20.05.18.39 (version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256); Thu, 20 Jun 2019 05:18:40 -0700 (PDT) From: Vitaly Kuznetsov To: Paolo Bonzini Cc: Aaron Lewis , linux-kernel@vger.kernel.org, kvm@vger.kernel.org Subject: Re: [PATCH] KVM: nVMX: reorganize initial steps of vmx_set_nested_state In-Reply-To: <1560959396-13969-1-git-send-email-pbonzini@redhat.com> References: <1560959396-13969-1-git-send-email-pbonzini@redhat.com> Date: Thu, 20 Jun 2019 14:18:39 +0200 Message-ID: <87zhmcfo0w.fsf@vitty.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Paolo Bonzini writes: > Commit 332d079735f5 ("KVM: nVMX: KVM_SET_NESTED_STATE - Tear down old EVMCS > state before setting new state", 2019-05-02) broke evmcs_test because the > eVMCS setup must be performed even if there is no VMXON region defined, > as long as the eVMCS bit is set in the assist page. > > While the simplest possible fix would be to add a check on > kvm_state->flags & KVM_STATE_NESTED_EVMCS in the initial "if" that > covers kvm_state->hdr.vmx.vmxon_pa == -1ull, that is quite ugly. > > Instead, this patch moves checks earlier in the function and > conditionalizes them on kvm_state->hdr.vmx.vmxon_pa, so that > vmx_set_nested_state always goes through vmx_leave_nested > and nested_enable_evmcs. > > Fixes: 332d079735f5 checkpatch.pl will likely complain here asking for full description, e.g. Fixes: 332d079735f5 ("KVM: nVMX: KVM_SET_NESTED_STATE - Tear down old EVMCS state before setting new state") There's also something wrong with the patch as it fails to apply because of (not only?) whitespace issues or maybe I'm just applying it to the wrong tree... > Cc: Aaron Lewis > Cc: Vitaly Kuznetsov > Signed-off-by: Paolo Bonzini Enlightened VMCS migration is just a 'theoretical' feature atm, we don't know if it actually works but it's good we have a selftest for it so we know when it definitely doesn't :-) Reviewed-by: Vitaly Kuznetsov > --- > arch/x86/kvm/vmx/nested.c | 26 ++++++++++-------- > .../kvm/x86_64/vmx_set_nested_state_test.c | 32 ++++++++++++++-------- > 2 files changed, 35 insertions(+), 23 deletions(-) > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index fb6d1f7b43f3..5f9c1a200201 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -5343,9 +5343,6 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu, > if (kvm_state->format != KVM_STATE_NESTED_FORMAT_VMX) > return -EINVAL; > > - if (!nested_vmx_allowed(vcpu)) > - return kvm_state->hdr.vmx.vmxon_pa == -1ull ? 0 : -EINVAL; > - > if (kvm_state->hdr.vmx.vmxon_pa == -1ull) { > if (kvm_state->hdr.vmx.smm.flags) > return -EINVAL; > @@ -5353,12 +5350,15 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu, > if (kvm_state->hdr.vmx.vmcs12_pa != -1ull) > return -EINVAL; > > - vmx_leave_nested(vcpu); > - return 0; > - } > + if (kvm_state->flags & ~KVM_STATE_NESTED_EVMCS) > + return -EINVAL; > + } else { > + if (!nested_vmx_allowed(vcpu)) > + return -EINVAL; > > - if (!page_address_valid(vcpu, kvm_state->hdr.vmx.vmxon_pa)) > - return -EINVAL; > + if (!page_address_valid(vcpu, kvm_state->hdr.vmx.vmxon_pa)) > + return -EINVAL; > + } > > if ((kvm_state->hdr.vmx.smm.flags & KVM_STATE_NESTED_SMM_GUEST_MODE) && > (kvm_state->flags & KVM_STATE_NESTED_GUEST_MODE)) > @@ -5381,11 +5381,15 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu, > return -EINVAL; > > vmx_leave_nested(vcpu); > - if (kvm_state->hdr.vmx.vmxon_pa == -1ull) > - return 0; > + if (kvm_state->flags & KVM_STATE_NESTED_EVMCS) { > + if (!nested_vmx_allowed(vcpu)) > + return -EINVAL; > > - if (kvm_state->flags & KVM_STATE_NESTED_EVMCS) > nested_enable_evmcs(vcpu, NULL); > + } > + > + if (kvm_state->hdr.vmx.vmxon_pa == -1ull) > + return 0; > > vmx->nested.vmxon_ptr = kvm_state->hdr.vmx.vmxon_pa; > ret = enter_vmx_operation(vcpu); > diff --git a/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c b/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c > index 0648fe6df5a8..e64ca20b315a 100644 > --- a/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c > +++ b/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c > @@ -123,36 +123,44 @@ void test_vmx_nested_state(struct kvm_vm *vm) > /* > * We cannot virtualize anything if the guest does not have VMX > * enabled. We expect KVM_SET_NESTED_STATE to return 0 if vmxon_pa > - * is set to -1ull. > + * is set to -1ull, but the flags must be zero. > */ > set_default_vmx_state(state, state_sz); > state->hdr.vmx.vmxon_pa = -1ull; > + test_nested_state_expect_einval(vm, state); > + > + state->hdr.vmx.vmcs12_pa = -1ull; > + state->flags = KVM_STATE_NESTED_EVMCS; > + test_nested_state_expect_einval(vm, state); > + > + state->flags = 0; > test_nested_state(vm, state); > > /* Enable VMX in the guest CPUID. */ > vcpu_set_cpuid(vm, VCPU_ID, kvm_get_supported_cpuid()); > > - /* It is invalid to have vmxon_pa == -1ull and SMM flags non-zero. */ > + /* > + * Setting vmxon_pa == -1ull and vmcs_pa == -1ull exits early without > + * setting the nested state but flags other than eVMCS must be clear. > + */ > set_default_vmx_state(state, state_sz); > state->hdr.vmx.vmxon_pa = -1ull; > + state->hdr.vmx.vmcs12_pa = -1ull; > + test_nested_state_expect_einval(vm, state); > + > + state->flags = KVM_STATE_NESTED_EVMCS; > + test_nested_state(vm, state); > + > + /* It is invalid to have vmxon_pa == -1ull and SMM flags non-zero. */ > state->hdr.vmx.smm.flags = 1; > test_nested_state_expect_einval(vm, state); > > /* It is invalid to have vmxon_pa == -1ull and vmcs_pa != -1ull. */ > set_default_vmx_state(state, state_sz); > state->hdr.vmx.vmxon_pa = -1ull; > - state->hdr.vmx.vmcs12_pa = 0; > + state->flags = 0; > test_nested_state_expect_einval(vm, state); > > - /* > - * Setting vmxon_pa == -1ull and vmcs_pa == -1ull exits early without > - * setting the nested state. > - */ > - set_default_vmx_state(state, state_sz); > - state->hdr.vmx.vmxon_pa = -1ull; > - state->hdr.vmx.vmcs12_pa = -1ull; > - test_nested_state(vm, state); > - > /* It is invalid to have vmxon_pa set to a non-page aligned address. */ > set_default_vmx_state(state, state_sz); > state->hdr.vmx.vmxon_pa = 1; -- Vitaly