diff mbox series

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

Message ID 20240530210714.364118-14-rick.p.edgecombe@intel.com (mailing list archive)
State New, archived
Headers show
Series TDX MMU prep series part 1 | expand

Commit Message

Rick Edgecombe May 30, 2024, 9:07 p.m. UTC 
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(-)

Comments

Paolo Bonzini June 7, 2024, 8:56 a.m. UTC | #1 
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
Rick Edgecombe June 7, 2024, 10:12 p.m. UTC | #2 
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.
Paolo Bonzini June 8, 2024, 9:15 a.m. UTC | #3 
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
Rick Edgecombe June 10, 2024, 1:06 a.m. UTC | #4 
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.
diff mbox series

Patch

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)