[RFC 0/8] Improvements to HPET interupts

[RFC 0/8] Improvements to HPET interupts

[Andrew Cooper]

This patch series is very RFC.  Some parts of the series are for debugging
purposes only, but I am open to including them formally if people think they
might be useful.

I have done the best I can at splitting out changes from the final patch, but
have mostly failed at identifying areas which can be pulled out in a
compile/bisect-safe way.

The "no-arat" command line option introduced by the first patch is required
for sensible testing of the patch.  There is very little hardware around which
is old enough to not have ARAT, but has MSI capable HPET channels; Any newer
hardware which is ARAT-capable will unconditionally use TSC deadline mode.

As far as functional testing goes, I have put as many ASSERT()s in as I think
are necessary, and have tested on a variety of hardware with Xen as idle as I
can make it (not even dom0 running).  The proportion of time spent in certain
C states (as reported by the 'c' debugkey) is consistent before and after the
change, and nothing has blown up.

I would appreciate any advice on where I might be able to split up the final
patch to make it clearer, and comments on the design/implementation.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>


Andrew Cooper (8):
  x86/timing: Command line parameter to disable ARAT
  x86/acpi: Warn about multiple HPET tables
  x86/hpet: Fix ambiguity in broadcast info message.
  x86/hpet: Debug and verbose hpet logging
  x86/msi: Refactor msi_compose_message() to not requrie an irq_desc
  x86/hpet: Adjust pointer vs array semantics of hpet_boot_cfg
  x86/hpet: debug keyhandlers
  x86/hpet: Use singe apic vector rather than irq_descs for HPET
    interrupts

 xen/arch/x86/acpi/boot.c            |   11 +
 xen/arch/x86/cpu/amd.c              |    2 +-
 xen/arch/x86/cpu/common.c           |    3 +
 xen/arch/x86/cpu/cpu.h              |    2 +
 xen/arch/x86/cpu/intel.c            |    5 +-
 xen/arch/x86/hpet.c                 |  596 +++++++++++++++++------------------
 xen/arch/x86/msi.c                  |    9 +-
 xen/drivers/passthrough/vtd/iommu.c |    2 +-
 xen/include/asm-x86/msi.h           |    2 +-
 9 files changed, 318 insertions(+), 314 deletions(-)

-- 
1.7.10.4

[RFC 1/8] x86/timing: Command line parameter to disable ARAT

[Andrew Cooper]

---
 xen/arch/x86/cpu/amd.c    |    2 +-
 xen/arch/x86/cpu/common.c |    3 +++
 xen/arch/x86/cpu/cpu.h    |    2 ++
 xen/arch/x86/cpu/intel.c  |    5 +++--
 4 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 27b7f71..8d86d98 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -502,7 +502,7 @@ static void __devinit init_amd(struct cpuinfo_x86 *c)
 	 * Family 0x12 and above processors have APIC timer
 	 * running in deep C states.
 	 */
-	if (c->x86 > 0x11)
+	if ( opt_arat && c->x86 > 0x11)
 		set_bit(X86_FEATURE_ARAT, c->x86_capability);
 
 	/*
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index e1220e6..7e829b2 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -18,6 +18,9 @@
 static bool_t __cpuinitdata use_xsave = 1;
 boolean_param("xsave", use_xsave);
 
+bool_t __cpuinitdata opt_arat = 1;
+boolean_param("arat", opt_arat);
+
 unsigned int __devinitdata opt_cpuid_mask_ecx = ~0u;
 integer_param("cpuid_mask_ecx", opt_cpuid_mask_ecx);
 unsigned int __devinitdata opt_cpuid_mask_edx = ~0u;
diff --git a/xen/arch/x86/cpu/cpu.h b/xen/arch/x86/cpu/cpu.h
index 72c41ca..dde4d3a 100644
--- a/xen/arch/x86/cpu/cpu.h
+++ b/xen/arch/x86/cpu/cpu.h
@@ -15,6 +15,8 @@ extern unsigned int opt_cpuid_mask_ecx, opt_cpuid_mask_edx;
 extern unsigned int opt_cpuid_mask_xsave_eax;
 extern unsigned int opt_cpuid_mask_ext_ecx, opt_cpuid_mask_ext_edx;
 
+extern bool_t opt_arat;
+
 extern int get_model_name(struct cpuinfo_x86 *c);
 extern void display_cacheinfo(struct cpuinfo_x86 *c);
 
diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
index 31b69c9..fa0851b 100644
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -226,8 +226,9 @@ static void __devinit init_intel(struct cpuinfo_x86 *c)
 		set_bit(X86_FEATURE_NONSTOP_TSC, c->x86_capability);
 		set_bit(X86_FEATURE_TSC_RELIABLE, c->x86_capability);
 	}
-	if ((c->cpuid_level >= 0x00000006) &&
-	    (cpuid_eax(0x00000006) & (1u<<2)))
+	if ( opt_arat &&
+	     (c->cpuid_level >= 0x00000006) &&
+	     (cpuid_eax(0x00000006) & (1u<<2)))
 		set_bit(X86_FEATURE_ARAT, c->x86_capability);
 }
 
-- 
1.7.10.4

[RFC 2/8] x86/acpi: Warn about multiple HPET tables

[Andrew Cooper]

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/acpi/boot.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/xen/arch/x86/acpi/boot.c b/xen/arch/x86/acpi/boot.c
index df26423..63e5c3b 100644
--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -289,6 +289,17 @@ static int __init acpi_parse_hpet(struct acpi_table_header *table)
 		return -1;
 	}
 
+	/*
+	 * Some BIOSes provide multiple HPET tables. Warn that we will ignore
+	 * them.
+	 */
+	if ( hpet_address )
+	{
+		printk(KERN_WARNING PREFIX
+		       "Found multiple HPET tables. Only using first\n");
+		return -1;
+	}
+
 	hpet_address = hpet_tbl->address.address;
 	hpet_blockid = hpet_tbl->sequence;
 	printk(KERN_INFO PREFIX "HPET id: %#x base: %#lx\n",
-- 
1.7.10.4

[RFC 3/8] x86/hpet: Fix ambiguity in broadcast info message.

[Andrew Cooper]

"$N will be used for broadcast" is ambiguous between "$N timers" or "timer
$N", particuarly when N is 0.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/hpet.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index 99882b1..091e624 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -430,7 +430,7 @@ static void __init hpet_fsb_cap_lookup(void)
             num_hpets_used++;
     }
 
-    printk(XENLOG_INFO "HPET: %u timers (%u will be used for broadcast)\n",
+    printk(XENLOG_INFO "HPET: %u timers (%u timers used for broadcast)\n",
            num_chs, num_hpets_used);
 }
 
-- 
1.7.10.4

[RFC 4/8] x86/hpet: Debug and verbose hpet logging

[Andrew Cooper]

This was for debugging purposes, but might perhaps be more useful generally.
I am happy to keep none, some or all of it, depending on how useful people
think it might be.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/hpet.c |   83 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index 091e624..4b08381 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -61,6 +61,36 @@ u8 __initdata hpet_blockid;
 static bool_t __initdata force_hpet_broadcast;
 boolean_param("hpetbroadcast", force_hpet_broadcast);
 
