[PATCH RFC 0/3] hpet: add support for level triggered interrupts

[PATCH RFC 0/3] hpet: add support for level triggered interrupts

[Roger Pau Monne]

Hello,

This series add support for level triggered HPET interrupts, which are
mandatory according to the spec.

First patch is a change for vpt in order to execute the timer
callbacks even if the interrupt is not injected because the IRQ is
masked. Second patch implements support for level triggered interrupts
in HPET and finally patch 3 is for XTF in order to add a basic HPET
functionality test.

The whole series is RFC because I've mainly tested it using the XTF
test provided in patch 3. Also it slightly changes (fixes) the
behavior of other timers because callbacks are now always executed.

Thanks, Roger.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH RFC 1/3] x86/vpt: execute callbacks for masked interrupts

[Roger Pau Monne]

Execute periodic_time callbacks even if the interrupt is not actually
injected because the IRQ is masked.

Current callbacks from emulated timer devices only update emulated
registers, which from my reading of the specs should happen regardless
of whether the interrupt has been injected or not.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Stefan Bader <stefan.bader@canonical.com>
---
 xen/arch/x86/hvm/vpt.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
index 181f4cb631..1a24fbaa44 100644
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -247,9 +247,31 @@ static void pt_timer_fn(void *data)
     pt_unlock(pt);
 }
 
