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

Alexander Graf
2008-10-29T03:44:16Z

Am 27.10.2008 um 08:07 schrieb 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)."

Basically IIRC on the ICH-7 the HPET base address is configured
indirectly by writing an address to the RCBA, which is mmio based
space configured in the LPC pci device config space.
Since we don't have an LPC device, but a PIIX ISA bridge, there was no
space to configure this on. That's why I faked and hardcoded some
parts here, as the OS should read the acpi tables to get the address
anyways.

Please double-check that information please, as I don't have the specs
with me atm.

Alex

>
>
> 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
>