| 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
| ||
|
Message-ID: <ZqJ_xANKf3bNcaHM@google.com>
Date: Thu, 25 Jul 2024 09:39:32 -0700
From: David Matlack <dmatlack@...gle.com>
To: James Houghton <jthoughton@...gle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
Paolo Bonzini <pbonzini@...hat.com>,
Ankit Agrawal <ankita@...dia.com>,
Axel Rasmussen <axelrasmussen@...gle.com>,
Catalin Marinas <catalin.marinas@....com>,
David Rientjes <rientjes@...gle.com>,
James Morse <james.morse@....com>, Jason Gunthorpe <jgg@...pe.ca>,
Jonathan Corbet <corbet@....net>, Marc Zyngier <maz@...nel.org>,
Oliver Upton <oliver.upton@...ux.dev>,
Raghavendra Rao Ananta <rananta@...gle.com>,
Ryan Roberts <ryan.roberts@....com>,
Sean Christopherson <seanjc@...gle.com>,
Shaoqin Huang <shahuang@...hat.com>,
Suzuki K Poulose <suzuki.poulose@....com>,
Wei Xu <weixugc@...gle.com>, Will Deacon <will@...nel.org>,
Yu Zhao <yuzhao@...gle.com>, Zenghui Yu <yuzenghui@...wei.com>,
kvmarm@...ts.linux.dev, kvm@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v6 01/11] KVM: Add lockless memslot walk to KVM
On 2024-07-24 01:10 AM, James Houghton wrote:
> Provide flexibility to the architecture to synchronize as optimally as
> they can instead of always taking the MMU lock for writing.
>
> Architectures that do their own locking must select
> CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS.
>
> The immediate application is to allow architectures to implement the
> test/clear_young MMU notifiers more cheaply.
>
> Suggested-by: Yu Zhao <yuzhao@...gle.com>
> Signed-off-by: James Houghton <jthoughton@...gle.com>
Aside from the cleanup suggestion (which should be in separate patches
anyway):
Reviewed-by: David Matlack <dmatlack@...gle.com>
> ---
> include/linux/kvm_host.h | 1 +
> virt/kvm/Kconfig | 3 +++
> virt/kvm/kvm_main.c | 26 +++++++++++++++++++-------
> 3 files changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 689e8be873a7..8cd80f969cff 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -266,6 +266,7 @@ struct kvm_gfn_range {
> gfn_t end;
> union kvm_mmu_notifier_arg arg;
> bool may_block;
> + bool lockless;
> };
> bool kvm_unmap_gfn_range(struct kvm *kvm, struct kvm_gfn_range *range);
> bool kvm_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range);
> diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
> index b14e14cdbfb9..632334861001 100644
> --- a/virt/kvm/Kconfig
> +++ b/virt/kvm/Kconfig
> @@ -100,6 +100,9 @@ config KVM_GENERIC_MMU_NOTIFIER
> select MMU_NOTIFIER
> bool
>
> +config KVM_MMU_NOTIFIER_YOUNG_LOCKLESS
> + bool
> +
> config KVM_GENERIC_MEMORY_ATTRIBUTES
> depends on KVM_GENERIC_MMU_NOTIFIER
> bool
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index d0788d0a72cc..33f8997a5c29 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -555,6 +555,7 @@ struct kvm_mmu_notifier_range {
> on_lock_fn_t on_lock;
> bool flush_on_ret;
> bool may_block;
> + bool lockless;
> };
>
> /*
> @@ -609,6 +610,10 @@ static __always_inline kvm_mn_ret_t __kvm_handle_hva_range(struct kvm *kvm,
> IS_KVM_NULL_FN(range->handler)))
> return r;
>
> + /* on_lock will never be called for lockless walks */
> + if (WARN_ON_ONCE(range->lockless && !IS_KVM_NULL_FN(range->on_lock)))
> + return r;
> +
> idx = srcu_read_lock(&kvm->srcu);
>
> for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
> @@ -640,15 +645,18 @@ static __always_inline kvm_mn_ret_t __kvm_handle_hva_range(struct kvm *kvm,
> gfn_range.start = hva_to_gfn_memslot(hva_start, slot);
> gfn_range.end = hva_to_gfn_memslot(hva_end + PAGE_SIZE - 1, slot);
> gfn_range.slot = slot;
> + gfn_range.lockless = range->lockless;
>
> if (!r.found_memslot) {
> r.found_memslot = true;
> - KVM_MMU_LOCK(kvm);
> - if (!IS_KVM_NULL_FN(range->on_lock))
> - range->on_lock(kvm);
> -
> - if (IS_KVM_NULL_FN(range->handler))
> - goto mmu_unlock;
> + if (!range->lockless) {
> + KVM_MMU_LOCK(kvm);
> + if (!IS_KVM_NULL_FN(range->on_lock))
> + range->on_lock(kvm);
> +
> + if (IS_KVM_NULL_FN(range->handler))
> + goto mmu_unlock;
> + }
> }
> r.ret |= range->handler(kvm, &gfn_range);
> }
> @@ -658,7 +666,7 @@ static __always_inline kvm_mn_ret_t __kvm_handle_hva_range(struct kvm *kvm,
> kvm_flush_remote_tlbs(kvm);
>
> mmu_unlock:
> - if (r.found_memslot)
> + if (r.found_memslot && !range->lockless)
> KVM_MMU_UNLOCK(kvm);
>
> srcu_read_unlock(&kvm->srcu, idx);
> @@ -679,6 +687,8 @@ static __always_inline int kvm_handle_hva_range(struct mmu_notifier *mn,
> .on_lock = (void *)kvm_null_fn,
> .flush_on_ret = true,
> .may_block = false,
> + .lockless =
> + IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS),
> };
>
> return __kvm_handle_hva_range(kvm, &range).ret;
> @@ -697,6 +707,8 @@ static __always_inline int kvm_handle_hva_range_no_flush(struct mmu_notifier *mn
> .on_lock = (void *)kvm_null_fn,
> .flush_on_ret = false,
> .may_block = false,
> + .lockless =
> + IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS),
kvm_handle_hva_range{,_no_flush}() have very generic names but
they're intimately tied to the "young" notifiers. Whereas
__kvm_handle_hva_range() is the truly generic handler function.
This is arguably a pre-existing issue, but adding
CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS makes these functions even more
intamtely tied to the "young" notifiers.
We could rename kvm_handle_hva_range{,_no_flush}() but I think the
cleanest thing to do might be to just drop them entirely and move their
contents into their callers (there are only 2 callers of these 3
functions). That will create a little duplication but IMO will make the
code easier to read.
And then we can also rename __kvm_handle_hva_range() to
kvm_handle_hva_range().
e.g. Something like this as the end result:
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 86fb2b560d98..0146c83e24bd 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -590,8 +590,8 @@ static void kvm_null_fn(void)
node; \
node = interval_tree_iter_next(node, start, last)) \
-static __always_inline kvm_mn_ret_t __kvm_handle_hva_range(struct kvm *kvm,
- const struct kvm_mmu_notifier_range *range)
+static __always_inline kvm_mn_ret_t kvm_handle_hva_range(struct kvm *kvm,
+ const struct kvm_mmu_notifier_range *range)
{
struct kvm_mmu_notifier_return r = {
.ret = false,
@@ -674,48 +674,6 @@ static __always_inline kvm_mn_ret_t __kvm_handle_hva_range(struct kvm *kvm,
return r;
}
-static __always_inline int kvm_handle_hva_range(struct mmu_notifier *mn,
- unsigned long start,
- unsigned long end,
- gfn_handler_t handler)
-{
- struct kvm *kvm = mmu_notifier_to_kvm(mn);
- const struct kvm_mmu_notifier_range range = {
- .start = start,
- .end = end,
- .handler = handler,
- .on_lock = (void *)kvm_null_fn,
- .flush_on_ret = true,
- .may_block = false,
- .lockless =
- IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS),
- };
-
- return __kvm_handle_hva_range(kvm, &range).ret;
-}
-
-static __always_inline int kvm_handle_hva_range_no_flush(struct mmu_notifier *mn,
- unsigned long start,
- unsigned long end,
- gfn_handler_t handler,
- bool fast_only)
-{
- struct kvm *kvm = mmu_notifier_to_kvm(mn);
- const struct kvm_mmu_notifier_range range = {
- .start = start,
- .end = end,
- .handler = handler,
- .on_lock = (void *)kvm_null_fn,
- .flush_on_ret = false,
- .may_block = false,
- .lockless =
- IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS),
- .arg.fast_only = fast_only,
- };
-
- return __kvm_handle_hva_range(kvm, &range).ret;
-}
-
void kvm_mmu_invalidate_begin(struct kvm *kvm)
{
lockdep_assert_held_write(&kvm->mmu_lock);
@@ -808,7 +766,7 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
* that guest memory has been reclaimed. This needs to be done *after*
* dropping mmu_lock, as x86's reclaim path is slooooow.
*/
- if (__kvm_handle_hva_range(kvm, &hva_range).found_memslot)
+ if (kvm_handle_hva_range(kvm, &hva_range).found_memslot)
kvm_arch_guest_memory_reclaimed(kvm);
return 0;
@@ -854,7 +812,7 @@ static void kvm_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn,
};
bool wake;
- __kvm_handle_hva_range(kvm, &hva_range);
+ kvm_handle_hva_range(kvm, &hva_range);
/* Pairs with the increment in range_start(). */
spin_lock(&kvm->mn_invalidate_lock);
@@ -876,6 +834,17 @@ static int kvm_mmu_notifier_clear_flush_young(struct mmu_notifier *mn,
unsigned long start,
unsigned long end)
{
+ struct kvm *kvm = mmu_notifier_to_kvm(mn);
+ const struct kvm_mmu_notifier_range range = {
+ .start = start,
+ .end = end,
+ .handler = kvm_age_gfn,
+ .on_lock = (void *)kvm_null_fn,
+ .flush_on_ret = true,
+ .may_block = false,
+ .lockless = IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS),
+ };
+
trace_kvm_age_hva(start, end, false);
return kvm_handle_hva_range(mn, start, end, kvm_age_gfn);
@@ -887,6 +856,18 @@ static int kvm_mmu_notifier_clear_young(struct mmu_notifier *mn,
unsigned long end,
bool fast_only)
{
+ struct kvm *kvm = mmu_notifier_to_kvm(mn);
+ const struct kvm_mmu_notifier_range range = {
+ .start = start,
+ .end = end,
+ .handler = kvm_age_gfn,
+ .on_lock = (void *)kvm_null_fn,
+ .flush_on_ret = false,
+ .may_block = false,
+ .lockless = IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS),
+ .arg.fast_only = fast_only,
+ };
+
trace_kvm_age_hva(start, end, fast_only);
/*
@@ -902,8 +883,7 @@ static int kvm_mmu_notifier_clear_young(struct mmu_notifier *mn,
* cadence. If we find this inaccurate, we might come up with a
* more sophisticated heuristic later.
*/
- return kvm_handle_hva_range_no_flush(mn, start, end, kvm_age_gfn,
- fast_only);
+ return kvm_handle_hva_range(kvm, &range).ret;
}
static int kvm_mmu_notifier_test_young(struct mmu_notifier *mn,
@@ -911,6 +891,18 @@ static int kvm_mmu_notifier_test_young(struct mmu_notifier *mn,
unsigned long address,
bool fast_only)
{
+ struct kvm *kvm = mmu_notifier_to_kvm(mn);
+ const struct kvm_mmu_notifier_range range = {
+ .start = address,
+ .end = address + 1,
+ .handler = kvm_test_age_gfn,
+ .on_lock = (void *)kvm_null_fn,
+ .flush_on_ret = false,
+ .may_block = false,
+ .lockless = IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS),
+ .arg.fast_only = fast_only,
+ };
+
trace_kvm_test_age_hva(address, fast_only);
return kvm_handle_hva_range_no_flush(mn, address, address + 1,
> };
>
> return __kvm_handle_hva_range(kvm, &range).ret;
> --
> 2.46.0.rc1.232.g9752f9e123-goog
>
Powered by blists - more mailing lists