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 12:34:39 +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 11/26] KVM: Add arch hooks for PM events with empty
 stub
Message-ID: <YxrCX2nhOyWbbGGY@gao-cwp>
References: <cover.1662679124.git.isaku.yamahata@intel.com>
 <fa7ecd7305d011940121466f094a544c6de39ea3.1662679124.git.isaku.yamahata@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <fa7ecd7305d011940121466f094a544c6de39ea3.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?RXEXGqghSObpro3DSBGSjRv8bJqeQjZ5wnQzIfPtMqtjIhr9L1lqZikVsG2B?=
 =?us-ascii?Q?odpA3HgP0FznSZL6r8p93qRx0b7gPYr4vge7V1F5Jb9Ymw8NYCmQvYDvV7bs?=
 =?us-ascii?Q?EBOGw/fVjhz2CydExcrLgQfwMEO6Gp/t0Xox7u0n5fOdEpJrgnoRD2XUXIw1?=
 =?us-ascii?Q?BELJ36v/gOlOoWTj0FscqL4A/tIQIppDuDgidC1zO6LvelAZyn38XQz5qyq5?=
 =?us-ascii?Q?osDFA0aCWFLScmZPeOsGbPdeiYib33SAhrVz8QhlP11cuzUG+9wl/BUrsvsr?=
 =?us-ascii?Q?8H2CdpiX0gDEMxa7A5nnA0lQ8NQzjbl/WJ/+1Kl9fS2ezQZyFPQGAzkCX4Nx?=
 =?us-ascii?Q?LloGkLJ2pdKETMDCzYT5BjEJ6dwhefv6z4Y/I8mJ6XzxKqs0Rv3k3ElNRSK/?=
 =?us-ascii?Q?vjuBG2SZSaJkhsKDpDTrx7CVNwYUfdVjxTqBWUUdu86Xf5WoFCOmdUPVqnsD?=
 =?us-ascii?Q?DYkqyN/AKL6DZDIt7e4IwqpID8z2iKuJaioPkGePKR9CZhSJtsN8NhbI8mnq?=
 =?us-ascii?Q?wChUaiTSrCLZX+qNBCY8hcetQFSUEdKRhlnfZCdff6Nmjsf4JxoVaCFwAN3U?=
 =?us-ascii?Q?KjsenLLtisr9tLXJPFVIAOWiV0T6CTAHy6NmBcHCPrbOSdDboKn4uV/lA/3b?=
 =?us-ascii?Q?hNK3hISW8b/CKI32sKUPFuPqkZ/WTYe4g+LzEDUBus6yJVwvKJTkf3F5xecP?=
 =?us-ascii?Q?wasLAGhrVfYXHgpLPvct/llP1o0K1t5ZKsxLvbhcz/5Yr9uM6FZraSNWFbGw?=
 =?us-ascii?Q?lON5zRSJ6Nrowd0hYdk2W053HqYsaJ94JrU+gsk6AkopPsjkU88Y9Kb7FCZc?=
 =?us-ascii?Q?AV9ty0GjFsHyOO3dWPleefpY2/UkyENM4an7vHLo6HFuymb0pJPztrEybSh1?=
 =?us-ascii?Q?ZOahXKapw3+3uDEi1GHFssbNnwAOZtudAiufx0M2Ltrc/mI3BnE8/Llgm4fz?=
 =?us-ascii?Q?3J/jwwa5MaINUWMaiHzf2pa40pa3pz2yPTkQU1QXvZl65XZb4S3nj40MW16a?=
 =?us-ascii?Q?LtU/BYpF2naRZYjr3QhatiyUXWiacYcDNFTv1HZSKlfmoPpJlizobD7IzlpN?=
 =?us-ascii?Q?r+i1GFS6XwqJSjsZmircLMuSaTiH9dp83B+gW2ZyKF1Qf2LkdQbSKwG5RCgb?=
 =?us-ascii?Q?f91Rsahp/p4QpIk3e15rUDDZ8WypLMLByvBzNYG4wExW44GPuuEOl6oNLUaA?=
 =?us-ascii?Q?eMHCEDVGUDOgICF3w5m2p5sA4ITgbAGI+wYXUi01dAUxAmZi22AeN3seGkHI?=
 =?us-ascii?Q?WTqbv8Lk0yWT4og6rc1vpdMojqo3cGd+ODE53MGOqPNLxFwrmd15gG9xZkTX?=
 =?us-ascii?Q?ITQWQ4jeqolaRw1GkzI8C0N0gg9IITduZHk4TZGvKBCYuHWw+3uGZq/58UIf?=
 =?us-ascii?Q?PiVkZfdpAN7q9iYkiugQ2ROHr2SBxRomPHLD10a2MwiShm79M+XYmtxcZroc?=
 =?us-ascii?Q?tfSoV8q2s1K4FzVuFZEViTtvwaBYPxUDXo6hJQyaLWLgu4KlliOuohfh2zCt?=
 =?us-ascii?Q?M6sNeiQ4g7J0sq/ypus4+YM3AQ0pe/f2oKHh9fH5vATvYDg7Dbgg+AFEfkrN?=
 =?us-ascii?Q?GvxXnLrWuwPTj9eEA6kZpVPKnD5sOM6ub51ML671?=
X-MS-Exchange-CrossTenant-Network-Message-Id: d9eeb790-4ce9-4b6b-afdb-08da921c9f37
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1101MB2221.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Sep 2022 04:34:45.6031
 (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: JNdmYRukJLndz/qcZL8ziPk3oMITSWrHPXEfoDfp3hSzzsHeDMz7ul4q3jncmU91TJ/Fd5KsRpmHpPpakx5v/g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB7275
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:27PM -0700, isaku.yamahata@intel.com wrote:
>From: Isaku Yamahata <isaku.yamahata@intel.com>
>
>Add arch hooks for reboot, suspend, resume, and CPU-online/offline events
>with empty stub functions.
>
>Suggested-by: Sean Christopherson <seanjc@google.com>
>Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
>---
> include/linux/kvm_host.h |  6 +++++
> virt/kvm/Makefile.kvm    |  2 +-
> virt/kvm/kvm_arch.c      | 44 ++++++++++++++++++++++++++++++
> virt/kvm/kvm_main.c      | 58 +++++++++++++++++++++++++---------------
> 4 files changed, 88 insertions(+), 22 deletions(-)
> create mode 100644 virt/kvm/kvm_arch.c
>
>diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>index eab352902de7..dd2a6d98d4de 100644
>--- a/include/linux/kvm_host.h
>+++ b/include/linux/kvm_host.h
>@@ -1448,6 +1448,12 @@ int kvm_arch_post_init_vm(struct kvm *kvm);
> void kvm_arch_pre_destroy_vm(struct kvm *kvm);
> int kvm_arch_create_vm_debugfs(struct kvm *kvm);
> 
>+int kvm_arch_suspend(int usage_count);
>+void kvm_arch_resume(int usage_count);
>+int kvm_arch_reboot(int val);
>+int kvm_arch_online_cpu(unsigned int cpu, int usage_count);
>+int kvm_arch_offline_cpu(unsigned int cpu, int usage_count);

Why not extract each of them with one separate patch?

>+
> #ifndef __KVM_HAVE_ARCH_VM_ALLOC
> /*
>  * All architectures that want to use vzalloc currently also
>diff --git a/virt/kvm/Makefile.kvm b/virt/kvm/Makefile.kvm
>index 2c27d5d0c367..c4210acabd35 100644
>--- a/virt/kvm/Makefile.kvm
>+++ b/virt/kvm/Makefile.kvm
>@@ -5,7 +5,7 @@
> 
> KVM ?= ../../../virt/kvm
> 
>-kvm-y := $(KVM)/kvm_main.o $(KVM)/eventfd.o $(KVM)/binary_stats.o
>+kvm-y := $(KVM)/kvm_main.o $(KVM)/kvm_arch.o $(KVM)/eventfd.o $(KVM)/binary_stats.o
> kvm-$(CONFIG_KVM_VFIO) += $(KVM)/vfio.o
> kvm-$(CONFIG_KVM_MMIO) += $(KVM)/coalesced_mmio.o
> kvm-$(CONFIG_KVM_ASYNC_PF) += $(KVM)/async_pf.o
>diff --git a/virt/kvm/kvm_arch.c b/virt/kvm/kvm_arch.c
>new file mode 100644
>index 000000000000..4748a76bcb03
>--- /dev/null
>+++ b/virt/kvm/kvm_arch.c
>@@ -0,0 +1,44 @@
>+// SPDX-License-Identifier: GPL-2.0-only
>+/*
>+ * kvm_arch.c: kvm default arch hooks for hardware enabling/disabling
>+ * Copyright (c) 2022 Intel Corporation.
>+ *
>+ * Author:
>+ *   Isaku Yamahata <isaku.yamahata@intel.com>
>+ *                  <isaku.yamahata@gmail.com>
>+ */
>+
>+#include <linux/kvm_host.h>
>+
>+/*
>+ * Called after the VM is otherwise initialized, but just before adding it to
>+ * the vm_list.
>+ */
>+__weak int kvm_arch_post_init_vm(struct kvm *kvm)
>+{
>+	return 0;
>+}

use "int __weak" to comply with kernel's convension.

> 
> static int kvm_offline_cpu(unsigned int cpu)
> {
>+	int ret = 0;
>+
> 	mutex_lock(&kvm_lock);
> 	if (kvm_usage_count) {
> 		/*
>@@ -5069,10 +5067,15 @@ static int kvm_offline_cpu(unsigned int cpu)
> 		 */
> 		preempt_disable();
> 		hardware_disable_nolock(NULL);
>+		ret = kvm_arch_offline_cpu(cpu, kvm_usage_count);
>+		if (ret) {
>+			(void)hardware_enable_nolock(NULL);
>+			atomic_set(&hardware_enable_failed, 0);

The error-handling code ignores hardware enabling failure which looks
weird to me. If you extract kvm_arch_offline_cpu() directly like what
you do in patch 14 (rather than add a stub function first and then move
some code to the stub function), the error-handling code isn't needed.