Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 9 Sep 2022 11:05:34 +0800
From:   Chao Gao <chao.gao@intel.com>
To:     <isaku.yamahata@intel.com>
CC:     <linux-kernel@vger.kernel.org>, <kvm@vger.kernel.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Sean Christopherson <seanjc@google.com>,
        Marc Zyngier <maz@kernel.org>, Will Deacon <will@kernel.org>,
        Yuan Yao <yuan.yao@linux.intel.com>,
        <isaku.yamahata@gmail.com>, Kai Huang <kai.huang@intel.com>,
        Atish Patra <atishp@atishpatra.org>,
        Shaokun Zhang <zhangshaokun@hisilicon.com>,
        Qi Liu <liuqi115@huawei.com>,
        John Garry <john.garry@huawei.com>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        "Huang Ying" <ying.huang@intel.com>,
        Huacai Chen <chenhuacai@kernel.org>
Subject: Re: [PATCH v4 10/26] KVM: Drop kvm_count_lock and instead protect
 kvm_usage_count with kvm_lock
Message-ID: <YxqtfmhBGQlkhTvU@gao-cwp>
References: <cover.1662679124.git.isaku.yamahata@intel.com>
 <c95e36034d7ffd67b7afed3ba790de921426e737.1662679124.git.isaku.yamahata@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <c95e36034d7ffd67b7afed3ba790de921426e737.1662679124.git.isaku.yamahata@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?FFM+/ssT3KBzo0J47pTYutX0zXovW0VrJZoyAugv/6uJYT1M6IJasbikAPJ8?=
 =?us-ascii?Q?8JxrMFjAjasQDyzYSVOE9XbURYUHm4XHBjhSCzkLk2uozy2TLRgRAa8hwV47?=
 =?us-ascii?Q?rjlxLx2E6cGs1G0WKZnPdwe/oWXcq0yvvUlRhrMCmt77wOGNPg2Ui/rfrNVb?=
 =?us-ascii?Q?Xaue1T0No7bHDsmHGhT3jTTBbfMpf+4nbNYC1k5fyO2JFgHuaeUQ1WXVf07E?=
 =?us-ascii?Q?ZufwWeBCJzDZqA9vDbGYTYs8WDW65trPod8TgqXoF39Ndc5XZ7PP97Xx5Nrx?=
 =?us-ascii?Q?8plQZHEe2SCgU8BqNOliX0cFmb7Gf/wpUjk95wdHlcrQlIFCswuOOw6WPaKV?=
 =?us-ascii?Q?KwaOzycIrI1vRbDJ8EbhanXwyGRVna6mmKO9g+BbqyCCV0USLhZHpTW3CDj5?=
 =?us-ascii?Q?LdXGjBHCbUAGbTbjzvt111qwPUswthAfqMydD+gk1lGJgLICup+p2cD0+lOz?=
 =?us-ascii?Q?opdftx38QNfj/I0mcX5EWDI/MbBZoHG18lS+yBK31cUuVN/+xsz+EwJ/3d/C?=
 =?us-ascii?Q?8k+6pxSbB6KRvUXXU57wL2y1aPyTdIs54Xxgeb/k3qUUymffTZAB+Zxk6RHl?=
 =?us-ascii?Q?eXhQS73YESP1aplhpObtjl38d7XqehNEIBn3CzEoGqLznvNF+hxPN/7uH3Fn?=
 =?us-ascii?Q?btEewnR4bUYOV9J1kuQVrUyuAbol7FFNDXpE7oFtzkCo4onMwOj4Ulc/gxp8?=
 =?us-ascii?Q?1pnBvfcYIM9EP0+hKhlIIJHDPQHOsLYowBWIfAi565S56j8RfknxhsOpqC4p?=
 =?us-ascii?Q?nUuzVANARnQA4F5WHVQUrv2NQH8df4umT0pcmVQu+cE60tnPTZ4ydHqVtu2f?=
 =?us-ascii?Q?EUWYh1ZlodYW/hDvoaY/PkNV/amj6VABNp5nc2leRLC+OJAZYO5Pnw6SAVbI?=
 =?us-ascii?Q?XAtuRh0BbgAOi3a7yFhYqa6+aF111TGyUde/WR9QMR8skXtJ13Ho1tiGVj0G?=
 =?us-ascii?Q?qoX90SnqFGkXaWOY5EBhxvALW8aKTKZU/Z7i8jklO+UZJpwLBRYpQPgWfR8K?=
 =?us-ascii?Q?N4QEs/LpWFVy8VdRoH0LVkxjZSEdDETFuyJisBRYrB81Ru2QMnUarIHIitO5?=
 =?us-ascii?Q?8qNCqhwRCMbFixZVQH3/3lSNEvDeMiAl6dZ+gU3DtriIqzAoBTpQ0no4ldVF?=
 =?us-ascii?Q?O//kidOiKLj9XMKo///SGhamTDbtuUqbXVzUyNLMhNH/hAtbRVPqiUJTx3XA?=
 =?us-ascii?Q?i6TFJWJjFKBDChifJlBoQaumVnSZlJK+ESb/tYQQog6LhFlKyXeltUuJet8r?=
 =?us-ascii?Q?3MYCcY62UkBZscubpPNqYB/TmvmpbODvQtANhohfgXFXDehhwNzrrAoMxJGD?=
 =?us-ascii?Q?HjwF3QdQjtpv+VcNLT0YqC2sz52m05aJqGlyZS6T+3o5ncSvn5NQM27mF+7N?=
 =?us-ascii?Q?JuWFYC8KQi3VcvQlLob4kGtaDqn/9AULZyGKTHGtmiTrztue09iHHza6L1p2?=
 =?us-ascii?Q?J6kPdvLp7sacW5GlMCi3pNxtZa1O7N0d6tqjI/YFTg389Qsf7jdB1ypPNPFZ?=
 =?us-ascii?Q?ZCqM/MA3Br+fFIkBD6FVksS9GWSlS/g77olsXSUBnMcBNbFpbOn/rs99LJPO?=
 =?us-ascii?Q?OTpHpUbM5gDyhSqYDGp9RStjhdsc7LM/IIkZ62Wq?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a9a6a509-0abe-4b00-f77b-08da92102c80
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1101MB2221.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Sep 2022 03:05:39.1377
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: yvVRRysDhMslkXnA0F1FmM620Gy3Y0IRsw4w7XhnmEjb9Pi1yoXzj3aXwCGSx5ff80hu78cacWT/1r7rA7b2Xg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB6217
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Sep 08, 2022 at 04:25:26PM -0700, isaku.yamahata@intel.com wrote:
>-
>-``kvm_count_lock``
>-^^^^^^^^^^^^^^^^^^
>-
>-:Type:		raw_spinlock_t
>-:Arch:		any
>-:Protects:	- hardware virtualization enable/disable
>-:Comment:	'raw' because hardware enabling/disabling must be atomic /wrt
>-		migration.
>+                - kvm_usage_count
>+                - hardware virtualization enable/disable
>+:Comment:	Use cpus_read_lock() for hardware virtualization enable/disable
>+                because hardware enabling/disabling must be atomic /wrt
>+                migration.  The lock order is cpus lock => kvm_lock.

