pit/hpet: Fix legacy mode switching

From: Jan Kiszka <[email protected]>

When the HPET enters legacy mode, the IRQ output of the PIT is suppressed and replaced by the HPET timer 0. But the current code to emulate this was broken in many ways. It reset the PIT state after re-enabling, it worked against a stale static PIT structure, and it did not properly saved/restored the IRQ output mask in the PIT vmstate.

This patch solves the PIT IRQ control in a different way. On x86, it both redirects the PIT IRQ to the HPET, just like the RTC. But it also keeps the control line from the HPET to the PIT. This allows to disable the PIT QEMU timer when it is not needed. The PIT's view on the control line state is now saved in the same format that qemu-kvm is already using.

Note that, in contrast to the suppressed RTC IRQ line, we do not need to save/restore the PIT line state in the HPET. As we trigger a PIT IRQ update via the control line, the line state is reconstructed on mode switch.

Signed-off-by Jan Kiszka <[email protected]>

---
 hw/alpha_dp264.c   |    2 +-
 hw/hpet.c          |   38 +++++++++++++++++--------------- hw/hpet_emul.h     |    3 ++
 hw/i8254.c         |   60 +++++++++++++++++++++++++++++---------------------- hw/mips_fulong2e.c |    2 +-
 hw/mips_jazz.c     |    2 +-
 hw/mips_malta.c    |    2 +-
 hw/mips_r4k.c      |    2 +-
 hw/pc.c            |   13 ++++++++--
 hw/pc.h            |   13 +----------
 hw/ppc_prep.c      |    2 +-

 11 files changed, 74 insertions(+), 65 deletions(-)

hw/alpha_dp264.c +1 -1

