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 <address@hidden> wrote:
> From: Jan Kiszka <address@hidden>
>
> 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 <address@hidden>
> ---
>  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
>