| Message ID | 20221027200316.2221027-2-dmatlack@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86/mmu: Do not recover NX Huge Pages when dirty logging is enabled | expand |
On Thu, Oct 27, 2022, David Matlack wrote: > Add a new field to struct kvm that keeps track of the number of memslots > with dirty logging enabled. This will be used in a future commit to > cheaply check if any memslot is doing dirty logging. > > Signed-off-by: David Matlack <dmatlack@google.com> > --- > include/linux/kvm_host.h | 2 ++ > virt/kvm/kvm_main.c | 10 ++++++++++ Why put this in common code? I'm having a hard time coming up with a second use case since the count isn't stable, i.e. it can't be used for anything except scenarios like x86's NX huge page mitigation where a false negative/positive is benign. > 2 files changed, 12 insertions(+) > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 32f259fa5801..25ed8c1725ff 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -709,6 +709,8 @@ struct kvm { > struct kvm_memslots __memslots[KVM_ADDRESS_SPACE_NUM][2]; > /* The current active memslot set for each address space */ > struct kvm_memslots __rcu *memslots[KVM_ADDRESS_SPACE_NUM]; > + /* The number of memslots with dirty logging enabled. */ > + int nr_memslots_dirty_logging; I believe this can technically be a u16, as even with SMM KVM ensures the total number of memslots fits in a u16. A BUILD_BUG_ON() sanity check is probably a good idea regardless. > struct xarray vcpu_array; > > /* Used to wait for completion of MMU notifiers. */ > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index e30f1b4ecfa5..57e4406005cd 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -1641,6 +1641,9 @@ static void kvm_commit_memory_region(struct kvm *kvm, > const struct kvm_memory_slot *new, > enum kvm_mr_change change) > { > + int old_flags = old ? old->flags : 0; > + int new_flags = new ? new->flags : 0; Not that it really matters, but kvm_memory_slot.flags is a u32. > /* > * Update the total number of memslot pages before calling the arch > * hook so that architectures can consume the result directly. > @@ -1650,6 +1653,13 @@ static void kvm_commit_memory_region(struct kvm *kvm, > else if (change == KVM_MR_CREATE) > kvm->nr_memslot_pages += new->npages; > > + if ((old_flags ^ new_flags) & KVM_MEM_LOG_DIRTY_PAGES) { > + if (new_flags & KVM_MEM_LOG_DIRTY_PAGES) > + kvm->nr_memslots_dirty_logging++; > + else > + kvm->nr_memslots_dirty_logging--; A sanity check that KVM hasn't botched the count is probably a good idea. E.g. __kvm_set_memory_region() as a WARN_ON_ONCE() sanity check that KVM won't end up underflowing nr_memslot_pages. > + } > + > kvm_arch_commit_memory_region(kvm, old, new, change); > > switch (change) { > -- > 2.38.1.273.g43a17bfeac-goog >
On Thu, Oct 27, 2022 at 1:35 PM Sean Christopherson <seanjc@google.com> wrote: > > On Thu, Oct 27, 2022, David Matlack wrote: > > Add a new field to struct kvm that keeps track of the number of memslots > > with dirty logging enabled. This will be used in a future commit to > > cheaply check if any memslot is doing dirty logging. > > > > Signed-off-by: David Matlack <dmatlack@google.com> > > --- > > include/linux/kvm_host.h | 2 ++ > > virt/kvm/kvm_main.c | 10 ++++++++++ > > Why put this in common code? I'm having a hard time coming up with a second use > case since the count isn't stable, i.e. it can't be used for anything except > scenarios like x86's NX huge page mitigation where a false negative/positive is benign. I agree, but what is the downside of putting it in common code? The downside of putting it in architecture-specific code is if any other architecture needs it (or something similar) in the future they are unlikely to look through x86 code to see if it already exists. i.e. we're more likely to end up with duplicate code. And while the count is not stable outside of slots_lock, it could still theoretically be used under slots_lock to coordinate something that depends on dirty logging being enabled in any slot. In our internal kernel, for example, we use it to decide when to create/destroy the KVM dirty log worker threads (although I doubt that specific usecase will ever see the light of day upstream :). > > > 2 files changed, 12 insertions(+) > > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > > index 32f259fa5801..25ed8c1725ff 100644 > > --- a/include/linux/kvm_host.h > > +++ b/include/linux/kvm_host.h > > @@ -709,6 +709,8 @@ struct kvm { > > struct kvm_memslots __memslots[KVM_ADDRESS_SPACE_NUM][2]; > > /* The current active memslot set for each address space */ > > struct kvm_memslots __rcu *memslots[KVM_ADDRESS_SPACE_NUM]; > > + /* The number of memslots with dirty logging enabled. */ > > + int nr_memslots_dirty_logging; > > I believe this can technically be a u16, as even with SMM KVM ensures the total > number of memslots fits in a u16. A BUILD_BUG_ON() sanity check is probably a > good idea regardless. Will do. > > > struct xarray vcpu_array; > > > > /* Used to wait for completion of MMU notifiers. */ > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index e30f1b4ecfa5..57e4406005cd 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -1641,6 +1641,9 @@ static void kvm_commit_memory_region(struct kvm *kvm, > > const struct kvm_memory_slot *new, > > enum kvm_mr_change change) > > { > > + int old_flags = old ? old->flags : 0; > > + int new_flags = new ? new->flags : 0; > > Not that it really matters, but kvm_memory_slot.flags is a u32. Oops, thanks. > > > /* > > * Update the total number of memslot pages before calling the arch > > * hook so that architectures can consume the result directly. > > @@ -1650,6 +1653,13 @@ static void kvm_commit_memory_region(struct kvm *kvm, > > else if (change == KVM_MR_CREATE) > > kvm->nr_memslot_pages += new->npages; > > > > + if ((old_flags ^ new_flags) & KVM_MEM_LOG_DIRTY_PAGES) { > > + if (new_flags & KVM_MEM_LOG_DIRTY_PAGES) > > + kvm->nr_memslots_dirty_logging++; > > + else > > + kvm->nr_memslots_dirty_logging--; > > A sanity check that KVM hasn't botched the count is probably a good idea. E.g. > __kvm_set_memory_region() as a WARN_ON_ONCE() sanity check that KVM won't end up > underflowing nr_memslot_pages. Good idea, will do.
On Thu, Oct 27, 2022, David Matlack wrote: > On Thu, Oct 27, 2022 at 1:35 PM Sean Christopherson <seanjc@google.com> wrote: > > > > On Thu, Oct 27, 2022, David Matlack wrote: > > > Add a new field to struct kvm that keeps track of the number of memslots > > > with dirty logging enabled. This will be used in a future commit to > > > cheaply check if any memslot is doing dirty logging. > > > > > > Signed-off-by: David Matlack <dmatlack@google.com> > > > --- > > > include/linux/kvm_host.h | 2 ++ > > > virt/kvm/kvm_main.c | 10 ++++++++++ > > > > Why put this in common code? I'm having a hard time coming up with a second use > > case since the count isn't stable, i.e. it can't be used for anything except > > scenarios like x86's NX huge page mitigation where a false negative/positive is benign. > > I agree, but what is the downside of putting it in common code? The potential for misuse, e.g. outside of slots_lock. > The downside of putting it in architecture-specific code is if any other > architecture needs it (or something similar) in the future they are unlikely > to look through x86 code to see if it already exists. i.e. we're more likely > to end up with duplicate code. > > And while the count is not stable outside of slots_lock, it could > still theoretically be used under slots_lock to coordinate something > that depends on dirty logging being enabled in any slot. In our > internal kernel, for example, we use it to decide when to > create/destroy the KVM dirty log worker threads (although I doubt that > specific usecase will ever see the light of day upstream :). Yeah, I'm definitely not dead set against putting it in common code. I suspect I'm a little overly sensitive at the moment to x86 pushing a bunch of x86-centric logic into kvm_main.c and making life miserable for everyone. Been spending far too much time unwinding the mess that is kvm_init()...
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 32f259fa5801..25ed8c1725ff 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -709,6 +709,8 @@ struct kvm { struct kvm_memslots __memslots[KVM_ADDRESS_SPACE_NUM][2]; /* The current active memslot set for each address space */ struct kvm_memslots __rcu *memslots[KVM_ADDRESS_SPACE_NUM]; + /* The number of memslots with dirty logging enabled. */ + int nr_memslots_dirty_logging; struct xarray vcpu_array; /* Used to wait for completion of MMU notifiers. */ diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index e30f1b4ecfa5..57e4406005cd 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1641,6 +1641,9 @@ static void kvm_commit_memory_region(struct kvm *kvm, const struct kvm_memory_slot *new, enum kvm_mr_change change) { + int old_flags = old ? old->flags : 0; + int new_flags = new ? new->flags : 0; + /* * Update the total number of memslot pages before calling the arch * hook so that architectures can consume the result directly. @@ -1650,6 +1653,13 @@ static void kvm_commit_memory_region(struct kvm *kvm, else if (change == KVM_MR_CREATE) kvm->nr_memslot_pages += new->npages; + if ((old_flags ^ new_flags) & KVM_MEM_LOG_DIRTY_PAGES) { + if (new_flags & KVM_MEM_LOG_DIRTY_PAGES) + kvm->nr_memslots_dirty_logging++; + else + kvm->nr_memslots_dirty_logging--; + } + kvm_arch_commit_memory_region(kvm, old, new, change); switch (change) {
Add a new field to struct kvm that keeps track of the number of memslots with dirty logging enabled. This will be used in a future commit to cheaply check if any memslot is doing dirty logging. Signed-off-by: David Matlack <dmatlack@google.com> --- include/linux/kvm_host.h | 2 ++ virt/kvm/kvm_main.c | 10 ++++++++++ 2 files changed, 12 insertions(+)