[PATCH 0/4] hw/timer/hpet: Make fw_cfg state private to HPET class

[PATCH 0/4] hw/timer/hpet: Make fw_cfg state private to HPET class

[Philippe Mathieu-Daudé]

No need to have an external hpet_cfg[] array accessed
outside of hpet.c. Move it in the class state.

Philippe Mathieu-Daudé (4):
  hw/timer/hpet: Introduce hpet_add_fw_cfg_bytes()
  hw/timer/hpet: Reduce hpet_cfg[] scope
  hw/timer/hpet: Have hpet_find() return an Object
  hw/timer/hpet: Hold fw_cfg state within HPET class

 include/hw/timer/hpet.h | 20 +++-------------
 hw/i386/fw_cfg.c        |  5 ++--
 hw/timer/hpet.c         | 52 ++++++++++++++++++++++++++++++++++-------
 3 files changed, 49 insertions(+), 28 deletions(-)

-- 
2.45.2

[PATCH 1/4] hw/timer/hpet: Introduce hpet_add_fw_cfg_bytes()

[Philippe Mathieu-Daudé]

Introduce hpet_add_fw_cfg_bytes() to restrict accesses
to hpet_cfg[] within hw/timer/hpet.c.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/timer/hpet.h |  2 ++
 hw/i386/fw_cfg.c        |  5 ++---
 hw/timer/hpet.c         | 10 ++++++++++
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/include/hw/timer/hpet.h b/include/hw/timer/hpet.h
index d17a8d43199..d361ab9e931 100644
--- a/include/hw/timer/hpet.h
+++ b/include/hw/timer/hpet.h
@@ -85,4 +85,6 @@ static inline bool hpet_find(void)
     return object_resolve_path_type("", TYPE_HPET, NULL);
 }
 
+bool hpet_add_fw_cfg_bytes(FWCfgState *fw_cfg, Error **errp);
+
 #endif
diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
index 0e4494627c2..16f806e76b1 100644
--- a/hw/i386/fw_cfg.c
+++ b/hw/i386/fw_cfg.c
@@ -26,8 +26,6 @@
 #include CONFIG_DEVICES
 #include "target/i386/cpu.h"
 
-struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
-
 const char *fw_cfg_arch_key_name(uint16_t key)
 {
     static const struct {
@@ -149,7 +147,8 @@ 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));
+    hpet_add_fw_cfg_bytes(fw_cfg, &error_fatal);
+
     /* 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.
diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 5399f1b2a3f..9ca1424565f 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -36,6 +36,7 @@
 #include "hw/rtc/mc146818rtc_regs.h"
 #include "migration/vmstate.h"
 #include "hw/timer/i8254.h"
+#include "hw/i386/fw_cfg.h"
 #include "exec/address-spaces.h"
 #include "qom/object.h"
 #include "trace.h"
@@ -86,6 +87,8 @@ struct HPETState {
     uint8_t  hpet_id;           /* instance id */
 };
 
+struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
+
 static uint32_t hpet_in_legacy_mode(HPETState *s)
 {
     return s->config & HPET_CFG_LEGACY;
@@ -777,3 +780,10 @@ static void hpet_register_types(void)
 }
 
 type_init(hpet_register_types)
+
+bool hpet_add_fw_cfg_bytes(FWCfgState *fw_cfg, Error **errp)
+{
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_cfg, sizeof(hpet_cfg));
+
+    return true;
+}
-- 
2.45.2

[PATCH 2/4] hw/timer/hpet: Reduce hpet_cfg[] scope

[Philippe Mathieu-Daudé]

Now than hpet_cfg[] is only accessed within hpet.c, make
it static. No need to expose the hpet_fw_entry/hpet_fw_config
structure declarations outside of it, so move them in the
source too.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/timer/hpet.h | 16 ----------------
 hw/timer/hpet.c         | 16 +++++++++++++++-
 2 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/include/hw/timer/hpet.h b/include/hw/timer/hpet.h
index d361ab9e931..2a95799a679 100644
--- a/include/hw/timer/hpet.h
+++ b/include/hw/timer/hpet.h
@@ -60,22 +60,6 @@
 #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"
diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 9ca1424565f..849cb3e669b 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -41,6 +41,20 @@
 #include "qom/object.h"
 #include "trace.h"
 
