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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.