[PATCH v2 13/15] KVM: x86/tdp_mmu: Make mmu notifier callbacks to check kvm_process

Rick Edgecombe posted 15 patches 1 year, 6 months ago
There is a newer version of this series
[PATCH v2 13/15] KVM: x86/tdp_mmu: Make mmu notifier callbacks to check kvm_process
Posted by Rick Edgecombe 1 year, 6 months ago
From: Isaku Yamahata <isaku.yamahata@intel.com>

Teach the MMU notifier callbacks how to check kvm_gfn_range.process to
filter which KVM MMU root types to operate on.

The private GPAs are backed by guest memfd. Such memory is not subjected
to MMU notifier callbacks because it can't be mapped into the host user
address space. Now kvm_gfn_range conveys info about which root to operate
on. Enhance the callback to filter the root page table type.

The KVM MMU notifier comes down to two functions.
kvm_tdp_mmu_unmap_gfn_range() and kvm_tdp_mmu_handle_gfn().

For VM's without a private/shared split in the EPT, all operations
should target the normal(direct) root.

invalidate_range_start() comes into kvm_tdp_mmu_unmap_gfn_range().
invalidate_range_end() doesn't come into arch code.

Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
TDX MMU Prep v2:
 - Use newly added kvm_process_to_root_types()

TDX MMU Prep:
 - Remove warning (Rick)
 - Remove confusing mention of mapping flags (Chao)
 - Re-write coverletter

v19:
 - type: test_gfn() => test_young()

v18:
 - newly added
---
 arch/x86/kvm/mmu/tdp_mmu.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 4f00ae7da072..da6024b8295f 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -1351,12 +1351,14 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
 	return ret;
 }
 