index fcc20e9..412ccf0 100644
@@ -70,7 +70,7 @@ static void clipper_init(ram_addr_t ram_size,
     pci_bus = typhoon_init(ram_size, &rtc_irq, cpus, clipper_pci_map_irq);
 
     rtc_init(1980, rtc_irq);
-    pit_init(0x40, 0);
+    pit_init(0x40, isa_get_irq(0));
     isa_create_simple("i8042");
 
     /* VGA setup.  Don't bother loading the bios.  */

hw/hpet.c +20 -18

index 1b64e6a..ace0b1d 100644
@@ -64,6 +64,7 @@ typedef struct HPETState {
     qemu_irq irqs[HPET_NUM_IRQ_ROUTES];
     uint32_t flags;
     uint8_t rtc_irq_level;
+    qemu_irq pit_enabled;
     uint8_t num_timers;
     HPETTimer timer[HPET_MAX_TIMERS];
 

@@ -572,12 +573,15 @@ static void hpet_ram_write(void *opaque, target_phys_addr_t addr,
                     hpet_del_timer(&s->timer[i]);
                 }
             }
-            /* i8254 and RTC are disabled when HPET is in legacy mode */
+            /* i8254 and RTC output pins are disabled
+             * when HPET is in legacy mode */
             if (activating_bit(old_val, new_val, HPET_CFG_LEGACY)) {
-                hpet_pit_disable();
+                qemu_set_irq(s->pit_enabled, 0);
+                qemu_irq_lower(s->irqs[0]);
                 qemu_irq_lower(s->irqs[RTC_ISA_IRQ]);
             } else if (deactivating_bit(old_val, new_val, HPET_CFG_LEGACY)) {
-                hpet_pit_enable();
+                qemu_irq_lower(s->irqs[0]);
+                qemu_set_irq(s->pit_enabled, 1);
                 qemu_set_irq(s->irqs[RTC_ISA_IRQ], s->rtc_irq_level);
             }
             break;

@@ -631,7 +635,6 @@ static void hpet_reset(DeviceState *d)
  {
     HPETState *s = FROM_SYSBUS(HPETState, sysbus_from_qdev(d));
     int i;
-    static int count = 0;
 
     for (i = 0; i < s->num_timers; i++) {
         HPETTimer *timer = &s->timer[i];

@@ -648,29 +651,27 @@ static void hpet_reset(DeviceState *d)
         timer->wrap_flag = 0;
     }
 
+    qemu_set_irq(s->pit_enabled, 1);
     s->hpet_counter = 0ULL;
     s->hpet_offset = 0ULL;
     s->config = 0ULL;
-    if (count > 0) {
-        /* we don't enable pit when hpet_reset is first called (by hpet_init)
-         * because hpet is taking over for pit here. On subsequent invocations,
-         * hpet_reset is called due to system reset. At this point control must
-         * be returned to pit until SW reenables hpet.
-         */
-        hpet_pit_enable();
-    }
     hpet_cfg.hpet[s->hpet_id].event_timer_block_id = (uint32_t)s->capability;
     hpet_cfg.hpet[s->hpet_id].address = sysbus_from_qdev(d)->mmio[0].addr;
-    count = 1;
  }
 
-static void hpet_handle_rtc_irq(void *opaque, int n, int level)
+static void hpet_handle_legacy_irq(void *opaque, int n, int level)
  {
     HPETState *s = FROM_SYSBUS(HPETState, opaque);
 
-    s->rtc_irq_level = level;
-    if (!hpet_in_legacy_mode(s)) {
-        qemu_set_irq(s->irqs[RTC_ISA_IRQ], level);
+    if (n == HPET_LEGACY_PIT_INT) {
+        if (!hpet_in_legacy_mode(s)) {
+            qemu_set_irq(s->irqs[0], level);
+        }
+    } else {
+        s->rtc_irq_level = level;
+        if (!hpet_in_legacy_mode(s)) {
+            qemu_set_irq(s->irqs[RTC_ISA_IRQ], level);
+        }
     }
  }
 

@@ -713,7 +714,8 @@ static int hpet_init(SysBusDevice *dev)
     s->capability |= (s->num_timers - 1) << HPET_ID_NUM_TIM_SHIFT;
     s->capability |= ((HPET_CLK_PERIOD) << 32);
 
-    qdev_init_gpio_in(&dev->qdev, hpet_handle_rtc_irq, 1);
+    qdev_init_gpio_in(&dev->qdev, hpet_handle_legacy_irq, 2);
+    qdev_init_gpio_out(&dev->qdev, &s->pit_enabled, 1);
 
     /* HPET Area */
     memory_region_init_io(&s->iomem, &hpet_ram_ops, s, "hpet", 0x400);

hw/hpet_emul.h +3 -0

index 6128702..757f79f 100644
@@ -22,6 +22,9 @@
 
  #define HPET_NUM_IRQ_ROUTES     32
 
+#define HPET_LEGACY_PIT_INT     0
+#define HPET_LEGACY_RTC_INT     1
+
  #define HPET_CFG_ENABLE 0x001
  #define HPET_CFG_LEGACY 0x002
 

hw/i8254.c +20 -25

index 12571ef..b3d9624 100644
@@ -51,18 +51,16 @@ typedef struct PITChannelState {
     int64_t next_transition_time;
     QEMUTimer *irq_timer;
     qemu_irq irq;
+    uint32_t irq_disabled;
  } PITChannelState;
 
  typedef struct PITState {
     ISADevice dev;
     MemoryRegion ioports;
-    uint32_t irq;
     uint32_t iobase;
     PITChannelState channels[3];
  } PITState;
 
-static PITState pit_state;
-
  static void pit_irq_timer_update(PITChannelState *s, int64_t current_time);
 
  static int pit_get_count(PITChannelState *s)

@@ -378,8 +376,9 @@ static void pit_irq_timer_update(PITChannelState *s, int64_t current_time)
     int64_t expire_time;
     int irq_level;
 
-    if (!s->irq_timer)
+    if (!s->irq_timer || s->irq_disabled) {
         return;
+    }
     expire_time = pit_get_next_transition_time(s, current_time);
     irq_level = pit_get_out1(s, current_time);
     qemu_set_irq(s->irq, irq_level);

@@ -450,6 +449,7 @@ static int pit_load_old(QEMUFile *f, void *opaque, int version_id)
         qemu_get_8s(f, &s->bcd);
         qemu_get_8s(f, &s->gate);
         s->count_load_time=qemu_get_be64(f);
+        s->irq_disabled = 0;
         if (s->irq_timer) {
             s->next_transition_time=qemu_get_be64(f);
             qemu_get_timer(f, s->irq_timer);

@@ -460,11 +460,12 @@ static int pit_load_old(QEMUFile *f, void *opaque, int version_id)
 
  static const VMStateDescription vmstate_pit = {
     .name = "i8254",
-    .version_id = 2,
+    .version_id = 3,
     .minimum_version_id = 2,
     .minimum_version_id_old = 1,
     .load_state_old = pit_load_old,
     .fields      = (VMStateField []) {
+        VMSTATE_UINT32_V(channels[0].irq_disabled, PITState, 3),
         VMSTATE_STRUCT_ARRAY(channels, PITState, 3, 2, vmstate_pit_channel, PITChannelState),
         VMSTATE_TIMER(channels[0].irq_timer, PITState),
         VMSTATE_END_OF_LIST()

@@ -485,26 +486,20 @@ static void pit_reset(DeviceState *dev)
     }
  }
 
-/* When HPET is operating in legacy mode, i8254 timer0 is disabled */
-void hpet_pit_disable(void) {
-    PITChannelState *s;
-    s = &pit_state.channels[0];
-    if (s->irq_timer)
-        qemu_del_timer(s->irq_timer);
-}
-
-/* When HPET is reset or leaving legacy mode, it must reenable i8254
- * timer 0
- */
-
-void hpet_pit_enable(void)
+/* When HPET is operating in legacy mode, suppress the ignored timer IRQ,
+ * reenable it when legacy mode is left again. */
+static void pit_irq_control(void *opaque, int n, int enable)
  {
-    PITState *pit = &pit_state;
-    PITChannelState *s;
-    s = &pit->channels[0];
-    s->mode = 3;
-    s->gate = 1;
-    pit_load_count(s, 0);
+    PITState *pit = opaque;
+    PITChannelState *s = &pit->channels[0];
+
+    if (enable) {
+        s->irq_disabled = 0;
+        pit_irq_timer_update(s, qemu_get_clock_ns(vm_clock));
+    } else {
+        s->irq_disabled = 1;
+        qemu_del_timer(s->irq_timer);
+    }
  }
 
  static const MemoryRegionPortio pit_portio[] = {

@@ -525,16 +520,30 @@ static int pit_initfn(ISADevice *dev)
     s = &pit->channels[0];
     /* the timer 0 is connected to an IRQ */
     s->irq_timer = qemu_new_timer_ns(vm_clock, pit_irq_timer, s);
-    s->irq = isa_get_irq(pit->irq);
+    qdev_init_gpio_out(&dev->qdev, &s->irq, 1);
 
     memory_region_init_io(&pit->ioports, &pit_ioport_ops, pit, "pit", 4);

hw/i8254.c +11 -1

+{
+    ISADevice *dev;
+
+    dev = isa_create("isa-pit");
+    qdev_prop_set_uint32(&dev->qdev, "iobase", base);
+    qdev_init_nofail(&dev->qdev);
+    qdev_connect_gpio_out(&dev->qdev, 0, irq);
+
+    return dev;
+}
+
  static ISADeviceInfo pit_info = {
     .qdev.name     = "isa-pit",
     .qdev.size     = sizeof(PITState),

@@ -543,7 +552,6 @@ static ISADeviceInfo pit_info = {
     .qdev.no_user  = 1,
     .init          = pit_initfn,
     .qdev.props = (Property[]) {
-        DEFINE_PROP_UINT32("irq", PITState, irq,  -1),
         DEFINE_PROP_HEX32("iobase", PITState, iobase,  -1),
         DEFINE_PROP_END_OF_LIST(),
     },

hw/mips_fulong2e.c +1 -1

index 04921c1..8a9c006 100644
@@ -358,7 +358,7 @@ static void mips_fulong2e_init(ram_addr_t ram_size, const char *boot_device,
     smbus_eeprom_init(smbus, 1, eeprom_spd, sizeof(eeprom_spd));
 
     /* init other devices */
-    pit = pit_init(0x40, 0);
+    pit = pit_init(0x40, isa_get_irq(0));
     cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1);
     DMA_init(0, cpu_exit_irq);
 

hw/mips_jazz.c +1 -1

index 358de59..a076e1d 100644
@@ -188,7 +188,7 @@ static void mips_jazz_init(MemoryRegion *address_space,
     isa_bus_irqs(i8259);
     cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1);
     DMA_init(0, cpu_exit_irq);
-    pit = pit_init(0x40, 0);
+    pit = pit_init(0x40, isa_get_irq(0));
     pcspk_init(pit);
 
     /* ISA IO space at 0x90000000 */

hw/mips_malta.c +1 -1

index bb49749..d09a48a 100644
@@ -953,7 +953,7 @@ void mips_malta_init (ram_addr_t ram_size,
                           NULL, NULL, 0);
     /* TODO: Populate SPD eeprom data.  */
     smbus_eeprom_init(smbus, 8, NULL, 0);
-    pit = pit_init(0x40, 0);
+    pit = pit_init(0x40, isa_get_irq(0));
     cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1);
     DMA_init(0, cpu_exit_irq);
 

hw/mips_r4k.c +1 -1

index d0564d4..9fef3ba 100644
@@ -266,7 +266,7 @@ void mips_r4k_init (ram_addr_t ram_size,
     isa_mmio_init(0x14000000, 0x00010000);
     isa_mem_base = 0x10000000;
 
-    pit = pit_init(0x40, 0);
+    pit = pit_init(0x40, isa_get_irq(0));
 
     for(i = 0; i < MAX_SERIAL_PORTS; i++) {
         if (serial_hds[i]) {

hw/pc.c +10 -3

index 33778fe..dcf87af 100644
@@ -1119,6 +1119,8 @@ void pc_basic_device_init(qemu_irq *gsi,
  {
     int i;
     DriveInfo *fd[MAX_FD];
+    DeviceState *hpet = NULL;
+    qemu_irq pit_irq = isa_get_irq(0);
     qemu_irq rtc_irq = NULL;
     qemu_irq *a20_line;
     ISADevice *i8042, *port92, *vmmouse, *pit;

@@ -1129,20 +1131,25 @@ void pc_basic_device_init(qemu_irq *gsi,
     register_ioport_write(0xf0, 1, 1, ioportF0_write, NULL);
 
     if (!no_hpet) {
-        DeviceState *hpet = sysbus_try_create_simple("hpet", HPET_BASE, NULL);
+        hpet = sysbus_try_create_simple("hpet", HPET_BASE, NULL);
 
         if (hpet) {
             for (i = 0; i < GSI_NUM_PINS; i++) {
                 sysbus_connect_irq(sysbus_from_qdev(hpet), i, gsi[i]);
             }
-            rtc_irq = qdev_get_gpio_in(hpet, 0);
+            pit_irq = qdev_get_gpio_in(hpet, HPET_LEGACY_PIT_INT);
+            rtc_irq = qdev_get_gpio_in(hpet, HPET_LEGACY_RTC_INT);
         }
     }
     *rtc_state = rtc_init(2000, rtc_irq);
 
     qemu_register_boot_set(pc_boot_set, *rtc_state);
 
-    pit = pit_init(0x40, 0);
+    pit = pit_init(0x40, pit_irq);
+    if (hpet) {
+        /* connect PIT to output control line of the HPET */
+        qdev_connect_gpio_out(hpet, 0, qdev_get_gpio_in(&pit->qdev, 0));
+    }
     pcspk_init(pit);
 
     for(i = 0; i < MAX_SERIAL_PORTS; i++) {

hw/pc.h +1 -12

index b7b7e40..20ee543 100644
@@ -84,18 +84,7 @@ void gsi_handler(void *opaque, int n, int level);
 
  #define PIT_FREQ 1193182
 
-static inline ISADevice *pit_init(int base, int irq)
-{
-    ISADevice *dev;
-
-    dev = isa_create("isa-pit");
-    qdev_prop_set_uint32(&dev->qdev, "iobase", base);
-    qdev_prop_set_uint32(&dev->qdev, "irq", irq);
-    qdev_init_nofail(&dev->qdev);
-
-    return dev;
-}
-
+ISADevice *pit_init(int base, qemu_irq irq);
  void pit_set_gate(ISADevice *dev, int channel, int val);
  int pit_get_gate(ISADevice *dev, int channel);
  int pit_get_initial_count(ISADevice *dev, int channel);

hw/ppc_prep.c +1 -2

index f22d5b9..84e2028 100644
@@ -641,7 +641,7 @@ static void ppc_prep_init (ram_addr_t ram_size,
     /* init basic PC hardware */
     pci_vga_init(pci_bus);
     //    openpic = openpic_init(0x00000000, 0xF0000000, 1);
-    //    pit = pit_init(0x40, 0);
+    //    pit = pit_init(0x40, isa_get_irq(0));
     rtc_init(2000, NULL);
 
     if (serial_hds[0])
--

1.7.3.4

Quoted 2 lines

+ISADevice *pit_init(int base, qemu_irq irq)

Please retain this function in pc.h, or even better, introduce i8254.h.

hw/i8254.c static int pit_initfn(ISADevice *dev) · line 546

Quoted 12 lines

hw/i8254.c +11 -0

+{
+    ISADevice *dev;
+
+    dev = isa_create("isa-pit");
+    qdev_prop_set_uint32(&dev->qdev, "iobase", base);
+    qdev_init_nofail(&dev->qdev);
+    qdev_connect_gpio_out(&dev->qdev, 0, irq);
+
+    return dev;
+}
+

Quoted 4 lines · Blue Swirl

Quoted 2 lines

+ISADevice *pit_init(int base, qemu_irq irq)

Please retain this function in pc.h, or even better, introduce i8254.h.

No concerns about i8254.h, but this function does not qualify for static inline.

Quoted 12 lines

hw/i8254.c static int pit_initfn(ISADevice *dev) · line 546

Quoted 12 lines

hw/i8254.c +11 -0

+{
+    ISADevice *dev;
+
+    dev = isa_create("isa-pit");
+    qdev_prop_set_uint32(&dev->qdev, "iobase", base);
+    qdev_init_nofail(&dev->qdev);
+    qdev_connect_gpio_out(&dev->qdev, 0, irq);
+
+    return dev;
+}
+

Jan

Quoted 4 lines · Jan Kiszka

Quoted 3 lines · Blue Swirl

Quoted 2 lines

+ISADevice *pit_init(int base, qemu_irq irq)

Quoted 4 lines

Quoted 1 line

Please retain this function in pc.h, or even better, introduce i8254.h.

No concerns about i8254.h, but this function does not qualify for static inline.

The function is static inline in a header file not for performance reasons, but to keep the instantiation separate from device internals.

Quoted 10 lines · Blue Swirl

Quoted 8 lines · Jan Kiszka

Quoted 4 lines · Blue Swirl

Quoted 2 lines

+ISADevice *pit_init(int base, qemu_irq irq)

Please retain this function in pc.h, or even better, introduce i8254.h.

No concerns about i8254.h, but this function does not qualify for static inline.

Quoted 2 lines

The function is static inline in a header file not for performance reasons, but to keep the instantiation separate from device internals.

Not performance, footprint and header dependencies. You need to pull in all the stuff the inline function needs for everyone including the header that contains this function. That's messy.

Even if the instantiation helper should not poke into the device model internals (and I don't want this to change as well), it belongs to the module that implements the device. We do the same with other fabric functions.

Quoted 11 lines · Jan Kiszka

Quoted 10 lines · Blue Swirl

Quoted 8 lines · Jan Kiszka

Quoted 4 lines · Blue Swirl

Quoted 2 lines

+ISADevice *pit_init(int base, qemu_irq irq)

Please retain this function in pc.h, or even better, introduce i8254.h.

No concerns about i8254.h, but this function does not qualify for static inline.

Quoted 6 lines

Quoted 2 lines

The function is static inline in a header file not for performance reasons, but to keep the instantiation separate from device internals.

Not performance, footprint and header dependencies. You need to pull in all the stuff the inline function needs for everyone including the header that contains this function. That's messy.

There's only ISA and qdev stuff, that's not messy since both are needed in any case.

Quoted 4 lines

Even if the instantiation helper should not poke into the device model internals (and I don't want this to change as well), it belongs to the module that implements the device. We do the same with other fabric functions.

In this case, the callers have the same needs and there are several of them. In general this need not be true at all, if for example some part of instantiation would have to be skipped, the functions may need to be manually inlined to the board level anyway. The instantiation definitely does not belong to the implementer but to the creator. Ideally file implementing the device contains only static functions and instantiation is either in a header file or at the board. This is true for example for several Sparc32 devices.

Quoted 27 lines · Blue Swirl

Quoted 11 lines · Jan Kiszka

Quoted 10 lines · Blue Swirl

Quoted 8 lines · Jan Kiszka

Quoted 4 lines · Blue Swirl

Quoted 2 lines

+ISADevice *pit_init(int base, qemu_irq irq)

Please retain this function in pc.h, or even better, introduce i8254.h.

No concerns about i8254.h, but this function does not qualify for static inline.

Quoted 6 lines

Quoted 2 lines

The function is static inline in a header file not for performance reasons, but to keep the instantiation separate from device internals.

Not performance, footprint and header dependencies. You need to pull in all the stuff the inline function needs for everyone including the header that contains this function. That's messy.

There's only ISA and qdev stuff, that's not messy since both are needed in any case.

Quoted 4 lines

Even if the instantiation helper should not poke into the device model internals (and I don't want this to change as well), it belongs to the module that implements the device. We do the same with other fabric functions.

Quoted 8 lines

In this case, the callers have the same needs and there are several of them. In general this need not be true at all, if for example some part of instantiation would have to be skipped, the functions may need to be manually inlined to the board level anyway. The instantiation definitely does not belong to the implementer but to the creator. Ideally file implementing the device contains only static functions and instantiation is either in a header file or at the board. This is true for example for several Sparc32 devices.

The helper is wrapping the property base API into a proper function call
- nothing that is board-specific.

This is a small preparatory series to allow the introduction of the KVM in-kernel PIT. Of course, it is also a fix for the various bugs in the related PIT/HPET code. See patches for details.

Jan Kiszka (2):
 hpet: Save/restore cached RTC IRQ level
 i8254: Rework & fix interaction with HPET in legacy mode

 hw/alpha_dp264.c   |    2 +-
 hw/hpet.c          |   64 +++++++++++++++++++++++++++++++++++++-------------- hw/hpet_emul.h     |    3 ++
 hw/i8254.c         |   60 +++++++++++++++++++++++++++--------------------- hw/mips_fulong2e.c |    2 +-
 hw/mips_jazz.c     |    2 +-
 hw/mips_malta.c    |    2 +-
 hw/mips_r4k.c      |    2 +-
 hw/pc.c            |   13 ++++++++--
 hw/pc.h            |   13 +---------
 hw/ppc_prep.c      |    2 +-

 11 files changed, 100 insertions(+), 65 deletions(-)

--
1.7.3.4