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;
MIME-Version: 1.0
References: <20221221222418.3307832-1-bgardon@google.com> <Y7hz8geAGgysptY5@google.com>
In-Reply-To: <Y7hz8geAGgysptY5@google.com>
From:   Ben Gardon <bgardon@google.com>
Date:   Mon, 9 Jan 2023 10:43:58 -0800
Message-ID: <CANgfPd-6-oxtH3cTH8+1KKJx7bnWABjvLVjCrMZ7Hp5Wmy53ZQ@mail.gmail.com>
Subject: Re: [RFC 00/14] KVM: x86/MMU: Formalize the Shadow MMU
To:     David Matlack <dmatlack@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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Jan 6, 2023 at 11:18 AM David Matlack <dmatlack@google.com> wrote:
>
> 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().

Awesome, thank you for pointing these out. I'll work them into a V1.

>
> > 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.

I tried implementing this refactor both ways and ultimately found this
way to be a lot cleaner. Preserving the git blame for the Shadow MMU
code would be nice, since IMO it's the more complex code, but it got
complicated quickly. The in-between stages of moving around function
definitions to header files, and detangling code to move it back to
mmu.c, was a nightmare. It's relatively easy to move the leaf
functions in the call-tree, but I found moving the upper level
functions was difficult to do bit-by-bit.
If anyone wants to try implementing this commit in a more elegant way,
I'm happy to rebase the rest of the series on  top of it.
As you said, either way we gotta move 3K lines of code.