linux-kernel - Re: [PATCH 15/24] kvm: mmu: Wrap mmu_lock cond

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <YAjIddUuw/SZ+7ut@google.com>
Date:   Wed, 20 Jan 2021 16:19:01 -0800
From:   Sean Christopherson <seanjc@...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>, Peter Shier <pshier@...gle.com>,
        Peter Feiner <pfeiner@...gle.com>,
        Junaid Shahid <junaids@...gle.com>,
        Jim Mattson <jmattson@...gle.com>,
        Yulei Zhang <yulei.kernel@...il.com>,
        Wanpeng Li <kernellwp@...il.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Xiao Guangrong <xiaoguangrong.eric@...il.com>
Subject: Re: [PATCH 15/24] kvm: mmu: Wrap mmu_lock cond_resched and needbreak

On Tue, Jan 12, 2021, Ben Gardon wrote:
> Wrap the MMU lock cond_reseched and needbreak operations in a function.
> This will support a refactoring to move the lock into the struct
> kvm_arch(s) so that x86 can change the spinlock to a rwlock without
> affecting the performance of other archs.

IMO, moving the lock to arch-specific code is bad for KVM.  The architectures'
MMUs already diverge pretty horribly, and once things diverge it's really hard
to go the other direction.  And this change, along with all of the wrappers,
thrash a  lot of code and add a fair amount of indirection without any real
benefit to the other architectures.

What if we simply make the common mmu_lock a union?  The rwlock_t is probably a
bit bigger, but that's a few bytes for an entire VM.  And maybe this would
entice/inspire other architectures to move to a similar MMU model.

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index f3b1013fb22c..bbc8efd4af62 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -451,7 +451,10 @@ struct kvm_memslots {
 };

 struct kvm {
-       spinlock_t mmu_lock;
+       union {
+               rwlock_t mmu_rwlock;
+               spinlock_t mmu_lock;
+       };
        struct mutex slots_lock;
        struct mm_struct *mm; /* userspace tied to this vm */
        struct kvm_memslots __rcu *memslots[KVM_ADDRESS_SPACE_NUM];