+static bool_t __read_mostly hpet_verbose;
+static bool_t __read_mostly hpet_debug;
+static void __init parse_hpet_param(char * s)
+{
+    char *ss;
+    int val;
+
+    do {
+        val = !!strncmp(s, "no-", 3);
+        if ( !val )
+            s += 3;
+
+        ss = strchr(s, ',');
+        if ( ss )
+            *ss = '\0';
+
+        if ( !strcmp(s, "verbose") )
+            hpet_verbose = val;
+        else if ( !strcmp(s, "debug") )
+        {
+            hpet_debug = val;
+            if ( val )
+                hpet_verbose = 1;
+        }
+
+        s = ss + 1;
+    } while ( ss );
+}
+custom_param("hpet", parse_hpet_param);
+
 /*
  * Calculate a multiplication factor for scaled math, which is used to convert
  * nanoseconds based values to clock ticks:
@@ -94,6 +124,35 @@ static inline unsigned long ns2ticks(unsigned long nsec, int shift,
     return (unsigned long) tmp;
 }
 
+static void __maybe_unused dump_hpet_timer(int timer)
+{
+    u32 cfg = hpet_read32(HPET_Tn_CFG(timer));
+
+    printk(XENLOG_INFO "HPET: Timer %02u CFG: raw 0x%08"PRIx32
+           " Caps: %d %c%c", timer, cfg,
+           cfg & HPET_TN_64BIT_CAP ? 64 : 32,
+           cfg & HPET_TN_FSB_CAP ? 'M' : '-',
+           cfg & HPET_TN_PERIODIC_CAP ? 'P' : '-');
+
+    printk("\n  Setup: ");
+
+    if ( (cfg & HPET_TN_FSB_CAP) && (cfg & HPET_TN_FSB) )
+        printk("FSB ");
+
+    if ( !(cfg & HPET_TN_FSB) )
+        printk("GSI %#x ",
+               (cfg & HPET_TN_ROUTE) >> HPET_TN_ROUTE_SHIFT);
+
+    if ( cfg & HPET_TN_32BIT )
+        printk("32bit ");
+
+    if ( cfg & HPET_TN_PERIODIC )
+        printk("Periodic ");
+
+    printk("%sabled ", cfg & HPET_TN_ENABLE ? "En" : "Dis");
+    printk("%s\n", cfg & HPET_TN_LEVEL ? "Level" : "Edge");
+}
+
 static int hpet_next_event(unsigned long delta, int timer)
 {
     uint32_t cnt, cmp;
@@ -769,7 +828,14 @@ u64 __init hpet_setup(void)
     unsigned int last;
 
     if ( hpet_rate )
+    {
+        if ( hpet_debug )
+            printk(XENLOG_DEBUG "HPET: Skipping re-setup\n");
         return hpet_rate;
+    }
+
+    if ( hpet_debug )
+        printk(XENLOG_DEBUG "HPET: Setting up hpet data\n");
 
     if ( hpet_address == 0 )
         return 0;
@@ -783,6 +849,20 @@ u64 __init hpet_setup(void)
         return 0;
     }
 
+    if ( hpet_verbose )
+    {
+        printk(XENLOG_INFO "HPET: Vendor: %04"PRIx16", Rev: %u, %u timers\n",
+               hpet_id >> HPET_ID_VENDOR_SHIFT,
+               hpet_id & HPET_ID_REV,
+               ((hpet_id & HPET_ID_NUMBER) >> HPET_ID_NUMBER_SHIFT) + 1);
+        printk(XENLOG_INFO "HPET:   Caps: ");
+        if ( hpet_id & HPET_ID_LEGSUP )
+            printk("Legacy ");
+        if ( hpet_id & HPET_ID_64BIT )
+            printk("64bit ");
+        printk("\n");
+    }
+
     /* Check for sane period (100ps <= period <= 100ns). */
     hpet_period = hpet_read32(HPET_PERIOD);
     if ( (hpet_period > 100000000) || (hpet_period < 100000) )
@@ -840,6 +920,9 @@ void hpet_resume(u32 *boot_cfg)
             cfg &= ~HPET_TN_RESERVED;
         }
         hpet_write32(cfg, HPET_Tn_CFG(i));
+
+        if ( hpet_verbose )
+            dump_hpet_timer(i);
     }
 
     cfg = hpet_read32(HPET_CFG);
-- 
1.7.10.4

[RFC 5/8] x86/msi: Refactor msi_compose_message() to not requrie an irq_desc

[Andrew Cooper]

Subsequent changes will cause HPET MSIs to not have an associated IRQ.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/hpet.c                 |    2 +-
 xen/arch/x86/msi.c                  |    9 ++++-----
 xen/drivers/passthrough/vtd/iommu.c |    2 +-
 xen/include/asm-x86/msi.h           |    2 +-
 4 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index 4b08381..2c39808 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -390,7 +390,7 @@ static int __hpet_setup_msi_irq(struct irq_desc *desc)
 {
     struct msi_msg msg;
 
-    msi_compose_msg(desc, &msg);
+    msi_compose_msg(desc->arch.vector, desc->arch.cpu_mask, &msg);
     return hpet_msi_write(desc->action->dev_id, &msg);
 }
 
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index b43c36a..284042e 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -124,13 +124,12 @@ static void msix_put_fixmap(struct arch_msix *msix, int idx)
 /*
  * MSI message composition
  */
