Re: [RFC PATCH v3 04/19] KVM: x86: mmu: allow to enable write tracking externally

[Maxim Levitsky <mlevitsk@redhat.com>]

On Mon, 2022-07-25 at 16:08 +0000, Sean Christopherson wrote:
> On Wed, Jul 20, 2022, Maxim Levitsky wrote:
> > On Sun, 2022-05-22 at 13:22 +0300, Maxim Levitsky wrote:
> > > On Thu, 2022-05-19 at 16:37 +0000, Sean Christopherson wrote:
> > > > On Wed, Apr 27, 2022, Maxim Levitsky wrote:
> > > > > @@ -5753,6 +5752,10 @@ int kvm_mmu_init_vm(struct kvm *kvm)
> > Now for nested AVIC, this is what I would like to do:
> >  
> > - just like mmu, I prefer to register the write tracking notifier, when the
> >   VM is created.
> > 
> > - just like mmu, write tracking should only be enabled when nested AVIC is
> >   actually used first time, so that write tracking is not always enabled when
> >   you just boot a VM with nested avic supported, since the VM might not use
> >   nested at all.
> >  
> > Thus I either need to use the __kvm_page_track_register_notifier too for AVIC
> > (and thus need to export it) or I need to have a boolean
> > (nested_avic_was_used_once) and register the write tracking notifier only
> > when false and do it not on VM creation but on first attempt to use nested
> > AVIC.
> >  
> > Do you think this is worth it? I mean there is some value of registering the
> > notifier only when needed (this way it is not called for nothing) but it does
> > complicate things a bit.
> 
> Compared to everything else that you're doing in the nested AVIC code, refcounting
> the shared kvm_page_track_notifier_node object is a trivial amount of complexity.
Makes sense.

> 
> And on that topic, do you have performance numbers to justify using a single
> shared node?  E.g. if every table instance has its own notifier, then no additional
> refcounting is needed. 

The thing is that KVM goes over the list of notifiers and calls them for every write from the emulator
in fact even just for mmio write, and when you enable write tracking on a page,
you just write protect the page and add a mark in the page track array, which is roughly 

'don't install spte, don't install mmio spte, but just emulate the page fault if it hits this page'

So adding more than a bare minimum to this list, seems just a bit wrong.


>  It's not obvious that a shared node will provide better
> performance, e.g. if there are only a handful of AVIC tables being shadowed, then
> a linear walk of all nodes is likely fast enough, and doesn't bring the risk of
> a write potentially being stalled due to having to acquire a VM-scoped mutex.

The thing is that if I register multiple notifiers, they all will be called anyway,
but yes I can use container_of, and discover which table the notifier belongs to,
instead of having a hash table where I lookup the GFN of the fault.

The above means practically that all the shadow physid tables will be in a linear
list of notifiers, so I could indeed avoid per vm mutex on the write tracking,
however for simplicity I probably will still need it because I do modify the page,
and having per physid table mutex complicates things.

Currently in my code the locking is very simple and somewhat dumb, but the performance
is very good because the code isn't executed often, most of the time the AVIC hardware
works alone without any VM exits.

Once the code is accepted upstream, it's one of the things that can be improved.


Note though that I still need a hash table and a mutex because on each VM entry,
the guest can use a different physid table, so I need to lookup it, and create it,
if not found, which would require read/write of the hash table and thus a mutex.



> 
> > I can also stash this boolean (like 'bool registered;') into the 'struct
> > kvm_page_track_notifier_node',  and thus allow the
> > kvm_page_track_register_notifier to be called more that once -  then I can
> > also get rid of __kvm_page_track_register_notifier. 
> 
> No, allowing redundant registration without proper refcounting leads to pain,
> e.g. X registers, Y registers, X unregisters, kaboom.
> 

True, but then what about adding a refcount to 'struct kvm_page_track_notifier_node'
instead of a boolean, and allowing redundant registration? 
Probably not worth it, in which case I am OK to add a refcount to my avic code.

Or maybe just scrap the whole thing and just leave registration and activation of the
write tracking as two separate things? Honestly now that looks like the most clean
solution.

Best regards,
	Maxim Levitsky