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