-void msi_compose_msg(struct irq_desc *desc, struct msi_msg *msg)
+void msi_compose_msg(unsigned vector, const cpumask_t *cpu_mask, struct msi_msg *msg)
 {
     unsigned dest;
-    int vector = desc->arch.vector;
 
     memset(msg, 0, sizeof(*msg));
-    if ( !cpumask_intersects(desc->arch.cpu_mask, &cpu_online_map) ) {
+    if ( !cpumask_intersects(cpu_mask, &cpu_online_map) ) {
         dprintk(XENLOG_ERR,"%s, compose msi message error!!\n", __func__);
         return;
     }
@@ -138,7 +137,7 @@ void msi_compose_msg(struct irq_desc *desc, struct msi_msg *msg)
     if ( vector ) {
         cpumask_t *mask = this_cpu(scratch_mask);
 
-        cpumask_and(mask, desc->arch.cpu_mask, &cpu_online_map);
+        cpumask_and(mask, cpu_mask, &cpu_online_map);
         dest = cpu_mask_to_apicid(mask);
 
         msg->address_hi = MSI_ADDR_BASE_HI;
@@ -491,7 +490,7 @@ int __setup_msi_irq(struct irq_desc *desc, struct msi_desc *msidesc,
 
     desc->msi_desc = msidesc;
     desc->handler = handler;
-    msi_compose_msg(desc, &msg);
+    msi_compose_msg(desc->arch.vector, desc->arch.cpu_mask, &msg);
     return write_msi_msg(msidesc, &msg);
 }
 
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 2dbe97a..97d5b5e 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1039,7 +1039,7 @@ static void dma_msi_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
         return;
     }
 
-    msi_compose_msg(desc, &msg);
+    msi_compose_msg(desc->arch.vector, desc->arch.cpu_mask, &msg);
     /* Are these overrides really needed? */
     if (x2apic_enabled)
         msg.address_hi = dest & 0xFFFFFF00;
diff --git a/xen/include/asm-x86/msi.h b/xen/include/asm-x86/msi.h
index 9eeef63..8c67ca8 100644
--- a/xen/include/asm-x86/msi.h
+++ b/xen/include/asm-x86/msi.h
@@ -231,7 +231,7 @@ struct arch_msix {
 };
 
 void early_msi_init(void);
-void msi_compose_msg(struct irq_desc *, struct msi_msg *);
+void msi_compose_msg(unsigned vector, const cpumask_t *mask, struct msi_msg *);
 void __msi_set_enable(u16 seg, u8 bus, u8 slot, u8 func, int pos, int enable);
 void mask_msi_irq(struct irq_desc *);
 void unmask_msi_irq(struct irq_desc *);
-- 
1.7.10.4

[RFC 6/8] x86/hpet: Adjust pointer vs array semantics of hpet_boot_cfg

[Andrew Cooper]

There should be no functional change as a result, but hpet_boot_cfg is an
array, not a pointer.  Code it as such.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/hpet.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index 2c39808..e8a3f66 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -53,6 +53,9 @@ DEFINE_PER_CPU(struct hpet_event_channel *, cpu_bc_channel);
 unsigned long __initdata hpet_address;
 u8 __initdata hpet_blockid;
 
+/* BIOS HPET configuration */
+static u32 *hpet_boot_cfg;
+
 /*
  * force_hpet_broadcast: by default legacy hpet broadcast will be stopped
  * if RTC interrupts are enabled. Enable this option if want to always enable
@@ -803,8 +806,8 @@ void hpet_broadcast_exit(void)
 
 int hpet_broadcast_is_available(void)
 {
-    return ((hpet_events && (hpet_events->flags & HPET_EVT_LEGACY))
-            || num_hpets_used > 0);
+    return ( num_hpets_used > 0 ||
+             (hpet_events && (hpet_events->flags & HPET_EVT_LEGACY)) );
 }
 
 int hpet_legacy_irq_tick(void)
@@ -819,8 +822,6 @@ int hpet_legacy_irq_tick(void)
     return 1;
 }
 
-static u32 *hpet_boot_cfg;
-
 u64 __init hpet_setup(void)
 {
     static u64 __initdata hpet_rate;
@@ -893,7 +894,7 @@ void hpet_resume(u32 *boot_cfg)
 
     cfg = hpet_read32(HPET_CFG);
     if ( boot_cfg )
-        *boot_cfg = cfg;
+        boot_cfg[0] = cfg;
     cfg &= ~(HPET_CFG_ENABLE | HPET_CFG_LEGACY);
     if ( cfg )
     {
@@ -942,12 +943,12 @@ void hpet_disable(void)
         return;
     }
 
-    hpet_write32(*hpet_boot_cfg & ~HPET_CFG_ENABLE, HPET_CFG);
+    hpet_write32(hpet_boot_cfg[0] & ~HPET_CFG_ENABLE, HPET_CFG);
 
     id = hpet_read32(HPET_ID);
     for ( i = 0; i <= ((id & HPET_ID_NUMBER) >> HPET_ID_NUMBER_SHIFT); ++i )
         hpet_write32(hpet_boot_cfg[i + 1], HPET_Tn_CFG(i));
 
-    if ( *hpet_boot_cfg & HPET_CFG_ENABLE )
-        hpet_write32(*hpet_boot_cfg, HPET_CFG);
+    if ( hpet_boot_cfg[0] & HPET_CFG_ENABLE )
+        hpet_write32(hpet_boot_cfg[0], HPET_CFG);
 }
-- 
1.7.10.4

[RFC 7/8] x86/hpet: debug keyhandlers

[Andrew Cooper]

Debug key for dumping HPET state.
---
 xen/arch/x86/hpet.c |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index e8a3f66..c5468fa 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -22,6 +22,8 @@
 #define MAX_DELTA_NS MILLISECS(10*1000)
 #define MIN_DELTA_NS MICROSECS(20)
 
+#include <xen/keyhandler.h>
+
 #define HPET_EVT_USED_BIT    0
 #define HPET_EVT_USED       (1 << HPET_EVT_USED_BIT)
 #define HPET_EVT_DISABLE_BIT 1
@@ -822,6 +824,21 @@ int hpet_legacy_irq_tick(void)
     return 1;
 }
 
+static void do_hpet_dump_state(unsigned char key)
+{
+    unsigned i;
+    printk("'%c' pressed - dumping HPET state\n", key);
+
+    for ( i = 0; i < num_hpets_used; ++i )
+        dump_hpet_timer(i);
+}
+
+static struct keyhandler hpet_dump_state = {
+    .irq_callback = 0,
+    .u.fn = do_hpet_dump_state,
+    .desc = "Dump hpet state"
+};
+
 u64 __init hpet_setup(void)
 {
     static u64 __initdata hpet_rate;
@@ -879,6 +896,8 @@ u64 __init hpet_setup(void)
     hpet_rate = 1000000000000000ULL; /* 10^15 */
     (void)do_div(hpet_rate, hpet_period);
 
+    register_keyhandler('1', &hpet_dump_state);
+
     return hpet_rate;
 }
 
-- 
1.7.10.4

[RFC 8/8] x86/hpet: Use singe apic vector rather than irq_descs for HPET interrupts

[Andrew Cooper]

This involves rewriting most of the MSI related HPET code, and as a result
this patch looks very complicated.  It is probably best viewed as an end
result, with the following notes explaining what is going on.

The new logic is as follows:
 * A single high priority vector is allocated and uses on all cpus.
 * Reliance on the irq infrastructure is completely removed.
 * Tracking of free hpet channels has changed.  It is now an individual
   bitmap, and allocation is based on winning a test_and_clear_bit()
   operation.
 * There is a notion of strict ownership of hpet channels.
 ** A cpu which owns an HPET channel can program it for a desired deadline.
 ** A cpu which can't find a free HPET channel to own may register for being
    woken up by another in-use HPET which will fire at an appropriate time.
 * Some functions have been renamed to be more descriptive.  Some functions
   have parameters changed to be more consistent.
 * Any function with a __hpet prefix expectes the appropriate lock to be held.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/hpet.c |  477 +++++++++++++++++++--------------------------------
 1 file changed, 181 insertions(+), 296 deletions(-)

diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index c5468fa..2f4d880 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -4,54 +4,59 @@
  * HPET management.
  */
 
-#include <xen/config.h>
+#include <xen/lib.h>
+#include <xen/init.h>
+#include <xen/cpuidle.h>
 #include <xen/errno.h>
-#include <xen/time.h>
-#include <xen/timer.h>
-#include <xen/smp.h>
 #include <xen/softirq.h>
-#include <xen/irq.h>
-#include <xen/numa.h>
+
+#include <mach_apic.h>
+
 #include <asm/fixmap.h>
 #include <asm/div64.h>
 #include <asm/hpet.h>
-#include <asm/msi.h>
-#include <mach_apic.h>
-#include <xen/cpuidle.h>
 
 #define MAX_DELTA_NS MILLISECS(10*1000)
 #define MIN_DELTA_NS MICROSECS(20)
 
 #include <xen/keyhandler.h>
 
-#define HPET_EVT_USED_BIT    0
-#define HPET_EVT_USED       (1 << HPET_EVT_USED_BIT)
-#define HPET_EVT_DISABLE_BIT 1
+#define HPET_EVT_DISABLE_BIT 0
 #define HPET_EVT_DISABLE    (1 << HPET_EVT_DISABLE_BIT)
-#define HPET_EVT_LEGACY_BIT  2
+#define HPET_EVT_LEGACY_BIT  1
 #define HPET_EVT_LEGACY     (1 << HPET_EVT_LEGACY_BIT)
 
 struct hpet_event_channel
 {
     unsigned long mult;
     int           shift;
+    unsigned int  flags; /* HPET_EVT_x */
     s_time_t      next_event;
-    cpumask_var_t cpumask;
+    cpumask_var_t cpumask; /* cpus wishing to be woken */
     spinlock_t    lock;
-    void          (*event_handler)(struct hpet_event_channel *);
-
     unsigned int idx;   /* physical channel idx */
     unsigned int cpu;   /* msi target */
     struct msi_desc msi;/* msi state */
-    unsigned int flags; /* HPET_EVT_x */
 } __cacheline_aligned;
 static struct hpet_event_channel *__read_mostly hpet_events;
 
 /* msi hpet channels used for broadcast */
 static unsigned int __read_mostly num_hpets_used;
 
-DEFINE_PER_CPU(struct hpet_event_channel *, cpu_bc_channel);
+/* High-priority vector for HPET interrupts */
+static u8 __read_mostly hpet_vector;
+
+/*
+ * HPET channel used for idling.  Either the HPET channel this cpu owns
+ * (indicated by channel->cpu pointing back), or the HPET channel belonging to
+ * another cpu with which we have requested to be woken.
+ */
+static DEFINE_PER_CPU(struct hpet_event_channel *, hpet_channel);
+
+/* Bitmask of currently-free HPET channels. */
+static uint32_t free_channels;
 
+/* Data from the HPET ACPI table */
 unsigned long __initdata hpet_address;
 u8 __initdata hpet_blockid;
 
@@ -158,7 +163,7 @@ static void __maybe_unused dump_hpet_timer(int timer)
     printk("%s\n", cfg & HPET_TN_LEVEL ? "Level" : "Edge");
 }
 