+/* Used by mmu notifier via kvm_unmap_gfn_range() */
 bool kvm_tdp_mmu_unmap_gfn_range(struct kvm *kvm, struct kvm_gfn_range *range,
 				 bool flush)
 {
+	enum kvm_tdp_mmu_root_types types = kvm_process_to_root_types(kvm, range->process);
 	struct kvm_mmu_page *root;
 
-	__for_each_tdp_mmu_root_yield_safe(kvm, root, range->slot->as_id, KVM_ANY_ROOTS)
+	__for_each_tdp_mmu_root_yield_safe(kvm, root, range->slot->as_id, types)
 		flush = tdp_mmu_zap_leafs(kvm, root, range->start, range->end,
 					  range->may_block, flush);
 
@@ -1370,6 +1372,7 @@ static __always_inline bool kvm_tdp_mmu_handle_gfn(struct kvm *kvm,
 						   struct kvm_gfn_range *range,
 						   tdp_handler_t handler)
 {
+	enum kvm_tdp_mmu_root_types types = kvm_process_to_root_types(kvm, range->process);
 	struct kvm_mmu_page *root;
 	struct tdp_iter iter;
 	bool ret = false;
@@ -1378,7 +1381,7 @@ static __always_inline bool kvm_tdp_mmu_handle_gfn(struct kvm *kvm,
 	 * Don't support rescheduling, none of the MMU notifiers that funnel
 	 * into this helper allow blocking; it'd be dead, wasteful code.
 	 */
-	for_each_tdp_mmu_root(kvm, root, range->slot->as_id) {
+	__for_each_tdp_mmu_root(kvm, root, range->slot->as_id, types) {
 		rcu_read_lock();
 
 		tdp_root_for_each_leaf_pte(iter, kvm, root, range->start, range->end)
-- 
2.34.1
Re: [PATCH v2 13/15] KVM: x86/tdp_mmu: Make mmu notifier callbacks to check kvm_process
Posted by Paolo Bonzini 1 year, 6 months ago
Subject: propagate enum kvm_process to MMU notifier callbacks

But again, the naming... I don't like kvm_process - in an OS process
is a word with a clear meaning. Yes, that is a noun and this is a
verb, but then naming an enum with a verb is also awkward.

Perhaps kvm_gfn_range_filter and range->attr_filter? A bit wordy but very clear:

enum kvm_tdp_mmu_root_types types =
    kvm_gfn_range_filter_to_root_types(kvm, range->attr_filter)

I think I like it.

This patch also should be earlier in the series; please move it after
patch 9, i.e. right after kvm_process_to_root_types is introduced.

Paolo
Re: [PATCH v2 13/15] KVM: x86/tdp_mmu: Make mmu notifier callbacks to check kvm_process
Posted by Edgecombe, Rick P 1 year, 6 months ago
On Fri, 2024-06-07 at 10:56 +0200, Paolo Bonzini wrote:
> Subject: propagate enum kvm_process to MMU notifier callbacks
> 
> But again, the naming... I don't like kvm_process - in an OS process
> is a word with a clear meaning. Yes, that is a noun and this is a
> verb, but then naming an enum with a verb is also awkward.
> 
> Perhaps kvm_gfn_range_filter and range->attr_filter? A bit wordy but very
> clear:
> 
> enum kvm_tdp_mmu_root_types types =
>     kvm_gfn_range_filter_to_root_types(kvm, range->attr_filter)
> 
> I think I like it.

Agree 'process' sticks out. Somehow having attr_filter and args.attributes in
the same struct feels a bit wrong. Not that process was a lot better.

I guess attr_filter is more about alias ranges, and args.attribute is more about
conversion to various types of memory (private, shared and ideas of other types
I guess). But since today we only have private and shared, I wonder if there is
some way to combine them within struct kvm_gfn_range? I've not thought this all
the way through.

> 
> This patch also should be earlier in the series; please move it after
> patch 9, i.e. right after kvm_process_to_root_types is introduced.

Hmm, I thought I remembered having to move this to be later, but I don't see any
problems moving it earlier. Thanks for pointing it out.
Re: [PATCH v2 13/15] KVM: x86/tdp_mmu: Make mmu notifier callbacks to check kvm_process
Posted by Paolo Bonzini 1 year, 6 months ago
On Sat, Jun 8, 2024 at 12:12 AM Edgecombe, Rick P
<rick.p.edgecombe@intel.com> wrote:
>
> On Fri, 2024-06-07 at 10:56 +0200, Paolo Bonzini wrote:
> > Subject: propagate enum kvm_process to MMU notifier callbacks
> >
> > But again, the naming... I don't like kvm_process - in an OS process
> > is a word with a clear meaning. Yes, that is a noun and this is a
> > verb, but then naming an enum with a verb is also awkward.
> >
> > Perhaps kvm_gfn_range_filter and range->attr_filter? A bit wordy but very
> > clear:
> >
> > enum kvm_tdp_mmu_root_types types =
> >     kvm_gfn_range_filter_to_root_types(kvm, range->attr_filter)
> >
> > I think I like it.
>
> Agree 'process' sticks out. Somehow having attr_filter and args.attributes in
> the same struct feels a bit wrong. Not that process was a lot better.
>
> I guess attr_filter is more about alias ranges, and args.attribute is more about
> conversion to various types of memory (private, shared and ideas of other types
> I guess). But since today we only have private and shared, I wonder if there is
> some way to combine them within struct kvm_gfn_range? I've not thought this all
> the way through.

I think it's better that they stay separate. One is an argument
(args.attribute), the other is not, it should be clear enough.

Paolo
Re: [PATCH v2 13/15] KVM: x86/tdp_mmu: Make mmu notifier callbacks to check kvm_process
Posted by Edgecombe, Rick P 1 year, 6 months ago
On Sat, 2024-06-08 at 11:15 +0200, Paolo Bonzini wrote:
> > Agree 'process' sticks out. Somehow having attr_filter and args.attributes
> > in
> > the same struct feels a bit wrong. Not that process was a lot better.
> > 
> > I guess attr_filter is more about alias ranges, and args.attribute is more
> > about
> > conversion to various types of memory (private, shared and ideas of other
> > types
> > I guess). But since today we only have private and shared, I wonder if there
> > is
> > some way to combine them within struct kvm_gfn_range? I've not thought this
> > all
> > the way through.
> 
> I think it's better that they stay separate. One is an argument
> (args.attribute), the other is not, it should be clear enough.

Ok, yea. Looking at this more, it makes sense.