Message ID | 20221102231911.3107438-11-seanjc@google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp199356wru; Wed, 2 Nov 2022 16:21:23 -0700 (PDT) X-Google-Smtp-Source: AMsMyM7yx+DArQVMzFUoEQs2GGP8SBAM4IfBFAtwZwE0EDzmZi+dnaIZdA8QANUbXYh2Pahtn+Ma X-Received: by 2002:a17:90a:ad08:b0:212:d5f1:e0c6 with SMTP id r8-20020a17090aad0800b00212d5f1e0c6mr44044817pjq.228.1667431283304; Wed, 02 Nov 2022 16:21:23 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1667431283; cv=none; d=google.com; s=arc-20160816; b=T1jRuusPhty0awiK/LJstW4sVUJUWFCfMDPgWB844Be9h0wIYoZ+nIjoeu5KUuGRaT 2NZntdt+bHk+7RdBRXxnlaMovSw+h+hW1uBpHS5Og6PCZ5+FH8r2OmqFC2g9AtqvU5mJ M/1xaS3N3cEFjmvKypMP5sefjqwLm3nDTF/p4K1ZmPLQDH8A4YFkZQsHFdoWbFDU9/vu FxND7GTgot3R4tWO/qocA4snXTMq+4kDotNh6uOLr23ttWk0xAN3Lwopb5Z8kMg0zjO5 NcRT8etiFi4BxjTqYoIj+BW2Oe2IBosTjx4DxZVs2VC/4TKL3i9rutrTXUKFYrPTSAOj hA2Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:from:subject:message-id:references :mime-version:in-reply-to:date:reply-to:dkim-signature; bh=4c2zoa+pA6B5dVM5IIaZmnVQdrF+CmLigOtVMLLIYD8=; b=isXKG0moaorL9/GbaghXieem+o8sEpeK7A7voTnyMAfHNVotH42y4Ch+dFgPIcOuwi GURiS3ucfJfnZCDw6ADNyIGyCDqlnXv/Zsflmk1/IZU4Js/7mu2TMMFUdWI9SInD8/jK 6XIwHuYmg23+ZUwWKAb/ss4pDAwWTx1uCttNQBonoKxe6gSAxoyQGLKGa9OMGD5jBkYN tch5US2vc4xsJEVTHCxX8Hy661UjlR8j79qIoa7iBf2efsOMmzfItV8vMXeFEZpgCB+o Nm/8Zfw2lgHb/dbcUNyDLRktt1yZTl53mwZq0Rw66NdpWpPtFBAuu+fSbDhv30hWIPeD i9Uw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20210112 header.b=ECpjGJtb; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id v185-20020a6389c2000000b004701c938a86si903961pgd.517.2022.11.02.16.21.09; Wed, 02 Nov 2022 16:21:23 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20210112 header.b=ECpjGJtb; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230500AbiKBXUb (ORCPT <rfc822;yves.mi.zy@gmail.com> + 99 others); Wed, 2 Nov 2022 19:20:31 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:32842 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230426AbiKBXUA (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 2 Nov 2022 19:20:00 -0400 Received: from mail-pj1-x1049.google.com (mail-pj1-x1049.google.com [IPv6:2607:f8b0:4864:20::1049]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 19BA526CF for <linux-kernel@vger.kernel.org>; Wed, 2 Nov 2022 16:19:31 -0700 (PDT) Received: by mail-pj1-x1049.google.com with SMTP id oo18-20020a17090b1c9200b0020bdba475afso2485980pjb.4 for <linux-kernel@vger.kernel.org>; Wed, 02 Nov 2022 16:19:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:reply-to:from:to:cc:subject:date:message-id:reply-to; bh=4c2zoa+pA6B5dVM5IIaZmnVQdrF+CmLigOtVMLLIYD8=; b=ECpjGJtbjyyvm3Zkn97le29FJrvdukE8ffHVj2dNk9FAwC++NI0lCVrbf3Opd4TWBf 7ONE7EX0w7ttmK3YMIoyE2Vh0FaG9SkGVZLCFbXid7ImD6yTFbtqmq7Vw0x0n0YWc1qr UvPnlqsBKgTOUZmLDWND50zLAkoG2FqWh124NRVJwySYvacHzZnlXGsktrezCTNW/gFJ apDpep1BU4A8VIiFKZFnkDLw7l8sDdIIv36URgpIHyxEBEvEU09NfxgFq/1iQFi4+zri llBfs0X8HKEdffCS4sfZ+5suanib1tg9fTY0dlx5A7HaJ0XDgPt9qN3X7bOUhQWSSKhm Q5OQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:reply-to:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=4c2zoa+pA6B5dVM5IIaZmnVQdrF+CmLigOtVMLLIYD8=; b=CLNT69BD262HuEXKGXm4q4XH9UsuUeD5wwytD25SA4eD7823ggvHG6vUIMw/bC/IwN c06IdToTFI/6lxR96BSxfH6BtHIZREnwE0DAbcchmMxD5k/w6k07feotAlTz5U8Css6E X18/luOzroIJ20u9tOYKtMKUIJ0XoP7pZHMbz4vYQI45PM5o/i2bzUpEj0OhwkyCPKHT kTpusHHI+j7XT1bcq85C14l9Yqe5oAgpadqxrUZ49RSf297aqCb5ys5uFOrZ+V6YKzgg fK9mySKxg1Jk7e2vNIwo48kPXY2HGMdQLUtByI7kYLLbspRCIGh5L9r1/+KpNGhv6+n9 wIqQ== X-Gm-Message-State: ACrzQf3szQxf8GJ4PJgSCROJpcXCbV+fc+AkJtfDEl0aqeEkv5FEoPk9 nRVj2LPaThzwrrynSEBEe+EWPx+IFk8= X-Received: from zagreus.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5c37]) (user=seanjc job=sendgmr) by 2002:a17:902:f602:b0:178:9818:48a4 with SMTP id n2-20020a170902f60200b00178981848a4mr27063044plg.148.1667431170921; Wed, 02 Nov 2022 16:19:30 -0700 (PDT) Reply-To: Sean Christopherson <seanjc@google.com> Date: Wed, 2 Nov 2022 23:18:37 +0000 In-Reply-To: <20221102231911.3107438-1-seanjc@google.com> Mime-Version: 1.0 References: <20221102231911.3107438-1-seanjc@google.com> X-Mailer: git-send-email 2.38.1.431.g37b22c650d-goog Message-ID: <20221102231911.3107438-11-seanjc@google.com> Subject: [PATCH 10/44] KVM: VMX: Clean up eVMCS enabling if KVM initialization fails From: Sean Christopherson <seanjc@google.com> To: Paolo Bonzini <pbonzini@redhat.com>, Marc Zyngier <maz@kernel.org>, Huacai Chen <chenhuacai@kernel.org>, Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>, Anup Patel <anup@brainfault.org>, Paul Walmsley <paul.walmsley@sifive.com>, Palmer Dabbelt <palmer@dabbelt.com>, Albert Ou <aou@eecs.berkeley.edu>, Christian Borntraeger <borntraeger@linux.ibm.com>, Janosch Frank <frankja@linux.ibm.com>, Claudio Imbrenda <imbrenda@linux.ibm.com>, Matthew Rosato <mjrosato@linux.ibm.com>, Eric Farman <farman@linux.ibm.com>, Sean Christopherson <seanjc@google.com>, Vitaly Kuznetsov <vkuznets@redhat.com> Cc: James Morse <james.morse@arm.com>, Alexandru Elisei <alexandru.elisei@arm.com>, Suzuki K Poulose <suzuki.poulose@arm.com>, Oliver Upton <oliver.upton@linux.dev>, Atish Patra <atishp@atishpatra.org>, David Hildenbrand <david@redhat.com>, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, kvmarm@lists.cs.columbia.edu, linux-mips@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, kvm-riscv@lists.infradead.org, linux-riscv@lists.infradead.org, linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org, Isaku Yamahata <isaku.yamahata@intel.com>, Fabiano Rosas <farosas@linux.ibm.com>, Michael Ellerman <mpe@ellerman.id.au>, Chao Gao <chao.gao@intel.com>, Thomas Gleixner <tglx@linutronix.de>, Yuan Yao <yuan.yao@intel.com> Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1748428425241862694?= X-GMAIL-MSGID: =?utf-8?q?1748428425241862694?= |
Series |
KVM: Rework kvm_init() and hardware enabling
|
|
Commit Message
Sean Christopherson
Nov. 2, 2022, 11:18 p.m. UTC
To make it obvious that KVM doesn't have a lurking bug, cleanup eVMCS
enabling if kvm_init() fails even though the enabling doesn't strictly
need to be unwound. eVMCS enabling only toggles values that are fully
contained in the VMX module, i.e. it's technically ok to leave the values
as-is since they'll disappear entirely when the module is unloaded, but
doing proper cleanup is relatively simple, and having a chunk of code
that isn't unwound is confusing.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/vmx/vmx.c | 137 +++++++++++++++++++++++------------------
1 file changed, 78 insertions(+), 59 deletions(-)
Comments
On 11/3/22 00:18, Sean Christopherson wrote:
> +static void hv_cleanup_evmcs(void)
This needs to be __init.
Paolo
On Thu, Nov 3, 2022 at 3:01 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 11/3/22 00:18, Sean Christopherson wrote: > > +static void hv_cleanup_evmcs(void) > > This needs to be __init. Error: brain temporarily disconnected. Paolo
Sean Christopherson <seanjc@google.com> writes: > To make it obvious that KVM doesn't have a lurking bug, cleanup eVMCS > enabling if kvm_init() fails even though the enabling doesn't strictly > need to be unwound. eVMCS enabling only toggles values that are fully > contained in the VMX module, i.e. it's technically ok to leave the values > as-is since they'll disappear entirely when the module is unloaded, but > doing proper cleanup is relatively simple, and having a chunk of code > that isn't unwound is confusing. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/vmx/vmx.c | 137 +++++++++++++++++++++++------------------ > 1 file changed, 78 insertions(+), 59 deletions(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 05a747c9a9ff..b3fd4049de01 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -524,6 +524,8 @@ static inline void vmx_segment_cache_clear(struct vcpu_vmx *vmx) > static unsigned long host_idt_base; > > #if IS_ENABLED(CONFIG_HYPERV) > +static struct kvm_x86_ops vmx_x86_ops __initdata; > + > static bool __read_mostly enlightened_vmcs = true; > module_param(enlightened_vmcs, bool, 0444); > > @@ -552,6 +554,71 @@ static int hv_enable_direct_tlbflush(struct kvm_vcpu *vcpu) > return 0; > } > > +static __init void hv_setup_evmcs(void) > +{ > + int cpu; > + > + if (!enlightened_vmcs) > + return; > + > + /* > + * Enlightened VMCS usage should be recommended and the host needs > + * to support eVMCS v1 or above. > + */ > + if (ms_hyperv.hints & HV_X64_ENLIGHTENED_VMCS_RECOMMENDED && > + (ms_hyperv.nested_features & HV_X64_ENLIGHTENED_VMCS_VERSION) >= > + KVM_EVMCS_VERSION) { > + > + /* Check that we have assist pages on all online CPUs */ > + for_each_online_cpu(cpu) { > + if (!hv_get_vp_assist_page(cpu)) { > + enlightened_vmcs = false; > + break; > + } > + } > + > + if (enlightened_vmcs) { > + pr_info("KVM: vmx: using Hyper-V Enlightened VMCS\n"); > + static_branch_enable(&enable_evmcs); > + } > + > + if (ms_hyperv.nested_features & HV_X64_NESTED_DIRECT_FLUSH) > + vmx_x86_ops.enable_direct_tlbflush > + = hv_enable_direct_tlbflush; > + > + } else { > + enlightened_vmcs = false; > + } > +} > +static void hv_cleanup_evmcs(void) > +{ > + struct hv_vp_assist_page *vp_ap; > + int cpu; > + > + if (!static_branch_unlikely(&enable_evmcs)) > + return; > + > + /* > + * Reset everything to support using non-enlightened VMCS access later > + * (e.g. when we reload the module with enlightened_vmcs=0) > + */ > + for_each_online_cpu(cpu) { > + vp_ap = hv_get_vp_assist_page(cpu); > + > + if (!vp_ap) > + continue; > + > + vp_ap->nested_control.features.directhypercall = 0; > + vp_ap->current_nested_vmcs = 0; > + vp_ap->enlighten_vmentry = 0; > + } Unrelated to your patch but while looking at this code I got curious about why don't we need a protection against CPU offlining here. Turns out that even when we offline a CPU, its VP assist page remains allocated (see hv_cpu_die()), we just write '0' to the MSR and thus accessing the page is safe. The consequent hv_cpu_init(), however, does not restore VP assist page when it's already allocated: # rdmsr -p 24 0x40000073 10212f001 # echo 0 > /sys/devices/system/cpu/cpu24/online # echo 1 > /sys/devices/system/cpu/cpu24/online # rdmsr -p 24 0x40000073 0 The culprit is commit e5d9b714fe402 ("x86/hyperv: fix root partition faults when writing to VP assist page MSR"). A patch is inbound. 'hv_root_partition' case is different though. We do memunmap() and reset VP assist page to zero so it is theoretically possible we're going to clash. Unless I'm missing some obvious reason why module unload can't coincide with CPU offlining, we may be better off surrounding this with cpus_read_lock()/cpus_read_unlock(). > + > + static_branch_disable(&enable_evmcs); > +} > + > +#else /* IS_ENABLED(CONFIG_HYPERV) */ > +static void hv_setup_evmcs(void) {} > +static void hv_cleanup_evmcs(void) {} > #endif /* IS_ENABLED(CONFIG_HYPERV) */ > > /* > @@ -8435,29 +8502,8 @@ static void vmx_exit(void) > > kvm_exit(); > > -#if IS_ENABLED(CONFIG_HYPERV) > - if (static_branch_unlikely(&enable_evmcs)) { > - int cpu; > - struct hv_vp_assist_page *vp_ap; > - /* > - * Reset everything to support using non-enlightened VMCS > - * access later (e.g. when we reload the module with > - * enlightened_vmcs=0) > - */ > - for_each_online_cpu(cpu) { > - vp_ap = hv_get_vp_assist_page(cpu); > + hv_cleanup_evmcs(); > > - if (!vp_ap) > - continue; > - > - vp_ap->nested_control.features.directhypercall = 0; > - vp_ap->current_nested_vmcs = 0; > - vp_ap->enlighten_vmentry = 0; > - } > - > - static_branch_disable(&enable_evmcs); > - } > -#endif > vmx_cleanup_l1d_flush(); > > allow_smaller_maxphyaddr = false; > @@ -8468,43 +8514,12 @@ static int __init vmx_init(void) > { > int r, cpu; > > -#if IS_ENABLED(CONFIG_HYPERV) > - /* > - * Enlightened VMCS usage should be recommended and the host needs > - * to support eVMCS v1 or above. We can also disable eVMCS support > - * with module parameter. > - */ > - if (enlightened_vmcs && > - ms_hyperv.hints & HV_X64_ENLIGHTENED_VMCS_RECOMMENDED && > - (ms_hyperv.nested_features & HV_X64_ENLIGHTENED_VMCS_VERSION) >= > - KVM_EVMCS_VERSION) { > - > - /* Check that we have assist pages on all online CPUs */ > - for_each_online_cpu(cpu) { > - if (!hv_get_vp_assist_page(cpu)) { > - enlightened_vmcs = false; > - break; > - } > - } > - > - if (enlightened_vmcs) { > - pr_info("KVM: vmx: using Hyper-V Enlightened VMCS\n"); > - static_branch_enable(&enable_evmcs); > - } > - > - if (ms_hyperv.nested_features & HV_X64_NESTED_DIRECT_FLUSH) > - vmx_x86_ops.enable_direct_tlbflush > - = hv_enable_direct_tlbflush; > - > - } else { > - enlightened_vmcs = false; > - } > -#endif > + hv_setup_evmcs(); > > r = kvm_init(&vmx_init_ops, sizeof(struct vcpu_vmx), > __alignof__(struct vcpu_vmx), THIS_MODULE); > if (r) > - return r; > + goto err_kvm_init; > > /* > * Must be called after kvm_init() so enable_ept is properly set > @@ -8514,10 +8529,8 @@ static int __init vmx_init(void) > * mitigation mode. > */ > r = vmx_setup_l1d_flush(vmentry_l1d_flush_param); > - if (r) { > - vmx_exit(); > - return r; > - } > + if (r) > + goto err_l1d_flush; > > vmx_setup_fb_clear_ctrl(); > > @@ -8542,5 +8555,11 @@ static int __init vmx_init(void) > allow_smaller_maxphyaddr = true; > > return 0; > + > +err_l1d_flush: > + vmx_exit(); > +err_kvm_init: > + hv_cleanup_evmcs(); > + return r; > } > module_init(vmx_init);
On Thu, Nov 03, 2022, Vitaly Kuznetsov wrote: > Sean Christopherson <seanjc@google.com> writes: > > + /* > > + * Reset everything to support using non-enlightened VMCS access later > > + * (e.g. when we reload the module with enlightened_vmcs=0) > > + */ > > + for_each_online_cpu(cpu) { > > + vp_ap = hv_get_vp_assist_page(cpu); > > + > > + if (!vp_ap) > > + continue; > > + > > + vp_ap->nested_control.features.directhypercall = 0; > > + vp_ap->current_nested_vmcs = 0; > > + vp_ap->enlighten_vmentry = 0; > > + } > > Unrelated to your patch but while looking at this code I got curious > about why don't we need a protection against CPU offlining here. Turns > out that even when we offline a CPU, its VP assist page remains > allocated (see hv_cpu_die()), we just write '0' to the MSR and thus Heh, "die". Hyper-V is quite dramatic. > accessing the page is safe. The consequent hv_cpu_init(), however, does > not restore VP assist page when it's already allocated: > > # rdmsr -p 24 0x40000073 > 10212f001 > # echo 0 > /sys/devices/system/cpu/cpu24/online > # echo 1 > /sys/devices/system/cpu/cpu24/online > # rdmsr -p 24 0x40000073 > 0 > > The culprit is commit e5d9b714fe402 ("x86/hyperv: fix root partition > faults when writing to VP assist page MSR"). A patch is inbound. > > 'hv_root_partition' case is different though. We do memunmap() and reset > VP assist page to zero so it is theoretically possible we're going to > clash. Unless I'm missing some obvious reason why module unload can't > coincide with CPU offlining, we may be better off surrounding this with > cpus_read_lock()/cpus_read_unlock(). I finally see what you're concerned about. If a CPU goes offline and its assist page is unmapped, zeroing out the nested/eVMCS stuff will fault. I think the real problem is that the purging of the eVMCS is in the wrong place. Move the clearing to vmx_hardware_disable() and then the CPU hotplug bug goes away once KVM disables hotplug during hardware enabling/disable later in the series. There's no need to wait until module exit, e.g. it's not like it costs much to clear a few variables, and IIUC the state is used only when KVM is actively using VMX/eVMCS. However, I believe there's a second bug. KVM's CPU online hook is called before Hyper-V's online hook (CPUHP_AP_ONLINE_DYN). Before this series, which moves KVM's hook from STARTING to ONLINE, KVM's hook is waaaay before Hyper-V's. That means that hv_cpu_init()'s allocation of the VP assist page will come _after_ KVM's check in vmx_hardware_enable() /* * This can happen if we hot-added a CPU but failed to allocate * VP assist page for it. */ if (static_branch_unlikely(&enable_evmcs) && !hv_get_vp_assist_page(cpu)) return -EFAULT; I.e. CPU hotplug will never work if KVM is running VMs as a Hyper-V guest. I bet you can repro by doing a SUSPEND+RESUME. Can you try to see if that's actually a bug? If so, the only sane fix seems to be to add a dedicated ONLINE action for Hyper-V. Per patch KVM: Rename and move CPUHP_AP_KVM_STARTING to ONLINE section from this series, CPUHP_AP_KVM_ONLINE needs to be before CPUHP_AP_SCHED_WAIT_EMPTY to ensure there are no tasks, i.e. no vCPUs, running on the to-be-unplugged CPU. Back to the original bug, proposed fix is below. The other advantage of moving the reset to hardware disabling is that the "cleanup" is just disabling the static key, and at that point can simply be deleted as there's no need to disable the static key when kvm-intel is unloaded since kvm-intel owns the key. I.e. this patch (that we're replying to) would get replaced with a patch to delete the disabling of the static key. -- From: Sean Christopherson <seanjc@google.com> Date: Thu, 10 Nov 2022 17:28:08 -0800 Subject: [PATCH] KVM: VMX: Reset eVMCS controls in VP assist page during hardware disabling Reset the eVMCS controls in the per-CPU VP assist page during hardware disabling instead of waiting until kvm-intel's module exit. The controls are activated if and only if KVM creates a VM, i.e. don't need to be reset if hardware is never enabled. Doing the reset during hardware disabling will naturally fix a potential NULL pointer deref bug once KVM disables CPU hotplug while enabling and disabling hardware (which is necessary to fix a variety of bugs). If the kernel is running as the root partition, the VP assist page is unmapped during CPU hot unplug, and so KVM's clearing of the eVMCS controls needs to occur with CPU hot(un)plug disabled, otherwise KVM could attempt to write to a CPU's VP assist page after it's unmapped. Reported-by: Vitaly Kuznetsov <vkuznets@redhat.com> Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/vmx/vmx.c | 50 +++++++++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 20 deletions(-) diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index aca88524fd1e..ae13aa3e8a1d 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -552,6 +552,33 @@ static int hv_enable_direct_tlbflush(struct kvm_vcpu *vcpu) return 0; } +static void hv_reset_evmcs(void) +{ + struct hv_vp_assist_page *vp_ap; + + if (!static_branch_unlikely(&enable_evmcs)) + return; + + /* + * KVM should enable eVMCS if and only if all CPUs have a VP assist + * page, and should reject CPU onlining if eVMCS is enabled the CPU + * doesn't have a VP assist page allocated. + */ + vp_ap = hv_get_vp_assist_page(smp_processor_id()); + if (WARN_ON_ONCE(!vp_ap)) + return; + + /* + * Reset everything to support using non-enlightened VMCS access later + * (e.g. when we reload the module with enlightened_vmcs=0) + */ + vp_ap->nested_control.features.directhypercall = 0; + vp_ap->current_nested_vmcs = 0; + vp_ap->enlighten_vmentry = 0; +} + +#else /* IS_ENABLED(CONFIG_HYPERV) */ +static void hv_reset_evmcs(void) {} #endif /* IS_ENABLED(CONFIG_HYPERV) */ /* @@ -2497,6 +2524,8 @@ static void vmx_hardware_disable(void) if (cpu_vmxoff()) kvm_spurious_fault(); + hv_reset_evmcs(); + intel_pt_handle_vmx(0); } @@ -8463,27 +8492,8 @@ static void vmx_exit(void) kvm_exit(); #if IS_ENABLED(CONFIG_HYPERV) - if (static_branch_unlikely(&enable_evmcs)) { - int cpu; - struct hv_vp_assist_page *vp_ap; - /* - * Reset everything to support using non-enlightened VMCS - * access later (e.g. when we reload the module with - * enlightened_vmcs=0) - */ - for_each_online_cpu(cpu) { - vp_ap = hv_get_vp_assist_page(cpu); - - if (!vp_ap) - continue; - - vp_ap->nested_control.features.directhypercall = 0; - vp_ap->current_nested_vmcs = 0; - vp_ap->enlighten_vmentry = 0; - } - + if (static_branch_unlikely(&enable_evmcs)) static_branch_disable(&enable_evmcs); - } #endif vmx_cleanup_l1d_flush(); base-commit: 5f47ba6894477dfbdc5416467a25fb7acb47d404 --
Sean Christopherson <seanjc@google.com> writes: > On Thu, Nov 03, 2022, Vitaly Kuznetsov wrote: >> Sean Christopherson <seanjc@google.com> writes: >> > + /* >> > + * Reset everything to support using non-enlightened VMCS access later >> > + * (e.g. when we reload the module with enlightened_vmcs=0) >> > + */ >> > + for_each_online_cpu(cpu) { >> > + vp_ap = hv_get_vp_assist_page(cpu); >> > + >> > + if (!vp_ap) >> > + continue; >> > + >> > + vp_ap->nested_control.features.directhypercall = 0; >> > + vp_ap->current_nested_vmcs = 0; >> > + vp_ap->enlighten_vmentry = 0; >> > + } >> >> Unrelated to your patch but while looking at this code I got curious >> about why don't we need a protection against CPU offlining here. Turns >> out that even when we offline a CPU, its VP assist page remains >> allocated (see hv_cpu_die()), we just write '0' to the MSR and thus > > Heh, "die". Hyper-V is quite dramatic. > >> accessing the page is safe. The consequent hv_cpu_init(), however, does >> not restore VP assist page when it's already allocated: >> >> # rdmsr -p 24 0x40000073 >> 10212f001 >> # echo 0 > /sys/devices/system/cpu/cpu24/online >> # echo 1 > /sys/devices/system/cpu/cpu24/online >> # rdmsr -p 24 0x40000073 >> 0 >> >> The culprit is commit e5d9b714fe402 ("x86/hyperv: fix root partition >> faults when writing to VP assist page MSR"). A patch is inbound. >> >> 'hv_root_partition' case is different though. We do memunmap() and reset >> VP assist page to zero so it is theoretically possible we're going to >> clash. Unless I'm missing some obvious reason why module unload can't >> coincide with CPU offlining, we may be better off surrounding this with >> cpus_read_lock()/cpus_read_unlock(). > > I finally see what you're concerned about. If a CPU goes offline and its assist > page is unmapped, zeroing out the nested/eVMCS stuff will fault. > > I think the real problem is that the purging of the eVMCS is in the wrong place. > Move the clearing to vmx_hardware_disable() and then the CPU hotplug bug goes > away once KVM disables hotplug during hardware enabling/disable later in the series. > There's no need to wait until module exit, e.g. it's not like it costs much to > clear a few variables, and IIUC the state is used only when KVM is actively using > VMX/eVMCS. > > However, I believe there's a second bug. KVM's CPU online hook is called before > Hyper-V's online hook (CPUHP_AP_ONLINE_DYN). Before this series, which moves KVM's > hook from STARTING to ONLINE, KVM's hook is waaaay before Hyper-V's. That means > that hv_cpu_init()'s allocation of the VP assist page will come _after_ KVM's > check in vmx_hardware_enable() > > /* > * This can happen if we hot-added a CPU but failed to allocate > * VP assist page for it. > */ > if (static_branch_unlikely(&enable_evmcs) && > !hv_get_vp_assist_page(cpu)) > return -EFAULT; > > I.e. CPU hotplug will never work if KVM is running VMs as a Hyper-V guest. I bet > you can repro by doing a SUSPEND+RESUME. > > Can you try to see if that's actually a bug? If so, the only sane fix seems to > be to add a dedicated ONLINE action for Hyper-V. It seems we can't get away without a dedicated stage for Hyper-V anyway, e.g. see our discussion with Michael: https://lore.kernel.org/linux-hyperv/878rkqr7ku.fsf@ovpn-192-136.brq.redhat.com/ All these issues are more or less "theoretical" as there's no real CPU hotplug on Hyper-V/Azure. Yes, it is possible to trigger problems by doing CPU offline/online but I don't see how this may come handy outside of testing envs. > Per patch > > KVM: Rename and move CPUHP_AP_KVM_STARTING to ONLINE section > > from this series, CPUHP_AP_KVM_ONLINE needs to be before CPUHP_AP_SCHED_WAIT_EMPTY > to ensure there are no tasks, i.e. no vCPUs, running on the to-be-unplugged CPU. > > Back to the original bug, proposed fix is below. The other advantage of moving > the reset to hardware disabling is that the "cleanup" is just disabling the static > key, and at that point can simply be deleted as there's no need to disable the > static key when kvm-intel is unloaded since kvm-intel owns the key. I.e. this > patch (that we're replying to) would get replaced with a patch to delete the > disabling of the static key. > From a quick glance looks good to me, I'll try to find some time to work on this issue. I will likely end up proposing a dedicated CPU hotplug stage for Hyper-V (which needs to happen before KVM's CPUHP_AP_KVM_ONLINE on CPU hotplug and after on unplug) anyway. Thanks for looking into this! > -- > From: Sean Christopherson <seanjc@google.com> > Date: Thu, 10 Nov 2022 17:28:08 -0800 > Subject: [PATCH] KVM: VMX: Reset eVMCS controls in VP assist page during > hardware disabling > > Reset the eVMCS controls in the per-CPU VP assist page during hardware > disabling instead of waiting until kvm-intel's module exit. The controls > are activated if and only if KVM creates a VM, i.e. don't need to be > reset if hardware is never enabled. > > Doing the reset during hardware disabling will naturally fix a potential > NULL pointer deref bug once KVM disables CPU hotplug while enabling and > disabling hardware (which is necessary to fix a variety of bugs). If the > kernel is running as the root partition, the VP assist page is unmapped > during CPU hot unplug, and so KVM's clearing of the eVMCS controls needs > to occur with CPU hot(un)plug disabled, otherwise KVM could attempt to > write to a CPU's VP assist page after it's unmapped. > > Reported-by: Vitaly Kuznetsov <vkuznets@redhat.com> > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/vmx/vmx.c | 50 +++++++++++++++++++++++++----------------- > 1 file changed, 30 insertions(+), 20 deletions(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index aca88524fd1e..ae13aa3e8a1d 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -552,6 +552,33 @@ static int hv_enable_direct_tlbflush(struct kvm_vcpu *vcpu) > return 0; > } > > +static void hv_reset_evmcs(void) > +{ > + struct hv_vp_assist_page *vp_ap; > + > + if (!static_branch_unlikely(&enable_evmcs)) > + return; > + > + /* > + * KVM should enable eVMCS if and only if all CPUs have a VP assist > + * page, and should reject CPU onlining if eVMCS is enabled the CPU > + * doesn't have a VP assist page allocated. > + */ > + vp_ap = hv_get_vp_assist_page(smp_processor_id()); > + if (WARN_ON_ONCE(!vp_ap)) > + return; > + > + /* > + * Reset everything to support using non-enlightened VMCS access later > + * (e.g. when we reload the module with enlightened_vmcs=0) > + */ > + vp_ap->nested_control.features.directhypercall = 0; > + vp_ap->current_nested_vmcs = 0; > + vp_ap->enlighten_vmentry = 0; > +} > + > +#else /* IS_ENABLED(CONFIG_HYPERV) */ > +static void hv_reset_evmcs(void) {} > #endif /* IS_ENABLED(CONFIG_HYPERV) */ > > /* > @@ -2497,6 +2524,8 @@ static void vmx_hardware_disable(void) > if (cpu_vmxoff()) > kvm_spurious_fault(); > > + hv_reset_evmcs(); > + > intel_pt_handle_vmx(0); > } > > @@ -8463,27 +8492,8 @@ static void vmx_exit(void) > kvm_exit(); > > #if IS_ENABLED(CONFIG_HYPERV) > - if (static_branch_unlikely(&enable_evmcs)) { > - int cpu; > - struct hv_vp_assist_page *vp_ap; > - /* > - * Reset everything to support using non-enlightened VMCS > - * access later (e.g. when we reload the module with > - * enlightened_vmcs=0) > - */ > - for_each_online_cpu(cpu) { > - vp_ap = hv_get_vp_assist_page(cpu); > - > - if (!vp_ap) > - continue; > - > - vp_ap->nested_control.features.directhypercall = 0; > - vp_ap->current_nested_vmcs = 0; > - vp_ap->enlighten_vmentry = 0; > - } > - > + if (static_branch_unlikely(&enable_evmcs)) > static_branch_disable(&enable_evmcs); > - } > #endif > vmx_cleanup_l1d_flush(); > > > base-commit: 5f47ba6894477dfbdc5416467a25fb7acb47d404
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 05a747c9a9ff..b3fd4049de01 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -524,6 +524,8 @@ static inline void vmx_segment_cache_clear(struct vcpu_vmx *vmx) static unsigned long host_idt_base; #if IS_ENABLED(CONFIG_HYPERV) +static struct kvm_x86_ops vmx_x86_ops __initdata; + static bool __read_mostly enlightened_vmcs = true; module_param(enlightened_vmcs, bool, 0444); @@ -552,6 +554,71 @@ static int hv_enable_direct_tlbflush(struct kvm_vcpu *vcpu) return 0; } +static __init void hv_setup_evmcs(void) +{ + int cpu; + + if (!enlightened_vmcs) + return; + + /* + * Enlightened VMCS usage should be recommended and the host needs + * to support eVMCS v1 or above. + */ + if (ms_hyperv.hints & HV_X64_ENLIGHTENED_VMCS_RECOMMENDED && + (ms_hyperv.nested_features & HV_X64_ENLIGHTENED_VMCS_VERSION) >= + KVM_EVMCS_VERSION) { + + /* Check that we have assist pages on all online CPUs */ + for_each_online_cpu(cpu) { + if (!hv_get_vp_assist_page(cpu)) { + enlightened_vmcs = false; + break; + } + } + + if (enlightened_vmcs) { + pr_info("KVM: vmx: using Hyper-V Enlightened VMCS\n"); + static_branch_enable(&enable_evmcs); + } + + if (ms_hyperv.nested_features & HV_X64_NESTED_DIRECT_FLUSH) + vmx_x86_ops.enable_direct_tlbflush + = hv_enable_direct_tlbflush; + + } else { + enlightened_vmcs = false; + } +} +static void hv_cleanup_evmcs(void) +{ + struct hv_vp_assist_page *vp_ap; + int cpu; + + if (!static_branch_unlikely(&enable_evmcs)) + return; + + /* + * Reset everything to support using non-enlightened VMCS access later + * (e.g. when we reload the module with enlightened_vmcs=0) + */ + for_each_online_cpu(cpu) { + vp_ap = hv_get_vp_assist_page(cpu); + + if (!vp_ap) + continue; + + vp_ap->nested_control.features.directhypercall = 0; + vp_ap->current_nested_vmcs = 0; + vp_ap->enlighten_vmentry = 0; + } + + static_branch_disable(&enable_evmcs); +} + +#else /* IS_ENABLED(CONFIG_HYPERV) */ +static void hv_setup_evmcs(void) {} +static void hv_cleanup_evmcs(void) {} #endif /* IS_ENABLED(CONFIG_HYPERV) */ /* @@ -8435,29 +8502,8 @@ static void vmx_exit(void) kvm_exit(); -#if IS_ENABLED(CONFIG_HYPERV) - if (static_branch_unlikely(&enable_evmcs)) { - int cpu; - struct hv_vp_assist_page *vp_ap; - /* - * Reset everything to support using non-enlightened VMCS - * access later (e.g. when we reload the module with - * enlightened_vmcs=0) - */ - for_each_online_cpu(cpu) { - vp_ap = hv_get_vp_assist_page(cpu); + hv_cleanup_evmcs(); - if (!vp_ap) - continue; - - vp_ap->nested_control.features.directhypercall = 0; - vp_ap->current_nested_vmcs = 0; - vp_ap->enlighten_vmentry = 0; - } - - static_branch_disable(&enable_evmcs); - } -#endif vmx_cleanup_l1d_flush(); allow_smaller_maxphyaddr = false; @@ -8468,43 +8514,12 @@ static int __init vmx_init(void) { int r, cpu; -#if IS_ENABLED(CONFIG_HYPERV) - /* - * Enlightened VMCS usage should be recommended and the host needs - * to support eVMCS v1 or above. We can also disable eVMCS support - * with module parameter. - */ - if (enlightened_vmcs && - ms_hyperv.hints & HV_X64_ENLIGHTENED_VMCS_RECOMMENDED && - (ms_hyperv.nested_features & HV_X64_ENLIGHTENED_VMCS_VERSION) >= - KVM_EVMCS_VERSION) { - - /* Check that we have assist pages on all online CPUs */ - for_each_online_cpu(cpu) { - if (!hv_get_vp_assist_page(cpu)) { - enlightened_vmcs = false; - break; - } - } - - if (enlightened_vmcs) { - pr_info("KVM: vmx: using Hyper-V Enlightened VMCS\n"); - static_branch_enable(&enable_evmcs); - } - - if (ms_hyperv.nested_features & HV_X64_NESTED_DIRECT_FLUSH) - vmx_x86_ops.enable_direct_tlbflush - = hv_enable_direct_tlbflush; - - } else { - enlightened_vmcs = false; - } -#endif + hv_setup_evmcs(); r = kvm_init(&vmx_init_ops, sizeof(struct vcpu_vmx), __alignof__(struct vcpu_vmx), THIS_MODULE); if (r) - return r; + goto err_kvm_init; /* * Must be called after kvm_init() so enable_ept is properly set @@ -8514,10 +8529,8 @@ static int __init vmx_init(void) * mitigation mode. */ r = vmx_setup_l1d_flush(vmentry_l1d_flush_param); - if (r) { - vmx_exit(); - return r; - } + if (r) + goto err_l1d_flush; vmx_setup_fb_clear_ctrl(); @@ -8542,5 +8555,11 @@ static int __init vmx_init(void) allow_smaller_maxphyaddr = true; return 0; + +err_l1d_flush: + vmx_exit(); +err_kvm_init: + hv_cleanup_evmcs(); + return r; } module_init(vmx_init);