[PATCH 0/2][RFC] Completing HPET in KVM (v7)

[PATCH 0/2][RFC] Completing HPET in KVM (v7)

[Beth Kon]

There is a problem in the latest git with savevm (it aborts). So I've 
been unable to test savevm with these patches, but am submitting them RFC. 
Everything else has been tested, including compatibility testing between
old/new kernel/userspace combinations.

[PATCH 1/2][RFC] Userspace changes for KVM HPET (v7)

[Beth Kon]

This patch series must be applied on top of the hpet branch. 

The big change here is handling of enabling/disabling of hpet legacy mode. When hpet enters
legacy mode, the spec says that the pit stops generating interrupts. In practice, we want to 
stop the pit periodic timer from running because it is wasteful in a virtual environment. 

We also have to worry about the hpet leaving legacy mode (which, at least in linux, happens
only during a shutdown or crash). At this point, according to the hpet spec, PIT interrupts
need to be reenabled. For us, it means the PIT timer needs to be restarted.  

This patch handles this situation better than the earlier versions by coming closer to 
just disabling PIT interrupts. It allows the PIT state to change if the OS modifies it,
even while PIT is disabled, but does not allow a pit timer to start. Then if HPET
legacy mode is disabled, whatever the PIT state is at that point, the PIT timer is 
restarted accordingly.

Changes from v6:

- added ioctl interface for setting hpet legacy mode in kernel pit
- moved check for hpet_legacy_mode in pit_load_count to allow state info
  to be copied before returning if legacy mode is enabled. 
- sprinkled in some #ifdef TARGET_I386
 

Signed-off-by: Beth Kon <eak@us.ibm.com>
diff --git a/hw/hpet.c b/hw/hpet.c
index 29db325..2f5255f 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -206,6 +206,9 @@ static int hpet_load(QEMUFile *f, void *opaque, int version_id)
             qemu_get_timer(f, s->timer[i].qemu_timer);
         }
     }
+    if (hpet_in_legacy_mode()) {
+        hpet_disable_pit();
+    }
     return 0;
 }
 
@@ -475,9 +478,11 @@ static void hpet_ram_writel(void *opaque, target_phys_addr_t addr,
                 }
                 /* i8254 and RTC are disabled when HPET is in legacy mode */
                 if (activating_bit(old_val, new_val, HPET_CFG_LEGACY)) {
-                    hpet_pit_disable();
+                    hpet_disable_pit();
+                    dprintf("qemu: hpet disabled pit\n");
                 } else if (deactivating_bit(old_val, new_val, HPET_CFG_LEGACY)) {
-                    hpet_pit_enable();
+                    hpet_enable_pit();
+                    dprintf("qemu: hpet enabled pit\n");
                 }
                 break;
             case HPET_CFG + 4:
@@ -554,13 +559,16 @@ static void hpet_reset(void *opaque) {
     /* 64-bit main counter; 3 timers supported; LegacyReplacementRoute. */
     s->capability = 0x8086a201ULL;
     s->capability |= ((HPET_CLK_PERIOD) << 32);
-    if (count > 0)
+    s->config = 0ULL;
+    if (count > 0) {
         /* we don't enable pit when hpet_reset is first called (by hpet_init)
          * because hpet is taking over for pit here. On subsequent invocations,
          * hpet_reset is called due to system reset. At this point control must
          * be returned to pit until SW reenables hpet.
          */
-        hpet_pit_enable();
+        hpet_enable_pit();
+        dprintf("qemu: hpet enabled pit\n");
+    }
     count = 1;
 }
 
diff --git a/hw/i8254-kvm.c b/hw/i8254-kvm.c
index 8390d75..76ce6f2 100644
--- a/hw/i8254-kvm.c
+++ b/hw/i8254-kvm.c
@@ -36,6 +36,7 @@ static void kvm_pit_save(QEMUFile *f, void *opaque)
     struct kvm_pit_state pit;
     struct kvm_pit_channel_state *c;
     struct PITChannelState *sc;
+    __u8 hpet_legacy_mode;
     int i;
 
     kvm_get_pit(kvm_context, &pit);
@@ -59,6 +60,10 @@ static void kvm_pit_save(QEMUFile *f, void *opaque)
     }
 
     pit_save(f, s);
+    if (kvm_has_hpet_legacy_mode(kvm_context)) {
+        kvm_get_hpet_legacy_mode(kvm_context, &hpet_legacy_mode);
+        qemu_put_8s(f, &hpet_legacy_mode);
+    }
 }
 
 static int kvm_pit_load(QEMUFile *f, void *opaque, int version_id)
@@ -67,6 +72,7 @@ static int kvm_pit_load(QEMUFile *f, void *opaque, int version_id)
     struct kvm_pit_state pit;
     struct kvm_pit_channel_state *c;
     struct PITChannelState *sc;
+    __u8 hpet_legacy_mode;
     int i;
 
     pit_load(f, s, version_id);
@@ -89,8 +95,13 @@ static int kvm_pit_load(QEMUFile *f, void *opaque, int version_id)
 	c->count_load_time = sc->count_load_time;
     }
 
-    kvm_set_pit(kvm_context, &pit);
+    if (kvm_has_hpet_legacy_mode(kvm_context)) {
+        qemu_get_8s(f, &hpet_legacy_mode);
+        kvm_get_hpet_legacy_mode(kvm_context, &hpet_legacy_mode);
+    }
 