+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;
+
 #define HPET_MSI_SUPPORT        0
 
 OBJECT_DECLARE_SIMPLE_TYPE(HPETState, HPET)
@@ -87,7 +101,7 @@ struct HPETState {
     uint8_t  hpet_id;           /* instance id */
 };
 
-struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
+static struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
 
 static uint32_t hpet_in_legacy_mode(HPETState *s)
 {
-- 
2.45.2

[PATCH 3/4] hw/timer/hpet: Have hpet_find() return an Object

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/timer/hpet.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/timer/hpet.h b/include/hw/timer/hpet.h
index 2a95799a679..d11cdff0347 100644
--- a/include/hw/timer/hpet.h
+++ b/include/hw/timer/hpet.h
@@ -64,7 +64,7 @@
 
 #define HPET_INTCAP "hpet-intcap"
 
-static inline bool hpet_find(void)
+static inline Object *hpet_find(void)
 {
     return object_resolve_path_type("", TYPE_HPET, NULL);
 }
-- 
2.45.2

[PATCH 4/4] hw/timer/hpet: Hold fw_cfg state within HPET class

[Philippe Mathieu-Daudé]

We maintain one hpet_cfg[] state for all HPET instances.
Move it to a new HPET class.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/timer/hpet.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 849cb3e669b..c5aeac860b4 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -57,7 +57,7 @@ struct hpet_fw_config
 
 #define HPET_MSI_SUPPORT        0
 
-OBJECT_DECLARE_SIMPLE_TYPE(HPETState, HPET)
+OBJECT_DECLARE_TYPE(HPETState, HPETClass, HPET)
 
 struct HPETState;
 typedef struct HPETTimer {  /* timers */
@@ -101,7 +101,11 @@ struct HPETState {
     uint8_t  hpet_id;           /* instance id */
 };
 
-static struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
+struct HPETClass {
+    SysBusDeviceClass parent_class;
+
+    struct hpet_fw_config fw_cfg;
+};
 
 static uint32_t hpet_in_legacy_mode(HPETState *s)
 {
@@ -279,6 +283,7 @@ static bool hpet_validate_num_timers(void *opaque, int version_id)
 static int hpet_post_load(void *opaque, int version_id)
 {
     HPETState *s = opaque;
+    HPETClass *hc = HPET_GET_CLASS(s);
     int i;
 
     for (i = 0; i < s->num_timers; i++) {
@@ -295,7 +300,7 @@ static int hpet_post_load(void *opaque, int version_id)
     /* Push number of timers into capability returned via HPET_ID */
     s->capability &= ~HPET_ID_NUM_TIM_MASK;
     s->capability |= (s->num_timers - 1) << HPET_ID_NUM_TIM_SHIFT;
-    hpet_cfg.hpet[s->hpet_id].event_timer_block_id = (uint32_t)s->capability;
+    hc->fw_cfg.hpet[s->hpet_id].event_timer_block_id = (uint32_t)s->capability;
 
     /* Derive HPET_MSI_SUPPORT from the capability of the first timer. */
     s->flags &= ~(1 << HPET_MSI_SUPPORT);
@@ -660,6 +665,7 @@ static const MemoryRegionOps hpet_ram_ops = {
 static void hpet_reset(DeviceState *d)
 {
     HPETState *s = HPET(d);
+    HPETClass *hc = HPET_GET_CLASS(d);
     SysBusDevice *sbd = SYS_BUS_DEVICE(d);
     int i;
 
@@ -682,8 +688,8 @@ static void hpet_reset(DeviceState *d)
     s->hpet_counter = 0ULL;
     s->hpet_offset = 0ULL;
     s->config = 0ULL;
-    hpet_cfg.hpet[s->hpet_id].event_timer_block_id = (uint32_t)s->capability;
-    hpet_cfg.hpet[s->hpet_id].address = sbd->mmio[0].addr;
+    hc->fw_cfg.hpet[s->hpet_id].event_timer_block_id = (uint32_t)s->capability;
+    hc->fw_cfg.hpet[s->hpet_id].address = sbd->mmio[0].addr;
 
     /* to document that the RTC lowers its output on reset as well */
     s->rtc_irq_level = 0;
@@ -719,23 +725,24 @@ static void hpet_realize(DeviceState *dev, Error **errp)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     HPETState *s = HPET(dev);
+    HPETClass *hc = HPET_GET_CLASS(dev);
     int i;
     HPETTimer *timer;
 
     if (!s->intcap) {
         warn_report("Hpet's intcap not initialized");
     }
-    if (hpet_cfg.count == UINT8_MAX) {
+    if (hc->fw_cfg.count == UINT8_MAX) {
         /* first instance */
-        hpet_cfg.count = 0;
+        hc->fw_cfg.count = 0;
     }
 
-    if (hpet_cfg.count == 8) {
+    if (hc->fw_cfg.count == 8) {
         error_setg(errp, "Only 8 instances of HPET is allowed");
         return;
     }
 
-    s->hpet_id = hpet_cfg.count++;
+    s->hpet_id = hc->fw_cfg.count++;
 
     for (i = 0; i < HPET_NUM_IRQ_ROUTES; i++) {
         sysbus_init_irq(sbd, &s->irqs[i]);
@@ -773,11 +780,14 @@ static Property hpet_device_properties[] = {
 static void hpet_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    HPETClass *hc = HPET_CLASS(klass);
 
     dc->realize = hpet_realize;
     device_class_set_legacy_reset(dc, hpet_reset);
     dc->vmsd = &vmstate_hpet;
     device_class_set_props(dc, hpet_device_properties);
+
+    hc->fw_cfg.count = UINT8_MAX;
 }
 
 static const TypeInfo hpet_device_info = {
@@ -797,7 +807,9 @@ type_init(hpet_register_types)
 
 bool hpet_add_fw_cfg_bytes(FWCfgState *fw_cfg, Error **errp)
 {
-    fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_cfg, sizeof(hpet_cfg));
+    HPETClass *hc = HPET_GET_CLASS(hpet_find());
+
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hc->fw_cfg, sizeof(hc->fw_cfg));
 
     return true;
 }
-- 
2.45.2

Re: [PATCH 4/4] hw/timer/hpet: Hold fw_cfg state within HPET class

[Daniel P. Berrangé]

On Fri, Dec 06, 2024 at 08:11:24PM +0100, Philippe Mathieu-Daudé wrote:
> We maintain one hpet_cfg[] state for all HPET instances.
> Move it to a new HPET class.

It is a conceptually rather wierd having state stored in a
class rather than an instance.

I don't know what hpet_cfg is used for though ? How / when
does its contents change ? If it were something initialized
once, *before* any instances are created, I might be
convinced that it could live in a class. If it is at all
dynamic though, it could feel more like a separate class
providing a singleton instance.

> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/timer/hpet.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 849cb3e669b..c5aeac860b4 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -57,7 +57,7 @@ struct hpet_fw_config
>  
>  #define HPET_MSI_SUPPORT        0
>  
> -OBJECT_DECLARE_SIMPLE_TYPE(HPETState, HPET)
> +OBJECT_DECLARE_TYPE(HPETState, HPETClass, HPET)
>  
>  struct HPETState;
>  typedef struct HPETTimer {  /* timers */
> @@ -101,7 +101,11 @@ struct HPETState {
>      uint8_t  hpet_id;           /* instance id */
>  };
>  
> -static struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
> +struct HPETClass {
> +    SysBusDeviceClass parent_class;
> +
> +    struct hpet_fw_config fw_cfg;
> +};
>  
>  static uint32_t hpet_in_legacy_mode(HPETState *s)
>  {
> @@ -279,6 +283,7 @@ static bool hpet_validate_num_timers(void *opaque, int version_id)
>  static int hpet_post_load(void *opaque, int version_id)
>  {
>      HPETState *s = opaque;
> +    HPETClass *hc = HPET_GET_CLASS(s);
>      int i;
>  
>      for (i = 0; i < s->num_timers; i++) {
> @@ -295,7 +300,7 @@ static int hpet_post_load(void *opaque, int version_id)
>      /* Push number of timers into capability returned via HPET_ID */
>      s->capability &= ~HPET_ID_NUM_TIM_MASK;
>      s->capability |= (s->num_timers - 1) << HPET_ID_NUM_TIM_SHIFT;
> -    hpet_cfg.hpet[s->hpet_id].event_timer_block_id = (uint32_t)s->capability;
> +    hc->fw_cfg.hpet[s->hpet_id].event_timer_block_id = (uint32_t)s->capability;
>  
>      /* Derive HPET_MSI_SUPPORT from the capability of the first timer. */
>      s->flags &= ~(1 << HPET_MSI_SUPPORT);
> @@ -660,6 +665,7 @@ static const MemoryRegionOps hpet_ram_ops = {
>  static void hpet_reset(DeviceState *d)
>  {
>      HPETState *s = HPET(d);
> +    HPETClass *hc = HPET_GET_CLASS(d);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(d);
>      int i;
>  
> @@ -682,8 +688,8 @@ static void hpet_reset(DeviceState *d)
>      s->hpet_counter = 0ULL;
>      s->hpet_offset = 0ULL;
>      s->config = 0ULL;
> -    hpet_cfg.hpet[s->hpet_id].event_timer_block_id = (uint32_t)s->capability;
> -    hpet_cfg.hpet[s->hpet_id].address = sbd->mmio[0].addr;
> +    hc->fw_cfg.hpet[s->hpet_id].event_timer_block_id = (uint32_t)s->capability;
> +    hc->fw_cfg.hpet[s->hpet_id].address = sbd->mmio[0].addr;
>  
>      /* to document that the RTC lowers its output on reset as well */
>      s->rtc_irq_level = 0;
> @@ -719,23 +725,24 @@ static void hpet_realize(DeviceState *dev, Error **errp)
>  {
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>      HPETState *s = HPET(dev);
> +    HPETClass *hc = HPET_GET_CLASS(dev);
>      int i;
>      HPETTimer *timer;
>  
>      if (!s->intcap) {
>          warn_report("Hpet's intcap not initialized");
>      }
> -    if (hpet_cfg.count == UINT8_MAX) {
> +    if (hc->fw_cfg.count == UINT8_MAX) {
>          /* first instance */
> -        hpet_cfg.count = 0;
> +        hc->fw_cfg.count = 0;
>      }
>  
> -    if (hpet_cfg.count == 8) {
> +    if (hc->fw_cfg.count == 8) {
>          error_setg(errp, "Only 8 instances of HPET is allowed");
>          return;
>      }
>  
> -    s->hpet_id = hpet_cfg.count++;
> +    s->hpet_id = hc->fw_cfg.count++;
>  
>      for (i = 0; i < HPET_NUM_IRQ_ROUTES; i++) {
>          sysbus_init_irq(sbd, &s->irqs[i]);
> @@ -773,11 +780,14 @@ static Property hpet_device_properties[] = {
>  static void hpet_device_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> +    HPETClass *hc = HPET_CLASS(klass);
>  
>      dc->realize = hpet_realize;
>      device_class_set_legacy_reset(dc, hpet_reset);
>      dc->vmsd = &vmstate_hpet;
>      device_class_set_props(dc, hpet_device_properties);
> +
> +    hc->fw_cfg.count = UINT8_MAX;
>  }
>  
>  static const TypeInfo hpet_device_info = {
> @@ -797,7 +807,9 @@ type_init(hpet_register_types)
>  
>  bool hpet_add_fw_cfg_bytes(FWCfgState *fw_cfg, Error **errp)
>  {
> -    fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_cfg, sizeof(hpet_cfg));
> +    HPETClass *hc = HPET_GET_CLASS(hpet_find());
> +
> +    fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hc->fw_cfg, sizeof(hc->fw_cfg));
>  
>      return true;
>  }
> -- 
> 2.45.2
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/4] hw/timer/hpet: Make fw_cfg state private to HPET class

[Philippe Mathieu-Daudé]

On 6/12/24 20:11, Philippe Mathieu-Daudé wrote:
> No need to have an external hpet_cfg[] array accessed
> outside of hpet.c. Move it in the class state.
> 
> Philippe Mathieu-Daudé (4):
>    hw/timer/hpet: Introduce hpet_add_fw_cfg_bytes()
>    hw/timer/hpet: Reduce hpet_cfg[] scope
>    hw/timer/hpet: Have hpet_find() return an Object
>    hw/timer/hpet: Hold fw_cfg state within HPET class

ping