From: David Woodhouse <dwmw@amazon.co.uk>

The vmclock device only has a reset method in order to plug its memory
region into the system memory. It was originally done this way in order
to defer the memory_region_add_subregion_overlap() from vmclock_realize(),
but that doesn't seem to be necessary (any longer?).

Still, allowing the platform code to do this is cleaner because it lets
the address be specified by the platform, easing the port to Arm and
other platforms in future. And the platform has to be involved anyway
because of the need to include the device in the ACPI tables (or DT).

So drop the reset method and provide a vmclock_mmio_map() function
instead, called from pc_machine_done().

Shift the ACPI table build into #ifdef CONFIG_ACPI_VMCLOCK too while
we're at it, since it looks like that wouldn't have built when vmclock
wasn't enabled.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/acpi/vmclock.c         | 18 ++++++------------
 hw/i386/acpi-build.c      | 16 ++++++++++------
 hw/i386/pc.c              | 10 ++++++++++
 include/hw/acpi/vmclock.h |  1 +
 4 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/hw/acpi/vmclock.c b/hw/acpi/vmclock.c
index 7387e5c9ca..36edfae0ed 100644
--- a/hw/acpi/vmclock.c
+++ b/hw/acpi/vmclock.c
@@ -20,7 +20,6 @@
 #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"
 
@@ -107,15 +106,14 @@ static const VMStateDescription vmstate_vmclock = {
     },
 };
 
-static void vmclock_handle_reset(void *opaque)
+void vmclock_mmio_map(Object *dev, hwaddr addr)
 {
-    VmclockState *vms = VMCLOCK(opaque);
+    VmclockState *vms = VMCLOCK(dev);
 
-    if (!memory_region_is_mapped(&vms->clk_page)) {
-        memory_region_add_subregion_overlap(get_system_memory(),
-                                            vms->physaddr,
-                                            &vms->clk_page, 0);
-    }
+    vms->physaddr = addr;
+    memory_region_add_subregion_overlap(get_system_memory(),
+                                        vms->physaddr,
+                                        &vms->clk_page, 0);
 }
 
 static void vmclock_realize(DeviceState *dev, Error **errp)
@@ -131,8 +129,6 @@ static void vmclock_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    vms->physaddr = VMCLOCK_ADDR;
-
     e820_add_entry(vms->physaddr, VMCLOCK_SIZE, E820_RESERVED);
 
     memory_region_init_ram(&vms->clk_page, OBJECT(dev), "vmclock_page",
@@ -149,8 +145,6 @@ static void vmclock_realize(DeviceState *dev, Error **errp)
     vms->clk->clock_status = VMCLOCK_STATUS_UNKNOWN;
     vms->clk->counter_id = VMCLOCK_COUNTER_INVALID;
 
-    qemu_register_reset(vmclock_handle_reset, vms);
-
     vmclock_update_guest(vms);
 }
 
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 53b7306b43..9db7b1f94e 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2446,7 +2446,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, *vmclock_dev;
+    Object *vmgenid_dev;
     char *oem_id;
     char *oem_table_id;
 
@@ -2519,12 +2519,16 @@ 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);
+#ifdef CONFIG_ACPI_VMCLOCK
+    {
+        Object *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);
+        }
     }
+#endif
 
     if (misc.has_hpet) {
         acpi_add_table(table_offsets, tables_blob);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index b46975c8a4..776c2c8a37 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -60,6 +60,7 @@
 #include "hw/i386/kvm/xen_gnttab.h"
 #include "hw/i386/kvm/xen_xenstore.h"
 #include "hw/mem/memory-device.h"
+#include "hw/acpi/vmclock.h"
 #include "e820_memory_layout.h"
 #include "trace.h"
 #include "sev.h"
@@ -635,6 +636,15 @@ void pc_machine_done(Notifier *notifier, void *data)
     pci_bus_add_fw_cfg_extra_pci_roots(x86ms->fw_cfg, pcms->pcibus,
                                        &error_abort);
 
+#ifdef CONFIG_ACPI_VMCLOCK
+    {
+        Object *vmclock = find_vmclock_dev();
+        if (vmclock) {
+            vmclock_mmio_map(vmclock, VMCLOCK_ADDR);
+        }
+    }
+#endif
+
     acpi_setup();
     if (x86ms->fw_cfg) {
         fw_cfg_build_smbios(pcms, x86ms->fw_cfg, pcms->smbios_entry_point_type);
diff --git a/include/hw/acpi/vmclock.h b/include/hw/acpi/vmclock.h
index 5605605812..97f8a30c0e 100644
--- a/include/hw/acpi/vmclock.h
+++ b/include/hw/acpi/vmclock.h
@@ -30,5 +30,6 @@ static inline Object *find_vmclock_dev(void)
 
 void vmclock_build_acpi(VmclockState *vms, GArray *table_data,
                         BIOSLinker *linker, const char *oem_id);
+void vmclock_mmio_map(Object *dev, hwaddr addr);
 
 #endif
-- 
2.48.1

On Fri, 2025-02-07 at 14:34 +0000, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> The vmclock device only has a reset method in order to plug its memory
> region into the system memory. It was originally done this way in order
> to defer the memory_region_add_subregion_overlap() from vmclock_realize(),
> but that doesn't seem to be necessary (any longer?).
> 
> Still, allowing the platform code to do this is cleaner because it lets
> the address be specified by the platform, easing the port to Arm and
> other platforms in future. And the platform has to be involved anyway
> because of the need to include the device in the ACPI tables (or DT).
> 
> So drop the reset method and provide a vmclock_mmio_map() function
> instead, called from pc_machine_done().
> 
> Shift the ACPI table build into #ifdef CONFIG_ACPI_VMCLOCK too while
> we're at it, since it looks like that wouldn't have built when vmclock
> wasn't enabled.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

Found this lurking in my working tree when I came to do something else.
Was it OK?

On Sat, Mar 29, 2025 at 1:43 PM David Woodhouse <dwmw2@infradead.org> wrote:
>
> On Fri, 2025-02-07 at 14:34 +0000, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> >
> > The vmclock device only has a reset method in order to plug its memory
> > region into the system memory. It was originally done this way in order
> > to defer the memory_region_add_subregion_overlap() from vmclock_realize(),
> > but that doesn't seem to be necessary (any longer?).
> >
> > Still, allowing the platform code to do this is cleaner because it lets
> > the address be specified by the platform, easing the port to Arm and
> > other platforms in future. And the platform has to be involved anyway
> > because of the need to include the device in the ACPI tables (or DT).
> >
> > So drop the reset method and provide a vmclock_mmio_map() function
> > instead, called from pc_machine_done().
> >
> > Shift the ACPI table build into #ifdef CONFIG_ACPI_VMCLOCK too while
> > we're at it, since it looks like that wouldn't have built when vmclock
> > wasn't enabled.
> >
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>
> Found this lurking in my working tree when I came to do something else.
> Was it OK?

I have tagged this for review next in my review queue.

>