+    kvm_set_hpet_legacy_mode(kvm_context, &hpet_legacy_mode);
+    kvm_set_pit(kvm_context, &pit);
     return 0;
 }
 
diff --git a/hw/i8254.c b/hw/i8254.c
index 2f229f9..0136c64 100644
--- a/hw/i8254.c
+++ b/hw/i8254.c
@@ -25,6 +25,7 @@
 #include "pc.h"
 #include "isa.h"
 #include "qemu-timer.h"
+#include "qemu-kvm.h"
 #include "i8254.h"
 
 //#define DEBUG_PIT
@@ -202,6 +203,11 @@ static inline void pit_load_count(PITChannelState *s, int val)
         val = 0x10000;
     s->count_load_time = qemu_get_clock(vm_clock);
     s->count = val;
+#ifdef TARGET_I386
+    if (s->channel == 0 && pit_state.hpet_legacy_mode) {
+        return;
+    }
+#endif
     pit_irq_timer_update(s, s->count_load_time);
 }
 
@@ -371,10 +377,11 @@ static void pit_irq_timer_update(PITChannelState *s, int64_t current_time)
            (double)(expire_time - current_time) / ticks_per_sec);
 #endif
     s->next_transition_time = expire_time;
-    if (expire_time != -1)
+    if (expire_time != -1) {
         qemu_mod_timer(s->irq_timer, expire_time);
-    else
+    } else {
         qemu_del_timer(s->irq_timer);
+    }
 }
 
 static void pit_irq_timer(void *opaque)
@@ -451,6 +458,9 @@ void pit_reset(void *opaque)
     PITChannelState *s;
     int i;
 
+#ifdef TARGET_I386
+    pit->hpet_legacy_mode = 0;
+#endif
     for(i = 0;i < 3; i++) {
         s = &pit->channels[i];
         s->mode = 3;
@@ -459,33 +469,50 @@ void pit_reset(void *opaque)
     }
 }
 
+#ifdef TARGET_I386
 /* When HPET is operating in legacy mode, i8254 timer0 is disabled */
