* [Qemu-devel] [PATCH, RFC 2/4] hpet: don't use any static state
@ 2010-05-23 12:39 Blue Swirl
2010-05-23 15:40 ` [Qemu-devel] " Jan Kiszka
0 siblings, 1 reply; 3+ messages in thread
From: Blue Swirl @ 2010-05-23 12:39 UTC (permalink / raw)
To: qemu-devel
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)
{
- 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];
--
1.6.2.4
^ permalink raw reply related [flat|nested] 3+ messages in thread* [Qemu-devel] Re: [PATCH, RFC 2/4] hpet: don't use any static state
2010-05-23 12:39 [Qemu-devel] [PATCH, RFC 2/4] hpet: don't use any static state Blue Swirl
@ 2010-05-23 15:40 ` Jan Kiszka
2010-05-23 16:20 ` Blue Swirl
0 siblings, 1 reply; 3+ messages in thread
From: Jan Kiszka @ 2010-05-23 15:40 UTC (permalink / raw)
To: Blue Swirl; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 11817 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread* [Qemu-devel] Re: [PATCH, RFC 2/4] hpet: don't use any static state
2010-05-23 15:40 ` [Qemu-devel] " Jan Kiszka
@ 2010-05-23 16:20 ` Blue Swirl
0 siblings, 0 replies; 3+ messages in thread
From: Blue Swirl @ 2010-05-23 16:20 UTC (permalink / raw)
To: Jan Kiszka; +Cc: qemu-devel
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!
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-05-23 16:21 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-05-23 12:39 [Qemu-devel] [PATCH, RFC 2/4] hpet: don't use any static state Blue Swirl
2010-05-23 15:40 ` [Qemu-devel] " Jan Kiszka
2010-05-23 16:20 ` Blue Swirl
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).