-static int hpet_next_event(unsigned long delta, int timer)
+static int __hpet_set_counter(struct hpet_event_channel *ch, int delta)
 {
     uint32_t cnt, cmp;
     unsigned long flags;
@@ -166,7 +171,7 @@ static int hpet_next_event(unsigned long delta, int timer)
     local_irq_save(flags);
     cnt = hpet_read32(HPET_COUNTER);
     cmp = cnt + delta;
-    hpet_write32(cmp, HPET_Tn_CMP(timer));
+    hpet_write32(cmp, HPET_Tn_CMP(ch->idx));
     cmp = hpet_read32(HPET_COUNTER);
     local_irq_restore(flags);
 
@@ -174,9 +179,8 @@ static int hpet_next_event(unsigned long delta, int timer)
     return ((cmp + 2 - cnt) > delta) ? -ETIME : 0;
 }
 
-static int reprogram_hpet_evt_channel(
-    struct hpet_event_channel *ch,
-    s_time_t expire, s_time_t now, int force)
+static int __program_hpet_time(struct hpet_event_channel *ch,
+                               s_time_t expire, s_time_t now, int force)
 {
     int64_t delta;
     int ret;
@@ -207,99 +211,50 @@ static int reprogram_hpet_evt_channel(
     delta = max_t(int64_t, delta, MIN_DELTA_NS);
     delta = ns2ticks(delta, ch->shift, ch->mult);
 
-    ret = hpet_next_event(delta, ch->idx);
+    ret = __hpet_set_counter(ch, delta);
     while ( ret && force )
     {
         delta += delta;
-        ret = hpet_next_event(delta, ch->idx);
+        ret = __hpet_set_counter(ch, delta);
     }
 
     return ret;
 }
 
-static void evt_do_broadcast(cpumask_t *mask)
+static void __hpet_wake_cpus(cpumask_t *mask)
 {
-    unsigned int cpu = smp_processor_id();
-
-    if ( cpumask_test_and_clear_cpu(cpu, mask) )
-        raise_softirq(TIMER_SOFTIRQ);
-
     cpuidle_wakeup_mwait(mask);
 
     if ( !cpumask_empty(mask) )
        cpumask_raise_softirq(mask, TIMER_SOFTIRQ);
 }
 
-static void handle_hpet_broadcast(struct hpet_event_channel *ch)
+static void __hpet_interrupt(struct hpet_event_channel *ch)
 {
-    cpumask_t mask;
-    s_time_t now, next_event;
-    unsigned int cpu;
-    unsigned long flags;
-
-    spin_lock_irqsave(&ch->lock, flags);
-
-again:
-    ch->next_event = STIME_MAX;
-
-    spin_unlock_irqrestore(&ch->lock, flags);
-
-    next_event = STIME_MAX;
-    cpumask_clear(&mask);
-    now = NOW();
-
-    /* find all expired events */
-    for_each_cpu(cpu, ch->cpumask)
-    {
-        s_time_t deadline;
-
-        rmb();
-        deadline = per_cpu(timer_deadline, cpu);
-        rmb();
-        if ( !cpumask_test_cpu(cpu, ch->cpumask) )
-            continue;
-
-        if ( deadline <= now )
-            cpumask_set_cpu(cpu, &mask);
-        else if ( deadline < next_event )
-            next_event = deadline;
-    }
-
-    /* wakeup the cpus which have an expired event. */
-    evt_do_broadcast(&mask);
-
-    if ( next_event != STIME_MAX )
-    {
-        spin_lock_irqsave(&ch->lock, flags);
-
-        if ( next_event < ch->next_event &&
-             reprogram_hpet_evt_channel(ch, next_event, now, 0) )
-            goto again;
-
-        spin_unlock_irqrestore(&ch->lock, flags);
-    }
+    __hpet_wake_cpus(ch->cpumask);
+    __program_hpet_time(ch, this_cpu(timer_deadline), NOW(), 1);
+    raise_softirq(TIMER_SOFTIRQ);
 }
 
-static void hpet_interrupt_handler(int irq, void *data,
-        struct cpu_user_regs *regs)
+static void hpet_interrupt_handler(struct cpu_user_regs *regs)
 {
-    struct hpet_event_channel *ch = (struct hpet_event_channel *)data;
-
-    this_cpu(irq_count)--;
+    unsigned int cpu = smp_processor_id();
+    struct hpet_event_channel *ch = this_cpu(hpet_channel);
 
-    if ( !ch->event_handler )
+    if ( ch )
     {
-        printk(XENLOG_WARNING "Spurious HPET timer interrupt on HPET timer %d\n", ch->idx);
-        return;
+        spin_lock(&ch->lock);
+        if ( ch->cpu == cpu )
+            __hpet_interrupt(ch);
+        spin_unlock(&ch->lock);
     }
 
-    ch->event_handler(ch);
+    ack_APIC_irq();
 }
 
-static void hpet_msi_unmask(struct irq_desc *desc)
+static void __hpet_msi_unmask(struct hpet_event_channel *ch)
 {
     u32 cfg;
-    struct hpet_event_channel *ch = desc->action->dev_id;
 
     cfg = hpet_read32(HPET_Tn_CFG(ch->idx));
     cfg |= HPET_TN_ENABLE;
@@ -307,10 +262,9 @@ static void hpet_msi_unmask(struct irq_desc *desc)
     ch->msi.msi_attrib.masked = 0;
 }
 
-static void hpet_msi_mask(struct irq_desc *desc)
+static void __hpet_msi_mask(struct hpet_event_channel *ch)
 {
     u32 cfg;
-    struct hpet_event_channel *ch = desc->action->dev_id;
 
     cfg = hpet_read32(HPET_Tn_CFG(ch->idx));
     cfg &= ~HPET_TN_ENABLE;
@@ -318,8 +272,10 @@ static void hpet_msi_mask(struct irq_desc *desc)
     ch->msi.msi_attrib.masked = 1;
 }
 
-static int hpet_msi_write(struct hpet_event_channel *ch, struct msi_msg *msg)
+static int __hpet_msi_write(struct hpet_event_channel *ch, struct msi_msg *msg)
 {
+    u32 cfg;
+
     ch->msi.msg = *msg;
 
     if ( iommu_intremap )
@@ -330,80 +286,33 @@ static int hpet_msi_write(struct hpet_event_channel *ch, struct msi_msg *msg)
             return rc;
     }
 
+    cfg = hpet_read32(HPET_Tn_CFG(ch->idx));
+    if ( cfg & HPET_TN_ENABLE )
+        hpet_write32(cfg & ~HPET_TN_ENABLE, HPET_Tn_CFG(ch->idx));
+
     hpet_write32(msg->data, HPET_Tn_ROUTE(ch->idx));
     hpet_write32(msg->address_lo, HPET_Tn_ROUTE(ch->idx) + 4);
 
-    return 0;
-}
-
-static void __maybe_unused
-hpet_msi_read(struct hpet_event_channel *ch, struct msi_msg *msg)
-{
-    msg->data = hpet_read32(HPET_Tn_ROUTE(ch->idx));
-    msg->address_lo = hpet_read32(HPET_Tn_ROUTE(ch->idx) + 4);
-    msg->address_hi = MSI_ADDR_BASE_HI;
-    if ( iommu_intremap )
-        iommu_read_msi_from_ire(&ch->msi, msg);
-}
+    if ( cfg & HPET_TN_ENABLE )
+        hpet_write32(cfg, HPET_Tn_CFG(ch->idx));
 
