Patchwork [RFC,2/4] hpet: don't use any static state

login
register
mail settings
Submitter Blue Swirl
Date May 23, 2010, 12:39 p.m.
Message ID <AANLkTikJsUKGDA_TNeg1AUGvojkGbkg8UIJDe9Ho1qoP@mail.gmail.com>
Download mbox | patch
Permalink /patch/53329/
State New
Headers show

Comments

Blue Swirl - May 23, 2010, 12:39 p.m.
Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 hw/hpet.c      |   68 +++++++++++++++++++++++++++++--------------------------
 hw/hpet_emul.h |    4 +-
 hw/pc.c        |    8 ++++--
 hw/pc.h        |    3 +-
 hw/pc_piix.c   |    3 +-
 5 files changed, 47 insertions(+), 39 deletions(-)
Jan Kiszka - May 23, 2010, 3:40 p.m.
Blue Swirl wrote:
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> ---
>  hw/hpet.c      |   68 +++++++++++++++++++++++++++++--------------------------
>  hw/hpet_emul.h |    4 +-
>  hw/pc.c        |    8 ++++--
>  hw/pc.h        |    3 +-
>  hw/pc_piix.c   |    3 +-
>  5 files changed, 47 insertions(+), 39 deletions(-)
> 
> diff --git a/hw/hpet.c b/hw/hpet.c
> index 8729fb2..f24e054 100644
> --- a/hw/hpet.c
> +++ b/hw/hpet.c
> @@ -37,14 +37,11 @@
>  #define DPRINTF(...)
>  #endif
> 
> -static HPETState *hpet_statep;
> -
> -uint32_t hpet_in_legacy_mode(void)
> +uint32_t hpet_in_legacy_mode(void *opaque)

uint32_t hpet_in_legacy_mode(HPETState *s)

please (will become DeviceState with my patches, but it should not be
void in any case).

