[Qemu-devel] Re: [PATCH 1/2] Add HPET emulation to qemu (v3)

[Beth Kon <eak@us.ibm.com>]

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)."

But in other areas of qemu code I see base addresses being hardcoded and am not sure anything different needs to be done 
here. Comments?


<snip>
> 
> Regards,
> 
> Anthony Liguori
> 
-- 
Elizabeth Kon (Beth)
IBM Linux Technology Center
Open Hypervisor Team
email: eak@us.ibm.com