Received-SPF: pass (google.com: domain of linux-kernel+bounces-64167-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240206153405.489531-1-avagin@google.com> <ZcujIJemLxhjnjfN@google.com>
In-Reply-To: <ZcujIJemLxhjnjfN@google.com>
From: Andrei Vagin <avagin@google.com>
Date: Tue, 13 Feb 2024 11:32:18 -0800
Message-ID: <CAEWA0a78GhJ1FUmk7JX-kCKTCD2vjvzNoBbFODV6BJeu=1mKLg@mail.gmail.com>
Subject: Re: [PATCH v2] kvm/x86: allocate the write-tracking metadata on-demand
To: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, linux-kernel@vger.kernel.org, x86@kernel.org, 
	kvm@vger.kernel.org, Zhenyu Wang <zhenyuw@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Feb 13, 2024 at 9:13=E2=80=AFAM Sean Christopherson <seanjc@google.=
com> wrote:
>
> On Tue, Feb 06, 2024, Andrei Vagin wrote:
> > The write-track is used externally only by the gpu/drm/i915 driver.
> > Currently, it is always enabled, if a kernel has been compiled with thi=
s
> > driver.
> >
> > Enabling the write-track mechanism adds a two-byte overhead per page ac=
ross
> > all memory slots. It isn't significant for regular VMs. However in gVis=
or,
> > where the entire process virtual address space is mapped into the VM, e=
ven
> > with a 39-bit address space, the overhead amounts to 256MB.
> >
> > This change rework the write-tracking mechanism to enable it on-demand
> > in kvm_page_track_register_notifier.
>
> Don't use "this change", "this patch", or any other variant of "this blah=
" that
> you come up with.  :-)  Simply phrase the changelog as a command.

ok:)

>
> > Here is Sean's comment about the locking scheme:
> >
> > The only potential hiccup would be if taking slots_arch_lock would
> > deadlock, but it should be impossible for slots_arch_lock to be taken i=
n
> > any other path that involves VFIO and/or KVMGT *and* can be coincident.
> > Except for kvm_arch_destroy_vm() (which deletes KVM's internal
> > memslots), slots_arch_lock is taken only through KVM ioctls(), and the
> > caller of kvm_page_track_register_notifier() *must* hold a reference to
> > the VM.
> >
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Sean Christopherson <seanjc@google.com>
> > Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> > Suggested-by: Sean Christopherson <seanjc@google.com>
> > Signed-off-by: Andrei Vagin <avagin@google.com>
> > ---
> > v1: https://lore.kernel.org/lkml/ZcErs9rPqT09nNge@google.com/T/
> > v2: allocate the write-tracking metadata on-demand
> >
> >  arch/x86/include/asm/kvm_host.h |  2 +
> >  arch/x86/kvm/mmu/mmu.c          | 24 +++++------
> >  arch/x86/kvm/mmu/page_track.c   | 74 ++++++++++++++++++++++++++++-----
> >  arch/x86/kvm/mmu/page_track.h   |  3 +-
> >  4 files changed, 78 insertions(+), 25 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm=
_host.h
> > index d271ba20a0b2..c35641add93c 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1503,6 +1503,8 @@ struct kvm_arch {
> >        */
> >  #define SPLIT_DESC_CACHE_MIN_NR_OBJECTS (SPTE_ENT_PER_PAGE + 1)
> >       struct kvm_mmu_memory_cache split_desc_cache;
> > +
> > +     bool page_write_tracking_enabled;
>
> Rather than a generic page_write_tracking_enabled, I think it makes sense=
 to
> explicitly track if there are *external* write tracking users.  One could=
 argue
> it makes the total tracking *too* fine grained, but I think it would be h=
elpful
> for readers to when KVM itself is using write tracking (shadow paging) ve=
rsus
> when KVM has write tracking enabled, but has not allocated rmaps (externa=
l write
> tracking user).
>
> That way, kernels with CONFIG_KVM_EXTERNAL_WRITE_TRACKING=3Dn don't need =
to check
> the bool (though they'll still check kvm_shadow_root_allocated()).  And a=
s a
> bonus, the diff is quite a bit smaller.
>

Your patch looks good to me. I ran kvm and gvisor tests and didn't
find any issues. I sent it as v3:
https://lkml.org/lkml/2024/2/13/1181

I didn't do any changes, so feel free to change the author.

Thanks for the help.