[Qemu-devel] [PATCH] HPET fixes for reg writes

[Qemu-devel] [PATCH] HPET fixes for reg writes

[Beth Kon]

This patch addresses the problems found by Andriy Gapon: 

- The code was incorrectly overwriting the high order 32 
  bits of the timer and hpet config registers. This didn't show up
  in testing because linux and windows use hpet in legacy mode,
  where the high order 32 bits (advertising available interrupts)
  of the timer config register are ignored, and the high order 32
  bits of the hpet config register are reserved and unused.


- The mask for level-triggered interrupts was off by a bit. (hpet
  doesn't currently support level-triggered interrupts).

In addition, I removed some unused #defines, and corrected the ioapic
interrupt values advertised. I'd set this up early in hpet development
and never went back to correct it, and no bugs resulted since linux and
windows use hpet in legacy mode where available interrupts are ignored.




Signed-off-by: Beth Kon <eak@us.ibm.com>
---

diff --git a/hw/hpet.c b/hw/hpet.c
index 24aee6a..01b10aa 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -371,7 +371,7 @@ static void hpet_ram_writel(void *opaque, target_phys_addr_t addr,
 {
     int i;
     HPETState *s = (HPETState *)opaque;
-    uint64_t old_val, new_val, index;
+    uint64_t old_val, new_val, val, index;
 
     dprintf("qemu: Enter hpet_ram_writel at %" PRIx64 " = %#x\n", addr, value);
     index = addr;
@@ -387,8 +387,8 @@ static void hpet_ram_writel(void *opaque, target_phys_addr_t addr,
         switch ((addr - 0x100) % 0x20) {
             case HPET_TN_CFG:
                 dprintf("qemu: hpet_ram_writel HPET_TN_CFG\n");
-                timer->config = hpet_fixup_reg(new_val, old_val, 
-                                               HPET_TN_CFG_WRITE_MASK);
+                val = hpet_fixup_reg(new_val, old_val, HPET_TN_CFG_WRITE_MASK);
+                timer->config = (timer->config & 0xffffffff00000000ULL) | val;
                 if (new_val & HPET_TN_32BIT) {
                     timer->cmp = (uint32_t)timer->cmp;
                     timer->period = (uint32_t)timer->period;
@@ -456,8 +456,8 @@ static void hpet_ram_writel(void *opaque, target_phys_addr_t addr,
             case HPET_ID:
                 return;
             case HPET_CFG:
-                s->config = hpet_fixup_reg(new_val, old_val, 
-                                           HPET_CFG_WRITE_MASK);
+                val = hpet_fixup_reg(new_val, old_val, HPET_CFG_WRITE_MASK);
+                s->config = (s->config & 0xffffffff00000000ULL) | val;
                 if (activating_bit(old_val, new_val, HPET_CFG_ENABLE)) {
                     /* Enable main counter and interrupt generation. */
                     s->hpet_offset = ticks_to_ns(s->hpet_counter)
@@ -541,8 +541,8 @@ static void hpet_reset(void *opaque) {
         timer->tn = i;
         timer->cmp = ~0ULL;
         timer->config =  HPET_TN_PERIODIC_CAP | HPET_TN_SIZE_CAP;
-        /* advertise availability of irqs 5,10,11 */
-        timer->config |=  0x00000c20ULL << 32;
+        /* advertise availability of ioapic inti2 */
+        timer->config |=  0x00000004ULL << 32;
         timer->state = s;
         timer->period = 0ULL;
         timer->wrap_flag = 0;
diff --git a/hw/hpet_emul.h b/hw/hpet_emul.h
index 60893b6..3258d8b 100644
--- a/hw/hpet_emul.h
+++ b/hw/hpet_emul.h
@@ -18,12 +18,7 @@
 
 #define FS_PER_NS 1000000
 #define HPET_NUM_TIMERS 3
-#define HPET_TIMER_TYPE_LEVEL 1
-#define HPET_TIMER_TYPE_EDGE 0
-#define HPET_TIMER_DELIVERY_APIC 0
-#define HPET_TIMER_DELIVERY_FSB 1
-#define HPET_TIMER_CAP_FSB_INT_DEL (1 << 15)
-#define HPET_TIMER_CAP_PER_INT (1 << 4)
+#define HPET_TIMER_TYPE_LEVEL 0x002
 
 #define HPET_CFG_ENABLE 0x001
 #define HPET_CFG_LEGACY 0x002

[Qemu-devel] Re: [PATCH] HPET fixes for reg writes

[Andriy Gapon]

on 24/07/2009 19:26 Beth Kon said the following:
> This patch addresses the problems found by Andriy Gapon: 
> 
> - The code was incorrectly overwriting the high order 32 
>   bits of the timer and hpet config registers. This didn't show up
>   in testing because linux and windows use hpet in legacy mode,
>   where the high order 32 bits (advertising available interrupts)
>   of the timer config register are ignored, and the high order 32
>   bits of the hpet config register are reserved and unused.
> 
> 
> - The mask for level-triggered interrupts was off by a bit. (hpet
>   doesn't currently support level-triggered interrupts).
> 
> In addition, I removed some unused #defines, and corrected the ioapic
> interrupt values advertised. I'd set this up early in hpet development
> and never went back to correct it, and no bugs resulted since linux and
> windows use hpet in legacy mode where available interrupts are ignored.
> 

Beth,

thanks a lot!

And a comment: it seems that the code doesn't verify interrupt configured by
software, it happily uses any interrupt even if it's not advertised in interrupt
capabilities. I know, the software should not do that, but it happens :)

Also, maybe it would be a good idea to keep support for some additional
interrupts? E.g. irq 10, 11 or some such, or maybe something >= 16 for APIC mode
(if possible at all). But I know that this needs some careful thinking to not
interfere with other emulated devices.

-- 
Andriy Gapon

Re: [Qemu-devel] Re: [PATCH] HPET fixes for reg writes

[Beth Kon]

Andriy Gapon wrote:
> on 24/07/2009 19:26 Beth Kon said the following:
>   
>> This patch addresses the problems found by Andriy Gapon: 
>>
>> - The code was incorrectly overwriting the high order 32 
>>   bits of the timer and hpet config registers. This didn't show up
>>   in testing because linux and windows use hpet in legacy mode,
>>   where the high order 32 bits (advertising available interrupts)
>>   of the timer config register are ignored, and the high order 32
>>   bits of the hpet config register are reserved and unused.
>>
>>
>> - The mask for level-triggered interrupts was off by a bit. (hpet
>>   doesn't currently support level-triggered interrupts).
>>
>> In addition, I removed some unused #defines, and corrected the ioapic
>> interrupt values advertised. I'd set this up early in hpet development
>> and never went back to correct it, and no bugs resulted since linux and
>> windows use hpet in legacy mode where available interrupts are ignored.
>>
>>     
>
> Beth,
>
> thanks a lot!
>
> And a comment: it seems that the code doesn't verify interrupt configured by
> software, it happily uses any interrupt even if it's not advertised in interrupt
> capabilities. I know, the software should not do that, but it happens :)
>   
Yes, that is absolutely a good suggestion. I will add a check for 
correctness in the event of future expansion of hpet emulation 
capabilities, but see below.
> Also, maybe it would be a good idea to keep support for some additional
> interrupts? E.g. irq 10, 11 or some such, or maybe something >= 16 for APIC mode
> (if possible at all). But I know that this needs some careful thinking to not
> interfere with other emulated devices.
>   
Your probing on this subject has made clear that this hpet 
implementation really supports only legacy mode, because there are no 
other interrupts available for non-legacy mode hpet timers.

QEMU currently supports only a single IOAPIC, with irqs 0-15 identity 
mapped on to it (with the exception of irq0->inti2 override), and no 
capability for IOAPIC interrupts above 15. And since the hpet 
implementation is currently edge-triggered, it can not share interrupt 
lines with PCI. This leaves no interrupts other than the legacy timer 
interrupts. I will submit another patch that will make this clear. I 
will reduce the number of available timers to 2 (the 2 legacy mode 
timers), and not advertise  any  IOAPIC interrupt capability.

This could all be changed with qemu enhancements of course, but at the 
moment, hpet legacy replacement mode appears to reasonably cover the 
requirements of linux and windows guests.