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 14:54:05 +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 16/26] KVM: kvm_arch.c: Remove a global variable,
 hardware_enable_failed
Message-ID: <YxrjDdn5xCxrJYUT@gao-cwp>
References: <cover.1662679124.git.isaku.yamahata@intel.com>
 <60ca73d7d931ec8046ac50b20f05723a97ef643b.1662679124.git.isaku.yamahata@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <60ca73d7d931ec8046ac50b20f05723a97ef643b.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?OEtpOXcOBKWpoqnNc4T5yLU4EOlGyyJIUVhfWGTuS1yMi+XViFdYDR++g2eG?=
 =?us-ascii?Q?cjDd27WXgpfFFP93bhD5J615Wt9tfZS/1LyAthzcj+cOrnFv3BA7q+WvEbKZ?=
 =?us-ascii?Q?oaRTKFfWuHID/C3UZRpYiYiFqZ6qcgcqobwRqd1G3EZAgkiFpuwUSChFzze7?=
 =?us-ascii?Q?VcsoaOA95QDwhhXApBCtBtt1JkCB/NgqjGqXnvDQkGsy25qaR+I0oydX41U/?=
 =?us-ascii?Q?9qcleGW+BdeyZDWbG1Jv2blV0Y1ITNmGQm/ZyAXUURLVeEFySD/3XYMCeA19?=
 =?us-ascii?Q?EqgLcEwm1rnebdrZUu5aeKVc4S24EnDFAq6kKtF/AGiDM3VF/bGjDi3EIEO7?=
 =?us-ascii?Q?T22cJWfniQFMkx6U24SeFCopa5aq/MTAW9RHAhoTcdJpU/r5rrvT38QwdBkU?=
 =?us-ascii?Q?piaiYisM0JKdcDC/3km0Klt3Shpf3QT83byvVAzdR42jjq8y1SOG+VvVMXXK?=
 =?us-ascii?Q?/UT83Aht665aT0wHrtAICq3VswnNkveGcy1JxeVNcaMdI3akcZZNOv0CFOmH?=
 =?us-ascii?Q?DXCdf/kd8k5OXzWiYQTZwxOEM2zg0UtJrDWDhV2sgczCprLUEKbbJCfN+NUL?=
 =?us-ascii?Q?nzRYzDWRy8GJM6cS84p2/4c2lUmZJcw03rmE6K984JpPj5bQuCxtOHZhspVe?=
 =?us-ascii?Q?48iPUm/JUZZYw4DQRcVfEKu1Da3LVtGzqVxir2NX8VtP19FqNUQM1g/t+EOf?=
 =?us-ascii?Q?CLuF3u3/ySPUsVJb8NdRE+lC1hzTKpSZ9wK/gN5avh7yR1i3L7KB2O1i3j8N?=
 =?us-ascii?Q?fyRu5cLaOWSffSX2qtefFR9FEe8iapm3piYeDAW7K7t+oDQykdDw1idRKd0w?=
 =?us-ascii?Q?a6kbRTnYAqf469pQZfQ3v/ry1yWho+mmuSQVxlEQqUjT7yYxZVbCDjhxMydX?=
 =?us-ascii?Q?GKiZQ03/l7Jlx0/2eKK+9mRdusffYUeUMuwWdQEvDFYkj0CZOWLfehh7pfq/?=
 =?us-ascii?Q?Ax9MopEXhLw7GsDTCQLC0O1Y02yMAAN6lJCfRwBQqGEd0GGQsA9FX38Yhti2?=
 =?us-ascii?Q?cTtbR/6XpKcZx1AKb62wIjU8JZP8MOYMsGiEGXNP+ZIQvUBq2XvyEaEK1iUd?=
 =?us-ascii?Q?5vIM52mrhZfh+r9hZ+CUyTwGb6Z0ilWHJTjTYxsamRQx4OYK9243GQai7KO/?=
 =?us-ascii?Q?EC/0+grY0UEj5/sgp7IlTaPpxiZOnavOOtnXdFtENdqGegtUSdHKeMi/PI4m?=
 =?us-ascii?Q?0XAcCe3gTWIbL4AmgDMPp027BPPPrQf3SdK6J8ZlteGrJ+Qwk9ax0ViEUMFs?=
 =?us-ascii?Q?tx6li/sekMJpzDvQODz/fZK8be6dwPBHoAwl/cRdFR0+IfdDun8GQTtjq8+5?=
 =?us-ascii?Q?+F3GyxPhCAxf8er+z4e2AuEVatvQDzprExfblhQR+Vfam99T7i/SkIaP1Qze?=
 =?us-ascii?Q?Ui8qTEjaavteuUvocgt4AER5hm+69p04KPIfKFnKKgJYw1Jc/97axhlJKpWI?=
 =?us-ascii?Q?uD2A6HZNFLS1mwTXeYELJ/i69QI+LLK1reETZnHvUzIwWXFTEjTLjIrrgfSS?=
 =?us-ascii?Q?jWgRfqzi18dlHtvwxRBIgL/lAbSC2K0B2scXEmdEp2j/gTs8PXt3jixKD4jE?=
 =?us-ascii?Q?1uAAEn7Gxzj0rhkr23km6dw7VCW1S2GzpDLiRPaw?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 443a93c2-6b03-4943-13b0-08da92301931
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1101MB2221.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Sep 2022 06:54:10.6328
 (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: SwgyLMM9yetI0l84GIKldNT0x6wfRu+wYlKjH4MwNATLzSY4Wvt7RJFjiB9Lc9t0Wavq/rDA4oV6o7vj6s/zng==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR11MB6362
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:32PM -0700, isaku.yamahata@intel.com wrote:
>From: Isaku Yamahata <isaku.yamahata@intel.com>
>
>A global variable hardware_enable_failed in kvm_arch.c is used only by
>kvm_arch_add_vm() and hardware_enable().  It doesn't have to be a global
>variable.  Make it function local.
>
>Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>

Reviewed-by: Chao Gao <chao.gao@intel.com>

>---
> virt/kvm/kvm_arch.c | 62 ++++++++++++++++++++++-----------------------
> 1 file changed, 31 insertions(+), 31 deletions(-)
>
>diff --git a/virt/kvm/kvm_arch.c b/virt/kvm/kvm_arch.c
>index 4fe16e8ef2e5..ad23537ebe3b 100644
>--- a/virt/kvm/kvm_arch.c
>+++ b/virt/kvm/kvm_arch.c
>@@ -13,14 +13,13 @@
> #include <linux/kvm_host.h>
> 
> static cpumask_t cpus_hardware_enabled = CPU_MASK_NONE;
>-static atomic_t hardware_enable_failed;
> 
> __weak int kvm_arch_post_init_vm(struct kvm *kvm)
> {
> 	return 0;
> }
> 
>-static void hardware_enable(void *caller_name)
>+static int __hardware_enable(void *caller_name)

nit: caller_name can be dropped and use __builtin_return_address(0) instead.

> {
> 	int cpu = raw_smp_processor_id();
> 	int r;
>@@ -28,18 +27,22 @@ static void hardware_enable(void *caller_name)
> 	WARN_ON_ONCE(preemptible());
> 
> 	if (cpumask_test_cpu(cpu, &cpus_hardware_enabled))
>-		return;
>-
>-	cpumask_set_cpu(cpu, &cpus_hardware_enabled);
>-
>+		return 0;
> 	r = kvm_arch_hardware_enable();
>-
>-	if (r) {
>-		cpumask_clear_cpu(cpu, &cpus_hardware_enabled);
>-		atomic_inc(&hardware_enable_failed);
>+	if (r)
> 		pr_warn("kvm: enabling virtualization on CPU%d failed during %s()\n",
> 			cpu, (const char *)caller_name);
>-	}
>+	else
>+		cpumask_set_cpu(cpu, &cpus_hardware_enabled);
>+	return r;
>+}
>+
>+static void hardware_enable(void *arg)
>+{
>+	atomic_t *failed = arg;
>+
>+	if (__hardware_enable((void *)__func__))
>+		atomic_inc(failed);
> }
> 
> static void hardware_disable(void *junk)
>@@ -65,15 +68,16 @@ __weak void kvm_arch_pre_hardware_unsetup(void)
>  */
> __weak int kvm_arch_add_vm(struct kvm *kvm, int usage_count)
> {
>+	atomic_t failed;

nit:
	atomic_t failed = ATOMIC_INIT(0);

then you can drop the atomic_set() below.

> 	int r = 0;
> 
> 	if (usage_count != 1)
> 		return 0;
> 
>-	atomic_set(&hardware_enable_failed, 0);
>-	on_each_cpu(hardware_enable, (void *)__func__, 1);
>+	atomic_set(&failed, 0);
>+	on_each_cpu(hardware_enable, &failed, 1);
> 
>-	if (atomic_read(&hardware_enable_failed)) {
>+	if (atomic_read(&failed)) {
> 		r = -EBUSY;
> 		goto err;
> 	}
>@@ -96,33 +100,29 @@ __weak int kvm_arch_del_vm(int usage_count)
> 
> __weak int kvm_arch_online_cpu(unsigned int cpu, int usage_count)
> {
>-	int ret = 0;
>+	int ret;
> 
> 	ret = kvm_arch_check_processor_compat();
> 	if (ret)
> 		return ret;
> 
>+	if (!usage_count)
>+		return 0;
>+
>+	/*
>+	 * arch callback kvm_arch_hardware_eanble() assumes that

						^ enable