-static unsigned int hpet_msi_startup(struct irq_desc *desc)
-{
-    hpet_msi_unmask(desc);
     return 0;
 }
 
-#define hpet_msi_shutdown hpet_msi_mask
-
-static void hpet_msi_ack(struct irq_desc *desc)
-{
-    irq_complete_move(desc);
-    move_native_irq(desc);
-    ack_APIC_irq();
-}
-
-static void hpet_msi_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
-{
-    struct hpet_event_channel *ch = desc->action->dev_id;
-    struct msi_msg msg = ch->msi.msg;
-
-    msg.dest32 = set_desc_affinity(desc, mask);
-    if ( msg.dest32 == BAD_APICID )
-        return;
-
-    msg.data &= ~MSI_DATA_VECTOR_MASK;
-    msg.data |= MSI_DATA_VECTOR(desc->arch.vector);
-    msg.address_lo &= ~MSI_ADDR_DEST_ID_MASK;
-    msg.address_lo |= MSI_ADDR_DEST_ID(msg.dest32);
-    if ( msg.data != ch->msi.msg.data || msg.dest32 != ch->msi.msg.dest32 )
-        hpet_msi_write(ch, &msg);
-}
-
-/*
- * IRQ Chip for MSI HPET Devices,
- */
-static hw_irq_controller hpet_msi_type = {
-    .typename   = "HPET-MSI",
-    .startup    = hpet_msi_startup,
-    .shutdown   = hpet_msi_shutdown,
-    .enable	    = hpet_msi_unmask,
-    .disable    = hpet_msi_mask,
-    .ack        = hpet_msi_ack,
-    .set_affinity   = hpet_msi_set_affinity,
-};
-
-static int __hpet_setup_msi_irq(struct irq_desc *desc)
+static int __hpet_setup_msi(struct hpet_event_channel *ch)
 {
     struct msi_msg msg;
 
-    msi_compose_msg(desc->arch.vector, desc->arch.cpu_mask, &msg);
-    return hpet_msi_write(desc->action->dev_id, &msg);
+    ASSERT(ch->cpu != -1);
+
+    msi_compose_msg(hpet_vector, cpumask_of(ch->cpu), &msg);
+    return __hpet_msi_write(ch, &msg);
 }
 
-static int __init hpet_setup_msi_irq(struct hpet_event_channel *ch)
+static int __init hpet_setup_msi(struct hpet_event_channel *ch)
 {
     int ret;
-    u32 cfg = hpet_read32(HPET_Tn_CFG(ch->idx));
-    irq_desc_t *desc = irq_to_desc(ch->msi.irq);
+    u32 cfg;
 
     if ( iommu_intremap )
     {
@@ -414,14 +323,12 @@ static int __init hpet_setup_msi_irq(struct hpet_event_channel *ch)
     }
 
     /* set HPET Tn as oneshot */
+    cfg = hpet_read32(HPET_Tn_CFG(ch->idx));
     cfg &= ~(HPET_TN_LEVEL | HPET_TN_PERIODIC);
     cfg |= HPET_TN_FSB | HPET_TN_32BIT;
     hpet_write32(cfg, HPET_Tn_CFG(ch->idx));
 
-    desc->handler = &hpet_msi_type;
-    ret = request_irq(ch->msi.irq, hpet_interrupt_handler, "HPET", ch);
-    if ( ret >= 0 )
-        ret = __hpet_setup_msi_irq(desc);
+    ret = __hpet_setup_msi(ch);
     if ( ret < 0 )
     {
         if ( iommu_intremap )
@@ -429,25 +336,6 @@ static int __init hpet_setup_msi_irq(struct hpet_event_channel *ch)
         return ret;
     }
 
-    desc->msi_desc = &ch->msi;
-
-    return 0;
-}
-
-static int __init hpet_assign_irq(struct hpet_event_channel *ch)
-{
-    int irq;
-
-    if ( (irq = create_irq(NUMA_NO_NODE)) < 0 )
-        return irq;
-
-    ch->msi.irq = irq;
-    if ( hpet_setup_msi_irq(ch) )
-    {
-        destroy_irq(irq);
-        return -EINVAL;
-    }
-
     return 0;
 }
 
@@ -468,6 +356,8 @@ static void __init hpet_fsb_cap_lookup(void)
     if ( !hpet_events )
         return;
 
+    alloc_direct_apic_vector(&hpet_vector, hpet_interrupt_handler);
+
     for ( i = 0; i < num_chs && num_hpets_used < nr_cpu_ids; i++ )
     {
         struct hpet_event_channel *ch = &hpet_events[num_hpets_used];
@@ -490,7 +380,7 @@ static void __init hpet_fsb_cap_lookup(void)
         ch->flags = 0;
         ch->idx = i;
 
-        if ( hpet_assign_irq(ch) == 0 )
+        if ( hpet_setup_msi(ch) == 0 )
             num_hpets_used++;
     }
 
@@ -498,102 +388,28 @@ static void __init hpet_fsb_cap_lookup(void)
            num_chs, num_hpets_used);
 }
 
