From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sean Christopherson Date: Wed, 16 Nov 2022 15:52:26 +0000 Subject: [PATCH 13/44] KVM: x86: Serialize vendor module initialization (hardware setup) In-Reply-To: References: <20221102231911.3107438-1-seanjc@google.com> <20221102231911.3107438-14-seanjc@google.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 Wed, 2022-11-02 at 23:18 +0000, Sean Christopherson wrote: > > Acquire a new mutex, vendor_module_lock, in kvm_x86_vendor_init() while > > doing hardware setup to ensure that concurrent calls are fully serialized. > > KVM rejects attempts to load vendor modules if a different module has > > already been loaded, but doesn't handle the case where multiple vendor > > modules are loaded at the same time, and module_init() doesn't run under > > the global module_mutex. > > > > Note, in practice, this is likely a benign bug as no platform exists that > > supports both SVM and VMX, i.e. barring a weird VM setup, one of the > > vendor modules is guaranteed to fail a support check before modifying > > common KVM state. > > > > Alternatively, KVM could perform an atomic CMPXCHG on .hardware_enable, > > but that comes with its own ugliness as it would require setting > > .hardware_enable before success is guaranteed, e.g. attempting to load > > the "wrong" could result in spurious failure to load the "right" module. > > > > Introduce a new mutex as using kvm_lock is extremely deadlock prone due > > to kvm_lock being taken under cpus_write_lock(), and in the future, under > > under cpus_read_lock(). Any operation that takes cpus_read_lock() while > > holding kvm_lock would potentially deadlock, e.g. kvm_timer_init() takes > > cpus_read_lock() to register a callback. In theory, KVM could avoid > > such problematic paths, i.e. do less setup under kvm_lock, but avoiding > > all calls to cpus_read_lock() is subtly difficult and thus fragile. E.g. > > updating static calls also acquires cpus_read_lock(). > > > > Inverting the lock ordering, i.e. always taking kvm_lock outside > > cpus_read_lock(), is not a viable option, e.g. kvm_online_cpu() takes > > kvm_lock and is called under cpus_write_lock(). > > "kvm_online_cpu() takes kvm_lock and is called under cpus_write_lock()" hasn't > happened yet. Doh, right. Thanks! > > The lockdep splat below is dependent on future patches to take > > cpus_read_lock() in hardware_enable_all(), but as above, deadlock is > > already is already possible. > > IIUC kvm_lock by design is supposed to protect vm_list, thus IMHO naturally it > doesn't fit to protect multiple vendor module loading. A different way to look at it is that kvm_lock protects anything that is global to all of KVM, and it just so happens that lists and counters of VMs are the only such resources (lumping in the usage in vm_uevent_notify_change() and the future usage to protect kvm_usage_count). > Looks above argument is good enough. I am not sure whether we need additional > justification which comes from future patches. :) To try to prevent someone from trying to eliminate the "extra" lock, like this series does for kvm_count_lock. Hopefully future someones that want to clean up the code do a git blame to understand why the lock was introduced and don't waste their time running into the same issues (or worse, don't run into the issues and break KVM). > Also, do you also want to update Documentation/virt/kvm/locking.rst" in this > patch? Hmm, yeah. That'd also be a good place to document why kvm_lock isn't used. 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 7C7D1C4332F for ; Wed, 16 Nov 2022 15:52:36 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id E299A4B8A2; Wed, 16 Nov 2022 10:52:35 -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 KggoPV6SnLZt; Wed, 16 Nov 2022 10:52:34 -0500 (EST) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id D965D4B8CD; Wed, 16 Nov 2022 10:52:34 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 81F064B8A2 for ; Wed, 16 Nov 2022 10:52:33 -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 TfF1b3yXLfto for ; Wed, 16 Nov 2022 10:52:32 -0500 (EST) Received: from mail-pj1-f51.google.com (mail-pj1-f51.google.com [209.85.216.51]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 412F14B897 for ; Wed, 16 Nov 2022 10:52:32 -0500 (EST) Received: by mail-pj1-f51.google.com with SMTP id v4-20020a17090a088400b00212cb0ed97eso2768721pjc.5 for ; Wed, 16 Nov 2022 07:52:32 -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=coErVjWyqhzO9EuKB5XSmlh7IJj5NvKWNvG7xdrbhC4=; b=Axl8RQ6KDdRrT9Z9C/Q7WDP07ILGsJIsXvmq6hEH7xYRxr1KYM7WfFS4wN+lhhZ4fX c4Jtlogr7Ros9vXyyEFv/zFAE8+4peFtT7kH9mbNBRnzYJ/2b8sYrYCafzb/t5b9hh4H MzZxnel8UMmtPU7lGv2FYN+iMfYeZiXKTt6kJLjx0bmpPltUoJ0RtRfd1Ec/nG1HLpUR NvlX1cr6oDpawENjKiQcMRdisM4zgyG2koLhhizlhvLd74nRufWrbXHhbtX0zrVof0Oh 3EiecDo5X1cIJiERj7KVmypmulB3/lI1LWoLIDeuu1zT2CC/G2udJFJRosnSOWJ19zuG AhsA== 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=coErVjWyqhzO9EuKB5XSmlh7IJj5NvKWNvG7xdrbhC4=; b=IWbAyql7UKMfORkEciWHWmGGV2F3tqA66lbuFw3DnnQkPH3Yf8yfJv8GMF1Jgab8SB jdenQk3n2dmyzajJNM+OPadPVQhgTc7orqI5IRgelDk76E7lbplWWRermgQ4keJxv9CK sfIoBlev6zISt+xOSQ8S6xUPpd0Mm4D2pFynZR6ClAPeYRLXWJ3x/Y9zdxRrYK4s6MN0 INhPdLEJHFNmhJ3n+Qv/kipr9ApZ/cTor/bJvWjCBsElYxB6tDWve+pzGrjMcUDaoUCZ 1tsnlvcnWta5XruBKrMO89sqaUeIyggKB53PM7ZdYAZ6+BgeuH+kn9D4+xZxqXmpfNk0 Fq0Q== X-Gm-Message-State: ANoB5pk7/JM9U66sc/wBMoH1ybzTQe5FS8s9bNmyko2HZ844K2JUNfOk DRJaBm/DlrSyjWc/td/+GmMaDQ== X-Google-Smtp-Source: AA0mqf7dFwkFbcATwoaCt8aciY1qylOnTaB4gr/nPlKa0OFTmbzR8xQHyljgxnYchchnXSvQiHP/Xg== X-Received: by 2002:a17:902:6944:b0:188:640f:f401 with SMTP id k4-20020a170902694400b00188640ff401mr9754670plt.44.1668613950911; Wed, 16 Nov 2022 07:52:30 -0800 (PST) Received: from google.com (7.104.168.34.bc.googleusercontent.com. [34.168.104.7]) by smtp.gmail.com with ESMTPSA id b14-20020a170902650e00b00177e5d83d3esm12341507plk.88.2022.11.16.07.52.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 16 Nov 2022 07:52:30 -0800 (PST) Date: Wed, 16 Nov 2022 15:52:26 +0000 From: Sean Christopherson To: "Huang, Kai" Subject: Re: [PATCH 13/44] KVM: x86: Serialize vendor module initialization (hardware setup) Message-ID: References: <20221102231911.3107438-1-seanjc@google.com> <20221102231911.3107438-14-seanjc@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Cc: "mjrosato@linux.ibm.com" , "david@redhat.com" , "Yao, Yuan" , "linux-mips@vger.kernel.org" , "linux-riscv@lists.infradead.org" , "imbrenda@linux.ibm.com" , "kvmarm@lists.cs.columbia.edu" , "linux-s390@vger.kernel.org" , "frankja@linux.ibm.com" , "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" , "kvm@vger.kernel.org" , "paul.walmsley@sifive.com" , "kvmarm@lists.linux.dev" , "tglx@linutronix.de" , "linux-arm-kernel@lists.infradead.org" , "Yamahata, Isaku" , "atishp@atishpatra.org" , "farosas@linux.ibm.com" , "linux-kernel@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 Wed, 2022-11-02 at 23:18 +0000, Sean Christopherson wrote: > > Acquire a new mutex, vendor_module_lock, in kvm_x86_vendor_init() while > > doing hardware setup to ensure that concurrent calls are fully serialized. > > KVM rejects attempts to load vendor modules if a different module has > > already been loaded, but doesn't handle the case where multiple vendor > > modules are loaded at the same time, and module_init() doesn't run under > > the global module_mutex. > > > > Note, in practice, this is likely a benign bug as no platform exists that > > supports both SVM and VMX, i.e. barring a weird VM setup, one of the > > vendor modules is guaranteed to fail a support check before modifying > > common KVM state. > > > > Alternatively, KVM could perform an atomic CMPXCHG on .hardware_enable, > > but that comes with its own ugliness as it would require setting > > .hardware_enable before success is guaranteed, e.g. attempting to load > > the "wrong" could result in spurious failure to load the "right" module. > > > > Introduce a new mutex as using kvm_lock is extremely deadlock prone due > > to kvm_lock being taken under cpus_write_lock(), and in the future, under > > under cpus_read_lock(). Any operation that takes cpus_read_lock() while > > holding kvm_lock would potentially deadlock, e.g. kvm_timer_init() takes > > cpus_read_lock() to register a callback. In theory, KVM could avoid > > such problematic paths, i.e. do less setup under kvm_lock, but avoiding > > all calls to cpus_read_lock() is subtly difficult and thus fragile. E.g. > > updating static calls also acquires cpus_read_lock(). > > > > Inverting the lock ordering, i.e. always taking kvm_lock outside > > cpus_read_lock(), is not a viable option, e.g. kvm_online_cpu() takes > > kvm_lock and is called under cpus_write_lock(). > > "kvm_online_cpu() takes kvm_lock and is called under cpus_write_lock()" hasn't > happened yet. Doh, right. Thanks! > > The lockdep splat below is dependent on future patches to take > > cpus_read_lock() in hardware_enable_all(), but as above, deadlock is > > already is already possible. > > IIUC kvm_lock by design is supposed to protect vm_list, thus IMHO naturally it > doesn't fit to protect multiple vendor module loading. A different way to look at it is that kvm_lock protects anything that is global to all of KVM, and it just so happens that lists and counters of VMs are the only such resources (lumping in the usage in vm_uevent_notify_change() and the future usage to protect kvm_usage_count). > Looks above argument is good enough. I am not sure whether we need additional > justification which comes from future patches. :) To try to prevent someone from trying to eliminate the "extra" lock, like this series does for kvm_count_lock. Hopefully future someones that want to clean up the code do a git blame to understand why the lock was introduced and don't waste their time running into the same issues (or worse, don't run into the issues and break KVM). > Also, do you also want to update Documentation/virt/kvm/locking.rst" in this > patch? Hmm, yeah. That'd also be a good place to document why kvm_lock isn't used. _______________________________________________ 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-f44.google.com (mail-pj1-f44.google.com [209.85.216.44]) (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 ACBD56118 for ; Wed, 16 Nov 2022 15:52:31 +0000 (UTC) Received: by mail-pj1-f44.google.com with SMTP id h14so16899700pjv.4 for ; Wed, 16 Nov 2022 07:52:31 -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=coErVjWyqhzO9EuKB5XSmlh7IJj5NvKWNvG7xdrbhC4=; b=Axl8RQ6KDdRrT9Z9C/Q7WDP07ILGsJIsXvmq6hEH7xYRxr1KYM7WfFS4wN+lhhZ4fX c4Jtlogr7Ros9vXyyEFv/zFAE8+4peFtT7kH9mbNBRnzYJ/2b8sYrYCafzb/t5b9hh4H MzZxnel8UMmtPU7lGv2FYN+iMfYeZiXKTt6kJLjx0bmpPltUoJ0RtRfd1Ec/nG1HLpUR NvlX1cr6oDpawENjKiQcMRdisM4zgyG2koLhhizlhvLd74nRufWrbXHhbtX0zrVof0Oh 3EiecDo5X1cIJiERj7KVmypmulB3/lI1LWoLIDeuu1zT2CC/G2udJFJRosnSOWJ19zuG AhsA== 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=coErVjWyqhzO9EuKB5XSmlh7IJj5NvKWNvG7xdrbhC4=; b=ZfpvgJOm1ik9sLuCkOeIIR/g/XY1lO9ZsyIsxHIfyNGuAlZpH91tkxuTzJWyV5jmbz 4fJG/KGB/tb3EIz50yrxpSz9eq9vFSeByMD/eHU4gOdxIu6SH1ubx5tvo2FPDOXYHRkO sE7QrEwdrXBkj5rVqOdxtRx9AlYsIs7PTaqLRZvc9MetfD/1LEEy0cW9r8mDPzEqvHT6 ny6QsF3j8NT8oY6RDdBsFn4TWKlojGxM44Duk6sCqq0UNvxkbBmEZBfEOiN9vPE20QwM xzvQq6+lny/IotRhLr1kWt/0NGQSvz6EnWDGmw+ZWjlGC+kfESFKNQF3/O3c/xdS3Jhy WtWw== X-Gm-Message-State: ANoB5pnGzw+EbpUSwJGgF27189/dOuV/dCZnJGIaY8mD/M4Kl4DyMPOc q/4OX2RTn6ajbLhX3CFJjsXDxg== X-Google-Smtp-Source: AA0mqf7dFwkFbcATwoaCt8aciY1qylOnTaB4gr/nPlKa0OFTmbzR8xQHyljgxnYchchnXSvQiHP/Xg== X-Received: by 2002:a17:902:6944:b0:188:640f:f401 with SMTP id k4-20020a170902694400b00188640ff401mr9754670plt.44.1668613950911; Wed, 16 Nov 2022 07:52:30 -0800 (PST) Received: from google.com (7.104.168.34.bc.googleusercontent.com. [34.168.104.7]) by smtp.gmail.com with ESMTPSA id b14-20020a170902650e00b00177e5d83d3esm12341507plk.88.2022.11.16.07.52.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 16 Nov 2022 07:52:30 -0800 (PST) Date: Wed, 16 Nov 2022 15:52:26 +0000 From: Sean Christopherson To: "Huang, Kai" Cc: "imbrenda@linux.ibm.com" , "aou@eecs.berkeley.edu" , "mjrosato@linux.ibm.com" , "vkuznets@redhat.com" , "farman@linux.ibm.com" , "chenhuacai@kernel.org" , "paul.walmsley@sifive.com" , "palmer@dabbelt.com" , "maz@kernel.org" , "anup@brainfault.org" , "pbonzini@redhat.com" , "borntraeger@linux.ibm.com" , "aleksandar.qemu.devel@gmail.com" , "frankja@linux.ibm.com" , "oliver.upton@linux.dev" , "kvm@vger.kernel.org" , "Yao, Yuan" , "farosas@linux.ibm.com" , "david@redhat.com" , "james.morse@arm.com" , "mpe@ellerman.id.au" , "alexandru.elisei@arm.com" , "linux-s390@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "tglx@linutronix.de" , "Yamahata, Isaku" , "kvmarm@lists.linux.dev" , "suzuki.poulose@arm.com" , "kvm-riscv@lists.infradead.org" , "linuxppc-dev@lists.ozlabs.org" , "linux-arm-kernel@lists.infradead.org" , "linux-mips@vger.kernel.org" , "kvmarm@lists.cs.columbia.edu" , "Gao, Chao" , "atishp@atishpatra.org" , "linux-riscv@lists.infradead.org" Subject: Re: [PATCH 13/44] KVM: x86: Serialize vendor module initialization (hardware setup) Message-ID: References: <20221102231911.3107438-1-seanjc@google.com> <20221102231911.3107438-14-seanjc@google.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: Message-ID: <20221116155226.s7hskIr9cf1uVUltghZPm_cFRL8yrr0c3NzBKNifiRQ@z> On Wed, Nov 16, 2022, Huang, Kai wrote: > On Wed, 2022-11-02 at 23:18 +0000, Sean Christopherson wrote: > > Acquire a new mutex, vendor_module_lock, in kvm_x86_vendor_init() while > > doing hardware setup to ensure that concurrent calls are fully serialized. > > KVM rejects attempts to load vendor modules if a different module has > > already been loaded, but doesn't handle the case where multiple vendor > > modules are loaded at the same time, and module_init() doesn't run under > > the global module_mutex. > > > > Note, in practice, this is likely a benign bug as no platform exists that > > supports both SVM and VMX, i.e. barring a weird VM setup, one of the > > vendor modules is guaranteed to fail a support check before modifying > > common KVM state. > > > > Alternatively, KVM could perform an atomic CMPXCHG on .hardware_enable, > > but that comes with its own ugliness as it would require setting > > .hardware_enable before success is guaranteed, e.g. attempting to load > > the "wrong" could result in spurious failure to load the "right" module. > > > > Introduce a new mutex as using kvm_lock is extremely deadlock prone due > > to kvm_lock being taken under cpus_write_lock(), and in the future, under > > under cpus_read_lock(). Any operation that takes cpus_read_lock() while > > holding kvm_lock would potentially deadlock, e.g. kvm_timer_init() takes > > cpus_read_lock() to register a callback. In theory, KVM could avoid > > such problematic paths, i.e. do less setup under kvm_lock, but avoiding > > all calls to cpus_read_lock() is subtly difficult and thus fragile. E.g. > > updating static calls also acquires cpus_read_lock(). > > > > Inverting the lock ordering, i.e. always taking kvm_lock outside > > cpus_read_lock(), is not a viable option, e.g. kvm_online_cpu() takes > > kvm_lock and is called under cpus_write_lock(). > > "kvm_online_cpu() takes kvm_lock and is called under cpus_write_lock()" hasn't > happened yet. Doh, right. Thanks! > > The lockdep splat below is dependent on future patches to take > > cpus_read_lock() in hardware_enable_all(), but as above, deadlock is > > already is already possible. > > IIUC kvm_lock by design is supposed to protect vm_list, thus IMHO naturally it > doesn't fit to protect multiple vendor module loading. A different way to look at it is that kvm_lock protects anything that is global to all of KVM, and it just so happens that lists and counters of VMs are the only such resources (lumping in the usage in vm_uevent_notify_change() and the future usage to protect kvm_usage_count). > Looks above argument is good enough. I am not sure whether we need additional > justification which comes from future patches. :) To try to prevent someone from trying to eliminate the "extra" lock, like this series does for kvm_count_lock. Hopefully future someones that want to clean up the code do a git blame to understand why the lock was introduced and don't waste their time running into the same issues (or worse, don't run into the issues and break KVM). > Also, do you also want to update Documentation/virt/kvm/locking.rst" in this > patch? Hmm, yeah. That'd also be a good place to document why kvm_lock isn't used. 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 D7C6FC433FE for ; Wed, 16 Nov 2022 15:52:57 +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=xhgbIjcbGU5GqUUidgsjizH4I7Yp0a/cgxOFHGT9Xxg=; b=kv9zM/7fPTZgp9 VE8n49SmvslTC1vo2z3GmXN6G777FisTukdXTBoT0CB9ww4jJCOI5+Z7vNRxD+DRQmgVVlj5kDoB2 1qrcKmxPZJin65Jy46lOWJBxUvlNz8MOzN/dy8qZNTtysH47tzkMwq0l9pkvxeSaTuyh2jGDPZ6Jw Aq4bFsT4oGn3HnDIgoVNn65vH0Zb4FMfP9bPAjpt/twRhVVjYJLBYNHpAS16sR3KBSjj+w8g1Yn86 hZ2yJLTIBJd+qg/yU8pVJ3/d1NRWyceIW39nv/rSFMCM9tHqAStbwneCez9p1elEidwIEjH6igx/J 0xbwy8Mx7ff3LOWDBnXw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ovKiU-005bh7-M3; Wed, 16 Nov 2022 15:52:46 +0000 Received: from mail-pj1-x102e.google.com ([2607:f8b0:4864:20::102e]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1ovKiI-005bcF-OR for linux-riscv@lists.infradead.org; Wed, 16 Nov 2022 15:52:37 +0000 Received: by mail-pj1-x102e.google.com with SMTP id k5so16888258pjo.5 for ; Wed, 16 Nov 2022 07:52:31 -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=coErVjWyqhzO9EuKB5XSmlh7IJj5NvKWNvG7xdrbhC4=; b=Axl8RQ6KDdRrT9Z9C/Q7WDP07ILGsJIsXvmq6hEH7xYRxr1KYM7WfFS4wN+lhhZ4fX c4Jtlogr7Ros9vXyyEFv/zFAE8+4peFtT7kH9mbNBRnzYJ/2b8sYrYCafzb/t5b9hh4H MzZxnel8UMmtPU7lGv2FYN+iMfYeZiXKTt6kJLjx0bmpPltUoJ0RtRfd1Ec/nG1HLpUR NvlX1cr6oDpawENjKiQcMRdisM4zgyG2koLhhizlhvLd74nRufWrbXHhbtX0zrVof0Oh 3EiecDo5X1cIJiERj7KVmypmulB3/lI1LWoLIDeuu1zT2CC/G2udJFJRosnSOWJ19zuG AhsA== 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=coErVjWyqhzO9EuKB5XSmlh7IJj5NvKWNvG7xdrbhC4=; b=J3tWFnSV+sRxsVldJet71A5EdodhLOZvsQA+DDTXmNKfTpXZhwfR++cjZoCHxexm9a /Fay6vOt8qzyxy3aat5sCd/HYJABqTcocRvHKYpIUQ+tUHUALTbs7eXBU+zJfPGXm5vK 0FrUqeXGwzRgAqKxrNcaSyBSugbLDvwXL1/uKvLlv9YutJoJaTSfPct6QfQXsrb6GJKW /FJGqp6VFwiTDzVPBxpEl/cCC83hAKAtjWCAGUC4pkkD8+/LTbN2aCs9HCeM1SIDDOxc sGkTrK3ZFwMBhgLqcebuB7X6GcnZJsQcE9vUeCInAmezdeVrx2q3wYUV8gzuyphCdFSD Thgw== X-Gm-Message-State: ANoB5pnPpsXImJA116rh0qwTcpiKyPznCiNdbwJAXNErHb9W736u878t 56tP822vvaZ8wdZd1Mi07wj8Fw== X-Google-Smtp-Source: AA0mqf7dFwkFbcATwoaCt8aciY1qylOnTaB4gr/nPlKa0OFTmbzR8xQHyljgxnYchchnXSvQiHP/Xg== X-Received: by 2002:a17:902:6944:b0:188:640f:f401 with SMTP id k4-20020a170902694400b00188640ff401mr9754670plt.44.1668613950911; Wed, 16 Nov 2022 07:52:30 -0800 (PST) Received: from google.com (7.104.168.34.bc.googleusercontent.com. [34.168.104.7]) by smtp.gmail.com with ESMTPSA id b14-20020a170902650e00b00177e5d83d3esm12341507plk.88.2022.11.16.07.52.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 16 Nov 2022 07:52:30 -0800 (PST) Date: Wed, 16 Nov 2022 15:52:26 +0000 From: Sean Christopherson To: "Huang, Kai" Cc: "imbrenda@linux.ibm.com" , "aou@eecs.berkeley.edu" , "mjrosato@linux.ibm.com" , "vkuznets@redhat.com" , "farman@linux.ibm.com" , "chenhuacai@kernel.org" , "paul.walmsley@sifive.com" , "palmer@dabbelt.com" , "maz@kernel.org" , "anup@brainfault.org" , "pbonzini@redhat.com" , "borntraeger@linux.ibm.com" , "aleksandar.qemu.devel@gmail.com" , "frankja@linux.ibm.com" , "oliver.upton@linux.dev" , "kvm@vger.kernel.org" , "Yao, Yuan" , "farosas@linux.ibm.com" , "david@redhat.com" , "james.morse@arm.com" , "mpe@ellerman.id.au" , "alexandru.elisei@arm.com" , "linux-s390@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "tglx@linutronix.de" , "Yamahata, Isaku" , "kvmarm@lists.linux.dev" , "suzuki.poulose@arm.com" , "kvm-riscv@lists.infradead.org" , "linuxppc-dev@lists.ozlabs.org" , "linux-arm-kernel@lists.infradead.org" , "linux-mips@vger.kernel.org" , "kvmarm@lists.cs.columbia.edu" , "Gao, Chao" , "atishp@atishpatra.org" , "linux-riscv@lists.infradead.org" Subject: Re: [PATCH 13/44] KVM: x86: Serialize vendor module initialization (hardware setup) Message-ID: References: <20221102231911.3107438-1-seanjc@google.com> <20221102231911.3107438-14-seanjc@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221116_075234_838472_F325DB18 X-CRM114-Status: GOOD ( 26.04 ) 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 Wed, 2022-11-02 at 23:18 +0000, Sean Christopherson wrote: > > Acquire a new mutex, vendor_module_lock, in kvm_x86_vendor_init() while > > doing hardware setup to ensure that concurrent calls are fully serialized. > > KVM rejects attempts to load vendor modules if a different module has > > already been loaded, but doesn't handle the case where multiple vendor > > modules are loaded at the same time, and module_init() doesn't run under > > the global module_mutex. > > > > Note, in practice, this is likely a benign bug as no platform exists that > > supports both SVM and VMX, i.e. barring a weird VM setup, one of the > > vendor modules is guaranteed to fail a support check before modifying > > common KVM state. > > > > Alternatively, KVM could perform an atomic CMPXCHG on .hardware_enable, > > but that comes with its own ugliness as it would require setting > > .hardware_enable before success is guaranteed, e.g. attempting to load > > the "wrong" could result in spurious failure to load the "right" module. > > > > Introduce a new mutex as using kvm_lock is extremely deadlock prone due > > to kvm_lock being taken under cpus_write_lock(), and in the future, under > > under cpus_read_lock(). Any operation that takes cpus_read_lock() while > > holding kvm_lock would potentially deadlock, e.g. kvm_timer_init() takes > > cpus_read_lock() to register a callback. In theory, KVM could avoid > > such problematic paths, i.e. do less setup under kvm_lock, but avoiding > > all calls to cpus_read_lock() is subtly difficult and thus fragile. E.g. > > updating static calls also acquires cpus_read_lock(). > > > > Inverting the lock ordering, i.e. always taking kvm_lock outside > > cpus_read_lock(), is not a viable option, e.g. kvm_online_cpu() takes > > kvm_lock and is called under cpus_write_lock(). > > "kvm_online_cpu() takes kvm_lock and is called under cpus_write_lock()" hasn't > happened yet. Doh, right. Thanks! > > The lockdep splat below is dependent on future patches to take > > cpus_read_lock() in hardware_enable_all(), but as above, deadlock is > > already is already possible. > > IIUC kvm_lock by design is supposed to protect vm_list, thus IMHO naturally it > doesn't fit to protect multiple vendor module loading. A different way to look at it is that kvm_lock protects anything that is global to all of KVM, and it just so happens that lists and counters of VMs are the only such resources (lumping in the usage in vm_uevent_notify_change() and the future usage to protect kvm_usage_count). > Looks above argument is good enough. I am not sure whether we need additional > justification which comes from future patches. :) To try to prevent someone from trying to eliminate the "extra" lock, like this series does for kvm_count_lock. Hopefully future someones that want to clean up the code do a git blame to understand why the lock was introduced and don't waste their time running into the same issues (or worse, don't run into the issues and break KVM). > Also, do you also want to update Documentation/virt/kvm/locking.rst" in this > patch? Hmm, yeah. That'd also be a good place to document why kvm_lock isn't used. _______________________________________________ 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 8FC66C433FE for ; Wed, 16 Nov 2022 15:53:30 +0000 (UTC) Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4NC6yx0cYWz3dtr for ; Thu, 17 Nov 2022 02:53:29 +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=Axl8RQ6K; 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=Axl8RQ6K; 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 4NC6xw3bQ0z3cGV for ; Thu, 17 Nov 2022 02:52:35 +1100 (AEDT) Received: by mail-pl1-x632.google.com with SMTP id p12so16796981plq.4 for ; Wed, 16 Nov 2022 07:52:35 -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=coErVjWyqhzO9EuKB5XSmlh7IJj5NvKWNvG7xdrbhC4=; b=Axl8RQ6KDdRrT9Z9C/Q7WDP07ILGsJIsXvmq6hEH7xYRxr1KYM7WfFS4wN+lhhZ4fX c4Jtlogr7Ros9vXyyEFv/zFAE8+4peFtT7kH9mbNBRnzYJ/2b8sYrYCafzb/t5b9hh4H MzZxnel8UMmtPU7lGv2FYN+iMfYeZiXKTt6kJLjx0bmpPltUoJ0RtRfd1Ec/nG1HLpUR NvlX1cr6oDpawENjKiQcMRdisM4zgyG2koLhhizlhvLd74nRufWrbXHhbtX0zrVof0Oh 3EiecDo5X1cIJiERj7KVmypmulB3/lI1LWoLIDeuu1zT2CC/G2udJFJRosnSOWJ19zuG AhsA== 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=coErVjWyqhzO9EuKB5XSmlh7IJj5NvKWNvG7xdrbhC4=; b=p1Lh4WjEmrM7ORzBtpURUoW58kD7w1QRmvoNiJr9ji6cGkbDFOFZqs6GzDW46RpSMH i6PhxZL2kYchUkZTbJeJeIU6jyMlg/jYJgC5a8Nq6rCPwUAxDExVdBPMFaHo3X+2GFVR ByQQ2RdakmDRGEzqO3c9tjz8zUIgCM3dydiPjkc8G+sAZcSdXvCHVRBu3DcUkHQTrYTX pM56uxx5OuobfxR1EUg9m6aFBcv03tZHHfD4JlVBDZrhinG1QrWYugGFAERjVczjfKr6 AIbfuYECSC977xam0cTfg/a+91B+68J7JYP8/6qjoCvreru6GpjJlkn7slFxNPo3c2rH i3vA== X-Gm-Message-State: ANoB5pk786l3u6/H0H4yxjQEMDNI1m6nw5+U050ldj7XEzm9czmt2Maf /uPks/lpNPs1BaJgozoFrb2p/w== X-Google-Smtp-Source: AA0mqf7dFwkFbcATwoaCt8aciY1qylOnTaB4gr/nPlKa0OFTmbzR8xQHyljgxnYchchnXSvQiHP/Xg== X-Received: by 2002:a17:902:6944:b0:188:640f:f401 with SMTP id k4-20020a170902694400b00188640ff401mr9754670plt.44.1668613950911; Wed, 16 Nov 2022 07:52:30 -0800 (PST) Received: from google.com (7.104.168.34.bc.googleusercontent.com. [34.168.104.7]) by smtp.gmail.com with ESMTPSA id b14-20020a170902650e00b00177e5d83d3esm12341507plk.88.2022.11.16.07.52.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 16 Nov 2022 07:52:30 -0800 (PST) Date: Wed, 16 Nov 2022 15:52:26 +0000 From: Sean Christopherson To: "Huang, Kai" Subject: Re: [PATCH 13/44] KVM: x86: Serialize vendor module initialization (hardware setup) Message-ID: References: <20221102231911.3107438-1-seanjc@google.com> <20221102231911.3107438-14-seanjc@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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" , "Yao, Yuan" , "linux-mips@vger.kernel.org" , "linux-riscv@lists.infradead.org" , "imbrenda@linux.ibm.com" , "kvmarm@lists.cs.columbia.edu" , "linux-s390@vger.kernel.org" , "frankja@linux.ibm.com" , "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" , "kvm@vger.kernel.org" , "paul.walmsley@sifive.com" , "kvmarm@lists.linux .dev" , "tglx@linutronix.de" , "alexandru.elisei@arm.com" , "linux-arm-kernel@lists.infradead.org" , "Yamahata, Isaku" , "atishp@atishpatra.org" , "farosas@linux.ibm.com" , "anup@brainfault.org" , "linux-kernel@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 Wed, 2022-11-02 at 23:18 +0000, Sean Christopherson wrote: > > Acquire a new mutex, vendor_module_lock, in kvm_x86_vendor_init() while > > doing hardware setup to ensure that concurrent calls are fully serialized. > > KVM rejects attempts to load vendor modules if a different module has > > already been loaded, but doesn't handle the case where multiple vendor > > modules are loaded at the same time, and module_init() doesn't run under > > the global module_mutex. > > > > Note, in practice, this is likely a benign bug as no platform exists that > > supports both SVM and VMX, i.e. barring a weird VM setup, one of the > > vendor modules is guaranteed to fail a support check before modifying > > common KVM state. > > > > Alternatively, KVM could perform an atomic CMPXCHG on .hardware_enable, > > but that comes with its own ugliness as it would require setting > > .hardware_enable before success is guaranteed, e.g. attempting to load > > the "wrong" could result in spurious failure to load the "right" module. > > > > Introduce a new mutex as using kvm_lock is extremely deadlock prone due > > to kvm_lock being taken under cpus_write_lock(), and in the future, under > > under cpus_read_lock(). Any operation that takes cpus_read_lock() while > > holding kvm_lock would potentially deadlock, e.g. kvm_timer_init() takes > > cpus_read_lock() to register a callback. In theory, KVM could avoid > > such problematic paths, i.e. do less setup under kvm_lock, but avoiding > > all calls to cpus_read_lock() is subtly difficult and thus fragile. E.g. > > updating static calls also acquires cpus_read_lock(). > > > > Inverting the lock ordering, i.e. always taking kvm_lock outside > > cpus_read_lock(), is not a viable option, e.g. kvm_online_cpu() takes > > kvm_lock and is called under cpus_write_lock(). > > "kvm_online_cpu() takes kvm_lock and is called under cpus_write_lock()" hasn't > happened yet. Doh, right. Thanks! > > The lockdep splat below is dependent on future patches to take > > cpus_read_lock() in hardware_enable_all(), but as above, deadlock is > > already is already possible. > > IIUC kvm_lock by design is supposed to protect vm_list, thus IMHO naturally it > doesn't fit to protect multiple vendor module loading. A different way to look at it is that kvm_lock protects anything that is global to all of KVM, and it just so happens that lists and counters of VMs are the only such resources (lumping in the usage in vm_uevent_notify_change() and the future usage to protect kvm_usage_count). > Looks above argument is good enough. I am not sure whether we need additional > justification which comes from future patches. :) To try to prevent someone from trying to eliminate the "extra" lock, like this series does for kvm_count_lock. Hopefully future someones that want to clean up the code do a git blame to understand why the lock was introduced and don't waste their time running into the same issues (or worse, don't run into the issues and break KVM). > Also, do you also want to update Documentation/virt/kvm/locking.rst" in this > patch? Hmm, yeah. That'd also be a good place to document why kvm_lock isn't used. 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 3E9EDC4332F for ; Wed, 16 Nov 2022 15:54:08 +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=Jb/EjrHWSfdJF43yrv5Z2BP3TfG8KYdo4Asp/P3R1pY=; b=yiMNFTm1QsI17J XFDyQq591VNyoH/agncY0/QZdtakHYkrJJ1kIcoLhqlum0XZx7JjFS0sg4OKSbK6CrmGYbg/Fk9cj PIxpp64UMzs3Nvvz9ZjxF62TfszVHpYaCy+dpH3Gv0qF/Iu/NT1TA94ckN01kOhMgd/wz21Iv0AY5 uSV8CE5sQ3zZWD9TPhDzNQ70yiq9ZdW9AYwb3wubl9u0ID+WK0b9LRk+1K9mchDhI+mFckIbahdLB OalGDNjW656GJszqLXcOBczzBmRyf6Nk928rDx5X+ok+d0Aos7TcTCV+cqXPM6eR3a8vrAMYn7hGG aYUOEXLHd7nDzKoJbSiw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ovKiL-005be3-PK; Wed, 16 Nov 2022 15:52:37 +0000 Received: from mail-pj1-x1029.google.com ([2607:f8b0:4864:20::1029]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1ovKiI-005bcE-OP for linux-arm-kernel@lists.infradead.org; Wed, 16 Nov 2022 15:52:36 +0000 Received: by mail-pj1-x1029.google.com with SMTP id v4-20020a17090a088400b00212cb0ed97eso2768720pjc.5 for ; Wed, 16 Nov 2022 07:52:31 -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=coErVjWyqhzO9EuKB5XSmlh7IJj5NvKWNvG7xdrbhC4=; b=Axl8RQ6KDdRrT9Z9C/Q7WDP07ILGsJIsXvmq6hEH7xYRxr1KYM7WfFS4wN+lhhZ4fX c4Jtlogr7Ros9vXyyEFv/zFAE8+4peFtT7kH9mbNBRnzYJ/2b8sYrYCafzb/t5b9hh4H MzZxnel8UMmtPU7lGv2FYN+iMfYeZiXKTt6kJLjx0bmpPltUoJ0RtRfd1Ec/nG1HLpUR NvlX1cr6oDpawENjKiQcMRdisM4zgyG2koLhhizlhvLd74nRufWrbXHhbtX0zrVof0Oh 3EiecDo5X1cIJiERj7KVmypmulB3/lI1LWoLIDeuu1zT2CC/G2udJFJRosnSOWJ19zuG AhsA== 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=coErVjWyqhzO9EuKB5XSmlh7IJj5NvKWNvG7xdrbhC4=; b=b6aBa1SviUiZyo8Gemzux4GLfCjlZiyWa3ek0ob/LEvVzPzUaCyB+RbGn68rWukKpb VVpWWsqPHZY3ef+g0R7b+aOuAQxvUM3nn8M7uAkCLRDHugBf71zrmXwoIgg6QmaPbsd+ YgT2pnRClZn5trmpSFQq6Lub8yGTUiMV2lyTg+jVUyyrvCVSWlKdxLec8guF8OhuOPyO 67tS5TIZMgr8U4pqu39LYWNjS28MR60K1PYylsRbJ/XatP9lg2C33ruA9kIPwUPQBTKg 176f+u+3VVHNEPko/zakRc3GMjnJd2NFMaGYnfr97Vuawrq3nT+AAgF+V2/6aFkAfyef e+1w== X-Gm-Message-State: ANoB5pm7MbW/ARCS6IzUMIUlsgCF25BoUxPGT8rZMJQZNbZMgjqrbnPe lcqUbQvi5VVN/OxwLlS0miDnIg== X-Google-Smtp-Source: AA0mqf7dFwkFbcATwoaCt8aciY1qylOnTaB4gr/nPlKa0OFTmbzR8xQHyljgxnYchchnXSvQiHP/Xg== X-Received: by 2002:a17:902:6944:b0:188:640f:f401 with SMTP id k4-20020a170902694400b00188640ff401mr9754670plt.44.1668613950911; Wed, 16 Nov 2022 07:52:30 -0800 (PST) Received: from google.com (7.104.168.34.bc.googleusercontent.com. [34.168.104.7]) by smtp.gmail.com with ESMTPSA id b14-20020a170902650e00b00177e5d83d3esm12341507plk.88.2022.11.16.07.52.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 16 Nov 2022 07:52:30 -0800 (PST) Date: Wed, 16 Nov 2022 15:52:26 +0000 From: Sean Christopherson To: "Huang, Kai" Cc: "imbrenda@linux.ibm.com" , "aou@eecs.berkeley.edu" , "mjrosato@linux.ibm.com" , "vkuznets@redhat.com" , "farman@linux.ibm.com" , "chenhuacai@kernel.org" , "paul.walmsley@sifive.com" , "palmer@dabbelt.com" , "maz@kernel.org" , "anup@brainfault.org" , "pbonzini@redhat.com" , "borntraeger@linux.ibm.com" , "aleksandar.qemu.devel@gmail.com" , "frankja@linux.ibm.com" , "oliver.upton@linux.dev" , "kvm@vger.kernel.org" , "Yao, Yuan" , "farosas@linux.ibm.com" , "david@redhat.com" , "james.morse@arm.com" , "mpe@ellerman.id.au" , "alexandru.elisei@arm.com" , "linux-s390@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "tglx@linutronix.de" , "Yamahata, Isaku" , "kvmarm@lists.linux.dev" , "suzuki.poulose@arm.com" , "kvm-riscv@lists.infradead.org" , "linuxppc-dev@lists.ozlabs.org" , "linux-arm-kernel@lists.infradead.org" , "linux-mips@vger.kernel.org" , "kvmarm@lists.cs.columbia.edu" , "Gao, Chao" , "atishp@atishpatra.org" , "linux-riscv@lists.infradead.org" Subject: Re: [PATCH 13/44] KVM: x86: Serialize vendor module initialization (hardware setup) Message-ID: References: <20221102231911.3107438-1-seanjc@google.com> <20221102231911.3107438-14-seanjc@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221116_075234_838244_79E8450D X-CRM114-Status: GOOD ( 27.45 ) 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 Wed, 2022-11-02 at 23:18 +0000, Sean Christopherson wrote: > > Acquire a new mutex, vendor_module_lock, in kvm_x86_vendor_init() while > > doing hardware setup to ensure that concurrent calls are fully serialized. > > KVM rejects attempts to load vendor modules if a different module has > > already been loaded, but doesn't handle the case where multiple vendor > > modules are loaded at the same time, and module_init() doesn't run under > > the global module_mutex. > > > > Note, in practice, this is likely a benign bug as no platform exists that > > supports both SVM and VMX, i.e. barring a weird VM setup, one of the > > vendor modules is guaranteed to fail a support check before modifying > > common KVM state. > > > > Alternatively, KVM could perform an atomic CMPXCHG on .hardware_enable, > > but that comes with its own ugliness as it would require setting > > .hardware_enable before success is guaranteed, e.g. attempting to load > > the "wrong" could result in spurious failure to load the "right" module. > > > > Introduce a new mutex as using kvm_lock is extremely deadlock prone due > > to kvm_lock being taken under cpus_write_lock(), and in the future, under > > under cpus_read_lock(). Any operation that takes cpus_read_lock() while > > holding kvm_lock would potentially deadlock, e.g. kvm_timer_init() takes > > cpus_read_lock() to register a callback. In theory, KVM could avoid > > such problematic paths, i.e. do less setup under kvm_lock, but avoiding > > all calls to cpus_read_lock() is subtly difficult and thus fragile. E.g. > > updating static calls also acquires cpus_read_lock(). > > > > Inverting the lock ordering, i.e. always taking kvm_lock outside > > cpus_read_lock(), is not a viable option, e.g. kvm_online_cpu() takes > > kvm_lock and is called under cpus_write_lock(). > > "kvm_online_cpu() takes kvm_lock and is called under cpus_write_lock()" hasn't > happened yet. Doh, right. Thanks! > > The lockdep splat below is dependent on future patches to take > > cpus_read_lock() in hardware_enable_all(), but as above, deadlock is > > already is already possible. > > IIUC kvm_lock by design is supposed to protect vm_list, thus IMHO naturally it > doesn't fit to protect multiple vendor module loading. A different way to look at it is that kvm_lock protects anything that is global to all of KVM, and it just so happens that lists and counters of VMs are the only such resources (lumping in the usage in vm_uevent_notify_change() and the future usage to protect kvm_usage_count). > Looks above argument is good enough. I am not sure whether we need additional > justification which comes from future patches. :) To try to prevent someone from trying to eliminate the "extra" lock, like this series does for kvm_count_lock. Hopefully future someones that want to clean up the code do a git blame to understand why the lock was introduced and don't waste their time running into the same issues (or worse, don't run into the issues and break KVM). > Also, do you also want to update Documentation/virt/kvm/locking.rst" in this > patch? Hmm, yeah. That'd also be a good place to document why kvm_lock isn't used. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel