[PATCH v2 00/10] Prevent one cause of "MP-BIOS bug: 8254 timer"... message from linux

[PATCH v2 00/10] Prevent one cause of "MP-BIOS bug: 8254 timer"... message from linux

[Don Slutz]

Changes v1 to v2:

  Drop the patch "hpet: Act more like real hardware" from v1 for
  several reasons:

    1) Only "fixes" less then 50% of the problem: diff < 0 or
       diff > too many periods.

    2) I have a better fix in #2 "hvm/hpet: Only call
       guest_time_hpet(h) one time per action."

    3) Reverts a previous bug fix.

  So all these patches are new and fix various bugs.

  #1 "hvm/hpet: Add manual unit test code.":
     Is optional.  I used it to validate the changes did what I expected.

v1: info that still applies

Based on the proposed fix in QEMU:

http://marc.info/?l=qemu-devel&m=139304386331192&w=2

That was provided for:

http://marc.info/?l=qemu-devel&m=139295851107140&w=2

Which is very close to a bug I have been looking into and asked some
questions about in:

http://lists.xen.org/archives/html/xen-devel/2014-02/msg01787.html



Don Slutz (10):
  hvm/hpet: Add manual unit test code.
  hvm/hpet: Only call guest_time_hpet(h) one time per action.
  hvm/hpet: Only set comparator or period not both.
  hvm/hpet: In hpet_save, correctly compute mc64.
  hvm/hpet: Init comparator64 like comparator.
  hvm/hpet: comparator can only change when master clock is enabled.
  hvm/hpet: Call hpet_get_comparator during hpet_save.
  hvm/hpet: Prevent master clock equal to comparator while enabled
  hvm/hpet: Correctly limit period to a maximum.
  hvm/hpet: handle 1st period special

 tools/tests/vhpet/.gitignore  |   4 +
 tools/tests/vhpet/emul.h      | 405 +++++++++++++++++++++++++++++++
 tools/tests/vhpet/main.c      | 541 ++++++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/hvm/hpet.c       | 161 ++++++++++---
 xen/include/asm-x86/hvm/vpt.h |   2 +
 5 files changed, 1074 insertions(+), 39 deletions(-)
 create mode 100644 tools/tests/vhpet/.gitignore
 create mode 100644 tools/tests/vhpet/emul.h
 create mode 100644 tools/tests/vhpet/main.c

-- 
1.8.4

[PATCH v2 01/10] hvm/hpet: Add manual unit test code.

[Don Slutz]

Add the code at tools/tests/vhpet.

Does repro the bug:

..MP-BIOS bug: 8254 timer not connected to IO-APIC

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 tools/tests/vhpet/.gitignore |   4 +
 tools/tests/vhpet/emul.h     | 405 ++++++++++++++++++++++++++++++++
 tools/tests/vhpet/main.c     | 541 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 950 insertions(+)
 create mode 100644 tools/tests/vhpet/.gitignore
 create mode 100644 tools/tests/vhpet/emul.h
 create mode 100644 tools/tests/vhpet/main.c

diff --git a/tools/tests/vhpet/.gitignore b/tools/tests/vhpet/.gitignore
new file mode 100644
index 0000000..6df805d
--- /dev/null
+++ b/tools/tests/vhpet/.gitignore
@@ -0,0 +1,4 @@
+test_vhpet
+hpet.h
+hpet.c
+foo
diff --git a/tools/tests/vhpet/emul.h b/tools/tests/vhpet/emul.h
new file mode 100644
index 0000000..a6f19e4
--- /dev/null
+++ b/tools/tests/vhpet/emul.h
@@ -0,0 +1,405 @@
+/*
+ * Xen emulation for hpet
+ *
+ * Copyright (C) 2014 Verizon Corporation
+ *
+ * This file is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License Version 2 (GPLv2)
+ * as published by the Free Software Foundation.
+ *
+ * This file is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details. <http://www.gnu.org/licenses/>.
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <inttypes.h>
+#include <string.h>
+
+#define PCI_HAVE_64BIT_ADDRESS
+#include <pci/types.h>
+
+#include "hpet.h"
+
+#define NR_CPUS 8
+
+typedef int64_t s_time_t;
+typedef int spinlock_t;
+typedef int bool_t;
+
+#define BITS_PER_LONG __WORDSIZE
+#define BITS_TO_LONGS(bits) \
+    (((bits) + BITS_PER_LONG - 1) / BITS_PER_LONG)
+#define DECLARE_BITMAP(name, bits) \
+    unsigned long name[BITS_TO_LONGS(bits)]
+typedef struct cpumask
+{
+    DECLARE_BITMAP(bits, NR_CPUS);
+} cpumask_t;
+typedef cpumask_t *cpumask_var_t;
+struct msi_desc
+{
+    struct msi_attrib
+    {
+        u8    type    : 5;    /* {0: unused, 5h:MSI, 11h:MSI-X} */
+        u8    maskbit : 1;    /* mask-pending bit supported ?   */
+        u8    masked  : 1;
+        u8    is_64   : 1;    /* Address size: 0=32bit 1=64bit  */
+        u8    pos;            /* Location of the msi capability */
+        u16   entry_nr;       /* specific enabled entry         */
+    } msi_attrib;
+};
+
+struct msi_msg
+{
+    u32     address_lo;     /* low 32 bits of msi message address */
+    u32     address_hi;     /* high 32 bits of msi message address */
+    u32     data;           /* 16 bits of msi message data */
+    u32     dest32;         /* used when Interrupt Remapping with EIM is enabled */
+};
+
+#ifndef ARRAY_SIZE
+#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
+#endif
+
+#define X86EMUL_OKAY 100
+#define EINVAL 101
+
+#define DBG_LEVEL_PIT 200
+
+#define TRC_HW_VCHIP_HPET_START_TIMER 300
+#define TRC_HW_VCHIP_HPET_STOP_TIMER 301
+#define TRC_HW_VCHIP_PIT_STOP_TIMER 302
+
+#define TRC_HVM_VCHIP_HPET_START_TIMER 400
+#define TRC_HVM_VCHIP_HPET_STOP_TIMER 401
+#define TRC_HVM_VCHIP_PIT_STOP_TIMER 402
+
+#define TRC_HVM_EMUL_HPET_START_TIMER 400
+#define TRC_HVM_EMUL_HPET_STOP_TIMER 401
+#define TRC_HVM_EMUL_PIT_STOP_TIMER 402
+
+#define __read_mostly
+#define __initdata
+#define __init
+#define __maybe_unused
+#define __cacheline_aligned
+#define boolean_param(a, b)
+#define fix_to_virt(a) a
+#define xmalloc_array(_type, _num) (void *)(_type)(_num)
+#define DEFINE_PER_CPU(_type, _name) _type _name
+
+#define KERN_DEBUG
+#define KERN_INFO
+
+#define XENLOG_WARNING
+#define XENLOG_INFO
+#define XENLOG_ERR
+#define XENLOG_GUEST
+
+#define MSI_TYPE_UNKNOWN 0
+#define MSI_TYPE_HPET    1
+#define MSI_TYPE_IOMMU   2
+
+#define STIME_MAX ((s_time_t)((uint64_t)~0ull>>1))
+
+/* Low-latency softirqs come first in the following list. */
+enum
+{
+    TIMER_SOFTIRQ = 0,
+    SCHEDULE_SOFTIRQ,
+    NEW_TLBFLUSH_CLOCK_PERIOD_SOFTIRQ,
+    RCU_SOFTIRQ,
+    TASKLET_SOFTIRQ,
+    NR_COMMON_SOFTIRQS
+};
+/*
+ * ..and if you can't take the strict
+ * types, you can specify one yourself.
+ *
+ * Or not use min/max at all, of course.
+ */
+#define min_t(type, x, y) \
+    ({ type __x = (x); type __y = (y); __x < __y ? __x : __y; })
+#define max_t(type, x, y) \
+    ({ type __x = (x); type __y = (y); __x > __y ? __x : __y; })
+#define offsetof(t, m) ((unsigned long )&((t *)0)->m)
+#define container_of(ptr, type, member) ({              \
+        typeof( ((type *)0)->member ) *__mptr = (ptr);  \
+        (type *)( (char *)__mptr - offsetof(type,member) ); })
+
+struct domain;
+
+struct vcpu
+{
+    int vcpu_id;
+    struct domain *domain;
+};
+
+typedef void time_cb(struct vcpu *v, void *opaque);
+
+struct periodic_time
+{
+#define PTSRC_isa    1 /* ISA time source */
+#define PTSRC_lapic  2 /* LAPIC time source */
+    u8 source;                  /* PTSRC_ */
+};
+
+void destroy_periodic_time(struct periodic_time *pt);
+void create_periodic_time(
+    struct vcpu *v, struct periodic_time *pt, uint64_t delta,
+    uint64_t period, uint8_t irq, time_cb *cb, void *data);
+
+#define HPET_TIMER_NUM 3
+
+struct hpet_registers
+{
+    /* Memory-mapped, software visible registers */
+    uint64_t capability;        /* capabilities */
+    uint64_t config;            /* configuration */
+    uint64_t isr;               /* interrupt status reg */
+    uint64_t mc64;              /* main counter */
+    struct                      /* timers */
+    {
+        uint64_t config;        /* configuration/cap */
+        uint64_t cmp;           /* comparator */
+        uint64_t fsb;           /* FSB route, not supported now */
+    } timers[HPET_TIMER_NUM];
+
+    /* Hidden register state */
+    uint64_t period[HPET_TIMER_NUM]; /* Last value written to comparator */
+    uint64_t comparator64[HPET_TIMER_NUM]; /* 64 bit running comparator */
+    uint64_t first_mc64[HPET_TIMER_NUM]; /* 1st interval main counter */
+    uint8_t first_enabled[HPET_TIMER_NUM]; /* In 1st interval */
+};
+
+typedef struct HPETState
+{
+    struct hpet_registers hpet;
+    uint64_t stime_freq;
+    uint64_t hpet_to_ns_scale; /* hpet ticks to ns (multiplied by 2^10) */
+    uint64_t hpet_to_ns_limit; /* max hpet ticks convertable to ns      */
+    uint64_t mc_offset;
+    struct periodic_time pt[HPET_TIMER_NUM];
+    spinlock_t lock;
+} HPETState;
+
+typedef struct PITState
+{
+    struct periodic_time pt0;
+    spinlock_t lock;
+} PITState;
+
+
+struct pl_time      /* platform time */
+{
+    struct HPETState vhpet;
+    /* guest_time = Xen sys time + stime_offset */
+    int64_t stime_offset;
+    /* Ensures monotonicity in appropriate timer modes. */
+    uint64_t last_guest_time;
+    spinlock_t pl_time_lock;
+};
+
+#define HVM_PARAM_HPET_ENABLED 11
+
+struct hvm_domain
+{
+    struct pl_time         pl_time;
+    long params[20];
+};
+
+struct arch_domain
+{
+    struct hvm_domain hvm_domain;
+    struct PITState vpit;
+};
+
+struct domain
+{
+    int domain_id;
+    struct arch_domain arch;
+    struct vcpu *vcpu[NR_CPUS];
+};
+
+typedef int (*hvm_mmio_read_t)(struct vcpu *v,
+                               unsigned long addr,
+                               unsigned long length,
+                               unsigned long *val);
+typedef int (*hvm_mmio_write_t)(struct vcpu *v,
+                                unsigned long addr,
+                                unsigned long length,
+                                unsigned long val);
+typedef int (*hvm_mmio_check_t)(struct vcpu *v, unsigned long addr);
+
+
+struct hvm_mmio_handler
+{
+    hvm_mmio_check_t check_handler;
+    hvm_mmio_read_t read_handler;
+    hvm_mmio_write_t write_handler;
+};
+
+/* Marshalling and unmarshalling uses a buffer with size and cursor. */
+typedef struct hvm_domain_context
+{
+    uint32_t cur;
+    uint32_t size;
+    uint8_t *data;
+} hvm_domain_context_t;
+
+int current_domain_id(void);
+#define dprintk(_l, _f, _a...)                  \
+    printk(_l "%s:%d: " _f, __FILE__ , __LINE__ , ## _a )
+#define gdprintk(_l, _f, _a...)                         \
+    printk(XENLOG_GUEST _l "%s:%d:d%d " _f, __FILE__,   \
+           __LINE__, current_domain_id() , ## _a )
+struct vcpu *get_current();
+#define current get_current()
+
+#define HVM_SAVE_CODE(_x) HVM_SAVE_CODE_##_x
+#define HVM_SAVE_LENGTH(_x) HVM_SAVE_LENGTH_##_x
+
+/*
+ * HPET
+ */
+
+uint64_t hvm_get_guest_time(struct vcpu *v);
+
+#define HPET_TIMER_NUM     3    /* 3 timers supported now */
+struct hvm_hw_hpet
+{
+    /* Memory-mapped, software visible registers */
+    uint64_t capability;        /* capabilities */
+    uint64_t res0;              /* reserved */
+    uint64_t config;            /* configuration */
+    uint64_t res1;              /* reserved */
+    uint64_t isr;               /* interrupt status reg */
+    uint64_t res2[25];          /* reserved */
+    uint64_t mc64;              /* main counter */
+    uint64_t res3;              /* reserved */
+    struct                      /* timers */
+    {
+        uint64_t config;        /* configuration/cap */
+        uint64_t cmp;           /* comparator */
+        uint64_t fsb;           /* FSB route, not supported now */
+        uint64_t res4;          /* reserved */
+    } timers[HPET_TIMER_NUM];
+    uint64_t res5[4 * (24 - HPET_TIMER_NUM)]; /* reserved, up to 0x3ff */
+
+    /* Hidden register state */
+    uint64_t period[HPET_TIMER_NUM]; /* Last value written to comparator */
+};
+
+typedef int (*hvm_save_handler)(struct domain *d,
+                                hvm_domain_context_t *h);
+typedef int (*hvm_load_handler)(struct domain *d,
+                                hvm_domain_context_t *h);
+
+struct hvm_save_descriptor
+{
+    uint16_t typecode;          /* Used to demux the various types below */
+    uint16_t instance;          /* Further demux within a type */
+    uint32_t length;            /* In bytes, *not* including this descriptor */
+};
+
+void hvm_register_savevm(uint16_t typecode,
+                         const char *name,
+                         hvm_save_handler save_state,
+                         hvm_load_handler load_state,
+                         size_t size, int kind);
+
+#define HVMSR_PER_DOM 1
+
+#define HVM_REGISTER_SAVE_RESTORE(_x, _save, _load, _num, _k)       \
+    int __init __hvm_register_##_x##_save_and_restore(void)     \
+    {                                                                   \
+        hvm_register_savevm(HVM_SAVE_CODE(_x),                          \
+                            #_x,                                        \
+                            &_save,                                     \
+                            &_load,                                     \
+                            (_num) * (HVM_SAVE_LENGTH(_x)               \
+                                 + sizeof(struct hvm_save_descriptor)), \
+                            _k);                                        \
+        return 0;                                                       \
+    }                                                                   \
+
+#define HVM_SAVE_CODE_HPET 0
+#define HVM_SAVE_LENGTH_HPET sizeof(struct hvm_hw_hpet)
+
+#define printk printf
+
+#define spin_lock(a)
+#define spin_unlock(a)
+#define spin_lock_init(a)
+#define spin_is_locked(a) 1
+#define ASSERT(a)
+
+#define ADDR (*(volatile long *) addr)
+
+static inline void __set_bit(int nr, volatile void *addr)
+{
+    asm volatile(
+        "btsl %1,%0"
+        : "=m"(ADDR)
+        : "Ir"(nr), "m"(ADDR) : "memory");
+}
+
+static inline void __clear_bit(int nr, volatile void *addr)
+{
+    asm volatile(
+        "btrl %1,%0"
+        : "=m"(ADDR)
+        : "Ir"(nr), "m"(ADDR) : "memory");
+}
+
+static inline unsigned int find_first_set_bit(unsigned long word)
+{
+    asm("bsf %1,%0" : "=r"(word) : "r"(word));
+    return (unsigned int)word;
+}
+
+#define HVM_DBG_LOG(level, _f, _a...)                   \
+    do {                                \
+        printf("[HVM:%d.%d] <%s> " _f "\n",                             \
+               current->domain->domain_id, current->vcpu_id, __func__,  \
+               ## _a);                                                  \
+    } while ( 0 )
+
+void __domain_crash(struct domain *d);
+#define domain_crash(d) do {                        \
+        printf("domain_crash called from %s:%d\n", __FILE__, __LINE__); \
+        __domain_crash(d);                                              \
+    } while ( 0 )
+
+#define MICROSECS(_us) ((s_time_t)((_us) * 1000ULL))
+
+#define pt_global_vcpu_target(d)        \
+    ((d)->vcpu ? (d)->vcpu[0] : NULL)
+
+#define TRACE_0D(a)
+#define TRACE_1D(a, b)
+#define TRACE_2D(a, b, c)
+#define TRACE_3D(a, b, c, d)
+#define TRACE_4D(a, b, c, d, e)
+#define TRACE_5D(a, b, c, d, e, f)
+
+/* debug */
+
+extern int __read_mostly hpet_debug;
+extern uint64_t __read_mostly hpet_force_diff;
+extern uint64_t __read_mostly hpet_force_mc64;
+extern uint64_t __read_mostly hpet_force_cmp;
+extern uint64_t __read_mostly hpet_force_period;
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/tests/vhpet/main.c b/tools/tests/vhpet/main.c
new file mode 100644
index 0000000..c74ed85
--- /dev/null
+++ b/tools/tests/vhpet/main.c
@@ -0,0 +1,541 @@
+/*
+ * Xen emulation for hpet
+ *
+ * Copyright (C) 2014 Verizon Corporation
+ *
+ * This file is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License Version 2 (GPLv2)
+ * as published by the Free Software Foundation.
+ *
+ * This file is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details. <http://www.gnu.org/licenses/>.
+ */
+
+/*
+ * http://www.intel.com/content/dam/www/public/us/en/documents/technical-specifications/software-developers-hpet-spec-1-0a.pdf
+ *
+ * xen_source is a directory that has all xen source below it.
+ *
+ * Usage:
+ *
+
+
+  xen_source=../../..
+  sed -e "/#include/d" -e "1i#include \"emul.h\"\n" <$xen_source/xen/arch/x86/hvm/hpet.c >hpet.c
+  cp $xen_source/xen/include/asm-x86/hpet.h .
+
+  gcc -g -o test_vhpet hpet.c main.c
+  ./test_vhpet >foo
+
+ *
+ *
+ * Also bad values for 3243567890123456 just not negative.
+ *
+ */
+
+#define FORCE_THOUSANDS_SEP
+
+#include <locale.h>
+#include <langinfo.h>
+#include <stdarg.h>
+#include "emul.h"
+#include "hpet.h"
+
+#define S_TO_NS    1000000000ULL           /* 1s  = 10^9  ns */
+
+#define START_MC64 0x108a8
+
+static int hpet_mult = 1;
+static int hpet_add;
+static int hvm_clock_cost = 1234567;
+static int tick_count = 1;
+static int debug = 3;
+
+static int skip_load;
+
+static char *global_thousep;
+
+extern const struct hvm_mmio_handler hpet_mmio_handler;
+
+struct domain dom1;
+struct vcpu vcpu0;
+struct hvm_hw_hpet hpet_save;
+
+
+uint64_t hvm_guest_time;
+
+static struct
+{
+    hvm_save_handler save;
+    hvm_load_handler load;
+    const char *name;
+    size_t size;
+    int kind;
+} hvm_sr_handlers[3] = {{NULL, NULL, "<?>"},};
+
+static uint64_t new_guest_time[] = {
+    0x20,
+    0x2a840,
+    0xf4200,
+    0x10000000000ULL,
+    0x0fffffffffefff00ULL,
+    0x20,
+    0xffffffff00000000ULL,
+    0x20,
+};
+
+static int print_error(const char *fmt, ...)
+{
+    va_list args;
+    int i = 0;
+
+    va_start(args, fmt);
+    if ( debug & 0x0001 )
+        i = vfprintf(stdout, fmt, args);
+    va_end(args);
+    va_start(args, fmt);
+    if ( debug & 0x0002 )
+        i = vfprintf(stderr, fmt, args);
+    va_end(args);
+    return i;
+}
+
+
+int current_domain_id(void)
+{
+    return current->domain->domain_id;
+}
+
+struct vcpu *get_current()
+{
+    return &vcpu0;
+}
+
+void __domain_crash(struct domain *d)
+{
+    exit(42);
+}
+
+uint64_t hvm_get_guest_time(struct vcpu *v)
+{
+    uint64_t ret = hvm_guest_time;
+
+    hvm_guest_time += hvm_clock_cost;
+    return ret;
+}
+
+int _hvm_init_entry(struct hvm_domain_context *h,
+                    uint16_t tc, uint16_t inst, uint32_t len)
+{
+    h->cur = 0;
+    h->size = sizeof(hpet_save);
+    h->data = (void *)&hpet_save;
+
+    return 0;
+}
+
+int _hvm_check_entry(struct hvm_domain_context *h,
+                     uint16_t type, uint32_t len, bool_t strict_length)
+{
+    h->cur = 0;
+    h->size = sizeof(hpet_save);
+    h->data = (void *)&hpet_save;
+
+    return 0;
+}
+
+void __init hvm_register_savevm(uint16_t typecode,
+                                const char *name,
+                                hvm_save_handler save_state,
+                                hvm_load_handler load_state,
+                                size_t size, int kind)
+{
+    hvm_sr_handlers[typecode].save = save_state;
+    hvm_sr_handlers[typecode].load = load_state;
+    hvm_sr_handlers[typecode].name = name;
+    hvm_sr_handlers[typecode].size = size;
+    hvm_sr_handlers[typecode].kind = kind;
+}
+
+int do_save(uint16_t typecode, struct domain *d, hvm_domain_context_t *h)
+{
+    return hvm_sr_handlers[typecode].save(d, h);
+}
+
+int do_load(uint16_t typecode, struct domain *d, hvm_domain_context_t *h)
+{
+    if (skip_load > 0)
+    {
+        printf("skip_load\n");
+    }
+    else
+    {
+        printf("do_load\n");
+        return hvm_sr_handlers[typecode].load(d, h);
+    }
+}
+
+static void dump_hpet(void)
+{
+    int i;
+    unsigned long long conf;
+    struct hvm_hw_hpet h = hpet_save;
+    conf = (unsigned long long) h.config;
+    printf("    HPET: capability %#llx config %#llx(%s%s)\n",
+           (unsigned long long) h.capability,
+           conf,
+           conf & HPET_CFG_ENABLE ? "E" : "",
+           conf & HPET_CFG_LEGACY ? "L" : "");
+    printf("          isr %#llx counter %#llx(%'lld)\n",
+           (unsigned long long) h.isr,
+           (unsigned long long) h.mc64,
+           (unsigned long long) h.mc64);
+    for (i = 0; i < HPET_TIMER_NUM; i++)
+    {
+        conf = (unsigned long long) h.timers[i].config;
+        printf("          timer%i config %#llx(%s%s%s) cmp %#llx(%'lld)\n", i,
+               conf,
+               conf & HPET_TN_ENABLE ? "E" : "",
+               conf & HPET_TN_PERIODIC ? "P" : "",
+               conf & HPET_TN_32BIT ? "32" : "",
+               (unsigned long long) h.timers[i].cmp,
+               (unsigned long long) h.timers[i].cmp);
+        printf("          timer%i period %#llx(%'lld) fsb %#llx\n", i,
+               (unsigned long long) h.period[i],
+               (unsigned long long) h.period[i],
+               (unsigned long long) h.timers[i].fsb);
+    }
+}
+
+void pit_stop_channel0_irq(PITState *pit)
+{
+    printf("pit_stop_channel0_irq: pit=%p\n", pit);
+
+    TRACE_1D(TRC_HVM_VCHIP_PIT_STOP_TIMER, get_cycles());
+    spin_lock(&pit->lock);
+    destroy_periodic_time(&pit->pt0);
+    spin_unlock(&pit->lock);
+}
+
+void destroy_periodic_time(struct periodic_time *pt)
+{
+    int idx = ((long)pt) & 0x7;
+
+    printf("destroy_periodic_time: pt=%d\n", idx);
+}
+
+void create_periodic_time(
+    struct vcpu *v, struct periodic_time *pt, uint64_t delta,
+    uint64_t period, uint8_t irq, time_cb *cb, void *data)
+{
+    int idx = ((long)pt) & 0x7;
+
+    if ( period )
+    {
+        printf("create_periodic_time: pt=%d delta=%'"PRId64" period=%'"PRIu64
+               " - %'"PRIu64".%02d Hz irq=%d\n",
+               idx, delta, period, (uint64_t)(S_TO_NS / period),
+               (int)((S_TO_NS / (period / 100ULL)) % 100), irq);
+        /* +160 is for hpet_tick_to_ns() not simple. */
+        if ( delta > (period * (hpet_mult + hpet_add + 160)) )
+            print_error("Possible ..MP-BIOS bug: 8254 timer...: delta=%'"PRId64
+                        " period=%'"PRIu64"\n",
+                        delta, period);
+    }
+    else
+        printf("create_periodic_time: pt=%d delta=%'"PRId64
+               " period=%'"PRIu64" irq=%d\n",
+               idx, delta, period, irq);
+}
+
+void udelay(int w)
+{
+}
+
+unsigned int hpet_readl(unsigned long a)
+{
+    unsigned long ret = 0;
+    hpet_mmio_handler.read_handler(current, a, 4, &ret);
+    return ret;
+}
+
+void hpet_writel(unsigned long d, unsigned long a)
+{
+    hpet_mmio_handler.write_handler(current, a, 4, d);
+    return;
+}
+
+static void _hpet_print_config(const char *function, int line)
+{
+    u32 i, timers, l, h;
+    printk(KERN_INFO "hpet: %s(%d):\n", function, line);
+    l = hpet_readl(HPET_ID);
+    h = hpet_readl(HPET_PERIOD);
+    timers = ((l & HPET_ID_NUMBER) >> HPET_ID_NUMBER_SHIFT) + 1;
+    printk(KERN_INFO "hpet: ID: 0x%x, PERIOD: 0x%x\n", l, h);
+    l = hpet_readl(HPET_CFG);
+    h = hpet_readl(HPET_STATUS);
+    printk(KERN_INFO "hpet: CFG: 0x%x, STATUS: 0x%x\n", l, h);
+    l = hpet_readl(HPET_COUNTER);
+    h = hpet_readl(HPET_COUNTER + 4);
+    printk(KERN_INFO "hpet: COUNTER_l: 0x%x, COUNTER_h: 0x%x\n", l, h);
+
+    for (i = 0; i < timers; i++)
+    {
+        l = hpet_readl(HPET_Tn_CFG(i));
+        h = hpet_readl(HPET_Tn_CFG(i) + 4);
+        printk(KERN_INFO "hpet: T%d: CFG_l: 0x%x, CFG_h: 0x%x\n",
+               i, l, h);
+        l = hpet_readl(HPET_Tn_CMP(i));
+        h = hpet_readl(HPET_Tn_CMP(i) + 4);
+        printk(KERN_INFO "hpet: T%d: CMP_l: 0x%x, CMP_h: 0x%x\n",
+               i, l, h);
+        l = hpet_readl(HPET_Tn_ROUTE(i));
+        h = hpet_readl(HPET_Tn_ROUTE(i) + 4);
+        printk(KERN_INFO "hpet: T%d ROUTE_l: 0x%x, ROUTE_h: 0x%x\n",
+               i, l, h);
+    }
+}
+
+#define hpet_print_config()                                     \
+    do {                                                            \
+        _hpet_print_config(__func__, __LINE__);     \
+    } while ( 0 )
+
+static void hpet_stop_counter(void)
+{
+    unsigned long cfg = hpet_readl(HPET_CFG);
+    cfg &= ~HPET_CFG_ENABLE;
+    hpet_writel(cfg, HPET_CFG);
+}
+
+static void hpet_reset_counter(void)
+{
+    hpet_writel(0, HPET_COUNTER);
+    hpet_writel(0, HPET_COUNTER + 4);
+}
+
+static void hpet_start_counter(void)
+{
+    unsigned long cfg = hpet_readl(HPET_CFG);
+    cfg |= HPET_CFG_ENABLE;
+    hpet_writel(cfg, HPET_CFG);
+}
+
+static void hpet_restart_counter(void)
+{
+    hpet_stop_counter();
+    hpet_reset_counter();
+    hpet_start_counter();
+}
+
+static void hpet_set_mode(uint64_t delta, int timer)
+{
+    unsigned long cfg, cmp, cmp2, now;
+
+    hpet_stop_counter();
+    now = hpet_readl(HPET_COUNTER);
+    cmp = now + (unsigned long)(hpet_mult * delta) + hpet_add;
+    cfg = hpet_readl(HPET_Tn_CFG(timer));
+    /* Make sure we use edge triggered interrupts */
+    cfg &= ~HPET_TN_LEVEL;
+    cfg |= HPET_TN_ENABLE | HPET_TN_PERIODIC |
+           HPET_TN_SETVAL | HPET_TN_32BIT;
+    /* Mask to 32 bits just like the hardware */
+    cmp = (uint32_t)cmp;
+    delta = (uint32_t)delta;
+    /* Do the config */
+    hpet_writel(cfg, HPET_Tn_CFG(timer));
+    hpet_writel(cmp, HPET_Tn_CMP(timer));
+    printf("hpet_writel: HPET_TN_SETVAL cmp=%#lx(%'ld) timer=%d\n",
+           cmp, cmp, timer);
+    udelay(1);
+    /*
+     * HPET on AMD 81xx needs a second write (with HPET_TN_SETVAL
+     * cleared) to T0_CMP to set the period. The HPET_TN_SETVAL
+     * bit is automatically cleared after the first write.
+     * (See AMD-8111 HyperTransport I/O Hub Data Sheet,
+     * Publication # 24674)
+     */
+    hpet_writel((unsigned long) delta, HPET_Tn_CMP(timer));
+    printf("hpet_writel: period=%#lx(%'ld) timer=%d\n",
+           (unsigned long) delta, (unsigned long) delta, timer);
+    cmp2 = hpet_readl(HPET_Tn_CMP(timer));
+    if ( cmp2 != cmp )
+        print_error("hpet: T%d Error: Set %#lx(%'ld) != %#lx(%'ld)\n",
+                    timer, cmp, cmp, cmp2, cmp2);
+    hpet_start_counter();
+    hpet_print_config();
+}
+
+int
+main(int argc, char **argv)
+{
+    hvm_domain_context_t hdc;
+    struct hvm_hw_hpet hpet0;
+    struct hvm_hw_hpet hpet1;
+    struct hvm_hw_hpet hpet2;
+    int i, k;
+
+    setlocale(LC_ALL, "");
+
+#ifdef FORCE_THOUSANDS_SEP
+    setlocale(LC_NUMERIC, "en_US.utf8");
+#endif
+    global_thousep = nl_langinfo(THOUSEP);
+
+    printf("test_vhpet 1.0\n");
+
+    if ( argc > 1 )
+        hvm_clock_cost = atoi(argv[1]);
+    if ( argc > 2 )
+        hpet_mult = atoi(argv[2]);
+    if ( argc > 3 )
+        hpet_add = atoi(argv[3]);
+    if ( argc > 4 )
+        tick_count = atoi(argv[4]);
+    if ( argc > 5 )
+        debug = strtol(argv[5], NULL, 0);
+
+    printf("hvm_clock_cost=%'d hpet_mult=%'d hpet_add=%'d tick_count=%d debug=%#x\n",
+           hvm_clock_cost, hpet_mult, hpet_add, tick_count, debug);
+
+    dom1.domain_id = 1;
+    dom1.vcpu[0] = &vcpu0;
+    vcpu0.vcpu_id = 0;
+    vcpu0.domain = &dom1;
+
+    __hvm_register_HPET_save_and_restore();
+
+    for (skip_load = 1; skip_load >= 0; skip_load--)
+    {
+
+        printf("\nskip_load=%d\n", skip_load);
+
+        hvm_guest_time = 16;
+
+        hpet_init(&vcpu0);
+
+        do_save(HVM_SAVE_CODE(HPET), &dom1, &hdc);
+        dump_hpet();
+        hpet0 = hpet_save;
+        do_save(HVM_SAVE_CODE(HPET), &dom1, &hdc);
+        dump_hpet();
+        hpet1 = hpet_save;
+        if (hpet0.mc64 != hpet1.mc64)
+            print_error("With clock stopped mc64 changed: %'ld to %'ld\n",
+                        hpet0.mc64, hpet1.mc64);
+
+        do_load(HVM_SAVE_CODE(HPET), &dom1, &hdc);
+        do_save(HVM_SAVE_CODE(HPET), &dom1, &hdc);
+        dump_hpet();
+        hpet2 = hpet_save;
+        if (hpet1.mc64 != hpet2.mc64)
+            print_error("With clock stopped mc64 changed: %'ld to %'ld\n",
+                        hpet1.mc64, hpet2.mc64);
+
+        dom1.arch.hvm_domain.pl_time.vhpet.hpet.mc64 = START_MC64;
+        dom1.arch.hvm_domain.pl_time.vhpet.mc_offset = START_MC64
+            - hvm_guest_time - hvm_clock_cost;
+        printf("\n"
+               "mc64=%#lx(%'ld) mc_offset=%#lx(%'ld)\n",
+               dom1.arch.hvm_domain.pl_time.vhpet.hpet.mc64,
+               dom1.arch.hvm_domain.pl_time.vhpet.hpet.mc64,
+               dom1.arch.hvm_domain.pl_time.vhpet.mc_offset,
+               dom1.arch.hvm_domain.pl_time.vhpet.mc_offset);
+
+        printf("\nhvm_guest_time=%#lx(%'ld)\n",
+               hvm_guest_time, hvm_guest_time);
+
+        do_save(HVM_SAVE_CODE(HPET), &dom1, &hdc);
+        dump_hpet();
+        hpet0 = hpet_save;
+        do_save(HVM_SAVE_CODE(HPET), &dom1, &hdc);
+        dump_hpet();
+        hpet1 = hpet_save;
+        if (hpet0.mc64 != hpet1.mc64)
+            print_error("With clock stopped mc64 changed: %'ld to %'ld\n",
+                        hpet0.mc64, hpet1.mc64);
+
+        do_load(HVM_SAVE_CODE(HPET), &dom1, &hdc);
+        do_save(HVM_SAVE_CODE(HPET), &dom1, &hdc);
+        dump_hpet();
+        hpet2 = hpet_save;
+        if (hpet1.mc64 != hpet2.mc64)
+            print_error("With clock stopped mc64 changed: %'ld to %'ld\n",
+                        hpet1.mc64, hpet2.mc64);
+
+        hpet_set_mode(0xf424, 0);
+        hpet_set_mode(0, 1);
+#ifdef BIG
+        hpet_set_mode(~0ULL, 2);
+#else
+        hpet_set_mode(0x80000000, 2);
+#endif
+
+        do_save(HVM_SAVE_CODE(HPET), &dom1, &hdc);
+        dump_hpet();
+        hpet0 = hpet_save;
+        do_save(HVM_SAVE_CODE(HPET), &dom1, &hdc);
+        dump_hpet();
+        hpet1 = hpet_save;
+
+        do_load(HVM_SAVE_CODE(HPET), &dom1, &hdc);
+        do_save(HVM_SAVE_CODE(HPET), &dom1, &hdc);
+        dump_hpet();
+        hpet2 = hpet_save;
+
+
+        for (k = 0; k < ARRAY_SIZE(new_guest_time); k++)
+        {
+            hvm_guest_time = new_guest_time[k];
+            printf("\nhvm_guest_time=%#lx(%'ld)\n",
+                   hvm_guest_time, hvm_guest_time);
+
+            do_save(HVM_SAVE_CODE(HPET), &dom1, &hdc);
+            dump_hpet();
+            hpet0 = hpet_save;
+            do_save(HVM_SAVE_CODE(HPET), &dom1, &hdc);
+            dump_hpet();
+            hpet1 = hpet_save;
+
+            do_load(HVM_SAVE_CODE(HPET), &dom1, &hdc);
+            do_save(HVM_SAVE_CODE(HPET), &dom1, &hdc);
+            dump_hpet();
+            hpet2 = hpet_save;
+
+            for (i = 0; i < tick_count; i++)
+            {
+                hvm_guest_time += 0x10;
+                printf("\nhvm_guest_time=%#lx(%'ld)\n",
+                       hvm_guest_time, hvm_guest_time);
+
+                do_save(HVM_SAVE_CODE(HPET), &dom1, &hdc);
+                dump_hpet();
+                hpet0 = hpet_save;
+                do_save(HVM_SAVE_CODE(HPET), &dom1, &hdc);
+                dump_hpet();
+                hpet1 = hpet_save;
+
+                do_load(HVM_SAVE_CODE(HPET), &dom1, &hdc);
+                do_save(HVM_SAVE_CODE(HPET), &dom1, &hdc);
+                dump_hpet();
+                hpet2 = hpet_save;
+
+            }
+        }
+    }
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
1.8.4

Re: [PATCH v2 01/10] hvm/hpet: Add manual unit test code.

[Jan Beulich]

>>> On 08.04.14 at 16:24, <dslutz@verizon.com> wrote:
> Add the code at tools/tests/vhpet.
> 
> Does repro the bug:
> 
> ..MP-BIOS bug: 8254 timer not connected to IO-APIC
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> ---
>  tools/tests/vhpet/.gitignore |   4 +
>  tools/tests/vhpet/emul.h     | 405 ++++++++++++++++++++++++++++++++
>  tools/tests/vhpet/main.c     | 541 +++++++++++++++++++++++++++++++++++++++++++

So I'm having difficulty understanding what this is good for, or how
it is supposed to be used: You don't add or change any Makefile, and
don't say anything to explain how this code is reproducing the bug
you mention above. Please be a little more verbose.

Jan

Re: [PATCH v2 01/10] hvm/hpet: Add manual unit test code.

[Don Slutz]

On 04/09/14 12:08, Jan Beulich wrote:
>>>> On 08.04.14 at 16:24, <dslutz@verizon.com> wrote:
>> Add the code at tools/tests/vhpet.
>>
>> Does repro the bug:
>>
>> ..MP-BIOS bug: 8254 timer not connected to IO-APIC
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> ---
>>   tools/tests/vhpet/.gitignore |   4 +
>>   tools/tests/vhpet/emul.h     | 405 ++++++++++++++++++++++++++++++++
>>   tools/tests/vhpet/main.c     | 541 +++++++++++++++++++++++++++++++++++++++++++
> So I'm having difficulty understanding what this is good for, or how
> it is supposed to be used: You don't add or change any Makefile, and
> don't say anything to explain how this code is reproducing the bug
> you mention above. Please be a little more verbose.
>
> Jan
>
Ok.

In a xen source tree (with this patch applied):

   cd tools/tests/vhpet

   xen_source=../../..
   sed -e "/#include/d" -e "1i#include \"emul.h\"\n" <$xen_source/xen/arch/x86/hvm/hpet.c >hpet.c
   cp $xen_source/xen/include/asm-x86/hpet.h .

   gcc -g -o test_vhpet hpet.c main.c
   ./test_vhpet >foo


Most of this is in main.c's comment.  Will add to commit message also.

    -Don Slutz

Re: [PATCH v2 01/10] hvm/hpet: Add manual unit test code.

[Jan Beulich]

>>> On 09.04.14 at 20:35, <dslutz@verizon.com> wrote:
> On 04/09/14 12:08, Jan Beulich wrote:
>>>>> On 08.04.14 at 16:24, <dslutz@verizon.com> wrote:
>>> Add the code at tools/tests/vhpet.
>>>
>>> Does repro the bug:
>>>
>>> ..MP-BIOS bug: 8254 timer not connected to IO-APIC
>>>
>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>>> ---
>>>   tools/tests/vhpet/.gitignore |   4 +
>>>   tools/tests/vhpet/emul.h     | 405 ++++++++++++++++++++++++++++++++
>>>   tools/tests/vhpet/main.c     | 541 +++++++++++++++++++++++++++++++++++++++++++
>> So I'm having difficulty understanding what this is good for, or how
>> it is supposed to be used: You don't add or change any Makefile, and
>> don't say anything to explain how this code is reproducing the bug
>> you mention above. Please be a little more verbose.
> 
> In a xen source tree (with this patch applied):
> 
>    cd tools/tests/vhpet
> 
>    xen_source=../../..
>    sed -e "/#include/d" -e "1i#include \"emul.h\"\n" <$xen_source/xen/arch/x86/hvm/hpet.c >hpet.c
>    cp $xen_source/xen/include/asm-x86/hpet.h .
> 
>    gcc -g -o test_vhpet hpet.c main.c

At least up to here this clearly should be in a Makefile, just like e.g.
done for the x86 instruction emulator test.

>    ./test_vhpet >foo
> 
> 
> Most of this is in main.c's comment.  Will add to commit message also.

Just referring to the comments there would be enough I guess. But
I'm still struggling to see how this would reproduce the Linux side
issue, and not just some random problem manifesting in similar
symptoms.

Jan

Re: [PATCH v2 01/10] hvm/hpet: Add manual unit test code.

[Don Slutz]

On 04/10/14 02:11, Jan Beulich wrote:
>>>> On 09.04.14 at 20:35,<dslutz@verizon.com>  wrote:

>> In a xen source tree (with this patch applied):
>>
>>     cd tools/tests/vhpet
>>
>>     xen_source=../../..
>>     sed -e "/#include/d" -e "1i#include \"emul.h\"\n" <$xen_source/xen/arch/x86/hvm/hpet.c >hpet.c
>>     cp $xen_source/xen/include/asm-x86/hpet.h .
>>
>>     gcc -g -o test_vhpet hpet.c main.c
> At least up to here this clearly should be in a Makefile, just like e.g.
> done for the x86 instruction emulator test.

Opps, I just noticed that the "optional" got dropped.  I had not planned
on this being required to get these patches applied.  I have not found
a simple way to use git-format-patch and have just 1 subject different.

Will work on using a makefile.  I will be happy to make this into a much
better test case. Since that will not happen quickly, it make sense to me
to drop just this 1 patch and start a new set that includes it.

I have no real clue on how to get it to be part of "normal" testing.

This is because "make test" gives me:

...
building 'checkpoint' extension
gcc -pthread -fno-strict-aliasing -O2 -g -pipe -Wall 
-Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector 
--param=ssp-buffer-size=4 -m64 -mtune=generic -D_GNU_SOURCE -fPIC 
-fwrapv -DNDEBUG -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions 
-fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic 
-D_GNU_SOURCE -fPIC -fwrapv -fPIC -I../../tools/include 
-I../../tools/libxc -I../../tools/xenstore -I/usr/include/python2.7 -c 
xen/lowlevel/checkpoint/checkpoint.c -o 
build/temp.linux-x86_64-2.7/xen/lowlevel/checkpoint/checkpoint.o 
-fno-strict-aliasing -Werror
gcc -pthread -fno-strict-aliasing -O2 -g -pipe -Wall 
-Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector 
--param=ssp-buffer-size=4 -m64 -mtune=generic -D_GNU_SOURCE -fPIC 
-fwrapv -DNDEBUG -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions 
-fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic 
-D_GNU_SOURCE -fPIC -fwrapv -fPIC -I../../tools/include 
-I../../tools/libxc -I../../tools/xenstore -I/usr/include/python2.7 -c 
xen/lowlevel/checkpoint/libcheckpoint.c -o 
build/temp.linux-x86_64-2.7/xen/lowlevel/checkpoint/libcheckpoint.o 
-fno-strict-aliasing -Werror
xen/lowlevel/checkpoint/libcheckpoint.c: In function 'stop_suspend_thread':
xen/lowlevel/checkpoint/libcheckpoint.c:842:7: error: variable 'err' set 
but not used [-Werror=unused-but-set-variable]
cc1: all warnings being treated as errors
error: command 'gcc' failed with exit status 1
Build failed 0x100
make[1]: *** [test] Error 1
make[1]: Leaving directory `/home/don/xen/tools/python'
make: *** [test] Error 2
dcs-xen-54:~/xen>make tests
make: *** No rule to make target `tests'.  Stop.
dcs-xen-54:~/xen>make check
make: *** No rule to make target `check'.  Stop.
dcs-xen-54:~/xen>e Makefile
dcs-xen-54:~/xen>make -C tools test
make: Entering directory `/home/don/xen/tools'
make: *** No rule to make target `test'.  Stop.
make: Leaving directory `/home/don/xen/tools'


And 4.3.1 on CentOS 5.10:

...
======================================================================
ERROR: Invalid Test (xen.xm.tests.test_create)
----------------------------------------------------------------------
Traceback (most recent call last):
   File "test.py", line 634, in get_suite
     mod = package_import(modname)
   File "test.py", line 608, in package_import
     mod = __import__(modname)
   File 
"/home/don/xen-4.3.1/tools/python/build/lib.linux-x86_64-2.4/xen/xm/tests/test_create.py", 
line 6, in ?
     import xen.xend.XendOptions
   File 
"/home/don/xen-4.3.1/tools/python/build/lib.linux-x86_64-2.4/xen/xend/XendOptions.py", 
line 35, in ?
     from xen.util import auxbin
   File 
"/home/don/xen-4.3.1/tools/python/build/lib.linux-x86_64-2.4/xen/util/auxbin.py", 
line 22, in ?
     from xen.util.path import *
ImportError: No module named path

----------------------------------------------------------------------
Ran 5 tests in 0.048s

FAILED (errors=3)
make[1]: Leaving directory `/home/don/xen-4.3.1/tools/python'





>>     ./test_vhpet >foo
>>
>>
>> Most of this is in main.c's comment.  Will add to commit message also.
> Just referring to the comments there would be enough I guess. But
> I'm still struggling to see how this would reproduce the Linux side
> issue, and not just some random problem manifesting in similar
> symptoms.

I will attempt to answer this.


During my looking for what was happening, I added debug code
that would allow me to force "diff" to selected values.  This was
how I found out that "-diff > HPET_TINY_TIME_SPAN" would cause
linux to report this and crash.  On closer looking into this, I was
able to determine that the use extInt path would also fail even if I
got xen to provide this interrupt.  The reason being that

  (uint32_t)(-HPET_TINY_TIME_SPAN-1)

Sets the  "delta" (ns) to more then 60 seconds in the future.  And
the timer test happens in the 1st 5 seconds of a linux boot on my
test server.

So I send out the v1 patch.

Later I found out that any value that is > ~3 seconds would also
cause a linux crash even if xen is changed to provide extInt
interrupts.  (And you said in:

http://lists.xen.org/archives/html/xen-devel/2014-02/msg01857.html

But in the end I think you're barking up the wrong tree: All of this
code would be of no interest at all if Linux didn't for some reason go
into that fallback code. And with that fallback code existing mainly
(if not exclusively) to deal with (half-)broken hardware/firmware, we
should really make sure our emulation isn't broken (i.e. the fallback
never being made use of). So finding the "TBD reason" is what is
really needed.


(This e-mail also has the way I was able to get xen to "handle" extInt.
and so far this "adjustment" to xen is not ready to be posted as a fix;
including a good reason to add the code.)


This caused me to send lots of time reading the hpet spec
and hopefully understanding most of it.  At the same time
(and while waiting for a longer running test of Windows Vista)
I would add additional debug output and trace output along with
changes.  The the turn around time was slow enough that
it made sense to me to take the code and run it standalone.

This is what this code does.  It allows you to use an almost unmodified
copy of hpet.c in a stand alone mode that needs nothing else from
the xen source tree nor even have xen installed.

Also during the long test time I noticed that using xen-hvmctx
the output of cmp was not changing, but master clock was.
This is what leads me to patch #7:

hvm/hpet: Call hpet_get_comparator during hpet_save.

Which is not needed do to the way hpet_get_comparator() works
until patch #10:

hvm/hpet: handle 1st period special

Which will not work correctly without patch #7.

Also during the long time run I found out  using "xentrace: Add
TRC_HVM_EMUL" (at the testing time it was TRC_HW_VCHIP, but
that is mostly a rename) and some hvm_debug code I had added
showed that the delta between guest_time_hpet() was offten ~200.
However I would some times get much larger values.

I also found out that linux expects that "delta" should be "period".

This was the key to finding the issue that patch #3:

hvm/hpet: Only set comparator or period not both

Fixes.  However since the spec says that "delta" can be any
relation to  "period"; using what linux wanted ("delta" should
never be greater then "period") would be a bad change.

Since the server that would report:

..MP-BIOS bug: 8254 timer..

Stopped doing this, and we were unable to reproduce this
anywhere else; the best I could do was guess that some how
the delta between the 2 calls to guest_time_hpet() in:


     tn_cmp   = hpet_get_comparator(h, tn);
     cur_tick = hpet_read_maincounter(h);


Would be a possible cause.  It was not until patches #2 to #9
had been coded and tested and I was still looking for a good way
to do patch #10 (most of my attempts would break something
else) was I able to find the right set of conditions to get the test
code to report on a bad "delta".


I am not sure at what time I noticed patch #9:

hvm/hpet: Correctly limit period to a maximum.

This patch set order is not in the order of issues found.  Just
one that makes sense to me and was easy to separate into
only 1 change per patch.

     -Don Slutz

> Jan
>

Re: [PATCH v2 01/10] hvm/hpet: Add manual unit test code.

[Jan Beulich]

>>> On 11.04.14 at 04:53, <dslutz@verizon.com> wrote:
> This is because "make test" gives me:

I don't think you need to be worried about this failing - just make sure
"make -C .../tools/tests/hpet" works, and ideally (again just like the
x86 emulator one) also have a "run" target in the Makefile.

> During my looking for what was happening, I added debug code
> that would allow me to force "diff" to selected values.  This was
> how I found out that "-diff > HPET_TINY_TIME_SPAN" would cause
> linux to report this and crash.  On closer looking into this, I was
> able to determine that the use extInt path would also fail even if I
> got xen to provide this interrupt.  The reason being that
> 
>   (uint32_t)(-HPET_TINY_TIME_SPAN-1)
> 
> Sets the  "delta" (ns) to more then 60 seconds in the future.  And
> the timer test happens in the 1st 5 seconds of a linux boot on my
> test server.
> 
> So I send out the v1 patch.
> 
> Later I found out that any value that is > ~3 seconds would also
> cause a linux crash even if xen is changed to provide extInt
> interrupts.

But Linux expectations shouldn't matter in this discussion at all,
except for verifying that changes don't break things. I.e. any
change you propose should be explained comparing with real
hardware behavior, not with what Linux (or Windows, or ...)
expect. OS expectation should become a reason for a change only
if there is something that we absolutely can't make behave
hardware like.

Jan

Re: [PATCH optional v2 01/10] hvm/hpet: Add manual unit test code.

[Don Slutz]

On 04/11/14 03:45, Jan Beulich wrote:
>>>> On 11.04.14 at 04:53, <dslutz@verizon.com> wrote:
>> This is because "make test" gives me:
> I don't think you need to be worried about this failing - just make sure
> "make -C .../tools/tests/hpet" works, and ideally (again just like the
> x86 emulator one) also have a "run" target in the Makefile.

I see that that test is not passing:

make -C tools/tests/x86_emulator run
...
Testing daa/das (all inputs)...         skipped
Testing movq %mm3,(%ecx)...             okay
Testing movq (%edx),%mm5...             okay
Testing movdqu %xmm2,(%ecx)...          okay
Testing movdqu (%edx),%xmm4...          okay
Testing vmovdqu %ymm2,(%ecx)...         failed!
make: *** [run] Error 1
make: Leaving directory `/home/don/xen/tools/tests/x86_emulator'

But I will use it as a guide.

The code is still missing many checks.  Including the fixes in later 
patches.
Most of this checking is not simple to implement.  Will add just a basic
Makefile.

>> During my looking for what was happening, I added debug code
>> that would allow me to force "diff" to selected values.  This was
>> how I found out that "-diff > HPET_TINY_TIME_SPAN" would cause
>> linux to report this and crash.  On closer looking into this, I was
>> able to determine that the use extInt path would also fail even if I
>> got xen to provide this interrupt.  The reason being that
>>
>>    (uint32_t)(-HPET_TINY_TIME_SPAN-1)
>>
>> Sets the  "delta" (ns) to more then 60 seconds in the future.  And
>> the timer test happens in the 1st 5 seconds of a linux boot on my
>> test server.
>>
>> So I send out the v1 patch.
>>
>> Later I found out that any value that is > ~3 seconds would also
>> cause a linux crash even if xen is changed to provide extInt
>> interrupts.
> But Linux expectations shouldn't matter in this discussion at all,
> except for verifying that changes don't break things. I.e. any
> change you propose should be explained comparing with real
> hardware behavior, not with what Linux (or Windows, or ...)
> expect. OS expectation should become a reason for a change only
> if there is something that we absolutely can't make behave
> hardware like.

I fully agree.  That is why none of commit messages after #2
have anything about linux. And in #2 I do not think it is used as
a Linux expectation.

 From #2:

  The software-developers-hpet-spec-1-0a.pdf does not say how long it
  takes after the main clock is enabled before the first change of the
  master clock.  Therefore multiple calls to guest_time_hpet(h) are
  not needed.  Since each timer is started by a loop, each ones start
  time will change on the multple calls.  In the real hardware, there
  is not delta based on which timer.


Is what I think you are looking for.

   -Don Slutz

> Jan
>

Re: [PATCH optional v2 01/10] hvm/hpet: Add manual unit test code.

[Jan Beulich]

>>> On 11.04.14 at 13:57, <dslutz@verizon.com> wrote:

> On 04/11/14 03:45, Jan Beulich wrote:
>>>>> On 11.04.14 at 04:53, <dslutz@verizon.com> wrote:
>>> This is because "make test" gives me:
>> I don't think you need to be worried about this failing - just make sure
>> "make -C .../tools/tests/hpet" works, and ideally (again just like the
>> x86 emulator one) also have a "run" target in the Makefile.
> 
> I see that that test is not passing:
> 
> make -C tools/tests/x86_emulator run
> ...
> Testing daa/das (all inputs)...         skipped
> Testing movq %mm3,(%ecx)...             okay
> Testing movq (%edx),%mm5...             okay
> Testing movdqu %xmm2,(%ecx)...          okay
> Testing movdqu (%edx),%xmm4...          okay
> Testing vmovdqu %ymm2,(%ecx)...         failed!

Odd - it worked the last time I tried. Was that perhaps on an AVX-
incapable system?

Jan

Re: [PATCH optional v2 01/10] hvm/hpet: Add manual unit test code.

[Don Slutz]

On 04/11/14 08:09, Jan Beulich wrote:
>>>> On 11.04.14 at 13:57, <dslutz@verizon.com> wrote:
>>
>> I see that that test is not passing:
>>
>> make -C tools/tests/x86_emulator run
>> ...
>> Testing daa/das (all inputs)...         skipped
>> Testing movq %mm3,(%ecx)...             okay
>> Testing movq (%edx),%mm5...             okay
>> Testing movdqu %xmm2,(%ecx)...          okay
>> Testing movdqu (%edx),%xmm4...          okay
>> Testing vmovdqu %ymm2,(%ecx)...         failed!
> Odd - it worked the last time I tried. Was that perhaps on an AVX-
> incapable system?

Is avx from /proc/cpuinfo:


dcs-xen-54:~/xen>lscpu
Architecture:          x86_64
CPU op-mode(s):        32-bit, 64-bit
Byte Order:            Little Endian
CPU(s):                8
On-line CPU(s) list:   0-7
Thread(s) per core:    2
Core(s) per socket:    4
Socket(s):             1
NUMA node(s):          1
Vendor ID:             GenuineIntel
CPU family:            6
Model:                 42
Stepping:              7
CPU MHz:               2400.080
BogoMIPS:              4800.16
Hypervisor vendor:     Xen
Virtualization type:   none
L1d cache:             32K
L1i cache:             32K
L2 cache:              256K
L3 cache:              8192K
NUMA node0 CPU(s):     0-7
dcs-xen-54:~/xen>cat /proc/cpuinfo
processor       : 0
vendor_id       : GenuineIntel
cpu family      : 6
model           : 42
model name      : Intel(R) Xeon(R) CPU E31265L @ 2.40GHz
stepping        : 7
microcode       : 0x17
cpu MHz         : 2400.080
cache size      : 8192 KB
physical id     : 0
siblings        : 8
core id         : 0
cpu cores       : 4
apicid          : 0
initial apicid  : 0
fpu             : yes
fpu_exception   : yes
cpuid level     : 13
wp              : yes
flags           : fpu de tsc msr pae mce cx8 apic sep mca cmov pat 
clflush acpi mmx fxsr sse sse2 ss ht syscall nx lm constant_tsc rep_good 
nopl nonstop_tsc eagerfpu pni pclmulqdq monitor est ssse3 cx16 sse4_1 
sse4_2 x2apic popcnt tsc_deadline_timer aes xsave avx hypervisor lahf_lm 
ida arat epb xsaveopt pln pts dtherm
bogomips        : 4800.16
clflush size    : 64
cache_alignment : 64
address sizes   : 36 bits physical, 48 bits virtual
power management:

processor       : 1
vendor_id       : GenuineIntel
cpu family      : 6
model           : 42
model name      : Intel(R) Xeon(R) CPU E31265L @ 2.40GHz
stepping        : 7
microcode       : 0x17
cpu MHz         : 2400.080
cache size      : 8192 KB
physical id     : 0
siblings        : 8
core id         : 0
cpu cores       : 4
apicid          : 1
initial apicid  : 1
fpu             : yes
fpu_exception   : yes
cpuid level     : 13
wp              : yes
flags           : fpu de tsc msr pae mce cx8 apic sep mca cmov pat 
clflush acpi mmx fxsr sse sse2 ss ht syscall nx lm constant_tsc rep_good 
nopl nonstop_tsc eagerfpu pni pclmulqdq monitor est ssse3 cx16 sse4_1 
sse4_2 x2apic popcnt tsc_deadline_timer aes xsave avx hypervisor lahf_lm 
ida arat epb xsaveopt pln pts dtherm
bogomips        : 4800.16
clflush size    : 64
cache_alignment : 64
address sizes   : 36 bits physical, 48 bits virtual
power management:

processor       : 2
vendor_id       : GenuineIntel
cpu family      : 6
model           : 42
model name      : Intel(R) Xeon(R) CPU E31265L @ 2.40GHz
stepping        : 7
microcode       : 0x17
cpu MHz         : 2400.080
cache size      : 8192 KB
physical id     : 0
siblings        : 8
core id         : 1
cpu cores       : 4
apicid          : 2
initial apicid  : 2
fpu             : yes
fpu_exception   : yes
cpuid level     : 13
wp              : yes
flags           : fpu de tsc msr pae mce cx8 apic sep mca cmov pat 
clflush acpi mmx fxsr sse sse2 ss ht syscall nx lm constant_tsc rep_good 
nopl nonstop_tsc eagerfpu pni pclmulqdq monitor est ssse3 cx16 sse4_1 
sse4_2 x2apic popcnt tsc_deadline_timer aes xsave avx hypervisor lahf_lm 
ida arat epb xsaveopt pln pts dtherm
bogomips        : 4800.16
clflush size    : 64
cache_alignment : 64
address sizes   : 36 bits physical, 48 bits virtual
power management:

processor       : 3
vendor_id       : GenuineIntel
cpu family      : 6
model           : 42
model name      : Intel(R) Xeon(R) CPU E31265L @ 2.40GHz
stepping        : 7
microcode       : 0x17
cpu MHz         : 2400.080
cache size      : 8192 KB
physical id     : 0
siblings        : 8
core id         : 1
cpu cores       : 4
apicid          : 3
initial apicid  : 3
fpu             : yes
fpu_exception   : yes
cpuid level     : 13
wp              : yes
flags           : fpu de tsc msr pae mce cx8 apic sep mca cmov pat 
clflush acpi mmx fxsr sse sse2 ss ht syscall nx lm constant_tsc rep_good 
nopl nonstop_tsc eagerfpu pni pclmulqdq monitor est ssse3 cx16 sse4_1 
sse4_2 x2apic popcnt tsc_deadline_timer aes xsave avx hypervisor lahf_lm 
ida arat epb xsaveopt pln pts dtherm
bogomips        : 4800.16
clflush size    : 64
cache_alignment : 64
address sizes   : 36 bits physical, 48 bits virtual
power management:

processor       : 4
vendor_id       : GenuineIntel
cpu family      : 6
model           : 42
model name      : Intel(R) Xeon(R) CPU E31265L @ 2.40GHz
stepping        : 7
microcode       : 0x17
cpu MHz         : 2400.080
cache size      : 8192 KB
physical id     : 0
siblings        : 8
core id         : 2
cpu cores       : 4
apicid          : 4
initial apicid  : 4
fpu             : yes
fpu_exception   : yes
cpuid level     : 13
wp              : yes
flags           : fpu de tsc msr pae mce cx8 apic sep mca cmov pat 
clflush acpi mmx fxsr sse sse2 ss ht syscall nx lm constant_tsc rep_good 
nopl nonstop_tsc eagerfpu pni pclmulqdq monitor est ssse3 cx16 sse4_1 
sse4_2 x2apic popcnt tsc_deadline_timer aes xsave avx hypervisor lahf_lm 
ida arat epb xsaveopt pln pts dtherm
bogomips        : 4800.16
clflush size    : 64
cache_alignment : 64
address sizes   : 36 bits physical, 48 bits virtual
power management:

processor       : 5
vendor_id       : GenuineIntel
cpu family      : 6
model           : 42
model name      : Intel(R) Xeon(R) CPU E31265L @ 2.40GHz
stepping        : 7
microcode       : 0x17
cpu MHz         : 2400.080
cache size      : 8192 KB
physical id     : 0
siblings        : 8
core id         : 2
cpu cores       : 4
apicid          : 5
initial apicid  : 5
fpu             : yes
fpu_exception   : yes
cpuid level     : 13
wp              : yes
flags           : fpu de tsc msr pae mce cx8 apic sep mca cmov pat 
clflush acpi mmx fxsr sse sse2 ss ht syscall nx lm constant_tsc rep_good 
nopl nonstop_tsc eagerfpu pni pclmulqdq monitor est ssse3 cx16 sse4_1 
sse4_2 x2apic popcnt tsc_deadline_timer aes xsave avx hypervisor lahf_lm 
ida arat epb xsaveopt pln pts dtherm
bogomips        : 4800.16
clflush size    : 64
cache_alignment : 64
address sizes   : 36 bits physical, 48 bits virtual
power management:

processor       : 6
vendor_id       : GenuineIntel
cpu family      : 6
model           : 42
model name      : Intel(R) Xeon(R) CPU E31265L @ 2.40GHz
stepping        : 7
microcode       : 0x17
cpu MHz         : 2400.080
cache size      : 8192 KB
physical id     : 0
siblings        : 8
core id         : 3
cpu cores       : 4
apicid          : 6
initial apicid  : 6
fpu             : yes
fpu_exception   : yes
cpuid level     : 13
wp              : yes
flags           : fpu de tsc msr pae mce cx8 apic sep mca cmov pat 
clflush acpi mmx fxsr sse sse2 ss ht syscall nx lm constant_tsc rep_good 
nopl nonstop_tsc eagerfpu pni pclmulqdq monitor est ssse3 cx16 sse4_1 
sse4_2 x2apic popcnt tsc_deadline_timer aes xsave avx hypervisor lahf_lm 
ida arat epb xsaveopt pln pts dtherm
bogomips        : 4800.16
clflush size    : 64
cache_alignment : 64
address sizes   : 36 bits physical, 48 bits virtual
power management:

processor       : 7
vendor_id       : GenuineIntel
cpu family      : 6
model           : 42
model name      : Intel(R) Xeon(R) CPU E31265L @ 2.40GHz
stepping        : 7
microcode       : 0x17
cpu MHz         : 2400.080
cache size      : 8192 KB
physical id     : 0
siblings        : 8
core id         : 3
cpu cores       : 4
apicid          : 7
initial apicid  : 7
fpu             : yes
fpu_exception   : yes
cpuid level     : 13
wp              : yes
flags           : fpu de tsc msr pae mce cx8 apic sep mca cmov pat 
clflush acpi mmx fxsr sse sse2 ss ht syscall nx lm constant_tsc rep_good 
nopl nonstop_tsc eagerfpu pni pclmulqdq monitor est ssse3 cx16 sse4_1 
sse4_2 x2apic popcnt tsc_deadline_timer aes xsave avx hypervisor lahf_lm 
ida arat epb xsaveopt pln pts dtherm
bogomips        : 4800.16
clflush size    : 64
cache_alignment : 64
address sizes   : 36 bits physical, 48 bits virtual
power management:


     -Don Slutz


> Jan
>

Re: [PATCH optional v2 01/10] hvm/hpet: Add manual unit test code.

[Jan Beulich]

>>> On 11.04.14 at 14:48, <dslutz@verizon.com> wrote:

> On 04/11/14 08:09, Jan Beulich wrote:
>>>>> On 11.04.14 at 13:57, <dslutz@verizon.com> wrote:
>>>
>>> I see that that test is not passing:
>>>
>>> make -C tools/tests/x86_emulator run
>>> ...
>>> Testing daa/das (all inputs)...         skipped
>>> Testing movq %mm3,(%ecx)...             okay
>>> Testing movq (%edx),%mm5...             okay
>>> Testing movdqu %xmm2,(%ecx)...          okay
>>> Testing movdqu (%edx),%xmm4...          okay
>>> Testing vmovdqu %ymm2,(%ecx)...         failed!
>> Odd - it worked the last time I tried. Was that perhaps on an AVX-
>> incapable system?
> 
> Is avx from /proc/cpuinfo:

Interesting - I just tried it again, and it works for me. Could of course
be an issue with your glibc clobbering %ymm2 - I always wondered for
how long we would get away with this not really correct register usage.
But otoh I would have expected the earlier test using %xmm2 to fail
then too...

Jan

Re: [PATCH optional v2 01/10] hvm/hpet: Add manual unit test code.

[Don Slutz]

On 04/11/14 11:14, Jan Beulich wrote:
>>>> On 11.04.14 at 14:48, <dslutz@verizon.com> wrote:
>> On 04/11/14 08:09, Jan Beulich wrote:
>>>>>> On 11.04.14 at 13:57, <dslutz@verizon.com> wrote:
>>>> I see that that test is not passing:
>>>>
>>>> make -C tools/tests/x86_emulator run
>>>> ...
>>>> Testing daa/das (all inputs)...         skipped
>>>> Testing movq %mm3,(%ecx)...             okay
>>>> Testing movq (%edx),%mm5...             okay
>>>> Testing movdqu %xmm2,(%ecx)...          okay
>>>> Testing movdqu (%edx),%xmm4...          okay
>>>> Testing vmovdqu %ymm2,(%ecx)...         failed!
>>> Odd - it worked the last time I tried. Was that perhaps on an AVX-
>>> incapable system?
>> Is avx from /proc/cpuinfo:
> Interesting - I just tried it again, and it works for me. Could of course
> be an issue with your glibc clobbering %ymm2 - I always wondered for
> how long we would get away with this not really correct register usage.
> But otoh I would have expected the earlier test using %xmm2 to fail
> then too...


Not sure what I can do to help.  A quick gdb says:

Testing movzxwd (%%eax),%%ecx...        okay
Testing xadd %%ax,(%%ecx)...            okay
Testing dec %%ax...                     okay
Testing lea 8(%%ebp),%%eax...           okay
Testing daa/das (all inputs)...         skipped
Testing movq %mm3,(%ecx)...             okay
Testing movq (%edx),%mm5...             okay

Breakpoint 4, main (argc=<optimized out>, argv=<optimized out>) at 
test_x86_emulator.c:610
610             if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) )
(gdb) p rc
$3 = 0
(gdb) c
Continuing.
Testing movdqu %xmm2,(%ecx)...          okay
Testing movdqu (%edx),%xmm4...          okay

Breakpoint 3, main (argc=<optimized out>, argv=<optimized out>) at 
test_x86_emulator.c:661
661             if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 16, 64) )
(gdb) p rc
$4 = 7
(gdb) q


This is from 4.3.1...

     -Don Slutz

> Jan
>

Re: [PATCH optional v2 01/10] hvm/hpet: Add manual unit test code.

[Jan Beulich]

>>> On 11.04.14 at 19:40, <dslutz@verizon.com> wrote:
> Testing movdqu %xmm2,(%ecx)...          okay
> Testing movdqu (%edx),%xmm4...          okay
> 
> Breakpoint 3, main (argc=<optimized out>, argv=<optimized out>) at 
> test_x86_emulator.c:661
> 661             if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 16, 64) )
> (gdb) p rc
> $4 = 7
> (gdb) q

I don't see how a return code of 7 could ever be observed here. But
It's then apparently not the suspected XMM register clobbering.

> This is from 4.3.1...

I.e. not really meaningful if not proven to behave the same on
-unstable. And anyway, if you're not up to debugging this, I don't
think there's much we can do.

Jan

test_x86_emulator (was Re: [PATCH optional v2 01/10] hvm/hpet: Add manual unit test code.)

[Don Slutz]

On 04/14/14 03:40, Jan Beulich wrote:
>>>> On 11.04.14 at 19:40, <dslutz@verizon.com> wrote:
>> Testing movdqu %xmm2,(%ecx)...          okay
>> Testing movdqu (%edx),%xmm4...          okay
>>
>> Breakpoint 3, main (argc=<optimized out>, argv=<optimized out>) at
>> test_x86_emulator.c:661
>> 661             if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 16, 64) )
>> (gdb) p rc
>> $4 = 7
>> (gdb) q
> I don't see how a return code of 7 could ever be observed here. But
> It's then apparently not the suspected XMM register clobbering.

On ac0f56a + All these patches (which should not effect this)

It is more complex then it appears at 1st look.

-O1:
...
Testing movdqu (%edx),%xmm4...          okay
Testing vmovdqu %ymm2,(%ecx)...         skipped
Testing vmovdqu (%edx),%ymm4...         skipped
Testing movsd %xmm5,(%ecx)...           okay
Testing movaps (%edx),%xmm7...          okay
...


-O2:
...
Testing movdqu (%edx),%xmm4...          okay
Testing vmovdqu %ymm2,(%ecx)...         failed!
make: *** [run] Error 1


How I got here:

...
Testing movq (%edx),%mm5...             okay
Testing movdqu %xmm2,(%ecx)...          okay
Testing movdqu (%edx),%xmm4...          okay

Breakpoint 1, main (argc=<optimized out>, argv=<optimized out>) at test_x86_emulator.c:702
702         printf("%-40s", "Testing vmovdqu %ymm2,(%ecx)...");
(gdb) c
Continuing.

Breakpoint 3, x86_emulate (ctxt=ctxt@entry=0x7fffffffdb80, ops=ops@entry=0x6152a0 <emulops>)
     at x86_emulate/x86_emulate.c:4164
4164                get_fpu(X86EMUL_FPU_ymm, &fic);
(gdb) p rc
$2 = 0
(gdb) s
get_fpu (exception_callback=0x402a60 <fpu_handle_exception>, exception_callback_arg=0x7fffffffd7d0,
     type=X86EMUL_FPU_ymm, ctxt=0x7fffffffdb80) at test_x86_emulator.c:138
