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:   Thu, 1 Sep 2022 10:49:53 -0700
From:   Isaku Yamahata <isaku.yamahata@gmail.com>
To:     Sean Christopherson <seanjc@google.com>
Cc:     Chao Gao <chao.gao@intel.com>, isaku.yamahata@intel.com,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        isaku.yamahata@gmail.com, Paolo Bonzini <pbonzini@redhat.com>,
        Kai Huang <kai.huang@intel.com>, Will Deacon <will@kernel.org>
Subject: Re: [PATCH v2 01/19] KVM: x86: Drop kvm_user_return_msr_cpu_online()
Message-ID: <20220901174953.GH2711697@ls.amr.corp.intel.com>
References: <cover.1661860550.git.isaku.yamahata@intel.com>
 <f63a395ead4204d44cab3b734c99b07f54c38463.1661860550.git.isaku.yamahata@intel.com>
 <YxBDRaAyRpyz/5Q+@gao-cwp>
 <YxC96HujrBAwlgK0@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <YxC96HujrBAwlgK0@google.com>
Precedence: bulk

On Thu, Sep 01, 2022 at 02:12:56PM +0000,
Sean Christopherson <seanjc@google.com> wrote:

> On Thu, Sep 01, 2022, Chao Gao wrote:
> > On Tue, Aug 30, 2022 at 05:01:16AM -0700, isaku.yamahata@intel.com wrote:
> > >From: Isaku Yamahata <isaku.yamahata@intel.com>
> > >
> > >KVM/X86 uses user return notifier to switch MSR for guest or user space.
> > >Snapshot host values on CPU online, change MSR values for guest, and
> > >restore them on returning to user space.  The current code abuses
> > >kvm_arch_hardware_enable() which is called on kvm module initialization or
> > >CPU online.
> > >
> > >Remove such the abuse of kvm_arch_hardware_enable by capturing the host
> > >value on the first change of the MSR value to guest VM instead of CPU
> > >online.
> > >
> > >Suggested-by: Sean Christopherson <seanjc@google.com>
> > >Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > >---
> > > arch/x86/kvm/x86.c | 43 ++++++++++++++++++++++++-------------------
> > > 1 file changed, 24 insertions(+), 19 deletions(-)
> > >
> > >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > >index 205ebdc2b11b..16104a2f7d8e 100644
> > >--- a/arch/x86/kvm/x86.c
> > >+++ b/arch/x86/kvm/x86.c
> > >@@ -200,6 +200,7 @@ struct kvm_user_return_msrs {
> > > 	struct kvm_user_return_msr_values {
> > > 		u64 host;
> > > 		u64 curr;
> > >+		bool initialized;
> > > 	} values[KVM_MAX_NR_USER_RETURN_MSRS];
> > 
> > The benefit of having an "initialized" state for each user return MSR on
> > each CPU is small. A per-cpu state looks suffice. With it, you can keep
> > kvm_user_return_msr_cpu_online() and simply call the function from
> > kvm_set_user_return_msr() if initialized is false on current CPU.
> 
> Yep, a per-CPU flag is I intended.  This is the completely untested patch that's
> sitting in a development branch of mine.

With the following fix, it worked.  I'll replace this patch with yours.

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 205ebdc2b11b..0e200fe44b35 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9212,7 +9217,12 @@ int kvm_arch_init(void *opaque)
                return -ENOMEM;
        }
 
-       user_return_msrs = alloc_percpu(struct kvm_user_return_msrs);
+       /*
+        * __GFP_ZERO to ensure user_return_msrs.values[].initialized = false.
+        * See kvm_user_return_msr_init_cpu().
+        */
+       user_return_msrs = alloc_percpu_gfp(struct kvm_user_return_msrs,
+                                           GFP_KERNEL | __GFP_ZERO);
        if (!user_return_msrs) {
                printk(KERN_ERR "kvm: failed to allocate percpu kvm_user_return_msrs\n");
                r = -ENOMEM;

-- 
Isaku Yamahata <isaku.yamahata@gmail.com>