Probably "/wrt CPU hotplug" is better.

> 
>@@ -5708,8 +5728,18 @@ static void kvm_init_debug(void)
> 
> static int kvm_suspend(void)
> {
>-	if (kvm_usage_count)
>+	/*
>+	 * The caller ensures that CPU hotlug is disabled by

					^hotplug

>+	 * cpu_hotplug_disable() and other CPUs are offlined.  No need for
>+	 * locking.
>+	 */
>+	lockdep_assert_not_held(&kvm_lock);
>+
>+	if (kvm_usage_count) {
>+		preempt_disable();
> 		hardware_disable_nolock(NULL);
>+		preempt_enable();

kvm_suspend() is called with interrupt disabled. So, no need to disable
preemption.

/**
 * syscore_suspend - Execute all the registered system core suspend callbacks.
 *
 * This function is executed with one CPU on-line and disabled interrupts.
 */
int syscore_suspend(void)


>+	}
> 	return 0;
> }
> 
>@@ -5723,8 +5753,10 @@ static void kvm_resume(void)
> 		return; /* FIXME: disable KVM */
> 
> 	if (kvm_usage_count) {
>-		lockdep_assert_not_held(&kvm_count_lock);
>+		lockdep_assert_not_held(&kvm_lock);
>+		preempt_disable();
> 		hardware_enable_nolock((void *)__func__);
>+		preempt_enable();

ditto.