[PATCH 0/2] pit/hpet: Fix legacy mode switching

[PATCH 0/2] pit/hpet: Fix legacy mode switching

[Jan Kiszka]

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

[Qemu-devel] [PATCH 0/2] pit/hpet: Fix legacy mode switching

[Jan Kiszka]

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

[PATCH 1/2] hpet: Save/restore cached RTC IRQ level

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

In legacy mode, the HPET suppresses the RTC interrupt delivery via IRQ
8 but keeps track of the RTC output level and applies it when legacy
mode is turned off again. This value has to be preserved across save/
restore as it cannot be reconstructed otherwise.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/hpet.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/hw/hpet.c b/hw/hpet.c
index 5312df7..1b64e6a 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -240,6 +240,24 @@ static int hpet_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static bool hpet_rtc_irq_level_needed(void *opaque)
+{
+    HPETState *s = opaque;
+
+    return s->rtc_irq_level != 0;
+}
+
+static const VMStateDescription vmstate_hpet_rtc_irq_level = {
+    .name = "hpet/rtc_irq_level",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_UINT8(rtc_irq_level, HPETState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_hpet_timer = {
     .name = "hpet_timer",
     .version_id = 1,
@@ -273,6 +291,14 @@ static const VMStateDescription vmstate_hpet = {
         VMSTATE_STRUCT_VARRAY_UINT8(timer, HPETState, num_timers, 0,
                                     vmstate_hpet_timer, HPETTimer),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection[]) {
+        {
+            .vmsd = &vmstate_hpet_rtc_irq_level,
+            .needed = hpet_rtc_irq_level_needed,
+        }, {
+            /* empty */
+        }
     }
 };
 
-- 
1.7.3.4

[Qemu-devel] [PATCH 1/2] hpet: Save/restore cached RTC IRQ level

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

In legacy mode, the HPET suppresses the RTC interrupt delivery via IRQ
8 but keeps track of the RTC output level and applies it when legacy
mode is turned off again. This value has to be preserved across save/
restore as it cannot be reconstructed otherwise.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/hpet.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/hw/hpet.c b/hw/hpet.c
index 5312df7..1b64e6a 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -240,6 +240,24 @@ static int hpet_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static bool hpet_rtc_irq_level_needed(void *opaque)
+{
+    HPETState *s = opaque;
+
+    return s->rtc_irq_level != 0;
+}
+
+static const VMStateDescription vmstate_hpet_rtc_irq_level = {
+    .name = "hpet/rtc_irq_level",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_UINT8(rtc_irq_level, HPETState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_hpet_timer = {
     .name = "hpet_timer",
     .version_id = 1,
@@ -273,6 +291,14 @@ static const VMStateDescription vmstate_hpet = {
         VMSTATE_STRUCT_VARRAY_UINT8(timer, HPETState, num_timers, 0,
                                     vmstate_hpet_timer, HPETTimer),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection[]) {
+        {
+            .vmsd = &vmstate_hpet_rtc_irq_level,
+            .needed = hpet_rtc_irq_level_needed,
+        }, {
+            /* empty */
+        }
     }
 };
 
-- 
1.7.3.4

[PATCH 2/2] i8254: Rework & fix interaction with HPET in legacy mode

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

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 <jan.kiszka@siemens.com>
---
 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(-)

diff --git a/hw/alpha_dp264.c b/hw/alpha_dp264.c
index fcc20e9..412ccf0 100644
--- a/hw/alpha_dp264.c
+++ b/hw/alpha_dp264.c
@@ -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.  */
diff --git a/hw/hpet.c b/hw/hpet.c
index 1b64e6a..ace0b1d 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -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);
diff --git a/hw/hpet_emul.h b/hw/hpet_emul.h
index 6128702..757f79f 100644
--- a/hw/hpet_emul.h
+++ b/hw/hpet_emul.h
@@ -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
 
diff --git a/hw/i8254.c b/hw/i8254.c
index 12571ef..b3d9624 100644
--- a/hw/i8254.c
+++ b/hw/i8254.c
@@ -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);
     isa_register_ioport(dev, &pit->ioports, pit->iobase);
 
+    qdev_init_gpio_in(&dev->qdev, pit_irq_control, 1);
+
     qdev_set_legacy_instance_id(&dev->qdev, pit->iobase, 2);
 
     return 0;
 }
 
+ISADevice *pit_init(int base, qemu_irq irq)
+{
+    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(),
     },
diff --git a/hw/mips_fulong2e.c b/hw/mips_fulong2e.c
index 04921c1..8a9c006 100644
--- a/hw/mips_fulong2e.c
+++ b/hw/mips_fulong2e.c
@@ -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);
 
diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c
index 358de59..a076e1d 100644
--- a/hw/mips_jazz.c
+++ b/hw/mips_jazz.c
@@ -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 */
diff --git a/hw/mips_malta.c b/hw/mips_malta.c
index bb49749..d09a48a 100644
--- a/hw/mips_malta.c
+++ b/hw/mips_malta.c
@@ -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);
 
diff --git a/hw/mips_r4k.c b/hw/mips_r4k.c
index d0564d4..9fef3ba 100644
--- a/hw/mips_r4k.c
+++ b/hw/mips_r4k.c
@@ -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]) {
diff --git a/hw/pc.c b/hw/pc.c
index 33778fe..dcf87af 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -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++) {
diff --git a/hw/pc.h b/hw/pc.h
index b7b7e40..20ee543 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -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);
diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
index f22d5b9..84e2028 100644
--- a/hw/ppc_prep.c
+++ b/hw/ppc_prep.c
@@ -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

[Qemu-devel] [PATCH 2/2] i8254: Rework & fix interaction with HPET in legacy mode

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

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 <jan.kiszka@siemens.com>
---
 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(-)

diff --git a/hw/alpha_dp264.c b/hw/alpha_dp264.c
index fcc20e9..412ccf0 100644
--- a/hw/alpha_dp264.c
+++ b/hw/alpha_dp264.c
@@ -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.  */
diff --git a/hw/hpet.c b/hw/hpet.c
index 1b64e6a..ace0b1d 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -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);
diff --git a/hw/hpet_emul.h b/hw/hpet_emul.h
index 6128702..757f79f 100644
--- a/hw/hpet_emul.h
+++ b/hw/hpet_emul.h
@@ -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
 
diff --git a/hw/i8254.c b/hw/i8254.c
index 12571ef..b3d9624 100644
--- a/hw/i8254.c
+++ b/hw/i8254.c
@@ -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);
     isa_register_ioport(dev, &pit->ioports, pit->iobase);
 
+    qdev_init_gpio_in(&dev->qdev, pit_irq_control, 1);
+
     qdev_set_legacy_instance_id(&dev->qdev, pit->iobase, 2);
 
     return 0;
 }
 
+ISADevice *pit_init(int base, qemu_irq irq)
+{
+    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(),
     },
diff --git a/hw/mips_fulong2e.c b/hw/mips_fulong2e.c
index 04921c1..8a9c006 100644
--- a/hw/mips_fulong2e.c
+++ b/hw/mips_fulong2e.c
@@ -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);
 
diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c
index 358de59..a076e1d 100644
--- a/hw/mips_jazz.c
+++ b/hw/mips_jazz.c
@@ -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 */
diff --git a/hw/mips_malta.c b/hw/mips_malta.c
index bb49749..d09a48a 100644
--- a/hw/mips_malta.c
+++ b/hw/mips_malta.c
@@ -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);
 
diff --git a/hw/mips_r4k.c b/hw/mips_r4k.c
index d0564d4..9fef3ba 100644
--- a/hw/mips_r4k.c
+++ b/hw/mips_r4k.c
@@ -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]) {
diff --git a/hw/pc.c b/hw/pc.c
index 33778fe..dcf87af 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -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++) {
diff --git a/hw/pc.h b/hw/pc.h
index b7b7e40..20ee543 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -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);
diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
index f22d5b9..84e2028 100644
--- a/hw/ppc_prep.c
+++ b/hw/ppc_prep.c
@@ -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

