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)
> > > node->track_write = kvm_mmu_pte_write;
> > > node->track_flush_slot = kvm_mmu_invalidate_zap_pages_in_memslot;
> > > kvm_page_track_register_notifier(kvm, node);
> >
> > Can you add a patch to move this call to kvm_page_track_register_notifier()
> > into
> > mmu_enable_write_tracking(), and simultaneously add a WARN in the register
> > path
> > that page tracking is enabled?
> >
> > Oh, actually, a better idea. Add an inner
> > __kvm_page_track_register_notifier()
> > that is not exported and thus used only by KVM, invoke
> > mmu_enable_write_tracking()
> > from the exported kvm_page_track_register_notifier(), and then do the above.
> > That will require modifying KVMGT and KVM in a single patch, but that's ok.
> >
> > That will avoid any possibility of an external user failing to enabling
> > tracking
> > before registering its notifier, and also avoids bikeshedding over what to
> > do with
> > the one-line wrapper to enable tracking.
> >
>
> This is a good idea as well, especially looking at kvmgt and seeing that
> it registers the page track notifier, when the vGPU is opened.
>
> I'll do this in the next series.
>
> Thanks for the review!
After putting some thought into this, I am not 100% sure anymore I want to do
it this way.
Let me explain the current state of things:
For mmu:
- write tracking notifier is registered on VM initialization (that is pretty
much always),
and if it is called because write tracking was enabled due to some other reason
(currently only KVMGT), it checks the number of shadow mmu pages and if zero,
bails out.
- write tracking enabled when shadow root is allocated.
This can be kept as is by using the __kvm_page_track_register_notifier as you
suggested.
For KVMGT:
- both write tracking and notifier are enabled when an vgpu mdev device is
first opened.
That 'works' only because KVMGT doesn't allow to assign more that one mdev to
same VM,
thus a per VM notifier and the write tracking for that VM are enabled at the
same time
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.
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.
What do you think about this?
Best regards,
Maxim Levitsky
>
> Best regards,
> Maxim Levitsky