-void hpet_pit_disable(void) {
-    PITChannelState *s;
-    s = &pit_state.channels[0];
-    if (s->irq_timer)
-        qemu_del_timer(s->irq_timer);
+
+void hpet_disable_pit(void)
+{
+    PITChannelState *s = &pit_state.channels[0];
+
+    if (qemu_kvm_pit_in_kernel()) {
+        kvm_hpet_disable_kpit();
+    } else {
+        pit_state.hpet_legacy_mode = 1;
+        if (s->irq_timer) {
+            qemu_del_timer(s->irq_timer);
+        }
+    }
 }
 
 /* When HPET is reset or leaving legacy mode, it must reenable i8254
  * timer 0
  */
 
-void hpet_pit_enable(void)
+void hpet_enable_pit(void)
 {
     PITState *pit = &pit_state;
-    PITChannelState *s;
-    s = &pit->channels[0];
-    s->mode = 3;
-    s->gate = 1;
-    pit_load_count(s, 0);
+    PITChannelState *s = &pit->channels[0];
+
+    if (qemu_kvm_pit_in_kernel()) {
+        kvm_hpet_enable_kpit();
+    } else {
+        pit_state.hpet_legacy_mode = 0;
+        pit_load_count(s, s->count);
+    }
 }
+#endif
 
 PITState *pit_init(int base, qemu_irq irq)
 {
     PITState *pit = &pit_state;
     PITChannelState *s;
+    int i;
 
+    for (i = 0; i < 3 ; i++) {
+        pit->channels[i].channel = i;
+    }
     s = &pit->channels[0];
     /* the timer 0 is connected to an IRQ */
     s->irq_timer = qemu_new_timer(vm_clock, pit_irq_timer, s);
diff --git a/hw/i8254.h b/hw/i8254.h
index ee68ab5..9efd4ab 100644
--- a/hw/i8254.h
+++ b/hw/i8254.h
@@ -35,6 +35,7 @@
 
 typedef struct PITChannelState {
     int count; /* can be 65536 */
+    uint8_t channel;
     uint16_t latched_count;
     uint8_t count_latched;
     uint8_t status_latched;
@@ -55,6 +56,7 @@ typedef struct PITChannelState {
 
 struct PITState {
     PITChannelState channels[3];
+    uint8_t hpet_legacy_mode;
 };
 
 void pit_save(QEMUFile *f, void *opaque);
diff --git a/hw/pc.h b/hw/pc.h
index 3af22f2..abac8d8 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -74,8 +74,8 @@ int pit_get_out(PITState *pit, int channel, int64_t current_time);
 
 PITState *kvm_pit_init(int base, qemu_irq irq);
 
-void hpet_pit_disable(void);
-void hpet_pit_enable(void);
+void hpet_disable_pit(void);
+void hpet_enable_pit(void);
 
 /* vmport.c */
 void vmport_init(void);
diff --git a/kvm/include/linux/kvm.h b/kvm/include/linux/kvm.h
index ca93871..3559628 100644
--- a/kvm/include/linux/kvm.h
+++ b/kvm/include/linux/kvm.h
@@ -464,6 +464,7 @@ struct kvm_trace_rec {
 /* Another bug in KVM_SET_USER_MEMORY_REGION fixed: */
 #define KVM_CAP_JOIN_MEMORY_REGIONS_WORKS 30
 #define KVM_CAP_PIT2 33
+#define KVM_CAP_HPET_LEGACY_MODE 35
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -613,6 +614,9 @@ struct kvm_debug_guest {
 #define KVM_IA64_VCPU_GET_STACK   _IOR(KVMIO,  0x9a, void *)
 #define KVM_IA64_VCPU_SET_STACK   _IOW(KVMIO,  0x9b, void *)
 
+#define KVM_GET_HPET_LEGACY_MODE   _IOR(KVMIO,   0x9c, __u8)
+#define KVM_SET_HPET_LEGACY_MODE   _IOWR(KVMIO,   0x9d, __u8)
+
 #define KVM_TRC_INJ_VIRQ         (KVM_TRC_HANDLER + 0x02)
 #define KVM_TRC_REDELIVER_EVT    (KVM_TRC_HANDLER + 0x03)
 #define KVM_TRC_PEND_INTR        (KVM_TRC_HANDLER + 0x04)
diff --git a/libkvm-all.h b/libkvm-all.h
index 4f7b9a3..6e76704 100644
--- a/libkvm-all.h
+++ b/libkvm-all.h
@@ -294,6 +294,36 @@ int kvm_get_interrupt_flag(kvm_vcpu_context_t vcpu);
 uint64_t kvm_get_apic_base(kvm_vcpu_context_t vcpu);
 
 /*!
+ * \brief Check for existence of kvm hpet legacy mode
+ *
+ * \param kvm Pointer to the current kvm_context
+ * \return 0 on success
+ */
+int kvm_has_hpet_legacy_mode(kvm_context_t kvm);
+
+/*!
+ * \brief Set hpet legacy mode
+ *
+ * mode = 1: start legacy mode, disable kpit
+ * mode = 0: end legacy mode, enable kpit
+ *
+ * \param kvm Pointer to the current kvm_context
+ * \param mode Pointer to the mode
+ * \return 0 on success
+ */
+int kvm_set_hpet_legacy_mode(kvm_context_t kvm, __u8 *mode);
+
+/*!
+ * \brief Get kernel hpet legacy mode
+ *
+ *
+ * \param kvm Pointer to the current kvm_context
+ * \param mode Pointer to the mode
+ * \return 0 on success
+ */
+int kvm_get_hpet_legacy_mode(kvm_context_t kvm, __u8 *mode);
+
+/*!
  * \brief Check if a vcpu is ready for interrupt injection
  *
  * This checks if vcpu interrupts are not masked by mov ss or sti.
diff --git a/qemu-kvm.c b/qemu-kvm.c
index 0f7adcc..6c57b36 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -1975,6 +1975,32 @@ int kvm_vcpu_inited(CPUState *env)
     return env->kvm_cpu_state.created;
 }
 
+#ifdef TARGET_I386
+void kvm_hpet_disable_kpit(void)
+{
+    int rc;
+    uint8_t mode = 1;
+
+    rc = kvm_set_hpet_legacy_mode(kvm_context, &mode);
+    if (rc < 0) {
+        fprintf(stderr,"HPET kernel legacy support unavailable! rc = %d", rc);
+        exit(1);
+    }
+}
+
+void kvm_hpet_enable_kpit(void)
+{
+    int rc;
+    uint8_t mode = 0;
+
+    rc = kvm_set_hpet_legacy_mode(kvm_context, &mode);
+    if (rc < 0) {
+        fprintf(stderr,"HPET kernel legacy support unavailable! rc = %d", rc);
+        exit(1);
+    }
+}
+#endif
+
 int kvm_init_ap(void)
 {
 #ifdef TARGET_I386
diff --git a/qemu-kvm.h b/qemu-kvm.h
index 2c0afb5..fcda865 100644
--- a/qemu-kvm.h
+++ b/qemu-kvm.h
@@ -35,6 +35,8 @@ void kvm_apic_init(CPUState *env);
 /* called from vcpu initialization */
 void qemu_kvm_load_lapic(CPUState *env);
 
+void kvm_hpet_enable_kpit(void);
+void kvm_hpet_disable_kpit(void);
 int kvm_set_irq(int irq, int level, int *status);
 
 int kvm_physical_memory_set_dirty_tracking(int enable);
@@ -170,6 +172,9 @@ int kvm_has_sync_mmu(void);
 #define qemu_kvm_irqchip_in_kernel() kvm_irqchip_in_kernel(kvm_context)
 #define qemu_kvm_pit_in_kernel() kvm_pit_in_kernel(kvm_context)
 #define qemu_kvm_has_gsi_routing() kvm_has_gsi_routing(kvm_context)
+#ifdef TARGET_I386
+#define qemu_kvm_has_hpet_legacy_mode() kvm_has_hpet_legacy_mode(kvm_context)
+#endif
 void kvm_init_vcpu(CPUState *env);
 void kvm_load_tsc(CPUState *env);
 #else
@@ -178,6 +183,9 @@ void kvm_load_tsc(CPUState *env);
 #define qemu_kvm_irqchip_in_kernel() (0)
 #define qemu_kvm_pit_in_kernel() (0)
 #define qemu_kvm_has_gsi_routing() (0)
+#ifdef TARGET_I386
+#define qemu_kvm_has_hpet_legacy_mode() (0)
+#endif
 #define kvm_load_registers(env) do {} while(0)
 #define kvm_save_registers(env) do {} while(0)
 #define qemu_kvm_cpu_stop(env) do {} while(0)
diff --git a/target-i386/libkvm.c b/target-i386/libkvm.c
index 0f4e009..c96bc69 100644
--- a/target-i386/libkvm.c
+++ b/target-i386/libkvm.c
@@ -562,6 +562,47 @@ int kvm_disable_tpr_access_reporting(kvm_vcpu_context_t vcpu)
 
 #endif
 
+int kvm_has_hpet_legacy_mode(kvm_context_t kvm)
+{
+	int r = 0;
+
+#ifdef KVM_CAP_HPET_LEGACY_MODE
+	r = kvm_check_extension(kvm, KVM_CAP_HPET_LEGACY_MODE);
+#endif
+	return r;
+}
+
+int kvm_get_hpet_legacy_mode(kvm_context_t kvm, uint8_t *mode)
+{
+	int r;
+
+	if (!kvm_has_hpet_legacy_mode(kvm)) {
+		return -1;
+	}
+	r = ioctl(kvm->vm_fd, KVM_GET_HPET_LEGACY_MODE, mode);
+	if (r == -1) {
+		r = -errno;
+		perror("kvm_get_hpet_legacy_mode");
+		return r;
+	}
+	return 0;
+}
+
+int kvm_set_hpet_legacy_mode(kvm_context_t kvm, uint8_t *mode)
+{
+	int r;
+
+	if (!kvm_has_hpet_legacy_mode(kvm)) {
+		return -1;
+	}
+	r = ioctl(kvm->vm_fd, KVM_SET_HPET_LEGACY_MODE, mode);
+	if (r == -1) {
+		r = -errno;
+		perror("kvm_set_hpet_legacy_mode");
+		return r;
+	}
+	return 0;
+}
 #ifdef KVM_CAP_EXT_CPUID
 
 static struct kvm_cpuid2 *try_get_cpuid(kvm_context_t kvm, int max)
diff --git a/vl.c b/vl.c
index 80daa52..7c248a9 100644
--- a/vl.c
+++ b/vl.c
@@ -248,7 +248,9 @@ int assigned_devices_index;
 int smp_cpus = 1;
 const char *vnc_display;
 int acpi_enabled = 1;
+#ifdef TARGET_I386
 int no_hpet = 0;
+#endif
 int fd_bootchk = 1;
 int no_reboot = 0;
 int no_shutdown = 0;
@@ -6195,10 +6197,21 @@ int main(int argc, char **argv, char **envp)
     module_call_init(MODULE_INIT_DEVICE);
 
     if (kvm_enabled()) {
-       kvm_init_ap();
+        kvm_init_ap();
 #ifdef USE_KVM
-        if (kvm_irqchip && !qemu_kvm_has_gsi_routing()) {
-            irq0override = 0;
+        if (kvm_irqchip) {
+            if (!qemu_kvm_has_gsi_routing()) {
+                irq0override = 0;
+                /* if kernel can't do irq routing, interrupt source
+                 * override 0->2 can not be set up as required by hpet,
+                 * so disable hpet.
+                 */
+#ifdef TARGET_I386
+                no_hpet=1;
+            } else  if (!qemu_kvm_has_hpet_legacy_mode()) {
+                no_hpet=1;
+            }
+#endif
         }
 #endif
     }

[PATCH 2/2][RFC] Kernel changes for HPET legacy mode (v7)

[Beth Kon]

When kvm is in hpet_legacy_mode, the hpet is providing the 
timer interrupt and the pit should not be. So in legacy mode, the pit timer is
destroyed, but the *state* of the pit is maintained. So if kvm or the guest
tries to modify the state of the pit, this modification is accepted, *except* 
that the timer isn't actually started. When we exit hpet_legacy_mode, 
the current state of the pit (which is up to date since we've been 
accepting modifications) is used to restart the pit timer.

The saved_mode code in kvm_pit_load_count temporarily changes mode to 
0xff in order to destroy the timer, but then restores the actual value,
again maintaining "current" state of the pit for possible later reenablement.

changes from v6:

- Added ioctl interface for legacy mode in order not to break the abi.


Signed-off-by: Beth Kon <eak@us.ibm.com>

diff --git a/arch/x86/include/asm/kvm.h b/arch/x86/include/asm/kvm.h
index 708b9c3..25cae50 100644
--- a/arch/x86/include/asm/kvm.h
+++ b/arch/x86/include/asm/kvm.h
@@ -18,6 +18,7 @@
 #define __KVM_HAVE_GUEST_DEBUG
 #define __KVM_HAVE_MSIX
 #define __KVM_HAVE_MCE
+#define __KVM_HAVE_HPET_LEGACY_MODE
 
 /* Architectural interrupt line count. */
 #define KVM_NR_INTERRUPTS 256
diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index 331705f..02de293 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -329,21 +329,32 @@ static void pit_load_count(struct kvm *kvm, int channel, u32 val)
 	case 1:
         /* FIXME: enhance mode 4 precision */
 	case 4:
-		create_pit_timer(ps, val, 0);
+		if (!ps->hpet_legacy_mode)
+			create_pit_timer(ps, val, 0);
 		break;
 	case 2:
 	case 3:
-		create_pit_timer(ps, val, 1);
+		if (!ps->hpet_legacy_mode)
+			create_pit_timer(ps, val, 1);
 		break;
 	default:
 		destroy_pit_timer(&ps->pit_timer);
 	}
 }
 
-void kvm_pit_load_count(struct kvm *kvm, int channel, u32 val)
+void kvm_pit_load_count(struct kvm *kvm, int channel, u32 val, int hpet_legacy_start)
 {
+	u8 saved_mode;
 	mutex_lock(&kvm->arch.vpit->pit_state.lock);
-	pit_load_count(kvm, channel, val);
+	if (hpet_legacy_start) {
+		/* save existing mode for later reenablement */
+		saved_mode = kvm->arch.vpit->pit_state.channels[0].mode;
+		kvm->arch.vpit->pit_state.channels[0].mode = 0xff; /* disable timer */
+		pit_load_count(kvm, channel, val);
+		kvm->arch.vpit->pit_state.channels[0].mode = saved_mode;
+	} else {
+		pit_load_count(kvm, channel, val);
+	}
 	mutex_unlock(&kvm->arch.vpit->pit_state.lock);
 }
 
@@ -548,6 +559,7 @@ void kvm_pit_reset(struct kvm_pit *pit)
 	struct kvm_kpit_channel_state *c;
 
 	mutex_lock(&pit->pit_state.lock);
+	pit->pit_state.hpet_legacy_mode = 0;
 	for (i = 0; i < 3; i++) {
 		c = &pit->pit_state.channels[i];
 		c->mode = 0xff;
diff --git a/arch/x86/kvm/i8254.h b/arch/x86/kvm/i8254.h
index b267018..b5967ca 100644
--- a/arch/x86/kvm/i8254.h
+++ b/arch/x86/kvm/i8254.h
@@ -21,6 +21,7 @@ struct kvm_kpit_channel_state {
 
 struct kvm_kpit_state {
 	struct kvm_kpit_channel_state channels[3];
+	u8 hpet_legacy_mode;
 	struct kvm_timer pit_timer;
 	bool is_periodic;
 	u32    speaker_data_on;
@@ -49,7 +50,7 @@ struct kvm_pit {
 #define KVM_PIT_CHANNEL_MASK	    0x3
 
 void kvm_inject_pit_timer_irqs(struct kvm_vcpu *vcpu);
-void kvm_pit_load_count(struct kvm *kvm, int channel, u32 val);
+void kvm_pit_load_count(struct kvm *kvm, int channel, u32 val, int hpet_legacy_start);
 struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags);
 void kvm_free_pit(struct kvm *kvm);
 void kvm_pit_reset(struct kvm_pit *pit);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6025e5b..8562eeb 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1134,6 +1134,7 @@ int kvm_dev_ioctl_check_extension(long ext)
 	case KVM_CAP_ASSIGN_DEV_IRQ:
 	case KVM_CAP_IRQFD:
 	case KVM_CAP_PIT2:
+	case KVM_CAP_HPET_LEGACY_MODE:
 		r = 1;
 		break;
 	case KVM_CAP_COALESCED_MMIO:
@@ -1986,7 +1987,24 @@ static int kvm_vm_ioctl_set_pit(struct kvm *kvm, struct kvm_pit_state *ps)
 	int r = 0;
 
 	memcpy(&kvm->arch.vpit->pit_state, ps, sizeof(struct kvm_pit_state));
-	kvm_pit_load_count(kvm, 0, ps->channels[0].count);
+	kvm_pit_load_count(kvm, 0, ps->channels[0].count, 0);
+	return r;
+}
+
+static int kvm_vm_ioctl_get_hpet_legacy_mode(struct kvm *kvm, u8 *mode)
+{
+	int r = 0;
+	*mode = kvm->arch.vpit->pit_state.hpet_legacy_mode;
+	return r;
+}
+
+static int kvm_vm_ioctl_set_hpet_legacy_mode(struct kvm *kvm, u8 *mode)
+{
+	int r = 0, start = 0;
+	if (kvm->arch.vpit->pit_state.hpet_legacy_mode == 0 && *mode == 1)
+		start = 1;
+	kvm->arch.vpit->pit_state.hpet_legacy_mode = *mode;
+	kvm_pit_load_count(kvm, 0, kvm->arch.vpit->pit_state.channels[0].count, start);
 	return r;
 }
 
@@ -2047,6 +2065,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		struct kvm_pit_state ps;
 		struct kvm_memory_alias alias;
 		struct kvm_pit_config pit_config;
+		u8 hpet_legacy_mode;
 	} u;
 
 	switch (ioctl) {
@@ -2227,6 +2246,32 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		r = 0;
 		break;
 	}
+	case KVM_GET_HPET_LEGACY_MODE: {
+		r = -ENXIO;
+		if (!kvm->arch.vpit)
+			goto out;
+		r = kvm_vm_ioctl_get_hpet_legacy_mode(kvm, &u.hpet_legacy_mode);
+		if (r)
+			goto out;
+		r = -EFAULT;
+		if (copy_to_user(argp, &u.ps, sizeof(u.hpet_legacy_mode)))
+			goto out;
+		r = 0;
+		break;
+	}
+	case KVM_SET_HPET_LEGACY_MODE: {
+		r = -EFAULT;
+		if (copy_from_user(&u.hpet_legacy_mode, argp, sizeof(u.hpet_legacy_mode)))
+			goto out;
+		r = -ENXIO;
+		if (!kvm->arch.vpit)
+			goto out;
+		r = kvm_vm_ioctl_set_hpet_legacy_mode(kvm, &u.hpet_legacy_mode);
+		if (r)
+			goto out;
+		r = 0;
+		break;
+	}
 	case KVM_REINJECT_CONTROL: {
 		struct kvm_reinject_control control;
 		r =  -EFAULT;
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 38ff31e..202cf6e 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -438,6 +438,9 @@ struct kvm_trace_rec {
 #define KVM_CAP_PIT2 33
 #endif
 #define KVM_CAP_SET_BOOT_CPU_ID 34
+#ifdef __KVM_HAVE_HPET_LEGACY_MODE
+#define KVM_CAP_HPET_LEGACY_MODE 35
+#endif
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -613,6 +616,9 @@ struct kvm_debug_guest {
 #define KVM_IA64_VCPU_GET_STACK   _IOR(KVMIO,  0x9a, void *)
 #define KVM_IA64_VCPU_SET_STACK   _IOW(KVMIO,  0x9b, void *)
 
+#define KVM_GET_HPET_LEGACY_MODE   _IOR(KVMIO,   0x9c, __u8)
+#define KVM_SET_HPET_LEGACY_MODE   _IOWR(KVMIO,   0x9d, __u8)
+
 #define KVM_TRC_INJ_VIRQ         (KVM_TRC_HANDLER + 0x02)
 #define KVM_TRC_REDELIVER_EVT    (KVM_TRC_HANDLER + 0x03)
 #define KVM_TRC_PEND_INTR        (KVM_TRC_HANDLER + 0x04)

Re: [PATCH 2/2][RFC] Kernel changes for HPET legacy mode (v7)

[Jan Kiszka]

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

Beth Kon wrote:
> When kvm is in hpet_legacy_mode, the hpet is providing the 
> timer interrupt and the pit should not be. So in legacy mode, the pit timer is
> destroyed, but the *state* of the pit is maintained. So if kvm or the guest
> tries to modify the state of the pit, this modification is accepted, *except* 
> that the timer isn't actually started. When we exit hpet_legacy_mode, 
> the current state of the pit (which is up to date since we've been 
> accepting modifications) is used to restart the pit timer.
> 
> The saved_mode code in kvm_pit_load_count temporarily changes mode to 
> 0xff in order to destroy the timer, but then restores the actual value,
> again maintaining "current" state of the pit for possible later reenablement.
> 
> changes from v6:
> 
> - Added ioctl interface for legacy mode in order not to break the abi.
> 
> 
> Signed-off-by: Beth Kon <eak@us.ibm.com>

...

> @@ -1986,7 +1987,24 @@ static int kvm_vm_ioctl_set_pit(struct kvm *kvm, struct kvm_pit_state *ps)
>  	int r = 0;
>  
>  	memcpy(&kvm->arch.vpit->pit_state, ps, sizeof(struct kvm_pit_state));
> -	kvm_pit_load_count(kvm, 0, ps->channels[0].count);
> +	kvm_pit_load_count(kvm, 0, ps->channels[0].count, 0);
> +	return r;
> +}
> +
> +static int kvm_vm_ioctl_get_hpet_legacy_mode(struct kvm *kvm, u8 *mode)
> +{
> +	int r = 0;
> +	*mode = kvm->arch.vpit->pit_state.hpet_legacy_mode;
> +	return r;
> +}

This only applies if we go for a separate mode IOCTL:
The legacy mode is not directly modifiable by the guest. Is it planned
to add in-kernel hpet support? Otherwise get_hpet_legacy_mode looks a
bit like overkill given that user space could easily track the state.

> +
> +static int kvm_vm_ioctl_set_hpet_legacy_mode(struct kvm *kvm, u8 *mode)
> +{
> +	int r = 0, start = 0;
> +	if (kvm->arch.vpit->pit_state.hpet_legacy_mode == 0 && *mode == 1)

Here you check more mode == 1, but legacy_mode is only checked for != 0.
I would make this consistent.

> +		start = 1;
> +	kvm->arch.vpit->pit_state.hpet_legacy_mode = *mode;
> +	kvm_pit_load_count(kvm, 0, kvm->arch.vpit->pit_state.channels[0].count, start);
>  	return r;
>  }
>  
> @@ -2047,6 +2065,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  		struct kvm_pit_state ps;
>  		struct kvm_memory_alias alias;
>  		struct kvm_pit_config pit_config;
> +		u8 hpet_legacy_mode;

Hmm, stead of introducing a new pair of singe-purpose IOCTLs, why not
add KVM_GET/SET_PIT2 which exchanges an extended kvm_pit_state2. And
that struct should also include some flags field and enough padding to
be potentially extended yet again in the future. In that case I see no
problem having also a mode read-back interface.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH 2/2][RFC] Kernel changes for HPET legacy mode (v7)

[Beth Kon]

Jan Kiszka wrote:
> Beth Kon wrote:
>   
>> When kvm is in hpet_legacy_mode, the hpet is providing the 
>> timer interrupt and the pit should not be. So in legacy mode, the pit timer is
>> destroyed, but the *state* of the pit is maintained. So if kvm or the guest
>> tries to modify the state of the pit, this modification is accepted, *except* 
>> that the timer isn't actually started. When we exit hpet_legacy_mode, 
>> the current state of the pit (which is up to date since we've been 
>> accepting modifications) is used to restart the pit timer.
>>
>> The saved_mode code in kvm_pit_load_count temporarily changes mode to 
>> 0xff in order to destroy the timer, but then restores the actual value,
>> again maintaining "current" state of the pit for possible later reenablement.
>>
>> changes from v6:
>>
>> - Added ioctl interface for legacy mode in order not to break the abi.
>>
>>
>> Signed-off-by: Beth Kon <eak@us.ibm.com>
>>     
>
> ...
>
>   
>> @@ -1986,7 +1987,24 @@ static int kvm_vm_ioctl_set_pit(struct kvm *kvm, struct kvm_pit_state *ps)
>>  	int r = 0;
>>  
>>  	memcpy(&kvm->arch.vpit->pit_state, ps, sizeof(struct kvm_pit_state));
>> -	kvm_pit_load_count(kvm, 0, ps->channels[0].count);
>> +	kvm_pit_load_count(kvm, 0, ps->channels[0].count, 0);
>> +	return r;
>> +}
>> +
>> +static int kvm_vm_ioctl_get_hpet_legacy_mode(struct kvm *kvm, u8 *mode)
>> +{
>> +	int r = 0;
>> +	*mode = kvm->arch.vpit->pit_state.hpet_legacy_mode;
>> +	return r;
>> +}
>>     
>
> This only applies if we go for a separate mode IOCTL:
> The legacy mode is not directly modifiable by the guest. Is it planned
> to add in-kernel hpet support? Otherwise get_hpet_legacy_mode looks a
> bit like overkill given that user space could easily track the state.
>   
Assuming I will at least generalize the ioctl, I'll leave this question 
for the time being.
>   
>> +
>> +static int kvm_vm_ioctl_set_hpet_legacy_mode(struct kvm *kvm, u8 *mode)
>> +{
>> +	int r = 0, start = 0;
>> +	if (kvm->arch.vpit->pit_state.hpet_legacy_mode == 0 && *mode == 1)
>>     
>
> Here you check more mode == 1, but legacy_mode is only checked for != 0.
> I would make this consistent.
>
>   
ok
>> +		start = 1;
>> +	kvm->arch.vpit->pit_state.hpet_legacy_mode = *mode;
>> +	kvm_pit_load_count(kvm, 0, kvm->arch.vpit->pit_state.channels[0].count, start);
>>  	return r;
>>  }
>>  
>> @@ -2047,6 +2065,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
>>  		struct kvm_pit_state ps;
>>  		struct kvm_memory_alias alias;
>>  		struct kvm_pit_config pit_config;
>> +		u8 hpet_legacy_mode;
>>     
>
> Hmm, stead of introducing a new pair of singe-purpose IOCTLs, why not
> add KVM_GET/SET_PIT2 which exchanges an extended kvm_pit_state2. And
> that struct should also include some flags field and enough padding to
> be potentially extended yet again in the future. In that case I see no
> problem having also a mode read-back interface.
>
>   
I thought about that, but it seemed to add unnecessary complexity, since 
this legacy control is really outside of normal PIT operation, which is 
embodied by KVM_GET/SET_PIT. It might be worth making this ioctl more 
general. Rather than SET_HPET_LEGACY, have SET_PIT_CONTROLS and pass a 
bit field, one of which is HPET_LEGACY. But, if general consensus is 
that it would be better to create a kvm_pit_state2, and get/set that, I 
can do that.
> Jan
>
>   

Re: [PATCH 2/2][RFC] Kernel changes for HPET legacy mode (v7)

[Avi Kivity]

On 06/18/2009 10:04 PM, Jan Kiszka wrote:
> Hmm, stead of introducing a new pair of singe-purpose IOCTLs, why not
> add KVM_GET/SET_PIT2 which exchanges an extended kvm_pit_state2. And
> that struct should also include some flags field and enough padding to
> be potentially extended yet again in the future. In that case I see no
> problem having also a mode read-back interface.
>    

We'd only add kernel hpet if we were forced to (I imagine the same 
applications/kernels that forced the PIT into the kernel will do the 
same for HPET).

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/2][RFC] Completing HPET in KVM (v7)

[Avi Kivity]

On 06/18/2009 03:47 PM, Beth Kon wrote:
> There is a problem in the latest git with savevm (it aborts).
>    

Now fixed.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 2/2][RFC] Kernel changes for HPET legacy mode (v7)

[Jan Kiszka]

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

Avi Kivity wrote:
> On 06/18/2009 10:04 PM, Jan Kiszka wrote:
>> Hmm, stead of introducing a new pair of singe-purpose IOCTLs, why not
>> add KVM_GET/SET_PIT2 which exchanges an extended kvm_pit_state2. And
>> that struct should also include some flags field and enough padding to
>> be potentially extended yet again in the future. In that case I see no
>> problem having also a mode read-back interface.
>>    
> 
> We'd only add kernel hpet if we were forced to (I imagine the same
> applications/kernels that forced the PIT into the kernel will do the
> same for HPET).
> 

Answer and citation does not yet correlate for me.

Could you comment more explicitly if your are fine with Beth's proposed
interface, rather prefer something like my suggestion or even want
something totally different?

Thanks,
Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH 2/2][RFC] Kernel changes for HPET legacy mode (v7)

[Avi Kivity]

On 06/22/2009 12:14 PM, Jan Kiszka wrote:
>>> Hmm, stead of introducing a new pair of singe-purpose IOCTLs, why not
>>> add KVM_GET/SET_PIT2 which exchanges an extended kvm_pit_state2. And
>>> that struct should also include some flags field and enough padding to
>>> be potentially extended yet again in the future. In that case I see no
>>> problem having also a mode read-back interface.
>>>
>>>        
>> We'd only add kernel hpet if we were forced to (I imagine the same
>> applications/kernels that forced the PIT into the kernel will do the
>> same for HPET).
>>
>>      
>
> Answer and citation does not yet correlate for me.
>    