Re: [PATCH 2/2] i8254: Rework & fix interaction with HPET in legacy mode

[Blue Swirl]

On Sat, Dec 10, 2011 at 12:28, Jan Kiszka <jan.kiszka@web.de> wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> 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 <jan.kiszka@siemens.com>
> ---
>  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(-)
>
> diff --git a/hw/alpha_dp264.c b/hw/alpha_dp264.c
> index fcc20e9..412ccf0 100644
> --- a/hw/alpha_dp264.c
> +++ b/hw/alpha_dp264.c
> @@ -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.  */
> diff --git a/hw/hpet.c b/hw/hpet.c
> index 1b64e6a..ace0b1d 100644
> --- a/hw/hpet.c
> +++ b/hw/hpet.c
> @@ -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);
> diff --git a/hw/hpet_emul.h b/hw/hpet_emul.h
> index 6128702..757f79f 100644
> --- a/hw/hpet_emul.h
> +++ b/hw/hpet_emul.h
> @@ -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
>
> diff --git a/hw/i8254.c b/hw/i8254.c
> index 12571ef..b3d9624 100644
> --- a/hw/i8254.c
> +++ b/hw/i8254.c
> @@ -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);
>     isa_register_ioport(dev, &pit->ioports, pit->iobase);
>
> +    qdev_init_gpio_in(&dev->qdev, pit_irq_control, 1);
> +
>     qdev_set_legacy_instance_id(&dev->qdev, pit->iobase, 2);
>
>     return 0;
>  }
>
> +ISADevice *pit_init(int base, qemu_irq irq)

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

> +{
> +    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(),
>     },
> diff --git a/hw/mips_fulong2e.c b/hw/mips_fulong2e.c
> index 04921c1..8a9c006 100644
> --- a/hw/mips_fulong2e.c
> +++ b/hw/mips_fulong2e.c
> @@ -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);
>
> diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c
> index 358de59..a076e1d 100644
> --- a/hw/mips_jazz.c
> +++ b/hw/mips_jazz.c
> @@ -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 */
> diff --git a/hw/mips_malta.c b/hw/mips_malta.c
> index bb49749..d09a48a 100644
> --- a/hw/mips_malta.c
> +++ b/hw/mips_malta.c
> @@ -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);
>
> diff --git a/hw/mips_r4k.c b/hw/mips_r4k.c
> index d0564d4..9fef3ba 100644
> --- a/hw/mips_r4k.c
> +++ b/hw/mips_r4k.c
> @@ -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]) {
> diff --git a/hw/pc.c b/hw/pc.c
> index 33778fe..dcf87af 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -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++) {
> diff --git a/hw/pc.h b/hw/pc.h
> index b7b7e40..20ee543 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -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);
> diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
> index f22d5b9..84e2028 100644
> --- a/hw/ppc_prep.c
> +++ b/hw/ppc_prep.c
> @@ -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
>

Re: [Qemu-devel] [PATCH 2/2] i8254: Rework & fix interaction with HPET in legacy mode

[Blue Swirl]

On Sat, Dec 10, 2011 at 12:28, Jan Kiszka <jan.kiszka@web.de> wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> 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 <jan.kiszka@siemens.com>
> ---
>  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(-)
>
> diff --git a/hw/alpha_dp264.c b/hw/alpha_dp264.c
> index fcc20e9..412ccf0 100644
> --- a/hw/alpha_dp264.c
> +++ b/hw/alpha_dp264.c
> @@ -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.  */
> diff --git a/hw/hpet.c b/hw/hpet.c
> index 1b64e6a..ace0b1d 100644
> --- a/hw/hpet.c
> +++ b/hw/hpet.c
> @@ -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);
> diff --git a/hw/hpet_emul.h b/hw/hpet_emul.h
> index 6128702..757f79f 100644
> --- a/hw/hpet_emul.h
> +++ b/hw/hpet_emul.h
> @@ -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
>
> diff --git a/hw/i8254.c b/hw/i8254.c
> index 12571ef..b3d9624 100644
> --- a/hw/i8254.c
> +++ b/hw/i8254.c
> @@ -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);
>     isa_register_ioport(dev, &pit->ioports, pit->iobase);
>
> +    qdev_init_gpio_in(&dev->qdev, pit_irq_control, 1);
> +
>     qdev_set_legacy_instance_id(&dev->qdev, pit->iobase, 2);
>
>     return 0;
>  }
>
> +ISADevice *pit_init(int base, qemu_irq irq)

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

> +{
> +    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(),
>     },
> diff --git a/hw/mips_fulong2e.c b/hw/mips_fulong2e.c
> index 04921c1..8a9c006 100644
> --- a/hw/mips_fulong2e.c
> +++ b/hw/mips_fulong2e.c
> @@ -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);
>
> diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c
> index 358de59..a076e1d 100644
> --- a/hw/mips_jazz.c
> +++ b/hw/mips_jazz.c
> @@ -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 */
> diff --git a/hw/mips_malta.c b/hw/mips_malta.c
> index bb49749..d09a48a 100644
> --- a/hw/mips_malta.c
> +++ b/hw/mips_malta.c
> @@ -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);
>
> diff --git a/hw/mips_r4k.c b/hw/mips_r4k.c
> index d0564d4..9fef3ba 100644
> --- a/hw/mips_r4k.c
> +++ b/hw/mips_r4k.c
> @@ -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]) {
> diff --git a/hw/pc.c b/hw/pc.c
> index 33778fe..dcf87af 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -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++) {
> diff --git a/hw/pc.h b/hw/pc.h
> index b7b7e40..20ee543 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -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);
> diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
> index f22d5b9..84e2028 100644
> --- a/hw/ppc_prep.c
> +++ b/hw/ppc_prep.c
> @@ -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
>

Re: [PATCH 2/2] i8254: Rework & fix interaction with HPET in legacy mode

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 538 bytes --]

On 2011-12-10 16:49, Blue Swirl wrote:
>>
>> +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.

> 
>> +{
>> +    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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH 2/2] i8254: Rework & fix interaction with HPET in legacy mode

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 538 bytes --]

On 2011-12-10 16:49, Blue Swirl wrote:
>>
>> +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.

> 
>> +{
>> +    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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH 2/2] i8254: Rework & fix interaction with HPET in legacy mode

[Blue Swirl]

On Sat, Dec 10, 2011 at 15:51, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2011-12-10 16:49, Blue Swirl wrote:
>>>
>>> +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.

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

>>
>>> +{
>>> +    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
>

Re: [Qemu-devel] [PATCH 2/2] i8254: Rework & fix interaction with HPET in legacy mode

[Blue Swirl]

On Sat, Dec 10, 2011 at 15:51, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2011-12-10 16:49, Blue Swirl wrote:
>>>
>>> +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.

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

>>
>>> +{
>>> +    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
>

Re: [PATCH 2/2] i8254: Rework & fix interaction with HPET in legacy mode

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 965 bytes --]

On 2011-12-10 16:54, Blue Swirl wrote:
> On Sat, Dec 10, 2011 at 15:51, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2011-12-10 16:49, Blue Swirl wrote:
>>>>
>>>> +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.
> 
> 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.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH 2/2] i8254: Rework & fix interaction with HPET in legacy mode

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 965 bytes --]

On 2011-12-10 16:54, Blue Swirl wrote:
> On Sat, Dec 10, 2011 at 15:51, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2011-12-10 16:49, Blue Swirl wrote:
>>>>
>>>> +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.
> 
> 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.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH 2/2] i8254: Rework & fix interaction with HPET in legacy mode

[Blue Swirl]

On Sat, Dec 10, 2011 at 16:03, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2011-12-10 16:54, Blue Swirl wrote:
>> On Sat, Dec 10, 2011 at 15:51, Jan Kiszka <jan.kiszka@web.de> wrote:
>>> On 2011-12-10 16:49, Blue Swirl wrote:
>>>>>
>>>>> +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.
>>
>> 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.

