On Tue, Jan 12, 2021, Ben Gardon wrote:
> Add a lock to protect the data structures that track the page table
> memory used by the TDP MMU. In order to handle multiple TDP MMU
> operations in parallel, pages of PT memory must be added and removed
> without the exclusive protection of the MMU lock. A new lock to protect
> the list(s) of in-use pages will cause some serialization, but only on
> non-leaf page table entries, so the lock is not expected to be very
> contended.
>
> Reviewed-by: Peter Feiner <pfeiner@xxxxxxxxxx>
>
> Signed-off-by: Ben Gardon <bgardon@xxxxxxxxxx>
> ---
> arch/x86/include/asm/kvm_host.h | 15 ++++++++
> arch/x86/kvm/mmu/tdp_mmu.c | 67 +++++++++++++++++++++++++++++----
> 2 files changed, 74 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 92d5340842c8..f8dccb27c722 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1034,6 +1034,21 @@ struct kvm_arch {
> * tdp_mmu_page set and a root_count of 0.
> */
> struct list_head tdp_mmu_pages;
> +
> + /*
> + * Protects accesses to the following fields when the MMU lock is
> + * not held exclusively:
> + * - tdp_mmu_pages (above)
> + * - the link field of struct kvm_mmu_pages used by the TDP MMU
> + * when they are part of tdp_mmu_pages (but not when they are part
> + * of the tdp_mmu_free_list or tdp_mmu_disconnected_list)
Neither tdp_mmu_free_list nor tdp_mmu_disconnected_list exists.
> + * - lpage_disallowed_mmu_pages
> + * - the lpage_disallowed_link field of struct kvm_mmu_pages used
> + * by the TDP MMU
> + * May be acquired under the MMU lock in read mode or non-overlapping
> + * with the MMU lock.
> + */
> + spinlock_t tdp_mmu_pages_lock;
> };
>
> struct kvm_vm_stat {
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 8b61bdb391a0..264594947c3b 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -33,6 +33,7 @@ void kvm_mmu_init_tdp_mmu(struct kvm *kvm)
> kvm->arch.tdp_mmu_enabled = true;
>
> INIT_LIST_HEAD(&kvm->arch.tdp_mmu_roots);
> + spin_lock_init(&kvm->arch.tdp_mmu_pages_lock);
> INIT_LIST_HEAD(&kvm->arch.tdp_mmu_pages);
> }
>
> @@ -262,6 +263,58 @@ static void handle_changed_spte_dirty_log(struct kvm *kvm, int as_id, gfn_t gfn,
> }
> }
>
> +/**
> + * tdp_mmu_link_page - Add a new page to the list of pages used by the TDP MMU
> + *
> + * @kvm: kvm instance
> + * @sp: the new page
> + * @atomic: This operation is not running under the exclusive use of the MMU
> + * lock and the operation must be atomic with respect to ther threads
> + * that might be adding or removing pages.
> + * @account_nx: This page replaces a NX large page and should be marked for
> + * eventual reclaim.
> + */
> +static void tdp_mmu_link_page(struct kvm *kvm, struct kvm_mmu_page *sp,
> + bool atomic, bool account_nx)
> +{
> + if (atomic)
This is unnecessary, there is exactly one caller and it is always "atomic".
Assuming some of this code lives on (see below), I'd prefer a different name
than "atomic". Writing the SPTE is atomic (though even that is a bit of a lie,
e.g. tdp_mmu_zap_spte_atomic() is very much not atomic), but all the other
operations are the exact opposite of atomic.
Maybe change it from a bool to an enum with READ/WRITE_LOCKED or something?
> + spin_lock(&kvm->arch.tdp_mmu_pages_lock);
> + else
> + kvm_mmu_lock_assert_held_exclusive(kvm);
> +
> + list_add(&sp->link, &kvm->arch.tdp_mmu_pages);
> + if (account_nx)
> + account_huge_nx_page(kvm, sp);
> +
> + if (atomic)
> + spin_unlock(&kvm->arch.tdp_mmu_pages_lock);
> +}
> +
> +/**
> + * tdp_mmu_unlink_page - Remove page from the list of pages used by the TDP MMU
> + *
> + * @kvm: kvm instance
> + * @sp: the page to be removed
> + * @atomic: This operation is not running under the exclusive use of the MMU
> + * lock and the operation must be atomic with respect to ther threads
> + * that might be adding or removing pages.
> + */
> +static void tdp_mmu_unlink_page(struct kvm *kvm, struct kvm_mmu_page *sp,
> + bool atomic)
> +{
> + if (atomic)
Summarizing an off-list discussion with Ben:
This path isn't reachable in this series, which means all the RCU stuff is more
or less untestable. Only the page fault path modifies the MMU while hold a read
lock, and it can't zap non-leaf shadow pages (only zaps large SPTEs and installs
new SPs).
The intent is to convert other zap-happy paths to a read lock, notably
kvm_mmu_zap_collapsible_sptes() and kvm_recover_nx_lpages(). Ben will include
patches to convert at least one of those in the next version of this series so
that there is justification and coverage for the RCU-deferred freeing.
> + spin_lock(&kvm->arch.tdp_mmu_pages_lock);
> + else
> + kvm_mmu_lock_assert_held_exclusive(kvm);
> + list_del(&sp->link);
> + if (sp->lpage_disallowed)
> + unaccount_huge_nx_page(kvm, sp);
> +
> + if (atomic)
> + spin_unlock(&kvm->arch.tdp_mmu_pages_lock);
> +}
> +
> /**
> * handle_disconnected_tdp_mmu_page - handle a pt removed from the TDP structure
> *