Misquote.  I meant to reply to your 'Is it planned to add in-kernel hpet 
support?' question.  Must be early in the morning in some timezone.

> Could you comment more explicitly if your are fine with Beth's proposed
> interface, rather prefer something like my suggestion or even want
> something totally different?
>    

GET/SET PIT2 looks like the best choice to me, at least until I find 
whoever designed the HPET/PIT interdependency and make him take it back.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 2/2][RFC] Kernel changes for HPET legacy mode (v7)

[Beth Kon]

Avi Kivity wrote:
> On 06/22/2009 12:14 PM, Jan Kiszka wrote:
>>>> Hmm, stead of introducing a new pair of singe-purpose IOCTLs, why not
>>>> add KVM_GET/SET_PIT2 which exchanges an extended kvm_pit_state2. And
>>>> that struct should also include some flags field and enough padding to
>>>> be potentially extended yet again in the future. In that case I see no
>>>> problem having also a mode read-back interface.
>>>>
>>>>        
>>> We'd only add kernel hpet if we were forced to (I imagine the same
>>> applications/kernels that forced the PIT into the kernel will do the
>>> same for HPET).
>>>
>>>      
>>
>> Answer and citation does not yet correlate for me.
>>    
>
> Misquote.  I meant to reply to your 'Is it planned to add in-kernel 
> hpet support?' question.  Must be early in the morning in some timezone.
>
>> Could you comment more explicitly if your are fine with Beth's proposed
>> interface, rather prefer something like my suggestion or even want
>> something totally different?
>>    
>
> GET/SET PIT2 looks like the best choice to me, at least until I find 
> whoever designed the HPET/PIT interdependency and make him take it back.
>
It seems to me that GET/SET PIT2 adds a good deal of complexity without 
any gain. PIT is not a very dynamic technology. GET/SET PIT works for 
PIT operational needs as defined by the PIT specifications. This whole 
problem existe because of the unfortunate requirement that hpet disable 
PIT interrupts, which is quite outside normal PIT operation. If I create 
a GET/SET PIT2, and a PITState2 that is a superset of PITState1, then I 
need to address all the cases where PITState is currently set/referenced 
and properly use PITState2/PITState, depending on whether the kernel 
supports PITState2. It just seems unnecessary, since hpet_legacy, and 
probably any other future "control" of the PIT will likely be outside of 
"normal" PIT operation.  I really think a separate ioctl that transfers 
just control information (of which one of the flag bits would be 
hpet_legacy) would be preferable and cleaner. Am I missing some 
advantage to PITState2? Or is there some simple way to implement this 
that I'm missing?

