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