-static struct hpet_event_channel *hpet_get_channel(unsigned int cpu)
+/*
+ * Search for, and allocate, a free HPET channel.  Returns a pointer to the
+ * channel, or NULL in the case that none were free.  The caller is
+ * responsible for returning the channel to the free pool.
+ */
+static struct hpet_event_channel *hpet_get_free_channel(void)
 {
-    static unsigned int next_channel;
-    unsigned int i, next;
-    struct hpet_event_channel *ch;
-
-    if ( num_hpets_used == 0 )
-        return hpet_events;
+    unsigned ch, tries;
 
-    if ( num_hpets_used >= nr_cpu_ids )
-        return &hpet_events[cpu];
-
-    do {
-        next = next_channel;
-        if ( (i = next + 1) == num_hpets_used )
-            i = 0;
-    } while ( cmpxchg(&next_channel, next, i) != next );
-
-    /* try unused channel first */
-    for ( i = next; i < next + num_hpets_used; i++ )
+    for ( tries = num_hpets_used; tries; --tries )
     {
-        ch = &hpet_events[i % num_hpets_used];
-        if ( !test_and_set_bit(HPET_EVT_USED_BIT, &ch->flags) )
-        {
-            ch->cpu = cpu;
-            return ch;
-        }
-    }
-
-    /* share a in-use channel */
-    ch = &hpet_events[next];
-    if ( !test_and_set_bit(HPET_EVT_USED_BIT, &ch->flags) )
-        ch->cpu = cpu;
-
-    return ch;
-}
-
-static void set_channel_irq_affinity(struct hpet_event_channel *ch)
-{
-    struct irq_desc *desc = irq_to_desc(ch->msi.irq);
-
-    ASSERT(!local_irq_is_enabled());
-    spin_lock(&desc->lock);
-    hpet_msi_mask(desc);
-    hpet_msi_set_affinity(desc, cpumask_of(ch->cpu));
-    hpet_msi_unmask(desc);
-    spin_unlock(&desc->lock);
-
-    spin_unlock(&ch->lock);
-
-    /* We may have missed an interrupt due to the temporary masking. */
-    if ( ch->event_handler && ch->next_event < NOW() )
-        ch->event_handler(ch);
-}
-
-static void hpet_attach_channel(unsigned int cpu,
-                                struct hpet_event_channel *ch)
-{
-    ASSERT(!local_irq_is_enabled());
-    spin_lock(&ch->lock);
-
-    per_cpu(cpu_bc_channel, cpu) = ch;
-
-    /* try to be the channel owner again while holding the lock */
-    if ( !test_and_set_bit(HPET_EVT_USED_BIT, &ch->flags) )
-        ch->cpu = cpu;
-
-    if ( ch->cpu != cpu )
-        spin_unlock(&ch->lock);
-    else
-        set_channel_irq_affinity(ch);
-}
-
-static void hpet_detach_channel(unsigned int cpu,
-                                struct hpet_event_channel *ch)
-{
-    spin_lock_irq(&ch->lock);
-
-    ASSERT(ch == per_cpu(cpu_bc_channel, cpu));
+        if ( (ch = ffs(free_channels)) == 0 )
+            break;
 
-    per_cpu(cpu_bc_channel, cpu) = NULL;
+        --ch;
+        ASSERT(ch < num_hpets_used);
 
-    if ( cpu != ch->cpu )
-        spin_unlock_irq(&ch->lock);
-    else if ( cpumask_empty(ch->cpumask) )
-    {
-        ch->cpu = -1;
-        clear_bit(HPET_EVT_USED_BIT, &ch->flags);
-        spin_unlock_irq(&ch->lock);
-    }
-    else
-    {
-        ch->cpu = cpumask_first(ch->cpumask);
-        set_channel_irq_affinity(ch);
-        local_irq_enable();
+        if ( test_and_clear_bit(ch, &free_channels) )
+            return &hpet_events[ch];
     }
+
+    return NULL;
 }
 
 #include <asm/mc146818rtc.h>
@@ -630,6 +446,7 @@ void __init hpet_broadcast_init(void)
         /* Stop HPET legacy interrupts */
         cfg &= ~HPET_CFG_LEGACY;
         n = num_hpets_used;
+        free_channels = (1U << n) - 1;
     }
     else
     {
@@ -673,9 +490,8 @@ void __init hpet_broadcast_init(void)
         hpet_events[i].shift = 32;
         hpet_events[i].next_event = STIME_MAX;
         spin_lock_init(&hpet_events[i].lock);
-        wmb();
-        hpet_events[i].event_handler = handle_hpet_broadcast;
 
+        hpet_events[1].msi.irq = -1;
         hpet_events[i].msi.msi_attrib.maskbit = 1;
         hpet_events[i].msi.msi_attrib.pos = MSI_TYPE_HPET;
     }
@@ -715,9 +531,6 @@ void hpet_broadcast_resume(void)
 
     for ( i = 0; i < n; i++ )
     {
-        if ( hpet_events[i].msi.irq >= 0 )
-            __hpet_setup_msi_irq(irq_to_desc(hpet_events[i].msi.irq));
-
         /* set HPET Tn as oneshot */
         cfg = hpet_read32(HPET_Tn_CFG(hpet_events[i].idx));
         cfg &= ~(HPET_TN_LEVEL | HPET_TN_PERIODIC);
@@ -725,6 +538,7 @@ void hpet_broadcast_resume(void)
         if ( !(hpet_events[i].flags & HPET_EVT_LEGACY) )
             cfg |= HPET_TN_FSB;
         hpet_write32(cfg, HPET_Tn_CFG(hpet_events[i].idx));
+        __hpet_setup_msi(&hpet_events[i]);
 
         hpet_events[i].next_event = STIME_MAX;
     }
