diff mbox

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

Message ID 42a283294478be02e4f1dc5b0a78421030b51bbe.1323520118.git.jan.kiszka@web.de
State New
Headers show

Commit Message

Jan Kiszka Dec. 10, 2011, 12:28 p.m. UTC 
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(-)

Comments

Blue Swirl Dec. 10, 2011, 3:49 p.m. UTC | #1 
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
>
Jan Kiszka Dec. 10, 2011, 3:51 p.m. UTC | #2 
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
Blue Swirl Dec. 10, 2011, 3:54 p.m. UTC | #3 
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
>
Jan Kiszka Dec. 10, 2011, 4:03 p.m. UTC | #4 
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
Blue Swirl Dec. 10, 2011, 4:26 p.m. UTC | #5 
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.
Jan Kiszka Dec. 10, 2011, 4:29 p.m. UTC | #6 
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
Blue Swirl Dec. 10, 2011, 4:49 p.m. UTC | #7 
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.
diff mbox

Patch

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