+static void execute_callbacks(struct vcpu *v, struct list_head *tm)
+{
+    spin_lock(&v->arch.hvm_vcpu.tm_lock);
+    while ( !list_empty(tm) )
+    {
+        struct periodic_time *pt = list_first_entry(tm, struct periodic_time,
+                                                    list);
+        time_cb *cb = pt->cb;
+        void *cb_priv = pt->priv;
+
+        list_del(&pt->list);
+        pt->on_list = 0;
+        spin_unlock(&v->arch.hvm_vcpu.tm_lock);
+
+        cb(v, cb_priv);
+
+        spin_lock(&v->arch.hvm_vcpu.tm_lock);
+    }
+    spin_unlock(&v->arch.hvm_vcpu.tm_lock);
+}
+
 int pt_update_irq(struct vcpu *v)
 {
     struct list_head *head = &v->arch.hvm_vcpu.tm_list;
+    LIST_HEAD(purged);
     struct periodic_time *pt, *temp, *earliest_pt;
     uint64_t max_lag;
     int irq, is_lapic, pt_vector;
@@ -267,7 +289,10 @@ int pt_update_irq(struct vcpu *v)
             {
                 /* suspend timer emulation */
                 list_del(&pt->list);
-                pt->on_list = 0;
+                if ( pt->cb )
+                    list_add(&pt->list, &purged);
+                else
+                    pt->on_list = 0;
             }
             else
             {
@@ -283,6 +308,7 @@ int pt_update_irq(struct vcpu *v)
     if ( earliest_pt == NULL )
     {
         spin_unlock(&v->arch.hvm_vcpu.tm_lock);
+        execute_callbacks(v, &purged);
         return -1;
     }
 
@@ -292,6 +318,8 @@ int pt_update_irq(struct vcpu *v)
 
     spin_unlock(&v->arch.hvm_vcpu.tm_lock);
 
+    execute_callbacks(v, &purged);
+
     /*
      * If periodic timer interrut is handled by lapic, its vector in
      * IRR is returned and used to set eoi_exit_bitmap for virtual
-- 
2.16.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH RFC 2/3] vhpet: add support for level triggered interrupts

[Roger Pau Monne]

Level triggered interrupts are not an optional feature of HPET, and
must be implemented in order to comply with the HPET specification.

Implement them by adding a callback to the timer which sets the
interrupt bit in the general interrupt status register. Further
interrupts (in case of periodic mode) will not be injected until the
bit is cleared.

In order to reset the interrupts when the status bit is clear Xen must
also detect accesses to such register.

While there convert tn and i in hpet_write to unsigned.

Reported-by: Stefan Bader <stefan.bader@canonical.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Stefan Bader <stefan.bader@canonical.com>
---
 xen/arch/x86/hvm/hpet.c | 46 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index f7aed7f69e..1cfd72592e 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -220,6 +220,17 @@ static void hpet_stop_timer(HPETState *h, unsigned int tn,
     hpet_get_comparator(h, tn, guest_time);
 }
 
+static void hpet_timer_fired(struct vcpu *v, void *data)
+{
+    unsigned int tn = (unsigned int)data;
+    HPETState *h = vcpu_vhpet(v);
+
+    write_lock(&h->lock);
+    ASSERT(!test_bit(tn, &h->hpet.isr));
+    __set_bit(tn, &h->hpet.isr);
+    write_unlock(&h->lock);
+}
+
 /* the number of HPET tick that stands for
  * 1/(2^10) second, namely, 0.9765625 milliseconds */
 #define  HPET_TINY_TIME_SPAN  ((h->stime_freq >> 10) / STIME_PER_HPET_TICK)
@@ -241,7 +252,7 @@ static void hpet_set_timer(HPETState *h, unsigned int tn,
         pit_stop_channel0_irq(&vhpet_domain(h)->arch.vpit);
     }
 
-    if ( !timer_enabled(h, tn) )
+    if ( !timer_enabled(h, tn) || test_bit(tn, &h->hpet.isr) )
         return;
 
     tn_cmp   = hpet_get_comparator(h, tn, guest_time);
@@ -277,8 +288,12 @@ static void hpet_set_timer(HPETState *h, unsigned int tn,
      * timer we also need the period which may be different because time may
      * have elapsed between the time the comparator was written and the timer
      * being enabled (now).
+     *
+     * NB: set periodic timers as oneshot if interrupt type is set to level
+     * because the user must ack the interrupt (by writing 1 to the interrupt
+     * status register) before another interrupt can be delivered.
      */
-    oneshot = !timer_is_periodic(h, tn);
+    oneshot = !timer_is_periodic(h, tn) || timer_level(h, tn);
     TRACE_2_LONG_4D(TRC_HVM_EMUL_HPET_START_TIMER, tn, irq,
                     TRC_PAR_LONG(hpet_tick_to_ns(h, diff)),
                     TRC_PAR_LONG(oneshot ? 0LL :
@@ -286,7 +301,9 @@ static void hpet_set_timer(HPETState *h, unsigned int tn,
     create_periodic_time(vhpet_vcpu(h), &h->pt[tn],
                          hpet_tick_to_ns(h, diff),
                          oneshot ? 0 : hpet_tick_to_ns(h, h->hpet.period[tn]),
-                         irq, NULL, NULL);
+                         irq,
+                         timer_level(h, tn) ? hpet_timer_fired : NULL,
+                         (void *)(unsigned long)tn);
 }
 
 static inline uint64_t hpet_fixup_reg(
@@ -304,7 +321,7 @@ static int hpet_write(
     HPETState *h = vcpu_vhpet(v);
     uint64_t old_val, new_val;
     uint64_t guest_time;
-    int tn, i;
+    unsigned int tn, i;
 
     /* Acculumate a bit mask of timers whos state is changed by this write. */
     unsigned long start_timers = 0;
@@ -360,6 +377,19 @@ static int hpet_write(
         }
         break;
 
+    case HPET_STATUS:
+        /* write 1 to clear. */
+        while (new_val)
+        {
+            i = find_first_set_bit(new_val);
+            if ( i >= HPET_TIMER_NUM )
+                break;
+            __clear_bit(i, &new_val);
+            if ( __test_and_clear_bit(i, &h->hpet.isr) )
+                set_start_timer(i);
+        }
+        break;
+
     case HPET_COUNTER:
         h->hpet.mc64 = new_val;
         if ( hpet_enabled(h) )
@@ -379,14 +409,6 @@ static int hpet_write(
 
         h->hpet.timers[tn].config = hpet_fixup_reg(new_val, old_val, 0x3f4e);
 
-        if ( timer_level(h, tn) )
-        {
-            gdprintk(XENLOG_ERR,
-                     "HPET: level triggered interrupt not supported now\n");
-            domain_crash(current->domain);
-            break;
-        }
-
         if ( new_val & HPET_TN_32BIT )
         {
             h->hpet.timers[tn].cmp = (uint32_t)h->hpet.timers[tn].cmp;
-- 
2.16.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH RFC 3/3] xtf: add minimal HPET functionality test

[Roger Pau Monne]

Add a basic HPET functionality test, note that this test requires the
HPET to support level triggered interrupts.

Further improvements should add support for interrupt delivery, and
testing all the available timers.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 arch/x86/include/arch/lib.h |  14 ++++
 docs/all-tests.dox          |   2 +
 tests/hpet/Makefile         |   9 +++
 tests/hpet/main.c           | 187 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 212 insertions(+)
 create mode 100644 tests/hpet/Makefile
 create mode 100644 tests/hpet/main.c

diff --git a/arch/x86/include/arch/lib.h b/arch/x86/include/arch/lib.h
index 6714bdc..3400890 100644
--- a/arch/x86/include/arch/lib.h
+++ b/arch/x86/include/arch/lib.h
@@ -392,6 +392,20 @@ static inline void write_xcr0(uint64_t xcr0)
     xsetbv(0, xcr0);
 }
 
+static inline uint64_t rdtsc(void)
+{
+    uint32_t low, high;
+
+    asm volatile ("rdtsc" : "=a" (low), "=d" (high));
+
+    return ((uint64_t)high << 32) | low;
+}
+
+static inline void pause(void)
+{
+    asm volatile ("pause");
+}
+
 #endif /* XTF_X86_LIB_H */
 
 /*
diff --git a/docs/all-tests.dox b/docs/all-tests.dox
index 355cb80..122840c 100644
--- a/docs/all-tests.dox
+++ b/docs/all-tests.dox
@@ -127,4 +127,6 @@ guest breakout.
 @subpage test-nested-svm - Nested SVM tests.
 
 @subpage test-nested-vmx - Nested VT-x tests.
+
+@subpage test-hpet - HPET functional test.
 */
diff --git a/tests/hpet/Makefile b/tests/hpet/Makefile
new file mode 100644
index 0000000..934e63c
--- /dev/null
+++ b/tests/hpet/Makefile
@@ -0,0 +1,9 @@
+include $(ROOT)/build/common.mk
+
+NAME      := hpet
+CATEGORY  := utility
+TEST-ENVS := hvm32
+
+obj-perenv += main.o
+
+include $(ROOT)/build/gen.mk
diff --git a/tests/hpet/main.c b/tests/hpet/main.c
new file mode 100644
index 0000000..57be410
--- /dev/null
+++ b/tests/hpet/main.c
@@ -0,0 +1,187 @@
+/**
+ * @file tests/hpet/main.c
+ * @ref test-hpet
+ *
+ * @page test-hpet hpet
+ *
+ * HPET functionality testing.
+ *
+ * Quite limited, currently only Timer N is tested. No interrupt delivery
+ * tests.
+ *
+ * @see tests/hpet/main.c
+ */
+#include <xtf.h>
+
+#define HPET_BASE_ADDRESS       0xfed00000
+
+#define HPET_ID                 0
+#define HPET_ID_NUMBER          0x1f00
+#define HPET_ID_NUMBER_SHIFT    8
+
+#define HPET_PERIOD             0x004
+#define HPET_MAX_PERIOD         0x05f5e100
+
+#define HPET_CFG                0x010
+#define HPET_CFG_ENABLE         0x001
+
+#define HPET_STATUS             0x020
+
+#define HPET_COUNTER            0x0f0
+
+#define HPET_Tn_CFG(n)          (0x100 + (n) * 0x20)
+#define HPET_TN_LEVEL           0x002
+#define HPET_TN_ENABLE          0x004
+#define HPET_TN_PERIODIC        0x008
+#define HPET_TN_32BIT           0x100
+#define HPET_TN_ROUTE_SHIFT     9
+
+#define HPET_Tn_CMP(n)          (0x108 + (n) * 0x20)
+
+/*
+ * NB: should probably be an explicit movl, but clang seems to generate good
+ * code.
+ */
+#define HPET_REG(reg) (*(volatile uint32_t *)(_p(HPET_BASE_ADDRESS) + (reg)))
+
+#define MS_TO_NS                1000000
+/* p is in fs */
+#define MS_TO_TICKS(ms, p)      (((ms) * MS_TO_NS) / ((p) / 1000000))
+
+const char test_title[] = "Test HPET";
+
+static uint32_t freq;
+
+static void set_freq(void)
+{
+    uint32_t eax, ebx, ecx, edx, base;
+    bool found = false;
+
+    /* Get tsc frequency from cpuid. */
+    for ( base = XEN_CPUID_FIRST_LEAF;
+          base < XEN_CPUID_FIRST_LEAF + 0x10000; base += 0x100 )
+    {
+        cpuid(base, &eax, &ebx, &ecx, &edx);
+
+        if ( (ebx == XEN_CPUID_SIGNATURE_EBX) &&
+             (ecx == XEN_CPUID_SIGNATURE_ECX) &&
+             (edx == XEN_CPUID_SIGNATURE_EDX) &&
+             ((eax - base) >= 2) )
+        {
+            found = true;
+            break;
+        }
+    }
+
+    if ( !found )
+        panic("Unable to locate Xen CPUID leaves\n");
+
+    cpuid_count(base + 3, 0, &eax, &ebx, &freq, &edx);
+    printk("TSC frequency %ukHz\n", freq);
+}
+
+/* Busy-wait implementation based on tsc value. */
+static void wait(unsigned int ms)
+{
+    uint64_t end = rdtsc() + (uint64_t)ms * (uint64_t)freq;
+
+    while ( rdtsc() < end )
+        pause();
+}
+
+void test_main(void)
+{
+    uint32_t period, route;
+    unsigned int nr, irq;
+
+    set_freq();
+
+    /* Sanity check main counter tick period. */
+    period = HPET_REG(HPET_PERIOD);
+    if ( period == 0 || period > HPET_MAX_PERIOD )
+    {
+        printk("Invalid period found in main counter tick: %u fs\n", period);
+        return xtf_error("Error: Cannot find valid HPET\n");
+    }
+
+    /* Get number of timers. */
+    nr = (HPET_REG(HPET_ID) & HPET_ID_NUMBER) >> HPET_ID_NUMBER_SHIFT;
+    printk("HPET reports %u timers period %u fs\n", nr + 1, period);
+
+    /* Check possible interrupt routing. */
+    route = HPET_REG(HPET_Tn_CFG(nr) + 4);
+    for ( irq = 0; irq < 32; irq++ )
+        if ( test_bit(irq, &route) )
+            break;
+    if ( irq == 32 )
+        return xtf_error("Error: Cannot find valid IRQ routing\n");
+    printk("Routing timer %u to IRQ %u\n", nr, irq);
+
+    /* Test oneshot timer nr. */
+    HPET_REG(HPET_Tn_CFG(nr)) |= (irq << HPET_TN_ROUTE_SHIFT) | HPET_TN_32BIT |
+                                 HPET_TN_LEVEL | HPET_TN_ENABLE;
+
+    /* Reset main counter. */
+    HPET_REG(HPET_COUNTER) = 0;
+    /* Set the comparator to fire in 50ms. */
+    HPET_REG(HPET_Tn_CMP(nr)) = MS_TO_TICKS(50, period);
+
+    /* Enable interrupts. */
+    HPET_REG(HPET_CFG) |= HPET_CFG_ENABLE;
+
+    /* Wait for 100ms. */
+    wait(100);
+    if ( !((HPET_REG(HPET_STATUS) >> nr) & 1) )
+        return xtf_failure("Fail: Status bit unset for level interrupt in oneshot mode\n");
+
+    /* Clean status bit. */
+    HPET_REG(HPET_STATUS) = 1 << nr;
+
+    /* Try periodic mode. */
+    HPET_REG(HPET_CFG) &= ~HPET_CFG_ENABLE;
+    /* Reset main counter. */
+    HPET_REG(HPET_COUNTER) = 0;
+    /* Enable periodic interrupts. */
+    HPET_REG(HPET_Tn_CFG(nr)) |= HPET_TN_PERIODIC;
+    /* Set comparator to 100ms period. */
+    HPET_REG(HPET_Tn_CMP(nr)) = MS_TO_TICKS(100, period);
+    HPET_REG(HPET_CFG) |= HPET_CFG_ENABLE;
+
+    /* Wait for 200ms. */
+    wait(200);
+    if ( !((HPET_REG(HPET_STATUS) >> nr) & 1) )
+        return xtf_failure("Fail: Status bit unset for level interrupt in periodic mode\n");
+
+    /*
+     * The comparator register should continue to be updated despite the status
+     * not being cleared.
+     */
+    wait(300);
+    if ( HPET_REG(HPET_Tn_CMP(nr)) < MS_TO_TICKS(400, period) )
+        return xtf_failure("Fail: Comparator not updated in periodic mode\n");
+
+    /* Clear the status bit and wait for it to be set again. */
+    HPET_REG(HPET_STATUS) = 1 << nr;
+    wait(200);
+    if ( !((HPET_REG(HPET_STATUS) >> nr) & 1) )
+        return xtf_failure("Fail: Status bit unset for level interrupt in periodic mode\n");
+
+    /* Switch to edge mode, clear status bit and check it's not set again. */
+    HPET_REG(HPET_Tn_CFG(nr)) &= ~HPET_TN_LEVEL;
+    HPET_REG(HPET_STATUS) = 1 << nr;
+    wait(200);
+    if ( ((HPET_REG(HPET_STATUS) >> nr) & 1) )
+        return xtf_failure("Fail: Status bit set for edge interrupt in periodic mode\n");
+
+    xtf_success(NULL);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.16.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH RFC 3/3] xtf: add minimal HPET functionality test

[Wei Liu]

On Fri, Feb 23, 2018 at 01:27:43PM +0000, Roger Pau Monne wrote:
> Add a basic HPET functionality test, note that this test requires the
> HPET to support level triggered interrupts.
> 
> Further improvements should add support for interrupt delivery, and
> testing all the available timers.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  arch/x86/include/arch/lib.h |  14 ++++
>  docs/all-tests.dox          |   2 +
>  tests/hpet/Makefile         |   9 +++
>  tests/hpet/main.c           | 187 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 212 insertions(+)
>  create mode 100644 tests/hpet/Makefile
>  create mode 100644 tests/hpet/main.c
> 
> diff --git a/arch/x86/include/arch/lib.h b/arch/x86/include/arch/lib.h
> index 6714bdc..3400890 100644
> --- a/arch/x86/include/arch/lib.h
> +++ b/arch/x86/include/arch/lib.h
> @@ -392,6 +392,20 @@ static inline void write_xcr0(uint64_t xcr0)
>      xsetbv(0, xcr0);
>  }
>  
> +static inline uint64_t rdtsc(void)
> +{
> +    uint32_t low, high;
> +
> +    asm volatile ("rdtsc" : "=a" (low), "=d" (high));
> +

You probably need to add lfence or mfence. See rdtsc_ordered in Xen.

> +    return ((uint64_t)high << 32) | low;
> +}
> +
[...]
> +static void set_freq(void)
> +{
> +    uint32_t eax, ebx, ecx, edx, base;
> +    bool found = false;
> +
> +    /* Get tsc frequency from cpuid. */
> +    for ( base = XEN_CPUID_FIRST_LEAF;
> +          base < XEN_CPUID_FIRST_LEAF + 0x10000; base += 0x100 )
> +    {
> +        cpuid(base, &eax, &ebx, &ecx, &edx);
> +
> +        if ( (ebx == XEN_CPUID_SIGNATURE_EBX) &&
> +             (ecx == XEN_CPUID_SIGNATURE_ECX) &&
> +             (edx == XEN_CPUID_SIGNATURE_EDX) &&
> +             ((eax - base) >= 2) )
> +        {
> +            found = true;
> +            break;
> +        }
> +    }
> +
> +    if ( !found )
> +        panic("Unable to locate Xen CPUID leaves\n");
> +

Finding Xen leaves should live in its own function and move to common
code if possible.

> +    cpuid_count(base + 3, 0, &eax, &ebx, &freq, &edx);
> +    printk("TSC frequency %ukHz\n", freq);
> +}
> +
> +/* Busy-wait implementation based on tsc value. */
> +static void wait(unsigned int ms)
> +{
> +    uint64_t end = rdtsc() + (uint64_t)ms * (uint64_t)freq;
> +
> +    while ( rdtsc() < end )
> +        pause();
> +}

Rename to mdelay and move to a helper file?

> +
> +void test_main(void)
> +{
[...]
> +    HPET_REG(HPET_Tn_CFG(nr)) &= ~HPET_TN_LEVEL;
> +    HPET_REG(HPET_STATUS) = 1 << nr;
> +    wait(200);
> +    if ( ((HPET_REG(HPET_STATUS) >> nr) & 1) )
> +        return xtf_failure("Fail: Status bit set for edge interrupt in periodic mode\n");
> +

Is it possible to use shorter time in the test? This test as-is will run
for 1 or 2 seconds which is a bit long as micro-kernel testing.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH RFC 1/3] x86/vpt: execute callbacks for masked interrupts

[Wei Liu]

On Fri, Feb 23, 2018 at 01:27:41PM +0000, Roger Pau Monne wrote:
> Execute periodic_time callbacks even if the interrupt is not actually
> injected because the IRQ is masked.
> 
> Current callbacks from emulated timer devices only update emulated
> registers, which from my reading of the specs should happen regardless
> of whether the interrupt has been injected or not.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Stefan Bader <stefan.bader@canonical.com>
> ---
>  xen/arch/x86/hvm/vpt.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
> index 181f4cb631..1a24fbaa44 100644
> --- a/xen/arch/x86/hvm/vpt.c
> +++ b/xen/arch/x86/hvm/vpt.c
> @@ -247,9 +247,31 @@ static void pt_timer_fn(void *data)
>      pt_unlock(pt);
>  }
>  
> +static void execute_callbacks(struct vcpu *v, struct list_head *tm)
> +{
> +    spin_lock(&v->arch.hvm_vcpu.tm_lock);
> +    while ( !list_empty(tm) )
> +    {
> +        struct periodic_time *pt = list_first_entry(tm, struct periodic_time,
> +                                                    list);
> +        time_cb *cb = pt->cb;
> +        void *cb_priv = pt->priv;
> +
> +        list_del(&pt->list);
> +        pt->on_list = 0;
> +        spin_unlock(&v->arch.hvm_vcpu.tm_lock);
> +
> +        cb(v, cb_priv);
> +
> +        spin_lock(&v->arch.hvm_vcpu.tm_lock);
> +    }
> +    spin_unlock(&v->arch.hvm_vcpu.tm_lock);
> +}
> +
>  int pt_update_irq(struct vcpu *v)
>  {
>      struct list_head *head = &v->arch.hvm_vcpu.tm_list;
> +    LIST_HEAD(purged);

to_purge?

>      struct periodic_time *pt, *temp, *earliest_pt;
>      uint64_t max_lag;
>      int irq, is_lapic, pt_vector;
> @@ -267,7 +289,10 @@ int pt_update_irq(struct vcpu *v)
>              {
>                  /* suspend timer emulation */
>                  list_del(&pt->list);
> -                pt->on_list = 0;
> +                if ( pt->cb )
> +                    list_add(&pt->list, &purged);
> +                else
> +                    pt->on_list = 0;
>              }
>              else
>              {
> @@ -283,6 +308,7 @@ int pt_update_irq(struct vcpu *v)
>      if ( earliest_pt == NULL )
>      {
>          spin_unlock(&v->arch.hvm_vcpu.tm_lock);
> +        execute_callbacks(v, &purged);

It would be better to check if the list is not empty before calling the
function to avoid the extra lock / unlock.

(Haven't checked if the basic premise of this patch is compatible with
the spec)

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH RFC 1/3] x86/vpt: execute callbacks for masked interrupts

[Roger Pau Monné]

On Mon, Feb 26, 2018 at 12:35:54PM +0000, Wei Liu wrote:
> On Fri, Feb 23, 2018 at 01:27:41PM +0000, Roger Pau Monne wrote:
> > Execute periodic_time callbacks even if the interrupt is not actually
> > injected because the IRQ is masked.
> > 
> > Current callbacks from emulated timer devices only update emulated
> > registers, which from my reading of the specs should happen regardless
> > of whether the interrupt has been injected or not.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: Stefan Bader <stefan.bader@canonical.com>
> > ---
> >  xen/arch/x86/hvm/vpt.c | 30 +++++++++++++++++++++++++++++-
> >  1 file changed, 29 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
> > index 181f4cb631..1a24fbaa44 100644
> > --- a/xen/arch/x86/hvm/vpt.c
> > +++ b/xen/arch/x86/hvm/vpt.c
> > @@ -247,9 +247,31 @@ static void pt_timer_fn(void *data)
> >      pt_unlock(pt);
> >  }
> >  
> > +static void execute_callbacks(struct vcpu *v, struct list_head *tm)
> > +{
> > +    spin_lock(&v->arch.hvm_vcpu.tm_lock);
> > +    while ( !list_empty(tm) )
> > +    {
> > +        struct periodic_time *pt = list_first_entry(tm, struct periodic_time,
> > +                                                    list);
> > +        time_cb *cb = pt->cb;
> > +        void *cb_priv = pt->priv;
> > +
> > +        list_del(&pt->list);
> > +        pt->on_list = 0;
> > +        spin_unlock(&v->arch.hvm_vcpu.tm_lock);
> > +
> > +        cb(v, cb_priv);
> > +
> > +        spin_lock(&v->arch.hvm_vcpu.tm_lock);
> > +    }
> > +    spin_unlock(&v->arch.hvm_vcpu.tm_lock);
> > +}
> > +
> >  int pt_update_irq(struct vcpu *v)
> >  {
> >      struct list_head *head = &v->arch.hvm_vcpu.tm_list;
> > +    LIST_HEAD(purged);
> 
> to_purge?

My point is that they have already been purged from the pt->list, but
I really don't have a preference.

> >      struct periodic_time *pt, *temp, *earliest_pt;
> >      uint64_t max_lag;
> >      int irq, is_lapic, pt_vector;
> > @@ -267,7 +289,10 @@ int pt_update_irq(struct vcpu *v)
> >              {
> >                  /* suspend timer emulation */
> >                  list_del(&pt->list);
> > -                pt->on_list = 0;
> > +                if ( pt->cb )
> > +                    list_add(&pt->list, &purged);
> > +                else
> > +                    pt->on_list = 0;
> >              }
> >              else
> >              {
> > @@ -283,6 +308,7 @@ int pt_update_irq(struct vcpu *v)
> >      if ( earliest_pt == NULL )
> >      {
> >          spin_unlock(&v->arch.hvm_vcpu.tm_lock);
> > +        execute_callbacks(v, &purged);
> 
> It would be better to check if the list is not empty before calling the
> function to avoid the extra lock / unlock.

The lock is also protecting the 'purged' list, so I think that for
consistency the lock needs to be held before accessing it.

Since this is only a empty check *and* there can't be any
additions to the list at this point I guess it would be safe to test
for emptiness without holding the lock, but I find it kind of
confusing.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH RFC 1/3] x86/vpt: execute callbacks for masked interrupts

[Jan Beulich]

>>> On 26.02.18 at 13:48, <roger.pau@citrix.com> wrote:
> On Mon, Feb 26, 2018 at 12:35:54PM +0000, Wei Liu wrote:
>> On Fri, Feb 23, 2018 at 01:27:41PM +0000, Roger Pau Monne wrote:
>> >  int pt_update_irq(struct vcpu *v)
>> >  {
>> >      struct list_head *head = &v->arch.hvm_vcpu.tm_list;
>> > +    LIST_HEAD(purged);
>> 
>> to_purge?
> 
> My point is that they have already been purged from the pt->list, but
> I really don't have a preference.
> 
>> >      struct periodic_time *pt, *temp, *earliest_pt;
>> >      uint64_t max_lag;
>> >      int irq, is_lapic, pt_vector;
>> > @@ -267,7 +289,10 @@ int pt_update_irq(struct vcpu *v)
>> >              {
>> >                  /* suspend timer emulation */
>> >                  list_del(&pt->list);
>> > -                pt->on_list = 0;
>> > +                if ( pt->cb )
>> > +                    list_add(&pt->list, &purged);
>> > +                else
>> > +                    pt->on_list = 0;
>> >              }
>> >              else
>> >              {
>> > @@ -283,6 +308,7 @@ int pt_update_irq(struct vcpu *v)
>> >      if ( earliest_pt == NULL )
>> >      {
>> >          spin_unlock(&v->arch.hvm_vcpu.tm_lock);
>> > +        execute_callbacks(v, &purged);
>> 
>> It would be better to check if the list is not empty before calling the
>> function to avoid the extra lock / unlock.
> 
> The lock is also protecting the 'purged' list, so I think that for
> consistency the lock needs to be held before accessing it.

But that's a local list, isn't it? No-one else can access it.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH RFC 1/3] x86/vpt: execute callbacks for masked interrupts

[Roger Pau Monné]

On Mon, Feb 26, 2018 at 06:04:51AM -0700, Jan Beulich wrote:
> >>> On 26.02.18 at 13:48, <roger.pau@citrix.com> wrote:
> > On Mon, Feb 26, 2018 at 12:35:54PM +0000, Wei Liu wrote:
> >> On Fri, Feb 23, 2018 at 01:27:41PM +0000, Roger Pau Monne wrote:
> >> >  int pt_update_irq(struct vcpu *v)
> >> >  {
> >> >      struct list_head *head = &v->arch.hvm_vcpu.tm_list;
> >> > +    LIST_HEAD(purged);
> >> 
> >> to_purge?
> > 
> > My point is that they have already been purged from the pt->list, but
> > I really don't have a preference.
> > 
> >> >      struct periodic_time *pt, *temp, *earliest_pt;
> >> >      uint64_t max_lag;
> >> >      int irq, is_lapic, pt_vector;
> >> > @@ -267,7 +289,10 @@ int pt_update_irq(struct vcpu *v)
> >> >              {
> >> >                  /* suspend timer emulation */
> >> >                  list_del(&pt->list);
> >> > -                pt->on_list = 0;
> >> > +                if ( pt->cb )
> >> > +                    list_add(&pt->list, &purged);
> >> > +                else
> >> > +                    pt->on_list = 0;
> >> >              }
> >> >              else
> >> >              {
> >> > @@ -283,6 +308,7 @@ int pt_update_irq(struct vcpu *v)
> >> >      if ( earliest_pt == NULL )
> >> >      {
> >> >          spin_unlock(&v->arch.hvm_vcpu.tm_lock);
> >> > +        execute_callbacks(v, &purged);
> >> 
> >> It would be better to check if the list is not empty before calling the
> >> function to avoid the extra lock / unlock.
> > 
> > The lock is also protecting the 'purged' list, so I think that for
> > consistency the lock needs to be held before accessing it.
> 
> But that's a local list, isn't it? No-one else can access it.

destroy_periodic_time can still remove items from this list, if a
timer that's on the 'purged' list is destroyed between added to the
list and executing the callback.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH RFC 3/3] xtf: add minimal HPET functionality test

[Roger Pau Monné]

On Fri, Feb 23, 2018 at 07:07:18PM +0000, Wei Liu wrote:
> On Fri, Feb 23, 2018 at 01:27:43PM +0000, Roger Pau Monne wrote:
> > Add a basic HPET functionality test, note that this test requires the
> > HPET to support level triggered interrupts.
> > 
> > Further improvements should add support for interrupt delivery, and
> > testing all the available timers.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> >  arch/x86/include/arch/lib.h |  14 ++++
> >  docs/all-tests.dox          |   2 +
> >  tests/hpet/Makefile         |   9 +++
> >  tests/hpet/main.c           | 187 ++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 212 insertions(+)
> >  create mode 100644 tests/hpet/Makefile
> >  create mode 100644 tests/hpet/main.c
> > 
> > diff --git a/arch/x86/include/arch/lib.h b/arch/x86/include/arch/lib.h
> > index 6714bdc..3400890 100644
> > --- a/arch/x86/include/arch/lib.h
> > +++ b/arch/x86/include/arch/lib.h
> > @@ -392,6 +392,20 @@ static inline void write_xcr0(uint64_t xcr0)
> >      xsetbv(0, xcr0);
> >  }
> >  
> > +static inline uint64_t rdtsc(void)
> > +{
> > +    uint32_t low, high;
> > +
> > +    asm volatile ("rdtsc" : "=a" (low), "=d" (high));
> > +
> 
> You probably need to add lfence or mfence. See rdtsc_ordered in Xen.

Oh, OK that's news to me. I guess just using a lfence before it
should be fine.

> > +    return ((uint64_t)high << 32) | low;
> > +}
> > +
> [...]
> > +static void set_freq(void)
> > +{
> > +    uint32_t eax, ebx, ecx, edx, base;
> > +    bool found = false;
> > +
> > +    /* Get tsc frequency from cpuid. */
> > +    for ( base = XEN_CPUID_FIRST_LEAF;
> > +          base < XEN_CPUID_FIRST_LEAF + 0x10000; base += 0x100 )
> > +    {
> > +        cpuid(base, &eax, &ebx, &ecx, &edx);
> > +
> > +        if ( (ebx == XEN_CPUID_SIGNATURE_EBX) &&
> > +             (ecx == XEN_CPUID_SIGNATURE_ECX) &&
> > +             (edx == XEN_CPUID_SIGNATURE_EDX) &&
> > +             ((eax - base) >= 2) )
> > +        {
> > +            found = true;
> > +            break;
> > +        }
> > +    }
> > +
> > +    if ( !found )
> > +        panic("Unable to locate Xen CPUID leaves\n");
> > +
> 
> Finding Xen leaves should live in its own function and move to common
> code if possible.

There's already one in common code. The right way to fix this seems to
be to store 'base' and use it in further cpuid related calls.

> > +    cpuid_count(base + 3, 0, &eax, &ebx, &freq, &edx);
> > +    printk("TSC frequency %ukHz\n", freq);
> > +}
> > +
> > +/* Busy-wait implementation based on tsc value. */
> > +static void wait(unsigned int ms)
> > +{
> > +    uint64_t end = rdtsc() + (uint64_t)ms * (uint64_t)freq;
> > +
> > +    while ( rdtsc() < end )
> > +        pause();
> > +}
> 
> Rename to mdelay and move to a helper file?

OK, wasn't sure whether this would be helpful or not.

> > +
> > +void test_main(void)
> > +{
> [...]
> > +    HPET_REG(HPET_Tn_CFG(nr)) &= ~HPET_TN_LEVEL;
> > +    HPET_REG(HPET_STATUS) = 1 << nr;
> > +    wait(200);
> > +    if ( ((HPET_REG(HPET_STATUS) >> nr) & 1) )
> > +        return xtf_failure("Fail: Status bit set for edge interrupt in periodic mode\n");
> > +
> 
> Is it possible to use shorter time in the test? This test as-is will run
> for 1 or 2 seconds which is a bit long as micro-kernel testing.

Likely, I can reduce the periods and waits to something smaller. Let
me use a define and then we can tweak this, but using a 1ms period
should be fine.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH RFC 3/3] xtf: add minimal HPET functionality test

[Jan Beulich]

>>> On 28.02.18 at 16:37, <roger.pau@citrix.com> wrote:
> On Fri, Feb 23, 2018 at 07:07:18PM +0000, Wei Liu wrote:
>> On Fri, Feb 23, 2018 at 01:27:43PM +0000, Roger Pau Monne wrote:
>> > --- a/arch/x86/include/arch/lib.h
>> > +++ b/arch/x86/include/arch/lib.h
>> > @@ -392,6 +392,20 @@ static inline void write_xcr0(uint64_t xcr0)
>> >      xsetbv(0, xcr0);
>> >  }
>> >  
>> > +static inline uint64_t rdtsc(void)
>> > +{
>> > +    uint32_t low, high;
>> > +
>> > +    asm volatile ("rdtsc" : "=a" (low), "=d" (high));
>> > +
>> 
>> You probably need to add lfence or mfence. See rdtsc_ordered in Xen.
> 
> Oh, OK that's news to me. I guess just using a lfence before it
> should be fine.

Except that on AMD, without LFENCE made dispatch serializing,
you'd need MFENCE.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH RFC 3/3] xtf: add minimal HPET functionality test

[Roger Pau Monné]

On Wed, Feb 28, 2018 at 09:24:09AM -0700, Jan Beulich wrote:
> >>> On 28.02.18 at 16:37, <roger.pau@citrix.com> wrote:
> > On Fri, Feb 23, 2018 at 07:07:18PM +0000, Wei Liu wrote:
> >> On Fri, Feb 23, 2018 at 01:27:43PM +0000, Roger Pau Monne wrote:
> >> > --- a/arch/x86/include/arch/lib.h
> >> > +++ b/arch/x86/include/arch/lib.h
> >> > @@ -392,6 +392,20 @@ static inline void write_xcr0(uint64_t xcr0)
> >> >      xsetbv(0, xcr0);
> >> >  }
> >> >  
> >> > +static inline uint64_t rdtsc(void)
> >> > +{
> >> > +    uint32_t low, high;
> >> > +
> >> > +    asm volatile ("rdtsc" : "=a" (low), "=d" (high));
> >> > +
> >> 
> >> You probably need to add lfence or mfence. See rdtsc_ordered in Xen.
> > 
> > Oh, OK that's news to me. I guess just using a lfence before it
> > should be fine.
> 
> Except that on AMD, without LFENCE made dispatch serializing,
> you'd need MFENCE.

So using just MFENCE will work everywhere since that's more
restrictive than a lfence?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH RFC 3/3] xtf: add minimal HPET functionality test

[Jan Beulich]

>>> On 01.03.18 at 10:55, <roger.pau@citrix.com> wrote:
> On Wed, Feb 28, 2018 at 09:24:09AM -0700, Jan Beulich wrote:
>> >>> On 28.02.18 at 16:37, <roger.pau@citrix.com> wrote:
>> > On Fri, Feb 23, 2018 at 07:07:18PM +0000, Wei Liu wrote:
>> >> On Fri, Feb 23, 2018 at 01:27:43PM +0000, Roger Pau Monne wrote:
>> >> > --- a/arch/x86/include/arch/lib.h
>> >> > +++ b/arch/x86/include/arch/lib.h
>> >> > @@ -392,6 +392,20 @@ static inline void write_xcr0(uint64_t xcr0)
>> >> >      xsetbv(0, xcr0);
>> >> >  }
>> >> >  
>> >> > +static inline uint64_t rdtsc(void)
>> >> > +{
>> >> > +    uint32_t low, high;
>> >> > +
>> >> > +    asm volatile ("rdtsc" : "=a" (low), "=d" (high));
>> >> > +
>> >> 
>> >> You probably need to add lfence or mfence. See rdtsc_ordered in Xen.
>> > 
>> > Oh, OK that's news to me. I guess just using a lfence before it
>> > should be fine.
>> 
>> Except that on AMD, without LFENCE made dispatch serializing,
>> you'd need MFENCE.
> 
> So using just MFENCE will work everywhere since that's more
> restrictive than a lfence?

Sadly no. I think that's explained in the description of the
hypervisor patches introducing rdtsc_ordered(), but you can also
simply read up on the LFENCE/MFENCE specifics on the Intel SDM
instruction description. In particular: "MFENCE does not serialize
the instruction stream." Yet that's what we're after here. MFENCE
is a serializing instruction on AMD hardware only.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH RFC 3/3] xtf: add minimal HPET functionality test

[Roger Pau Monné]

On Thu, Mar 01, 2018 at 03:04:29AM -0700, Jan Beulich wrote:
> >>> On 01.03.18 at 10:55, <roger.pau@citrix.com> wrote:
> > On Wed, Feb 28, 2018 at 09:24:09AM -0700, Jan Beulich wrote:
> >> >>> On 28.02.18 at 16:37, <roger.pau@citrix.com> wrote:
> >> > On Fri, Feb 23, 2018 at 07:07:18PM +0000, Wei Liu wrote:
> >> >> On Fri, Feb 23, 2018 at 01:27:43PM +0000, Roger Pau Monne wrote:
> >> >> > --- a/arch/x86/include/arch/lib.h
> >> >> > +++ b/arch/x86/include/arch/lib.h
> >> >> > @@ -392,6 +392,20 @@ static inline void write_xcr0(uint64_t xcr0)
> >> >> >      xsetbv(0, xcr0);
> >> >> >  }
> >> >> >  
> >> >> > +static inline uint64_t rdtsc(void)
> >> >> > +{
> >> >> > +    uint32_t low, high;
> >> >> > +
> >> >> > +    asm volatile ("rdtsc" : "=a" (low), "=d" (high));
> >> >> > +
> >> >> 
> >> >> You probably need to add lfence or mfence. See rdtsc_ordered in Xen.
> >> > 
> >> > Oh, OK that's news to me. I guess just using a lfence before it
> >> > should be fine.
> >> 
> >> Except that on AMD, without LFENCE made dispatch serializing,
> >> you'd need MFENCE.
> > 
> > So using just MFENCE will work everywhere since that's more
> > restrictive than a lfence?
> 
> Sadly no. I think that's explained in the description of the
> hypervisor patches introducing rdtsc_ordered(), but you can also
> simply read up on the LFENCE/MFENCE specifics on the Intel SDM
> instruction description. In particular: "MFENCE does not serialize
> the instruction stream." Yet that's what we're after here. MFENCE
> is a serializing instruction on AMD hardware only.

Hm, nice. Then I basically need to issue both a LFENCE and MFENCE
because I don't plan to introduce an alternatives framework to XTF
ATM.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH RFC 3/3] xtf: add minimal HPET functionality test

[Jan Beulich]

>>> On 01.03.18 at 11:14, <roger.pau@citrix.com> wrote:
> Hm, nice. Then I basically need to issue both a LFENCE and MFENCE
> because I don't plan to introduce an alternatives framework to XTF
> ATM.

Or you could skip the test if you find LFENCE isn't serializing on
AMD. After all the hypervisor now makes it serializing wherever
it can, and it is serializing on the two families where the respective
MSR doesn't exist. Of course the question is whether seeing the
raw (host) value of that MSR is actually something such a test
should be permitted to rely upon; Andrew may have very specific
thoughts on this.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH RFC 3/3] xtf: add minimal HPET functionality test

[Andrew Cooper]

On 23/02/18 13:27, Roger Pau Monne wrote:
> Add a basic HPET functionality test, note that this test requires the
> HPET to support level triggered interrupts.
>
> Further improvements should add support for interrupt delivery, and
> testing all the available timers.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  arch/x86/include/arch/lib.h |  14 ++++
>  docs/all-tests.dox          |   2 +
>  tests/hpet/Makefile         |   9 +++
>  tests/hpet/main.c           | 187 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 212 insertions(+)
>  create mode 100644 tests/hpet/Makefile
>  create mode 100644 tests/hpet/main.c
>
> diff --git a/arch/x86/include/arch/lib.h b/arch/x86/include/arch/lib.h
> index 6714bdc..3400890 100644
> --- a/arch/x86/include/arch/lib.h
> +++ b/arch/x86/include/arch/lib.h
> @@ -392,6 +392,20 @@ static inline void write_xcr0(uint64_t xcr0)
>      xsetbv(0, xcr0);
>  }
>  
> +static inline uint64_t rdtsc(void)
> +{
> +    uint32_t low, high;
> +
> +    asm volatile ("rdtsc" : "=a" (low), "=d" (high));

For my own timing purposes, I've been using rdtscp because it is
strictly more helpful, but this isn't a general solution.

For rdtsc, (contrary to the way the other thread is progressing), what
matters is a dispatch serialising event, which is different to an
architecturally serialising event.

The easiest fix for now is to unconditionally use mfence, leaving a
comment saying that this should be lfence on Intel and when the AMD
pipeline is configured correctly.  Please name the function
rdtscp_ordered() though, to distinguish it from a plain rdtsc instruction.

> +
> +    return ((uint64_t)high << 32) | low;
> +}
> +
> +static inline void pause(void)
> +{
> +    asm volatile ("pause");
> +}
> +
>  #endif /* XTF_X86_LIB_H */
>  
>  /*
> diff --git a/docs/all-tests.dox b/docs/all-tests.dox
> index 355cb80..122840c 100644
> --- a/docs/all-tests.dox
> +++ b/docs/all-tests.dox
> @@ -127,4 +127,6 @@ guest breakout.
>  @subpage test-nested-svm - Nested SVM tests.
>  
>  @subpage test-nested-vmx - Nested VT-x tests.
> +
> +@subpage test-hpet - HPET functional test.

This page is sorted by test category first, but this is the
"in-development" section.

FWIW, I think "in-development" is probably a better category than
utility, because we will eventually want to get this test into automation.

>  */
> diff --git a/tests/hpet/Makefile b/tests/hpet/Makefile
> new file mode 100644
> index 0000000..934e63c
> --- /dev/null
> +++ b/tests/hpet/Makefile
> @@ -0,0 +1,9 @@
> +include $(ROOT)/build/common.mk
> +
> +NAME      := hpet
> +CATEGORY  := utility
> +TEST-ENVS := hvm32
> +
> +obj-perenv += main.o
> +
> +include $(ROOT)/build/gen.mk
> diff --git a/tests/hpet/main.c b/tests/hpet/main.c
> new file mode 100644
> index 0000000..57be410
> --- /dev/null
> +++ b/tests/hpet/main.c
> @@ -0,0 +1,187 @@
> +/**
> + * @file tests/hpet/main.c
> + * @ref test-hpet
> + *
> + * @page test-hpet hpet
> + *
> + * HPET functionality testing.
> + *
> + * Quite limited, currently only Timer N is tested. No interrupt delivery
> + * tests.
> + *
> + * @see tests/hpet/main.c
> + */
> +#include <xtf.h>
> +
> +#define HPET_BASE_ADDRESS       0xfed00000
> +
> +#define HPET_ID                 0
> +#define HPET_ID_NUMBER          0x1f00
> +#define HPET_ID_NUMBER_SHIFT    8
> +
> +#define HPET_PERIOD             0x004
> +#define HPET_MAX_PERIOD         0x05f5e100
> +
> +#define HPET_CFG                0x010
> +#define HPET_CFG_ENABLE         0x001
> +
> +#define HPET_STATUS             0x020
> +
> +#define HPET_COUNTER            0x0f0
> +
> +#define HPET_Tn_CFG(n)          (0x100 + (n) * 0x20)
> +#define HPET_TN_LEVEL           0x002
> +#define HPET_TN_ENABLE          0x004
> +#define HPET_TN_PERIODIC        0x008
> +#define HPET_TN_32BIT           0x100
> +#define HPET_TN_ROUTE_SHIFT     9
> +
> +#define HPET_Tn_CMP(n)          (0x108 + (n) * 0x20)
> +
> +/*
> + * NB: should probably be an explicit movl, but clang seems to generate good
> + * code.
> + */
> +#define HPET_REG(reg) (*(volatile uint32_t *)(_p(HPET_BASE_ADDRESS) + (reg)))

A lot of the above should be in a dedicated hpet driver, rather than in
the test.  See the selftest test_driver_init(), and apic.{h,c} which is
the closes similar example.

That said, HPET registers are in general 64 bits wide rather than 32. 
It is probably best to split the basic hpet infrastructure into a
separate patch from the test.

> +
> +#define MS_TO_NS                1000000
> +/* p is in fs */
> +#define MS_TO_TICKS(ms, p)      (((ms) * MS_TO_NS) / ((p) / 1000000))
> +
> +const char test_title[] = "Test HPET";
> +
> +static uint32_t freq;
> +
> +static void set_freq(void)
> +{
> +    uint32_t eax, ebx, ecx, edx, base;
> +    bool found = false;
> +
> +    /* Get tsc frequency from cpuid. */
> +    for ( base = XEN_CPUID_FIRST_LEAF;
> +          base < XEN_CPUID_FIRST_LEAF + 0x10000; base += 0x100 )
> +    {
> +        cpuid(base, &eax, &ebx, &ecx, &edx);
> +
> +        if ( (ebx == XEN_CPUID_SIGNATURE_EBX) &&
> +             (ecx == XEN_CPUID_SIGNATURE_ECX) &&
> +             (edx == XEN_CPUID_SIGNATURE_EDX) &&
> +             ((eax - base) >= 2) )
> +        {
> +            found = true;
> +            break;
> +        }
> +    }
> +
> +    if ( !found )
> +        panic("Unable to locate Xen CPUID leaves\n");
> +
> +    cpuid_count(base + 3, 0, &eax, &ebx, &freq, &edx);
> +    printk("TSC frequency %ukHz\n", freq);

Calculate what you need in arch/x86/setup.c and export it via
arch/x86/include/arch/cpuid.h

However, you can't rely on the frequency being constant.  It might be
better to busy wait on the percpu wallclock instead, if you don't want
to sort out interrupts.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH RFC 3/3] xtf: add minimal HPET functionality test

[Jan Beulich]

>>> On 01.03.18 at 12:46, <andrew.cooper3@citrix.com> wrote:
> On 23/02/18 13:27, Roger Pau Monne wrote:
>> Add a basic HPET functionality test, note that this test requires the
>> HPET to support level triggered interrupts.
>>
>> Further improvements should add support for interrupt delivery, and
>> testing all the available timers.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>>  arch/x86/include/arch/lib.h |  14 ++++
>>  docs/all-tests.dox          |   2 +
>>  tests/hpet/Makefile         |   9 +++
>>  tests/hpet/main.c           | 187 
> ++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 212 insertions(+)
>>  create mode 100644 tests/hpet/Makefile
>>  create mode 100644 tests/hpet/main.c
>>
>> diff --git a/arch/x86/include/arch/lib.h b/arch/x86/include/arch/lib.h
>> index 6714bdc..3400890 100644
>> --- a/arch/x86/include/arch/lib.h
>> +++ b/arch/x86/include/arch/lib.h
>> @@ -392,6 +392,20 @@ static inline void write_xcr0(uint64_t xcr0)
>>      xsetbv(0, xcr0);
>>  }
>>  
>> +static inline uint64_t rdtsc(void)
>> +{
>> +    uint32_t low, high;
>> +
>> +    asm volatile ("rdtsc" : "=a" (low), "=d" (high));
> 
> For my own timing purposes, I've been using rdtscp because it is
> strictly more helpful, but this isn't a general solution.
> 
> For rdtsc, (contrary to the way the other thread is progressing), what
> matters is a dispatch serialising event, which is different to an
> architecturally serialising event.
> 
> The easiest fix for now is to unconditionally use mfence, leaving a
> comment saying that this should be lfence on Intel and when the AMD
> pipeline is configured correctly.  Please name the function
> rdtscp_ordered() though, to distinguish it from a plain rdtsc instruction.

Interesting. Right above you say "dispatch serialising", but then
you suggest MFENCE, which isn't on Intel? It's those differences
that I've been trying to explain on that other part of the thread
(apparently with little success, seeing your "contrary to").

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel