On Tue, 2008-10-21 at 10:21 -0500, Anthony Liguori wrote:
Beth Kon wrote:
<snip>
Thanks for the feedback, Anthony. I'll only respond where I have
specific comments. Otherwise, I agree to your suggestions and will
make
the changes.
<snip>
+ if(timer_enabled(timer) && hpet_enabled(timer->state)) {
+ qemu_irq_pulse(irq);
+ /* windows wants timer0 on irq2 and linux wants irq0,
+ * so we pulse both
+ */
+ if (do_ioapic)
+ qemu_irq_pulse(timer->state->irqs[2]);
This seems curious and not quite right. We should be able to detect
whether the HPET is being used in IO APIC mode and raise the
appropriate
interrupt instead of generating a spurious irq0 interrupt.
After digging further on this, it turns out that the need for the 2
interrupts was caused by what looks like a problem with the way qemu
is
generating interrupts for the ioapic. I will send out a separate patch
for that issue, and make the necessary changes in this hpet code.
+ }
+}
+
+static void hpet_save(QEMUFile *f, void *opaque)
+{
+ HPETState *s = opaque;
+ int i;
+ qemu_put_be64s(f, &s->config);
+ qemu_put_be64s(f, &s->isr);
+ /* save current counter value */
+ s->hpet_counter = hpet_get_ticks(s);
+ qemu_put_be64s(f, &s->hpet_counter);
+
+ for(i = 0; i < HPET_NUM_TIMERS; i++) {
+ qemu_put_8s(f, &s->timer[i].tn);
+ qemu_put_be64s(f, &s->timer[i].config);
+ qemu_put_be64s(f, &s->timer[i].cmp);
+ qemu_put_be64s(f, &s->timer[i].fsb);
+ qemu_put_be64s(f, &s->timer[i].period);
+ if (s->timer[i].qemu_timer) {
+ qemu_put_timer(f, s->timer[i].qemu_timer);
+ }
Would qemu_timer ever be NULL?
You're right... the answer is no. I'll fix that.
<snip>
+
+
+ diff = hpet_calculate_diff(t, cur_tick);
+ qemu_mod_timer(t->qemu_timer, qemu_get_clock(vm_clock)
+ + (int64_t)ticks_to_ns(diff));
May want to convert ticks_to_ns to take and return an int64_t. The
explicit casting could introduce very subtle bugs.
It seems better this way to me, since muldiv64 in ticks_to_ns takes
uint64_t.
The likelihood of diff being big enough to create a problem seems
small enough. Am I
missing something?
+ case HPET_COUNTER:
+ if (hpet_enabled(s))
+ cur_tick = hpet_get_ticks(s);
Any reason for hpet_get_ticks(s) to not have this check integrated
into it?
When the hpet is being disabled, we need to get the actual count,
even though the
hpet_enabled check would return false. So if I made this change it
would introduce an
ordering issue in the disable code (i.e., get the ticks before
setting the hpet to
disabled)
<snip>
+
+ /* XXX this is a dirty hack for HPET support w/o LPC
+ Actually this is a config descriptor for the RCBA */
What's the dirty hack?
This comment is left over from Alexander Graf's code. I'm not sure
why it is in this location and will I'll remove it. But
in comments on the first version of hpet code I produced, Alexander
said, regarding the fixed assignment of HPET_BASE:
"This is a dirty hack that I did to make Mac OS X happy. Actually
the HPET base address gets specified in the RCBA on the
LPC and is configured by the BIOS to point to a valid address, with
0xfed00000 being the default (IIRC if you write 0 to
the fields you end up with that address)."