Re: [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c

[Zhao Liu <zhao1.liu@intel.com>]

> This breaks if you disable HPET, which is why fw_cfg.c defines it.
> 
> You can do something like
> 
> diff --git a/include/hw/timer/hpet-fw-cfg.h b/include/hw/timer/hpet-fw-cfg.h
> new file mode 100644
> index 00000000000..234a49fc92e
> --- /dev/null
> +++ b/include/hw/timer/hpet-fw-cfg.h
> @@ -0,0 +1,16 @@
> +struct hpet_fw_entry
> +{
> +    uint32_t event_timer_block_id;
> +    uint64_t address;
> +    uint16_t min_tick;
> +    uint8_t page_prot;
> +} QEMU_PACKED;
> +
> +struct hpet_fw_config
> +{
> +    uint8_t count;
> +    struct hpet_fw_entry hpet[8];
> +} QEMU_PACKED;
> +
> +extern struct hpet_fw_config hpet_fw_cfg;
> +
> diff --git a/include/hw/timer/hpet.h b/include/hw/timer/hpet.h
> index d17a8d43199..6f7fcbc3c60 100644
> --- a/include/hw/timer/hpet.h
> +++ b/include/hw/timer/hpet.h
> @@ -60,26 +60,12 @@
>  #define HPET_TN_INT_ROUTE_CAP_SHIFT 32
>  #define HPET_TN_CFG_BITS_READONLY_OR_RESERVED 0xffff80b1U
> -struct hpet_fw_entry
> -{
> -    uint32_t event_timer_block_id;
> -    uint64_t address;
> -    uint16_t min_tick;
> -    uint8_t page_prot;
> -} QEMU_PACKED;
> -
> -struct hpet_fw_config
> -{
> -    uint8_t count;
> -    struct hpet_fw_entry hpet[8];
> -} QEMU_PACKED;
> -
> -extern struct hpet_fw_config hpet_cfg;
> -
>  #define TYPE_HPET "hpet"
>  #define HPET_INTCAP "hpet-intcap"
> +#include "hw/timer/hpet-fw-cfg.h"
> +
>  static inline bool hpet_find(void)
>  {
>      return object_resolve_path_type("", TYPE_HPET, NULL);
> diff --git a/rust/wrapper.h b/rust/wrapper.h
> index 285d0eb6ad0..82381e43472 100644
> --- a/rust/wrapper.h
> +++ b/rust/wrapper.h
> @@ -62,3 +62,4 @@ typedef enum memory_order {
>  #include "qapi/error.h"
>  #include "migration/vmstate.h"
>  #include "chardev/char-serial.h"
> +#include "hw/timer/hpet-fw-cfg.h"
> 
> 
> but you will have to use unsafe to access it since it's a "static mut".
> 

Unfortunately, this way doesn't work either, if we disable both
CONFIG_HPET and CONFIG_X_HPET_RUST.

This is because I integrates hpet_fw_cfg into hpet lib which is compiled
under CONFIG_X_HPET_RUST along with other HPET parts.

The place broken is when hpet_fw_cfg is written into machine's fw_cfg (in
hw/i386/fw_cfg.c).

I think we can just wrap such write like:

diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
index 162785019b7a..3635b83620da 100644
--- a/hw/i386/fw_cfg.c
+++ b/hw/i386/fw_cfg.c
@@ -147,7 +147,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_fw_cfg, sizeof(hpet_fw_cfg));
+#if defined(CONFIG_HPET) || defined(CONFIG_X_HPET_RUST)
+    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_fw_cfg, sizeof(hpet_fw_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.

---

The hpet_fw_cfg is written unconditionally since 40ac17cd, because it
concerns ACPI HPET is created unconditionally. But that fact has changed
and ACPI checks HPET device now (hw/i386/acpi-build.c):

static void acpi_get_misc_info(AcpiMiscInfo *info)
{
    info->has_hpet = hpet_find();
#ifdef CONFIG_TPM
    info->tpm_version = tpm_get_version(tpm_find());
#endif
}

I think this is a thorough enough solution and I can post a separate
patch.

Thanks,
Zhao