From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sean Christopherson Date: Wed, 16 Nov 2022 17:11:18 +0000 Subject: [PATCH 38/44] KVM: Disable CPU hotplug during hardware enabling In-Reply-To: <95ca433349eca601bdd2b16d70f59ba8e56d8e3f.camel@intel.com> References: <20221102231911.3107438-1-seanjc@google.com> <20221102231911.3107438-39-seanjc@google.com> <88e920944de70e7d69a98f74005b49c59b5aaa3b.camel@intel.com> <95ca433349eca601bdd2b16d70f59ba8e56d8e3f.camel@intel.com> Message-ID: List-Id: To: kvm-riscv@lists.infradead.org MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On Wed, Nov 16, 2022, Huang, Kai wrote: > On Tue, 2022-11-15 at 20:16 +0000, Sean Christopherson wrote: > > On Thu, Nov 10, 2022, Huang, Kai wrote: > > > On Thu, 2022-11-10 at 01:33 +0000, Huang, Kai wrote: > > > Hmm.. I wasn't thinking thoroughly. I forgot CPU compatibility check also > > > happens on all online cpus when loading KVM. For this case, IRQ is disabled and > > > cpu_active() is true. For the hotplug case, IRQ is enabled but cpu_active() is > > > false. > > > > Actually, you're right (and wrong). You're right in that the WARN is flawed. And > > the reason for that is because you're wrong about the hotplug case. In this version > > of things, the compatibility checks are routed through hardware enabling, i.e. this > > flow is used only when loading KVM. This helper should only be called via SMP function > > call, which means that IRQs should always be disabled. > > Did you mean below code change in later patch "[PATCH 39/44] KVM: Drop > kvm_count_lock and instead protect kvm_usage_count with kvm_lock"? > > /* > * Abort the CPU online process if hardware virtualization cannot > * be enabled. Otherwise running VMs would encounter unrecoverable > @@ -5039,13 +5039,16 @@ static int kvm_online_cpu(unsigned int cpu) > if (kvm_usage_count) { > WARN_ON_ONCE(atomic_read(&hardware_enable_failed)); > > + local_irq_save(flags); > hardware_enable_nolock(NULL); > + local_irq_restore(flags); Sort of. What I was saying is that in this v1, the compatibility checks that are done during harware enabling are initiated from vendor code, i.e. VMX and SVM call {svm,vmx}_check_processor_compat() directly. As a result, the compat checks that are handled in common code: if (__cr4_reserved_bits(cpu_has, c) != __cr4_reserved_bits(cpu_has, &boot_cpu_data)) return -EIO; are skipped. And if that's fixed, then the above hardware_enable_nolock() call will bounce through kvm_x86_check_processor_compatibility() with IRQs enabled once the KVM hotplug hook is moved to the ONLINE section. As above, the simple "fix" would be to disable IRQs, but that's not actually necessary. The only requirement is that preemption is disabled so that the checks are done on the current CPU. The "IRQs disabled" check was a deliberately agressive WARN that was added to guard against doing compatibility checks from the "wrong" location. E.g. this is what I ended up with for a changelog to drop the irqs_disabled() check and for the end code (though it's not tested yet...) Drop kvm_x86_check_processor_compatibility()'s WARN that IRQs are disabled, as the ONLINE section runs with IRQs disabled. The WARN wasn't intended to be a requirement, e.g. disabling preemption is sufficient, the IRQ thing was purely an aggressive sanity check since the helper was only ever invoked via SMP function call. static int kvm_x86_check_processor_compatibility(void) { int cpu = smp_processor_id(); struct cpuinfo_x86 *c = &cpu_data(cpu); /* * Compatibility checks are done when loading KVM and when enabling * hardware, e.g. during CPU hotplug, to ensure all online CPUs are * compatible, i.e. KVM should never perform a compatibility check on * an offline CPU. */ WARN_ON(!cpu_online(cpu)); if (__cr4_reserved_bits(cpu_has, c) != __cr4_reserved_bits(cpu_has, &boot_cpu_data)) return -EIO; return static_call(kvm_x86_check_processor_compatibility)(); } int kvm_arch_hardware_enable(void) { struct kvm *kvm; struct kvm_vcpu *vcpu; unsigned long i; int ret; u64 local_tsc; u64 max_tsc = 0; bool stable, backwards_tsc = false; kvm_user_return_msr_cpu_online(); ret = kvm_x86_check_processor_compatibility(); if (ret) return ret; ret = static_call(kvm_x86_hardware_enable)(); if (ret != 0) return ret; .... } 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 Received: from mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4BFF2C4332F for ; Wed, 16 Nov 2022 17:11:27 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id CE58A4B852; Wed, 16 Nov 2022 12:11:26 -0500 (EST) X-Virus-Scanned: at lists.cs.columbia.edu Authentication-Results: mm01.cs.columbia.edu (amavisd-new); dkim=softfail (fail, message has been altered) header.i=@google.com Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 2O5BHmS-JXMb; Wed, 16 Nov 2022 12:11:25 -0500 (EST) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 7E1144B86B; Wed, 16 Nov 2022 12:11:25 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 70E404B852 for ; Wed, 16 Nov 2022 12:11:24 -0500 (EST) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id NDytO2dnys+K for ; Wed, 16 Nov 2022 12:11:23 -0500 (EST) Received: from mail-pl1-f171.google.com (mail-pl1-f171.google.com [209.85.214.171]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 18EA84B82B for ; Wed, 16 Nov 2022 12:11:23 -0500 (EST) Received: by mail-pl1-f171.google.com with SMTP id w23so10576906ply.12 for ; Wed, 16 Nov 2022 09:11:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=qU5KpuuvMvabmlf4l/Mi3tQhh2fL8Dh2/iugoH77F0k=; b=TdgWtef6UhHbqKe4QS/3Ayu3VNZZ3i9eoJmtCy7dLqrysJ7wB0UBIiB1NHl2XD8yEJ 9A9e40vVPz0LeaGnBm+xWkbLjcxsdpGQjMtsawyPcUDXFiJtlqbZJWuF4NPJNhW8gWhc yklS7sdK1V06+Vo7Vtm2MrCNKTPLN0kZs4hpWbYlAx+pUoMaBEUe2EJ/R8vPCcvbrDoh 3WsWgepDpESG+KKK928nilFdVglC19Gmb2mv5/mmmkx20fEfIehPjExF++q08zwUw6nJ A1VBgI5fVGcYhdLGJAFBS9e+51HNi1lhj80m/YmoiHsls5U6I/EdT06kMnKY5Lu6o+oq Zk/Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=qU5KpuuvMvabmlf4l/Mi3tQhh2fL8Dh2/iugoH77F0k=; b=jeW2ebjTcGY7exeE1NCZKRO7PsUiwe0pr/r9WS2sxScNJCvXDPNeN5quPLs6HdIBmg N9Z6anqjxv8EVRWO90X3zHtDuruPptnZOVdeKQN6I19EDy6wDDKIMuIhj12Q0sicKqFS +eT8OfeHRk44OA86LuVMfqr3anIRWoJ7lGJ8ZmBRx8uM4dONIZSd6LiwdOlP1JAmanCs NDZkje1nT2/PLSbN/uVUrO8+NS3OC77jy5u1iwSvHWbD7lKeBJlS7HdhtKkjTo/jNHeh ZqgV7rR8TDLsChMyj9YCEXJAWeZTBCMKn4NKklXqXv5f4FECjzJyX69BXej1gq+LsWPF 0zmw== X-Gm-Message-State: ANoB5pl+aErlWXUTj25fbrVKeFMFqTFVyk21fqk/rHhv/ES1ObzWLf18 oj0PrfYOla7FCsTbuEls8jGvtQ== X-Google-Smtp-Source: AA0mqf6YIHZPZijJW3drWLmCC0J1i77v6BfIk1GaXTBydT1hr0zvtFJFuTOMD/RjlZsQvYbm7cfMpQ== X-Received: by 2002:a17:902:ed41:b0:175:105a:3087 with SMTP id y1-20020a170902ed4100b00175105a3087mr10067985plb.65.1668618681834; Wed, 16 Nov 2022 09:11:21 -0800 (PST) Received: from google.com (7.104.168.34.bc.googleusercontent.com. [34.168.104.7]) by smtp.gmail.com with ESMTPSA id j6-20020a17090276c600b001788ccecbf5sm12424413plt.31.2022.11.16.09.11.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 16 Nov 2022 09:11:21 -0800 (PST) Date: Wed, 16 Nov 2022 17:11:18 +0000 From: Sean Christopherson To: "Huang, Kai" Subject: Re: [PATCH 38/44] KVM: Disable CPU hotplug during hardware enabling Message-ID: References: <20221102231911.3107438-1-seanjc@google.com> <20221102231911.3107438-39-seanjc@google.com> <88e920944de70e7d69a98f74005b49c59b5aaa3b.camel@intel.com> <95ca433349eca601bdd2b16d70f59ba8e56d8e3f.camel@intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <95ca433349eca601bdd2b16d70f59ba8e56d8e3f.camel@intel.com> Cc: "mjrosato@linux.ibm.com" , "david@redhat.com" , "paul.walmsley@sifive.com" , "linux-kernel@vger.kernel.org" , "linux-riscv@lists.infradead.org" , "imbrenda@linux.ibm.com" , "kvmarm@lists.cs.columbia.edu" , "linux-s390@vger.kernel.org" , "kvm@vger.kernel.org" , "mpe@ellerman.id.au" , "chenhuacai@kernel.org" , "aleksandar.qemu.devel@gmail.com" , "borntraeger@linux.ibm.com" , "Gao, Chao" , "farman@linux.ibm.com" , "aou@eecs.berkeley.edu" , "Yao, Yuan" , "kvmarm@lists.linux.dev" , "tglx@linutronix.de" , "linux-arm-kernel@lists.infradead.org" , "frankja@linux.ibm.com" , "Yamahata, Isaku" , "atishp@atishpatra.org" , "farosas@linux.ibm.com" , "linux-mips@vger.kernel.org" , "palmer@dabbelt.com" , "kvm-riscv@lists.infradead.org" , "maz@kernel.org" , "pbonzini@redhat.com" , "vkuznets@redhat.com" , "linuxppc-dev@lists.ozlabs.org" X-BeenThere: kvmarm@lists.cs.columbia.edu X-Mailman-Version: 2.1.14 Precedence: list List-Id: Where KVM/ARM decisions are made List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu On Wed, Nov 16, 2022, Huang, Kai wrote: > On Tue, 2022-11-15 at 20:16 +0000, Sean Christopherson wrote: > > On Thu, Nov 10, 2022, Huang, Kai wrote: > > > On Thu, 2022-11-10 at 01:33 +0000, Huang, Kai wrote: > > > Hmm.. I wasn't thinking thoroughly. I forgot CPU compatibility check also > > > happens on all online cpus when loading KVM. For this case, IRQ is disabled and > > > cpu_active() is true. For the hotplug case, IRQ is enabled but cpu_active() is > > > false. > > > > Actually, you're right (and wrong). You're right in that the WARN is flawed. And > > the reason for that is because you're wrong about the hotplug case. In this version > > of things, the compatibility checks are routed through hardware enabling, i.e. this > > flow is used only when loading KVM. This helper should only be called via SMP function > > call, which means that IRQs should always be disabled. > > Did you mean below code change in later patch "[PATCH 39/44] KVM: Drop > kvm_count_lock and instead protect kvm_usage_count with kvm_lock"? > > /* > * Abort the CPU online process if hardware virtualization cannot > * be enabled. Otherwise running VMs would encounter unrecoverable > @@ -5039,13 +5039,16 @@ static int kvm_online_cpu(unsigned int cpu) > if (kvm_usage_count) { > WARN_ON_ONCE(atomic_read(&hardware_enable_failed)); > > + local_irq_save(flags); > hardware_enable_nolock(NULL); > + local_irq_restore(flags); Sort of. What I was saying is that in this v1, the compatibility checks that are done during harware enabling are initiated from vendor code, i.e. VMX and SVM call {svm,vmx}_check_processor_compat() directly. As a result, the compat checks that are handled in common code: if (__cr4_reserved_bits(cpu_has, c) != __cr4_reserved_bits(cpu_has, &boot_cpu_data)) return -EIO; are skipped. And if that's fixed, then the above hardware_enable_nolock() call will bounce through kvm_x86_check_processor_compatibility() with IRQs enabled once the KVM hotplug hook is moved to the ONLINE section. As above, the simple "fix" would be to disable IRQs, but that's not actually necessary. The only requirement is that preemption is disabled so that the checks are done on the current CPU. The "IRQs disabled" check was a deliberately agressive WARN that was added to guard against doing compatibility checks from the "wrong" location. E.g. this is what I ended up with for a changelog to drop the irqs_disabled() check and for the end code (though it's not tested yet...) Drop kvm_x86_check_processor_compatibility()'s WARN that IRQs are disabled, as the ONLINE section runs with IRQs disabled. The WARN wasn't intended to be a requirement, e.g. disabling preemption is sufficient, the IRQ thing was purely an aggressive sanity check since the helper was only ever invoked via SMP function call. static int kvm_x86_check_processor_compatibility(void) { int cpu = smp_processor_id(); struct cpuinfo_x86 *c = &cpu_data(cpu); /* * Compatibility checks are done when loading KVM and when enabling * hardware, e.g. during CPU hotplug, to ensure all online CPUs are * compatible, i.e. KVM should never perform a compatibility check on * an offline CPU. */ WARN_ON(!cpu_online(cpu)); if (__cr4_reserved_bits(cpu_has, c) != __cr4_reserved_bits(cpu_has, &boot_cpu_data)) return -EIO; return static_call(kvm_x86_check_processor_compatibility)(); } int kvm_arch_hardware_enable(void) { struct kvm *kvm; struct kvm_vcpu *vcpu; unsigned long i; int ret; u64 local_tsc; u64 max_tsc = 0; bool stable, backwards_tsc = false; kvm_user_return_msr_cpu_online(); ret = kvm_x86_check_processor_compatibility(); if (ret) return ret; ret = static_call(kvm_x86_hardware_enable)(); if (ret != 0) return ret; .... } _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-f50.google.com (mail-pj1-f50.google.com [209.85.216.50]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9204BD504 for ; Wed, 16 Nov 2022 17:11:22 +0000 (UTC) Received: by mail-pj1-f50.google.com with SMTP id l6so17135008pjj.0 for ; Wed, 16 Nov 2022 09:11:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=qU5KpuuvMvabmlf4l/Mi3tQhh2fL8Dh2/iugoH77F0k=; b=TdgWtef6UhHbqKe4QS/3Ayu3VNZZ3i9eoJmtCy7dLqrysJ7wB0UBIiB1NHl2XD8yEJ 9A9e40vVPz0LeaGnBm+xWkbLjcxsdpGQjMtsawyPcUDXFiJtlqbZJWuF4NPJNhW8gWhc yklS7sdK1V06+Vo7Vtm2MrCNKTPLN0kZs4hpWbYlAx+pUoMaBEUe2EJ/R8vPCcvbrDoh 3WsWgepDpESG+KKK928nilFdVglC19Gmb2mv5/mmmkx20fEfIehPjExF++q08zwUw6nJ A1VBgI5fVGcYhdLGJAFBS9e+51HNi1lhj80m/YmoiHsls5U6I/EdT06kMnKY5Lu6o+oq Zk/Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=qU5KpuuvMvabmlf4l/Mi3tQhh2fL8Dh2/iugoH77F0k=; b=kQLUfBs5bQeIw4D1Mdp2bEx7cF62Cfx4FVEtbuzEXOKn85lcc8t2NT0G+ELI+U2fDX CR9LrZeVs96N8LISIJ2P7M5vF+nSbqpzCR2Nj21fu7iJorCIqHpYlyTZP+wbFrOdTh7z I6f1khrxxGH37yoP3Mq3OZAwsmns1bf10ZqmZMGjSfHakOaq9265Tjv4lqT2qoLARY3y wGX/xcaVPUm63gCG6niAe96PcV+Xag463886QAzaL+5gzJoJw4xNnmuEduHN9rnKd4Lt 05VMGL1BIcV6p26fDu2hzTRlYl/2h8iER/jxwHGE3AiMIwapUPjSpB7juMTiPXxQ7AXi wqZw== X-Gm-Message-State: ANoB5pmGzHEGOptCwhA9fNLWk1zhcAORb0wNzDX8CXyjbYeoADXOn2Xm 85VNQhbnsjVTYtmKB/uokulb9Q== X-Google-Smtp-Source: AA0mqf6YIHZPZijJW3drWLmCC0J1i77v6BfIk1GaXTBydT1hr0zvtFJFuTOMD/RjlZsQvYbm7cfMpQ== X-Received: by 2002:a17:902:ed41:b0:175:105a:3087 with SMTP id y1-20020a170902ed4100b00175105a3087mr10067985plb.65.1668618681834; Wed, 16 Nov 2022 09:11:21 -0800 (PST) Received: from google.com (7.104.168.34.bc.googleusercontent.com. [34.168.104.7]) by smtp.gmail.com with ESMTPSA id j6-20020a17090276c600b001788ccecbf5sm12424413plt.31.2022.11.16.09.11.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 16 Nov 2022 09:11:21 -0800 (PST) Date: Wed, 16 Nov 2022 17:11:18 +0000 From: Sean Christopherson To: "Huang, Kai" Cc: "borntraeger@linux.ibm.com" , "kvm-riscv@lists.infradead.org" , "Yao, Yuan" , "tglx@linutronix.de" , "kvm@vger.kernel.org" , "Yamahata, Isaku" , "suzuki.poulose@arm.com" , "pbonzini@redhat.com" , "david@redhat.com" , "linux-mips@vger.kernel.org" , "linuxppc-dev@lists.ozlabs.org" , "linux-riscv@lists.infradead.org" , "kvmarm@lists.linux.dev" , "linux-kernel@vger.kernel.org" , "mjrosato@linux.ibm.com" , "oliver.upton@linux.dev" , "farosas@linux.ibm.com" , "linux-s390@vger.kernel.org" , "palmer@dabbelt.com" , "chenhuacai@kernel.org" , "aou@eecs.berkeley.edu" , "alexandru.elisei@arm.com" , "mpe@ellerman.id.au" , "vkuznets@redhat.com" , "maz@kernel.org" , "anup@brainfault.org" , "frankja@linux.ibm.com" , "james.morse@arm.com" , "farman@linux.ibm.com" , "aleksandar.qemu.devel@gmail.com" , "kvmarm@lists.cs.columbia.edu" , "paul.walmsley@sifive.com" , "linux-arm-kernel@lists.infradead.org" , "atishp@atishpatra.org" , "imbrenda@linux.ibm.com" , "Gao, Chao" Subject: Re: [PATCH 38/44] KVM: Disable CPU hotplug during hardware enabling Message-ID: References: <20221102231911.3107438-1-seanjc@google.com> <20221102231911.3107438-39-seanjc@google.com> <88e920944de70e7d69a98f74005b49c59b5aaa3b.camel@intel.com> <95ca433349eca601bdd2b16d70f59ba8e56d8e3f.camel@intel.com> Precedence: bulk X-Mailing-List: kvmarm@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <95ca433349eca601bdd2b16d70f59ba8e56d8e3f.camel@intel.com> Message-ID: <20221116171118.aTzosgpt6EL4TNVPm05ul4dlcbQFcQU8woTL7jS0Pdc@z> On Wed, Nov 16, 2022, Huang, Kai wrote: > On Tue, 2022-11-15 at 20:16 +0000, Sean Christopherson wrote: > > On Thu, Nov 10, 2022, Huang, Kai wrote: > > > On Thu, 2022-11-10 at 01:33 +0000, Huang, Kai wrote: > > > Hmm.. I wasn't thinking thoroughly. I forgot CPU compatibility check also > > > happens on all online cpus when loading KVM. For this case, IRQ is disabled and > > > cpu_active() is true. For the hotplug case, IRQ is enabled but cpu_active() is > > > false. > > > > Actually, you're right (and wrong). You're right in that the WARN is flawed. And > > the reason for that is because you're wrong about the hotplug case. In this version > > of things, the compatibility checks are routed through hardware enabling, i.e. this > > flow is used only when loading KVM. This helper should only be called via SMP function > > call, which means that IRQs should always be disabled. > > Did you mean below code change in later patch "[PATCH 39/44] KVM: Drop > kvm_count_lock and instead protect kvm_usage_count with kvm_lock"? > > /* > * Abort the CPU online process if hardware virtualization cannot > * be enabled. Otherwise running VMs would encounter unrecoverable > @@ -5039,13 +5039,16 @@ static int kvm_online_cpu(unsigned int cpu) > if (kvm_usage_count) { > WARN_ON_ONCE(atomic_read(&hardware_enable_failed)); > > + local_irq_save(flags); > hardware_enable_nolock(NULL); > + local_irq_restore(flags); Sort of. What I was saying is that in this v1, the compatibility checks that are done during harware enabling are initiated from vendor code, i.e. VMX and SVM call {svm,vmx}_check_processor_compat() directly. As a result, the compat checks that are handled in common code: if (__cr4_reserved_bits(cpu_has, c) != __cr4_reserved_bits(cpu_has, &boot_cpu_data)) return -EIO; are skipped. And if that's fixed, then the above hardware_enable_nolock() call will bounce through kvm_x86_check_processor_compatibility() with IRQs enabled once the KVM hotplug hook is moved to the ONLINE section. As above, the simple "fix" would be to disable IRQs, but that's not actually necessary. The only requirement is that preemption is disabled so that the checks are done on the current CPU. The "IRQs disabled" check was a deliberately agressive WARN that was added to guard against doing compatibility checks from the "wrong" location. E.g. this is what I ended up with for a changelog to drop the irqs_disabled() check and for the end code (though it's not tested yet...) Drop kvm_x86_check_processor_compatibility()'s WARN that IRQs are disabled, as the ONLINE section runs with IRQs disabled. The WARN wasn't intended to be a requirement, e.g. disabling preemption is sufficient, the IRQ thing was purely an aggressive sanity check since the helper was only ever invoked via SMP function call. static int kvm_x86_check_processor_compatibility(void) { int cpu = smp_processor_id(); struct cpuinfo_x86 *c = &cpu_data(cpu); /* * Compatibility checks are done when loading KVM and when enabling * hardware, e.g. during CPU hotplug, to ensure all online CPUs are * compatible, i.e. KVM should never perform a compatibility check on * an offline CPU. */ WARN_ON(!cpu_online(cpu)); if (__cr4_reserved_bits(cpu_has, c) != __cr4_reserved_bits(cpu_has, &boot_cpu_data)) return -EIO; return static_call(kvm_x86_check_processor_compatibility)(); } int kvm_arch_hardware_enable(void) { struct kvm *kvm; struct kvm_vcpu *vcpu; unsigned long i; int ret; u64 local_tsc; u64 max_tsc = 0; bool stable, backwards_tsc = false; kvm_user_return_msr_cpu_online(); ret = kvm_x86_check_processor_compatibility(); if (ret) return ret; ret = static_call(kvm_x86_hardware_enable)(); if (ret != 0) return ret; .... } 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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 320EDC433FE for ; Wed, 16 Nov 2022 17:12:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=dF6PFoQNHxPbt47yWsnx3TLc46bIm4z85q0M8awihPU=; b=Vp2Jub5OY86OSr O+fBXfbJG5vbWYZ9/QsYVz7TCKjhLjX0t2eQV1cSMphQ0bfEg6+/0DwsRECMbo1D4KBKWZ/5Ggt2j TIViPgEvaZDuEpIkx7m4F0pnDqlH2NPNLTX5S0rfn5GBzvGs/0WFCNCUy5c1sGXytK09ZrWc25qHQ aEVPEVmQOCqee+ckAlsUATMsoQgUefLyxSZc21vu7mH3bVVcX4Le6zmpcqGsNU2zCamC9jTx+wttm v9G3620Sks9W87m9yt6/5HDW1NuJJx6xbWMtLd6kqiMzvHR7O+7locAg3CzpxERNDiGRWC1uWPvpE nXo6Nm4aiAqn3CHmyDaQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ovLxE-006Jbt-Fg; Wed, 16 Nov 2022 17:12:04 +0000 Received: from mail-pl1-x631.google.com ([2607:f8b0:4864:20::631]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1ovLwa-006JIN-QV for linux-riscv@lists.infradead.org; Wed, 16 Nov 2022 17:11:27 +0000 Received: by mail-pl1-x631.google.com with SMTP id p21so17008222plr.7 for ; Wed, 16 Nov 2022 09:11:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=qU5KpuuvMvabmlf4l/Mi3tQhh2fL8Dh2/iugoH77F0k=; b=TdgWtef6UhHbqKe4QS/3Ayu3VNZZ3i9eoJmtCy7dLqrysJ7wB0UBIiB1NHl2XD8yEJ 9A9e40vVPz0LeaGnBm+xWkbLjcxsdpGQjMtsawyPcUDXFiJtlqbZJWuF4NPJNhW8gWhc yklS7sdK1V06+Vo7Vtm2MrCNKTPLN0kZs4hpWbYlAx+pUoMaBEUe2EJ/R8vPCcvbrDoh 3WsWgepDpESG+KKK928nilFdVglC19Gmb2mv5/mmmkx20fEfIehPjExF++q08zwUw6nJ A1VBgI5fVGcYhdLGJAFBS9e+51HNi1lhj80m/YmoiHsls5U6I/EdT06kMnKY5Lu6o+oq Zk/Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=qU5KpuuvMvabmlf4l/Mi3tQhh2fL8Dh2/iugoH77F0k=; b=ZIYx10/mxGNS8dkvFBSKoS4m85KUkjaAYjjWEBz/S/IPcU20Q/t1fi1PFP83QdByKO RxwMeP265YuIekc9VSBgEG0aEqDf7ckRvoul8BsFlws+5rjqNYqI5Z9XX0rmzawvMIjI axYMt388tVnd/NogxCt+JrrP4lUcrVh7nvexlFjqYJ8P/Pb/k0n3SRs8jcwINKZK6rLx EejA6gOXUH5E0sK385h3mvIdv+ZS+UkCWfYbVSvxxNV/Ta44utQyicYStGr3+17aQ6nu PF+ZG2Vhj3Oze/FqWQDpEm7XCiBHoUGaiP5z6BoI972hHbufPVnXwxx209T4TMafefj7 7/7A== X-Gm-Message-State: ANoB5pkwSvI7CP2YBs6/al53HTSCJFJ+ImrlrBXI4+CIBDnWiIkaDquR x3zrYySQVLTtiQsaFDrHEJApzA== X-Google-Smtp-Source: AA0mqf6YIHZPZijJW3drWLmCC0J1i77v6BfIk1GaXTBydT1hr0zvtFJFuTOMD/RjlZsQvYbm7cfMpQ== X-Received: by 2002:a17:902:ed41:b0:175:105a:3087 with SMTP id y1-20020a170902ed4100b00175105a3087mr10067985plb.65.1668618681834; Wed, 16 Nov 2022 09:11:21 -0800 (PST) Received: from google.com (7.104.168.34.bc.googleusercontent.com. [34.168.104.7]) by smtp.gmail.com with ESMTPSA id j6-20020a17090276c600b001788ccecbf5sm12424413plt.31.2022.11.16.09.11.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 16 Nov 2022 09:11:21 -0800 (PST) Date: Wed, 16 Nov 2022 17:11:18 +0000 From: Sean Christopherson To: "Huang, Kai" Cc: "borntraeger@linux.ibm.com" , "kvm-riscv@lists.infradead.org" , "Yao, Yuan" , "tglx@linutronix.de" , "kvm@vger.kernel.org" , "Yamahata, Isaku" , "suzuki.poulose@arm.com" , "pbonzini@redhat.com" , "david@redhat.com" , "linux-mips@vger.kernel.org" , "linuxppc-dev@lists.ozlabs.org" , "linux-riscv@lists.infradead.org" , "kvmarm@lists.linux.dev" , "linux-kernel@vger.kernel.org" , "mjrosato@linux.ibm.com" , "oliver.upton@linux.dev" , "farosas@linux.ibm.com" , "linux-s390@vger.kernel.org" , "palmer@dabbelt.com" , "chenhuacai@kernel.org" , "aou@eecs.berkeley.edu" , "alexandru.elisei@arm.com" , "mpe@ellerman.id.au" , "vkuznets@redhat.com" , "maz@kernel.org" , "anup@brainfault.org" , "frankja@linux.ibm.com" , "james.morse@arm.com" , "farman@linux.ibm.com" , "aleksandar.qemu.devel@gmail.com" , "kvmarm@lists.cs.columbia.edu" , "paul.walmsley@sifive.com" , "linux-arm-kernel@lists.infradead.org" , "atishp@atishpatra.org" , "imbrenda@linux.ibm.com" , "Gao, Chao" Subject: Re: [PATCH 38/44] KVM: Disable CPU hotplug during hardware enabling Message-ID: References: <20221102231911.3107438-1-seanjc@google.com> <20221102231911.3107438-39-seanjc@google.com> <88e920944de70e7d69a98f74005b49c59b5aaa3b.camel@intel.com> <95ca433349eca601bdd2b16d70f59ba8e56d8e3f.camel@intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <95ca433349eca601bdd2b16d70f59ba8e56d8e3f.camel@intel.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221116_091124_897946_42AE376C X-CRM114-Status: GOOD ( 22.34 ) X-BeenThere: linux-riscv@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-riscv" Errors-To: linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org On Wed, Nov 16, 2022, Huang, Kai wrote: > On Tue, 2022-11-15 at 20:16 +0000, Sean Christopherson wrote: > > On Thu, Nov 10, 2022, Huang, Kai wrote: > > > On Thu, 2022-11-10 at 01:33 +0000, Huang, Kai wrote: > > > Hmm.. I wasn't thinking thoroughly. I forgot CPU compatibility check also > > > happens on all online cpus when loading KVM. For this case, IRQ is disabled and > > > cpu_active() is true. For the hotplug case, IRQ is enabled but cpu_active() is > > > false. > > > > Actually, you're right (and wrong). You're right in that the WARN is flawed. And > > the reason for that is because you're wrong about the hotplug case. In this version > > of things, the compatibility checks are routed through hardware enabling, i.e. this > > flow is used only when loading KVM. This helper should only be called via SMP function > > call, which means that IRQs should always be disabled. > > Did you mean below code change in later patch "[PATCH 39/44] KVM: Drop > kvm_count_lock and instead protect kvm_usage_count with kvm_lock"? > > /* > * Abort the CPU online process if hardware virtualization cannot > * be enabled. Otherwise running VMs would encounter unrecoverable > @@ -5039,13 +5039,16 @@ static int kvm_online_cpu(unsigned int cpu) > if (kvm_usage_count) { > WARN_ON_ONCE(atomic_read(&hardware_enable_failed)); > > + local_irq_save(flags); > hardware_enable_nolock(NULL); > + local_irq_restore(flags); Sort of. What I was saying is that in this v1, the compatibility checks that are done during harware enabling are initiated from vendor code, i.e. VMX and SVM call {svm,vmx}_check_processor_compat() directly. As a result, the compat checks that are handled in common code: if (__cr4_reserved_bits(cpu_has, c) != __cr4_reserved_bits(cpu_has, &boot_cpu_data)) return -EIO; are skipped. And if that's fixed, then the above hardware_enable_nolock() call will bounce through kvm_x86_check_processor_compatibility() with IRQs enabled once the KVM hotplug hook is moved to the ONLINE section. As above, the simple "fix" would be to disable IRQs, but that's not actually necessary. The only requirement is that preemption is disabled so that the checks are done on the current CPU. The "IRQs disabled" check was a deliberately agressive WARN that was added to guard against doing compatibility checks from the "wrong" location. E.g. this is what I ended up with for a changelog to drop the irqs_disabled() check and for the end code (though it's not tested yet...) Drop kvm_x86_check_processor_compatibility()'s WARN that IRQs are disabled, as the ONLINE section runs with IRQs disabled. The WARN wasn't intended to be a requirement, e.g. disabling preemption is sufficient, the IRQ thing was purely an aggressive sanity check since the helper was only ever invoked via SMP function call. static int kvm_x86_check_processor_compatibility(void) { int cpu = smp_processor_id(); struct cpuinfo_x86 *c = &cpu_data(cpu); /* * Compatibility checks are done when loading KVM and when enabling * hardware, e.g. during CPU hotplug, to ensure all online CPUs are * compatible, i.e. KVM should never perform a compatibility check on * an offline CPU. */ WARN_ON(!cpu_online(cpu)); if (__cr4_reserved_bits(cpu_has, c) != __cr4_reserved_bits(cpu_has, &boot_cpu_data)) return -EIO; return static_call(kvm_x86_check_processor_compatibility)(); } int kvm_arch_hardware_enable(void) { struct kvm *kvm; struct kvm_vcpu *vcpu; unsigned long i; int ret; u64 local_tsc; u64 max_tsc = 0; bool stable, backwards_tsc = false; kvm_user_return_msr_cpu_online(); ret = kvm_x86_check_processor_compatibility(); if (ret) return ret; ret = static_call(kvm_x86_hardware_enable)(); if (ret != 0) return ret; .... } _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv 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 Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id DC1DCC4332F for ; Wed, 16 Nov 2022 17:12:28 +0000 (UTC) Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4NC8k32wj3z3f6s for ; Thu, 17 Nov 2022 04:12:27 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.a=rsa-sha256 header.s=20210112 header.b=TdgWtef6; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=google.com (client-ip=2607:f8b0:4864:20::632; helo=mail-pl1-x632.google.com; envelope-from=seanjc@google.com; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.a=rsa-sha256 header.s=20210112 header.b=TdgWtef6; dkim-atps=neutral Received: from mail-pl1-x632.google.com (mail-pl1-x632.google.com [IPv6:2607:f8b0:4864:20::632]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4NC8hs1hdrz3fGb for ; Thu, 17 Nov 2022 04:11:24 +1100 (AEDT) Received: by mail-pl1-x632.google.com with SMTP id l2so16981091pld.13 for ; Wed, 16 Nov 2022 09:11:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=qU5KpuuvMvabmlf4l/Mi3tQhh2fL8Dh2/iugoH77F0k=; b=TdgWtef6UhHbqKe4QS/3Ayu3VNZZ3i9eoJmtCy7dLqrysJ7wB0UBIiB1NHl2XD8yEJ 9A9e40vVPz0LeaGnBm+xWkbLjcxsdpGQjMtsawyPcUDXFiJtlqbZJWuF4NPJNhW8gWhc yklS7sdK1V06+Vo7Vtm2MrCNKTPLN0kZs4hpWbYlAx+pUoMaBEUe2EJ/R8vPCcvbrDoh 3WsWgepDpESG+KKK928nilFdVglC19Gmb2mv5/mmmkx20fEfIehPjExF++q08zwUw6nJ A1VBgI5fVGcYhdLGJAFBS9e+51HNi1lhj80m/YmoiHsls5U6I/EdT06kMnKY5Lu6o+oq Zk/Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=qU5KpuuvMvabmlf4l/Mi3tQhh2fL8Dh2/iugoH77F0k=; b=xs7THzsde0x+8u21SUv0oh8hxWMyEK/Xw3bazOMg+n88C7oQuH8Fi4KzN5AP4QvT61 48oXlGDuugirN/3HQ3N45MZj0nBpNakYAI94F99rn5N/S01zCHvL/mCRHPhHrAxjH17A ggQ29F1inmPozpSjwpISyL0Jmon+1kr+panCT+SgwqSalY5JK3X9AC2vTKIPWkjXU1A2 epzWium1Tj5eMworhuHhzlt7suvbM90og/jIfHK8zov0m/v3U/iBFxFbW0cPe/h47rRS Qboy4o2m6yXUlWs/xfcu4fEzHKqnp3kh8Inrx7at5qt4A9yJ2gl81oWZ0qGWZYTbZtLt VgSA== X-Gm-Message-State: ANoB5pmaHbSHKLaRbRfXBtpdFxlPwDPFUmdHNoyOPvIM2g0wZQXWaz1n edq/lgcSoRVbzFPTk8EvTJFwDg== X-Google-Smtp-Source: AA0mqf6YIHZPZijJW3drWLmCC0J1i77v6BfIk1GaXTBydT1hr0zvtFJFuTOMD/RjlZsQvYbm7cfMpQ== X-Received: by 2002:a17:902:ed41:b0:175:105a:3087 with SMTP id y1-20020a170902ed4100b00175105a3087mr10067985plb.65.1668618681834; Wed, 16 Nov 2022 09:11:21 -0800 (PST) Received: from google.com (7.104.168.34.bc.googleusercontent.com. [34.168.104.7]) by smtp.gmail.com with ESMTPSA id j6-20020a17090276c600b001788ccecbf5sm12424413plt.31.2022.11.16.09.11.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 16 Nov 2022 09:11:21 -0800 (PST) Date: Wed, 16 Nov 2022 17:11:18 +0000 From: Sean Christopherson To: "Huang, Kai" Subject: Re: [PATCH 38/44] KVM: Disable CPU hotplug during hardware enabling Message-ID: References: <20221102231911.3107438-1-seanjc@google.com> <20221102231911.3107438-39-seanjc@google.com> <88e920944de70e7d69a98f74005b49c59b5aaa3b.camel@intel.com> <95ca433349eca601bdd2b16d70f59ba8e56d8e3f.camel@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <95ca433349eca601bdd2b16d70f59ba8e56d8e3f.camel@intel.com> X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "mjrosato@linux.ibm.com" , "david@redhat.com" , "paul.walmsley@sifive.com" , "linux-kernel@vger.kernel.org" , "linux-riscv@lists.infradead.org" , "imbrenda@linux.ibm.com" , "kvmarm@lists.cs.columbia.edu" , "linux-s390@vger.kernel.org" , "kvm@vger.kernel.org" , "chenhuacai@kernel.org" , "aleksandar.qemu.devel@gmail.com" , "james.morse@arm.com" , "borntraeger@linux.ibm.com" , "Gao, Chao" , "farman@linux.ibm.com" , "aou@eecs.berkeley.edu" , "suzuki.poulose@arm.com" , "Yao, Yuan" , "kvmarm@lists.linux.dev" , "tglx@linutro nix.de" , "alexandru.elisei@arm.com" , "linux-arm-kernel@lists.infradead.org" , "frankja@linux.ibm.com" , "Yamahata, Isaku" , "atishp@atishpatra.org" , "farosas@linux.ibm.com" , "anup@brainfault.org" , "linux-mips@vger.kernel.org" , "oliver.upton@linux.dev" , "palmer@dabbelt.com" , "kvm-riscv@lists.infradead.org" , "maz@kernel.org" , "pbonzini@redhat.com" , "vkuznets@redhat.com" , "linuxppc-dev@lists.ozlabs.org" Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" On Wed, Nov 16, 2022, Huang, Kai wrote: > On Tue, 2022-11-15 at 20:16 +0000, Sean Christopherson wrote: > > On Thu, Nov 10, 2022, Huang, Kai wrote: > > > On Thu, 2022-11-10 at 01:33 +0000, Huang, Kai wrote: > > > Hmm.. I wasn't thinking thoroughly. I forgot CPU compatibility check also > > > happens on all online cpus when loading KVM. For this case, IRQ is disabled and > > > cpu_active() is true. For the hotplug case, IRQ is enabled but cpu_active() is > > > false. > > > > Actually, you're right (and wrong). You're right in that the WARN is flawed. And > > the reason for that is because you're wrong about the hotplug case. In this version > > of things, the compatibility checks are routed through hardware enabling, i.e. this > > flow is used only when loading KVM. This helper should only be called via SMP function > > call, which means that IRQs should always be disabled. > > Did you mean below code change in later patch "[PATCH 39/44] KVM: Drop > kvm_count_lock and instead protect kvm_usage_count with kvm_lock"? > > /* > * Abort the CPU online process if hardware virtualization cannot > * be enabled. Otherwise running VMs would encounter unrecoverable > @@ -5039,13 +5039,16 @@ static int kvm_online_cpu(unsigned int cpu) > if (kvm_usage_count) { > WARN_ON_ONCE(atomic_read(&hardware_enable_failed)); > > + local_irq_save(flags); > hardware_enable_nolock(NULL); > + local_irq_restore(flags); Sort of. What I was saying is that in this v1, the compatibility checks that are done during harware enabling are initiated from vendor code, i.e. VMX and SVM call {svm,vmx}_check_processor_compat() directly. As a result, the compat checks that are handled in common code: if (__cr4_reserved_bits(cpu_has, c) != __cr4_reserved_bits(cpu_has, &boot_cpu_data)) return -EIO; are skipped. And if that's fixed, then the above hardware_enable_nolock() call will bounce through kvm_x86_check_processor_compatibility() with IRQs enabled once the KVM hotplug hook is moved to the ONLINE section. As above, the simple "fix" would be to disable IRQs, but that's not actually necessary. The only requirement is that preemption is disabled so that the checks are done on the current CPU. The "IRQs disabled" check was a deliberately agressive WARN that was added to guard against doing compatibility checks from the "wrong" location. E.g. this is what I ended up with for a changelog to drop the irqs_disabled() check and for the end code (though it's not tested yet...) Drop kvm_x86_check_processor_compatibility()'s WARN that IRQs are disabled, as the ONLINE section runs with IRQs disabled. The WARN wasn't intended to be a requirement, e.g. disabling preemption is sufficient, the IRQ thing was purely an aggressive sanity check since the helper was only ever invoked via SMP function call. static int kvm_x86_check_processor_compatibility(void) { int cpu = smp_processor_id(); struct cpuinfo_x86 *c = &cpu_data(cpu); /* * Compatibility checks are done when loading KVM and when enabling * hardware, e.g. during CPU hotplug, to ensure all online CPUs are * compatible, i.e. KVM should never perform a compatibility check on * an offline CPU. */ WARN_ON(!cpu_online(cpu)); if (__cr4_reserved_bits(cpu_has, c) != __cr4_reserved_bits(cpu_has, &boot_cpu_data)) return -EIO; return static_call(kvm_x86_check_processor_compatibility)(); } int kvm_arch_hardware_enable(void) { struct kvm *kvm; struct kvm_vcpu *vcpu; unsigned long i; int ret; u64 local_tsc; u64 max_tsc = 0; bool stable, backwards_tsc = false; kvm_user_return_msr_cpu_online(); ret = kvm_x86_check_processor_compatibility(); if (ret) return ret; ret = static_call(kvm_x86_hardware_enable)(); if (ret != 0) return ret; .... } 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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 92417C433FE for ; Wed, 16 Nov 2022 17:12:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=y9htp9eGaUDP2XfDtabGY/OH2X28UksTmBf/a7OZ/8k=; b=N4kzG5sBUPO34q 1dIrBYtEC7V9hKL7H60P7Ydo+i9UEUHosPLzHRZVpaj1Pg0Al5wEv0mDHvdrDI8lGTrGmt9vShRzM 15yBYqysq6GkjOoYP/VLjZU4S1HuZZSH9phQ5Hjyze6mYQ3osi8l77+9wiO0kQI0yZOJTi9Zn9qy9 3wMF1oEFOAjPsRvbAa/HX64YU3cTWwtzUwFSU48uPd3wzjB6wf/o9VhDGu1AMeOF8vr15jQD+z9J4 Rw5VaUb6KdHOSnv86VoJFNvvtgf1+l24Rdkc0Q9MLD8LUKgZAZ9IrKoydcpzHjWFAC04ywr5jCwtV A7lTrgxmY6MZHePZHDIA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ovLwj-006JMN-QW; Wed, 16 Nov 2022 17:11:35 +0000 Received: from mail-pj1-x1034.google.com ([2607:f8b0:4864:20::1034]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1ovLwa-006JIQ-QS for linux-arm-kernel@lists.infradead.org; Wed, 16 Nov 2022 17:11:26 +0000 Received: by mail-pj1-x1034.google.com with SMTP id h14so17097916pjv.4 for ; Wed, 16 Nov 2022 09:11:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=qU5KpuuvMvabmlf4l/Mi3tQhh2fL8Dh2/iugoH77F0k=; b=TdgWtef6UhHbqKe4QS/3Ayu3VNZZ3i9eoJmtCy7dLqrysJ7wB0UBIiB1NHl2XD8yEJ 9A9e40vVPz0LeaGnBm+xWkbLjcxsdpGQjMtsawyPcUDXFiJtlqbZJWuF4NPJNhW8gWhc yklS7sdK1V06+Vo7Vtm2MrCNKTPLN0kZs4hpWbYlAx+pUoMaBEUe2EJ/R8vPCcvbrDoh 3WsWgepDpESG+KKK928nilFdVglC19Gmb2mv5/mmmkx20fEfIehPjExF++q08zwUw6nJ A1VBgI5fVGcYhdLGJAFBS9e+51HNi1lhj80m/YmoiHsls5U6I/EdT06kMnKY5Lu6o+oq Zk/Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=qU5KpuuvMvabmlf4l/Mi3tQhh2fL8Dh2/iugoH77F0k=; b=xrdBqXDQ8LYVy0E38yIoDUVDQg/9h6r5G4gzi/VsduQ4YzEG05Mvch1cP4v4ezjCnX zUjKxnXWgI5cCfLOJkrze9+U4QgDJNRMjzmkUuPnd1fekF8bqHPftvSutn4LjufMXkqO Wopm67xZjsv5cnf19OfrabgFQw897cN4fEZDMHUxJ4TnBW92gbiv/wKkt3wWFRBlOfg5 LoMkf87NqRAPhpg45ASRnR+dB2SdbktXMmlYmOxzJupsL2H+ZQSsetoF/Th6ilY4ndGg RvxBjhqsFMNuI84OfJa075ZYw929OYNY08Y/WFzVoqj/aqJ6tHliZMwXALxRLmhI4g+2 AZmA== X-Gm-Message-State: ANoB5plIOxgUISF8nkft5mxiuujxFUOei5P7fneEktwTvmy3TeSp8FZg 29pNxehMZ5clJMjXOFsFrf0mCw== X-Google-Smtp-Source: AA0mqf6YIHZPZijJW3drWLmCC0J1i77v6BfIk1GaXTBydT1hr0zvtFJFuTOMD/RjlZsQvYbm7cfMpQ== X-Received: by 2002:a17:902:ed41:b0:175:105a:3087 with SMTP id y1-20020a170902ed4100b00175105a3087mr10067985plb.65.1668618681834; Wed, 16 Nov 2022 09:11:21 -0800 (PST) Received: from google.com (7.104.168.34.bc.googleusercontent.com. [34.168.104.7]) by smtp.gmail.com with ESMTPSA id j6-20020a17090276c600b001788ccecbf5sm12424413plt.31.2022.11.16.09.11.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 16 Nov 2022 09:11:21 -0800 (PST) Date: Wed, 16 Nov 2022 17:11:18 +0000 From: Sean Christopherson To: "Huang, Kai" Cc: "borntraeger@linux.ibm.com" , "kvm-riscv@lists.infradead.org" , "Yao, Yuan" , "tglx@linutronix.de" , "kvm@vger.kernel.org" , "Yamahata, Isaku" , "suzuki.poulose@arm.com" , "pbonzini@redhat.com" , "david@redhat.com" , "linux-mips@vger.kernel.org" , "linuxppc-dev@lists.ozlabs.org" , "linux-riscv@lists.infradead.org" , "kvmarm@lists.linux.dev" , "linux-kernel@vger.kernel.org" , "mjrosato@linux.ibm.com" , "oliver.upton@linux.dev" , "farosas@linux.ibm.com" , "linux-s390@vger.kernel.org" , "palmer@dabbelt.com" , "chenhuacai@kernel.org" , "aou@eecs.berkeley.edu" , "alexandru.elisei@arm.com" , "mpe@ellerman.id.au" , "vkuznets@redhat.com" , "maz@kernel.org" , "anup@brainfault.org" , "frankja@linux.ibm.com" , "james.morse@arm.com" , "farman@linux.ibm.com" , "aleksandar.qemu.devel@gmail.com" , "kvmarm@lists.cs.columbia.edu" , "paul.walmsley@sifive.com" , "linux-arm-kernel@lists.infradead.org" , "atishp@atishpatra.org" , "imbrenda@linux.ibm.com" , "Gao, Chao" Subject: Re: [PATCH 38/44] KVM: Disable CPU hotplug during hardware enabling Message-ID: References: <20221102231911.3107438-1-seanjc@google.com> <20221102231911.3107438-39-seanjc@google.com> <88e920944de70e7d69a98f74005b49c59b5aaa3b.camel@intel.com> <95ca433349eca601bdd2b16d70f59ba8e56d8e3f.camel@intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <95ca433349eca601bdd2b16d70f59ba8e56d8e3f.camel@intel.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221116_091124_890652_60A03105 X-CRM114-Status: GOOD ( 23.75 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Nov 16, 2022, Huang, Kai wrote: > On Tue, 2022-11-15 at 20:16 +0000, Sean Christopherson wrote: > > On Thu, Nov 10, 2022, Huang, Kai wrote: > > > On Thu, 2022-11-10 at 01:33 +0000, Huang, Kai wrote: > > > Hmm.. I wasn't thinking thoroughly. I forgot CPU compatibility check also > > > happens on all online cpus when loading KVM. For this case, IRQ is disabled and > > > cpu_active() is true. For the hotplug case, IRQ is enabled but cpu_active() is > > > false. > > > > Actually, you're right (and wrong). You're right in that the WARN is flawed. And > > the reason for that is because you're wrong about the hotplug case. In this version > > of things, the compatibility checks are routed through hardware enabling, i.e. this > > flow is used only when loading KVM. This helper should only be called via SMP function > > call, which means that IRQs should always be disabled. > > Did you mean below code change in later patch "[PATCH 39/44] KVM: Drop > kvm_count_lock and instead protect kvm_usage_count with kvm_lock"? > > /* > * Abort the CPU online process if hardware virtualization cannot > * be enabled. Otherwise running VMs would encounter unrecoverable > @@ -5039,13 +5039,16 @@ static int kvm_online_cpu(unsigned int cpu) > if (kvm_usage_count) { > WARN_ON_ONCE(atomic_read(&hardware_enable_failed)); > > + local_irq_save(flags); > hardware_enable_nolock(NULL); > + local_irq_restore(flags); Sort of. What I was saying is that in this v1, the compatibility checks that are done during harware enabling are initiated from vendor code, i.e. VMX and SVM call {svm,vmx}_check_processor_compat() directly. As a result, the compat checks that are handled in common code: if (__cr4_reserved_bits(cpu_has, c) != __cr4_reserved_bits(cpu_has, &boot_cpu_data)) return -EIO; are skipped. And if that's fixed, then the above hardware_enable_nolock() call will bounce through kvm_x86_check_processor_compatibility() with IRQs enabled once the KVM hotplug hook is moved to the ONLINE section. As above, the simple "fix" would be to disable IRQs, but that's not actually necessary. The only requirement is that preemption is disabled so that the checks are done on the current CPU. The "IRQs disabled" check was a deliberately agressive WARN that was added to guard against doing compatibility checks from the "wrong" location. E.g. this is what I ended up with for a changelog to drop the irqs_disabled() check and for the end code (though it's not tested yet...) Drop kvm_x86_check_processor_compatibility()'s WARN that IRQs are disabled, as the ONLINE section runs with IRQs disabled. The WARN wasn't intended to be a requirement, e.g. disabling preemption is sufficient, the IRQ thing was purely an aggressive sanity check since the helper was only ever invoked via SMP function call. static int kvm_x86_check_processor_compatibility(void) { int cpu = smp_processor_id(); struct cpuinfo_x86 *c = &cpu_data(cpu); /* * Compatibility checks are done when loading KVM and when enabling * hardware, e.g. during CPU hotplug, to ensure all online CPUs are * compatible, i.e. KVM should never perform a compatibility check on * an offline CPU. */ WARN_ON(!cpu_online(cpu)); if (__cr4_reserved_bits(cpu_has, c) != __cr4_reserved_bits(cpu_has, &boot_cpu_data)) return -EIO; return static_call(kvm_x86_check_processor_compatibility)(); } int kvm_arch_hardware_enable(void) { struct kvm *kvm; struct kvm_vcpu *vcpu; unsigned long i; int ret; u64 local_tsc; u64 max_tsc = 0; bool stable, backwards_tsc = false; kvm_user_return_msr_cpu_online(); ret = kvm_x86_check_processor_compatibility(); if (ret) return ret; ret = static_call(kvm_x86_hardware_enable)(); if (ret != 0) return ret; .... } _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel