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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5D7DFC433F5 for ; Mon, 23 May 2022 22:27:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229762AbiEWW12 (ORCPT ); Mon, 23 May 2022 18:27:28 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44800 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229542AbiEWW1Y (ORCPT ); Mon, 23 May 2022 18:27:24 -0400 Received: from mail-yb1-xb2b.google.com (mail-yb1-xb2b.google.com [IPv6:2607:f8b0:4864:20::b2b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2709F8A305 for ; Mon, 23 May 2022 15:27:22 -0700 (PDT) Received: by mail-yb1-xb2b.google.com with SMTP id p190so990085ybg.4 for ; Mon, 23 May 2022 15:27:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=x9UvpRLSY5zsz98jmsxKkKeHOvgVXmiYIhmkTUpVkyM=; b=jckEK50RFGV8DufRw4qT6Z0WDGO6UlG2Lh2PP4EDu2xD7NgLU6p4j3NitfMa1A/LFx 1kTG1pVXrmpgTXrSOru9knU3e1C/aycFL2S3ifwCHAn+eUTpx/WWwCqJixVugsDWQZFx d9hF/nT/rsbB5Ffx9nIft4by/5AJR7Qsfkwa7+jE4KRKDC20Nq4EWU0Jp2leg3UvuygL S5hgOEFUyU/EFN6piNWGz89ad0yoz04JckXNTXK2UKUKg3UbK1m6BXKuzWTljP8DIYJR 5921C4ORMend1M0cg4FvpsuB6GSjr3Z4RysSk04+lYsh5Z/CCD052zwvIacuqaPbAU1o TQ1w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=x9UvpRLSY5zsz98jmsxKkKeHOvgVXmiYIhmkTUpVkyM=; b=wwRjdz8GcQmKd0chJbm5cEMk70NU6aBQg8gW/8SFJ4Vz4r06iOHLbsfPo/YVp9UKVE kxvXgpmyUkctHPgaKZRy1JUTY8wCegACP0B3bg48KtH8jVyNrqqF0Ebs1c3IugxRbAAh zAujhgquEkxae/uMhSAacueG5hnxr5wZD8e/r1TiBzW16FdGzTQtpOH1Iz6C/9HqDRwM Kxh3G4VdpRfXA1fS7QpP0flT3sm13ZxquPrFNEZ/43mkne+qfbLCX6BgPDIyVh0cyTlV NGKssuS/kGEqyOHfxJfl0L9VGE4TQzUOGWorkrkbsPmk0+qwgBsFcHv9wbfOJD1Pf89O jmFA== X-Gm-Message-State: AOAM532fVpB7TdFdlzvLO1tul2ixGaBZqGoX5j0SHqJm9ZRh/sOXRG3L gFe9iedzkTFD9/XXZ3wFI5B2VgEwjvOLs4a98MV1zhyRg6ej+g== X-Google-Smtp-Source: ABdhPJzIghg22UtEe1dAs5j2rwLl7sRnoxOFf4nS7Fci0D6HMktnaBG16wLf6m/6zODNYR4O0NjfWUHIC4uJeGMFR+Y= X-Received: by 2002:a05:6902:1102:b0:64f:37a3:6b9e with SMTP id o2-20020a056902110200b0064f37a36b9emr22428531ybu.21.1653344841151; Mon, 23 May 2022 15:27:21 -0700 (PDT) MIME-Version: 1.0 References: <75912816e498ddf62e7efb6a187d763c89e72f45.1651774250.git.isaku.yamahata@intel.com> In-Reply-To: <75912816e498ddf62e7efb6a187d763c89e72f45.1651774250.git.isaku.yamahata@intel.com> From: Sagi Shahar Date: Mon, 23 May 2022 15:27:10 -0700 Message-ID: Subject: Re: [RFC PATCH v6 003/104] KVM: Refactor CPU compatibility check on module initialiization To: "Yamahata, Isaku" Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, isaku.yamahata@gmail.com, Paolo Bonzini , Erdem Aktas , Sean Christopherson Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 5, 2022 at 11:15 AM wrote: > > From: Isaku Yamahata > > Although non-x86 arch doesn't break as long as I inspected code, it's by > code inspection. This should be reviewed by each arch maintainers. > > kvm_init() checks CPU compatibility by calling > kvm_arch_check_processor_compat() on all online CPUs. Move the callback > to hardware_enable_nolock() and add hardware_enable_all() and > hardware_disable_all(). > Add arch specific callback kvm_arch_post_hardware_enable_setup() for arch > to do arch specific initialization that required hardware_enable_all(). There's no reference to kvm_arch_post_hardware_enable_setup in this patch. Looks like it's introduced in a later patch in the series. This patch might be clearer if the kvm_arch_post_hardware_enable_setup is introduced and used here. Otherwise, the commit log should be updated to make it clear that kvm_arch_post_hardware_enable_setup is introduced in a later patch in the series. > This makes a room for TDX module to initialize on kvm module loading. TDX > module requires all online cpu to enable VMX by VMXON. > > If kvm_arch_hardware_enable/disable() depend on (*) part, such dependency > must be called before kvm_init(). In fact kvm_intel() does. Although > other arch doesn't as long as I checked as follows, it should be reviewed > by each arch maintainers. > > Before this patch: > - Arch module initialization > - kvm_init() > - kvm_arch_init() > - kvm_arch_check_processor_compat() on each CPUs > - post arch specific initialization ---- (*) > > - when creating/deleting first/last VM > - kvm_arch_hardware_enable() on each CPUs --- (A) > - kvm_arch_hardware_disable() on each CPUs --- (B) > > After this patch: > - Arch module initialization > - kvm_init() > - kvm_arch_init() > - kvm_arch_hardware_enable() on each CPUs (A) > - kvm_arch_check_processor_compat() on each CPUs > - kvm_arch_hardware_disable() on each CPUs (B) > - post arch specific initialization --- (*) > > Code inspection result: > (A)/(B) can depend on (*) before this patch. If there is dependency, such > initialization must be moved before kvm_init() with this patch. VMX does > in fact. As long as I inspected other archs and find only mips has it. > > - arch/mips/kvm/mips.c > module init function, kvm_mips_init(), does some initialization after > kvm_init(). Compile test only. Needs review. > > - arch/x86/kvm/x86.c > - uses vm_list which is statically initialized. > - static_call(kvm_x86_hardware_enable)(); > - SVM: (*) is empty. > - VMX: needs fix > > - arch/powerpc/kvm/powerpc.c > kvm_arch_hardware_enable/disable() are nop > > - arch/s390/kvm/kvm-s390.c > kvm_arch_hardware_enable/disable() are nop > > - arch/arm64/kvm/arm.c > module init function, arm_init(), calls only kvm_init(). > (*) is empty > > - arch/riscv/kvm/main.c > module init function, riscv_kvm_init(), calls only kvm_init(). > (*) is empty > > Co-developed-by: Sean Christopherson > Signed-off-by: Sean Christopherson > Signed-off-by: Isaku Yamahata > --- > arch/mips/kvm/mips.c | 12 +++++++----- > arch/x86/kvm/vmx/vmx.c | 15 +++++++++++---- > virt/kvm/kvm_main.c | 20 ++++++++++---------- > 3 files changed, 28 insertions(+), 19 deletions(-) > > diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c > index 092d09fb6a7e..17228584485d 100644 > --- a/arch/mips/kvm/mips.c > +++ b/arch/mips/kvm/mips.c > @@ -1643,11 +1643,6 @@ static int __init kvm_mips_init(void) > } > > ret = kvm_mips_entry_setup(); > - if (ret) > - return ret; > - > - ret = kvm_init(NULL, sizeof(struct kvm_vcpu), 0, THIS_MODULE); > - > if (ret) > return ret; > > @@ -1656,6 +1651,13 @@ static int __init kvm_mips_init(void) > > register_die_notifier(&kvm_mips_csr_die_notifier); > > + ret = kvm_init(NULL, sizeof(struct kvm_vcpu), 0, THIS_MODULE); > + > + if (ret) { > + unregister_die_notifier(&kvm_mips_csr_die_notifier); > + return ret; > + } > + > return 0; > } > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index e30493fe4553..9bc46c1e64d9 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -8254,6 +8254,15 @@ static void vmx_exit(void) > } > module_exit(vmx_exit); > > +/* initialize before kvm_init() so that hardware_enable/disable() can work. */ > +static void __init vmx_init_early(void) > +{ > + int cpu; > + > + for_each_possible_cpu(cpu) > + INIT_LIST_HEAD(&per_cpu(loaded_vmcss_on_cpu, cpu)); > +} > + > static int __init vmx_init(void) > { > int r, cpu; > @@ -8291,6 +8300,7 @@ static int __init vmx_init(void) > } > #endif > > + vmx_init_early(); > r = kvm_init(&vmx_init_ops, sizeof(struct vcpu_vmx), > __alignof__(struct vcpu_vmx), THIS_MODULE); > if (r) > @@ -8309,11 +8319,8 @@ static int __init vmx_init(void) > return r; > } > > - for_each_possible_cpu(cpu) { > - INIT_LIST_HEAD(&per_cpu(loaded_vmcss_on_cpu, cpu)); > - > + for_each_possible_cpu(cpu) > pi_init_cpu(cpu); > - } > > #ifdef CONFIG_KEXEC_CORE > rcu_assign_pointer(crash_vmclear_loaded_vmcss, > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index ec365291c625..0ff03889aa5d 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -4883,8 +4883,13 @@ static void hardware_enable_nolock(void *junk) > > cpumask_set_cpu(cpu, cpus_hardware_enabled); > > + r = kvm_arch_check_processor_compat(); > + if (r) > + goto out; > + > r = kvm_arch_hardware_enable(); > > +out: > if (r) { > cpumask_clear_cpu(cpu, cpus_hardware_enabled); > atomic_inc(&hardware_enable_failed); > @@ -5681,11 +5686,6 @@ void kvm_unregister_perf_callbacks(void) > } > #endif > > -static void check_processor_compat(void *rtn) > -{ > - *(int *)rtn = kvm_arch_check_processor_compat(); > -} > - > int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align, > struct module *module) > { > @@ -5716,11 +5716,11 @@ int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align, > if (r < 0) > goto out_free_1; > > - for_each_online_cpu(cpu) { > - smp_call_function_single(cpu, check_processor_compat, &r, 1); > - if (r < 0) > - goto out_free_2; > - } > + /* hardware_enable_nolock() checks CPU compatibility on each CPUs. */ > + r = hardware_enable_all(); > + if (r) > + goto out_free_2; > + hardware_disable_all(); > > r = cpuhp_setup_state_nocalls(CPUHP_AP_KVM_STARTING, "kvm/cpu:starting", > kvm_starting_cpu, kvm_dying_cpu); > -- > 2.25.1 > Sagi