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

[Philippe Mathieu-Daudé]

On 16/1/25 16:32, David Woodhouse wrote:
> On Thu, 2025-01-16 at 16:15 +0100, Philippe Mathieu-Daudé wrote:
> > > --- 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 don't think it's strictly wrong; there are no circumstances in which
> PC is set not I386 && ACPI, or vice versa? I was just going from the
> existing setup for VMGENID, which I think could also theoretically
> exist on Arm too?

Unfortunately PC (and MALTA) are bad examples, beeing ones of the
oldest QEMU machines. Their code is spaghetti.

ACPI_VMGENID seems over-restricted. IIUC it should be:

    select ACPI
    select FW_CFG

The idea is to keep the smallest dependency, i.e. if someone wants
to build a binary with only microvm machine and use vmclock in it,
it shouldn't have to build the PC machines.

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

Actually the Kconfig should be:

    select ACPI
    depends on I386

> > If later we want ARM support we'll have to rework the e820_add_entry()
> > call.
>
> Sure, that certainly makes it easier to add Arm later, and I really do
> intend to do so. I've done it in my tree. Thanks.