linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: David Matlack <dmatlack@google.com>
To: Ben Gardon <bgardon@google.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
	Paolo Bonzini <pbonzini@redhat.com>, Peter Xu <peterx@redhat.com>,
	Sean Christopherson <seanjc@google.com>,
	Vipin Sharma <vipinsh@google.com>,
	Nagareddy Reddy <nspreddy@google.com>
Subject: Re: [RFC 00/14] KVM: x86/MMU: Formalize the Shadow MMU
Date: Fri, 6 Jan 2023 11:18:10 -0800	[thread overview]
Message-ID: <Y7hz8geAGgysptY5@google.com> (raw)
In-Reply-To: <20221221222418.3307832-1-bgardon@google.com>

On Wed, Dec 21, 2022 at 10:24:04PM +0000, Ben Gardon wrote:
> This series makes the Shadow MMU a distinct part of the KVM x86 MMU,
> implemented in separate files, with a defined interface to common code.

Overall I really like the end result.

While looking through I found a few more bits of code that should
probably be moved into shadow_mmu.c:

 - kvm_mmu_zap_all(): Move the shadow MMU zapping to shadow_mmu.c (the
   active_mmu_pages loop + commit_zap_page).

 - need_topup(), need_topup_split_caches_or_resched()
   topup_split_caches() should be static functions in shadow_mmu.c.

 - Split out kvm_mmu_init/uninit_vm() functions for the shadow MMU.
   Notably, the split caches, active_mmu_pages, zapped_obsolete_pages,
   and other Shadow MMU-specific stuff can go in shadow_mmu.c.

 - The Shadow MMU parts of walk_shadow_page_lockless_begin/end() should
   go in shadow_mmu.c. e.g. kvm_shadow_mmu_walk_lockless_begin/end().

> Patch 3 is an enormous change, and doing it all at once in a single
> commit all but guarantees merge conflicts and makes it hard to review. I
> don't have a good answer to this problem as there's no easy way to move
> 3.5K lines between files. I tried moving the code bit-by-bit but the
> intermediate steps added complexity and ultimately the 50+ patches it
> created didn't seem any easier to review.
> Doing the big move all at once at least makes it easier to get past when
> doing Git archeology, and doing it at the beggining of the series allows the
> rest of the commits to still show up in Git blame.

An alternative would be to rename mmu.c to shadow_mmu.c first and then
move code in the opposite direction. That would preserve the git-blame
history for shadow_mmu.c. But by the end of the series mmu.c and
shadow_mmu.c are both ~3K LOC, so I don't think doing this is really any
better. Either way, you have to move ~3K LOC.

  parent reply	other threads:[~2023-01-06 19:18 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-21 22:24 [RFC 00/14] KVM: x86/MMU: Formalize the Shadow MMU Ben Gardon
2022-12-21 22:24 ` [RFC 01/14] KVM: x86/MMU: Add shadow_mmu.(c|h) Ben Gardon
2023-02-01 19:45   ` Sean Christopherson
2023-02-01 19:48     ` Ben Gardon
2022-12-21 22:24 ` [RFC 02/14] KVM: x86/MMU: Expose functions for the Shadow MMU Ben Gardon
2022-12-21 22:24 ` [RFC 03/14] KVM: x86/MMU: Move the Shadow MMU implementation to shadow_mmu.c Ben Gardon
2022-12-21 22:40   ` Ben Gardon
2023-01-13 18:06     ` Vipin Sharma
2022-12-21 22:24 ` [RFC 04/14] KVM: x86/MMU: Expose functions for paging_tmpl.h Ben Gardon
2023-01-06 19:49   ` David Matlack
2023-01-09 18:47     ` Ben Gardon
2022-12-21 22:24 ` [RFC 05/14] KVM: x86/MMU: Move paging_tmpl.h includes to shadow_mmu.c Ben Gardon
2022-12-21 22:24 ` [RFC 06/14] KVM: x86/MMU: Clean up Shadow MMU exports Ben Gardon
2022-12-21 22:24 ` [RFC 07/14] KVM: x86/MMU: Cleanup shrinker interface with Shadow MMU Ben Gardon
2023-01-13 17:43   ` Vipin Sharma
2023-01-13 17:51   ` Vipin Sharma
2022-12-21 22:24 ` [RFC 08/14] KVM: x86/MMU: Clean up naming of exported Shadow MMU functions Ben Gardon
2022-12-21 22:24 ` [RFC 09/14] KVM: x86/MMU: Only make pages available on Shadow MMU fault Ben Gardon
2022-12-29 18:30   ` David Matlack
2022-12-21 22:24 ` [RFC 10/14] KVM: x86/MMU: Fix naming on prepare / commit zap page functions Ben Gardon
2022-12-21 22:24 ` [RFC 11/14] KVM: x86/MMU: Factor Shadow MMU wrprot / clear dirty ops out of mmu.c Ben Gardon
2022-12-21 22:24 ` [RFC 12/14] KVM: x86/MMU: Remove unneeded exports from shadow_mmu.c Ben Gardon
2022-12-21 22:24 ` [RFC 13/14] KVM: x86/MMU: Wrap uses of kvm_handle_gfn_range in mmu.c Ben Gardon
2023-02-01 21:25   ` Sean Christopherson
2023-02-01 22:30     ` Ben Gardon
2022-12-21 22:24 ` [RFC 14/14] KVM: x86/MMU: Add kvm_shadow_mmu_ to the last few functions in shadow_mmu.h Ben Gardon
2023-01-06 19:18 ` David Matlack [this message]
2023-01-09 18:43   ` [RFC 00/14] KVM: x86/MMU: Formalize the Shadow MMU Ben Gardon
2023-02-01 20:02 ` Sean Christopherson
2023-02-01 20:45   ` Ben Gardon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y7hz8geAGgysptY5@google.com \
    --to=dmatlack@google.com \
    --cc=bgardon@google.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nspreddy@google.com \
    --cc=pbonzini@redhat.com \
    --cc=peterx@redhat.com \
    --cc=seanjc@google.com \
    --cc=vipinsh@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).