> 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.

Re: [Qemu-devel] [PATCH 2/2] i8254: Rework & fix interaction with HPET in legacy mode

[Blue Swirl]

On Sat, Dec 10, 2011 at 16:03, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2011-12-10 16:54, Blue Swirl wrote:
>> On Sat, Dec 10, 2011 at 15:51, Jan Kiszka <jan.kiszka@web.de> wrote:
>>> On 2011-12-10 16:49, Blue Swirl wrote:
>>>>>
>>>>> +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.
>>
>> 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.

> 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.

Re: [PATCH 2/2] i8254: Rework & fix interaction with HPET in legacy mode

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1889 bytes --]

On 2011-12-10 17:26, Blue Swirl wrote:
> On Sat, Dec 10, 2011 at 16:03, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2011-12-10 16:54, Blue Swirl wrote:
>>> On Sat, Dec 10, 2011 at 15:51, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>> On 2011-12-10 16:49, Blue Swirl wrote:
>>>>>>
>>>>>> +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.
>>>
>>> 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.
> 
>> 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.

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

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH 2/2] i8254: Rework & fix interaction with HPET in legacy mode

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1889 bytes --]

On 2011-12-10 17:26, Blue Swirl wrote:
> On Sat, Dec 10, 2011 at 16:03, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2011-12-10 16:54, Blue Swirl wrote:
>>> On Sat, Dec 10, 2011 at 15:51, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>> On 2011-12-10 16:49, Blue Swirl wrote:
>>>>>>
>>>>>> +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.
>>>
>>> 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.
> 
>> 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.

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

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH 2/2] i8254: Rework & fix interaction with HPET in legacy mode

[Blue Swirl]

On Sat, Dec 10, 2011 at 16:29, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2011-12-10 17:26, Blue Swirl wrote:
>> On Sat, Dec 10, 2011 at 16:03, Jan Kiszka <jan.kiszka@web.de> wrote:
>>> On 2011-12-10 16:54, Blue Swirl wrote:
>>>> On Sat, Dec 10, 2011 at 15:51, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>>> On 2011-12-10 16:49, Blue Swirl wrote:
>>>>>>>
>>>>>>> +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.
>>>>
>>>> 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.
>>
>>> 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.
>
> The helper is wrapping the property base API into a proper function call
> - nothing that is board-specific.

Not in this case, but in general boards could need to pass different
sets of properties or avoid passing something at all.

Re: [Qemu-devel] [PATCH 2/2] i8254: Rework & fix interaction with HPET in legacy mode

[Blue Swirl]

On Sat, Dec 10, 2011 at 16:29, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2011-12-10 17:26, Blue Swirl wrote:
>> On Sat, Dec 10, 2011 at 16:03, Jan Kiszka <jan.kiszka@web.de> wrote:
>>> On 2011-12-10 16:54, Blue Swirl wrote:
>>>> On Sat, Dec 10, 2011 at 15:51, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>>> On 2011-12-10 16:49, Blue Swirl wrote:
>>>>>>>
>>>>>>> +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.
>>>>
>>>> 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.
>>
>>> 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.
>
> The helper is wrapping the property base API into a proper function call
> - nothing that is board-specific.

Not in this case, but in general boards could need to pass different
sets of properties or avoid passing something at all.

Re: [PATCH 0/2] pit/hpet: Fix legacy mode switching

[Blue Swirl]

On Sat, Dec 10, 2011 at 12:28, Jan Kiszka <jan.kiszka@web.de> wrote:
> 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

I had one comment to this patch.

Otherwise nice cleanups, I think this logic matches real PIT/HPET
routing better.

>  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
>

Re: [Qemu-devel] [PATCH 0/2] pit/hpet: Fix legacy mode switching

[Blue Swirl]

On Sat, Dec 10, 2011 at 12:28, Jan Kiszka <jan.kiszka@web.de> wrote:
> 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

I had one comment to this patch.

Otherwise nice cleanups, I think this logic matches real PIT/HPET
routing better.

>  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
>