On Wed, Jan 8, 2025 at 12:27 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> KVM: for the scope.
>
> On Tue, Jan 07, 2025, Suleiman Souhlal wrote:
> > It returns the cumulative nanoseconds that the host has been suspended.
>
> Please write changelogs that are standalone. "It returns ..." is completely
> nonsensical without the context of the shortlog.
>
> > It is intended to be used for reporting host suspend time to the guest.
> >
> > Change-Id: I8f644c9fbdb2c48d2c99dd9efaa5c85a83a14c2a
>
> Drop gerrit's metadata before posting.
>
> > Signed-off-by: Suleiman Souhlal <suleiman@xxxxxxxxxx>
> > ---
> > include/linux/kvm_host.h | 2 ++
> > virt/kvm/kvm_main.c | 26 ++++++++++++++++++++++++++
> > 2 files changed, 28 insertions(+)
> >
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index 401439bb21e3e6..cf926168b30820 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -2553,4 +2553,6 @@ long kvm_arch_vcpu_pre_fault_memory(struct kvm_vcpu *vcpu,
> > struct kvm_pre_fault_memory *range);
> > #endif
> >
> > +u64 kvm_total_suspend_ns(void);
> > +
> > #endif
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index de2c11dae23163..d5ae237df76d0d 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -889,13 +889,39 @@ static int kvm_init_mmu_notifier(struct kvm *kvm)
> >
> > #endif /* CONFIG_KVM_GENERIC_MMU_NOTIFIER */
> >
> > +static u64 last_suspend;
> > +static u64 total_suspend_ns;
> > +
> > +u64 kvm_total_suspend_ns(void)
> > +{
> > + return total_suspend_ns;
> > +}
> > +
> > +
>
> Extra whitespace.
>
> > #ifdef CONFIG_HAVE_KVM_PM_NOTIFIER
> > +static int kvm_pm_notifier(struct kvm *kvm, unsigned long state)
> > +{
> > + switch (state) {
> > + case PM_HIBERNATION_PREPARE:
> > + case PM_SUSPEND_PREPARE:
> > + last_suspend = ktime_get_boottime_ns();
> > + case PM_POST_HIBERNATION:
> > + case PM_POST_SUSPEND:
> > + total_suspend_ns += ktime_get_boottime_ns() - last_suspend;
>
> This is broken. As should be quite clear from the function parameters, this is
> a per-VM notifier. While clobbering "last_suspend" is relatively benign,
> accumulating into "total_suspend_ns" for every VM will cause the "total" suspend
> time to be wildly inaccurate if there are multiple VMs.
Good catch. Thanks for spotting that.
>
> > + }
> > +
> > + return NOTIFY_DONE;
> > +}
> > +
> > static int kvm_pm_notifier_call(struct notifier_block *bl,
> > unsigned long state,
> > void *unused)
> > {
> > struct kvm *kvm = container_of(bl, struct kvm, pm_notifier);
> >
> > + if (kvm_pm_notifier(kvm, state) != NOTIFY_DONE)
> > + return NOTIFY_BAD;
>
> This is ridiculous on multiple fronts. There's zero reason for kvm_pm_notifier()
> to return a value, it always succeeds. I also see zero reason to put this code
> in common KVM. It's 100% an x86-only concept, and x86 is the only architecture
> that implements kvm_arch_pm_notifier(). So just shove the logic into x86's
> implementation.
The feedback I thought I got in v1 was that this could be useful for
other architectures too.
But given that the current series only implements it for x86, I guess
it's fair for it to be out of common KVM for now.
I will move it back into x86's implementation.
Thanks for the very quick reviews.
-- Suleiman
