[Qemu-devel] Re: [RFC][PATCH] Add HPET emulation to qemu

[Beth Kon]

On Sat, 2008-07-12 at 17:42 +0200, Alexander Graf wrote:
> Hi Beth,
> 
> On Jul 10, 2008, at 5:48 AM, Beth Kon wrote:
> 
> > This patch, based on an earlier patch by Alexander Graf, adds HPET
> > emulation to qemu. I am sending out a separate patch to kvm with the
> > required bios changes.
> >
> > This work is incomplete.
> 
> Wow it's good to see that someone's working on it. I am pretty sure  
> that you're basing on an older version of my HPET emulation, so you  
> might also want to take a look at the current patch file residing in 
> http://alex.csgraf.de/qemu/osxpatches.tar.bz2
<snip>
Hi Alex. Thanks for the feedback! Sorry for the delayed response, I've
been on vacation. I did check the patch you pointed me to and it is
actually the same one that I started with.
> 
> While reading through the code I realized how badly commented it is.  
> At least the functions should have some comments on them what their  
> purpose is.
> Furthermore there still are a lot of magic numbers in the code. While  
> that is "normal qemu code style" and I wrote it this way, I'm not too  
> fond of it. So it might be a good idea to at least make the switch  
> numbers defines.
> 
Ok... added those things to my todo list :-)

> >
> > The one area that feels ugly/wrong at the moment is handling the
> > disabling of 8254 and RTC timer interrupts when operating in legacy
> > mode. The HPET spec says "in this case the 8254/RTC timer will not  
> > cause
> > any interrupts". I'm not sure if I should disable the RTC/8254 in some
> > more general way, or just disable interrupts. Comments appreciated.
> 
> IIRC the spec defines that the HPET _can_ replace the 8254, but does  
> not have to. So you should be mostly fine on that part.
> 
> >

> > +
> > +//#define HPET_DEBUG
> > +
> > +#define HPET_BASE          0xfed00000
> 
> 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).
Yes, Ryan Harper's BIOS patch that was submitted with this patch
specified the HPET address in ACPI. I am not familiar with this stuff,
so not sure how that relates to the RCBA and whether more needs to be
done here. For the time being I'll add it to the todo list.

> 
> >
> > +#define HPET_PERIOD             0x00989680 /* 10000000 femptoseconds,
> > 10ns*/
> 
> Any reason why this is a hex value? I find 10000000 a lot easier to  
> read :-)
> 
Well that's a VERY good question! Job security? :-)

