Re: [PATCH v4] hw/acpi: Add vmclock device

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Mon, 07 Oct 2024 14:53:54 +0100
David Woodhouse <dwmw2@infradead.org> 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>
Hi David,

A couple of drive by comments (as we all love those!)

Jonathan

> ---

> diff --git a/hw/acpi/vmclock-abi.h b/hw/acpi/vmclock-abi.h
> new file mode 100644
> index 0000000000..19cbf85efd
> --- /dev/null
> +++ b/hw/acpi/vmclock-abi.h
> @@ -0,0 +1,186 @@


> +#endif /*  __VMCLOCK_ABI_H__ */
> diff --git a/hw/acpi/vmclock.c b/hw/acpi/vmclock.c
> new file mode 100644
> index 0000000000..e7df047c33
> --- /dev/null
> +++ b/hw/acpi/vmclock.c

> +
> +void vmclock_build_acpi(VmclockState *vms, GArray *table_data,
> +                        BIOSLinker *linker, const char *oem_id)
> +{
> +    Aml *ssdt, *dev, *scope, *method, *addr, *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")));

Nice _HID :)

> +    aml_append(dev, aml_name_decl("_CID", aml_string("VMCLOCK")));
> +    aml_append(dev, aml_name_decl("_DDN", aml_string("VMCLOCK")));
> +
> +    /* Simple status method */
> +    method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> +    addr = aml_local(0);
> +    aml_append(method, aml_store(aml_int(0xf), addr));
> +    aml_append(method, aml_return(addr));
> +    aml_append(dev, method);

It's static so you should be able to do:
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 Property vmclock_device_properties[] = {
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void vmclock_device_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->vmsd = &vmstate_vmclock;
> +    dc->realize = vmclock_realize;
> +    device_class_set_props(dc, vmclock_device_properties);

Probably a silly question but why register no properties?
Is that any different to not registering them?
+ a quick look suggests not all class_init functions
set them.



> +    dc->hotpluggable = false;
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +}
> +
> +static const TypeInfo vmclock_device_info = {
> +    .name          = TYPE_VMCLOCK,
> +    .parent        = TYPE_DEVICE,
> +    .instance_size = sizeof(VmclockState),
> +    .class_init    = vmclock_device_class_init,
> +};
> +
> +static void vmclock_register_types(void)
> +{
> +    type_register_static(&vmclock_device_info);
> +}
> +
> +type_init(vmclock_register_types)