Skip to contentSkip to conversation

pass info about hpets to seabios.

PATCH Superseded Gleb Natapov 3 participants 7 messages Jun 13, 2010 – Jun 14, 2010

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Gleb Natapov

author
#0 lore raw

PATCH 1/1 pass info about hpets to seabios.

Currently HPET ACPI table is created regardless of whether qemu actually created hpet device. This may confuse some guests that don't check that hpet is functional before using it. Solve this by passing info about hpets in qemu to seabios via fw config interface. Additional benefit is that seabios no longer uses hard coded hpet configuration. Proposed interface supports up to 256 hpets. This is the number defined by hpet spec.

Signed-off-by Gleb Natapov <[email protected]>
hw/hpet.c +5 -0
index 93fc399..f2a4514 100644
@@ -73,6 +73,8 @@ typedef struct HPETState {
     uint64_t hpet_counter;      /* main counter */
 } HPETState;
 
+struct hpet_fw_config hpet_cfg = {.valid = 1};
+
 static uint32_t hpet_in_legacy_mode(HPETState *s)
 {
     return s->config & HPET_CFG_LEGACY;

@@ -661,6 +663,9 @@ static void hpet_reset(DeviceState *d)
          */
         hpet_pit_enable();
     }
+    hpet_cfg.count = 1;
+    hpet_cfg.hpet.event_timer_block_id = (uint32_t)s->capability;
+    hpet_cfg.hpet.address = sysbus_from_qdev(d)->mmio[0].addr;
     count = 1;
 }
hw/hpet_emul.h +16 -0
index d7bc102..5cf5463 100644
@@ -53,4 +53,20 @@
 #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;
+} __attribute__ ((packed));
+
+struct hpet_fw_config
+{
+    uint8_t valid;
+    uint8_t count;
+    struct hpet_fw_entry hpet;
+} __attribute__ ((packed));
+
+extern struct hpet_fw_config hpet_cfg;
 #endif
hw/pc.c +3 -0
index 1491129..d14d657 100644
@@ -61,6 +61,7 @@
 #define FW_CFG_SMBIOS_ENTRIES (FW_CFG_ARCH_LOCAL + 1)
 #define FW_CFG_IRQ0_OVERRIDE (FW_CFG_ARCH_LOCAL + 2)
 #define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
+#define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
 
 #define E820_NR_ENTRIES		16
 

@@ -484,6 +485,8 @@ static void *bochs_bios_init(void)
     fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE, (uint8_t *)&e820_table,
                      sizeof(struct e820_table));
 
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, (uint8_t *)&hpet_cfg,
+                     sizeof(struct hpet_fw_config));
     /* 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.

--
Gleb.

#1 lore raw
Quoted 7 lines · Gleb Natapov

Currently HPET ACPI table is created regardless of whether qemu actually created hpet device. This may confuse some guests that don't check that hpet is functional before using it. Solve this by passing info about hpets in qemu to seabios via fw config interface. Additional benefit is that seabios no longer uses hard coded hpet configuration. Proposed interface supports up to 256 hpets. This is the number defined by hpet spec.

Nice, this lays the ground for adding hpets via -device.

(But I think I read there can only be 8 hpets with a total sum of 256 timers.)

Quoted 16 lines
Signed-off-by Gleb Natapov <[email protected]>
hw/hpet.c +2 -0
index 93fc399..f2a4514 100644
@@ -73,6 +73,8 @@ typedef struct HPETState {
     uint64_t hpet_counter;      /* main counter */
 } HPETState;
 
+struct hpet_fw_config hpet_cfg = {.valid = 1};
+
 static uint32_t hpet_in_legacy_mode(HPETState *s)
 {
     return s->config & HPET_CFG_LEGACY;

hw/hpet.c static void hpet_reset(DeviceState *d) · line 667

Quoted 5 lines
hw/hpet.c +2 -0
@@ -661,6 +663,9 @@ static void hpet_reset(DeviceState *d)
          */
         hpet_pit_enable();
     }
+    hpet_cfg.count = 1;
+    hpet_cfg.hpet.event_timer_block_id = (uint32_t)s->capability;

The number of timers, thus the content of capability can change on vmload. So you need to update hpet_cfg there as well.