>  {
> -    if (hpet_statep)
> -        return hpet_statep->config & HPET_CFG_LEGACY;
> -    else
> -        return 0;
> +    HPETState *s = opaque;
> +
> +    return s->config & HPET_CFG_LEGACY;
>  }
> 
>  static uint32_t timer_int_route(struct HPETTimer *timer)
> @@ -54,9 +51,9 @@ static uint32_t timer_int_route(struct HPETTimer *timer)
>      return route;
>  }
> 
> -static uint32_t hpet_enabled(void)
> +static uint32_t hpet_enabled(HPETState *s)
>  {
> -    return hpet_statep->config & HPET_CFG_ENABLE;
> +    return s->config & HPET_CFG_ENABLE;
>  }
> 
>  static uint32_t timer_is_periodic(HPETTimer *t)
> @@ -106,10 +103,10 @@ static int deactivating_bit(uint64_t old,
> uint64_t new, uint64_t mask)
>      return ((old & mask) && !(new & mask));
>  }
> 
> -static uint64_t hpet_get_ticks(void)
> +static uint64_t hpet_get_ticks(HPETState *s)
>  {
>      uint64_t ticks;
> -    ticks = ns_to_ticks(qemu_get_clock(vm_clock) + hpet_statep->hpet_offset);
> +    ticks = ns_to_ticks(qemu_get_clock(vm_clock) + s->hpet_offset);
>      return ticks;
>  }
> 
> @@ -139,7 +136,7 @@ static void update_irq(struct HPETTimer *timer)
>      qemu_irq irq;
>      int route;
> 
> -    if (timer->tn <= 1 && hpet_in_legacy_mode()) {
> +    if (timer->tn <= 1 && hpet_in_legacy_mode(timer->state)) {
>          /* if LegacyReplacementRoute bit is set, HPET specification requires
>           * timer0 be routed to IRQ0 in NON-APIC or IRQ2 in the I/O APIC,
>           * timer1 be routed to IRQ8 in NON-APIC or IRQ8 in the I/O APIC.
> @@ -152,7 +149,7 @@ static void update_irq(struct HPETTimer *timer)
>          route=timer_int_route(timer);
>          irq=timer->state->irqs[route];
>      }
> -    if (timer_enabled(timer) && hpet_enabled()) {
> +    if (timer_enabled(timer) && hpet_enabled(timer->state)) {
>          qemu_irq_pulse(irq);
>      }
>  }
> @@ -161,7 +158,7 @@ static void hpet_pre_save(void *opaque)
>  {
>      HPETState *s = opaque;
>      /* save current counter value */
> -    s->hpet_counter = hpet_get_ticks();
> +    s->hpet_counter = hpet_get_ticks(s);
>  }
> 
>  static int hpet_post_load(void *opaque, int version_id)
> @@ -216,7 +213,7 @@ static void hpet_timer(void *opaque)
>      uint64_t diff;
> 
>      uint64_t period = t->period;
> -    uint64_t cur_tick = hpet_get_ticks();
> +    uint64_t cur_tick = hpet_get_ticks(t->state);
> 
>      if (timer_is_periodic(t) && period != 0) {
>          if (t->config & HPET_TN_32BIT) {
> @@ -244,7 +241,7 @@ static void hpet_set_timer(HPETTimer *t)
>  {
>      uint64_t diff;
>      uint32_t wrap_diff;  /* how many ticks until we wrap? */
> -    uint64_t cur_tick = hpet_get_ticks();
> +    uint64_t cur_tick = hpet_get_ticks(t->state);
> 
>      /* whenever new timer is being set up, make sure wrap_flag is 0 */
>      t->wrap_flag = 0;
> @@ -326,17 +323,19 @@ static uint32_t hpet_ram_readl(void *opaque,
> target_phys_addr_t addr)
>                  DPRINTF("qemu: invalid HPET_CFG + 4 hpet_ram_readl \n");
>                  return 0;
>              case HPET_COUNTER:
> -                if (hpet_enabled())
> -                    cur_tick = hpet_get_ticks();
> -                else
> +                if (hpet_enabled(s)) {
> +                    cur_tick = hpet_get_ticks(s);
> +                } else {
>                      cur_tick = s->hpet_counter;
> +                }
>                  DPRINTF("qemu: reading counter  = %" PRIx64 "\n", cur_tick);
>                  return cur_tick;
>              case HPET_COUNTER + 4:
> -                if (hpet_enabled())
> -                    cur_tick = hpet_get_ticks();
> -                else
> +                if (hpet_enabled(s)) {
> +                    cur_tick = hpet_get_ticks(s);
> +                } else {
>                      cur_tick = s->hpet_counter;
> +                }
>                  DPRINTF("qemu: reading counter + 4  = %" PRIx64 "\n",
> cur_tick);
>                  return cur_tick >> 32;
>              case HPET_STATUS:
> @@ -419,8 +418,9 @@ static void hpet_ram_writel(void *opaque,
> target_phys_addr_t addr,
>                                       | new_val;
>                  }
>                  timer->config &= ~HPET_TN_SETVAL;
> -                if (hpet_enabled())
> +                if (hpet_enabled(s)) {
>                      hpet_set_timer(timer);
> +                }
>                  break;
>              case HPET_TN_CMP + 4: // comparator register high order
>                  DPRINTF("qemu: hpet_ram_writel HPET_TN_CMP + 4\n");
> @@ -439,8 +439,9 @@ static void hpet_ram_writel(void *opaque,
> target_phys_addr_t addr,
>                                       | new_val << 32;
>                  }
>                  timer->config &= ~HPET_TN_SETVAL;
> -                if (hpet_enabled())
> +                if (hpet_enabled(s)) {
>                      hpet_set_timer(timer);
> +                }
>                  break;
>              case HPET_TN_ROUTE + 4:
>                  DPRINTF("qemu: hpet_ram_writel HPET_TN_ROUTE + 4\n");
> @@ -467,7 +468,7 @@ static void hpet_ram_writel(void *opaque,
> target_phys_addr_t addr,
>                  }
>                  else if (deactivating_bit(old_val, new_val, HPET_CFG_ENABLE)) {
>                      /* Halt main counter and disable interrupt generation. */
> -                    s->hpet_counter = hpet_get_ticks();
> +                    s->hpet_counter = hpet_get_ticks(s);
>                      for (i = 0; i < HPET_NUM_TIMERS; i++)
>                          hpet_del_timer(&s->timer[i]);
>                  }
> @@ -485,16 +486,18 @@ static void hpet_ram_writel(void *opaque,
> target_phys_addr_t addr,
>                  /* FIXME: need to handle level-triggered interrupts */
>                  break;
>              case HPET_COUNTER:
> -               if (hpet_enabled())
> -                   printf("qemu: Writing counter while HPET enabled!\n");
> +                if (hpet_enabled(s)) {
> +                    printf("qemu: Writing counter while HPET enabled!\n");

Missed it in my series as well: Should become DPRINTF at this chance.
Also below.

> +                }
>                 s->hpet_counter = (s->hpet_counter & 0xffffffff00000000ULL)
>                                    | value;
>                 DPRINTF("qemu: HPET counter written. ctr = %#x -> %"
> PRIx64 "\n",
>                          value, s->hpet_counter);
>                 break;
>              case HPET_COUNTER + 4:
> -               if (hpet_enabled())
> -                   printf("qemu: Writing counter while HPET enabled!\n");
> +                if (hpet_enabled(s)) {
> +                    printf("qemu: Writing counter while HPET enabled!\n");
> +                }
>                 s->hpet_counter = (s->hpet_counter & 0xffffffffULL)
>                                    | (((uint64_t)value) << 32);
>                 DPRINTF("qemu: HPET counter + 4 written. ctr = %#x ->
> %" PRIx64 "\n",
> @@ -563,15 +566,14 @@ static void hpet_reset(void *opaque) {
>      count = 1;
>  }
> 
> -
> -void hpet_init(qemu_irq *irq) {
> +HPETState *hpet_init(qemu_irq *irq)
> +{
>      int i, iomemtype;
>      HPETState *s;
> 
>      DPRINTF ("hpet_init\n");
> 
>      s = qemu_mallocz(sizeof(HPETState));
> -    hpet_statep = s;
>      s->irqs = irq;
>      for (i=0; i<HPET_NUM_TIMERS; i++) {
>          HPETTimer *timer = &s->timer[i];
> @@ -583,4 +585,6 @@ void hpet_init(qemu_irq *irq) {
>      iomemtype = cpu_register_io_memory(hpet_ram_read,
>                                         hpet_ram_write, s);
>      cpu_register_physical_memory(HPET_BASE, 0x400, iomemtype);
> +
> +    return s;
>  }
> diff --git a/hw/hpet_emul.h b/hw/hpet_emul.h
> index cfd95b4..88dbf0d 100644
> --- a/hw/hpet_emul.h
> +++ b/hw/hpet_emul.h
> @@ -75,8 +75,8 @@ typedef struct HPETState {
>  } HPETState;
> 
>  #if defined TARGET_I386
> -extern uint32_t hpet_in_legacy_mode(void);
> -extern void hpet_init(qemu_irq *irq);
> +uint32_t hpet_in_legacy_mode(void *opaque);
> +HPETState *hpet_init(qemu_irq *irq);
>  #endif
> 
>  #endif
> diff --git a/hw/pc.c b/hw/pc.c
> index 5a703e1..9f1a9d6 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -83,7 +83,7 @@ static void rtc_irq_handler(IsaIrqState *isa, int level)
>       * mode is established while interrupt is raised. We want it to
>       * be lowered in any case.
>       */
> -    if (!hpet_in_legacy_mode() || !level) {
> +    if ((isa->hpet_state && !hpet_in_legacy_mode(isa->hpet_state)) || !level) {
>          isa_set_irq(isa, RTC_ISA_IRQ, level);
>      }
>  }
> @@ -945,7 +945,7 @@ static void cpu_request_exit(void *opaque, int
> irq, int level)
>      }
>  }
> 
> -void pc_basic_device_init(qemu_irq *isa_irq,
> +void pc_basic_device_init(qemu_irq *isa_irq, IsaIrqState *isa,
>                            FDCtrl **floppy_controller,
>                            ISADevice **rtc_state)
>  {
> @@ -966,8 +966,10 @@ void pc_basic_device_init(qemu_irq *isa_irq,
> 
>      pit = pit_init(0x40, isa_reserve_irq(0));
>      pcspk_init(pit);
> +
> +    isa->hpet_state = NULL;
>      if (!no_hpet) {
> -        hpet_init(isa_irq);
> +        isa->hpet_state = hpet_init(isa_irq);
>      }
> 
>      for(i = 0; i < MAX_SERIAL_PORTS; i++) {
> diff --git a/hw/pc.h b/hw/pc.h
> index 73cccef..3e085b9 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -42,6 +42,7 @@ void irq_info(Monitor *mon);
>  typedef struct isa_irq_state {
>      qemu_irq *i8259;
>      qemu_irq *ioapic;
> +    void *hpet_state;
>  } IsaIrqState;
> 
>  void isa_irq_handler(void *opaque, int n, int level);
> @@ -94,7 +95,7 @@ void pc_memory_init(ram_addr_t ram_size,
>                      ram_addr_t *above_4g_mem_size_p);
>  qemu_irq *pc_allocate_cpu_irq(void);
>  void pc_vga_init(PCIBus *pci_bus);
> -void pc_basic_device_init(qemu_irq *isa_irq,
> +void pc_basic_device_init(qemu_irq *isa_irq, IsaIrqState *isa,
>                            FDCtrl **floppy_controller,
>                            ISADevice **rtc_state);
>  void pc_init_ne2k_isa(NICInfo *nd);
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 70f563a..1479a28 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -93,7 +93,8 @@ static void pc_init1(ram_addr_t ram_size,
>      pc_vga_init(pci_enabled? pci_bus: NULL);
> 
>      /* init basic PC hardware */
> -    pc_basic_device_init(isa_irq, &floppy_controller, &rtc_state);
> +    pc_basic_device_init(isa_irq, isa_irq_state, &floppy_controller,
> +                         &rtc_state);
> 
>      for(i = 0; i < nb_nics; i++) {
>          NICInfo *nd = &nd_table[i];

I have a longer hpet fix/qdev'ify/enhance series queued [1], held back
until the device_show thing is through. Your patch break it, but it
makes sense. Will rebase on top.

Jan

[1] git://git.kiszka.org/qemu.git queues/hpet
Blue Swirl - May 23, 2010, 4:20 p.m.
On Sun, May 23, 2010 at 3:40 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> Blue Swirl wrote:
>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>> ---
>>  hw/hpet.c      |   68 +++++++++++++++++++++++++++++--------------------------
>>  hw/hpet_emul.h |    4 +-
>>  hw/pc.c        |    8 ++++--
>>  hw/pc.h        |    3 +-
>>  hw/pc_piix.c   |    3 +-
>>  5 files changed, 47 insertions(+), 39 deletions(-)
>>
>> diff --git a/hw/hpet.c b/hw/hpet.c
>> index 8729fb2..f24e054 100644
>> --- a/hw/hpet.c
>> +++ b/hw/hpet.c
>> @@ -37,14 +37,11 @@
>>  #define DPRINTF(...)
>>  #endif
>>
>> -static HPETState *hpet_statep;
>> -
>> -uint32_t hpet_in_legacy_mode(void)
>> +uint32_t hpet_in_legacy_mode(void *opaque)
>
> uint32_t hpet_in_legacy_mode(HPETState *s)
>
> please (will become DeviceState with my patches, but it should not be
> void in any case).

I tried that, but HPTState is not available for all cases where pc.h
is #included. DeviceState or ISADeviceState would be much better, the
callers have no need to access HPETState fields.

>>  {
>> -    if (hpet_statep)
>> -        return hpet_statep->config & HPET_CFG_LEGACY;
>> -    else
>> -        return 0;
>> +    HPETState *s = opaque;
>> +
>> +    return s->config & HPET_CFG_LEGACY;
>>  }
>>
>>  static uint32_t timer_int_route(struct HPETTimer *timer)
>> @@ -54,9 +51,9 @@ static uint32_t timer_int_route(struct HPETTimer *timer)
>>      return route;
>>  }
>>
>> -static uint32_t hpet_enabled(void)
>> +static uint32_t hpet_enabled(HPETState *s)
>>  {
>> -    return hpet_statep->config & HPET_CFG_ENABLE;
>> +    return s->config & HPET_CFG_ENABLE;
>>  }
>>
>>  static uint32_t timer_is_periodic(HPETTimer *t)
>> @@ -106,10 +103,10 @@ static int deactivating_bit(uint64_t old,
>> uint64_t new, uint64_t mask)
>>      return ((old & mask) && !(new & mask));
>>  }
>>
>> -static uint64_t hpet_get_ticks(void)
>> +static uint64_t hpet_get_ticks(HPETState *s)
>>  {
>>      uint64_t ticks;
>> -    ticks = ns_to_ticks(qemu_get_clock(vm_clock) + hpet_statep->hpet_offset);
>> +    ticks = ns_to_ticks(qemu_get_clock(vm_clock) + s->hpet_offset);
>>      return ticks;
>>  }
>>
>> @@ -139,7 +136,7 @@ static void update_irq(struct HPETTimer *timer)
>>      qemu_irq irq;
>>      int route;
>>
>> -    if (timer->tn <= 1 && hpet_in_legacy_mode()) {
>> +    if (timer->tn <= 1 && hpet_in_legacy_mode(timer->state)) {
>>          /* if LegacyReplacementRoute bit is set, HPET specification requires
>>           * timer0 be routed to IRQ0 in NON-APIC or IRQ2 in the I/O APIC,
>>           * timer1 be routed to IRQ8 in NON-APIC or IRQ8 in the I/O APIC.
>> @@ -152,7 +149,7 @@ static void update_irq(struct HPETTimer *timer)
>>          route=timer_int_route(timer);
>>          irq=timer->state->irqs[route];
>>      }
>> -    if (timer_enabled(timer) && hpet_enabled()) {
>> +    if (timer_enabled(timer) && hpet_enabled(timer->state)) {
>>          qemu_irq_pulse(irq);
>>      }
>>  }
>> @@ -161,7 +158,7 @@ static void hpet_pre_save(void *opaque)
>>  {
>>      HPETState *s = opaque;
>>      /* save current counter value */
>> -    s->hpet_counter = hpet_get_ticks();
>> +    s->hpet_counter = hpet_get_ticks(s);
>>  }
>>
>>  static int hpet_post_load(void *opaque, int version_id)
>> @@ -216,7 +213,7 @@ static void hpet_timer(void *opaque)
>>      uint64_t diff;
>>
>>      uint64_t period = t->period;
>> -    uint64_t cur_tick = hpet_get_ticks();
>> +    uint64_t cur_tick = hpet_get_ticks(t->state);
>>
>>      if (timer_is_periodic(t) && period != 0) {
>>          if (t->config & HPET_TN_32BIT) {
>> @@ -244,7 +241,7 @@ static void hpet_set_timer(HPETTimer *t)
>>  {
>>      uint64_t diff;
>>      uint32_t wrap_diff;  /* how many ticks until we wrap? */
>> -    uint64_t cur_tick = hpet_get_ticks();
>> +    uint64_t cur_tick = hpet_get_ticks(t->state);
>>
>>      /* whenever new timer is being set up, make sure wrap_flag is 0 */
>>      t->wrap_flag = 0;
>> @@ -326,17 +323,19 @@ static uint32_t hpet_ram_readl(void *opaque,
>> target_phys_addr_t addr)
>>                  DPRINTF("qemu: invalid HPET_CFG + 4 hpet_ram_readl \n");
>>                  return 0;
>>              case HPET_COUNTER:
>> -                if (hpet_enabled())
>> -                    cur_tick = hpet_get_ticks();
>> -                else
>> +                if (hpet_enabled(s)) {
>> +                    cur_tick = hpet_get_ticks(s);
>> +                } else {
>>                      cur_tick = s->hpet_counter;
>> +                }
>>                  DPRINTF("qemu: reading counter  = %" PRIx64 "\n", cur_tick);
>>                  return cur_tick;
>>              case HPET_COUNTER + 4:
>> -                if (hpet_enabled())
>> -                    cur_tick = hpet_get_ticks();
>> -                else
>> +                if (hpet_enabled(s)) {
>> +                    cur_tick = hpet_get_ticks(s);
>> +                } else {
>>                      cur_tick = s->hpet_counter;
>> +                }
>>                  DPRINTF("qemu: reading counter + 4  = %" PRIx64 "\n",
>> cur_tick);
>>                  return cur_tick >> 32;
>>              case HPET_STATUS:
>> @@ -419,8 +418,9 @@ static void hpet_ram_writel(void *opaque,
>> target_phys_addr_t addr,
>>                                       | new_val;
>>                  }
>>                  timer->config &= ~HPET_TN_SETVAL;
>> -                if (hpet_enabled())
>> +                if (hpet_enabled(s)) {
>>                      hpet_set_timer(timer);
>> +                }
>>                  break;
>>              case HPET_TN_CMP + 4: // comparator register high order
>>                  DPRINTF("qemu: hpet_ram_writel HPET_TN_CMP + 4\n");
>> @@ -439,8 +439,9 @@ static void hpet_ram_writel(void *opaque,
>> target_phys_addr_t addr,
>>                                       | new_val << 32;
>>                  }
>>                  timer->config &= ~HPET_TN_SETVAL;
>> -                if (hpet_enabled())
>> +                if (hpet_enabled(s)) {
>>                      hpet_set_timer(timer);
>> +                }
>>                  break;
>>              case HPET_TN_ROUTE + 4:
>>                  DPRINTF("qemu: hpet_ram_writel HPET_TN_ROUTE + 4\n");
>> @@ -467,7 +468,7 @@ static void hpet_ram_writel(void *opaque,
>> target_phys_addr_t addr,
>>                  }
>>                  else if (deactivating_bit(old_val, new_val, HPET_CFG_ENABLE)) {
>>                      /* Halt main counter and disable interrupt generation. */
>> -                    s->hpet_counter = hpet_get_ticks();
>> +                    s->hpet_counter = hpet_get_ticks(s);
>>                      for (i = 0; i < HPET_NUM_TIMERS; i++)
>>                          hpet_del_timer(&s->timer[i]);
>>                  }
>> @@ -485,16 +486,18 @@ static void hpet_ram_writel(void *opaque,
>> target_phys_addr_t addr,
>>                  /* FIXME: need to handle level-triggered interrupts */
>>                  break;
>>              case HPET_COUNTER:
>> -               if (hpet_enabled())
>> -                   printf("qemu: Writing counter while HPET enabled!\n");
>> +                if (hpet_enabled(s)) {
>> +                    printf("qemu: Writing counter while HPET enabled!\n");
>
> Missed it in my series as well: Should become DPRINTF at this chance.
> Also below.

Well, I only touched the line because the indentation is off. Perhaps
included in your patch 'hpet: Coding style cleanups and some
refactorings'? The file should also be reindented if possible.

>
>> +                }
>>                 s->hpet_counter = (s->hpet_counter & 0xffffffff00000000ULL)
>>                                    | value;
>>                 DPRINTF("qemu: HPET counter written. ctr = %#x -> %"
>> PRIx64 "\n",
>>                          value, s->hpet_counter);
>>                 break;
>>              case HPET_COUNTER + 4:
>> -               if (hpet_enabled())
>> -                   printf("qemu: Writing counter while HPET enabled!\n");
>> +                if (hpet_enabled(s)) {
>> +                    printf("qemu: Writing counter while HPET enabled!\n");
>> +                }
>>                 s->hpet_counter = (s->hpet_counter & 0xffffffffULL)
>>                                    | (((uint64_t)value) << 32);
>>                 DPRINTF("qemu: HPET counter + 4 written. ctr = %#x ->
>> %" PRIx64 "\n",
>> @@ -563,15 +566,14 @@ static void hpet_reset(void *opaque) {
>>      count = 1;
>>  }
>>
>> -
>> -void hpet_init(qemu_irq *irq) {
>> +HPETState *hpet_init(qemu_irq *irq)
>> +{
>>      int i, iomemtype;
>>      HPETState *s;
>>
>>      DPRINTF ("hpet_init\n");
>>
>>      s = qemu_mallocz(sizeof(HPETState));
>> -    hpet_statep = s;
>>      s->irqs = irq;
>>      for (i=0; i<HPET_NUM_TIMERS; i++) {
>>          HPETTimer *timer = &s->timer[i];
>> @@ -583,4 +585,6 @@ void hpet_init(qemu_irq *irq) {
>>      iomemtype = cpu_register_io_memory(hpet_ram_read,
>>                                         hpet_ram_write, s);
>>      cpu_register_physical_memory(HPET_BASE, 0x400, iomemtype);
>> +
>> +    return s;
>>  }
>> diff --git a/hw/hpet_emul.h b/hw/hpet_emul.h
>> index cfd95b4..88dbf0d 100644
>> --- a/hw/hpet_emul.h
>> +++ b/hw/hpet_emul.h
>> @@ -75,8 +75,8 @@ typedef struct HPETState {
>>  } HPETState;
>>
>>  #if defined TARGET_I386
>> -extern uint32_t hpet_in_legacy_mode(void);
>> -extern void hpet_init(qemu_irq *irq);
>> +uint32_t hpet_in_legacy_mode(void *opaque);
>> +HPETState *hpet_init(qemu_irq *irq);
>>  #endif
>>
>>  #endif
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 5a703e1..9f1a9d6 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -83,7 +83,7 @@ static void rtc_irq_handler(IsaIrqState *isa, int level)
>>       * mode is established while interrupt is raised. We want it to
>>       * be lowered in any case.
>>       */
>> -    if (!hpet_in_legacy_mode() || !level) {
>> +    if ((isa->hpet_state && !hpet_in_legacy_mode(isa->hpet_state)) || !level) {
>>          isa_set_irq(isa, RTC_ISA_IRQ, level);
>>      }
>>  }
>> @@ -945,7 +945,7 @@ static void cpu_request_exit(void *opaque, int
>> irq, int level)
>>      }
>>  }
>>
>> -void pc_basic_device_init(qemu_irq *isa_irq,
>> +void pc_basic_device_init(qemu_irq *isa_irq, IsaIrqState *isa,
>>                            FDCtrl **floppy_controller,
>>                            ISADevice **rtc_state)
>>  {
>> @@ -966,8 +966,10 @@ void pc_basic_device_init(qemu_irq *isa_irq,
>>
>>      pit = pit_init(0x40, isa_reserve_irq(0));
>>      pcspk_init(pit);
>> +
>> +    isa->hpet_state = NULL;
>>      if (!no_hpet) {
>> -        hpet_init(isa_irq);
>> +        isa->hpet_state = hpet_init(isa_irq);
>>      }
>>
>>      for(i = 0; i < MAX_SERIAL_PORTS; i++) {
>> diff --git a/hw/pc.h b/hw/pc.h
>> index 73cccef..3e085b9 100644
>> --- a/hw/pc.h
>> +++ b/hw/pc.h
>> @@ -42,6 +42,7 @@ void irq_info(Monitor *mon);
>>  typedef struct isa_irq_state {
>>      qemu_irq *i8259;
>>      qemu_irq *ioapic;
>> +    void *hpet_state;
>>  } IsaIrqState;
>>
>>  void isa_irq_handler(void *opaque, int n, int level);
>> @@ -94,7 +95,7 @@ void pc_memory_init(ram_addr_t ram_size,
>>                      ram_addr_t *above_4g_mem_size_p);
>>  qemu_irq *pc_allocate_cpu_irq(void);
>>  void pc_vga_init(PCIBus *pci_bus);
>> -void pc_basic_device_init(qemu_irq *isa_irq,
>> +void pc_basic_device_init(qemu_irq *isa_irq, IsaIrqState *isa,
>>                            FDCtrl **floppy_controller,
>>                            ISADevice **rtc_state);
>>  void pc_init_ne2k_isa(NICInfo *nd);
>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
>> index 70f563a..1479a28 100644
>> --- a/hw/pc_piix.c
>> +++ b/hw/pc_piix.c
>> @@ -93,7 +93,8 @@ static void pc_init1(ram_addr_t ram_size,
>>      pc_vga_init(pci_enabled? pci_bus: NULL);
>>
>>      /* init basic PC hardware */
>> -    pc_basic_device_init(isa_irq, &floppy_controller, &rtc_state);
>> +    pc_basic_device_init(isa_irq, isa_irq_state, &floppy_controller,
>> +                         &rtc_state);
>>
>>      for(i = 0; i < nb_nics; i++) {
>>          NICInfo *nd = &nd_table[i];
>
> I have a longer hpet fix/qdev'ify/enhance series queued [1], held back
> until the device_show thing is through. Your patch break it, but it
> makes sense. Will rebase on top.

But I'd like to rebase my patches to at least 'hpet: Convert to qdev',
that would make the pc.h field part cleaner. Could you extract that
from the series and send it earlier?

>
> Jan
>
> [1] git://git.kiszka.org/qemu.git queues/hpet
>
>

Nice work, the patches look OK. Thanks also for the review!

Patch

diff --git a/hw/hpet.c b/hw/hpet.c
index 8729fb2..f24e054 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -37,14 +37,11 @@ 
 #define DPRINTF(...)
 #endif

-static HPETState *hpet_statep;
-
-uint32_t hpet_in_legacy_mode(void)
+uint32_t hpet_in_legacy_mode(void *opaque)
 {
-    if (hpet_statep)
-        return hpet_statep->config & HPET_CFG_LEGACY;
-    else
-        return 0;
+    HPETState *s = opaque;
+
+    return s->config & HPET_CFG_LEGACY;
 }

 static uint32_t timer_int_route(struct HPETTimer *timer)
@@ -54,9 +51,9 @@  static uint32_t timer_int_route(struct HPETTimer *timer)
     return route;
 }

-static uint32_t hpet_enabled(void)
+static uint32_t hpet_enabled(HPETState *s)
 {
-    return hpet_statep->config & HPET_CFG_ENABLE;
+    return s->config & HPET_CFG_ENABLE;
 }

 static uint32_t timer_is_periodic(HPETTimer *t)
@@ -106,10 +103,10 @@  static int deactivating_bit(uint64_t old,
uint64_t new, uint64_t mask)
     return ((old & mask) && !(new & mask));
 }

-static uint64_t hpet_get_ticks(void)
+static uint64_t hpet_get_ticks(HPETState *s)
 {
     uint64_t ticks;
-    ticks = ns_to_ticks(qemu_get_clock(vm_clock) + hpet_statep->hpet_offset);
+    ticks = ns_to_ticks(qemu_get_clock(vm_clock) + s->hpet_offset);
     return ticks;
 }

@@ -139,7 +136,7 @@  static void update_irq(struct HPETTimer *timer)
     qemu_irq irq;
     int route;

-    if (timer->tn <= 1 && hpet_in_legacy_mode()) {
+    if (timer->tn <= 1 && hpet_in_legacy_mode(timer->state)) {
         /* if LegacyReplacementRoute bit is set, HPET specification requires
          * timer0 be routed to IRQ0 in NON-APIC or IRQ2 in the I/O APIC,
          * timer1 be routed to IRQ8 in NON-APIC or IRQ8 in the I/O APIC.
@@ -152,7 +149,7 @@  static void update_irq(struct HPETTimer *timer)
         route=timer_int_route(timer);
         irq=timer->state->irqs[route];
     }
-    if (timer_enabled(timer) && hpet_enabled()) {
+    if (timer_enabled(timer) && hpet_enabled(timer->state)) {
         qemu_irq_pulse(irq);
     }
 }
@@ -161,7 +158,7 @@  static void hpet_pre_save(void *opaque)
 {
     HPETState *s = opaque;
     /* save current counter value */
-    s->hpet_counter = hpet_get_ticks();
+    s->hpet_counter = hpet_get_ticks(s);
 }

 static int hpet_post_load(void *opaque, int version_id)
@@ -216,7 +213,7 @@  static void hpet_timer(void *opaque)
     uint64_t diff;

     uint64_t period = t->period;
-    uint64_t cur_tick = hpet_get_ticks();
+    uint64_t cur_tick = hpet_get_ticks(t->state);

     if (timer_is_periodic(t) && period != 0) {
         if (t->config & HPET_TN_32BIT) {
@@ -244,7 +241,7 @@  static void hpet_set_timer(HPETTimer *t)
 {
     uint64_t diff;
     uint32_t wrap_diff;  /* how many ticks until we wrap? */
-    uint64_t cur_tick = hpet_get_ticks();
+    uint64_t cur_tick = hpet_get_ticks(t->state);

     /* whenever new timer is being set up, make sure wrap_flag is 0 */
     t->wrap_flag = 0;
@@ -326,17 +323,19 @@  static uint32_t hpet_ram_readl(void *opaque,
target_phys_addr_t addr)
                 DPRINTF("qemu: invalid HPET_CFG + 4 hpet_ram_readl \n");
                 return 0;
             case HPET_COUNTER:
-                if (hpet_enabled())
-                    cur_tick = hpet_get_ticks();
-                else
+                if (hpet_enabled(s)) {
+                    cur_tick = hpet_get_ticks(s);
+                } else {
                     cur_tick = s->hpet_counter;
+                }
                 DPRINTF("qemu: reading counter  = %" PRIx64 "\n", cur_tick);
                 return cur_tick;
             case HPET_COUNTER + 4:
-                if (hpet_enabled())
-                    cur_tick = hpet_get_ticks();
-                else
+                if (hpet_enabled(s)) {
+                    cur_tick = hpet_get_ticks(s);
+                } else {
                     cur_tick = s->hpet_counter;
+                }
                 DPRINTF("qemu: reading counter + 4  = %" PRIx64 "\n",
cur_tick);
                 return cur_tick >> 32;
             case HPET_STATUS:
@@ -419,8 +418,9 @@  static void hpet_ram_writel(void *opaque,
target_phys_addr_t addr,
                                      | new_val;
                 }
                 timer->config &= ~HPET_TN_SETVAL;
-                if (hpet_enabled())
+                if (hpet_enabled(s)) {
                     hpet_set_timer(timer);
+                }
                 break;
             case HPET_TN_CMP + 4: // comparator register high order
                 DPRINTF("qemu: hpet_ram_writel HPET_TN_CMP + 4\n");
@@ -439,8 +439,9 @@  static void hpet_ram_writel(void *opaque,
target_phys_addr_t addr,
                                      | new_val << 32;
                 }
                 timer->config &= ~HPET_TN_SETVAL;
-                if (hpet_enabled())
+                if (hpet_enabled(s)) {
                     hpet_set_timer(timer);
+                }
                 break;
             case HPET_TN_ROUTE + 4:
                 DPRINTF("qemu: hpet_ram_writel HPET_TN_ROUTE + 4\n");
@@ -467,7 +468,7 @@  static void hpet_ram_writel(void *opaque,
target_phys_addr_t addr,
                 }
                 else if (deactivating_bit(old_val, new_val, HPET_CFG_ENABLE)) {
                     /* Halt main counter and disable interrupt generation. */
-                    s->hpet_counter = hpet_get_ticks();
+                    s->hpet_counter = hpet_get_ticks(s);
                     for (i = 0; i < HPET_NUM_TIMERS; i++)
                         hpet_del_timer(&s->timer[i]);
                 }
@@ -485,16 +486,18 @@  static void hpet_ram_writel(void *opaque,
target_phys_addr_t addr,
                 /* FIXME: need to handle level-triggered interrupts */
                 break;
             case HPET_COUNTER:
-               if (hpet_enabled())
-                   printf("qemu: Writing counter while HPET enabled!\n");
+                if (hpet_enabled(s)) {
+                    printf("qemu: Writing counter while HPET enabled!\n");
+                }
                s->hpet_counter = (s->hpet_counter & 0xffffffff00000000ULL)
                                   | value;
                DPRINTF("qemu: HPET counter written. ctr = %#x -> %"
PRIx64 "\n",
                         value, s->hpet_counter);
                break;
             case HPET_COUNTER + 4:
-               if (hpet_enabled())
-                   printf("qemu: Writing counter while HPET enabled!\n");
+                if (hpet_enabled(s)) {
+                    printf("qemu: Writing counter while HPET enabled!\n");
+                }
                s->hpet_counter = (s->hpet_counter & 0xffffffffULL)
                                   | (((uint64_t)value) << 32);
                DPRINTF("qemu: HPET counter + 4 written. ctr = %#x ->
%" PRIx64 "\n",
@@ -563,15 +566,14 @@  static void hpet_reset(void *opaque) {
     count = 1;
 }

-
-void hpet_init(qemu_irq *irq) {
+HPETState *hpet_init(qemu_irq *irq)
+{
     int i, iomemtype;
     HPETState *s;

     DPRINTF ("hpet_init\n");

     s = qemu_mallocz(sizeof(HPETState));
-    hpet_statep = s;
     s->irqs = irq;
     for (i=0; i<HPET_NUM_TIMERS; i++) {
         HPETTimer *timer = &s->timer[i];
@@ -583,4 +585,6 @@  void hpet_init(qemu_irq *irq) {
     iomemtype = cpu_register_io_memory(hpet_ram_read,
                                        hpet_ram_write, s);
     cpu_register_physical_memory(HPET_BASE, 0x400, iomemtype);
+
+    return s;
 }
diff --git a/hw/hpet_emul.h b/hw/hpet_emul.h
index cfd95b4..88dbf0d 100644
--- a/hw/hpet_emul.h
+++ b/hw/hpet_emul.h
@@ -75,8 +75,8 @@  typedef struct HPETState {
 } HPETState;

 #if defined TARGET_I386
-extern uint32_t hpet_in_legacy_mode(void);
-extern void hpet_init(qemu_irq *irq);
+uint32_t hpet_in_legacy_mode(void *opaque);
+HPETState *hpet_init(qemu_irq *irq);
 #endif

 #endif
diff --git a/hw/pc.c b/hw/pc.c
index 5a703e1..9f1a9d6 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -83,7 +83,7 @@  static void rtc_irq_handler(IsaIrqState *isa, int level)
      * mode is established while interrupt is raised. We want it to
      * be lowered in any case.
      */
-    if (!hpet_in_legacy_mode() || !level) {
+    if ((isa->hpet_state && !hpet_in_legacy_mode(isa->hpet_state)) || !level) {
         isa_set_irq(isa, RTC_ISA_IRQ, level);
     }
 }
@@ -945,7 +945,7 @@  static void cpu_request_exit(void *opaque, int
irq, int level)
     }
 }

-void pc_basic_device_init(qemu_irq *isa_irq,
+void pc_basic_device_init(qemu_irq *isa_irq, IsaIrqState *isa,
                           FDCtrl **floppy_controller,
                           ISADevice **rtc_state)
 {
@@ -966,8 +966,10 @@  void pc_basic_device_init(qemu_irq *isa_irq,

     pit = pit_init(0x40, isa_reserve_irq(0));
     pcspk_init(pit);
+
+    isa->hpet_state = NULL;
     if (!no_hpet) {
-        hpet_init(isa_irq);
+        isa->hpet_state = hpet_init(isa_irq);
     }

     for(i = 0; i < MAX_SERIAL_PORTS; i++) {
diff --git a/hw/pc.h b/hw/pc.h
index 73cccef..3e085b9 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -42,6 +42,7 @@  void irq_info(Monitor *mon);
 typedef struct isa_irq_state {
     qemu_irq *i8259;
     qemu_irq *ioapic;
+    void *hpet_state;
 } IsaIrqState;

 void isa_irq_handler(void *opaque, int n, int level);
@@ -94,7 +95,7 @@  void pc_memory_init(ram_addr_t ram_size,
                     ram_addr_t *above_4g_mem_size_p);
 qemu_irq *pc_allocate_cpu_irq(void);
 void pc_vga_init(PCIBus *pci_bus);
-void pc_basic_device_init(qemu_irq *isa_irq,
+void pc_basic_device_init(qemu_irq *isa_irq, IsaIrqState *isa,
                           FDCtrl **floppy_controller,
                           ISADevice **rtc_state);
 void pc_init_ne2k_isa(NICInfo *nd);
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 70f563a..1479a28 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -93,7 +93,8 @@  static void pc_init1(ram_addr_t ram_size,
     pc_vga_init(pci_enabled? pci_bus: NULL);

     /* init basic PC hardware */
-    pc_basic_device_init(isa_irq, &floppy_controller, &rtc_state);
+    pc_basic_device_init(isa_irq, isa_irq_state, &floppy_controller,
+                         &rtc_state);

     for(i = 0; i < nb_nics; i++) {
         NICInfo *nd = &nd_table[i];