[Qemu-devel] [PATCH 1/2] Make HPET Legacy Only

[Qemu-devel] [PATCH 1/2] Make HPET Legacy Only

[Beth Kon]

Because of unavailability of IOAPIC interrupt lines, HPET currently 
supports only legacy mode, where the legacy timer interrupt (inti2) is
used. This patch makes this explicit by reducing the number of timers 
supported by HPET to 2 (the 2 legacy timers) and advertising no
available IOAPIC interrupt lines.

This patch also adds a check, suggested by Andriy Gapon, to ensure that
the interrupt chosen by the guest is one of those advertised by the HPET
as available. This is currently a noop since HPET advertises none as 
available, but in the event of future expansion and availability of 
IOAPIC interrupt lines, or implementation of level-triggered interrupts
that can share interrupt lines, this code will be in place. 

This subset of HPET capability (legacy-only) is reasonable, since the 
basic operation of linux and windows only uses the HPET in legacy mode, 
at least that's what all my testing so far has shown. Only userspace
access to the hpet would use non-legacy timers.




diff --git a/hw/hpet.c b/hw/hpet.c
index 01b10aa..56fa27c 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -47,12 +47,16 @@ uint32_t hpet_in_legacy_mode(void)
         return 0;
 }
 
-static uint32_t timer_int_route(struct HPETTimer *timer)
-{
-    uint32_t route;
-    route = (timer->config & HPET_TN_INT_ROUTE_MASK) >> HPET_TN_INT_ROUTE_SHIFT;
-    return route;
-}
+/* future code for when non-legacy interrupts are supported
+ * (when spare ioapic interrupts are available)
+ * static uint32_t timer_int_route(struct HPETTimer *timer)
+ * {
+ *    uint32_t route;
+ *    route = (timer->config & HPET_TN_INT_ROUTE_CNF_MASK) 
+ *                              >> HPET_TN_INT_ROUTE_CNF_SHIFT;
+ *    return route;
+ *}
+ */
 
 static uint32_t hpet_enabled(void)
 {
@@ -137,7 +141,6 @@ static inline uint64_t hpet_calculate_diff(HPETTimer *t, uint64_t current)
 static void update_irq(struct HPETTimer *timer)
 {
     qemu_irq irq;
-    int route;
 
     if (timer->tn <= 1 && hpet_in_legacy_mode()) {
         /* if LegacyReplacementRoute bit is set, HPET specification requires
@@ -149,8 +152,15 @@ static void update_irq(struct HPETTimer *timer)
         } else
             irq=timer->state->irqs[8];
     } else {
-        route=timer_int_route(timer);
-        irq=timer->state->irqs[route];
+        /* hpet implementation does not currently support 
+         * non-legacy interrupts due to unavailability of 
+         * IOAPIC interrupts */
+        printf("qemu: hpet only supports legacy interrupts\n");
+        exit (-1);
+        /* future code for when non-legacy interrupts are supported
+         * replace above printf and exit with 2 lines below
+         * route=timer_int_route(timer);
+         * irq=timer->state->irqs[route]; */
     }
     if (timer_enabled(timer) && hpet_enabled()) {
         qemu_irq_pulse(irq);
@@ -372,6 +382,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, val, index;
+    uint32_t route_cap, route_req;
 
     dprintf("qemu: Enter hpet_ram_writel at %" PRIx64 " = %#x\n", addr, value);
     index = addr;
@@ -388,6 +399,14 @@ static void hpet_ram_writel(void *opaque, target_phys_addr_t addr,
             case HPET_TN_CFG:
                 dprintf("qemu: hpet_ram_writel HPET_TN_CFG\n");
                 val = hpet_fixup_reg(new_val, old_val, HPET_TN_CFG_WRITE_MASK);
+                route_cap = timer->config >> 32;
+                route_req = (val & HPET_TN_INT_ROUTE_CNF_MASK) >> 
+                                                  HPET_TN_INT_ROUTE_CNF_SHIFT;
+                /* check if requested route is included in advertised 
+                 * route capabilities. If  not, set route cnf to 0 */
+                if (!((route_cap >> route_req) & 1)) {
+                    val = val & ~HPET_TN_INT_ROUTE_CNF_MASK;
+                }
                 timer->config = (timer->config & 0xffffffff00000000ULL) | val;
                 if (new_val & HPET_TN_32BIT) {
                     timer->cmp = (uint32_t)timer->cmp;
@@ -542,7 +561,7 @@ static void hpet_reset(void *opaque) {
         timer->cmp = ~0ULL;
         timer->config =  HPET_TN_PERIODIC_CAP | HPET_TN_SIZE_CAP;
         /* advertise availability of ioapic inti2 */
-        timer->config |=  0x00000004ULL << 32;
+        timer->config |=  HPET_TN_INT_ROUTE_CAP << 32;
         timer->state = s;
         timer->period = 0ULL;
         timer->wrap_flag = 0;
@@ -550,8 +569,8 @@ static void hpet_reset(void *opaque) {
 
     s->hpet_counter = 0ULL;
     s->hpet_offset = 0ULL;
-    /* 64-bit main counter; 3 timers supported; LegacyReplacementRoute. */
-    s->capability = 0x8086a201ULL;
+    /* 64-bit main counter; 2 timers supported; LegacyReplacementRoute. */
+    s->capability = 0x8086a101ULL;
     s->capability |= ((HPET_CLK_PERIOD) << 32);
     s->config = 0ULL;
     if (count > 0)
diff --git a/hw/hpet_emul.h b/hw/hpet_emul.h
index 3258d8b..d7233aa 100644
--- a/hw/hpet_emul.h
+++ b/hw/hpet_emul.h
@@ -17,7 +17,7 @@
 #define HPET_CLK_PERIOD         10000000ULL /* 10000000 femtoseconds == 10ns*/
 
 #define FS_PER_NS 1000000
-#define HPET_NUM_TIMERS 3
+#define HPET_NUM_TIMERS 2
 #define HPET_TIMER_TYPE_LEVEL 0x002
 
 #define HPET_CFG_ENABLE 0x001
@@ -40,10 +40,12 @@
 #define HPET_TN_SIZE_CAP         0x020
 #define HPET_TN_SETVAL           0x040
 #define HPET_TN_32BIT            0x100
-#define HPET_TN_INT_ROUTE_MASK  0x3e00
-#define HPET_TN_CFG_WRITE_MASK  0x3f4e
-#define HPET_TN_INT_ROUTE_SHIFT      9
+#define HPET_TN_INT_ENB_CNF_MASK  0x004
+#define HPET_TN_INT_ROUTE_CNF_MASK  0x3e00
+#define HPET_TN_INT_ROUTE_CNF_SHIFT      9
+#define HPET_TN_INT_ROUTE_CAP   0x00000000ULL /* ioapic interrupts available for hpet non-legacy use */
 #define HPET_TN_INT_ROUTE_CAP_SHIFT 32
+#define HPET_TN_CFG_WRITE_MASK  0x3f4e
 #define HPET_TN_CFG_BITS_READONLY_OR_RESERVED 0xffff80b1U
 
 struct HPETState;

[Qemu-devel] [PATCH 2/2] Make HPET Legacy Only - BIOS change

[Beth Kon]

Change the number of advertised HPET timers
from 3 to 2. 

diff --git a/bios/rombios32.c b/bios/rombios32.c
index 3d15283..bb7c339 100644
--- a/bios/rombios32.c
+++ b/bios/rombios32.c
@@ -1904,7 +1904,7 @@ void acpi_bios_init(void)
     /* Note timer_block_id value must be kept in sync with value advertised by
      * emulated hpet
      */
-    hpet->timer_block_id = cpu_to_le32(0x8086a201);
+    hpet->timer_block_id = cpu_to_le32(0x8086a101);
     hpet->addr.address = cpu_to_le32(ACPI_HPET_ADDRESS);
     acpi_build_table_header((struct  acpi_table_header *)hpet,
                              "HPET", sizeof(*hpet), 1);

Re: [Qemu-devel] [PATCH 1/2] Make HPET Legacy Only

[Anthony Liguori]

Beth Kon wrote:
> Because of unavailability of IOAPIC interrupt lines, HPET currently 
> supports only legacy mode, where the legacy timer interrupt (inti2) is
> used. This patch makes this explicit by reducing the number of timers 
> supported by HPET to 2 (the 2 legacy timers) and advertising no
> available IOAPIC interrupt lines.
>
> This patch also adds a check, suggested by Andriy Gapon, to ensure that
> the interrupt chosen by the guest is one of those advertised by the HPET
> as available. This is currently a noop since HPET advertises none as 
> available, but in the event of future expansion and availability of 
> IOAPIC interrupt lines, or implementation of level-triggered interrupts
> that can share interrupt lines, this code will be in place. 
>
> This subset of HPET capability (legacy-only) is reasonable, since the 
> basic operation of linux and windows only uses the HPET in legacy mode, 
> at least that's what all my testing so far has shown. Only userspace
> access to the hpet would use non-legacy timers.
>
>
>
>
> diff --git a/hw/hpet.c b/hw/hpet.c
> index 01b10aa..56fa27c 100644
> --- a/hw/hpet.c
> +++ b/hw/hpet.c
> @@ -47,12 +47,16 @@ uint32_t hpet_in_legacy_mode(void)
>          return 0;
>  }
>  
> -static uint32_t timer_int_route(struct HPETTimer *timer)
> -{
> -    uint32_t route;
> -    route = (timer->config & HPET_TN_INT_ROUTE_MASK) >> HPET_TN_INT_ROUTE_SHIFT;
> -    return route;
> -}
> +/* future code for when non-legacy interrupts are supported
> + * (when spare ioapic interrupts are available)
> + * static uint32_t timer_int_route(struct HPETTimer *timer)
> + * {
> + *    uint32_t route;
> + *    route = (timer->config & HPET_TN_INT_ROUTE_CNF_MASK) 
> + *                              >> HPET_TN_INT_ROUTE_CNF_SHIFT;
> + *    return route;
> + *}
> + */
>   

Please don't introduce dead code.  It will just rot.  A comment would 
suffice.

Regards,

Anthony Liguori