> >
> > 
> > +static uint32_t hpet_ram_readw(void *opaque, target_phys_addr_t addr)
> > +{
> > +#ifdef HPET_DEBUG
> > +    fprintf(stderr, "qemu: hpet_read w at %#lx\n", addr);
> > +#endif
> > +    return 10;
> > +}
> 
> If I'm not completely mistaken, all reads and writes need to be in 32-  
> or 64-bit mode. So it's pretty safe to remove these. I only added them  
> to see if Mac OS X actually would access them. To still enable other  
> people to do the same you might as well ifdef them out.
> 
Yep, you're right. I'll do that.
<snip>
> >
> > +
> > +static uint32_t hpet_ram_readl(void *opaque, target_phys_addr_t addr)
> > +{
> > +    HPETState *s = (HPETState *)opaque;
> > +#ifdef HPET_DEBUG
> > +  fprintf(stderr, "qemu: hpet_read l at %#lx\n", addr);
> > +#endif
> > +    switch(addr - HPET_BASE) {
> > +        case 0x00:
> > +            return 0x8086a201;
> > +        case 0x04:
> > +            return HPET_PERIOD;
> > +        case 0x10:
> > +            return ((s->legacy_route << 1) | s->enabled);
> > +        case 0x14:
> > +#ifdef HPET_DEBUG
> > +            fprintf(stderr, "qemu: invalid hpet_read l at %#lx\n",
> > addr);
> > +#endif
> > +            return 0;
> > +        case 0xf0:
> > +            s->hpet_counter = ns_to_ticks(qemu_get_clock(vm_clock)
> > +                                          - s->hpet_offset) ;
> 
> I'm having trouble understanding this part. The hpet_counter is  
> actually the ticks of the internal main clock of the HPET. This value  
> is actually supposed to constantly change wrt to the current time. The  
> "timers" in the HPET can now compare themselves to the "current value"  
> of the hpet_counter at all times, rising an interrupt if something  
> matches.
> 
> So far for the theory. I thought that it'd be a lot more convenient to  
> simply write down an internal offset (hpet_counter) to the actual  
> clock and base all calculations on that, so we can actually trigger a  
> timer based on event_time - offset. I don't see any reason to set  
> hpet_counter again, but maybe it's been too long that I've looked at  
> that code :).
> 
> Hum ... having looked further, is that what hpet_offset is supposed to  
> be and the reason you're setting s->updated?

Ok, let me explain my thinking. The hpet_counter tracks elapsed ticks,
but these ticks are emulated based on the value of the vm_clock. So to
find the elapsed ticks, I have to determine how many nanoseconds have
elapsed since the hpet_counter was enabled by software, then translate
nanoseconds to ticks. The number of elapsed nanoseconds equals the
current value of the vm_clock minus the value of the vm_clock when the
hpet_counter was last updated and enabled by software
(qemu_get_clock(vm_clock)-s->hpet_offset). s->updated marks when
software has updated the value of the counter, so that when the counter
is next enabled, hpet_offset (current value of vm_clock) can be saved. I
need to add code to handle the HPET being disabled then enabled without
the counter value being updated. This should just stop the clock
temporarily, so will require different modification to hpet_offset than
the case where the value of hpet_counter has been changed.

Whenever the hpet counter is read, it should return the current value of
elapsed ticks, which must be derived, as described above, from
qemu_get_clock. This is the only way for the guest to perceive that the
clock ticks are occurring.

Looking over the code though, I realize I oversimplified the comparator
stuff a little too much. The comparator doesn't necessarily equal a
delta from the current time. I'll need to translate the comparator value
to ticks and compare to the latest hpet_counter value to find the
"delta". A bit messy but I see no way around it.


> 
> >
<snip>
> > +    switch(addr - HPET_BASE) {
> > +        case 0x00:
> > +            return;
> > +        case 0x10:
> > +            if(value <= 3) {
> 
> If I may cite Avi:Algorithmic and Bit-operations don't mix well. I  
> don't really see why we shouldn't interpret the first two bits when  
> any bit > 1 is set. If I read the spec correctly, the values are  
> reserved and should not be used, don't really make anything fail for  
> now though. This check would make the operation a noop though! A  
> warning would be nice though.
> 
> Looking at my code, it seems I did the same - so blame me if anyone  
> asks :-).
> 
Yeah, I just took that from you :-) But I agree. I'll change it.
> >
> > +                s->enabled = value & 1;
> > +                if (s->enabled && s->updated) {
> > +                    /* subtract hpet_counter in case it was set to a
> > non-zero
> > +                       value */
> > +                    s->hpet_offset = qemu_get_clock(vm_clock)
> > +                                     - ticks_to_ns(s->hpet_counter);
> > +                    s->updated = 0;
> > +                }
> > +                s->legacy_route = (value >> 1) & 1;
> > +                hpet_legacy = s->legacy_route;
> 
> Is this what real hardware does? As soon as the HPET is there and  
> someone sets a random bit on it suddenly other completely unrelated  
> devices cease to work? I kinda doubt that.
I don't know what real hardware does. But the spec says if legacy
replacement option is set, "In this case, the 8254 timer will not cause
any interrupts." and "In this case the RTC will not cause any
interrupts". The spec also has a figure (figure 7) with the LEG_RT_EN
bit controlling a multiplexer between the hpet and the 8254. So that
would appear immediate to me. Any other ideas on how this should be
handled?

> 
<snip>
> > +    for(i=0; i<HPET_NUM_TIMERS; i++) {
> > +        HPETTimer *timer = &s->timer[i];
> > +        timer->comparator = 0xffffffff;
> > +        timer->state = s;
> > +        timer->timer = qemu_new_timer(vm_clock, hpet_timer, s->timer
> > +i);
> > +        switch(i) {
> > +            case 0:
> > +                timer->apic_port = 0;
> 
> Isn't this IRQ2?
Well, the weird thing is that both 0 and 2 work. I see in the qemu code
that the i8259 interrupts get routed through the apic (alt_irq_func) but
haven't dug into the details and don't understand why both work. But
since the "initial" mapping is to the 8259, 0 seemed appropriate. I'll
add a note to look into this further. 
> 
> [snip]
> 
> Alex
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to address@hidden
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Elizabeth Kon (Beth)
IBM Linux Technology Center
Open Hypervisor Team
email: address@hidden