@@ -760,50 +574,116 @@ void hpet_disable_legacy_broadcast(void)
 void hpet_broadcast_enter(void)
 {
     unsigned int cpu = smp_processor_id();
-    struct hpet_event_channel *ch = per_cpu(cpu_bc_channel, cpu);
+    struct hpet_event_channel *ch = this_cpu(hpet_channel);
+    s_time_t deadline = this_cpu(timer_deadline);
+
+    ASSERT(!local_irq_is_enabled());
+    ASSERT(ch == NULL);
 
-    if ( per_cpu(timer_deadline, cpu) == 0 )
+    if ( deadline == 0 )
         return;
 
-    if ( !ch )
-        ch = hpet_get_channel(cpu);
+    ch = hpet_get_free_channel();
 
-    ASSERT(!local_irq_is_enabled());
+    if ( ch )
+    {
+        /*
+         * It is not intended to be possible to assign a legacy channel to a
+         * cpu, so getting here should be impossible.
+         */
+        ASSERT( !(ch->flags & HPET_EVT_LEGACY) );
 
-    if ( !(ch->flags & HPET_EVT_LEGACY) )
-        hpet_attach_channel(cpu, ch);
+        spin_lock(&ch->lock);
+
+        this_cpu(hpet_channel) = ch;
+        ch->cpu = cpu;
+        cpumask_set_cpu(cpu, ch->cpumask);
+
+        __hpet_setup_msi(ch);
+        __program_hpet_time(ch, deadline, NOW(), 1);
+        __hpet_msi_unmask(ch);
+
+        spin_unlock(&ch->lock);
+
+    }
+    else
+    {
+        /* TODO - this seems very ugly */
+        unsigned i;
+
+        for ( i = 0; i < num_hpets_used; ++i )
+        {
+            ch = &hpet_events[i];
+            spin_lock(&ch->lock);
+
+            if ( ch->cpu == -1 )
+                goto continue_search;
+
+            if ( ch->next_event >= deadline - MICROSECS(50) &&
+                 ch->next_event <= deadline )
+                break;
+
+        continue_search:
+            spin_unlock(&ch->lock);
+            ch = NULL;
+        }
+
+        if ( ch )
+        {
+            cpumask_set_cpu(cpu, ch->cpumask);
+            this_cpu(hpet_channel) = ch;
+            spin_unlock(&ch->lock);
+        }
+        else
+            this_cpu(timer_deadline) = NOW();
+
+    }
 
     /* Disable LAPIC timer interrupts. */
     disable_APIC_timer();
-    cpumask_set_cpu(cpu, ch->cpumask);
-
-    spin_lock(&ch->lock);
-    /* reprogram if current cpu expire time is nearer */
-    if ( per_cpu(timer_deadline, cpu) < ch->next_event )
-        reprogram_hpet_evt_channel(ch, per_cpu(timer_deadline, cpu), NOW(), 1);
-    spin_unlock(&ch->lock);
 }
 
 void hpet_broadcast_exit(void)
 {
     unsigned int cpu = smp_processor_id();
-    struct hpet_event_channel *ch = per_cpu(cpu_bc_channel, cpu);
+    struct hpet_event_channel *ch = this_cpu(hpet_channel);
 
-    if ( per_cpu(timer_deadline, cpu) == 0 )
-        return;
+    ASSERT(local_irq_is_enabled());
 
     if ( !ch )
-        ch = hpet_get_channel(cpu);
+        return;
 
-    /* Reprogram the deadline; trigger timer work now if it has passed. */
-    enable_APIC_timer();
-    if ( !reprogram_timer(per_cpu(timer_deadline, cpu)) )
-        raise_softirq(TIMER_SOFTIRQ);
+    if ( this_cpu(timer_deadline) == 0 )
+        return;
+
+    spin_lock_irq(&ch->lock);
 
     cpumask_clear_cpu(cpu, ch->cpumask);
 
-    if ( !(ch->flags & HPET_EVT_LEGACY) )
-        hpet_detach_channel(cpu, ch);
+    /* If we own the channel, detach it */
+    if ( ch->cpu == cpu )
+    {
+        __hpet_msi_mask(ch);
+
+        /*
+         * It is not intended to be possible to assign a legacy channel to a
+         * cpu, so getting here should be impossible.
+         */
+        ASSERT( !(ch->flags & HPET_EVT_LEGACY) );
+
+        __hpet_wake_cpus(ch->cpumask);
+        ch->cpu = -1;
+        set_bit(ch->idx, &free_channels);
+    }
+
+    this_cpu(hpet_channel) = NULL;
+
+    spin_unlock_irq(&ch->lock);
+
+    /* Reprogram the deadline; trigger timer work now if it has passed. */
+    enable_APIC_timer();
+    if ( !reprogram_timer(this_cpu(timer_deadline)) )
+        raise_softirq(TIMER_SOFTIRQ);
 }
 
 int hpet_broadcast_is_available(void)
@@ -820,7 +700,12 @@ int hpet_legacy_irq_tick(void)
          (hpet_events->flags & (HPET_EVT_DISABLE|HPET_EVT_LEGACY)) !=
          HPET_EVT_LEGACY )
         return 0;
-    hpet_events->event_handler(hpet_events);
+
+    /* TODO - Does this really make sense for legacy ticks ? */
+    spin_lock_irq(&hpet_events->lock);
+    __hpet_interrupt(hpet_events);
+    spin_unlock_irq(&hpet_events->lock);
+
     return 1;
 }
 
-- 
1.7.10.4

Re: [RFC 1/8] x86/timing: Command line parameter to disable ARAT

[Jan Beulich]

>>> On 04.11.13 at 19:54, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -18,6 +18,9 @@
>  static bool_t __cpuinitdata use_xsave = 1;
>  boolean_param("xsave", use_xsave);
>  
> +bool_t __cpuinitdata opt_arat = 1;
> +boolean_param("arat", opt_arat);

Two options: Either put this in a "#ifndef NDEBUG" section,
to make clear it's for debugging only. Or add it to
docs/misc/xen-command-line.markdown.

Beyond that - very much appreciated, as I too had to suppress
recognizing ARAT a number of times in the past.

Jan

Re: [RFC 2/8] x86/acpi: Warn about multiple HPET tables

[Jan Beulich]

>>> On 04.11.13 at 19:54, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/acpi/boot.c
> +++ b/xen/arch/x86/acpi/boot.c
> @@ -289,6 +289,17 @@ static int __init acpi_parse_hpet(struct acpi_table_header *table)
>  		return -1;
>  	}
>  
> +	/*
> +	 * Some BIOSes provide multiple HPET tables. Warn that we will ignore
> +	 * them.
> +	 */
> +	if ( hpet_address )
> +	{
> +		printk(KERN_WARNING PREFIX
> +		       "Found multiple HPET tables. Only using first\n");
> +		return -1;
> +	}

If there really are examples of this, and if those HPETs work
properly, perhaps we should rather make use of them?

Jan

Re: [RFC 3/8] x86/hpet: Fix ambiguity in broadcast info message.

[Jan Beulich]

