linux-kernel - Re: [RFC 00/14] KVM: x86/MMU: Formalize the Shadow MMU

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y7hz8geAGgysptY5@google.com>
Date:   Fri, 6 Jan 2023 11:18:10 -0800
From:   David Matlack <dmatlack@...gle.com>
To:     Ben Gardon <bgardon@...gle.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        Peter Xu <peterx@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Vipin Sharma <vipinsh@...gle.com>,
        Nagareddy Reddy <nspreddy@...gle.com>
Subject: Re: [RFC 00/14] KVM: x86/MMU: Formalize the Shadow MMU

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.