And I think we can move the capability setup into init. But this is not directly related to this patch, would just avoid adding this hunk to hpet_reset.

Quoted 15 lines
hw/hpet.c +1 -0
+    hpet_cfg.hpet.address = sysbus_from_qdev(d)->mmio[0].addr;
     count = 1;
 }
hw/hpet_emul.h +3 -0
index d7bc102..5cf5463 100644
@@ -53,4 +53,20 @@
 #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;

hw/hpet_emul.h line 68

Quoted 10 lines
hw/hpet_emul.h +10 -0
+    uint64_t address;
+    uint16_t min_tick;
+    uint8_t page_prot;
+} __attribute__ ((packed));
+
+struct hpet_fw_config
+{
+    uint8_t valid;
+    uint8_t count;
+    struct hpet_fw_entry hpet;

Why not already struct hpet_fw_entry hpet[8]? Once the bios bits are merge, we can quickly remove the single hpet limitation on qemu side.

Quoted 26 lines
hw/hpet_emul.h +3 -0
+} __attribute__ ((packed));
+
+extern struct hpet_fw_config hpet_cfg;
 #endif
hw/pc.c +3 -1
index 1491129..d14d657 100644
@@ -61,6 +61,7 @@
 #define FW_CFG_SMBIOS_ENTRIES (FW_CFG_ARCH_LOCAL + 1)
 #define FW_CFG_IRQ0_OVERRIDE (FW_CFG_ARCH_LOCAL + 2)
 #define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
+#define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
 
 #define E820_NR_ENTRIES		16
 

@@ -484,6 +485,8 @@ static void *bochs_bios_init(void)
     fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE, (uint8_t *)&e820_table,
                      sizeof(struct e820_table));
 
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, (uint8_t *)&hpet_cfg,
+                     sizeof(struct hpet_fw_config));
     /* 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.
--
Quoted 3 lines

Gleb.

Jan

author
#2 lore raw
Quoted 9 lines · Jan Kiszka
Quoted 7 lines · Gleb Natapov

Currently HPET ACPI table is created regardless of whether qemu actually created hpet device. This may confuse some guests that don't check that hpet is functional before using it. Solve this by passing info about hpets in qemu to seabios via fw config interface. Additional benefit is that seabios no longer uses hard coded hpet configuration. Proposed interface supports up to 256 hpets. This is the number defined by hpet spec.

Quoted 5 lines

Nice, this lays the ground for adding hpets via -device.

(But I think I read there can only be 8 hpets with a total sum of 256 timers.)

Ah, correct. I thought to myself 256 hpets should be to much :)

Quoted 10 lines
Quoted 10 lines
Signed-off-by Gleb Natapov <[email protected]>
hw/hpet.c +0 -0
index 93fc399..f2a4514 100644
@@ -73,6 +73,8 @@ typedef struct HPETState {
     uint64_t hpet_counter;      /* main counter */
 } HPETState;
Quoted 15 lines

hw/hpet.c static void hpet_reset(DeviceState *d) · line 667

Quoted 11 lines
hw/hpet.c +4 -0
+struct hpet_fw_config hpet_cfg = {.valid = 1};
+
 static uint32_t hpet_in_legacy_mode(HPETState *s)
 {
     return s->config & HPET_CFG_LEGACY;

@@ -661,6 +663,9 @@ static void hpet_reset(DeviceState *d)
          */
         hpet_pit_enable();
     }
+    hpet_cfg.count = 1;
+    hpet_cfg.hpet.event_timer_block_id = (uint32_t)s->capability;

The number of timers, thus the content of capability can change on vmload. So you need to update hpet_cfg there as well.

How it can change? User is required to run the same command line on src and dst, no?

Quoted 3 lines

And I think we can move the capability setup into init. But this is not directly related to this patch, would just avoid adding this hunk to hpet_reset.

I actually did that initially and tried to init hpet_cfg there too, but then noticed that mmio[0].addr below is not initialized at init time yet.

Quoted 13 lines
Quoted 13 lines
hw/hpet.c +1 -0
+    hpet_cfg.hpet.address = sysbus_from_qdev(d)->mmio[0].addr;
     count = 1;
 }
