On Wed, Feb 10, 2021 at 07:39:08PM +0900, Hikaru Nishida wrote:
> From: Hikaru Nishida <hikal...@chromium.org>
>
> This adds a sysfs interface /sys/kernel/boottime_adj to enable advancing
> CLOCK_BOOTTIME from the userspace without actual susupend/resume cycles.
>
> This gives a way to mitigate CLOCK_BOOTTIME divergence between guest
> and host on virtualized environments after suspend/resume cycles on
> the host.
>
> We observed an issue of a guest application that expects there is a gap
> between CLOCK_BOOTTIME and CLOCK_MONOTONIC after the device is suspended
> to detect whether the device went into suspend or not.
> Since the guest is paused instead of being actually suspended during the
> host's suspension, guest kernel doesn't advance CLOCK_BOOTTIME correctly
> and there is no way to correct that.
>
> To solve the problem, this change introduces a way to modify a gap
> between those clocks and align the timer behavior to host's one.
>
> Signed-off-by: Hikaru Nishida <hikal...@chromium.org>
> ---
>
> drivers/virt/Kconfig | 9 ++++++
> drivers/virt/Makefile | 1 +
> drivers/virt/boottime_adj.c | 57 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 67 insertions(+)
> create mode 100644 drivers/virt/boottime_adj.c
>
> diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
> index 80c5f9c16ec1..149b4e763e4d 100644
> --- a/drivers/virt/Kconfig
> +++ b/drivers/virt/Kconfig
> @@ -13,6 +13,15 @@ menuconfig VIRT_DRIVERS
>
> if VIRT_DRIVERS
>
> +config BOOTTIME_ADJUSTMENT
> + tristate "CLOCK_BOOTTIME adjustment sysfs interface"
> + help
> + The CLOCK_BOOTTIME adjustment sysfs interface driver
> + provides a sysfs interface ( /sys/kernel/boottime_adj )
> + to enable adjusting CLOCK_BOOTTIME from the userspace.
> +
> + If unsure, say N.
> +
> config FSL_HV_MANAGER
> tristate "Freescale hypervisor management driver"
> depends on FSL_SOC
> diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
> index f28425ce4b39..1bbb476ddba9 100644
> --- a/drivers/virt/Makefile
> +++ b/drivers/virt/Makefile
> @@ -3,6 +3,7 @@
> # Makefile for drivers that support virtualization
> #
>
> +obj-$(CONFIG_BOOTTIME_ADJUSTMENT) += boottime_adj.o
> obj-$(CONFIG_FSL_HV_MANAGER) += fsl_hypervisor.o
> obj-y += vboxguest/
>
> diff --git a/drivers/virt/boottime_adj.c b/drivers/virt/boottime_adj.c
> new file mode 100644
> index 000000000000..9cc717d8accc
> --- /dev/null
> +++ b/drivers/virt/boottime_adj.c
> @@ -0,0 +1,57 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
Do you really mean "or later"? I have to ask...
> +/*
> + * CLOCK_BOOTTIME Adjustment Interface Driver
No copyright, nice! Your company lawyers will be having a word with
you... :(
> + */
> +
> +#include <linux/kobject.h>
> +#include <linux/module.h>
> +#include <linux/timekeeping.h>
> +
> +static struct kobject *kobj_boottime_adj;
> +
> +/*
> + * Write to /sys/kernel/boottime_adj advances CLOCK_BOOTTIME by given delta.
> + */
> +static ssize_t boottime_adj_write(struct kobject *kobj,
> + struct kobj_attribute *attr, const char *buf,
> + size_t count)
> +{
> + int error;
> + struct timespec64 delta;
> +
> + if (sscanf(buf, "%lld %ld", &delta.tv_sec, &delta.tv_nsec) != 2)
> + return -EINVAL;
sysfs is "one value per file", but as you did not provide documentation
on what this does, I can't tell if you really are following this or not.
> +
> + error = timekeeping_adjust_boottime(&delta);
> + if (error)
> + return error;
> +
> + pr_info("%s: CLOCK_BOOTTIME has been advanced by %+lld seconds and %+ld
> nanoseconds\n",
> + __func__, delta.tv_sec, delta.tv_nsec);
If kernels are working normally, no messages are printed to the log. Do
not allow userspace to spam things.
> + return count;
> +}
> +
> +static struct kobj_attribute boottime_adj_attr =
> +__ATTR(boottime_adj, 0200, NULL, boottime_adj_write);
__ATTR_RO() please.
> +
> +static int __init boottime_adj_init(void)
> +{
> + int error;
> +
> + error = sysfs_create_file(kernel_kobj, &boottime_adj_attr.attr);
> + if (error) {
> + pr_warn("%s: failed to init\n", __func__);
why warn again?
> + return error;
> + }
> + return 0;
> +}
> +
> +static void __exit boottime_adj_cleanup(void)
> +{
> + kobject_put(kobj_boottime_adj);
You just called put on a kobject you never initialized?????
Are you sure this file has been tested?
odd...
greg k-h
