On 9/14/2023 9:55 AM, Sean Christopherson wrote:
> From: Chao Peng <[email protected]>
>
> Currently in mmu_notifier invalidate path, hva range is recorded and
> then checked against by mmu_notifier_retry_hva() in the page fault
> handling path. However, for the to be introduced private memory, a page
> fault may not have a hva associated, checking gfn(gpa) makes more sense.
>
> For existing hva based shared memory, gfn is expected to also work. The
> only downside is when aliasing multiple gfns to a single hva, the
> current algorithm of checking multiple ranges could result in a much
> larger range being rejected. Such aliasing should be uncommon, so the
> impact is expected small.
>
> Suggested-by: Sean Christopherson <[email protected]>
> Signed-off-by: Chao Peng <[email protected]>
> Reviewed-by: Fuad Tabba <[email protected]>
> Tested-by: Fuad Tabba <[email protected]>
> [sean: convert vmx_set_apic_access_page_addr() to gfn-based API]
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/kvm/mmu/mmu.c | 10 ++++++----
> arch/x86/kvm/vmx/vmx.c | 11 +++++------
> include/linux/kvm_host.h | 33 +++++++++++++++++++++------------
> virt/kvm/kvm_main.c | 40 +++++++++++++++++++++++++++++++---------
> 4 files changed, 63 insertions(+), 31 deletions(-)
>
[...]
>
> -void kvm_mmu_invalidate_begin(struct kvm *kvm, unsigned long start,
> - unsigned long end)
> +void kvm_mmu_invalidate_begin(struct kvm *kvm)
> {
> + lockdep_assert_held_write(&kvm->mmu_lock);
> /*
> * The count increase must become visible at unlock time as no
> * spte can be established without taking the mmu_lock and
> * count is also read inside the mmu_lock critical section.
> */
> kvm->mmu_invalidate_in_progress++;
> +
> + if (likely(kvm->mmu_invalidate_in_progress == 1))
> + kvm->mmu_invalidate_range_start = INVALID_GPA;
> +}
> +
> +void kvm_mmu_invalidate_range_add(struct kvm *kvm, gfn_t start, gfn_t end)
> +{
> + lockdep_assert_held_write(&kvm->mmu_lock);
> +
> + WARN_ON_ONCE(!kvm->mmu_invalidate_in_progress);
> +
> if (likely(kvm->mmu_invalidate_in_progress == 1)) {
> kvm->mmu_invalidate_range_start = start;
> kvm->mmu_invalidate_range_end = end;
> @@ -771,6 +781,12 @@ void kvm_mmu_invalidate_begin(struct kvm *kvm, unsigned long start,
> }
> }
>
> +static bool kvm_mmu_unmap_gfn_range(struct kvm *kvm, struct kvm_gfn_range *range)
> +{
> + kvm_mmu_invalidate_range_add(kvm, range->start, range->end);
> + return kvm_unmap_gfn_range(kvm, range);
> +}
> +
> static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
> const struct mmu_notifier_range *range)
> {
> @@ -778,7 +794,7 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
> const struct kvm_mmu_notifier_range hva_range = {
> .start = range->start,
> .end = range->end,
> - .handler = kvm_unmap_gfn_range,
> + .handler = kvm_mmu_unmap_gfn_range,
> .on_lock = kvm_mmu_invalidate_begin,
> .on_unlock = kvm_arch_guest_memory_reclaimed,
> .flush_on_ret = true,
> @@ -817,8 +833,7 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
> return 0;
> }
>
> -void kvm_mmu_invalidate_end(struct kvm *kvm, unsigned long start,
> - unsigned long end)
> +void kvm_mmu_invalidate_end(struct kvm *kvm)
> {
> /*
> * This sequence increase will notify the kvm page fault that
> @@ -833,6 +848,13 @@ void kvm_mmu_invalidate_end(struct kvm *kvm, unsigned long start,
> * in conjunction with the smp_rmb in mmu_invalidate_retry().
> */
> kvm->mmu_invalidate_in_progress--;
> +
> + /*
> + * Assert that at least one range must be added between start() and
> + * end(). Not adding a range isn't fatal, but it is a KVM bug.
> + */
> + WARN_ON_ONCE(kvm->mmu_invalidate_in_progress &&
> + kvm->mmu_invalidate_range_start == INVALID_GPA);
Should the check happen before the decrease of
kvm->mmu_invalidate_in_progress?
Otherwise, KVM calls kvm_mmu_invalidate_begin(), then
kvm_mmu_invalidate_end()
the check will not take effect.
> }
>
> static void kvm_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn,
On Thu, Sep 14, 2023, Binbin Wu wrote:
>
> On 9/14/2023 9:55 AM, Sean Christopherson wrote:
> > +void kvm_mmu_invalidate_end(struct kvm *kvm)
> > {
> > /*
> > * This sequence increase will notify the kvm page fault that
> > @@ -833,6 +848,13 @@ void kvm_mmu_invalidate_end(struct kvm *kvm, unsigned long start,
> > * in conjunction with the smp_rmb in mmu_invalidate_retry().
> > */
> > kvm->mmu_invalidate_in_progress--;
> > +
> > + /*
> > + * Assert that at least one range must be added between start() and
> > + * end(). Not adding a range isn't fatal, but it is a KVM bug.
> > + */
> > + WARN_ON_ONCE(kvm->mmu_invalidate_in_progress &&
> > + kvm->mmu_invalidate_range_start == INVALID_GPA);
> Should the check happen before the decrease of kvm->mmu_invalidate_in_progress?
> Otherwise, KVM calls kvm_mmu_invalidate_begin(), then kvm_mmu_invalidate_end()
> the check will not take effect.
Indeed. I'm pretty sure I added this code, not sure what I was thinking. There's
no reason to check mmu_invalidate_in_progress, it's not like KVM allows
mmu_invalidate_in_progress to go negative. The comment is also a bit funky. I'll
post a fixup patch to make it look like this (assuming I'm not forgetting a subtle
reason for guarding the check with the in-progress flag):
/*
* Assert that at least one range was added between start() and end().
* Not adding a range isn't fatal, but it is a KVM bug.
*/
WARN_ON_ONCE(kvm->mmu_invalidate_range_start == INVALID_GPA);
Regarding kvm->mmu_invalidate_in_progress, this would be a good opportunity to
move the BUG_ON() into the common end(), e.g. as is, an end() without a start()
from something other than the generic mmu_notifier would go unnoticed. And I
_think_ we can replace the BUG_ON() with a KVM_BUG_ON() without putting the
kernel at risk. E.g.
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index dd948276e5d6..54480655bcce 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -870,6 +870,7 @@ void kvm_mmu_invalidate_end(struct kvm *kvm)
* in conjunction with the smp_rmb in mmu_invalidate_retry().
*/
kvm->mmu_invalidate_in_progress--;
+ KVM_BUG_ON(kvm->mmu_invalidate_in_progress < 0, kvm);
/*
* Assert that at least one range was added between start() and end().
@@ -905,8 +906,6 @@ static void kvm_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn,
*/
if (wake)
rcuwait_wake_up(&kvm->mn_memslots_update_rcuwait);
-
- BUG_ON(kvm->mmu_invalidate_in_progress < 0);
}
static int kvm_mmu_notifier_clear_flush_young(struct mmu_notifier *mn,