Re: [PATCH v3 1/3] kvm: Introduce kvm_total_suspend_ns().

[Sean Christopherson <seanjc@google.com>]

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@google.com>
> ---
>  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.

> +	}
> +
> +	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.

> +
>  	return kvm_arch_pm_notifier(kvm, state);
>  }
>  
> -- 
> 2.47.1.613.gc27f4b7a9f-goog
>