[PATCH RESEND] i386: Only configure HPET firmware info when HPET is enabled

[PATCH RESEND] i386: Only configure HPET firmware info when HPET is enabled

[Zhao Liu]

At present, the hpet_cfg is written unconditionally since 40ac17cd56eb
("pass info about hpets to seabios.]"), because it concerns ACPI HPET is
created unconditionally.

But that fact has changed since 51124bbfd2ea ("i386: acpi: Don't build
HPET ACPI entry if HPET is disabled") and ACPI checks if HPET device
exists in (hw/i386/acpi-build.c).

Therefore, configure HPET firmware information if and only if HPET is
enabled.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Resend:
 * Resend the patch since it was missed on https://lore.kernel.org/qemu-devel/.
---
 hw/i386/fw_cfg.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
index 91bf1df0f2e4..d2cb08715a21 100644
--- a/hw/i386/fw_cfg.c
+++ b/hw/i386/fw_cfg.c
@@ -149,7 +149,14 @@ FWCfgState *fw_cfg_arch_create(MachineState *ms,
 #endif
     fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, 1);
 
-    fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_cfg, sizeof(hpet_cfg));
+#ifdef CONFIG_HPET
+    PCMachineState *pcms =
+        (PCMachineState *)object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE);
+    if (pcms && pcms->hpet_enabled) {
+        fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_cfg, sizeof(hpet_cfg));
+    }
+#endif
+
     /* allocate memory for the NUMA channel: one (64bit) word for the number
      * of nodes, one word for each VCPU->node and one word for each node to
      * hold the amount of memory.
-- 
2.34.1

Re: [PATCH RESEND] i386: Only configure HPET firmware info when HPET is enabled

[Michael S. Tsirkin]

On Tue, Jan 21, 2025 at 10:01:21PM +0800, Zhao Liu wrote:
> At present, the hpet_cfg is written unconditionally since 40ac17cd56eb
> ("pass info about hpets to seabios.]"), because it concerns ACPI HPET is
> created unconditionally.
> 
> But that fact has changed since 51124bbfd2ea ("i386: acpi: Don't build
> HPET ACPI entry if HPET is disabled") and ACPI checks if HPET device
> exists in (hw/i386/acpi-build.c).
> 
> Therefore, configure HPET firmware information if and only if HPET is
> enabled.
>

and what is the gain from this change? just a cleanup?

> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
> Resend:
>  * Resend the patch since it was missed on https://lore.kernel.org/qemu-devel/.




> ---
>  hw/i386/fw_cfg.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
> index 91bf1df0f2e4..d2cb08715a21 100644
> --- a/hw/i386/fw_cfg.c
> +++ b/hw/i386/fw_cfg.c
> @@ -149,7 +149,14 @@ FWCfgState *fw_cfg_arch_create(MachineState *ms,
>  #endif
>      fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, 1);
>  
> -    fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_cfg, sizeof(hpet_cfg));
> +#ifdef CONFIG_HPET
> +    PCMachineState *pcms =
> +        (PCMachineState *)object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE);
> +    if (pcms && pcms->hpet_enabled) {
> +        fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_cfg, sizeof(hpet_cfg));
> +    }
> +#endif
> +


Hmm. Wouldn't this break cross version migration? I suspect we need
a compat tweak if we do this. Might not be worth it ...


>      /* allocate memory for the NUMA channel: one (64bit) word for the number
>       * of nodes, one word for each VCPU->node and one word for each node to
>       * hold the amount of memory.
> -- 
> 2.34.1

Re: [PATCH RESEND] i386: Only configure HPET firmware info when HPET is enabled

[Zhao Liu]

Hi Michael,

Thanks for looking at here!

On Thu, Feb 20, 2025 at 10:16:56AM -0500, Michael S. Tsirkin wrote:
> Date: Thu, 20 Feb 2025 10:16:56 -0500
> From: "Michael S. Tsirkin" <mst@redhat.com>
> Subject: Re: [PATCH RESEND] i386: Only configure HPET firmware info when
>  HPET is enabled
> 
> On Tue, Jan 21, 2025 at 10:01:21PM +0800, Zhao Liu wrote:
> > At present, the hpet_cfg is written unconditionally since 40ac17cd56eb
> > ("pass info about hpets to seabios.]"), because it concerns ACPI HPET is
> > created unconditionally.
> > 
> > But that fact has changed since 51124bbfd2ea ("i386: acpi: Don't build
> > HPET ACPI entry if HPET is disabled") and ACPI checks if HPET device
> > exists in (hw/i386/acpi-build.c).
> > 
> > Therefore, configure HPET firmware information if and only if HPET is
> > enabled.
> >
> 
> and what is the gain from this change? just a cleanup?

This patch was trying to decouple hpet_cfw with i386 codes, which can
resolve the compilation issue when both the Rust and C versions of HPET
are both disabled.

But this patch is not needed since Paolo has figured out a simpler way
[*].

And thank you anyway!

[*]: https://lore.kernel.org/qemu-devel/CABgObfb6PhiKO9=iWne9AoXQ+Ek7FddoW8D0VcWvw3Qa3TW-9w@mail.gmail.com/

Regards,
Zhao