>>> On 04.11.13 at 19:54, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> "$N will be used for broadcast" is ambiguous between "$N timers" or "timer
> $N", particuarly when N is 0.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> ---
>  xen/arch/x86/hpet.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
> index 99882b1..091e624 100644
> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -430,7 +430,7 @@ static void __init hpet_fsb_cap_lookup(void)
>              num_hpets_used++;
>      }
>  
> -    printk(XENLOG_INFO "HPET: %u timers (%u will be used for broadcast)\n",
> +    printk(XENLOG_INFO "HPET: %u timers (%u timers used for broadcast)\n",

If you already alter this (which I'm not convinced is necessary - I
personally never considered the message ambiguous), please also
change "used" to "usable". And then maybe you agree adding the
2nd "timers" becomes unnecessary.

Jan

Re: [RFC 5/8] x86/msi: Refactor msi_compose_message() to not requrie an irq_desc

[Jan Beulich]

>>> On 04.11.13 at 19:54, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> --- a/xen/include/asm-x86/msi.h
> +++ b/xen/include/asm-x86/msi.h
> @@ -231,7 +231,7 @@ struct arch_msix {
>  };
>  
>  void early_msi_init(void);
> -void msi_compose_msg(struct irq_desc *, struct msi_msg *);
> +void msi_compose_msg(unsigned vector, const cpumask_t *mask, struct msi_msg *);

Please be consistent here - either re-add a name for the last
parameter, or drop the name of the middle one. Reviewed-by
with that change.

Jan

Re: [RFC 6/8] x86/hpet: Adjust pointer vs array semantics of hpet_boot_cfg

[Jan Beulich]

>>> On 04.11.13 at 19:54, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> There should be no functional change as a result, but hpet_boot_cfg is an
> array, not a pointer.  Code it as such.

This is rather pointless a patch - there's nothing wrong with accessing
the first member of an array simply via unary *. It's less typing. Plus
there are changes in here that don't match up with subject and
description at all.

Jan

Re: [RFC 7/8] x86/hpet: debug keyhandlers

[Jan Beulich]

>>> On 04.11.13 at 19:54, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> @@ -879,6 +896,8 @@ u64 __init hpet_setup(void)
>      hpet_rate = 1000000000000000ULL; /* 10^15 */
>      (void)do_div(hpet_rate, hpet_period);
>  
> +    register_keyhandler('1', &hpet_dump_state);

I'd prefer the numbers to be left alone if at all possible. How
about 'E'?

Jan

Re: [RFC 7/8] x86/hpet: debug keyhandlers

[Andrew Cooper]

On 05/11/13 11:11, Jan Beulich wrote:
>>>> On 04.11.13 at 19:54, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> @@ -879,6 +896,8 @@ u64 __init hpet_setup(void)
>>      hpet_rate = 1000000000000000ULL; /* 10^15 */
>>      (void)do_div(hpet_rate, hpet_period);
>>  
>> +    register_keyhandler('1', &hpet_dump_state);
> I'd prefer the numbers to be left alone if at all possible. How
> about 'E'?
>
> Jan
>

I wasn't intending for this keyhandler to be committed, at least
certainly not taking the place of '1'  (As people might be able to guess
from similar debugging patches of mine, I regularly have many of the
numbers in use for temporary keyhandlers)

While it was very useful while working on this issue, I don't think it
would be particularly useful in general, especially given the increasing
scarcity of keys.

~Andrew

Re: [RFC 3/8] x86/hpet: Fix ambiguity in broadcast info message.

[David Vrabel]

On 05/11/13 11:02, Jan Beulich wrote:
>>>> On 04.11.13 at 19:54, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> "$N will be used for broadcast" is ambiguous between "$N timers" or "timer
>> $N", particuarly when N is 0.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <JBeulich@suse.com>
>> ---
>>  xen/arch/x86/hpet.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
>> index 99882b1..091e624 100644
>> --- a/xen/arch/x86/hpet.c
>> +++ b/xen/arch/x86/hpet.c
>> @@ -430,7 +430,7 @@ static void __init hpet_fsb_cap_lookup(void)
>>              num_hpets_used++;
>>      }
>>  
>> -    printk(XENLOG_INFO "HPET: %u timers (%u will be used for broadcast)\n",
>> +    printk(XENLOG_INFO "HPET: %u timers (%u timers used for broadcast)\n",
> 
> If you already alter this (which I'm not convinced is necessary - I
> personally never considered the message ambiguous), please also
> change "used" to "usable". And then maybe you agree adding the
> 2nd "timers" becomes unnecessary.

I agree with Andrew that the original wording is ambiguous.  It needs to
be clear that the second %u is a count, and not the subject of the
sub-clause.

My suggested wording would be:

"HPET: %u timers, %u are broadcast capable"

Or

"HPET: %u timers (%u are broadcast capable)"

If the broadcast capability is much less interesting that the overall
number of timers.

David

Re: [RFC 3/8] x86/hpet: Fix ambiguity in broadcast info message.

[Jan Beulich]

>>> On 05.11.13 at 14:28, David Vrabel <david.vrabel@citrix.com> wrote:
> On 05/11/13 11:02, Jan Beulich wrote:
>>>>> On 04.11.13 at 19:54, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> "$N will be used for broadcast" is ambiguous between "$N timers" or "timer
>>> $N", particuarly when N is 0.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> CC: Keir Fraser <keir@xen.org>
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> ---
>>>  xen/arch/x86/hpet.c |    2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
>>> index 99882b1..091e624 100644
>>> --- a/xen/arch/x86/hpet.c
>>> +++ b/xen/arch/x86/hpet.c
>>> @@ -430,7 +430,7 @@ static void __init hpet_fsb_cap_lookup(void)
>>>              num_hpets_used++;
>>>      }
>>>  
>>> -    printk(XENLOG_INFO "HPET: %u timers (%u will be used for broadcast)\n",
>>> +    printk(XENLOG_INFO "HPET: %u timers (%u timers used for broadcast)\n",
>> 
>> If you already alter this (which I'm not convinced is necessary - I
>> personally never considered the message ambiguous), please also
>> change "used" to "usable". And then maybe you agree adding the
>> 2nd "timers" becomes unnecessary.
> 
> I agree with Andrew that the original wording is ambiguous.  It needs to
> be clear that the second %u is a count, and not the subject of the
> sub-clause.
> 
> My suggested wording would be:
> 
> "HPET: %u timers, %u are broadcast capable"
> 
> Or
> 
> "HPET: %u timers (%u are broadcast capable)"
> 
> If the broadcast capability is much less interesting that the overall
> number of timers.

In fact all we care about are those timers that can be used for
broadcast. "Boradcast capable", however, doesn't properly
express things - timers aren't "capable of", they can only be
"used for" broadcasting.

How about "HPET: %u timers usable for broadcast (%u total)"?

Jan

Re: [RFC 3/8] x86/hpet: Fix ambiguity in broadcast info message.

[David Vrabel]

On 05/11/13 13:34, Jan Beulich wrote:
> 
> How about "HPET: %u timers usable for broadcast (%u total)"?

This works for me.

David

Re: [RFC 8/8] x86/hpet: Use singe apic vector rather than irq_descs for HPET interrupts

[Jan Beulich]

>>> On 04.11.13 at 19:54, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> This involves rewriting most of the MSI related HPET code, and as a result
> this patch looks very complicated.  It is probably best viewed as an end
> result, with the following notes explaining what is going on.

Problem is - it likely won't apply without the earlier changes in the
series (I admittedly didn't try); patch 6 - which I suggested to drop -
seems like the most likely candidate for conflicts. Hence looking at
the end result makes sense only once you re-posted. I'll therefore
defer reviewing till then. The high level description sounds good in
any event.

Jan

Re: [RFC 2/8] x86/acpi: Warn about multiple HPET tables

[Andrew Cooper]

On 05/11/13 10:58, Jan Beulich wrote:
>>>> On 04.11.13 at 19:54, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/acpi/boot.c
>> +++ b/xen/arch/x86/acpi/boot.c
>> @@ -289,6 +289,17 @@ static int __init acpi_parse_hpet(struct acpi_table_header *table)
>>  		return -1;
>>  	}
>>  
>> +	/*
>> +	 * Some BIOSes provide multiple HPET tables. Warn that we will ignore
>> +	 * them.
>> +	 */
>> +	if ( hpet_address )
>> +	{
>> +		printk(KERN_WARNING PREFIX
>> +		       "Found multiple HPET tables. Only using first\n");
>> +		return -1;
>> +	}
> If there really are examples of this, and if those HPETs work
> properly, perhaps we should rather make use of them?
>
> Jan
>

The cause of this was two HPET acpi tables appearing.  This got 'fixed'
by a BIOS update (after which there was only a single HPET table), but I
felt it was worth warning about.

It might be nice to support multiple hpets is such a system existed.  It
would however be another fairly large chunk of work and I do not believe
I have appropriate hardware to test any development work on.

~Andrew