* [PATCH v3 00/11] Prevent one cause of "MP-BIOS bug: 8254 timer"... message from linux
@ 2014-04-17 17:42 Don Slutz
2014-04-17 17:42 ` [optional PATCH v3 01/11] hvm/hpet: Add manual unit test code Don Slutz
` (10 more replies)
0 siblings, 11 replies; 34+ messages in thread
From: Don Slutz @ 2014-04-17 17:42 UTC (permalink / raw)
To: xen-devel; +Cc: Keir Fraser, Don Slutz, Jan Beulich
Changes v2 to v3:
Add new patch #8 "hvm/hpet: Use signed divide in hpet_get_comparator."
Re-order to group changes in the same area:
#5, #6 are both about hpet_save
#8, #9, #10, and #11 are all about corner cases of the lines in
hpet_get_comparator():
elapsed = master clock ...
comparator += (elapsed / period) * period
#4 "hvm/hpet: Correctly limit period to a maximum."
was v2 #9
#5 "hvm/hpet: In hpet_save, correctly compute mc64."
was v2 #4
#6 "hvm/hpet: In hpet_save, call hpet_get_comparator."
was v2 #7 "hvm/hpet: Call hpet_get_comparator during hpet_save."
#7 "hvm/hpet: Init comparator64 like comparator."
was v2 #5
#9 "hvm/hpet: comparator can only change when master"
was v2 #6
#10 "hvm/hpet: Prevent master clock equal to comparator"
was v2 #8
#11 "hvm/hpet: handle 1st period special"
was v2 #10
My changes:
#1 "hvm/hpet: Add manual unit test code.":
Make it optional.
Add hpet_check_stopped() testing.
Adjust print_error messages.
#6 "hvm/hpet: In hpet_save, call hpet_get_comparator.":
Reword subject from "hvm/hpet: Call hpet_get_comparator during hpet_save."
#11 "hvm/hpet: handle 1st period special":
More setting of first_mc64 & first_enabled when needed.
Switch to bool_t.
Jan Beulich:
#1 "hvm/hpet: Add manual unit test code.":
Add Makefile.
Better commit message.
#2 "hvm/hpet: Only call guest_time_hpet(h) one time per":
Did not add Reviewed-by do to amount of change
Added passing of guest_time to hpet_read64() and
hpet_stop_timer().
Dropped mc_starting.
#3 "hvm/hpet: Only set comparator or period not both.":
Only have 2 blocks of code.
Set comparator64 before truncation
#6 "hvm/hpet: In hpet_save, call hpet_get_comparator.":
Better commit message.
#9 "hvm/hpet: comparator can only change when master":
Better commit message.
#11 "hvm/hpet: handle 1st period special":
Better commit message.
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 (11):
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: Correctly limit period to a maximum.
hvm/hpet: In hpet_save, correctly compute mc64.
hvm/hpet: In hpet_save, call hpet_get_comparator.
hvm/hpet: Init comparator64 like comparator.
hvm/hpet: Use signed divide in hpet_get_comparator.
hvm/hpet: comparator can only change when master clock is enabled.
hvm/hpet: Prevent master clock equal to comparator while enabled
hvm/hpet: handle 1st period special
tools/tests/vhpet/.gitignore | 4 +
tools/tests/vhpet/Makefile | 28 ++
tools/tests/vhpet/emul.h | 416 +++++++++++++++++++++++
tools/tests/vhpet/main.c | 768 ++++++++++++++++++++++++++++++++++++++++++
xen/arch/x86/hvm/hpet.c | 192 ++++++++---
xen/include/asm-x86/hvm/vpt.h | 2 +
6 files changed, 1359 insertions(+), 51 deletions(-)
create mode 100644 tools/tests/vhpet/.gitignore
create mode 100644 tools/tests/vhpet/Makefile
create mode 100644 tools/tests/vhpet/emul.h
create mode 100644 tools/tests/vhpet/main.c
--
1.8.4
^ permalink raw reply [flat|nested] 34+ messages in thread
* [optional PATCH v3 01/11] hvm/hpet: Add manual unit test code.
2014-04-17 17:42 [PATCH v3 00/11] Prevent one cause of "MP-BIOS bug: 8254 timer"... message from linux Don Slutz
@ 2014-04-17 17:42 ` Don Slutz
2014-04-23 14:41 ` Jan Beulich
2014-04-17 17:42 ` [PATCH v3 02/11] hvm/hpet: Only call guest_time_hpet(h) one time per action Don Slutz
` (9 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Don Slutz @ 2014-04-17 17:42 UTC (permalink / raw)
To: xen-devel; +Cc: Keir Fraser, Don Slutz, Jan Beulich
Add the code at tools/tests/vhpet.
See comment in tools/tests/vhpet/main.c for details on running
either in a xen source tree or elsewhere.
A basic in source tree usage is:
make -C tools/tests/vhpet run
Does repro the bug:
..MP-BIOS bug: 8254 timer not connected to IO-APIC
The make file includes coping hpet.c and hpet.h from the source
tree. hpet.c is then modifed to remove all include file and add the
emul.h include file.
The manual test code has only a few automatic checks that output
messages to stderr:
1) Possible ..MP-BIOS bug: 8254 timer...
if 1st period is not <= the expected value
2) hpet_set_mode(%ld): T%d Error: Set ...
if read of comparator != write of comparator in
3) hpet_check_stopped(%ld): T%d Error: Set ...
if read != write
4) main(%ld): With clock stopped mc64 changed: ...
if hpet_save returns different master clock values when called
more then once.
It also generates a lot of output, which is why the sugested way to
use includes a redirect of stdout to a file.
Signed-off-by: Don Slutz <dslutz@verizon.com>
---
v3:
Add Makefile.
Better commit message.
Make it optional.
Add hpet_check_stopped() testing.
Adjust print_error messages.
tools/tests/vhpet/.gitignore | 4 +
tools/tests/vhpet/Makefile | 28 ++
tools/tests/vhpet/emul.h | 416 +++++++++++++++++++++++
tools/tests/vhpet/main.c | 768 +++++++++++++++++++++++++++++++++++++++++++
4 files changed, 1216 insertions(+)
create mode 100644 tools/tests/vhpet/.gitignore
create mode 100644 tools/tests/vhpet/Makefile
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..4cefa62
--- /dev/null
+++ b/tools/tests/vhpet/.gitignore
@@ -0,0 +1,4 @@
+test_vhpet
+test_vhpet.out
+hpet.h
+hpet.c
diff --git a/tools/tests/vhpet/Makefile b/tools/tests/vhpet/Makefile
new file mode 100644
index 0000000..4a21355
--- /dev/null
+++ b/tools/tests/vhpet/Makefile
@@ -0,0 +1,28 @@
+
+XEN_ROOT=$(CURDIR)/../../..
+include $(XEN_ROOT)/tools/Rules.mk
+
+TARGET := test_vhpet
+
+.PHONY: all
+all: $(TARGET)
+
+.PHONY: run
+run: $(TARGET)
+ ./$(TARGET) > $(TARGET).out
+
+$(TARGET): hpet.c main.c hpet.h emul.h Makefile
+ $(HOSTCC) -g -o $@ hpet.c main.c
+
+.PHONY: clean
+clean:
+ rm -rf $(TARGET) $(TARGET).out *.o *~ core* hpet.h hpet.c
+
+.PHONY: install
+install:
+
+hpet.h: $(XEN_ROOT)/xen/include/asm-x86/hpet.h
+ cp $(XEN_ROOT)/xen/include/asm-x86/hpet.h .
+
+hpet.c: $(XEN_ROOT)/xen/arch/x86/hvm/hpet.c
+ sed -e "/#include/d" -e "1i#include \"emul.h\"\n" <$(XEN_ROOT)/xen/arch/x86/hvm/hpet.c >hpet.c
diff --git a/tools/tests/vhpet/emul.h b/tools/tests/vhpet/emul.h
new file mode 100644
index 0000000..09e4611
--- /dev/null
+++ b/tools/tests/vhpet/emul.h
@@ -0,0 +1,416 @@
+/*
+ * 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 offset64[HPET_TIMER_NUM]; /* offset so comparator calc "works" */
+ uint64_t first_mc64[HPET_TIMER_NUM]; /* 1st interval main counter */
+ bool_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)
+#define TRACE_6D(a, b, c, d, e, f, g)
+
+#define TRC_PAR_LONG(par) ((par)&0xFFFFFFFF),((par)>>32)
+
+#define TRACE_2_LONG_2D(_e, d1, d2, ...) \
+ TRACE_4D(_e, d1, d2)
+#define TRACE_2_LONG_3D(_e, d1, d2, d3, ...) \
+ TRACE_5D(_e, d1, d2, d3)
+#define TRACE_2_LONG_4D(_e, d1, d2, d3, d4, ...) \
+ TRACE_6D(_e, d1, d2, d3, d4)
+
+/* 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..e24f038
--- /dev/null
+++ b/tools/tests/vhpet/main.c
@@ -0,0 +1,768 @@
+/*
+ * 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 >test_vhpet.out
+
+ *
+ *
+ * This is almost the same as
+ *
+
+ make run
+
+ *
+ * Or
+ *
+ * make -C tools/tests/vhpet run
+ *
+ * From a xen source tree. The differance
+ * is that you need to be in a xen source tree
+ * and normal make rules apply.
+ *
+ */
+
+#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 & 0x1)
+ {
+ printf("skip_load=%#x\n", skip_load);
+ }
+ 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 ( debug & 0x0010 )
+ {
+ int i;
+
+ printf("create_periodic_time: "
+ "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);
+ for (i = 0; i < 3; i++)
+ {
+ printf(" "
+ "[%d] cmp64=%#lx(%'ld) cmp=%#lx(%'ld)\n", i,
+ dom1.arch.hvm_domain.pl_time.vhpet.hpet.comparator64[i],
+ dom1.arch.hvm_domain.pl_time.vhpet.hpet.comparator64[i],
+ dom1.arch.hvm_domain.pl_time.vhpet.hpet.timers[i].cmp,
+ dom1.arch.hvm_domain.pl_time.vhpet.hpet.timers[i].cmp);
+ }
+ }
+ 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("%s(%ld): Possible ..MP-BIOS bug: 8254 timer...: delta=%'"PRId64
+ " period=%'"PRIu64"\n", __func__, __LINE__,
+ 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(unsigned long low, unsigned long high)
+{
+ hpet_writel(low, HPET_COUNTER);
+ hpet_writel(high, 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(0, 0);
+ 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("%s(%ld): HPET_TN_SETVAL cmp=%#lx(%'ld) timer=%d\n",
+ __func__, __LINE__, 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("%s(%ld): period=%#lx(%'ld) timer=%d\n", __func__, __LINE__,
+ (unsigned long) delta, (unsigned long) delta, timer);
+ cmp2 = hpet_readl(HPET_Tn_CMP(timer));
+ if ( cmp2 != cmp )
+ print_error("%s(%ld): T%d Error: Set %#lx(%'ld) != %#lx(%'ld)\n",
+ __func__, __LINE__, timer, cmp, cmp, cmp2, cmp2);
+
+ hpet_start_counter();
+ hpet_print_config();
+}
+
+
+hpet_check_stopped(uint64_t old_delta, int timer)
+{
+ unsigned long mc_low, mc_high, old_cmp, now;
+ unsigned long cfg, cmp, delta, cmp2, cmp3;
+
+ if (skip_load & 0x2)
+ {
+ printf("Skip hpet_check_stopped. skip_load=%#x\n", skip_load);
+ return;
+ }
+ hpet_stop_counter();
+ mc_low = hpet_readl(HPET_COUNTER);
+ mc_high = hpet_readl(HPET_COUNTER + 4);
+ old_cmp = hpet_readl(HPET_Tn_CMP(timer));
+
+ hpet_reset_counter(67752, 0);
+ cmp = 255252;
+ delta = 62500;
+
+ now = hpet_readl(HPET_COUNTER);
+ if ( now != 67752 )
+ print_error("%s(%ld): T%d Error: Set mc %#lx(%'ld) != %#lx(%'ld)\n",
+ __func__, __LINE__, timer, 67752, 67752, now, now);
+ cfg = hpet_readl(HPET_Tn_CFG(timer));
+ cfg |= HPET_TN_SETVAL;
+ hpet_writel(cfg, HPET_Tn_CFG(timer));
+ hpet_writel(cmp, HPET_Tn_CMP(timer));
+ printf("%s(%ld): HPET_TN_SETVAL cmp=%#lx(%'ld) timer=%d\n",
+ __func__, __LINE__, cmp, cmp, timer);
+ cmp2 = hpet_readl(HPET_Tn_CMP(timer));
+ if ( cmp2 != cmp )
+ print_error("%s(%ld): T%d Error: Set cmp %#lx(%'ld) != %#lx(%'ld)\n",
+ __func__, __LINE__, timer, cmp, cmp, cmp2, cmp2);
+
+ hpet_writel((unsigned long) delta, HPET_Tn_CMP(timer));
+ printf("%s(%ld): period=%#lx(%'ld) timer=%d\n", __func__, __LINE__,
+ (unsigned long) delta, (unsigned long) delta, timer);
+ cmp3 = hpet_readl(HPET_Tn_CMP(timer));
+ if ( cmp3 != cmp )
+ print_error("%s(%ld): T%d Error: Set period, cmp %#lx(%'ld) != %#lx(%'ld)\n",
+ __func__, __LINE__, timer, cmp, cmp, cmp3, cmp3);
+
+ if ( dom1.arch.hvm_domain.pl_time.vhpet.hpet.period[timer] != delta )
+ printf("%s(%ld): T%d Warning: Set period %#lx(%'ld) != %#lx(%'ld)\n",
+ __func__, __LINE__, timer, delta, delta,
+ dom1.arch.hvm_domain.pl_time.vhpet.hpet.period[timer],
+ dom1.arch.hvm_domain.pl_time.vhpet.hpet.period[timer]);
+
+ hpet_reset_counter(67752, 0);
+ cmp = 255252;
+ delta = 62500;
+
+ now = hpet_readl(HPET_COUNTER);
+ if ( now != 67752 )
+ print_error("%s(%ld): T%d Error: Set mc %#lx(%'ld) != %#lx(%'ld)\n",
+ __func__, __LINE__, timer, 67752, 67752, now, now);
+ cfg = hpet_readl(HPET_Tn_CFG(timer));
+ cfg |= HPET_TN_SETVAL;
+ hpet_writel(cfg, HPET_Tn_CFG(timer));
+ hpet_writel(cmp, HPET_Tn_CMP(timer));
+ printf("%s(%ld): HPET_TN_SETVAL cmp=%#lx(%'ld) timer=%d\n",
+ __func__, __LINE__, cmp, cmp, timer);
+ cmp2 = hpet_readl(HPET_Tn_CMP(timer));
+ if ( cmp2 != cmp )
+ print_error("%s(%ld): T%d Error: Set cmp %#lx(%'ld) != %#lx(%'ld)\n",
+ __func__, __LINE__, timer, cmp, cmp, cmp2, cmp2);
+
+ hpet_writel((unsigned long) delta, HPET_Tn_CMP(timer));
+ printf("%s(%ld): period=%#lx(%'ld) timer=%d\n", __func__, __LINE__,
+ (unsigned long) delta, (unsigned long) delta, timer);
+ cmp3 = hpet_readl(HPET_Tn_CMP(timer));
+ if ( cmp3 != cmp )
+ print_error("%s(%ld): T%d Error: Set period, cmp %#lx(%'ld) != %#lx(%'ld)\n",
+ __func__, __LINE__, timer, cmp, cmp, cmp3, cmp3);
+
+ if ( dom1.arch.hvm_domain.pl_time.vhpet.hpet.period[timer] != delta )
+ printf("%s(%ld): T%d Warning: Set period %#lx(%'ld) != %#lx(%'ld)\n",
+ __func__, __LINE__, timer, delta, delta,
+ dom1.arch.hvm_domain.pl_time.vhpet.hpet.period[timer],
+ dom1.arch.hvm_domain.pl_time.vhpet.hpet.period[timer]);
+
+ hpet_reset_counter(67700, 0);
+
+ now = hpet_readl(HPET_COUNTER);
+ if ( now != 67700 )
+ print_error("%s(%ld): T%d Error: Set mc %#lx(%'ld) != %#lx(%'ld)\n",
+ __func__, __LINE__, timer, 67752, 67752, now, now);
+ cmp2 = hpet_readl(HPET_Tn_CMP(timer));
+ if ( cmp2 != cmp )
+ print_error("%s(%ld): T%d Error: Set mc, cmp %#lx(%'ld) != %#lx(%'ld)\n",
+ __func__, __LINE__, timer, cmp, cmp, cmp2, cmp2);
+
+ cmp3 = hpet_readl(HPET_Tn_CMP(timer));
+ if ( cmp3 != cmp )
+ print_error("%s(%ld): T%d Error: Set mc, cmp %#lx(%'ld) != %#lx(%'ld)\n",
+ __func__, __LINE__, timer, cmp, cmp, cmp3, cmp3);
+
+ if ( dom1.arch.hvm_domain.pl_time.vhpet.hpet.period[timer] != delta )
+ printf("%s(%ld): T%d Warning: Set mc, period %#lx(%'ld) != %#lx(%'ld)\n",
+ __func__, __LINE__, timer, delta, delta,
+ dom1.arch.hvm_domain.pl_time.vhpet.hpet.period[timer],
+ dom1.arch.hvm_domain.pl_time.vhpet.hpet.period[timer]);
+
+ cmp = 67701;
+
+ now = hpet_readl(HPET_COUNTER);
+ if ( now != 67700 )
+ print_error("%s(%ld): T%d Error: Set cmp, mc %#lx(%'ld) != %#lx(%'ld)\n",
+ __func__, __LINE__, timer, 67752, 67752, now, now);
+ cfg = hpet_readl(HPET_Tn_CFG(timer));
+ cfg |= HPET_TN_SETVAL;
+ hpet_writel(cfg, HPET_Tn_CFG(timer));
+ hpet_writel(cmp, HPET_Tn_CMP(timer));
+ printf("%s(%ld): HPET_TN_SETVAL cmp=%#lx(%'ld) timer=%d\n",
+ __func__, __LINE__, cmp, cmp, timer);
+ cmp2 = hpet_readl(HPET_Tn_CMP(timer));
+ if ( cmp2 != cmp )
+ print_error("%s(%ld): T%d Error: Set cmp, cmp %#lx(%'ld) != %#lx(%'ld)\n",
+ __func__, __LINE__, timer, cmp, cmp, cmp2, cmp2);
+
+ cmp3 = hpet_readl(HPET_Tn_CMP(timer));
+ if ( cmp3 != cmp )
+ print_error("%s(%ld): T%d Error: Set cmp, cmp %#lx(%'ld) != %#lx(%'ld)\n",
+ __func__, __LINE__, timer, cmp, cmp, cmp3, cmp3);
+
+ if ( dom1.arch.hvm_domain.pl_time.vhpet.hpet.period[timer] != delta )
+ printf("%s(%ld): T%d Warning: Set cmp, period %#lx(%'ld) != %#lx(%'ld)\n",
+ __func__, __LINE__, timer, delta, delta,
+ dom1.arch.hvm_domain.pl_time.vhpet.hpet.period[timer],
+ dom1.arch.hvm_domain.pl_time.vhpet.hpet.period[timer]);
+
+ delta = 500;
+
+ now = hpet_readl(HPET_COUNTER);
+ if ( now != 67700 )
+ print_error("%s(%ld): T%d Error: Set period, mc %#lx(%'ld) != %#lx(%'ld)\n",
+ __func__, __LINE__, timer, 67752, 67752, now, now);
+ cmp2 = hpet_readl(HPET_Tn_CMP(timer));
+ if ( cmp2 != cmp )
+ print_error("%s(%ld): T%d Error: Set period, cmp %#lx(%'ld) != %#lx(%'ld)\n",
+ __func__, __LINE__, timer, cmp, cmp, cmp2, cmp2);
+
+ hpet_writel((unsigned long) delta, HPET_Tn_CMP(timer));
+ printf("%s(%ld): period=%#lx(%'ld) timer=%d\n", __func__, __LINE__,
+ (unsigned long) delta, (unsigned long) delta, timer);
+ cmp3 = hpet_readl(HPET_Tn_CMP(timer));
+ if ( cmp3 != cmp )
+ print_error("%s(%ld): T%d Error: Set period, cmp %#lx(%'ld) != %#lx(%'ld)\n",
+ __func__, __LINE__, timer, cmp, cmp, cmp3, cmp3);
+
+ if ( dom1.arch.hvm_domain.pl_time.vhpet.hpet.period[timer] != delta )
+ printf("%s(%ld): T%d Warning: Set period, period %#lx(%'ld) != %#lx(%'ld)\n",
+ __func__, __LINE__, timer, delta, delta,
+ dom1.arch.hvm_domain.pl_time.vhpet.hpet.period[timer],
+ dom1.arch.hvm_domain.pl_time.vhpet.hpet.period[timer]);
+
+ hpet_reset_counter(mc_low, mc_high);
+ cfg = hpet_readl(HPET_Tn_CFG(timer));
+ cfg |= HPET_TN_SETVAL;
+ hpet_writel(cfg, HPET_Tn_CFG(timer));
+ hpet_writel(old_cmp, HPET_Tn_CMP(timer));
+ hpet_writel(old_delta, HPET_Tn_CMP(timer));
+ hpet_start_counter();
+}
+
+
+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 = 3; 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("%s(%ld): With clock stopped mc64 changed: %'ld to %'ld\n",
+ __func__, __LINE__, 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("%s(%ld): With clock stopped mc64 changed: %'ld to %'ld\n",
+ __func__, __LINE__, 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("%s(%ld): With clock stopped mc64 changed: %'ld to %'ld\n",
+ __func__, __LINE__, 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("%s(%ld): With clock stopped mc64 changed: %'ld to %'ld\n",
+ __func__, __LINE__, hpet1.mc64, hpet2.mc64);
+
+ hpet_set_mode(0xf424, 0);
+ hpet_check_stopped(0xf424, 0);
+
+ 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;
+
+ hpet_set_mode(0, 1);
+ hpet_check_stopped(0, 1);
+
+ 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;
+
+ hpet_set_mode(~0ULL, 2);
+ hpet_check_stopped(~0ULL, 2);
+
+ hpet_set_mode(0x80000000, 2);
+ hpet_check_stopped(0x80000000, 2);
+
+ 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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 02/11] hvm/hpet: Only call guest_time_hpet(h) one time per action.
2014-04-17 17:42 [PATCH v3 00/11] Prevent one cause of "MP-BIOS bug: 8254 timer"... message from linux Don Slutz
2014-04-17 17:42 ` [optional PATCH v3 01/11] hvm/hpet: Add manual unit test code Don Slutz
@ 2014-04-17 17:42 ` Don Slutz
2014-04-23 15:07 ` Jan Beulich
2014-04-17 17:42 ` [PATCH v3 03/11] hvm/hpet: Only set comparator or period not both Don Slutz
` (8 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Don Slutz @ 2014-04-17 17:42 UTC (permalink / raw)
To: xen-devel; +Cc: Keir Fraser, Don Slutz, Jan Beulich
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>
---
v3:
Did not add Reviewed-by (Jan Beulich) do to amount of change
Added passing of guest_time to hpet_read64() and
hpet_stop_timer().
Dropped mc_starting.
xen/arch/x86/hvm/hpet.c | 55 ++++++++++++++++++++++++++++++-------------------
1 file changed, 34 insertions(+), 21 deletions(-)
diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 42c93f3..4fd6470 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -75,17 +75,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;
@@ -97,7 +98,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;
}
@@ -109,7 +111,8 @@ static uint64_t hpet_get_comparator(HPETState *h, unsigned int tn)
h->hpet.timers[tn].cmp = comparator;
return comparator;
}
-static inline uint64_t hpet_read64(HPETState *h, unsigned long addr)
+static inline uint64_t hpet_read64(HPETState *h, unsigned long addr,
+ uint64_t guest_time)
{
addr &= ~7;
@@ -122,7 +125,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);
case HPET_Tn_CFG(0):
case HPET_Tn_CFG(1):
case HPET_Tn_CFG(2):
@@ -130,7 +133,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);
case HPET_Tn_ROUTE(0):
case HPET_Tn_ROUTE(1):
case HPET_Tn_ROUTE(2):
@@ -176,7 +179,7 @@ static int hpet_read(
spin_lock(&h->lock);
- val = hpet_read64(h, addr);
+ val = hpet_read64(h, addr, guest_time_hpet(h));
result = val;
if ( length != 8 )
@@ -189,7 +192,8 @@ static int hpet_read(
return X86EMUL_OKAY;
}
-static void hpet_stop_timer(HPETState *h, unsigned int tn)
+static void hpet_stop_timer(HPETState *h, unsigned int tn,
+ uint64_t guest_time)
{
ASSERT(tn < HPET_TIMER_NUM);
ASSERT(spin_is_locked(&h->lock));
@@ -197,14 +201,15 @@ 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);
}
/* 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,
+ uint64_t guest_time)
{
uint64_t tn_cmp, cur_tick, diff;
unsigned int irq;
@@ -223,8 +228,8 @@ 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);
+ 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;
@@ -282,6 +287,7 @@ static int hpet_write(
{
HPETState *h = vcpu_vhpet(v);
uint64_t old_val, new_val;
+ uint64_t guest_time;
int tn, i;
/* Acculumate a bit mask of timers whos state is changed by this write. */
@@ -298,7 +304,8 @@ static int hpet_write(
spin_lock(&h->lock);
- old_val = hpet_read64(h, addr);
+ guest_time = guest_time_hpet(h);
+ old_val = hpet_read64(h, addr, guest_time);
new_val = val;
if ( length != 8 )
new_val = hpet_fixup_reg(
@@ -313,7 +320,7 @@ static int hpet_write(
if ( !(old_val & HPET_CFG_ENABLE) && (new_val & HPET_CFG_ENABLE) )
{
/* Enable main counter and interrupt generation. */
- h->mc_offset = h->hpet.mc64 - guest_time_hpet(h);
+ h->mc_offset = h->hpet.mc64 - guest_time;
for ( i = 0; i < HPET_TIMER_NUM; i++ )
{
h->hpet.comparator64[i] =
@@ -327,7 +334,7 @@ static int hpet_write(
else if ( (old_val & HPET_CFG_ENABLE) && !(new_val & HPET_CFG_ENABLE) )
{
/* Halt main counter and disable interrupt generation. */
- h->hpet.mc64 = h->mc_offset + guest_time_hpet(h);
+ h->hpet.mc64 = h->mc_offset + guest_time;
for ( i = 0; i < HPET_TIMER_NUM; i++ )
if ( timer_enabled(h, i) )
set_stop_timer(i);
@@ -441,14 +448,14 @@ static int hpet_write(
{
i = find_first_set_bit(stop_timers);
__clear_bit(i, &stop_timers);
- hpet_stop_timer(h, i);
+ hpet_stop_timer(h, i, guest_time);
}
while (start_timers)
{
i = find_first_set_bit(start_timers);
__clear_bit(i, &start_timers);
- hpet_set_timer(h, i);
+ hpet_set_timer(h, i, guest_time);
}
#undef set_stop_timer
@@ -524,6 +531,7 @@ static int hpet_load(struct domain *d, hvm_domain_context_t *h)
HPETState *hp = domain_vhpet(d);
struct hvm_hw_hpet *rec;
uint64_t cmp;
+ uint64_t guest_time;
int i;
spin_lock(&hp->lock);
@@ -562,14 +570,15 @@ static int hpet_load(struct domain *d, hvm_domain_context_t *h)
#undef C
/* Recalculate the offset between the main counter and guest time */
- hp->mc_offset = hp->hpet.mc64 - guest_time_hpet(hp);
+ guest_time = guest_time_hpet(hp);
+ hp->mc_offset = hp->hpet.mc64 - guest_time;
/* restart all timers */
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, guest_time);
spin_unlock(&hp->lock);
@@ -617,9 +626,13 @@ void hpet_deinit(struct domain *d)
spin_lock(&h->lock);
if ( hpet_enabled(h) )
+ {
+ uint64_t guest_time = guest_time_hpet(h);
+
for ( i = 0; i < HPET_TIMER_NUM; i++ )
if ( timer_enabled(h, i) )
- hpet_stop_timer(h, i);
+ hpet_stop_timer(h, i, guest_time);
+ }
spin_unlock(&h->lock);
}
--
1.8.4
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 03/11] hvm/hpet: Only set comparator or period not both.
2014-04-17 17:42 [PATCH v3 00/11] Prevent one cause of "MP-BIOS bug: 8254 timer"... message from linux Don Slutz
2014-04-17 17:42 ` [optional PATCH v3 01/11] hvm/hpet: Add manual unit test code Don Slutz
2014-04-17 17:42 ` [PATCH v3 02/11] hvm/hpet: Only call guest_time_hpet(h) one time per action Don Slutz
@ 2014-04-17 17:42 ` Don Slutz
2014-04-23 15:10 ` Jan Beulich
2014-04-17 17:42 ` [PATCH v3 04/11] hvm/hpet: Correctly limit period to a maximum Don Slutz
` (7 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Don Slutz @ 2014-04-17 17:42 UTC (permalink / raw)
To: xen-devel; +Cc: Keir Fraser, Don Slutz, Jan Beulich
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>
---
v3:
Only have 2 blocks of code.
Set comparator64 before truncation
xen/arch/x86/hvm/hpet.c | 36 +++++++++++++++++++++---------------
1 file changed, 21 insertions(+), 15 deletions(-)
diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 4fd6470..910d87d 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -401,20 +401,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:
@@ -426,7 +414,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;
+ /* truncate if timer is in 32 bit mode */
+ if ( timer_is_32bit(h, tn) )
+ new_val = (uint32_t)new_val;
+ h->hpet.timers[tn].cmp = new_val;
+ }
if ( hpet_enabled(h) && timer_enabled(h, tn) )
set_restart_timer(tn);
break;
--
1.8.4
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 04/11] hvm/hpet: Correctly limit period to a maximum.
2014-04-17 17:42 [PATCH v3 00/11] Prevent one cause of "MP-BIOS bug: 8254 timer"... message from linux Don Slutz
` (2 preceding siblings ...)
2014-04-17 17:42 ` [PATCH v3 03/11] hvm/hpet: Only set comparator or period not both Don Slutz
@ 2014-04-17 17:42 ` Don Slutz
2014-04-23 15:11 ` Jan Beulich
2014-04-17 17:42 ` [PATCH v3 05/11] hvm/hpet: In hpet_save, correctly compute mc64 Don Slutz
` (6 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Don Slutz @ 2014-04-17 17:42 UTC (permalink / raw)
To: xen-devel; +Cc: Keir Fraser, Don Slutz, Jan Beulich
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 | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 910d87d..c64c547 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -404,6 +404,8 @@ static int hpet_write(
if ( timer_is_periodic(h, tn) &&
!(h->hpet.timers[tn].config & HPET_TN_SETVAL) )
{
+ 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
@@ -411,7 +413,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;
}
else
--
1.8.4
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 05/11] hvm/hpet: In hpet_save, correctly compute mc64.
2014-04-17 17:42 [PATCH v3 00/11] Prevent one cause of "MP-BIOS bug: 8254 timer"... message from linux Don Slutz
` (3 preceding siblings ...)
2014-04-17 17:42 ` [PATCH v3 04/11] hvm/hpet: Correctly limit period to a maximum Don Slutz
@ 2014-04-17 17:42 ` Don Slutz
2014-04-23 15:12 ` Jan Beulich
2014-04-17 17:43 ` [PATCH v3 06/11] hvm/hpet: In hpet_save, call hpet_get_comparator Don Slutz
` (5 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Don Slutz @ 2014-04-17 17:42 UTC (permalink / raw)
To: xen-devel; +Cc: Keir Fraser, Don Slutz, Jan Beulich
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 c64c547..0789947 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -499,7 +499,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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 06/11] hvm/hpet: In hpet_save, call hpet_get_comparator.
2014-04-17 17:42 [PATCH v3 00/11] Prevent one cause of "MP-BIOS bug: 8254 timer"... message from linux Don Slutz
` (4 preceding siblings ...)
2014-04-17 17:42 ` [PATCH v3 05/11] hvm/hpet: In hpet_save, correctly compute mc64 Don Slutz
@ 2014-04-17 17:43 ` Don Slutz
2014-04-23 15:21 ` Jan Beulich
2014-04-17 17:43 ` [PATCH v3 07/11] hvm/hpet: Init comparator64 like comparator Don Slutz
` (4 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Don Slutz @ 2014-04-17 17:43 UTC (permalink / raw)
To: xen-devel; +Cc: Keir Fraser, Don Slutz, Jan Beulich
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. And hpet_get_comparator() is
effectivly called in hpet_load via hpet_set_timer.
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.
The software-developers-hpet-spec-1-0a.pdf says that the comparator
will allways be greater than master clock for a periodic timer.
Signed-off-by: Don Slutz <dslutz@verizon.com>
---
v3:
Better commit message.
Reword subject from "hvm/hpet: Call hpet_get_comparator during hpet_save."
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 0789947..fccc77c 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -495,6 +495,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);
@@ -524,6 +525,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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 07/11] hvm/hpet: Init comparator64 like comparator.
2014-04-17 17:42 [PATCH v3 00/11] Prevent one cause of "MP-BIOS bug: 8254 timer"... message from linux Don Slutz
` (5 preceding siblings ...)
2014-04-17 17:43 ` [PATCH v3 06/11] hvm/hpet: In hpet_save, call hpet_get_comparator Don Slutz
@ 2014-04-17 17:43 ` Don Slutz
2014-04-23 15:23 ` Jan Beulich
2014-04-17 17:43 ` [PATCH v3 08/11] hvm/hpet: Use signed divide in hpet_get_comparator Don Slutz
` (3 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Don Slutz @ 2014-04-17 17:43 UTC (permalink / raw)
To: xen-devel; +Cc: Keir Fraser, Don Slutz, Jan Beulich
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 fccc77c..7964df2 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -631,6 +631,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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 08/11] hvm/hpet: Use signed divide in hpet_get_comparator.
2014-04-17 17:42 [PATCH v3 00/11] Prevent one cause of "MP-BIOS bug: 8254 timer"... message from linux Don Slutz
` (6 preceding siblings ...)
2014-04-17 17:43 ` [PATCH v3 07/11] hvm/hpet: Init comparator64 like comparator Don Slutz
@ 2014-04-17 17:43 ` Don Slutz
2014-04-23 15:45 ` Jan Beulich
2014-04-17 17:43 ` [PATCH v3 09/11] hvm/hpet: comparator can only change when master clock is enabled Don Slutz
` (2 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Don Slutz @ 2014-04-17 17:43 UTC (permalink / raw)
To: xen-devel; +Cc: Keir Fraser, Don Slutz, Jan Beulich
This is done so that when comparator is less then or equal to one
period it does not change.
The code lines:
elapsed = hpet_read_maincounter(h, guest_time) +
period - 1 - comparator;
comparator += (elapsed / period) * period;
are what matter here. For almost all cases where
"hpet_read_maincounter() + period - 1" is greater then
"comparator", a signed divide will produce the same answer as an
unsigned divide. One of the problem areas is when
"hpet_read_maincounter() + period - 1" needs more then 64 bits to
represent it. It includes cases where hpet_read_maincounter() has
wrapped (I.E. needs more then 64 bits to correctly represent it),
but "comparator" has not wrapped. However "elapsed" is correctly
greater then zero as long as the mathematical result only takes 63
bits to represent it.
It is safe to declare period as a signed integer because the code
already limits it to 63 bits.
Using some numbers to help show the issue:
hpet_read_maincounter(h, guest_time) = 67752
period = 62500
comparator = 130252 == 67752 + 62500
what unsigned signed
comparator : 130252 130252
elapsed : 18446744073709551615 -1
elapsed/period : 295147905179352 0
comparator_delta : 18446744073709500000 0
new comparator : 78636 130252
Signed-off-by: Don Slutz <dslutz@verizon.com>
---
xen/arch/x86/hvm/hpet.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 7964df2..e24bc46 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -89,13 +89,13 @@ static uint64_t hpet_get_comparator(HPETState *h, unsigned int tn,
uint64_t guest_time)
{
uint64_t comparator;
- uint64_t elapsed;
+ int64_t elapsed;
comparator = h->hpet.comparator64[tn];
if ( timer_is_periodic(h, tn) )
{
/* update comparator by number of periods elapsed since last update */
- uint64_t period = h->hpet.period[tn];
+ int64_t period = h->hpet.period[tn];
if (period)
{
elapsed = hpet_read_maincounter(h, guest_time) +
--
1.8.4
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 09/11] hvm/hpet: comparator can only change when master clock is enabled.
2014-04-17 17:42 [PATCH v3 00/11] Prevent one cause of "MP-BIOS bug: 8254 timer"... message from linux Don Slutz
` (7 preceding siblings ...)
2014-04-17 17:43 ` [PATCH v3 08/11] hvm/hpet: Use signed divide in hpet_get_comparator Don Slutz
@ 2014-04-17 17:43 ` Don Slutz
2014-04-25 12:23 ` Jan Beulich
2014-04-17 17:43 ` [PATCH v3 10/11] hvm/hpet: Prevent master clock equal to comparator while enabled Don Slutz
2014-04-17 17:43 ` [PATCH v3 11/11] hvm/hpet: handle 1st period special Don Slutz
10 siblings, 1 reply; 34+ messages in thread
From: Don Slutz @ 2014-04-17 17:43 UTC (permalink / raw)
To: xen-devel; +Cc: Keir Fraser, Don Slutz, Jan Beulich
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.
When the overall enable is off (the main count is halted), none of
the compare registers should change.
The code lines:
elapsed = hpet_read_maincounter(h, guest_time) +
period - 1 - comparator;
comparator += (elapsed / period) * period;
are what matter here. They will always adjust comparator to be no
more then one period away.
Using some numbers to help show the issue:
hpet_read_maincounter(h, guest_time) = 67752
period = 62500
comparator = 255252 == 67752 + 3 * 62500
comparator : 255252
elapsed : -125001
elapsed/period : -2
comparator_delta : -125000
new comparator : 130252
Signed-off-by: Don Slutz <dslutz@verizon.com>
---
v3:
Better commit message.
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 e24bc46..6aa6e9b 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -92,7 +92,7 @@ static uint64_t hpet_get_comparator(HPETState *h, unsigned int tn,
int64_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 */
int64_t period = h->hpet.period[tn];
--
1.8.4
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 10/11] hvm/hpet: Prevent master clock equal to comparator while enabled
2014-04-17 17:42 [PATCH v3 00/11] Prevent one cause of "MP-BIOS bug: 8254 timer"... message from linux Don Slutz
` (8 preceding siblings ...)
2014-04-17 17:43 ` [PATCH v3 09/11] hvm/hpet: comparator can only change when master clock is enabled Don Slutz
@ 2014-04-17 17:43 ` Don Slutz
2014-04-25 12:25 ` Jan Beulich
2014-04-17 17:43 ` [PATCH v3 11/11] hvm/hpet: handle 1st period special Don Slutz
10 siblings, 1 reply; 34+ messages in thread
From: Don Slutz @ 2014-04-17 17:43 UTC (permalink / raw)
To: xen-devel; +Cc: Keir Fraser, Don Slutz, Jan Beulich
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.
The code lines:
elapsed = hpet_read_maincounter(h, guest_time) +
period - 1 - comparator;
comparator += (elapsed / period) * period;
are what matter here.
Using some numbers to help show the issue:
hpet_read_maincounter(h, guest_time) = 130252
period = 62500
comparator : 130252
elapsed : 62499
elapsed/period : 0
comparator_delta : 0
new comparator : 130252
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 6aa6e9b..50047f5 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -99,7 +99,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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v3 11/11] hvm/hpet: handle 1st period special
2014-04-17 17:42 [PATCH v3 00/11] Prevent one cause of "MP-BIOS bug: 8254 timer"... message from linux Don Slutz
` (9 preceding siblings ...)
2014-04-17 17:43 ` [PATCH v3 10/11] hvm/hpet: Prevent master clock equal to comparator while enabled Don Slutz
@ 2014-04-17 17:43 ` Don Slutz
2014-04-25 12:32 ` Jan Beulich
10 siblings, 1 reply; 34+ messages in thread
From: Don Slutz @ 2014-04-17 17:43 UTC (permalink / raw)
To: xen-devel; +Cc: Keir Fraser, Don Slutz, Jan Beulich
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.
The code lines:
elapsed = mc + period - comparator;
comparator += (elapsed / period) * period;
are what matter here.
Using some numbers to help show the issue:
mc = 67752
period = 62500
comparator : 380252
elapsed : -250000
elapsed/period : -4
comparator_delta : -250000
new comparator : 130252
So this code needs to be skipped on the 1st period.
The change "hvm/hpet: In hpet_save, call hpet_get_comparator." that
previously was "technically not required", is now required. Because
hpet_load now handles 1st period differently, the hpet_save needs to
provide the data as the software-developers-hpet-spec-1-0a.pdf
implies.
Signed-off-by: Don Slutz <dslutz@verizon.com>
---
v3:
Better commit message.
More setting of first_mc64 & first_enabled when needed.
Switch to bool_t.
xen/arch/x86/hvm/hpet.c | 83 ++++++++++++++++++++++++++++++++++++-------
xen/include/asm-x86/hvm/vpt.h | 2 ++
2 files changed, 73 insertions(+), 12 deletions(-)
diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 50047f5..eddc6a0 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -96,10 +96,18 @@ static uint64_t hpet_get_comparator(HPETState *h, unsigned int tn,
{
/* update comparator by number of periods elapsed since last update */
int64_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; /* skip update */
+ 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;
}
@@ -208,12 +216,18 @@ 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)
+{
+ bool_t *first_enabled_p = (bool_t *)priv;
+
+ *first_enabled_p = 0;
+}
+
static void hpet_set_timer(HPETState *h, unsigned int tn,
uint64_t guest_time)
{
uint64_t tn_cmp, cur_tick, diff;
unsigned int irq;
- unsigned int oneshot;
ASSERT(tn < HPET_TIMER_NUM);
ASSERT(spin_is_locked(&h->lock));
@@ -262,15 +276,34 @@ static void hpet_set_timer(HPETState *h, unsigned int tn,
* have elapsed between the time the comparator was written and the timer
* being enabled (now).
*/
- oneshot = !timer_is_periodic(h, tn);
- TRACE_2_LONG_4D(TRC_HVM_EMUL_HPET_START_TIMER, tn, irq,
- TRC_PAR_LONG(hpet_tick_to_ns(h, diff)),
- TRC_PAR_LONG(oneshot ? 0LL :
- hpet_tick_to_ns(h, h->hpet.period[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 ( !timer_is_periodic(h, tn) )
+ {
+ TRACE_2_LONG_4D(TRC_HVM_EMUL_HPET_START_TIMER, tn, irq,
+ TRC_PAR_LONG(hpet_tick_to_ns(h, diff)),
+ TRC_PAR_LONG(0LL));
+ create_periodic_time(vhpet_vcpu(h), &h->pt[tn],
+ hpet_tick_to_ns(h, diff),
+ 0, irq, NULL, NULL);
+ }
+ else
+ {
+ TRACE_2_LONG_4D(TRC_HVM_EMUL_HPET_START_TIMER, tn, irq,
+ TRC_PAR_LONG(hpet_tick_to_ns(h, diff)),
+ TRC_PAR_LONG(
+ hpet_tick_to_ns(h, h->hpet.period[tn])));
+ if ( h->hpet.first_enabled[tn] )
+ 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]);
+ 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, NULL, NULL);
+ }
}
static inline uint64_t hpet_fixup_reg(
@@ -343,6 +376,15 @@ static int hpet_write(
case HPET_COUNTER:
h->hpet.mc64 = new_val;
+ for ( i = 0; i < HPET_TIMER_NUM; i++ )
+ if ( timer_enabled(h, i) )
+ {
+ if ( timer_is_periodic(h, i) && h->hpet.period[i] )
+ {
+ h->hpet.first_mc64[i] = new_val;
+ h->hpet.first_enabled[i] = 1;
+ }
+ }
if ( hpet_enabled(h) )
{
gdprintk(XENLOG_WARNING,
@@ -416,6 +458,8 @@ static int hpet_write(
if ( new_val > max_period )
new_val = max_period;
h->hpet.period[tn] = new_val;
+ h->hpet.first_mc64[tn] = hpet_read_maincounter(h, guest_time);
+ h->hpet.first_enabled[tn] = 1;
}
else
{
@@ -435,6 +479,11 @@ static int hpet_write(
if ( timer_is_32bit(h, tn) )
new_val = (uint32_t)new_val;
h->hpet.timers[tn].cmp = new_val;
+ if ( timer_is_periodic(h, tn) && h->hpet.period[tn] )
+ {
+ h->hpet.first_mc64[tn] = hpet_read_maincounter(h, guest_time);
+ h->hpet.first_enabled[tn] = 1;
+ }
}
if ( hpet_enabled(h) && timer_enabled(h, tn) )
set_restart_timer(tn);
@@ -583,6 +632,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
@@ -595,7 +647,14 @@ 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) )
+ {
+ if ( timer_is_periodic(hp, i) && hp->hpet.period[i] )
+ {
+ hp->hpet.first_mc64[i] = hp->hpet.mc64;
+ hp->hpet.first_enabled[i] = 1;
+ }
hpet_set_timer(hp, i, guest_time);
+ }
spin_unlock(&hp->lock);
diff --git a/xen/include/asm-x86/hvm/vpt.h b/xen/include/asm-x86/hvm/vpt.h
index 41159d8..71c383d 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 */
+ bool_t first_enabled[HPET_TIMER_NUM]; /* In 1st interval */
};
typedef struct HPETState {
--
1.8.4
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [optional PATCH v3 01/11] hvm/hpet: Add manual unit test code.
2014-04-17 17:42 ` [optional PATCH v3 01/11] hvm/hpet: Add manual unit test code Don Slutz
@ 2014-04-23 14:41 ` Jan Beulich
2014-04-25 21:26 ` Don Slutz
0 siblings, 1 reply; 34+ messages in thread
From: Jan Beulich @ 2014-04-23 14:41 UTC (permalink / raw)
To: Don Slutz; +Cc: Keir Fraser, xen-devel
>>> On 17.04.14 at 19:42, <dslutz@verizon.com> wrote:
> +hpet.h: $(XEN_ROOT)/xen/include/asm-x86/hpet.h
> + cp $(XEN_ROOT)/xen/include/asm-x86/hpet.h .
> +
> +hpet.c: $(XEN_ROOT)/xen/arch/x86/hvm/hpet.c
> + sed -e "/#include/d" -e "1i#include \"emul.h\"\n" <$(XEN_ROOT)/xen/arch/x86/hvm/hpet.c >hpet.c
Just as a general remark (I'm not myself intending to review or commit
this patch): You should try to use shortcuts when possible, i.e. $< and
$@ in both above rules.
Jan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 02/11] hvm/hpet: Only call guest_time_hpet(h) one time per action.
2014-04-17 17:42 ` [PATCH v3 02/11] hvm/hpet: Only call guest_time_hpet(h) one time per action Don Slutz
@ 2014-04-23 15:07 ` Jan Beulich
2014-04-23 15:42 ` Don Slutz
0 siblings, 1 reply; 34+ messages in thread
From: Jan Beulich @ 2014-04-23 15:07 UTC (permalink / raw)
To: Don Slutz; +Cc: Keir Fraser, xen-devel
>>> On 17.04.14 at 19:42, <dslutz@verizon.com> wrote:
> v3:
> Did not add Reviewed-by (Jan Beulich) do to amount of change
> Added passing of guest_time to hpet_read64() and
> hpet_stop_timer().
But that wasn't as a result of the review of v2, was it? Especially for
the hpet_read64() case I fear this is going a little too far, since now
you call the supposedly expensive function even when the value isn't
needed. I suppose you could resolve this by passing a known-invalid
value to the function from hpet_read() (so it knows to call
guest_time_hpet() itself) and use the always-preset variant on from
hpet_write().
Otoh I admit that the only supposedly frequently use operations
ought to be the two ones needing the value, so perhaps the
overhead for the other ones is tolerable. Let's see whether others
have any opinion on this.
Apart from that I'm still fine with the patch.
Jan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 03/11] hvm/hpet: Only set comparator or period not both.
2014-04-17 17:42 ` [PATCH v3 03/11] hvm/hpet: Only set comparator or period not both Don Slutz
@ 2014-04-23 15:10 ` Jan Beulich
0 siblings, 0 replies; 34+ messages in thread
From: Jan Beulich @ 2014-04-23 15:10 UTC (permalink / raw)
To: Don Slutz; +Cc: Keir Fraser, xen-devel
>>> On 17.04.14 at 19:42, <dslutz@verizon.com> wrote:
> 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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
> v3:
> Only have 2 blocks of code.
> Set comparator64 before truncation
>
> xen/arch/x86/hvm/hpet.c | 36 +++++++++++++++++++++---------------
> 1 file changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
> index 4fd6470..910d87d 100644
> --- a/xen/arch/x86/hvm/hpet.c
> +++ b/xen/arch/x86/hvm/hpet.c
> @@ -401,20 +401,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:
> @@ -426,7 +414,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;
> + /* truncate if timer is in 32 bit mode */
> + if ( timer_is_32bit(h, tn) )
> + new_val = (uint32_t)new_val;
> + h->hpet.timers[tn].cmp = new_val;
> + }
> if ( hpet_enabled(h) && timer_enabled(h, tn) )
> set_restart_timer(tn);
> break;
> --
> 1.8.4
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 04/11] hvm/hpet: Correctly limit period to a maximum.
2014-04-17 17:42 ` [PATCH v3 04/11] hvm/hpet: Correctly limit period to a maximum Don Slutz
@ 2014-04-23 15:11 ` Jan Beulich
0 siblings, 0 replies; 34+ messages in thread
From: Jan Beulich @ 2014-04-23 15:11 UTC (permalink / raw)
To: Don Slutz; +Cc: Keir Fraser, xen-devel
>>> On 17.04.14 at 19:42, <dslutz@verizon.com> wrote:
> 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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
> xen/arch/x86/hvm/hpet.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
> index 910d87d..c64c547 100644
> --- a/xen/arch/x86/hvm/hpet.c
> +++ b/xen/arch/x86/hvm/hpet.c
> @@ -404,6 +404,8 @@ static int hpet_write(
> if ( timer_is_periodic(h, tn) &&
> !(h->hpet.timers[tn].config & HPET_TN_SETVAL) )
> {
> + 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
> @@ -411,7 +413,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;
> }
> else
> --
> 1.8.4
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 05/11] hvm/hpet: In hpet_save, correctly compute mc64.
2014-04-17 17:42 ` [PATCH v3 05/11] hvm/hpet: In hpet_save, correctly compute mc64 Don Slutz
@ 2014-04-23 15:12 ` Jan Beulich
0 siblings, 0 replies; 34+ messages in thread
From: Jan Beulich @ 2014-04-23 15:12 UTC (permalink / raw)
To: Don Slutz; +Cc: Keir Fraser, xen-devel
>>> On 17.04.14 at 19:42, <dslutz@verizon.com> wrote:
> 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>
Reviewed-by: Jan Beulich <jbeulich@suse.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 c64c547..0789947 100644
> --- a/xen/arch/x86/hvm/hpet.c
> +++ b/xen/arch/x86/hvm/hpet.c
> @@ -499,7 +499,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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 06/11] hvm/hpet: In hpet_save, call hpet_get_comparator.
2014-04-17 17:43 ` [PATCH v3 06/11] hvm/hpet: In hpet_save, call hpet_get_comparator Don Slutz
@ 2014-04-23 15:21 ` Jan Beulich
2014-04-25 21:42 ` Don Slutz
0 siblings, 1 reply; 34+ messages in thread
From: Jan Beulich @ 2014-04-23 15:21 UTC (permalink / raw)
To: Don Slutz; +Cc: Keir Fraser, xen-devel
>>> On 17.04.14 at 19:43, <dslutz@verizon.com> wrote:
> --- a/xen/arch/x86/hvm/hpet.c
> +++ b/xen/arch/x86/hvm/hpet.c
> @@ -495,6 +495,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);
>
> @@ -524,6 +525,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;
Wouldn't you be better off loading guest_time via guest_time_hpet()
right after taking the lock, using the variable also in the assignment
to mc64? I'm particularly suspecting an inconsistency in the
!hpet_enabled() case (i.e. when mc64 doesn't get updated
anymore). Iirc a later patch (in the earlier version of the series)
makes the adjustment in hpet_get_comparator() also conditional
upon the HPET being enabled, in which case the end effect would
be identical (due to hpet_get_comparator() then becoming a nop for
that case).
In any event, no matter that the immediately following comment is
malformed too, please fix your comment style (and feel free to
adjust the one below at once).
> + 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];
Jan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 07/11] hvm/hpet: Init comparator64 like comparator.
2014-04-17 17:43 ` [PATCH v3 07/11] hvm/hpet: Init comparator64 like comparator Don Slutz
@ 2014-04-23 15:23 ` Jan Beulich
2014-04-25 22:00 ` Don Slutz
0 siblings, 1 reply; 34+ messages in thread
From: Jan Beulich @ 2014-04-23 15:23 UTC (permalink / raw)
To: Don Slutz; +Cc: Keir Fraser, xen-devel
>>> On 17.04.14 at 19:43, <dslutz@verizon.com> wrote:
> 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.
But the immediately preceding patch arranged for the field to get
suitably updated, so is this change really doing anything but
cosmetics?
Jan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 02/11] hvm/hpet: Only call guest_time_hpet(h) one time per action.
2014-04-23 15:07 ` Jan Beulich
@ 2014-04-23 15:42 ` Don Slutz
2014-04-23 15:54 ` Jan Beulich
0 siblings, 1 reply; 34+ messages in thread
From: Don Slutz @ 2014-04-23 15:42 UTC (permalink / raw)
To: Jan Beulich, Don Slutz; +Cc: Keir Fraser, xen-devel
On 4/23/2014 11:07 AM, Jan Beulich wrote:
>>>> On 17.04.14 at 19:42, <dslutz@verizon.com> wrote:
>> v3:
>> Did not add Reviewed-by (Jan Beulich) do to amount of change
>> Added passing of guest_time to hpet_read64() and
>> hpet_stop_timer().
> But that wasn't as a result of the review of v2, was it?
Not on this thread. I was using gdb to get answers for a later patch.
> Especially for
> the hpet_read64() case I fear this is going a little too far, since now
> you call the supposedly expensive function even when the value isn't
> needed. I suppose you could resolve this by passing a known-invalid
> value to the function from hpet_read() (so it knows to call
> guest_time_hpet() itself) and use the always-preset variant on from
> hpet_write().
I also considered a case statement on the call (since the value is
needed later when
hpet_write also uses it). I do not think that any value is invalid.
>
> Otoh I admit that the only supposedly frequently use operations
> ought to be the two ones needing the value, so perhaps the
> overhead for the other ones is tolerable. Let's see whether others
> have any opinion on this.
I also expect that the frequently used operations are the ones that need
it. And it
is the lower risk of add a bug. Will wait for any other comments.
-Don Slutz
> Apart from that I'm still fine with the patch.
>
> Jan
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 08/11] hvm/hpet: Use signed divide in hpet_get_comparator.
2014-04-17 17:43 ` [PATCH v3 08/11] hvm/hpet: Use signed divide in hpet_get_comparator Don Slutz
@ 2014-04-23 15:45 ` Jan Beulich
2014-04-26 1:52 ` Slutz, Donald Christopher
0 siblings, 1 reply; 34+ messages in thread
From: Jan Beulich @ 2014-04-23 15:45 UTC (permalink / raw)
To: Don Slutz; +Cc: Keir Fraser, xen-devel
>>> On 17.04.14 at 19:43, <dslutz@verizon.com> wrote:
> This is done so that when comparator is less then or equal to one
> period it does not change.
>
> The code lines:
>
> elapsed = hpet_read_maincounter(h, guest_time) +
> period - 1 - comparator;
> comparator += (elapsed / period) * period;
>
> are what matter here. For almost all cases where
> "hpet_read_maincounter() + period - 1" is greater then
> "comparator", a signed divide will produce the same answer as an
> unsigned divide. One of the problem areas is when
> "hpet_read_maincounter() + period - 1" needs more then 64 bits to
> represent it. It includes cases where hpet_read_maincounter() has
"It includes" implies there are other cases, yet I can't see any.
(Also twice s/then/than/ I think.)
> wrapped (I.E. needs more then 64 bits to correctly represent it),
> but "comparator" has not wrapped.
But can this happen in practice? Main counter and comparator
shouldn't be very far apart, and hence other than
"hpet_read_maincounter() + period - 1",
"hpet_read_maincounter() + period - 1 - comparator" shouldn't
ever require more than 64 bits to correctly represent it. All you
really need to deal with is the case where
hpet_read_maincounter() + period == comparator, i.e. the 1
being subtracted makes the expression degenerate. So you could
simply make the computation and update dependent upon
hpet_read_maincounter() + period > comparator.
And then again I don't really see why the subtraction of 1 is needed
here in the first place.
Jan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 02/11] hvm/hpet: Only call guest_time_hpet(h) one time per action.
2014-04-23 15:42 ` Don Slutz
@ 2014-04-23 15:54 ` Jan Beulich
0 siblings, 0 replies; 34+ messages in thread
From: Jan Beulich @ 2014-04-23 15:54 UTC (permalink / raw)
To: Don Slutz; +Cc: Keir Fraser, xen-devel
>>> On 23.04.14 at 17:42, <dslutz@verizon.com> wrote:
> On 4/23/2014 11:07 AM, Jan Beulich wrote:
>>>>> On 17.04.14 at 19:42, <dslutz@verizon.com> wrote:
>>> v3:
>>> Did not add Reviewed-by (Jan Beulich) do to amount of change
>>> Added passing of guest_time to hpet_read64() and
>>> hpet_stop_timer().
>> But that wasn't as a result of the review of v2, was it?
>
> Not on this thread. I was using gdb to get answers for a later patch.
>
>> Especially for
>> the hpet_read64() case I fear this is going a little too far, since now
>> you call the supposedly expensive function even when the value isn't
>> needed. I suppose you could resolve this by passing a known-invalid
>> value to the function from hpet_read() (so it knows to call
>> guest_time_hpet() itself) and use the always-preset variant on from
>> hpet_write().
>
> I also considered a case statement on the call (since the value is
> needed later when
> hpet_write also uses it). I do not think that any value is invalid.
With
#define STIME_PER_HPET_TICK 16
#define guest_time_hpet(hpet) \
(hvm_get_guest_time(vhpet_vcpu(hpet)) / STIME_PER_HPET_TICK)
any value with one or more of the high 4 bits set would be invalid.
Jan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 09/11] hvm/hpet: comparator can only change when master clock is enabled.
2014-04-17 17:43 ` [PATCH v3 09/11] hvm/hpet: comparator can only change when master clock is enabled Don Slutz
@ 2014-04-25 12:23 ` Jan Beulich
0 siblings, 0 replies; 34+ messages in thread
From: Jan Beulich @ 2014-04-25 12:23 UTC (permalink / raw)
To: Don Slutz; +Cc: Keir Fraser, xen-devel
>>> On 17.04.14 at 19:43, <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.
>
> When the overall enable is off (the main count is halted), none of
> the compare registers should change.
>
> The code lines:
>
> elapsed = hpet_read_maincounter(h, guest_time) +
> period - 1 - comparator;
> comparator += (elapsed / period) * period;
>
> are what matter here. They will always adjust comparator to be no
> more then one period away.
>
> Using some numbers to help show the issue:
>
> hpet_read_maincounter(h, guest_time) = 67752
> period = 62500
> comparator = 255252 == 67752 + 3 * 62500
>
> comparator : 255252
> elapsed : -125001
> elapsed/period : -2
> comparator_delta : -125000
> new comparator : 130252
>
> Signed-off-by: Don Slutz <dslutz@verizon.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
> ---
> v3:
> Better commit message.
>
> 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 e24bc46..6aa6e9b 100644
> --- a/xen/arch/x86/hvm/hpet.c
> +++ b/xen/arch/x86/hvm/hpet.c
> @@ -92,7 +92,7 @@ static uint64_t hpet_get_comparator(HPETState *h, unsigned
> int tn,
> int64_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
> */
> int64_t period = h->hpet.period[tn];
> --
> 1.8.4
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 10/11] hvm/hpet: Prevent master clock equal to comparator while enabled
2014-04-17 17:43 ` [PATCH v3 10/11] hvm/hpet: Prevent master clock equal to comparator while enabled Don Slutz
@ 2014-04-25 12:25 ` Jan Beulich
2014-04-26 1:50 ` Slutz, Donald Christopher
0 siblings, 1 reply; 34+ messages in thread
From: Jan Beulich @ 2014-04-25 12:25 UTC (permalink / raw)
To: Don Slutz; +Cc: Keir Fraser, xen-devel
>>> On 17.04.14 at 19:43, <dslutz@verizon.com> wrote:
> 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.
>
> The code lines:
>
> elapsed = hpet_read_maincounter(h, guest_time) +
> period - 1 - comparator;
> comparator += (elapsed / period) * period;
>
> are what matter here.
So this is basically answering the question I raised on an earlier
patch, and I think it implies that this earlier patch is then pointless
(or should likely be folded with this one).
Jan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 11/11] hvm/hpet: handle 1st period special
2014-04-17 17:43 ` [PATCH v3 11/11] hvm/hpet: handle 1st period special Don Slutz
@ 2014-04-25 12:32 ` Jan Beulich
2014-04-26 14:10 ` Slutz, Donald Christopher
2014-05-01 10:31 ` Tim Deegan
0 siblings, 2 replies; 34+ messages in thread
From: Jan Beulich @ 2014-04-25 12:32 UTC (permalink / raw)
To: Don Slutz; +Cc: Keir Fraser, xen-devel
>>> On 17.04.14 at 19:43, <dslutz@verizon.com> wrote:
> v3:
> Better commit message.
> More setting of first_mc64 & first_enabled when needed.
> Switch to bool_t.
>
> xen/arch/x86/hvm/hpet.c | 83 ++++++++++++++++++++++++++++++++++++-------
> xen/include/asm-x86/hvm/vpt.h | 2 ++
> 2 files changed, 73 insertions(+), 12 deletions(-)
I still can't help myself thinking that this must be achievable with less
special casing - I just can't imagine that hardware also has this huge
an amount of special case logic just for the first period.
> @@ -208,12 +216,18 @@ 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)
> +{
> + bool_t *first_enabled_p = (bool_t *)priv;
Pointless cast.
Jan
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [optional PATCH v3 01/11] hvm/hpet: Add manual unit test code.
2014-04-23 14:41 ` Jan Beulich
@ 2014-04-25 21:26 ` Don Slutz
0 siblings, 0 replies; 34+ messages in thread
From: Don Slutz @ 2014-04-25 21:26 UTC (permalink / raw)
To: Jan Beulich, Don Slutz; +Cc: Keir Fraser, xen-devel
On 04/23/14 10:41, Jan Beulich wrote:
>>>> On 17.04.14 at 19:42, <dslutz@verizon.com> wrote:
>> +hpet.h: $(XEN_ROOT)/xen/include/asm-x86/hpet.h
>> + cp $(XEN_ROOT)/xen/include/asm-x86/hpet.h .
>> +
>> +hpet.c: $(XEN_ROOT)/xen/arch/x86/hvm/hpet.c
>> + sed -e "/#include/d" -e "1i#include \"emul.h\"\n" <$(XEN_ROOT)/xen/arch/x86/hvm/hpet.c >hpet.c
> Just as a general remark (I'm not myself intending to review or commit
> this patch): You should try to use shortcuts when possible, i.e. $< and
> $@ in both above rules.
Ok
-Don Slutz
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 06/11] hvm/hpet: In hpet_save, call hpet_get_comparator.
2014-04-23 15:21 ` Jan Beulich
@ 2014-04-25 21:42 ` Don Slutz
0 siblings, 0 replies; 34+ messages in thread
From: Don Slutz @ 2014-04-25 21:42 UTC (permalink / raw)
To: Jan Beulich, Don Slutz; +Cc: Keir Fraser, xen-devel
On 04/23/14 11:21, Jan Beulich wrote:
>>>> On 17.04.14 at 19:43, <dslutz@verizon.com> wrote:
>> --- a/xen/arch/x86/hvm/hpet.c
>> +++ b/xen/arch/x86/hvm/hpet.c
>> @@ -495,6 +495,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);
>>
>> @@ -524,6 +525,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;
> Wouldn't you be better off loading guest_time via guest_time_hpet()
> right after taking the lock, using the variable also in the assignment
> to mc64? I'm particularly suspecting an inconsistency in the
> !hpet_enabled() case (i.e. when mc64 doesn't get updated
> anymore). Iirc a later patch (in the earlier version of the series)
> makes the adjustment in hpet_get_comparator() also conditional
> upon the HPET being enabled, in which case the end effect would
> be identical (due to hpet_get_comparator() then becoming a nop for
> that case).
You are right.
> In any event, no matter that the immediately following comment is
> malformed too, please fix your comment style (and feel free to
> adjust the one below at once).
Will do.
-Don Slutz
>> + 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];
> Jan
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 07/11] hvm/hpet: Init comparator64 like comparator.
2014-04-23 15:23 ` Jan Beulich
@ 2014-04-25 22:00 ` Don Slutz
0 siblings, 0 replies; 34+ messages in thread
From: Don Slutz @ 2014-04-25 22:00 UTC (permalink / raw)
To: Jan Beulich, Don Slutz; +Cc: Keir Fraser, xen-devel
On 04/23/14 11:23, Jan Beulich wrote:
>>>> On 17.04.14 at 19:43, <dslutz@verizon.com> wrote:
>> 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.
> But the immediately preceding patch arranged for the field to get
> suitably updated, so is this change really doing anything but
> cosmetics?
The preceding patch only makes a difference when the hpet is enabled. A
disabled
hpet (like when a guest is starting), should start with the value the
spec says.
Both hpet_save and the guest reading the comparator register use
comparator64
to get the value. So instead of just adding the line with comparator64,
I could also
delete the setting of .cmp. I picked the simpler one (just add the init).
So either way, not doing this change and for example using xen-hvmctx on
a guest
that was started paused, you see all 0 not all 1.
-Don Slutz
> Jan
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 10/11] hvm/hpet: Prevent master clock equal to comparator while enabled
2014-04-25 12:25 ` Jan Beulich
@ 2014-04-26 1:50 ` Slutz, Donald Christopher
0 siblings, 0 replies; 34+ messages in thread
From: Slutz, Donald Christopher @ 2014-04-26 1:50 UTC (permalink / raw)
To: Jan Beulich, Slutz, Donald Christopher; +Cc: Keir Fraser, xen-devel
On 04/25/14 08:25, Jan Beulich wrote:
>>>> On 17.04.14 at 19:43,<dslutz@verizon.com> wrote:
>> 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.
>>
>> The code lines:
>>
>> elapsed = hpet_read_maincounter(h, guest_time) +
>> period - 1 - comparator;
>> comparator += (elapsed / period) * period;
>>
>> are what matter here.
> So this is basically answering the question I raised on an earlier
> patch, and I think it implies that this earlier patch is then pointless
> (or should likely be folded with this one).
Yes, the -1 is wrong.
After more thinking on it, the last patch (#11) does make the previous
patch (#8) not needed.
Looks like I will be dropping patch #8 "hvm/hpet: Use signed divide in
hpet_get_comparator."
as not needed.
-Don Slutz
> Jan
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 08/11] hvm/hpet: Use signed divide in hpet_get_comparator.
2014-04-23 15:45 ` Jan Beulich
@ 2014-04-26 1:52 ` Slutz, Donald Christopher
0 siblings, 0 replies; 34+ messages in thread
From: Slutz, Donald Christopher @ 2014-04-26 1:52 UTC (permalink / raw)
To: Jan Beulich; +Cc: Keir Fraser, xen-devel
On 04/23/14 11:45, Jan Beulich wrote:
>>>> On 17.04.14 at 19:43, <dslutz@verizon.com> wrote:
>> This is done so that when comparator is less then or equal to one
>> period it does not change.
>>
>> The code lines:
>>
>> elapsed = hpet_read_maincounter(h, guest_time) +
>> period - 1 - comparator;
>> comparator += (elapsed / period) * period;
>>
>> are what matter here. For almost all cases where
>> "hpet_read_maincounter() + period - 1" is greater then
>> "comparator", a signed divide will produce the same answer as an
>> unsigned divide. One of the problem areas is when
>> "hpet_read_maincounter() + period - 1" needs more then 64 bits to
>> represent it. It includes cases where hpet_read_maincounter() has
> "It includes" implies there are other cases, yet I can't see any.
> (Also twice s/then/than/ I think.)
The boundary cases of 2's complement show up here (8 bits is just
nice small numbers: -128 (signed) vs 128 (unsigned)).
Ok.
>> wrapped (I.E. needs more then 64 bits to correctly represent it),
>> but "comparator" has not wrapped.
> But can this happen in practice? Main counter and comparator
> shouldn't be very far apart, and hence other than
> "hpet_read_maincounter() + period - 1",
> "hpet_read_maincounter() + period - 1 - comparator" shouldn't
> ever require more than 64 bits to correctly represent it. All you
> really need to deal with is the case where
> hpet_read_maincounter() + period == comparator, i.e. the 1
> being subtracted makes the expression degenerate. So you could
> simply make the computation and update dependent upon
> hpet_read_maincounter() + period > comparator.
> And then again I don't really see why the subtraction of 1 is needed
> here in the first place.
As you saw in patch #10, the -1 is an issue.
Doing patch #10 1st, removes the normal Linux case.
After more thinking on it, the last patch (#11) does make this patch
(#8) not needed. The case
that is overlapping is:
If the 1st period's cmp is > 1 period by less then 2 periods (using
numbers from #8 adjusted):
hpet_read_maincounter(h, guest_time) = 67752
period = 62500
comparator = 130253 == 67752 + 62500 + 1
what unsigned signed
comparator : 130253 130253
elapsed : 18446744073709551615 -1
elapsed/period : 295147905179352 0
comparator_delta : 18446744073709500000 0
new comparator : 78637 130253
and
hpet_read_maincounter(h, guest_time) = 67752
period = 62500
comparator = 161502 == 67752 + 62500 + 31250
what unsigned signed
comparator : 161502 161502
elapsed : 18446744073709520366 -31250
elapsed/period : 295147905179352 0
comparator_delta : 18446744073709500000 0
new comparator : 109886 161502
It does the right thing, but does not handle the general case.
But since the last patch also handles this case, I will be dropping this
patch in v4.
-Don Slutz
>
> Jan
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 11/11] hvm/hpet: handle 1st period special
2014-04-25 12:32 ` Jan Beulich
@ 2014-04-26 14:10 ` Slutz, Donald Christopher
2014-05-01 10:31 ` Tim Deegan
1 sibling, 0 replies; 34+ messages in thread
From: Slutz, Donald Christopher @ 2014-04-26 14:10 UTC (permalink / raw)
To: Jan Beulich; +Cc: Keir Fraser, xen-devel
On 04/25/14 08:32, Jan Beulich wrote:
>>>> On 17.04.14 at 19:43, <dslutz@verizon.com> wrote:
>> v3:
>> Better commit message.
>> More setting of first_mc64 & first_enabled when needed.
>> Switch to bool_t.
>>
>> xen/arch/x86/hvm/hpet.c | 83 ++++++++++++++++++++++++++++++++++++-------
>> xen/include/asm-x86/hvm/vpt.h | 2 ++
>> 2 files changed, 73 insertions(+), 12 deletions(-)
> I still can't help myself thinking that this must be achievable with less
> special casing - I just can't imagine that hardware also has this huge
> an amount of special case logic just for the first period.
The hardware is simple. And the code to simulate it:
do_count()
{
mc++;
if ( mc == comparator )
handle_int();
if ( periodic )
comparator += period;
}
}
is also simple. However this way has a very high cost in that you have
to call do_count at the frequency of the master clock. Instead of the
simple simulation, the current code uses arithmetic to compute the
current state of mc and comparator. This is where the complexity comes
in. During answering of comments on #8 (signed divide) and #10
(prevent mc equal), I noticed that what I coded here only works for
expected case where mc is close to zero and comparator starts
out > mc in unsigned. I will be sending a v4 with this issue fixed.
New code (untested):
+ if ( h->hpet.first_enabled[tn] )
+ {
+ int64_t mc_diff = mc - h->hpet.first_mc64[tn];
+
+ if ( mc_diff >= 0 && mc_diff < comparator - h->hpet.first_mc64[tn] )
was:
+ if ( mc >= h->hpet.first_mc64[tn] && mc < comparator )
>> @@ -208,12 +216,18 @@ 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)
>> +{
>> + bool_t *first_enabled_p = (bool_t *)priv;
> Pointless cast.
Will drop.
-Don Slutz
> Jan
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 11/11] hvm/hpet: handle 1st period special
2014-04-25 12:32 ` Jan Beulich
2014-04-26 14:10 ` Slutz, Donald Christopher
@ 2014-05-01 10:31 ` Tim Deegan
2014-05-01 20:19 ` Don Slutz
1 sibling, 1 reply; 34+ messages in thread
From: Tim Deegan @ 2014-05-01 10:31 UTC (permalink / raw)
To: Jan Beulich; +Cc: Keir Fraser, Don Slutz, xen-devel
At 13:32 +0100 on 25 Apr (1398429157), Jan Beulich wrote:
> >>> On 17.04.14 at 19:43, <dslutz@verizon.com> wrote:
> > v3:
> > Better commit message.
> > More setting of first_mc64 & first_enabled when needed.
> > Switch to bool_t.
> >
> > xen/arch/x86/hvm/hpet.c | 83 ++++++++++++++++++++++++++++++++++++-------
> > xen/include/asm-x86/hvm/vpt.h | 2 ++
> > 2 files changed, 73 insertions(+), 12 deletions(-)
>
> I still can't help myself thinking that this must be achievable with less
> special casing - I just can't imagine that hardware also has this huge
> an amount of special case logic just for the first period.
+1. I didn't follow the explanation on v1 very clearly, so perhaps
you can correct my understanding, but I _think_ the issue you're
describing is:
when we read the comparator, we try to wind it forward from its
current value towards the main counter value, by adding as many
periods as will fit.
If the guest sets the comparator >1 period away, we'll incorrectly
warp the comparator to some foolish value as soon as we read it.
So, you're adding mechanism to detect the first interrupt after a
comparator set (or when the main counter is being started, or on hpet
load -- both of those rely on 'first_enabled' being essentialy
harmless if set when it's _not_ the first interrupt, which suggests
that maybe that's not the best name for the flag).
Couldn't it be fixed more simply by detecting comparator values in the
past in hpet_get_comparator()?
Tim.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 11/11] hvm/hpet: handle 1st period special
2014-05-01 10:31 ` Tim Deegan
@ 2014-05-01 20:19 ` Don Slutz
2014-05-02 13:19 ` Tim Deegan
0 siblings, 1 reply; 34+ messages in thread
From: Don Slutz @ 2014-05-01 20:19 UTC (permalink / raw)
To: Tim Deegan, Jan Beulich; +Cc: Keir Fraser, Don Slutz, xen-devel
On 05/01/14 06:31, Tim Deegan wrote:
> At 13:32 +0100 on 25 Apr (1398429157), Jan Beulich wrote:
>>>>> On 17.04.14 at 19:43, <dslutz@verizon.com> wrote:
>>> v3:
>>> Better commit message.
>>> More setting of first_mc64 & first_enabled when needed.
>>> Switch to bool_t.
>>>
>>> xen/arch/x86/hvm/hpet.c | 83 ++++++++++++++++++++++++++++++++++++-------
>>> xen/include/asm-x86/hvm/vpt.h | 2 ++
>>> 2 files changed, 73 insertions(+), 12 deletions(-)
>> I still can't help myself thinking that this must be achievable with less
>> special casing - I just can't imagine that hardware also has this huge
>> an amount of special case logic just for the first period.
> +1. I didn't follow the explanation on v1 very clearly, so perhaps
> you can correct my understanding, but I _think_ the issue you're
> describing is:
>
> when we read the comparator, we try to wind it forward from its
> current value towards the main counter value, by adding as many
> periods as will fit.
Yes.
> If the guest sets the comparator >1 period away, we'll incorrectly
> warp the comparator to some foolish value as soon as we read it.
Yes. The "foolish value" falls within the bounds of a period.
> So, you're adding mechanism to detect the first interrupt after a
> comparator set (or when the main counter is being started, or on hpet
> load -- both of those rely on 'first_enabled' being essentialy
> harmless if set when it's _not_ the first interrupt, which suggests
> that maybe that's not the best name for the flag).
It is not harmless if it is set when _not_ in the first interval (before
first interrupt). Since it is used by hpet_save during migration,
the correct values need to be saved. I do not care about the
name.
You are missing the case when the guest sets the main counter.
> Couldn't it be fixed more simply by detecting comparator values in the
> past in hpet_get_comparator()?
This statement only works using 64-bit arithmetic for the main counter never
63
changing by more then 2 . (Which is a boundary case that should not
happen in my life time.)
Without patch #11 (this patch) and without patch #8 (Use signed divide...)
and adding:
commit 8c450bed530b13c3f3d18b9dafb3d1b1d69ac1f2
Author: Don Slutz <dslutz@verizon.com>
Date: Thu May 1 14:02:47 2014 -0400
hvm/hpet: Detect comparator values in the past
Signed-off-by: Don Slutz <dslutz@verizon.com>
diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
index 4e4da20..d93dd69 100644
--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -98,10 +98,12 @@ 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, guest_time) +
- period - comparator;
- comparator += (elapsed / period) * period;
- h->hpet.comparator64[tn] = comparator;
+ elapsed = hpet_read_maincounter(h, guest_time) - comparator;
+ if ( (int64_t)elapsed >= 0 )
+ {
+ comparator += ((elapsed + period) / period) * period;
+ h->hpet.comparator64[tn] = comparator;
+ }
}
}
Which I think was what you mean.
It looks to be "ok" (ignoring the boundary case). So should
I send it as v4?
-Don Slutz
> Tim.
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v3 11/11] hvm/hpet: handle 1st period special
2014-05-01 20:19 ` Don Slutz
@ 2014-05-02 13:19 ` Tim Deegan
0 siblings, 0 replies; 34+ messages in thread
From: Tim Deegan @ 2014-05-02 13:19 UTC (permalink / raw)
To: Don Slutz; +Cc: Keir Fraser, Jan Beulich, xen-devel
At 16:19 -0400 on 01 May (1398957542), Don Slutz wrote:
> On 05/01/14 06:31, Tim Deegan wrote:
> > Couldn't it be fixed more simply by detecting comparator values in the
> > past in hpet_get_comparator()?
>
> This statement only works using 64-bit arithmetic for the main counter never
> 63
> changing by more then 2 . (Which is a boundary case that should not
> happen in my life time.)
>
>
> Without patch #11 (this patch) and without patch #8 (Use signed divide...)
> and adding:
>
>
> commit 8c450bed530b13c3f3d18b9dafb3d1b1d69ac1f2
> Author: Don Slutz <dslutz@verizon.com>
> Date: Thu May 1 14:02:47 2014 -0400
>
> hvm/hpet: Detect comparator values in the past
>
> Signed-off-by: Don Slutz <dslutz@verizon.com>
>
> diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c
> index 4e4da20..d93dd69 100644
> --- a/xen/arch/x86/hvm/hpet.c
> +++ b/xen/arch/x86/hvm/hpet.c
> @@ -98,10 +98,12 @@ 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, guest_time) +
> - period - comparator;
> - comparator += (elapsed / period) * period;
> - h->hpet.comparator64[tn] = comparator;
> + elapsed = hpet_read_maincounter(h, guest_time) - comparator;
> + if ( (int64_t)elapsed >= 0 )
> + {
> + comparator += ((elapsed + period) / period) * period;
> + h->hpet.comparator64[tn] = comparator;
> + }
> }
> }
>
>
>
> Which I think was what you mean.
>
> It looks to be "ok" (ignoring the boundary case). So should
> I send it as v4?
Yes, that looks good. You can add
Acked-by: Tim Deegan <tim@xen.org>
to it too.
Cheers,
Tim.
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2014-05-02 13:19 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-17 17:42 [PATCH v3 00/11] Prevent one cause of "MP-BIOS bug: 8254 timer"... message from linux Don Slutz
2014-04-17 17:42 ` [optional PATCH v3 01/11] hvm/hpet: Add manual unit test code Don Slutz
2014-04-23 14:41 ` Jan Beulich
2014-04-25 21:26 ` Don Slutz
2014-04-17 17:42 ` [PATCH v3 02/11] hvm/hpet: Only call guest_time_hpet(h) one time per action Don Slutz
2014-04-23 15:07 ` Jan Beulich
2014-04-23 15:42 ` Don Slutz
2014-04-23 15:54 ` Jan Beulich
2014-04-17 17:42 ` [PATCH v3 03/11] hvm/hpet: Only set comparator or period not both Don Slutz
2014-04-23 15:10 ` Jan Beulich
2014-04-17 17:42 ` [PATCH v3 04/11] hvm/hpet: Correctly limit period to a maximum Don Slutz
2014-04-23 15:11 ` Jan Beulich
2014-04-17 17:42 ` [PATCH v3 05/11] hvm/hpet: In hpet_save, correctly compute mc64 Don Slutz
2014-04-23 15:12 ` Jan Beulich
2014-04-17 17:43 ` [PATCH v3 06/11] hvm/hpet: In hpet_save, call hpet_get_comparator Don Slutz
2014-04-23 15:21 ` Jan Beulich
2014-04-25 21:42 ` Don Slutz
2014-04-17 17:43 ` [PATCH v3 07/11] hvm/hpet: Init comparator64 like comparator Don Slutz
2014-04-23 15:23 ` Jan Beulich
2014-04-25 22:00 ` Don Slutz
2014-04-17 17:43 ` [PATCH v3 08/11] hvm/hpet: Use signed divide in hpet_get_comparator Don Slutz
2014-04-23 15:45 ` Jan Beulich
2014-04-26 1:52 ` Slutz, Donald Christopher
2014-04-17 17:43 ` [PATCH v3 09/11] hvm/hpet: comparator can only change when master clock is enabled Don Slutz
2014-04-25 12:23 ` Jan Beulich
2014-04-17 17:43 ` [PATCH v3 10/11] hvm/hpet: Prevent master clock equal to comparator while enabled Don Slutz
2014-04-25 12:25 ` Jan Beulich
2014-04-26 1:50 ` Slutz, Donald Christopher
2014-04-17 17:43 ` [PATCH v3 11/11] hvm/hpet: handle 1st period special Don Slutz
2014-04-25 12:32 ` Jan Beulich
2014-04-26 14:10 ` Slutz, Donald Christopher
2014-05-01 10:31 ` Tim Deegan
2014-05-01 20:19 ` Don Slutz
2014-05-02 13:19 ` Tim Deegan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.