hw/hpet_emul.h +0 -0
index d7bc102..5cf5463 100644
@@ -53,4 +53,20 @@
 #define HPET_TN_INT_ROUTE_CAP_SHIFT 32
 #define HPET_TN_CFG_BITS_READONLY_OR_RESERVED 0xffff80b1U
Quoted 17 lines

hw/hpet_emul.h line 68

Quoted 13 lines
hw/hpet_emul.h +13 -0
+struct hpet_fw_entry
+{
+    uint32_t event_timer_block_id;
+    uint64_t address;
+    uint16_t min_tick;
+    uint8_t page_prot;
+} __attribute__ ((packed));
+
+struct hpet_fw_config
+{
+    uint8_t valid;
+    uint8_t count;
+    struct hpet_fw_entry hpet;

Why not already struct hpet_fw_entry hpet[8]? Once the bios bits are merge, we can quickly remove the single hpet limitation on qemu side.

Number 256 somehow stuck in my head. 8 hpets is Ok to do from the start.

Quoted 20 lines
Quoted 20 lines
hw/hpet_emul.h +3 -0
+} __attribute__ ((packed));
+
+extern struct hpet_fw_config hpet_cfg;
 #endif
hw/pc.c +1 -0
index 1491129..d14d657 100644
@@ -61,6 +61,7 @@
 #define FW_CFG_SMBIOS_ENTRIES (FW_CFG_ARCH_LOCAL + 1)
 #define FW_CFG_IRQ0_OVERRIDE (FW_CFG_ARCH_LOCAL + 2)
 #define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
+#define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
 
 #define E820_NR_ENTRIES		16
 

@@ -484,6 +485,8 @@ static void *bochs_bios_init(void)
     fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE, (uint8_t *)&e820_table,
                      sizeof(struct e820_table));
Quoted 12 lines
Quoted 9 lines
hw/pc.c +2 -1
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, (uint8_t *)&hpet_cfg,
+                     sizeof(struct hpet_fw_config));
     /* 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.
--

Gleb.

Jan

--
Gleb.

#3 lore raw
Quoted 39 lines · Gleb Natapov
Quoted 8 lines · Jan Kiszka
Quoted 7 lines · Gleb Natapov

Currently HPET ACPI table is created regardless of whether qemu actually created hpet device. This may confuse some guests that don't check that hpet is functional before using it. Solve this by passing info about hpets in qemu to seabios via fw config interface. Additional benefit is that seabios no longer uses hard coded hpet configuration. Proposed interface supports up to 256 hpets. This is the number defined by hpet spec.

Quoted 5 lines

Nice, this lays the ground for adding hpets via -device.

(But I think I read there can only be 8 hpets with a total sum of 256 timers.)

Ah, correct. I thought to myself 256 hpets should be to much :)

Quoted 9 lines
Quoted 9 lines
Signed-off-by Gleb Natapov <[email protected]>
hw/hpet.c +0 -0
index 93fc399..f2a4514 100644
@@ -73,6 +73,8 @@ typedef struct HPETState {
     uint64_t hpet_counter;      /* main counter */
 } HPETState;
Quoted 14 lines

hw/hpet.c static void hpet_reset(DeviceState *d) · line 667

Quoted 11 lines
hw/hpet.c +4 -0
+struct hpet_fw_config hpet_cfg = {.valid = 1};
+
 static uint32_t hpet_in_legacy_mode(HPETState *s)
 {
     return s->config & HPET_CFG_LEGACY;

@@ -661,6 +663,9 @@ static void hpet_reset(DeviceState *d)
          */
         hpet_pit_enable();
     }
+    hpet_cfg.count = 1;
+    hpet_cfg.hpet.event_timer_block_id = (uint32_t)s->capability;

The number of timers, thus the content of capability can change on vmload. So you need to update hpet_cfg there as well.

Quoted 2 lines

How it can change? User is required to run the same command line on src and dst, no?

Generally, yes. But there is no technical need to match the hpet props so far, they are included in the vmstate (implicitly).

Quoted 6 lines
Quoted 4 lines

And I think we can move the capability setup into init. But this is not directly related to this patch, would just avoid adding this hunk to hpet_reset.

I actually did that initially and tried to init hpet_cfg there too, but then noticed that mmio[0].addr below is not initialized at init time yet.

Indeed.

You may try sysbus_init_mmio_cb instead for a callback to be invoked once the address was set. That handler could also be called on vmload.

Jan

author
#4 lore raw
Quoted 40 lines · Jan Kiszka
Quoted 39 lines · Gleb Natapov
Quoted 8 lines · Jan Kiszka
Quoted 7 lines · Gleb Natapov

Currently HPET ACPI table is created regardless of whether qemu actually created hpet device. This may confuse some guests that don't check that hpet is functional before using it. Solve this by passing info about hpets in qemu to seabios via fw config interface. Additional benefit is that seabios no longer uses hard coded hpet configuration. Proposed interface supports up to 256 hpets. This is the number defined by hpet spec.

Quoted 5 lines

Nice, this lays the ground for adding hpets via -device.

(But I think I read there can only be 8 hpets with a total sum of 256 timers.)

Ah, correct. I thought to myself 256 hpets should be to much :)

Quoted 23 lines
Quoted 15 lines
Signed-off-by Gleb Natapov <[email protected]>
hw/hpet.c +2 -0
index 93fc399..f2a4514 100644
@@ -73,6 +73,8 @@ typedef struct HPETState {
     uint64_t hpet_counter;      /* main counter */
 } HPETState;
 
+struct hpet_fw_config hpet_cfg = {.valid = 1};
+
 static uint32_t hpet_in_legacy_mode(HPETState *s)
 {
     return s->config & HPET_CFG_LEGACY;

hw/hpet.c static void hpet_reset(DeviceState *d) · line 667

Quoted 5 lines
hw/hpet.c +2 -0
@@ -661,6 +663,9 @@ static void hpet_reset(DeviceState *d)
          */
         hpet_pit_enable();
     }
+    hpet_cfg.count = 1;
+    hpet_cfg.hpet.event_timer_block_id = (uint32_t)s->capability;

The number of timers, thus the content of capability can change on vmload. So you need to update hpet_cfg there as well.

Quoted 6 lines
Quoted 2 lines

How it can change? User is required to run the same command line on src and dst, no?

Generally, yes. But there is no technical need to match the hpet props so far, they are included in the vmstate (implicitly).

Assumption that config at src and dst are identical is everywhere in the code and in the future we should be able to pass actual config as part of the migration protocol. Better fail migration if we can detect that configs are different then trying to fix some things, but not others.

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines

And I think we can move the capability setup into init. But this is not directly related to this patch, would just avoid adding this hunk to hpet_reset.

Quoted 8 lines
Quoted 2 lines

I actually did that initially and tried to init hpet_cfg there too, but then noticed that mmio[0].addr below is not initialized at init time yet.

Indeed.

You may try sysbus_init_mmio_cb instead for a callback to be invoked once the address was set. That handler could also be called on vmload.

sysbus_init_mmio_cb calls callback instead of cpu_register_physical_memory(). Looks like unneeded complication. Doing it at reset should be fine as far as I see.

--
Gleb.

#5 lore raw
In reply to Gleb Natapov
Quoted 8 lines
Quoted 3 lines

And I think we can move the capability setup into init. But this is not directly related to this patch, would just avoid adding this hunk to hpet_reset.

I actually did that initially and tried to init hpet_cfg there too, but then noticed that mmio[0].addr below is not initialized at init time yet.

Quoted 1 line
Quoted 1 line

+ hpet_cfg.hpet.address = sysbus_from_qdev(d)->mmio[0].addr;

This is one of the reasons that you're not supposed to be messing with DeviceState contents directly.

Paul

author
#6 lore raw
Quoted 4 lines · Paul Brook
Quoted 4 lines
Quoted 3 lines

And I think we can move the capability setup into init. But this is not directly related to this patch, would just avoid adding this hunk to hpet_reset.

Quoted 8 lines
Quoted 4 lines

I actually did that initially and tried to init hpet_cfg there too, but then noticed that mmio[0].addr below is not initialized at init time yet.

Quoted 1 line
Quoted 1 line

+ hpet_cfg.hpet.address = sysbus_from_qdev(d)->mmio[0].addr;

This is one of the reasons that you're not supposed to be messing with DeviceState contents directly.

Alternatives?

--
Gleb.