138         switch ( type )
(gdb) n
137     {
(gdb)
138         switch ( type )
(gdb)
154         return X86EMUL_OKAY;
(gdb)
138         switch ( type )
(gdb)
152             return X86EMUL_UNHANDLEABLE;
(gdb)
138         switch ( type )
(gdb)
132     int get_fpu(
(gdb)
155     }
(gdb)
x86_emulate (ctxt=ctxt@entry=0x7fffffffdb80, ops=ops@entry=0x6152a0 <emulops>) at x86_emulate/x86_emulate.c:4525
4525    }
(gdb)
main (argc=<optimized out>, argv=<optimized out>) at test_x86_emulator.c:719
719             if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 16, 64) )
(gdb)
934         printf("failed!\n");


And so I tried at the lower -O1, and "it works".




>> This is from 4.3.1...
> I.e. not really meaningful if not proven to behave the same on
> -unstable. And anyway, if you're not up to debugging this, I don't
> think there's much we can do.

Since I know almost nothing about both AVX instructions and x86_emulate, it is
hard for me to quickly help.

    -Don Slutz

> Jan
>

Re: test_x86_emulator

[Jan Beulich]

>>> On 14.04.14 at 19:29, <dslutz@verizon.com> wrote:
> It is more complex then it appears at 1st look.
> 
> -O1:
> ...
> Testing movdqu (%edx),%xmm4...          okay
> Testing vmovdqu %ymm2,(%ecx)...         skipped
> Testing vmovdqu (%edx),%ymm4...         skipped
> Testing movsd %xmm5,(%ecx)...           okay
> Testing movaps (%edx),%xmm7...          okay
> ...
> 
> -O2:
> ...
> Testing movdqu (%edx),%xmm4...          okay
> Testing vmovdqu %ymm2,(%ecx)...         failed!
> make: *** [run] Error 1

Now that's of course very interesting - looks more like a problem
with test_x86_emulator.c's use of CPUID then (because you said
you do this on an AVX-capable system, the -O1 test shouldn't skip
these).

> get_fpu (exception_callback=0x402a60 <fpu_handle_exception>, 
> exception_callback_arg=0x7fffffffd7d0,
>      type=X86EMUL_FPU_ymm, ctxt=0x7fffffffdb80) at test_x86_emulator.c:138
> 138         switch ( type )
> (gdb) n
> 137     {
> (gdb)
> 138         switch ( type )
> (gdb)
> 154         return X86EMUL_OKAY;
> (gdb)
> 138         switch ( type )
> (gdb)
> 152             return X86EMUL_UNHANDLEABLE;
> (gdb)
> 138         switch ( type )
> (gdb)
> 132     int get_fpu(
> (gdb)
> 155     }

Or a code generation problem: An input of X86EMUL_FPU_ymm should
obviously result in execution making to the respective case statement,
yet the steps above lead directly to the default one.

What gcc version are you using? And could you perhaps make both
binaries (-O1 and -O2) available for inspection?

Jan

Re: test_x86_emulator

[Don Slutz]

[-- Attachment #1: Type: text/plain, Size: 1757 bytes --]

On 04/15/14 02:49, Jan Beulich wrote:
>>>> On 14.04.14 at 19:29, <dslutz@verizon.com> wrote:
>> It is more complex then it appears at 1st look.
>>
>> -O1:
>> ...
>> Testing movdqu (%edx),%xmm4...          okay
>> Testing vmovdqu %ymm2,(%ecx)...         skipped
>> Testing vmovdqu (%edx),%ymm4...         skipped
>> Testing movsd %xmm5,(%ecx)...           okay
>> Testing movaps (%edx),%xmm7...          okay
>> ...
>>
>> -O2:
>> ...
>> Testing movdqu (%edx),%xmm4...          okay
>> Testing vmovdqu %ymm2,(%ecx)...         failed!
>> make: *** [run] Error 1
> Now that's of course very interesting - looks more like a problem
> with test_x86_emulator.c's use of CPUID then (because you said
> you do this on an AVX-capable system, the -O1 test shouldn't skip
> these).
>
>> get_fpu (exception_callback=0x402a60 <fpu_handle_exception>,
>> exception_callback_arg=0x7fffffffd7d0,
>>       type=X86EMUL_FPU_ymm, ctxt=0x7fffffffdb80) at test_x86_emulator.c:138
>> 138         switch ( type )
>> (gdb) n
>> 137     {
>> (gdb)
>> 138         switch ( type )
>> (gdb)
>> 154         return X86EMUL_OKAY;
>> (gdb)
>> 138         switch ( type )
>> (gdb)
>> 152             return X86EMUL_UNHANDLEABLE;
>> (gdb)
>> 138         switch ( type )
>> (gdb)
>> 132     int get_fpu(
>> (gdb)
>> 155     }
> Or a code generation problem: An input of X86EMUL_FPU_ymm should
> obviously result in execution making to the respective case statement,
> yet the steps above lead directly to the default one.
>
> What gcc version are you using? And could you perhaps make both
> binaries (-O1 and -O2) available for inspection?

dcs-xen-54:~/xen/tools/tests/x86_emulator>gcc --version
gcc (GCC) 4.7.2 20120921 (Red Hat 4.7.2-2)

Attached are the binaries.

    -Don Slutz
> Jan
>


[-- Attachment #2: x86_emulate.o.O2 --]
[-- Type: application/octet-stream, Size: 548584 bytes --]

[-- Attachment #3: x86_emulate.o.O1 --]
[-- Type: application/octet-stream, Size: 295952 bytes --]

[-- Attachment #4: test_x86_emulator.o.O1 --]
[-- Type: application/octet-stream, Size: 71008 bytes --]

[-- Attachment #5: test_x86_emulator.o.O2 --]
[-- Type: application/octet-stream, Size: 92944 bytes --]

[-- Attachment #6: blowfish.o.O2 --]
[-- Type: application/octet-stream, Size: 14344 bytes --]

[-- Attachment #7: blowfish.o.O1 --]
[-- Type: application/octet-stream, Size: 14344 bytes --]

[-- Attachment #8: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: test_x86_emulator

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1691 bytes --]

>>> On 15.04.14 at 16:24, <dslutz@verizon.com> wrote:
>> What gcc version are you using? And could you perhaps make both
>> binaries (-O1 and -O2) available for inspection?

So it's indeed a compiler version difference, with both us and them not
being fully correct. The patch below should make things work.

Jan

x86: fix instruction emulator test's xgetbv constraints

The "A" constraint, while documented up to gcc 4.5 as "The a and d
registers, as a pair (for instructions that return half the result in
one and half in the other)," never really behaved that (natural) way,
but always meant (and is now also documented so) %eax _or_ %edx (%rax
_or_ %rdx on x86-64) unless the operand was wide enough to require both
(i.e. more than 32 bits on ix86 and more than 64 bits on x86-64).

Interestingly something internal to the compiler changed between 4.4
and 4.5 to actually expose the difference - up to gcc 4.4 I was unable
to construct a case where, when only the low half of the result is
actually looked at, the result would be considered to be in %edx/%rdx
(and %eax/%rax would be treated as unmodified by the instruction).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/tools/tests/x86_emulator/test_x86_emulator.c
+++ b/tools/tests/x86_emulator/test_x86_emulator.c
@@ -102,11 +102,11 @@ static int cpuid(
 
 static inline uint64_t xgetbv(uint32_t xcr)
 {
-    uint64_t res;
+    uint32_t lo, hi;
 
-    asm ( ".byte 0x0f, 0x01, 0xd0" : "=A" (res) : "c" (xcr) );
+    asm ( ".byte 0x0f, 0x01, 0xd0" : "=a" (lo), "=d" (hi) : "c" (xcr) );
 
-    return res;
+    return ((uint64_t)hi << 32) | lo;
 }
 
 #define cpu_has_avx ({ \




[-- Attachment #2: x86-emul-test-xgetbv-constraint.patch --]
[-- Type: text/plain, Size: 1362 bytes --]

x86: fix instruction emulator test's xgetbv constraints

The "A" constraint, while documented up to gcc 4.5 as "The a and d
registers, as a pair (for instructions that return half the result in
one and half in the other)," never really behaved that (natural) way,
but always meant (and is now also documented so) %eax _or_ %edx (%rax
_or_ %rdx on x86-64) unless the operand was wide enough to require both
(i.e. more than 32 bits on ix86 and more than 64 bits on x86-64).

Interestingly something internal to the compiler changed between 4.4
and 4.5 to actually expose the difference - up to gcc 4.4 I was unable
to construct a case where, when only the low half of the result is
actually looked at, the result would be considered to be in %edx/%rdx
(and %eax/%rax would be treated as unmodified by the instruction).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/tools/tests/x86_emulator/test_x86_emulator.c
+++ b/tools/tests/x86_emulator/test_x86_emulator.c
@@ -102,11 +102,11 @@ static int cpuid(
 
 static inline uint64_t xgetbv(uint32_t xcr)
 {
-    uint64_t res;
+    uint32_t lo, hi;
 
-    asm ( ".byte 0x0f, 0x01, 0xd0" : "=A" (res) : "c" (xcr) );
+    asm ( ".byte 0x0f, 0x01, 0xd0" : "=a" (lo), "=d" (hi) : "c" (xcr) );
 
-    return res;
+    return ((uint64_t)hi << 32) | lo;
 }
 
 #define cpu_has_avx ({ \

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: test_x86_emulator

[Keir Fraser]

Jan Beulich wrote:
> x86: fix instruction emulator test's xgetbv constraints
>
> The "A" constraint, while documented up to gcc4.5  as "The a and d
> registers, as a pair (for instructions that return half the result in
> one and half in the other)," never really behaved that (natural) way,
> but always meant (and is now also documented so) %eax_or_  %edx (%rax
> _or_  %rdx on x86-64) unless the operand was wide enough to require both
> (i.e. more than 32 bits on ix86 and more than 64 bits on x86-64).
>
> Interestingly something internal to the compiler changed between4.4
> and4.5  to actually expose the difference - up to gcc4.4  I was unable
> to construct a case where, when only the low half of the result is
> actually looked at, the result would be considered to be in %edx/%rdx
> (and %eax/%rax would be treated as unmodified by the instruction).
>
> Signed-off-by: Jan Beulich<jbeulich@suse.com>

Acked-by: Keir Fraser <keir@xen.org>

Re: test_x86_emulator

[Don Slutz]

On 04/16/14 05:32, Keir Fraser wrote:
> Jan Beulich wrote:
>> x86: fix instruction emulator test's xgetbv constraints
>>
>> The "A" constraint, while documented up to gcc4.5  as "The a and d
>> registers, as a pair (for instructions that return half the result in
>> one and half in the other)," never really behaved that (natural) way,
>> but always meant (and is now also documented so) %eax_or_  %edx (%rax
>> _or_  %rdx on x86-64) unless the operand was wide enough to require both
>> (i.e. more than 32 bits on ix86 and more than 64 bits on x86-64).
>>
>> Interestingly something internal to the compiler changed between4.4
>> and4.5  to actually expose the difference - up to gcc4.4  I was unable
>> to construct a case where, when only the low half of the result is
>> actually looked at, the result would be considered to be in %edx/%rdx
>> (and %eax/%rax would be treated as unmodified by the instruction).
>>
>> Signed-off-by: Jan Beulich<jbeulich@suse.com>
>
> Acked-by: Keir Fraser <keir@xen.org>

And it works for me so:

Tested-by: Don Slutz <dslutz@verizon.com>

    -Don Slutz

[PATCH v2 02/10] hvm/hpet: Only call guest_time_hpet(h) one time per action.

[Don Slutz]

This call is expensive and will cause extra time to pass.

The software-developers-hpet-spec-1-0a.pdf does not say how long it
takes after the main clock is enabled before the first change of the
master clock.  Therefore multiple calls to guest_time_hpet(h) are
not needed.  Since each timer is started by a loop, each ones start
time will change on the multple calls.  In the real hardware, there
is not delta based on which timer.

Without this change it is possible for an HVM guest running linux to
get the message:

..MP-BIOS bug: 8254 timer not connected to IO-APIC

On the guest console(s); and the guest will panic.

Also Xen hypervisor console with be flooded with:

vioapic.c:352:d1 Unsupported delivery mode 7
vioapic.c:352:d1 Unsupported delivery mode 7
vioapic.c:352:d1 Unsupported delivery mode 7

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 xen/arch/x86/hvm/hpet.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 4324b52..16c0f07 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -73,17 +73,18 @@
     ((timer_config(h, n) & HPET_TN_INT_ROUTE_CAP_MASK) \
         >> HPET_TN_INT_ROUTE_CAP_SHIFT)
 
-static inline uint64_t hpet_read_maincounter(HPETState *h)
+static inline uint64_t hpet_read_maincounter(HPETState *h, uint64_t guest_time)
 {
     ASSERT(spin_is_locked(&h->lock));
 
     if ( hpet_enabled(h) )
-        return guest_time_hpet(h) + h->mc_offset;
+        return guest_time + h->mc_offset;
     else
         return h->hpet.mc64;
 }
 
-static uint64_t hpet_get_comparator(HPETState *h, unsigned int tn)
+static uint64_t hpet_get_comparator(HPETState *h, unsigned int tn,
+                                    uint64_t guest_time)
 {
     uint64_t comparator;
     uint64_t elapsed;
@@ -95,7 +96,8 @@ static uint64_t hpet_get_comparator(HPETState *h, unsigned int tn)
         uint64_t period = h->hpet.period[tn];
         if (period)
         {
-            elapsed = hpet_read_maincounter(h) + period - 1 - comparator;
+            elapsed = hpet_read_maincounter(h, guest_time) +
+                period - 1 - comparator;
             comparator += (elapsed / period) * period;
             h->hpet.comparator64[tn] = comparator;
         }
@@ -120,7 +122,7 @@ static inline uint64_t hpet_read64(HPETState *h, unsigned long addr)
     case HPET_STATUS:
         return h->hpet.isr;
     case HPET_COUNTER:
-        return hpet_read_maincounter(h);
+        return hpet_read_maincounter(h, guest_time_hpet(h));
     case HPET_Tn_CFG(0):
     case HPET_Tn_CFG(1):
     case HPET_Tn_CFG(2):
@@ -128,7 +130,7 @@ static inline uint64_t hpet_read64(HPETState *h, unsigned long addr)
     case HPET_Tn_CMP(0):
     case HPET_Tn_CMP(1):
     case HPET_Tn_CMP(2):
-        return hpet_get_comparator(h, HPET_TN(CMP, addr));
+        return hpet_get_comparator(h, HPET_TN(CMP, addr), guest_time_hpet(h));
     case HPET_Tn_ROUTE(0):
     case HPET_Tn_ROUTE(1):
     case HPET_Tn_ROUTE(2):
@@ -194,18 +196,19 @@ static void hpet_stop_timer(HPETState *h, unsigned int tn)
     destroy_periodic_time(&h->pt[tn]);
     /* read the comparator to get it updated so a read while stopped will
      * return the expected value. */
-    hpet_get_comparator(h, tn);
+    hpet_get_comparator(h, tn, guest_time_hpet(h));
 }
 
 /* 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)
 
-static void hpet_set_timer(HPETState *h, unsigned int tn)
+static void hpet_set_timer(HPETState *h, unsigned int tn, int mc_starting)
 {
     uint64_t tn_cmp, cur_tick, diff;
     unsigned int irq;
     unsigned int oneshot;
+    uint64_t guest_time;
 
     ASSERT(tn < HPET_TIMER_NUM);
     ASSERT(spin_is_locked(&h->lock));
@@ -220,8 +223,13 @@ static void hpet_set_timer(HPETState *h, unsigned int tn)
     if ( !timer_enabled(h, tn) )
         return;
 
-    tn_cmp   = hpet_get_comparator(h, tn);
-    cur_tick = hpet_read_maincounter(h);
+    if ( mc_starting )
+        guest_time = h->hpet.mc64 - h->mc_offset;
+    else
+        guest_time = guest_time_hpet(h);
+
+    tn_cmp   = hpet_get_comparator(h, tn, guest_time);
+    cur_tick = hpet_read_maincounter(h, guest_time);
     if ( timer_is_32bit(h, tn) )
     {
         tn_cmp   = (uint32_t)tn_cmp;
@@ -283,6 +291,7 @@ static int hpet_write(
 #define set_stop_timer(n)    (__set_bit((n), &stop_timers))
 #define set_start_timer(n)   (__set_bit((n), &start_timers))
 #define set_restart_timer(n) (set_stop_timer(n),set_start_timer(n))
+    int mc_starting = 0;
 
     addr &= HPET_MMAP_SIZE-1;
 
@@ -307,6 +316,7 @@ static int hpet_write(
         {
             /* Enable main counter and interrupt generation. */
             h->mc_offset = h->hpet.mc64 - guest_time_hpet(h);
+            mc_starting = 1;
             for ( i = 0; i < HPET_TIMER_NUM; i++ )
             {
                 h->hpet.comparator64[i] =
@@ -441,7 +451,7 @@ static int hpet_write(
     {
         i = find_first_set_bit(start_timers);
         __clear_bit(i, &start_timers);
-        hpet_set_timer(h, i);
+        hpet_set_timer(h, i, mc_starting);
     }
 
 #undef set_stop_timer
@@ -562,7 +572,7 @@ static int hpet_load(struct domain *d, hvm_domain_context_t *h)
     if ( hpet_enabled(hp) )
         for ( i = 0; i < HPET_TIMER_NUM; i++ )
             if ( timer_enabled(hp, i) )
-                hpet_set_timer(hp, i);
+                hpet_set_timer(hp, i, 1);
 
     spin_unlock(&hp->lock);
 
-- 
1.8.4

Re: [PATCH v2 02/10] hvm/hpet: Only call guest_time_hpet(h) one time per action.

[Jan Beulich]

>>> On 08.04.14 at 16:24, <dslutz@verizon.com> wrote:

This looks good to me, just a minor comment:

> -static void hpet_set_timer(HPETState *h, unsigned int tn)
> +static void hpet_set_timer(HPETState *h, unsigned int tn, int mc_starting)

This as well as ...

> @@ -283,6 +291,7 @@ static int hpet_write(
>  #define set_stop_timer(n)    (__set_bit((n), &stop_timers))
>  #define set_start_timer(n)   (__set_bit((n), &start_timers))
>  #define set_restart_timer(n) (set_stop_timer(n),set_start_timer(n))
> +    int mc_starting = 0;

... this wants to be bool_t. Or alternatively you might want to see
whether just like in the other case you could pass guest_time in.

With that adjusted,
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v2 02/10] hvm/hpet: Only call guest_time_hpet(h) one time per action.

[Don Slutz]

On 04/14/14 10:31, Jan Beulich wrote:
>>>> On 08.04.14 at 16:24, <dslutz@verizon.com> wrote:
> This looks good to me, just a minor comment:
>
>> -static void hpet_set_timer(HPETState *h, unsigned int tn)
>> +static void hpet_set_timer(HPETState *h, unsigned int tn, int mc_starting)
> This as well as ...
>
>> @@ -283,6 +291,7 @@ static int hpet_write(
>>   #define set_stop_timer(n)    (__set_bit((n), &stop_timers))
>>   #define set_start_timer(n)   (__set_bit((n), &start_timers))
>>   #define set_restart_timer(n) (set_stop_timer(n),set_start_timer(n))
>> +    int mc_starting = 0;
> ... this wants to be bool_t. Or alternatively you might want to see
> whether just like in the other case you could pass guest_time in.

Will change to bool_t.

> With that adjusted,
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

     -Don Slutz

> Jan
>

[PATCH v2 03/10] hvm/hpet: Only set comparator or period not both.

[Don Slutz]

The current code sets both.  If setting the comparator also set
comparator64 (the hidden version).

Based on:

software-developers-hpet-spec-1-0a.pdf

A write call should only change comparator or period, not both.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 xen/arch/x86/hvm/hpet.c | 58 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 16c0f07..0795f29 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -399,30 +399,48 @@ static int hpet_write(
         tn = HPET_TN(CMP, addr);
         if ( timer_is_32bit(h, tn) )
             new_val = (uint32_t)new_val;
-        h->hpet.timers[tn].cmp = new_val;
-        if ( h->hpet.timers[tn].config & HPET_TN_SETVAL )
-            /*
-             * When SETVAL is one, software is able to "directly set a periodic
-             * timer's accumulator."  That is, set the comparator without
-             * adjusting the period.  Much the same as just setting the
-             * comparator on an enabled one-shot timer.
-             *
-             * This configuration bit clears when the comparator is written.
-             */
-            h->hpet.timers[tn].config &= ~HPET_TN_SETVAL;
-        else if ( timer_is_periodic(h, tn) )
+        if ( timer_is_periodic(h, tn) )
+        {
+            if ( h->hpet.timers[tn].config & HPET_TN_SETVAL )
+            {
+                /*
+                 * When SETVAL is one, software is able to "directly
+                 * set a periodic timer's accumulator."  That is,
+                 * set the comparator without adjusting the period.
+                 * Much the same as just setting the comparator on
+                 * an enabled one-shot timer.
+                 *
+                 * This configuration bit clears when the comparator
+                 * is written.
+                 */
+                h->hpet.timers[tn].config &= ~HPET_TN_SETVAL;
+                h->hpet.timers[tn].cmp = new_val;
+                h->hpet.comparator64[tn] = new_val;
+            }
+            else
+            {
+                /*
+                 * Clamp period to reasonable min/max values:
+                 *  - minimum is 100us, same as timers controlled by vpt.c
+                 *  - maximum is to prevent overflow in time_after()
+                 *    calculations
+                 */
+                if ( hpet_tick_to_ns(h, new_val) < MICROSECS(100) )
+                    new_val = (MICROSECS(100) << 10) / h->hpet_to_ns_scale;
+                new_val &= (timer_is_32bit(h, tn) ? ~0u : ~0ull) >> 1;
+                h->hpet.period[tn] = new_val;
+            }
+        }
+        else
         {
             /*
-             * Clamp period to reasonable min/max values:
-             *  - minimum is 100us, same as timers controlled by vpt.c
-             *  - maximum is to prevent overflow in time_after() calculations
+             * The spec says "do not use", but clear if specified.
              */
-            if ( hpet_tick_to_ns(h, new_val) < MICROSECS(100) )
-                new_val = (MICROSECS(100) << 10) / h->hpet_to_ns_scale;
-            new_val &= (timer_is_32bit(h, tn) ? ~0u : ~0ull) >> 1;
-            h->hpet.period[tn] = new_val;
+            if ( h->hpet.timers[tn].config & HPET_TN_SETVAL )
+                h->hpet.timers[tn].config &= ~HPET_TN_SETVAL;
+            h->hpet.timers[tn].cmp = new_val;
+            h->hpet.comparator64[tn] = new_val;
         }
-        h->hpet.comparator64[tn] = new_val;
         if ( hpet_enabled(h) && timer_enabled(h, tn) )
             set_restart_timer(tn);
         break;
-- 
1.8.4

Re: [PATCH v2 03/10] hvm/hpet: Only set comparator or period not both.

[Jan Beulich]

>>> On 08.04.14 at 16:24, <dslutz@verizon.com> wrote:
> +        if ( timer_is_periodic(h, tn) )
> +        {
> +            if ( h->hpet.timers[tn].config & HPET_TN_SETVAL )
> +            {
> +                /*
> +                 * When SETVAL is one, software is able to "directly
> +                 * set a periodic timer's accumulator."  That is,
> +                 * set the comparator without adjusting the period.
> +                 * Much the same as just setting the comparator on
> +                 * an enabled one-shot timer.
> +                 *
> +                 * This configuration bit clears when the comparator
> +                 * is written.
> +                 */
> +                h->hpet.timers[tn].config &= ~HPET_TN_SETVAL;
> +                h->hpet.timers[tn].cmp = new_val;
> +                h->hpet.comparator64[tn] = new_val;
> +            }
> +            else
> +            {
> +                /*
> +                 * Clamp period to reasonable min/max values:
> +                 *  - minimum is 100us, same as timers controlled by vpt.c
> +                 *  - maximum is to prevent overflow in time_after()
> +                 *    calculations
> +                 */
> +                if ( hpet_tick_to_ns(h, new_val) < MICROSECS(100) )
> +                    new_val = (MICROSECS(100) << 10) / h->hpet_to_ns_scale;
> +                new_val &= (timer_is_32bit(h, tn) ? ~0u : ~0ull) >> 1;
> +                h->hpet.period[tn] = new_val;
> +            }
> +        }
> +        else
>          {
>              /*
> -             * Clamp period to reasonable min/max values:
> -             *  - minimum is 100us, same as timers controlled by vpt.c
> -             *  - maximum is to prevent overflow in time_after() calculations
> +             * The spec says "do not use", but clear if specified.
>               */
> -            if ( hpet_tick_to_ns(h, new_val) < MICROSECS(100) )
> -                new_val = (MICROSECS(100) << 10) / h->hpet_to_ns_scale;
> -            new_val &= (timer_is_32bit(h, tn) ? ~0u : ~0ull) >> 1;
> -            h->hpet.period[tn] = new_val;
> +            if ( h->hpet.timers[tn].config & HPET_TN_SETVAL )
> +                h->hpet.timers[tn].config &= ~HPET_TN_SETVAL;

The if() is clearly unnecessary here, and with it removed I don't see
any difference left with the inner if() path above. Hence I guess
they should be folded. Once that's done, new_val as calculated
before the outer if() isn't needed in the inner "else" path, and
hence its truncation could be moved inside the if(). Which in turn
allows you to fix the comparator64 related bug here too: All other
code stores the non-truncated value there, just the code here
doesn't.

Jan

Re: [PATCH v2 03/10] hvm/hpet: Only set comparator or period not both.

[Don Slutz]

On 04/14/14 10:58, Jan Beulich wrote:
>>>> On 08.04.14 at 16:24, <dslutz@verizon.com> wrote:

> The if() is clearly unnecessary here, and with it removed I don't see
> any difference left with the inner if() path above. Hence I guess
> they should be folded. Once that's done, new_val as calculated
> before the outer if() isn't needed in the inner "else" path, and
> hence its truncation could be moved inside the if(). Which in turn
> allows you to fix the comparator64 related bug here too: All other
> code stores the non-truncated value there, just the code here
> doesn't.
>
> Jan

If I understand this correctly, this leads to the following change
(Hopefully not line wrapped):


diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index c3f286f..913beb1 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -404,20 +404,8 @@ static int hpet_write(
      case HPET_Tn_CMP(1):
      case HPET_Tn_CMP(2):
          tn = HPET_TN(CMP, addr);
-        if ( timer_is_32bit(h, tn) )
-            new_val = (uint32_t)new_val;
-        h->hpet.timers[tn].cmp = new_val;
-        if ( h->hpet.timers[tn].config & HPET_TN_SETVAL )
-            /*
-             * When SETVAL is one, software is able to "directly set a periodic
-             * timer's accumulator."  That is, set the comparator without
-             * adjusting the period.  Much the same as just setting the
-             * comparator on an enabled one-shot timer.
-             *
-             * This configuration bit clears when the comparator is written.
-             */
-            h->hpet.timers[tn].config &= ~HPET_TN_SETVAL;
-        else if ( timer_is_periodic(h, tn) )
+        if ( timer_is_periodic(h, tn) &&
+             !(h->hpet.timers[tn].config & HPET_TN_SETVAL) )
          {
              /*
               * Clamp period to reasonable min/max values:
@@ -429,7 +417,25 @@ static int hpet_write(
              new_val &= (timer_is_32bit(h, tn) ? ~0u : ~0ull) >> 1;
              h->hpet.period[tn] = new_val;
          }
-        h->hpet.comparator64[tn] = new_val;
+        else
+        {
+            /*
+             * When SETVAL is one, software is able to "directly set
+             * a periodic timer's accumulator."  That is, set the
+             * comparator without adjusting the period.  Much the
+             * same as just setting the comparator on an enabled
+             * one-shot timer.
+             *
+             * This configuration bit clears when the comparator is
+             * written.
+             */
+            h->hpet.timers[tn].config &= ~HPET_TN_SETVAL;
+            h->hpet.comparator64[tn] = new_val;
+            if ( timer_is_32bit(h, tn) )
+                h->hpet.timers[tn].cmp = (uint32_t)new_val;
+            else
+                h->hpet.timers[tn].cmp = new_val;
+        }
          if ( hpet_enabled(h) && timer_enabled(h, tn) )
              set_restart_timer(tn);
          break;


I am still testing that this re-write still does the "right
thing".  Which is why it is yet to be a v3.  This preview
is sent to check that I did correctly do the requested change.


    -Don Slutz

Re: [PATCH v2 03/10] hvm/hpet: Only set comparator or period not both.

[Jan Beulich]

>>> On 15.04.14 at 00:53, <dslutz@verizon.com> wrote:
> On 04/14/14 10:58, Jan Beulich wrote:
>>>>> On 08.04.14 at 16:24, <dslutz@verizon.com> wrote:
> 
>> The if() is clearly unnecessary here, and with it removed I don't see
>> any difference left with the inner if() path above. Hence I guess
>> they should be folded. Once that's done, new_val as calculated
>> before the outer if() isn't needed in the inner "else" path, and
>> hence its truncation could be moved inside the if(). Which in turn
>> allows you to fix the comparator64 related bug here too: All other
>> code stores the non-truncated value there, just the code here
>> doesn't.
> 
> If I understand this correctly, this leads to the following change
> (Hopefully not line wrapped):

Yes, with a minor comment.

> --- a/xen/arch/x86/hvm/hpet.c
> +++ b/xen/arch/x86/hvm/hpet.c
> @@ -404,20 +404,8 @@ static int hpet_write(
>       case HPET_Tn_CMP(1):
>       case HPET_Tn_CMP(2):
>           tn = HPET_TN(CMP, addr);
> -        if ( timer_is_32bit(h, tn) )
> -            new_val = (uint32_t)new_val;
> -        h->hpet.timers[tn].cmp = new_val;
> -        if ( h->hpet.timers[tn].config & HPET_TN_SETVAL )
> -            /*
> -             * When SETVAL is one, software is able to "directly set a periodic
> -             * timer's accumulator."  That is, set the comparator without
> -             * adjusting the period.  Much the same as just setting the
> -             * comparator on an enabled one-shot timer.
> -             *
> -             * This configuration bit clears when the comparator is written.
> -             */
> -            h->hpet.timers[tn].config &= ~HPET_TN_SETVAL;
> -        else if ( timer_is_periodic(h, tn) )
> +        if ( timer_is_periodic(h, tn) &&
> +             !(h->hpet.timers[tn].config & HPET_TN_SETVAL) )
>           {
>               /*
>                * Clamp period to reasonable min/max values:
> @@ -429,7 +417,25 @@ static int hpet_write(
>               new_val &= (timer_is_32bit(h, tn) ? ~0u : ~0ull) >> 1;
>               h->hpet.period[tn] = new_val;
>           }
> -        h->hpet.comparator64[tn] = new_val;
> +        else
> +        {
> +            /*
> +             * When SETVAL is one, software is able to "directly set
> +             * a periodic timer's accumulator."  That is, set the
> +             * comparator without adjusting the period.  Much the
> +             * same as just setting the comparator on an enabled
> +             * one-shot timer.
> +             *
> +             * This configuration bit clears when the comparator is
> +             * written.
> +             */
> +            h->hpet.timers[tn].config &= ~HPET_TN_SETVAL;
> +            h->hpet.comparator64[tn] = new_val;
> +            if ( timer_is_32bit(h, tn) )
> +                h->hpet.timers[tn].cmp = (uint32_t)new_val;
> +            else
> +                h->hpet.timers[tn].cmp = new_val;

I'd continue to do the truncation on new_val, and write just once
(as the old code did).

Jan

Re: [PATCH v2 03/10] hvm/hpet: Only set comparator or period not both.

[Don Slutz]

On 04/15/14 02:59, Jan Beulich wrote:
>>>> On 15.04.14 at 00:53, <dslutz@verizon.com> wrote:
> Yes, with a minor comment.

...

>> +            if ( timer_is_32bit(h, tn) )
>> +                h->hpet.timers[tn].cmp = (uint32_t)new_val;
>> +            else
>> +                h->hpet.timers[tn].cmp = new_val;
> I'd continue to do the truncation on new_val, and write just once
> (as the old code did).

Will do in v3.
     -Don Slutz

> Jan
>

[PATCH v2 04/10] hvm/hpet: In hpet_save, correctly compute mc64.

[Don Slutz]

When the master clock is not enabled, mc64 has the right value.

Basicly do the same thing as hpet_read_maincounter().

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 xen/arch/x86/hvm/hpet.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 0795f29..180fc10 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -504,7 +504,8 @@ static int hpet_save(struct domain *d, hvm_domain_context_t *h)
     spin_lock(&hp->lock);
 
     /* Write the proper value into the main counter */
-    hp->hpet.mc64 = hp->mc_offset + guest_time_hpet(hp);
+    if ( hpet_enabled(hp) )
+        hp->hpet.mc64 = hp->mc_offset + guest_time_hpet(hp);
 
     /* Save the HPET registers */
     rc = _hvm_init_entry(h, HVM_SAVE_CODE(HPET), 0, HVM_SAVE_LENGTH(HPET));
-- 
1.8.4

[PATCH v2 05/10] hvm/hpet: Init comparator64 like comparator.

[Don Slutz]

The software-developers-hpet-spec-1-0a.pdf says that the comparator
starts as all 1's.  Also make the hidden register comparator64 the same.

Since only the hidden register comparator64 is used by hpet_save, it
needs to start out with the right value.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 xen/arch/x86/hvm/hpet.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 180fc10..bcd2778 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -627,6 +627,7 @@ void hpet_init(struct vcpu *v)
         h->hpet.timers[i].config =
             HPET_TN_INT_ROUTE_CAP | HPET_TN_64BIT_CAP | HPET_TN_PERIODIC_CAP;
         h->hpet.timers[i].cmp = ~0ULL;
+        h->hpet.comparator64[i] = ~0ULL;
         h->pt[i].source = PTSRC_isa;
     }
 }
-- 
1.8.4

[PATCH v2 06/10] hvm/hpet: comparator can only change when master clock is enabled.

[Don Slutz]

This is based on software-developers-hpet-spec-1-0a.pdf saying:

When the main counter value matches the value in the timer's
comparator register, an interrupt can be generated.  The hardware
will then automatically increase the value in the compare register
by the last value written to that register.

And since interrupt can only be generated when the master clock
is enabled.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 xen/arch/x86/hvm/hpet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index bcd2778..c0529e7 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -90,7 +90,7 @@ static uint64_t hpet_get_comparator(HPETState *h, unsigned int tn,
     uint64_t elapsed;
 
     comparator = h->hpet.comparator64[tn];
-    if ( timer_is_periodic(h, tn) )
+    if ( hpet_enabled(h) && timer_is_periodic(h, tn) )
     {
         /* update comparator by number of periods elapsed since last update */
         uint64_t period = h->hpet.period[tn];
-- 
1.8.4

Re: [PATCH v2 06/10] hvm/hpet: comparator can only change when master clock is enabled.

[Jan Beulich]

>>> On 08.04.14 at 16:24, <dslutz@verizon.com> wrote:
> This is based on software-developers-hpet-spec-1-0a.pdf saying:
> 
> When the main counter value matches the value in the timer's
> comparator register, an interrupt can be generated.  The hardware
> will then automatically increase the value in the compare register
> by the last value written to that register.
> 
> And since interrupt can only be generated when the master clock
> is enabled.

This last sentence looks truncated.

As to the change - I'm not sure: The quoted description from the
specification cal also be read to mean that interrupt generation is
optional, but comparator increment will always happen. As long as
this can't be clarified, I'd prefer to stay with the code as is.

Jan

Re: [PATCH v2 06/10] hvm/hpet: comparator can only change when master clock is enabled.

[Don Slutz]

On 04/14/14 11:07, Jan Beulich wrote:
>>>> On 08.04.14 at 16:24, <dslutz@verizon.com> wrote:
>> This is based on software-developers-hpet-spec-1-0a.pdf saying:
>>
>> When the main counter value matches the value in the timer's
>> comparator register, an interrupt can be generated.  The hardware
>> will then automatically increase the value in the compare register
>> by the last value written to that register.
>>
>> And since interrupt can only be generated when the master clock
>> is enabled.
> This last sentence looks truncated.

It is. I think I had:

And since interrupt can only be generated when the master clock
is enabled, the compare register should also change only then.

Which looks too complex to me now.

 From the spec:


ENABLE_CNF

Overall Enable:

This bit must be set to enable any of the timers to generate interrupts. If
this bit is 0, then the main counter will halt (will not increment) and no interrupts will be
caused by any of these timers.

• 0 – Halt main count and disable all timer interrupts
• 1 – allow main counter to run, and allow timer interrupts if enabled




I think a better statement would be:

When the overall enable is off (the main count is halted), none of
the compare registers should change.


So I will change this in the commit message.

> As to the change - I'm not sure: The quoted description from the
> specification cal also be read to mean that interrupt generation is
> optional, but comparator increment will always happen. As long as
> this can't be clarified, I'd prefer to stay with the code as is.

I think the code needs to change to match the spec.

#define timer_enabled(h, n) (timer_config(h, n) & HPET_TN_ENABLE)

vs

#define hpet_enabled(h) (h->hpet.config & HPET_CFG_ENABLE)


The change uses hpet_enabled() (I.E. Overall Enable).

-Don Slutz

>
> Jan
>

Re: [PATCH v2 06/10] hvm/hpet: comparator can only change when master clock is enabled.

[Jan Beulich]

>>> On 14.04.14 at 21:50, <dslutz@verizon.com> wrote:
> On 04/14/14 11:07, Jan Beulich wrote:
>> As to the change - I'm not sure: The quoted description from the
>> specification cal also be read to mean that interrupt generation is
>> optional, but comparator increment will always happen. As long as
>> this can't be clarified, I'd prefer to stay with the code as is.
> 
> I think the code needs to change to match the spec.
> 
> #define timer_enabled(h, n) (timer_config(h, n) & HPET_TN_ENABLE)
> 
> vs
> 
> #define hpet_enabled(h) (h->hpet.config & HPET_CFG_ENABLE)
> 
> 
> The change uses hpet_enabled() (I.E. Overall Enable).

Correct. But do we really need this? When the HPET is globally
disabled, hpet_read_maincounter() returns a fixed value, and
hence - due to the comparators not changing either -
hpet_get_comparator() will too even without the addition. So if
at all, the change would be mostly for documentation purposes.

Jan

Re: [PATCH v2 06/10] hvm/hpet: comparator can only change when master clock is enabled.

[Don Slutz]

On 04/15/14 03:05, Jan Beulich wrote:
>>>> On 14.04.14 at 21:50, <dslutz@verizon.com> wrote:
>> On 04/14/14 11:07, Jan Beulich wrote:
>>> As to the change - I'm not sure: The quoted description from the
>>> specification cal also be read to mean that interrupt generation is
>>> optional, but comparator increment will always happen. As long as
>>> this can't be clarified, I'd prefer to stay with the code as is.
>> I think the code needs to change to match the spec.
>>
>> #define timer_enabled(h, n) (timer_config(h, n) & HPET_TN_ENABLE)
>>
>> vs
>>
>> #define hpet_enabled(h) (h->hpet.config & HPET_CFG_ENABLE)
>>
>>
>> The change uses hpet_enabled() (I.E. Overall Enable).
> Correct. But do we really need this? When the HPET is globally
> disabled, hpet_read_maincounter() returns a fixed value, and
> hence - due to the comparators not changing either -
> hpet_get_comparator() will too even without the addition. So if
> at all, the change would be mostly for documentation purposes.

We do need this.  hpet_get_comparator() does not return a
fixed value.  Without this change it will always adjust to
hpet_read_maincounter().  Here is an example using the
manual test code:


dcs-xen-54:~/xen/tools/tests/vhpet>gdb --args ./test_vhpet 0 3 0 1 0x13
GNU gdb (GDB) 7.6.2.20140108-cvs
Copyright (C) 2013 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-unknown-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /home/don/xen/tools/tests/vhpet/test_vhpet...done.
(gdb) b hpet.c:128
Breakpoint 1 at 0x400ac1: file hpet.c, line 128.
(gdb) r
Starting program: /home/don/xen/tools/tests/vhpet/./test_vhpet 0 3 0 1 0x13
warning: no loadable sections found in added symbol-file system-supplied DSO at 0x7ffff7ffd000
test_vhpet 1.0
hvm_clock_cost=0 hpet_mult=3 hpet_add=0 tick_count=1 debug=0x13

skip_load=1
     HPET: capability 0xf424008086a201 config 0()
           isr 0 counter 0(0)
           timer0 config 0xf0000000000030() cmp 0xffffffffffffffff(-1)
           timer0 period 0(0) fsb 0
           timer1 config 0xf0000000000030() cmp 0xffffffffffffffff(-1)
           timer1 period 0(0) fsb 0
           timer2 config 0xf0000000000030() cmp 0xffffffffffffffff(-1)
           timer2 period 0(0) fsb 0
     HPET: capability 0xf424008086a201 config 0()
           isr 0 counter 0(0)
           timer0 config 0xf0000000000030() cmp 0xffffffffffffffff(-1)
           timer0 period 0(0) fsb 0
           timer1 config 0xf0000000000030() cmp 0xffffffffffffffff(-1)
           timer1 period 0(0) fsb 0
           timer2 config 0xf0000000000030() cmp 0xffffffffffffffff(-1)
           timer2 period 0(0) fsb 0
skip_load
     HPET: capability 0xf424008086a201 config 0()
           isr 0 counter 0(0)
           timer0 config 0xf0000000000030() cmp 0xffffffffffffffff(-1)
           timer0 period 0(0) fsb 0
           timer1 config 0xf0000000000030() cmp 0xffffffffffffffff(-1)
           timer1 period 0(0) fsb 0
           timer2 config 0xf0000000000030() cmp 0xffffffffffffffff(-1)
           timer2 period 0(0) fsb 0

mc64=0x108a8(67,752) mc_offset=0x10898(67,736)

hvm_guest_time=0x10(16)
     HPET: capability 0xf424008086a201 config 0()
           isr 0 counter 0x108a8(67,752)
           timer0 config 0xf0000000000030() cmp 0xffffffffffffffff(-1)
           timer0 period 0(0) fsb 0
           timer1 config 0xf0000000000030() cmp 0xffffffffffffffff(-1)
           timer1 period 0(0) fsb 0
           timer2 config 0xf0000000000030() cmp 0xffffffffffffffff(-1)
           timer2 period 0(0) fsb 0
     HPET: capability 0xf424008086a201 config 0()
           isr 0 counter 0x108a8(67,752)
           timer0 config 0xf0000000000030() cmp 0xffffffffffffffff(-1)
           timer0 period 0(0) fsb 0
           timer1 config 0xf0000000000030() cmp 0xffffffffffffffff(-1)
           timer1 period 0(0) fsb 0
           timer2 config 0xf0000000000030() cmp 0xffffffffffffffff(-1)
           timer2 period 0(0) fsb 0
skip_load
     HPET: capability 0xf424008086a201 config 0()
           isr 0 counter 0x108a8(67,752)
           timer0 config 0xf0000000000030() cmp 0xffffffffffffffff(-1)
           timer0 period 0(0) fsb 0
           timer1 config 0xf0000000000030() cmp 0xffffffffffffffff(-1)
           timer1 period 0(0) fsb 0
           timer2 config 0xf0000000000030() cmp 0xffffffffffffffff(-1)
           timer2 period 0(0) fsb 0

Breakpoint 1, hpet_read64 (h=0x604d68 <dom1+8>, addr=264) at hpet.c:128
128             return hpet_get_comparator(h, HPET_TN(CMP, addr), guest_time_hpet(h));
(gdb) c
Continuing.
hpet_writel: HPET_TN_SETVAL cmp=0x3e514(255,252) timer=0

Breakpoint 1, hpet_read64 (h=0x604d68 <dom1+8>, addr=264) at hpet.c:128
128             return hpet_get_comparator(h, HPET_TN(CMP, addr), guest_time_hpet(h));
(gdb) c
Continuing.
hpet_writel: period=0xf424(62,500) timer=0

Breakpoint 1, hpet_read64 (h=0x604d68 <dom1+8>, addr=264) at hpet.c:128
128             return hpet_get_comparator(h, HPET_TN(CMP, addr), guest_time_hpet(h));
(gdb) b hpet_get_comparator
Breakpoint 2 at 0x400839: file hpet.c, line 87.
(gdb) c
Continuing.

Breakpoint 2, hpet_get_comparator (h=0x604d68 <dom1+8>, tn=0, guest_time=1) at hpet.c:87
87          comparator = h->hpet.comparator64[tn];
(gdb) n
88          if ( timer_is_periodic(h, tn) )
(gdb)
91              uint64_t period = h->hpet.period[tn];
(gdb)
92              if (period)
(gdb)
94                  elapsed = hpet_read_maincounter(h, guest_time) +
(gdb)
95                      period - 1 - comparator;
(gdb)
94                  elapsed = hpet_read_maincounter(h, guest_time) +
(gdb)
96                  comparator += (elapsed / period) * period;
(gdb) p comparator
$1 = 255252
(gdb) p/x comparator
$2 = 0x3e514
(gdb) p elapsed
$3 = 18446744073709426615
(gdb) p/x elapsed
$4 = 0xfffffffffffe17b7
(gdb) p elapsed / period
$5 = 295147905179350
(gdb) p/x elapsed / period
$6 = 0x10c6f7a0b5ed6
(gdb) p 295147905179350 * period
$7 = 18446744073709375000
(gdb) p/x 295147905179350 * period
$8 = 0xfffffffffffd4e18
(gdb) n
97                  h->hpet.comparator64[tn] = comparator;
(gdb) p comparator
$9 = 78636
(gdb) p/x comparator
$10 = 0x1332c
(gdb)


      -Don Slutz




> Jan
>

Re: [PATCH v2 06/10] hvm/hpet: comparator can only change when master clock is enabled.

[Jan Beulich]

>>> On 15.04.14 at 17:53, <dslutz@verizon.com> wrote:
> On 04/15/14 03:05, Jan Beulich wrote:
>>>>> On 14.04.14 at 21:50, <dslutz@verizon.com> wrote:
>>> On 04/14/14 11:07, Jan Beulich wrote:
>>>> As to the change - I'm not sure: The quoted description from the
>>>> specification cal also be read to mean that interrupt generation is
>>>> optional, but comparator increment will always happen. As long as
>>>> this can't be clarified, I'd prefer to stay with the code as is.
>>> I think the code needs to change to match the spec.
>>>
>>> #define timer_enabled(h, n) (timer_config(h, n) & HPET_TN_ENABLE)
>>>
>>> vs
>>>
>>> #define hpet_enabled(h) (h->hpet.config & HPET_CFG_ENABLE)
>>>
>>>
>>> The change uses hpet_enabled() (I.E. Overall Enable).
>> Correct. But do we really need this? When the HPET is globally
>> disabled, hpet_read_maincounter() returns a fixed value, and
>> hence - due to the comparators not changing either -
>> hpet_get_comparator() will too even without the addition. So if
>> at all, the change would be mostly for documentation purposes.
> 
> We do need this.  hpet_get_comparator() does not return a
> fixed value.  Without this change it will always adjust to
> hpet_read_maincounter().

But as said, hpet_read_maincounter() returns a fixed value in that
case too (or at least it is supposed to be).

And no, I'm not fully trusting the test program, so please explain
things relative to the source code rather than by pointing at the
test program showing something.

Jan

Re: [PATCH v2 06/10] hvm/hpet: comparator can only change when master clock is enabled.

[Don Slutz]

On 04/15/14 12:14, Jan Beulich wrote:
>>>> On 15.04.14 at 17:53, <dslutz@verizon.com> wrote:
>> On 04/15/14 03:05, Jan Beulich wrote:
>>>>>> On 14.04.14 at 21:50, <dslutz@verizon.com> wrote:
>>>> On 04/14/14 11:07, Jan Beulich wrote:
>>>>> As to the change - I'm not sure: The quoted description from the
>>>>> specification cal also be read to mean that interrupt generation is
>>>>> optional, but comparator increment will always happen. As long as
>>>>> this can't be clarified, I'd prefer to stay with the code as is.
>>>> I think the code needs to change to match the spec.
>>>>
>>>> #define timer_enabled(h, n) (timer_config(h, n) & HPET_TN_ENABLE)
>>>>
>>>> vs
>>>>
>>>> #define hpet_enabled(h) (h->hpet.config & HPET_CFG_ENABLE)
>>>>
>>>>
>>>> The change uses hpet_enabled() (I.E. Overall Enable).
>>> Correct. But do we really need this? When the HPET is globally
>>> disabled, hpet_read_maincounter() returns a fixed value, and
>>> hence - due to the comparators not changing either -
>>> hpet_get_comparator() will too even without the addition. So if
>>> at all, the change would be mostly for documentation purposes.
>> We do need this.  hpet_get_comparator() does not return a
>> fixed value.  Without this change it will always adjust to
>> hpet_read_maincounter().
> But as said, hpet_read_maincounter() returns a fixed value in that
> case too (or at least it is supposed to be).
>
> And no, I'm not fully trusting the test program, so please explain
> things relative to the source code rather than by pointing at the
> test program showing something.

Ok,

Since this is all about when master clock is not enabled, I will
work with hpet_read_maincounter() returning a fix value. I will
also only talk about timer 0 (tn==0).

Since master clock, comparator, and period can all be set by
the guest, I am picking values:

master clock = 67752 (0x108a8)
comparator = 255252 (0x3e514)
period = 62500 (0xf424)

Using code as of:

commit d2b4c27c0718f27deba00a16317a8ba04c1a2cb7
     xen/arm32: __cmpxchg_mb should be marked always_inline


86:static uint64_t hpet_get_comparator(HPETState *h, unsigned int tn)
87:{
88:    uint64_t comparator;
89:    uint64_t elapsed;
91:    comparator = h->hpet.comparator64[tn];
92:    if ( timer_is_periodic(h, tn) )
93:    {
94:        /* update comparator by number of periods elapsed since last update */
95:        uint64_t period = h->hpet.period[tn];
96:        if (period)
97:        {
98:            elapsed = hpet_read_maincounter(h) + period - 1 - comparator;
99:            comparator += (elapsed / period) * period;
100:            h->hpet.comparator64[tn] = comparator;
101:        }
102:    }
104:    /* truncate if timer is in 32 bit mode */
105:    if ( timer_is_32bit(h, tn) )
106:        comparator = (uint32_t)comparator;
107:    h->hpet.timers[tn].cmp = comparator;
108:    return comparator;
109:}

so on line 98:

elapsed = 67752 + 62500 - 1 - 255252

bc
bc 1.06.95
Copyright 1991-1994, 1997, 1998, 2000, 2004, 2006 Free Software Foundation, Inc.
This is free software with ABSOLUTELY NO WARRANTY.
For details type `warranty'.
67752 + 62500 - 1 - 255252
-125001

Or in hex: 0xFFFFFFFFFFFE17B7

Next:

-125001/62500
-2
-2*62500
-125000


So on line 99 the comparator is changed by -125000.

I.E. the value written is not the value read.

    -Don Slutz

> Jan
>
>

Re: [PATCH v2 06/10] hvm/hpet: comparator can only change when master clock is enabled.

[Jan Beulich]

>>> On 15.04.14 at 20:11, <dslutz@verizon.com> wrote:
> Since this is all about when master clock is not enabled, I will
> work with hpet_read_maincounter() returning a fix value. I will
> also only talk about timer 0 (tn==0).
> 
> Since master clock, comparator, and period can all be set by
> the guest, ...

Oh, okay, this is the piece I ignored so far - I assumed that the
values would be unchanged from when last updated by the
hypervisor. IOW I agree with the change then, but would ask for
the description to point out why this is needed in a more explicit
fashion.

Jan

[PATCH v2 07/10] hvm/hpet: Call hpet_get_comparator during hpet_save.

[Don Slutz]

This changes save data to consistent/expected values.  It is not
technically required because hpet_get_comparator() will adjust from
any value to the correct value.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 xen/arch/x86/hvm/hpet.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index c0529e7..11ba2ba 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -500,6 +500,7 @@ static int hpet_save(struct domain *d, hvm_domain_context_t *h)
 {
     HPETState *hp = domain_vhpet(d);
     int rc;
+    uint64_t guest_time;
 
     spin_lock(&hp->lock);
 
@@ -529,6 +530,12 @@ static int hpet_save(struct domain *d, hvm_domain_context_t *h)
         C(period[1]);
         C(period[2]);
 #undef C
+        /* read the comparator to get it updated so hpet_save will
+         * return the expected value. */
+        guest_time = hp->hpet.mc64 - hp->mc_offset;
+        hpet_get_comparator(hp, 0, guest_time);
+        hpet_get_comparator(hp, 1, guest_time);
+        hpet_get_comparator(hp, 2, guest_time);
         /* save the 64 bit comparator in the 64 bit timer[n].cmp field
          * regardless of whether or not the timer is in 32 bit mode. */
         rec->timers[0].cmp = hp->hpet.comparator64[0];
-- 
1.8.4

Re: [PATCH v2 07/10] hvm/hpet: Call hpet_get_comparator during hpet_save.

[Jan Beulich]

>>> On 08.04.14 at 16:24, <dslutz@verizon.com> wrote:
> This changes save data to consistent/expected values.  It is not
> technically required because hpet_get_comparator() will adjust from
> any value to the correct value.

So am I getting it right that this is just to alter what's in the save image
then? I'm somewhat confused by the wording above, which I first read
mostly like "here is a change that we don't need".

Jan

Re: [PATCH v2 07/10] hvm/hpet: Call hpet_get_comparator during hpet_save.

[Don Slutz]

On 04/14/14 11:13, Jan Beulich wrote:
>>>> On 08.04.14 at 16:24, <dslutz@verizon.com> wrote:
>> This changes save data to consistent/expected values.  It is not
>> technically required because hpet_get_comparator() will adjust from
>> any value to the correct value.
> So am I getting it right that this is just to alter what's in the save image
> then?

Yes.

> I'm somewhat confused by the wording above, which I first read
> mostly like "here is a change that we don't need".

This is a change that is only "needed" with patch #10
"handle 1st period special".

However it does look strange to people
that the output from xen-hvmctx for the comparator
values do not change when the master clock does.

Since hpet_get_comparator() will always adjust
the current value of comparator  to the current value
of the master clock, migration just "works".

    -Don Slutz

> Jan
>

Re: [PATCH v2 07/10] hvm/hpet: Call hpet_get_comparator during hpet_save.

[Jan Beulich]

>>> On 15.04.14 at 02:21, <dslutz@verizon.com> wrote:
> On 04/14/14 11:13, Jan Beulich wrote:
>>>>> On 08.04.14 at 16:24, <dslutz@verizon.com> wrote:
>>> This changes save data to consistent/expected values.  It is not
>>> technically required because hpet_get_comparator() will adjust from
>>> any value to the correct value.
>> So am I getting it right that this is just to alter what's in the save image
>> then?
> 
> Yes.
> 
>> I'm somewhat confused by the wording above, which I first read
>> mostly like "here is a change that we don't need".
> 
> This is a change that is only "needed" with patch #10
> "handle 1st period special".
> 
> However it does look strange to people
> that the output from xen-hvmctx for the comparator
> values do not change when the master clock does.

Maybe you should mention that aspect then in the commit message?

Jan

Re: [PATCH v2 07/10] hvm/hpet: Call hpet_get_comparator during hpet_save.

[Don Slutz]

On 04/15/14 03:06, Jan Beulich wrote:
>>>> On 15.04.14 at 02:21, <dslutz@verizon.com> wrote:
>> On 04/14/14 11:13, Jan Beulich wrote:
>>>>>> On 08.04.14 at 16:24, <dslutz@verizon.com> wrote:




>>> I'm somewhat confused by the wording above, which I first read
>>> mostly like "here is a change that we don't need".
>> This is a change that is only "needed" with patch #10
>> "handle 1st period special".
>>
>> However it does look strange to people
>> that the output from xen-hvmctx for the comparator
>> values do not change when the master clock does.
> Maybe you should mention that aspect then in the commit message?

I will.
    -Don Slutz

> Jan
>

[PATCH v2 08/10] hvm/hpet: Prevent master clock equal to comparator while enabled

[Don Slutz]

Based on the software-developers-hpet-spec-1-0a.pdf, the comparator
for a periodic timer will change to the new value when it matches
the master clock.  The current code here uses a very standard
rounding formula of "((x + y - 1) / y) * y".  This is wrong because
in this case you need to go to the next comparator value when "x"
equals "y". Not when "x + 1" equals "y".  In this case "y" is the
period and "x" is the master clock.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 xen/arch/x86/hvm/hpet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 11ba2ba..28f5224 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -97,7 +97,7 @@ static uint64_t hpet_get_comparator(HPETState *h, unsigned int tn,
         if (period)
         {
             elapsed = hpet_read_maincounter(h, guest_time) +
-                period - 1 - comparator;
+                period - comparator;
             comparator += (elapsed / period) * period;
             h->hpet.comparator64[tn] = comparator;
         }
-- 
1.8.4

[PATCH v2 09/10] hvm/hpet: Correctly limit period to a maximum.

[Don Slutz]

In the code section after the comment:

    /*
     * Clamp period to reasonable min/max values:
     *  - minimum is 100us, same as timers controlled by vpt.c
     *  - maximum is to prevent overflow in time_after() calculations
     */

The current maximum limit actually allows "bad" values like 0 and 1.
This is because it uses a mask not a maximum.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 xen/arch/x86/hvm/hpet.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 28f5224..3226a61 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -419,6 +419,9 @@ static int hpet_write(
             }
             else
             {
+                uint64_t max_period =
+                    (timer_is_32bit(h, tn) ? ~0u : ~0ull) >> 1;
+
                 /*
                  * Clamp period to reasonable min/max values:
                  *  - minimum is 100us, same as timers controlled by vpt.c
@@ -427,7 +430,8 @@ static int hpet_write(
                  */
                 if ( hpet_tick_to_ns(h, new_val) < MICROSECS(100) )
                     new_val = (MICROSECS(100) << 10) / h->hpet_to_ns_scale;
-                new_val &= (timer_is_32bit(h, tn) ? ~0u : ~0ull) >> 1;
+                if ( new_val > max_period )
+                    new_val = max_period;
                 h->hpet.period[tn] = new_val;
             }
         }
-- 
1.8.4

[PATCH v2 10/10] hvm/hpet: handle 1st period special

[Don Slutz]

The software-developers-hpet-spec-1-0a.pdf says that the 1st
interrupt is based on the setting of comparator.  After that it will
be on each period.  Add code that checks for this special case.

Add a callback routine when this special case is active that
will disable the special case on the 1st interrupt.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 xen/arch/x86/hvm/hpet.c       | 56 +++++++++++++++++++++++++++++++++++++------
 xen/include/asm-x86/hvm/vpt.h |  2 ++
 2 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 3226a61..f8ed792 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -94,10 +94,18 @@ static uint64_t hpet_get_comparator(HPETState *h, unsigned int tn,
     {
         /* update comparator by number of periods elapsed since last update */
         uint64_t period = h->hpet.period[tn];
+        uint64_t mc = hpet_read_maincounter(h, guest_time);
+
+        if ( h->hpet.first_enabled[tn] )
+        {
+            if ( mc >= h->hpet.first_mc64[tn] && mc < comparator )
+                period = 0; /* act like oneshot */
+            else
+                h->hpet.first_enabled[tn] = 0;
+        }
         if (period)
         {
-            elapsed = hpet_read_maincounter(h, guest_time) +
-                period - comparator;
+            elapsed = mc + period - comparator;
             comparator += (elapsed / period) * period;
             h->hpet.comparator64[tn] = comparator;
         }
@@ -203,6 +211,13 @@ static void hpet_stop_timer(HPETState *h, unsigned int tn)
  * 1/(2^10) second, namely, 0.9765625 milliseconds */
 #define  HPET_TINY_TIME_SPAN  ((h->stime_freq >> 10) / STIME_PER_HPET_TICK)
 
+static void hpet_time_fired(struct vcpu *v, void *priv)
+{
+    uint8_t *first_enabled_p = (uint8_t *)priv;
+
+    *first_enabled_p = 0;
+}
+
 static void hpet_set_timer(HPETState *h, unsigned int tn, int mc_starting)
 {
     uint64_t tn_cmp, cur_tick, diff;
@@ -223,8 +238,16 @@ static void hpet_set_timer(HPETState *h, unsigned int tn, int mc_starting)
     if ( !timer_enabled(h, tn) )
         return;
 
+    oneshot = !timer_is_periodic(h, tn);
     if ( mc_starting )
+    {
         guest_time = h->hpet.mc64 - h->mc_offset;
+        if ( !oneshot )
+        {
+            h->hpet.first_mc64[tn] = h->hpet.mc64;
+            h->hpet.first_enabled[tn] = 1;
+        }
+    }
     else
         guest_time = guest_time_hpet(h);
 
@@ -262,11 +285,27 @@ static void hpet_set_timer(HPETState *h, unsigned int tn, int mc_starting)
      * have elapsed between the time the comparator was written and the timer
      * being enabled (now).
      */
-    oneshot = !timer_is_periodic(h, 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);
+    if ( oneshot )
+        create_periodic_time(vhpet_vcpu(h), &h->pt[tn],
+                             hpet_tick_to_ns(h, diff),
+                             0, irq, NULL, NULL);
+    else
+    {
+        if ( diff <= h->hpet.period[tn] )
+        {
+            h->hpet.first_enabled[tn] = 0;
+            create_periodic_time(vhpet_vcpu(h), &h->pt[tn],
+                                 hpet_tick_to_ns(h, diff),
+                                 hpet_tick_to_ns(h, h->hpet.period[tn]),
+                                 irq, NULL, NULL);
+        }
+        else
+            create_periodic_time(vhpet_vcpu(h), &h->pt[tn],
+                                 hpet_tick_to_ns(h, diff),
+                                 hpet_tick_to_ns(h, h->hpet.period[tn]),
+                                 irq, hpet_time_fired,
+                                 &h->hpet.first_enabled[tn]);
+    }
 }
 
 static inline uint64_t hpet_fixup_reg(
@@ -591,6 +630,9 @@ static int hpet_load(struct domain *d, hvm_domain_context_t *h)
         if ( timer_is_32bit(hp, i) )
             cmp = (uint32_t)cmp;
         hp->hpet.timers[i].cmp = cmp;
+        /* Init hidden regs also */
+        hp->hpet.first_mc64[i] = 0;
+        hp->hpet.first_enabled[i] = 0;
     }
 #undef C
 
diff --git a/xen/include/asm-x86/hvm/vpt.h b/xen/include/asm-x86/hvm/vpt.h
index 41159d8..aea5121 100644
--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -87,6 +87,8 @@ struct hpet_registers {
     /* Hidden register state */
     uint64_t period[HPET_TIMER_NUM]; /* Last value written to comparator */
     uint64_t comparator64[HPET_TIMER_NUM]; /* 64 bit running comparator */
+    uint64_t first_mc64[HPET_TIMER_NUM]; /* 1st interval main counter */
+    uint8_t first_enabled[HPET_TIMER_NUM]; /* In 1st interval */
 };
 
 typedef struct HPETState {
-- 
1.8.4

Re: [PATCH v2 10/10] hvm/hpet: handle 1st period special

[Jan Beulich]

>>> On 08.04.14 at 16:24, <dslutz@verizon.com> wrote:
> The software-developers-hpet-spec-1-0a.pdf says that the 1st
> interrupt is based on the setting of comparator.  After that it will
> be on each period.  Add code that checks for this special case.

The way hpet_set_timer() call create_periodic_time() to me seems to
do exactly that. What am I missing?

Jan

Re: [PATCH v2 10/10] hvm/hpet: handle 1st period special

[Don Slutz]

On 04/14/14 11:27, Jan Beulich wrote:
>>>> On 08.04.14 at 16:24, <dslutz@verizon.com> wrote:
>> The software-developers-hpet-spec-1-0a.pdf says that the 1st
>> interrupt is based on the setting of comparator.  After that it will
>> be on each period.  Add code that checks for this special case.
> The way hpet_set_timer() call create_periodic_time() to me seems to
> do exactly that. What am I missing?

Since hpet_get_comparator() will always adjust
the current value of comparator  to the current value
of the master clock, the call to create_periodic_time()
which uses diff which is:


     tn_cmp   = hpet_get_comparator(h, tn);
     cur_tick = hpet_read_maincounter(h);
     ...
     diff = tn_cmp - cur_tick;



So using the manual test code and asking for 1 and 3 starting
periods:

dcs-xen-54:~/xen/tools/tests/vhpet>./test_vhpet 0 1 0 3 3 >foo
hpet: T0 Error: Set 0x1fcbd(130,237) != 0x1e848(125,000)
...

Which is not the default mode, and the important arguments are:

hvm_clock_cost=0 hpet_mult=1 hpet_add=0

(I.E. the 1st 3)

create_periodic_time: pt=0 delta=916,208 period=1,000,000 - 1,000.00 Hz irq=0


and

dcs-xen-54:~/xen/tools/tests/vhpet>./test_vhpet 0 3 0 3 3 >foo
hpet: T0 Error: Set 0x3e505(255,237) != 0x1e848(125,000)
...
(hvm_clock_cost=0 hpet_mult=3 hpet_add=0)


create_periodic_time: pt=0 delta=916,208 period=1,000,000 - 1,000.00 Hz irq=0


Notice that the cmp jumps back to the same value.  The master clock is not at
the period (it is 0x10899(67,737)) and so cmp is adjusted "backwards" in this case.

This output is with no changes.  It is not until patch #6:
hvm/hpet: comparator can only change when master clock is enabled.

That the unit test stops printing the error.   However the hpet_save output still
shows the changed value.

It is not till after this patch that:

dcs-xen-54:~/xen/tools/tests/vhpet>./test_vhpet 0 1 0 3 0x13 >foo1
dcs-xen-54:~/xen/tools/tests/vhpet>./test_vhpet 0 3 0 3 0x13 >foo3
dcs-xen-54:~/xen>diff -suwp tools/tests/vhpet/foo1 tools/tests/vhpet/foo3 | head -30
--- tools/tests/vhpet/foo1      2014-04-14 20:17:51.923684610 -0400
+++ tools/tests/vhpet/foo3      2014-04-14 20:19:02.148251889 -0400
@@ -1,5 +1,5 @@
  test_vhpet 1.0
-hvm_clock_cost=0 hpet_mult=1 hpet_add=0 tick_count=3 debug=0x13
+hvm_clock_cost=0 hpet_mult=3 hpet_add=0 tick_count=3 debug=0x13

  skip_load=1
      HPET: capability 0xf424008086a201 config 0()
@@ -56,19 +56,19 @@ skip_load
            timer1 period 0(0) fsb 0
            timer2 config 0xf0000000000030() cmp 0xffffffffffffffff(-1)
            timer2 period 0(0) fsb 0
-hpet_writel: HPET_TN_SETVAL cmp=0x1fccc(130,252) timer=0
+hpet_writel: HPET_TN_SETVAL cmp=0x3e514(255,252) timer=0
  hpet_writel: period=0xf424(62,500) timer=0
  create_periodic_time: mc64=0x108a8(67,752) mc_offset=0x108a7(67,751)
-                 [0] cmp64=0x1fccc(130,252) cmp=0x1fccc(130,252)
+                 [0] cmp64=0x3e514(255,252) cmp=0x3e514(255,252)
                   [1] cmp64=0xffffffffffffffff(-1) cmp=0xffffffffffffffff(-1)
                   [2] cmp64=0xffffffffffffffff(-1) cmp=0xffffffffffffffff(-1)
-create_periodic_time: pt=0 delta=1,000,000 period=1,000,000 - 1,000.00 Hz irq=0
+create_periodic_time: pt=0 delta=3,000,000 period=1,000,000 - 1,000.00 Hz irq=0
  hpet: hpet_set_mode(402):
  hpet: ID: 0x8086a201, PERIOD: 0xf42400
  hpet: CFG: 0x1, STATUS: 0x0
  hpet: COUNTER_l: 0x108a8, COUNTER_h: 0x0
  hpet: T0: CFG_l: 0x13c, CFG_h: 0xf00000
-hpet: T0: CMP_l: 0x1fccc, CMP_h: 0x0
+hpet: T0: CMP_l: 0x3e514, CMP_h: 0x0


that the 1st delta is actually the request one in both cases.


    -Don Slutz


> Jan
>