Re: [PATCH v7 3/3] hw/acpi: Add vmclock device

[Philippe Mathieu-Daudé]

Hi David,

On 16/1/25 14:59, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> The vmclock device addresses the problem of live migration with
> precision clocks. The tolerances of a hardware counter (e.g. TSC) are
> typically around ±50PPM. A guest will use NTP/PTP/PPS to discipline that
> counter against an external source of 'real' time, and track the precise
> frequency of the counter as it changes with environmental conditions.
>
> When a guest is live migrated, anything it knows about the frequency of
> the underlying counter becomes invalid. It may move from a host where
> the counter running at -50PPM of its nominal frequency, to a host where
> it runs at +50PPM. There will also be a step change in the value of the
> counter, as the correctness of its absolute value at migration is
> limited by the accuracy of the source and destination host's time
> synchronization.
>
> The device exposes a shared memory region to guests, which can be mapped
> all the way to userspace. In the first phase, this merely advertises a
> 'disruption_marker', which indicates that the guest should throw away any
> NTP synchronization it thinks it has, and start again.
>
> Because the region can be exposed all the way to userspace, applications
> can still use time from a fast vDSO 'system call', and check the
> disruption marker to be sure that their timestamp is indeed truthful.
>
> The structure also allows for the precise time, as known by the host, to
> be exposed directly to guests so that they don't have to wait for NTP to
> resync from scratch.
>
> The values and fields are based on the nascent virtio-rtc specification,
> and the intent is that a version (hopefully precisely this version) of
> this structure will be included as an optional part of that spec. In the
> meantime, a simple ACPI device along the lines of VMGENID is perfectly
> sufficient and is compatible with what's being shipped in certain
> commercial hypervisors.
>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Reviewed-by: Paul Durrant <paul@xen.org>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>   MAINTAINERS               |   5 ++
>   hw/acpi/Kconfig           |   5 ++
>   hw/acpi/meson.build       |   1 +
>   hw/acpi/vmclock.c         | 179 ++++++++++++++++++++++++++++++++++++++
>   hw/i386/Kconfig           |   1 +
>   hw/i386/acpi-build.c      |  10 ++-
>   include/hw/acpi/vmclock.h |  34 ++++++++
>   7 files changed, 234 insertions(+), 1 deletion(-)
>   create mode 100644 hw/acpi/vmclock.c
>   create mode 100644 include/hw/acpi/vmclock.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8b9d9a7cac..b51860aaed 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2414,6 +2414,11 @@ F: hw/audio/virtio-snd-pci.c
>   F: include/hw/audio/virtio-snd.h
>   F: docs/system/devices/virtio-snd.rst
>   
> +vmclock
> +M: David Woodhouse <dwmw2@infradead.org>
> +S: Supported
> +F: hw/acpi/vmclock.c
> +
>   nvme
>   M: Keith Busch <kbusch@kernel.org>
>   M: Klaus Jensen <its@irrelevant.dk>
> diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
> index e07d3204eb..1d4e9f0845 100644
> --- a/hw/acpi/Kconfig
> +++ b/hw/acpi/Kconfig
> @@ -60,6 +60,11 @@ config ACPI_VMGENID
>       default y
>       depends on PC
>   
> +config ACPI_VMCLOCK
> +    bool
> +    default y
> +    depends on PC

This doesn't look right (apparently the kernel side also build on ARM).

I'm only seeing e820_add_entry (I386) and ACPI API called. So:

    depends on I386 && ACPI

If later we want ARM support we'll have to rework the e820_add_entry()
call.

> diff --git a/hw/acpi/vmclock.c b/hw/acpi/vmclock.c
> new file mode 100644
> index 0000000000..7387e5c9ca
> --- /dev/null
> +++ b/hw/acpi/vmclock.c
> @@ -0,0 +1,179 @@
> +/*
> + * Virtual Machine Clock Device
> + *
> + * Copyright © 2024 Amazon.com, Inc. or its affiliates. All Rights Reserved.
> + *
> + * Authors: David Woodhouse <dwmw2@infradead.org>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu/module.h"
> +#include "hw/i386/e820_memory_layout.h"
> +#include "hw/acpi/acpi.h"
> +#include "hw/acpi/aml-build.h"
> +#include "hw/acpi/vmclock.h"
> +#include "hw/nvram/fw_cfg.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/qdev-properties-system.h"
> +#include "migration/vmstate.h"
> +#include "system/reset.h"
> +
> +#include "standard-headers/linux/vmclock-abi.h"
> +
> +void vmclock_build_acpi(VmclockState *vms, GArray *table_data,
> +                        BIOSLinker *linker, const char *oem_id)
> +{
> +    Aml *ssdt, *dev, *scope, *crs;
> +    AcpiTable table = { .sig = "SSDT", .rev = 1,
> +                        .oem_id = oem_id, .oem_table_id = "VMCLOCK" };
> +
> +    /* Put VMCLOCK into a separate SSDT table */
> +    acpi_table_begin(&table, table_data);
> +    ssdt = init_aml_allocator();
> +
> +    scope = aml_scope("\\_SB");
> +    dev = aml_device("VCLK");
> +    aml_append(dev, aml_name_decl("_HID", aml_string("AMZNC10C")));
> +    aml_append(dev, aml_name_decl("_CID", aml_string("VMCLOCK")));
> +    aml_append(dev, aml_name_decl("_DDN", aml_string("VMCLOCK")));
> +
> +    /* Simple status method */
> +    aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
> +
> +    crs = aml_resource_template();
> +    aml_append(crs, aml_qword_memory(AML_POS_DECODE,
> +                                     AML_MIN_FIXED, AML_MAX_FIXED,
> +                                     AML_CACHEABLE, AML_READ_ONLY,
> +                                     0xffffffffffffffffULL,
> +                                     vms->physaddr,
> +                                     vms->physaddr + VMCLOCK_SIZE - 1,
> +                                     0, VMCLOCK_SIZE));
> +    aml_append(dev, aml_name_decl("_CRS", crs));
> +    aml_append(scope, dev);
> +    aml_append(ssdt, scope);
> +
> +    g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
> +    acpi_table_end(linker, &table);
> +    free_aml_allocator();
> +}


> +static void vmclock_realize(DeviceState *dev, Error **errp)
> +{
> +    VmclockState *vms = VMCLOCK(dev);
> +
> +    /*
> +     * Given that this function is executing, there is at least one VMCLOCK
> +     * device. Check if there are several.
> +     */
> +    if (!find_vmclock_dev()) {
> +        error_setg(errp, "at most one %s device is permitted", TYPE_VMCLOCK);

Hmm.

> +        return;
> +    }
> +
> +    vms->physaddr = VMCLOCK_ADDR;
> +
> +    e820_add_entry(vms->physaddr, VMCLOCK_SIZE, E820_RESERVED);

I386 specific code, OK.

> +
> +    memory_region_init_ram(&vms->clk_page, OBJECT(dev), "vmclock_page",
> +                           VMCLOCK_SIZE, &error_abort);
> +    memory_region_set_enabled(&vms->clk_page, true);
> +    vms->clk = memory_region_get_ram_ptr(&vms->clk_page);
> +    memset(vms->clk, 0, VMCLOCK_SIZE);
> +
> +    vms->clk->magic = cpu_to_le32(VMCLOCK_MAGIC);
> +    vms->clk->size = cpu_to_le16(VMCLOCK_SIZE);
> +    vms->clk->version = cpu_to_le16(1);
> +
> +    /* These are all zero and thus default, but be explicit */
> +    vms->clk->clock_status = VMCLOCK_STATUS_UNKNOWN;
> +    vms->clk->counter_id = VMCLOCK_COUNTER_INVALID;
> +
> +    qemu_register_reset(vmclock_handle_reset, vms);

Isn't qemu_register_reset() legacy API?

> +
> +    vmclock_update_guest(vms);
> +}

> diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
> index 32818480d2..d34ce07b21 100644
> --- a/hw/i386/Kconfig
> +++ b/hw/i386/Kconfig
> @@ -43,6 +43,7 @@ config PC
>       select SERIAL_ISA
>       select ACPI_PCI
>       select ACPI_VMGENID
> +    select ACPI_VMCLOCK

IIUC this is optional (see [*] below) so:

        imply ACPI_VMCLOCK

>       select VIRTIO_PMEM_SUPPORTED
>       select VIRTIO_MEM_SUPPORTED
>       select HV_BALLOON_SUPPORTED
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 733b8f0851..d482f974df 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -43,6 +43,7 @@
>   #include "system/tpm.h"
>   #include "hw/acpi/tpm.h"
>   #include "hw/acpi/vmgenid.h"
> +#include "hw/acpi/vmclock.h"
>   #include "hw/acpi/erst.h"
>   #include "hw/acpi/piix4.h"
>   #include "system/tpm_backend.h"
> @@ -2432,7 +2433,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState 
> *machine)
>       uint8_t *u;
>       GArray *tables_blob = tables->table_data;
>       AcpiSlicOem slic_oem = { .id = NULL, .table_id = NULL };
> -    Object *vmgenid_dev;
> +    Object *vmgenid_dev, *vmclock_dev;
>       char *oem_id;
>       char *oem_table_id;
>   
> @@ -2505,6 +2506,13 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>                              tables->vmgenid, tables->linker, x86ms->oem_id);
>       }
>   
> +    vmclock_dev = find_vmclock_dev();
> +    if (vmclock_dev) {

[*]

> +        acpi_add_table(table_offsets, tables_blob);
> +        vmclock_build_acpi(VMCLOCK(vmclock_dev), tables_blob, tables->linker,
> +                           x86ms->oem_id);
> +    }

Regards,

Phil.