Re: [PATCH 2/2][RFC] Kernel changes for HPET legacy mode (v7)

[Jan Kiszka]

Beth Kon wrote:
> Avi Kivity wrote:
>> On 06/22/2009 12:14 PM, Jan Kiszka wrote:
>>>>> Hmm, stead of introducing a new pair of singe-purpose IOCTLs, why not
>>>>> add KVM_GET/SET_PIT2 which exchanges an extended kvm_pit_state2. And
>>>>> that struct should also include some flags field and enough padding to
>>>>> be potentially extended yet again in the future. In that case I see no
>>>>> problem having also a mode read-back interface.
>>>>>
>>>>>        
>>>> We'd only add kernel hpet if we were forced to (I imagine the same
>>>> applications/kernels that forced the PIT into the kernel will do the
>>>> same for HPET).
>>>>
>>>>      
>>>
>>> Answer and citation does not yet correlate for me.
>>>    
>>
>> Misquote.  I meant to reply to your 'Is it planned to add in-kernel
>> hpet support?' question.  Must be early in the morning in some timezone.
>>
>>> Could you comment more explicitly if your are fine with Beth's proposed
>>> interface, rather prefer something like my suggestion or even want
>>> something totally different?
>>>    
>>
>> GET/SET PIT2 looks like the best choice to me, at least until I find
>> whoever designed the HPET/PIT interdependency and make him take it back.
>>
> It seems to me that GET/SET PIT2 adds a good deal of complexity without
> any gain. PIT is not a very dynamic technology. GET/SET PIT works for
> PIT operational needs as defined by the PIT specifications. This whole
> problem existe because of the unfortunate requirement that hpet disable
> PIT interrupts, which is quite outside normal PIT operation. If I create
> a GET/SET PIT2, and a PITState2 that is a superset of PITState1, then I
> need to address all the cases where PITState is currently set/referenced
> and properly use PITState2/PITState, depending on whether the kernel
> supports PITState2. It just seems unnecessary, since hpet_legacy, and
> probably any other future "control" of the PIT will likely be outside of
> "normal" PIT operation.  I really think a separate ioctl that transfers
> just control information (of which one of the flag bits would be
> hpet_legacy) would be preferable and cleaner. Am I missing some
> advantage to PITState2? Or is there some simple way to implement this
> that I'm missing?

I think you just need to refactor out the processing of
kvm_pit_channel_state and call it from the legacy get/set handler as
well as from the new one. And the new handlers will also process the
additional fields that will come with kvm_pit_state2. Not really more
complex than the current proposal.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux