[PATCH v12 rebased 0/8] pv event to notify host when the guest is panicked

[PATCH v12 rebased 0/8] pv event to notify host when the guest is panicked

[Hu Tao]

This series implements a new interface, kvm pv event, to notify host when
some events happen in guest. Right now there is one supported event: guest
panic.

Also, the cpu runstate is preserved during save/load vm and migration. Thus,
if vm is panicked during migration, we can still know it by quring the status
of vm in destination host.

This version is a rebase and no code change.

v12: http://lists.gnu.org/archive/html/qemu-devel/2012-12/msg01459.html
v11: http://lists.gnu.org/archive/html/qemu-devel/2012-10/msg04361.html

Hu Tao (7):
  save/load cpu runstate
  update kernel headers
  add a new runstate: RUN_STATE_GUEST_PANICKED
  add a new qevent: QEVENT_GUEST_PANICKED
  introduce a new qom device to deal with panicked event
  allower the user to disable pv event support
  pv event: add document to describe the usage

Wen Congyang (1):
  start vm after resetting it

 docs/pv-event.txt                |  17 ++++
 hw/kvm/Makefile.objs             |   2 +-
 hw/kvm/pv_event.c                | 197 +++++++++++++++++++++++++++++++++++++++
 hw/pc_piix.c                     |  12 +++
 include/block/block.h            |   2 +
 include/monitor/monitor.h        |   1 +
 include/sysemu/kvm.h             |   2 +
 include/sysemu/sysemu.h          |   2 +
 kvm-stub.c                       |   4 +
 linux-headers/asm-x86/kvm_para.h |   1 +
 linux-headers/linux/kvm_para.h   |   6 ++
 migration.c                      |   7 +-
 monitor.c                        |   6 +-
 qapi-schema.json                 |   6 +-
 qemu-options.hx                  |   3 +-
 qmp.c                            |   5 +-
 savevm.c                         |   1 +
 vl.c                             |  56 ++++++++++-
 18 files changed, 313 insertions(+), 17 deletions(-)
 create mode 100644 docs/pv-event.txt
 create mode 100644 hw/kvm/pv_event.c

-- 
1.8.0.1.240.ge8a1f5a

[PATCH v12 rebased] kvm: notify host when the guest is panicked

[Hu Tao]

We can know the guest is panicked when the guest runs on xen.
But we do not have such feature on kvm.

Another purpose of this feature is: management app(for example:
libvirt) can do auto dump when the guest is panicked. If management
app does not do auto dump, the guest's user can do dump by hand if
he sees the guest is panicked.

We have three solutions to implement this feature:
1. use vmcall
2. use I/O port
3. use virtio-serial.

We have decided to avoid touching hypervisor. The reason why I choose
choose the I/O port is:
1. it is easier to implememt
2. it does not depend any virtual device
3. it can work when starting the kernel

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 arch/ia64/kvm/irq.h                  | 19 +++++++++++++
 arch/powerpc/include/asm/kvm_para.h  | 18 ++++++++++++
 arch/s390/include/asm/kvm_para.h     | 19 +++++++++++++
 arch/x86/include/asm/kvm_para.h      | 20 ++++++++++++++
 arch/x86/include/uapi/asm/kvm_para.h |  2 ++
 arch/x86/kernel/kvm.c                | 53 ++++++++++++++++++++++++++++++++++++
 include/linux/kvm_para.h             | 18 ++++++++++++
 include/uapi/linux/kvm_para.h        |  6 ++++
 kernel/panic.c                       |  4 +++
 9 files changed, 159 insertions(+)

diff --git a/arch/ia64/kvm/irq.h b/arch/ia64/kvm/irq.h
index c0785a7..b3870f8 100644
--- a/arch/ia64/kvm/irq.h
+++ b/arch/ia64/kvm/irq.h
@@ -30,4 +30,23 @@ static inline int irqchip_in_kernel(struct kvm *kvm)
 	return 1;
 }
 
+static inline int kvm_arch_pv_event_init(void)
+{
+	return 0;
+}
+
+static inline unsigned int kvm_arch_pv_features(void)
+{
+	return 0;
+}
+
+static inline void kvm_arch_pv_eject_event(unsigned int event)
+{
+}
+
+static inline bool kvm_arch_pv_event_enabled(void)
+{
+	return false;
+}
+
 #endif
diff --git a/arch/powerpc/include/asm/kvm_para.h b/arch/powerpc/include/asm/kvm_para.h
index 2b11965..17dd013 100644
--- a/arch/powerpc/include/asm/kvm_para.h
+++ b/arch/powerpc/include/asm/kvm_para.h
@@ -144,4 +144,22 @@ static inline bool kvm_check_and_clear_guest_paused(void)
 	return false;
 }
 
+static inline int kvm_arch_pv_event_init(void)
+{
+	return 0;
+}
+
+static inline unsigned int kvm_arch_pv_features(void)
+{
+	return 0;
+}
+
+static inline void kvm_arch_pv_eject_event(unsigned int event)
+{
+}
+
+static inline bool kvm_arch_pv_event_enabled(void)
+{
+	return false;
+}
 #endif /* __POWERPC_KVM_PARA_H__ */
diff --git a/arch/s390/include/asm/kvm_para.h b/arch/s390/include/asm/kvm_para.h
index e0f8423..81d87ec 100644
--- a/arch/s390/include/asm/kvm_para.h
+++ b/arch/s390/include/asm/kvm_para.h
@@ -154,4 +154,23 @@ static inline bool kvm_check_and_clear_guest_paused(void)
 	return false;
 }
 
+static inline int kvm_arch_pv_event_init(void)
+{
+	return 0;
+}
+
+static inline unsigned int kvm_arch_pv_features(void)
+{
+	return 0;
+}
+
+static inline void kvm_arch_pv_eject_event(unsigned int event)
+{
+}
+
+static inline bool kvm_arch_pv_event_enabled(void)
+{
+	return false;
+}
+
 #endif /* __S390_KVM_PARA_H */
diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
index 5ed1f161..c3f2ca8 100644
--- a/arch/x86/include/asm/kvm_para.h
+++ b/arch/x86/include/asm/kvm_para.h
@@ -133,4 +133,24 @@ static inline void kvm_disable_steal_time(void)
 }
 #endif
 
+static inline int kvm_arch_pv_event_init(void)
+{
+	if (!request_region(KVM_PV_EVENT_PORT, 4, "KVM_PV_EVENT"))
+		return -1;
+
+	return 0;
+}
+
+static inline unsigned int kvm_arch_pv_features(void)
+{
+	return inl(KVM_PV_EVENT_PORT);
+}
+
+static inline void kvm_arch_pv_eject_event(unsigned int event)
+{
+	outl(event, KVM_PV_EVENT_PORT);
+}
+
+bool kvm_arch_pv_event_enabled(void);
+
 #endif /* _ASM_X86_KVM_PARA_H */
diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h
index 06fdbd9..c15ef33 100644
--- a/arch/x86/include/uapi/asm/kvm_para.h
+++ b/arch/x86/include/uapi/asm/kvm_para.h
@@ -96,5 +96,7 @@ struct kvm_vcpu_pv_apf_data {
 #define KVM_PV_EOI_ENABLED KVM_PV_EOI_MASK
 #define KVM_PV_EOI_DISABLED 0x0
 
+#define KVM_PV_EVENT_PORT	(0x505UL)
+
 
 #endif /* _UAPI_ASM_X86_KVM_PARA_H */
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 9c2bd8b..0aa7b3e 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -73,6 +73,20 @@ static int parse_no_kvmclock_vsyscall(char *arg)
 
 early_param("no-kvmclock-vsyscall", parse_no_kvmclock_vsyscall);
 
+static int pv_event = 1;
+static int parse_no_pv_event(char *arg)
+{
+	pv_event = 0;
+	return 0;
+}
+
+bool kvm_arch_pv_event_enabled(void)
+{
+	return !!pv_event;
+}
+
+early_param("no-pv-event", parse_no_pv_event);
+
 static DEFINE_PER_CPU(struct kvm_vcpu_pv_apf_data, apf_reason) __aligned(64);
 static DEFINE_PER_CPU(struct kvm_steal_time, steal_time) __aligned(64);
 static int has_steal_clock = 0;
@@ -385,6 +399,17 @@ static struct notifier_block kvm_pv_reboot_nb = {
 	.notifier_call = kvm_pv_reboot_notify,
 };
 
+static int
+kvm_pv_panic_notify(struct notifier_block *nb, unsigned long code, void *unused)
+{
+	kvm_pv_eject_event(KVM_PV_EVENT_PANICKED);
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block kvm_pv_panic_nb = {
+	.notifier_call = kvm_pv_panic_notify,
+};
+
 static u64 kvm_steal_clock(int cpu)
 {
 	u64 steal;
@@ -462,6 +487,34 @@ static void __init kvm_apf_trap_init(void)
 	set_intr_gate(14, &async_page_fault);
 }
 
+static void __init kvm_pv_panicked_event_init(void)
+{
+	if (!kvm_para_available())
+		return;
+
+	if (kvm_pv_has_feature(KVM_PV_FEATURE_PANICKED))
+		atomic_notifier_chain_register(&panic_notifier_list,
+			&kvm_pv_panic_nb);
+}
+
+static inline int kvm_pv_event_init(void)
+{
+	return kvm_arch_pv_event_init();
+}
+
+static int __init enable_pv_event(void)
+{
+	if (pv_event) {
+		if (kvm_pv_event_init())
+			return 0;
+
+		kvm_pv_panicked_event_init();
+	}
+
+	return 0;
+}
+arch_initcall(enable_pv_event);
+
 void __init kvm_guest_init(void)
 {
 	int i;
diff --git a/include/linux/kvm_para.h b/include/linux/kvm_para.h
index 00a97bb..6fb6198 100644
--- a/include/linux/kvm_para.h
+++ b/include/linux/kvm_para.h
@@ -10,4 +10,22 @@ static inline int kvm_para_has_feature(unsigned int feature)
 		return 1;
 	return 0;
 }
+
+static inline int kvm_pv_has_feature(unsigned int feature)
+{
+	if (kvm_arch_pv_features() & (1UL << feature))
+		return 1;
+	return 0;
+}
+
+static inline void kvm_pv_eject_event(unsigned int event)
+{
+	kvm_arch_pv_eject_event(event);
+}
+
+static inline bool kvm_pv_event_enabled(void)
+{
+	return kvm_arch_pv_event_enabled();
+}
+
 #endif /* __LINUX_KVM_PARA_H */
diff --git a/include/uapi/linux/kvm_para.h b/include/uapi/linux/kvm_para.h
index cea2c5c..c41ddce 100644
--- a/include/uapi/linux/kvm_para.h
+++ b/include/uapi/linux/kvm_para.h
@@ -20,6 +20,12 @@
 #define KVM_HC_FEATURES			3
 #define KVM_HC_PPC_MAP_MAGIC_PAGE	4
 
+/* The bit of supported pv event */
+#define KVM_PV_FEATURE_PANICKED	0
+
+/* The pv event value */
+#define KVM_PV_EVENT_PANICKED	1
+
 /*
  * hypercalls use architecture specific
  */
diff --git a/kernel/panic.c b/kernel/panic.c
index e1b2822..a764d2e 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -23,6 +23,7 @@
 #include <linux/init.h>
 #include <linux/nmi.h>
 #include <linux/dmi.h>
+#include <linux/kvm_para.h>
 
 #define PANIC_TIMER_STEP 100
 #define PANIC_BLINK_SPD 18
@@ -132,6 +133,9 @@ void panic(const char *fmt, ...)
 	if (!panic_blink)
 		panic_blink = no_blink;
 
+	if (kvm_pv_event_enabled())
+		panic_timeout = 0;
+
 	if (panic_timeout > 0) {
 		/*
 		 * Delay timeout seconds before rebooting the machine.
-- 
1.8.0.1.240.ge8a1f5a

[PATCH v12 rebased 1/8] preserve cpu runstate

[Hu Tao]

This patch enables preservation of cpu runstate during save/load vm.
So when a vm is restored from snapshot, the cpu runstate is restored,
too.

See following example:

# save two vms: one is running, the other is paused
(qemu) info status
VM status: running
(qemu) savevm running
(qemu) stop
(qemu) info status
VM status: paused
(qemu) savevm paused

# restore the one running
(qemu) info status
VM status: paused
(qemu) loadvm running
(qemu) info status
VM status: running

# restore the one paused
(qemu) loadvm paused
(qemu) info status
VM status: paused
(qemu) cont
(qemu)info status
VM status: running


Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 include/sysemu/sysemu.h |  2 ++
 migration.c             |  6 +-----
 monitor.c               |  5 ++---
 savevm.c                |  1 +
 vl.c                    | 34 ++++++++++++++++++++++++++++++++++
 5 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 337ce7d..7a69fde 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -19,6 +19,8 @@ extern uint8_t qemu_uuid[];
 int qemu_uuid_parse(const char *str, uint8_t *uuid);
 #define UUID_FMT "%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
 
+void save_run_state(void);
+void load_run_state(void);
 bool runstate_check(RunState state);
 void runstate_set(RunState new_state);
 int runstate_is_running(void);
diff --git a/migration.c b/migration.c
index 77c1971..f96cfd6 100644
--- a/migration.c
+++ b/migration.c
@@ -108,11 +108,7 @@ static void process_incoming_migration_co(void *opaque)
     /* Make sure all file formats flush their mutable metadata */
     bdrv_invalidate_cache_all();
 
-    if (autostart) {
-        vm_start();
-    } else {
-        runstate_set(RUN_STATE_PAUSED);
-    }
+    load_run_state();
 }
 
 static void enter_migration_coroutine(void *opaque)
diff --git a/monitor.c b/monitor.c
index 20bd19b..9381ed0 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2059,13 +2059,12 @@ void qmp_closefd(const char *fdname, Error **errp)
 
 static void do_loadvm(Monitor *mon, const QDict *qdict)
 {
-    int saved_vm_running  = runstate_is_running();
     const char *name = qdict_get_str(qdict, "name");
 
     vm_stop(RUN_STATE_RESTORE_VM);
 
-    if (load_vmstate(name) == 0 && saved_vm_running) {
-        vm_start();
+    if (load_vmstate(name) == 0) {
+        load_run_state();
     }
 }
 
diff --git a/savevm.c b/savevm.c
index 304d1ef..10f1d56 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2112,6 +2112,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     }
 
     saved_vm_running = runstate_is_running();
+    save_run_state();
     vm_stop(RUN_STATE_SAVE_VM);
 
     memset(sn, 0, sizeof(*sn));
diff --git a/vl.c b/vl.c
index 4ee1302..b0bcf1e 100644
--- a/vl.c
+++ b/vl.c
@@ -520,6 +520,7 @@ static int default_driver_check(QemuOpts *opts, void *opaque)
 /* QEMU state */
 
 static RunState current_run_state = RUN_STATE_PRELAUNCH;
+static RunState saved_run_state = RUN_STATE_PRELAUNCH;
 
 typedef struct {
     RunState from;
@@ -543,6 +544,7 @@ static const RunStateTransition runstate_transitions_def[] = {
     { RUN_STATE_PAUSED, RUN_STATE_FINISH_MIGRATE },
 
     { RUN_STATE_POSTMIGRATE, RUN_STATE_RUNNING },
+    { RUN_STATE_POSTMIGRATE, RUN_STATE_PAUSED },
     { RUN_STATE_POSTMIGRATE, RUN_STATE_FINISH_MIGRATE },
 
     { RUN_STATE_PRELAUNCH, RUN_STATE_RUNNING },
@@ -553,6 +555,7 @@ static const RunStateTransition runstate_transitions_def[] = {
     { RUN_STATE_FINISH_MIGRATE, RUN_STATE_POSTMIGRATE },
 
     { RUN_STATE_RESTORE_VM, RUN_STATE_RUNNING },
+    { RUN_STATE_RESTORE_VM, RUN_STATE_PAUSED },
 
     { RUN_STATE_RUNNING, RUN_STATE_DEBUG },
     { RUN_STATE_RUNNING, RUN_STATE_INTERNAL_ERROR },
@@ -582,11 +585,39 @@ static const RunStateTransition runstate_transitions_def[] = {
 
 static bool runstate_valid_transitions[RUN_STATE_MAX][RUN_STATE_MAX];
 
+void save_run_state(void)
+{
+    saved_run_state = current_run_state;
+}
+
+void load_run_state(void)
+{
+    if (saved_run_state == RUN_STATE_RUNNING) {
+        vm_start();
+    } else if (!runstate_check(saved_run_state)) {
+        runstate_set(saved_run_state);
+    } else {
+        ; /* leave unchanged */
+    }
+}
+
 bool runstate_check(RunState state)
 {
     return current_run_state == state;
 }
 
+static void runstate_save(QEMUFile *f, void *opaque)
+{
+    qemu_put_byte(f, saved_run_state);
+}
+
+static int runstate_load(QEMUFile *f, void *opaque, int version_id)
+{
+    saved_run_state = qemu_get_byte(f);
+
+    return 0;
+}
+
 static void runstate_init(void)
 {
     const RunStateTransition *p;
@@ -596,6 +627,9 @@ static void runstate_init(void)
     for (p = &runstate_transitions_def[0]; p->from != RUN_STATE_MAX; p++) {
         runstate_valid_transitions[p->from][p->to] = true;
     }
+
+    register_savevm(NULL, "runstate", 0, 1,
+                    runstate_save, runstate_load, NULL);
 }
 
 /* This function will abort() on invalid state transitions */
-- 
1.8.0.1.240.ge8a1f5a

[PATCH v12 rebased 2/8] start vm after resetting it

[Hu Tao]

From: Wen Congyang <wency@cn.fujitsu.com>

The guest should run after resetting it, but it does not run if its
old state is RUN_STATE_INTERNAL_ERROR or RUN_STATE_PAUSED.

We don't set runstate to RUN_STATE_PAUSED when resetting the guest,
so the runstate will be changed from RUN_STATE_INTERNAL_ERROR or
RUN_STATE_PAUSED to RUN_STATE_RUNNING(not RUN_STATE_PAUSED).

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 include/block/block.h | 2 ++
 qmp.c                 | 2 +-
 vl.c                  | 7 ++++---
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index ffd1936..5e82ccb 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -366,6 +366,8 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs);
 void bdrv_set_in_use(BlockDriverState *bs, int in_use);
 int bdrv_in_use(BlockDriverState *bs);
 
+void iostatus_bdrv_it(void *opaque, BlockDriverState *bs);
+
 #ifdef CONFIG_LINUX_AIO
 int raw_get_aio_fd(BlockDriverState *bs);
 #else
diff --git a/qmp.c b/qmp.c
index 55b056b..5f1bed1 100644
--- a/qmp.c
+++ b/qmp.c
@@ -130,7 +130,7 @@ SpiceInfo *qmp_query_spice(Error **errp)
 };
 #endif
 
-static void iostatus_bdrv_it(void *opaque, BlockDriverState *bs)
+void iostatus_bdrv_it(void *opaque, BlockDriverState *bs)
 {
     bdrv_iostatus_reset(bs);
 }
diff --git a/vl.c b/vl.c
index b0bcf1e..1d2edaa 100644
--- a/vl.c
+++ b/vl.c
@@ -534,7 +534,7 @@ static const RunStateTransition runstate_transitions_def[] = {
     { RUN_STATE_INMIGRATE, RUN_STATE_RUNNING },
     { RUN_STATE_INMIGRATE, RUN_STATE_PAUSED },
 
-    { RUN_STATE_INTERNAL_ERROR, RUN_STATE_PAUSED },
+    { RUN_STATE_INTERNAL_ERROR, RUN_STATE_RUNNING },
     { RUN_STATE_INTERNAL_ERROR, RUN_STATE_FINISH_MIGRATE },
 
     { RUN_STATE_IO_ERROR, RUN_STATE_RUNNING },
@@ -569,7 +569,7 @@ static const RunStateTransition runstate_transitions_def[] = {
 
     { RUN_STATE_SAVE_VM, RUN_STATE_RUNNING },
 
-    { RUN_STATE_SHUTDOWN, RUN_STATE_PAUSED },
+    { RUN_STATE_SHUTDOWN, RUN_STATE_RUNNING },
     { RUN_STATE_SHUTDOWN, RUN_STATE_FINISH_MIGRATE },
 
     { RUN_STATE_DEBUG, RUN_STATE_SUSPENDED },
@@ -1951,7 +1951,8 @@ static bool main_loop_should_exit(void)
         resume_all_vcpus();
         if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
             runstate_check(RUN_STATE_SHUTDOWN)) {
-            runstate_set(RUN_STATE_PAUSED);
+            bdrv_iterate(iostatus_bdrv_it, NULL);
+            vm_start();
         }
     }
     if (qemu_wakeup_requested()) {
-- 
1.8.0.1.240.ge8a1f5a

[PATCH v12 rebased 3/8] update kernel headers

[Hu Tao]

update kernel headers to add pv event macros.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 linux-headers/asm-x86/kvm_para.h | 1 +
 linux-headers/linux/kvm_para.h   | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/linux-headers/asm-x86/kvm_para.h b/linux-headers/asm-x86/kvm_para.h
index a1c3d72..781959a 100644
--- a/linux-headers/asm-x86/kvm_para.h
+++ b/linux-headers/asm-x86/kvm_para.h
@@ -96,5 +96,6 @@ struct kvm_vcpu_pv_apf_data {
 #define KVM_PV_EOI_ENABLED KVM_PV_EOI_MASK
 #define KVM_PV_EOI_DISABLED 0x0
 
+#define KVM_PV_EVENT_PORT	(0x505UL)
 
 #endif /* _ASM_X86_KVM_PARA_H */
diff --git a/linux-headers/linux/kvm_para.h b/linux-headers/linux/kvm_para.h
index 7bdcf93..f6be0bb 100644
--- a/linux-headers/linux/kvm_para.h
+++ b/linux-headers/linux/kvm_para.h
@@ -20,6 +20,12 @@
 #define KVM_HC_FEATURES			3
 #define KVM_HC_PPC_MAP_MAGIC_PAGE	4
 
+/* The bit of supported pv event */
+#define KVM_PV_FEATURE_PANICKED	0
+
+/* The pv event value */
+#define KVM_PV_EVENT_PANICKED	1
+
 /*
  * hypercalls use architecture specific
  */
-- 
1.8.0.1.240.ge8a1f5a

[PATCH v12 rebased 4/8] add a new runstate: RUN_STATE_GUEST_PANICKED

[Hu Tao]

The guest will be in this state when it is panicked.

If guest is panicked during live migration, the runstate
RUN_STATE_GUEST_PANICKED will be transferred to dest machine.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 migration.c      |  1 +
 qapi-schema.json |  6 +++++-
 qmp.c            |  3 ++-
 vl.c             | 11 ++++++++++-
 4 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/migration.c b/migration.c
index f96cfd6..2b51913 100644
--- a/migration.c
+++ b/migration.c
@@ -705,6 +705,7 @@ static void *buffered_file_thread(void *opaque)
                 int64_t start_time, end_time;
 
                 DPRINTF("done iterating\n");
+                save_run_state();
                 start_time = qemu_get_clock_ms(rt_clock);
                 qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
                 if (old_vm_running) {
diff --git a/qapi-schema.json b/qapi-schema.json
index 6d7252b..b49094b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -174,11 +174,15 @@
 # @suspended: guest is suspended (ACPI S3)
 #
 # @watchdog: the watchdog action is configured to pause and has been triggered
+#
+# @guest-panicked: the panicked action is configured to pause and has been
+# triggered.
 ##
 { 'enum': 'RunState',
   'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
             'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
-            'running', 'save-vm', 'shutdown', 'suspended', 'watchdog' ] }
+            'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
+            'guest-panicked' ] }
 
 ##
 # @SnapshotInfo
diff --git a/qmp.c b/qmp.c
index 5f1bed1..f5027f6 100644
--- a/qmp.c
+++ b/qmp.c
@@ -150,7 +150,8 @@ void qmp_cont(Error **errp)
     Error *local_err = NULL;
 
     if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
-               runstate_check(RUN_STATE_SHUTDOWN)) {
+            runstate_check(RUN_STATE_SHUTDOWN) ||
+            runstate_check(RUN_STATE_GUEST_PANICKED)) {
         error_set(errp, QERR_RESET_REQUIRED);
         return;
     } else if (runstate_check(RUN_STATE_SUSPENDED)) {
diff --git a/vl.c b/vl.c
index 1d2edaa..5aae03f 100644
--- a/vl.c
+++ b/vl.c
@@ -533,6 +533,7 @@ static const RunStateTransition runstate_transitions_def[] = {
 
     { RUN_STATE_INMIGRATE, RUN_STATE_RUNNING },
     { RUN_STATE_INMIGRATE, RUN_STATE_PAUSED },
+    { RUN_STATE_INMIGRATE, RUN_STATE_GUEST_PANICKED },
 
     { RUN_STATE_INTERNAL_ERROR, RUN_STATE_RUNNING },
     { RUN_STATE_INTERNAL_ERROR, RUN_STATE_FINISH_MIGRATE },
@@ -546,6 +547,7 @@ static const RunStateTransition runstate_transitions_def[] = {
     { RUN_STATE_POSTMIGRATE, RUN_STATE_RUNNING },
     { RUN_STATE_POSTMIGRATE, RUN_STATE_PAUSED },
     { RUN_STATE_POSTMIGRATE, RUN_STATE_FINISH_MIGRATE },
+    { RUN_STATE_POSTMIGRATE, RUN_STATE_GUEST_PANICKED },
 
     { RUN_STATE_PRELAUNCH, RUN_STATE_RUNNING },
     { RUN_STATE_PRELAUNCH, RUN_STATE_FINISH_MIGRATE },
@@ -556,6 +558,7 @@ static const RunStateTransition runstate_transitions_def[] = {
 
     { RUN_STATE_RESTORE_VM, RUN_STATE_RUNNING },
     { RUN_STATE_RESTORE_VM, RUN_STATE_PAUSED },
+    { RUN_STATE_RESTORE_VM, RUN_STATE_GUEST_PANICKED },
 
     { RUN_STATE_RUNNING, RUN_STATE_DEBUG },
     { RUN_STATE_RUNNING, RUN_STATE_INTERNAL_ERROR },
@@ -566,6 +569,7 @@ static const RunStateTransition runstate_transitions_def[] = {
     { RUN_STATE_RUNNING, RUN_STATE_SAVE_VM },
     { RUN_STATE_RUNNING, RUN_STATE_SHUTDOWN },
     { RUN_STATE_RUNNING, RUN_STATE_WATCHDOG },
+    { RUN_STATE_RUNNING, RUN_STATE_GUEST_PANICKED },
 
     { RUN_STATE_SAVE_VM, RUN_STATE_RUNNING },
 
@@ -580,6 +584,10 @@ static const RunStateTransition runstate_transitions_def[] = {
     { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
     { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
 
+    { RUN_STATE_GUEST_PANICKED, RUN_STATE_RUNNING },
+    { RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED },
+    { RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE },
+
     { RUN_STATE_MAX, RUN_STATE_MAX },
 };
 
@@ -1950,7 +1958,8 @@ static bool main_loop_should_exit(void)
         qemu_system_reset(VMRESET_REPORT);
         resume_all_vcpus();
         if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
-            runstate_check(RUN_STATE_SHUTDOWN)) {
+            runstate_check(RUN_STATE_SHUTDOWN) ||
+            runstate_check(RUN_STATE_GUEST_PANICKED)) {
             bdrv_iterate(iostatus_bdrv_it, NULL);
             vm_start();
         }
-- 
1.8.0.1.240.ge8a1f5a

[PATCH v12 rebased 5/8] add a new qevent: QEVENT_GUEST_PANICKED

[Hu Tao]

This event will be emited when the guest is panicked.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 include/monitor/monitor.h | 1 +
 monitor.c                 | 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 87fb49c..4006905 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -45,6 +45,7 @@ typedef enum MonitorEvent {
     QEVENT_WAKEUP,
     QEVENT_BALLOON_CHANGE,
     QEVENT_SPICE_MIGRATE_COMPLETED,
+    QEVENT_GUEST_PANICKED,
 
     /* Add to 'monitor_event_names' array in monitor.c when
      * defining new events here */
diff --git a/monitor.c b/monitor.c
index 9381ed0..61beeb4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -463,6 +463,7 @@ static const char *monitor_event_names[] = {
     [QEVENT_WAKEUP] = "WAKEUP",
     [QEVENT_BALLOON_CHANGE] = "BALLOON_CHANGE",
     [QEVENT_SPICE_MIGRATE_COMPLETED] = "SPICE_MIGRATE_COMPLETED",
+    [QEVENT_GUEST_PANICKED] = "GUEST_PANICKED",
 };
 QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX)
 
-- 
1.8.0.1.240.ge8a1f5a

[PATCH v12 rebased 6/8] introduce a new qom device to deal with panicked event

[Hu Tao]

If the target is x86/x86_64, the guest's kernel will write 0x01 to the
port KVM_PV_EVENT_PORT when it is panciked. This patch introduces a new
qom device kvm_pv_ioport to listen this I/O port, and deal with panicked
event according to panicked_action's value. The possible actions are:
1. emit QEVENT_GUEST_PANICKED only
2. emit QEVENT_GUEST_PANICKED and pause the guest
3. emit QEVENT_GUEST_PANICKED and poweroff the guest
4. emit QEVENT_GUEST_PANICKED and reset the guest

I/O ports does not work for some targets(for example: s390). And you
can implement another qom device, and include it's code into pv_event.c
for such target.

Note: if we emit QEVENT_GUEST_PANICKED only, and the management
application does not receive this event(the management may not
run when the event is emitted), the management won't know the
guest is panicked.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 hw/kvm/Makefile.objs |   2 +-
 hw/kvm/pv_event.c    | 197 +++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pc_piix.c         |   5 ++
 include/sysemu/kvm.h |   2 +
 kvm-stub.c           |   4 ++
 5 files changed, 209 insertions(+), 1 deletion(-)
 create mode 100644 hw/kvm/pv_event.c

diff --git a/hw/kvm/Makefile.objs b/hw/kvm/Makefile.objs
index f620d7f..cf93199 100644
--- a/hw/kvm/Makefile.objs
+++ b/hw/kvm/Makefile.objs
@@ -1 +1 @@
-obj-$(CONFIG_KVM) += clock.o apic.o i8259.o ioapic.o i8254.o pci-assign.o
+obj-$(CONFIG_KVM) += clock.o apic.o i8259.o ioapic.o i8254.o pci-assign.o pv_event.o
diff --git a/hw/kvm/pv_event.c b/hw/kvm/pv_event.c
new file mode 100644
index 0000000..f32f82e
--- /dev/null
+++ b/hw/kvm/pv_event.c
@@ -0,0 +1,197 @@
+/*
+ * QEMU KVM support, paravirtual event device
+ *
+ * Copyright Fujitsu, Corp. 2012
+ *
+ * Authors:
+ *     Wen Congyang <wency@cn.fujitsu.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include <linux/kvm_para.h>
+#include <asm/kvm_para.h>
+#include <qapi/qmp/qobject.h>
+#include <qapi/qmp/qjson.h>
+#include <monitor/monitor.h>
+#include <sysemu/sysemu.h>
+#include <sysemu/kvm.h>
+
+/* Possible values for action parameter. */
+#define PANICKED_REPORT     1   /* emit QEVENT_GUEST_PANICKED only */
+#define PANICKED_PAUSE      2   /* emit QEVENT_GUEST_PANICKED and pause VM */
+#define PANICKED_POWEROFF   3   /* emit QEVENT_GUEST_PANICKED and quit VM */
+#define PANICKED_RESET      4   /* emit QEVENT_GUEST_PANICKED and reset VM */
+
+#define PV_EVENT_DRIVER     "kvm_pv_event"
+
+struct PVEventAction {
+    char *panicked_action;
+    int panicked_action_value;
+};
+
+#define DEFINE_PV_EVENT_PROPERTIES(_state, _conf)   \
+    DEFINE_PROP_STRING("panicked_action", _state, _conf.panicked_action)
+
+static void panicked_mon_event(const char *action)
+{
+    QObject *data;
+
+    data = qobject_from_jsonf("{ 'action': %s }", action);
+    monitor_protocol_event(QEVENT_GUEST_PANICKED, data);
+    qobject_decref(data);
+}
+
+static void panicked_perform_action(uint32_t panicked_action)
+{
+    switch (panicked_action) {
+    case PANICKED_REPORT:
+        panicked_mon_event("report");
+        break;
+
+    case PANICKED_PAUSE:
+        panicked_mon_event("pause");
+        vm_stop(RUN_STATE_GUEST_PANICKED);
+        break;
+
+    case PANICKED_POWEROFF:
+        panicked_mon_event("poweroff");
+        qemu_system_shutdown_request();
+        break;
+
+    case PANICKED_RESET:
+        panicked_mon_event("reset");
+        qemu_system_reset_request();
+        break;
+    }
+}
+
+static uint64_t supported_event(void)
+{
+    return 1 << KVM_PV_FEATURE_PANICKED;
+}
+
+static void handle_event(int event, struct PVEventAction *conf)
+{
+    if (event == KVM_PV_EVENT_PANICKED) {
+        panicked_perform_action(conf->panicked_action_value);
+    }
+}
+
+static int pv_event_init(struct PVEventAction *conf)
+{
+    if (!conf->panicked_action) {
+        conf->panicked_action_value = PANICKED_REPORT;
+    } else if (strcasecmp(conf->panicked_action, "none") == 0) {
+        conf->panicked_action_value = PANICKED_REPORT;
+    } else if (strcasecmp(conf->panicked_action, "pause") == 0) {
+        conf->panicked_action_value = PANICKED_PAUSE;
+    } else if (strcasecmp(conf->panicked_action, "poweroff") == 0) {
+        conf->panicked_action_value = PANICKED_POWEROFF;
+    } else if (strcasecmp(conf->panicked_action, "reset") == 0) {
+        conf->panicked_action_value = PANICKED_RESET;
+    } else {
+        return -1;
+    }
+
+    return 0;
+}
+
+#if defined(KVM_PV_EVENT_PORT)
+
+#include "hw/isa.h"
+
+typedef struct {
+    ISADevice dev;
+    struct PVEventAction conf;
+    MemoryRegion ioport;
+} PVIOPortState;
+
+static uint64_t pv_io_read(void *opaque, hwaddr addr, unsigned size)
+{
+    return supported_event();
+}
+
+static void pv_io_write(void *opaque, hwaddr addr, uint64_t val,
+                        unsigned size)
+{
+    PVIOPortState *s = opaque;
+
+    handle_event(val, &s->conf);
+}
+
+static const MemoryRegionOps pv_io_ops = {
+    .read = pv_io_read,
+    .write = pv_io_write,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
+
+static int pv_ioport_initfn(ISADevice *dev)
+{
+    PVIOPortState *s = DO_UPCAST(PVIOPortState, dev, dev);
+
+    if (pv_event_init(&s->conf) < 0) {
+        return -1;
+    }
+
+    memory_region_init_io(&s->ioport, &pv_io_ops, s, "pv_event", 1);
+    isa_register_ioport(dev, &s->ioport, KVM_PV_EVENT_PORT);
+
+    return 0;
+}
+
+static Property pv_ioport_properties[] = {
+    DEFINE_PV_EVENT_PROPERTIES(PVIOPortState, conf),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void pv_ioport_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    ISADeviceClass *ic = ISA_DEVICE_CLASS(klass);
+
+    ic->init = pv_ioport_initfn;
+    dc->no_user = 1;
+    dc->props = pv_ioport_properties;
+}
+
+static TypeInfo pv_ioport_info = {
+    .name          = PV_EVENT_DRIVER,
+    .parent        = TYPE_ISA_DEVICE,
+    .instance_size = sizeof(PVIOPortState),
+    .class_init    = pv_ioport_class_init,
+};
+
+static void pv_ioport_register_types(void)
+{
+    type_register_static(&pv_ioport_info);
+}
+
+type_init(pv_ioport_register_types)
+
+void kvm_pv_event_init(void *opaque)
+{
+    ISABus *bus = opaque;
+    ISADevice *dev;
+
+    dev = isa_create(bus, PV_EVENT_DRIVER);
+    qdev_init_nofail(&dev->qdev);
+}
+
+#else
+
+/*
+ * If you need a qom device to handle pv event, this device should
+ * be created and initialized in kvm_pv_event_init().
+ *
+ * The parameter opaque is the device's parent bus.
+ */
+void kvm_pv_event_init(void *opaque)
+{
+}
+#endif
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 0a6923d..fed6ccf 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -47,6 +47,7 @@
 #ifdef CONFIG_XEN
 #  include <xen/hvm/hvm_info_table.h>
 #endif
+#include <linux/kvm_para.h>
 
 #define MAX_IDE_BUS 2
 
@@ -216,6 +217,10 @@ static void pc_init1(MemoryRegion *system_memory,
     if (pci_enabled) {
         pc_pci_device_init(pci_bus);
     }
+
+    if (kvm_enabled()) {
+        kvm_pv_event_init(isa_bus);
+    }
 }
 
 static void pc_init_pci(QEMUMachineInitArgs *args)
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 6bdd513..63f466f 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -292,4 +292,6 @@ int kvm_irqchip_add_irqfd_notifier(KVMState *s, EventNotifier *n, int virq);
 int kvm_irqchip_remove_irqfd_notifier(KVMState *s, EventNotifier *n, int virq);
 void kvm_pc_gsi_handler(void *opaque, int n, int level);
 void kvm_pc_setup_irq_routing(bool pci_enabled);
+
+void kvm_pv_event_init(void *opaque);
 #endif
diff --git a/kvm-stub.c b/kvm-stub.c
index 47f8dca..fe5ad22 100644
--- a/kvm-stub.c
+++ b/kvm-stub.c
@@ -145,3 +145,7 @@ int kvm_irqchip_remove_irqfd_notifier(KVMState *s, EventNotifier *n, int virq)
 {
     return -ENOSYS;
 }
+
+void kvm_pv_event_init(void *opaque)
+{
+}
-- 
1.8.0.1.240.ge8a1f5a

[PATCH v12 rebased 7/8] allower the user to disable pv event support

[Hu Tao]

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 hw/pc_piix.c    | 9 ++++++++-
 qemu-options.hx | 3 ++-
 vl.c            | 4 ++++
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index fed6ccf..507c98b 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -44,6 +44,7 @@
 #include "exec/memory.h"
 #include "exec/address-spaces.h"
 #include "cpu.h"
+#include "qemu/config-file.h"
 #ifdef CONFIG_XEN
 #  include <xen/hvm/hvm_info_table.h>
 #endif
@@ -86,6 +87,8 @@ static void pc_init1(MemoryRegion *system_memory,
     MemoryRegion *pci_memory;
     MemoryRegion *rom_memory;
     void *fw_cfg = NULL;
+    QemuOptsList *list = qemu_find_opts("machine");
+    bool enable_pv_event = false;
 
     pc_cpus_init(cpu_model);
     pc_acpi_init("acpi-dsdt.aml");
@@ -218,7 +221,11 @@ static void pc_init1(MemoryRegion *system_memory,
         pc_pci_device_init(pci_bus);
     }
 
-    if (kvm_enabled()) {
+    if (list && !QTAILQ_EMPTY(&list->head)) {
+        enable_pv_event = qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
+                                            "enable_pv_event", false);
+    }
+    if (kvm_enabled() && enable_pv_event) {
         kvm_pv_event_init(isa_bus);
     }
 }
diff --git a/qemu-options.hx b/qemu-options.hx
index 4e2b499..7522f4a 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -35,7 +35,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
     "                kernel_irqchip=on|off controls accelerated irqchip support\n"
     "                kvm_shadow_mem=size of KVM shadow MMU\n"
     "                dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
-    "                mem-merge=on|off controls memory merge support (default: on)\n",
+    "                mem-merge=on|off controls memory merge support (default: on)\n"
+    "                enable_pv_event=on|off controls pv event support (default: off)\n",
     QEMU_ARCH_ALL)
 STEXI
 @item -machine [type=]@var{name}[,prop=@var{value}[,...]]
diff --git a/vl.c b/vl.c
index 5aae03f..aa15b23 100644
--- a/vl.c
+++ b/vl.c
@@ -424,6 +424,10 @@ static QemuOptsList qemu_machine_opts = {
             .name = "usb",
             .type = QEMU_OPT_BOOL,
             .help = "Set on/off to enable/disable usb",
+        }, {
+            .name = "enable_pv_event",
+            .type = QEMU_OPT_BOOL,
+            .help = "handle pv event"
         },
         { /* End of list */ }
     },
-- 
1.8.0.1.240.ge8a1f5a

[PATCH v12 rebased 8/8] pv event: add document to describe the usage

[Hu Tao]

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 docs/pv-event.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 docs/pv-event.txt

diff --git a/docs/pv-event.txt b/docs/pv-event.txt
new file mode 100644
index 0000000..ac9e7fa
--- /dev/null
+++ b/docs/pv-event.txt
@@ -0,0 +1,17 @@
+KVM PV EVENT
+============
+
+kvm pv event allows guest OS to notify host OS of some events, for
+example, guest panic. Currently, there is one event supported, that
+is, guest panic. More events can be added later.
+
+By default, kvm pv event is disabled. In order to enable it, you have
+to specify enable_pv_event=on for -machine command line option, along
+with -global kvm_pv_event.panicked_action to specify the action taken
+when panic event has occurred. Aviable panic actions are: "none",
+"pause", "poweroff" and "reset". Following is example:
+
+  qemu-system-x86_64 -enable-kvm -machine pc-0.12,enable_pv_event=on \
+    -global kvm_pv_event.panicked_action=pause <other options>
+
+kvm pv event needs kvm support.
-- 
1.8.0.1.240.ge8a1f5a

Re: [PATCH v12 rebased] kvm: notify host when the guest is panicked

[Marcelo Tosatti]

Hi,

On Wed, Jan 23, 2013 at 03:19:21PM +0800, Hu Tao wrote:
> We can know the guest is panicked when the guest runs on xen.
> But we do not have such feature on kvm.
> 
> Another purpose of this feature is: management app(for example:
> libvirt) can do auto dump when the guest is panicked. If management
> app does not do auto dump, the guest's user can do dump by hand if
> he sees the guest is panicked.
> 
> We have three solutions to implement this feature:
> 1. use vmcall
> 2. use I/O port
> 3. use virtio-serial.
> 
> We have decided to avoid touching hypervisor. The reason why I choose
> choose the I/O port is:
> 1. it is easier to implememt
> 2. it does not depend any virtual device
> 3. it can work when starting the kernel
> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  arch/ia64/kvm/irq.h                  | 19 +++++++++++++
>  arch/powerpc/include/asm/kvm_para.h  | 18 ++++++++++++
>  arch/s390/include/asm/kvm_para.h     | 19 +++++++++++++
>  arch/x86/include/asm/kvm_para.h      | 20 ++++++++++++++
>  arch/x86/include/uapi/asm/kvm_para.h |  2 ++
>  arch/x86/kernel/kvm.c                | 53 ++++++++++++++++++++++++++++++++++++
>  include/linux/kvm_para.h             | 18 ++++++++++++
>  include/uapi/linux/kvm_para.h        |  6 ++++
>  kernel/panic.c                       |  4 +++
>  9 files changed, 159 insertions(+)
> 
> diff --git a/arch/ia64/kvm/irq.h b/arch/ia64/kvm/irq.h
> index c0785a7..b3870f8 100644
> --- a/arch/ia64/kvm/irq.h
> +++ b/arch/ia64/kvm/irq.h
> @@ -30,4 +30,23 @@ static inline int irqchip_in_kernel(struct kvm *kvm)
>  	return 1;
>  }
>  
> +static inline int kvm_arch_pv_event_init(void)
> +{
> +	return 0;
> +}
> +
> +static inline unsigned int kvm_arch_pv_features(void)
> +{
> +	return 0;
> +}
> +
> +static inline void kvm_arch_pv_eject_event(unsigned int event)
> +{
> +}
> +
> +static inline bool kvm_arch_pv_event_enabled(void)
> +{
> +	return false;
> +}
> +

The interface is x86 only, no need to touch other architectures.

>  #endif
> diff --git a/arch/powerpc/include/asm/kvm_para.h b/arch/powerpc/include/asm/kvm_para.h
> index 2b11965..17dd013 100644
> --- a/arch/powerpc/include/asm/kvm_para.h
> +++ b/arch/powerpc/include/asm/kvm_para.h
> @@ -144,4 +144,22 @@ static inline bool kvm_check_and_clear_guest_paused(void)
>  	return false;
>  }
>  
> +static inline int kvm_arch_pv_event_init(void)
> +{
> +	return 0;
> +}
> +
> +static inline unsigned int kvm_arch_pv_features(void)
> +{
> +	return 0;
> +}
> +
> +static inline void kvm_arch_pv_eject_event(unsigned int event)
> +{
> +}
> +
> +static inline bool kvm_arch_pv_event_enabled(void)
> +{
> +	return false;
> +}
>  #endif /* __POWERPC_KVM_PARA_H__ */
> diff --git a/arch/s390/include/asm/kvm_para.h b/arch/s390/include/asm/kvm_para.h
> index e0f8423..81d87ec 100644
> --- a/arch/s390/include/asm/kvm_para.h
> +++ b/arch/s390/include/asm/kvm_para.h
> @@ -154,4 +154,23 @@ static inline bool kvm_check_and_clear_guest_paused(void)
>  	return false;
>  }
>  
> +static inline int kvm_arch_pv_event_init(void)
> +{
> +	return 0;
> +}
> +
> +static inline unsigned int kvm_arch_pv_features(void)
> +{
> +	return 0;
> +}
> +
> +static inline void kvm_arch_pv_eject_event(unsigned int event)
> +{
> +}
> +
> +static inline bool kvm_arch_pv_event_enabled(void)
> +{
> +	return false;
> +}
> +
>  #endif /* __S390_KVM_PARA_H */
> --- a/arch/x86/include/asm/kvm_para.h
> +++ b/arch/x86/include/asm/kvm_para.h
> @@ -133,4 +133,24 @@ static inline void kvm_disable_steal_time(void)
>  }
>  #endif
>  
> +static inline int kvm_arch_pv_event_init(void)
> +{
> +	if (!request_region(KVM_PV_EVENT_PORT, 4, "KVM_PV_EVENT"))
> +		return -1;
> +
> +	return 0;
> +}

This should be in a driver in arch/x86/kernel/kvm-panic.c, or so.

> +
> +static inline unsigned int kvm_arch_pv_features(void)
> +{
> +	return inl(KVM_PV_EVENT_PORT);
> +}
> +
> +static inline void kvm_arch_pv_eject_event(unsigned int event)
> +{
> +	outl(event, KVM_PV_EVENT_PORT);
> +}

> +
> +bool kvm_arch_pv_event_enabled(void);
> +
>  #endif /* _ASM_X86_KVM_PARA_H */
> diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h
> index 06fdbd9..c15ef33 100644
> --- a/arch/x86/include/uapi/asm/kvm_para.h
> +++ b/arch/x86/include/uapi/asm/kvm_para.h
> @@ -96,5 +96,7 @@ struct kvm_vcpu_pv_apf_data {
>  #define KVM_PV_EOI_ENABLED KVM_PV_EOI_MASK
>  #define KVM_PV_EOI_DISABLED 0x0
>  
> +#define KVM_PV_EVENT_PORT	(0x505UL)
> +

No need for the ioport to be hard coded. What are the options to
communicate an address to the guest? An MSR, via ACPI?

>  
>  #endif /* _UAPI_ASM_X86_KVM_PARA_H */
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 9c2bd8b..0aa7b3e 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -73,6 +73,20 @@ static int parse_no_kvmclock_vsyscall(char *arg)
>  
>  early_param("no-kvmclock-vsyscall", parse_no_kvmclock_vsyscall);
>  
> +static int pv_event = 1;
> +static int parse_no_pv_event(char *arg)
> +{
> +	pv_event = 0;
> +	return 0;
> +}
> +
> +bool kvm_arch_pv_event_enabled(void)
> +{
> +	return !!pv_event;
> +}
> +
> +early_param("no-pv-event", parse_no_pv_event);
> +

"pv-event" is a bad name for an interface which is specific to notify
panic events. Please use pv-panic everywhere.

>  static DEFINE_PER_CPU(struct kvm_vcpu_pv_apf_data, apf_reason) __aligned(64);
>  static DEFINE_PER_CPU(struct kvm_steal_time, steal_time) __aligned(64);
>  static int has_steal_clock = 0;
> @@ -385,6 +399,17 @@ static struct notifier_block kvm_pv_reboot_nb = {
>  	.notifier_call = kvm_pv_reboot_notify,
>  };
>  
> +static int
> +kvm_pv_panic_notify(struct notifier_block *nb, unsigned long code, void *unused)
> +{
> +	kvm_pv_eject_event(KVM_PV_EVENT_PANICKED);
> +	return NOTIFY_DONE;
> +}

Why 'eject' ?

> +
> +static struct notifier_block kvm_pv_panic_nb = {
> +	.notifier_call = kvm_pv_panic_notify,
> +};
> +
>  static u64 kvm_steal_clock(int cpu)
>  {
>  	u64 steal;
> @@ -462,6 +487,34 @@ static void __init kvm_apf_trap_init(void)
>  	set_intr_gate(14, &async_page_fault);
>  }
>  
> +static void __init kvm_pv_panicked_event_init(void)
> +{
> +	if (!kvm_para_available())
> +		return;
> +
> +	if (kvm_pv_has_feature(KVM_PV_FEATURE_PANICKED))
> +		atomic_notifier_chain_register(&panic_notifier_list,
> +			&kvm_pv_panic_nb);
> +}
> +
> +static inline int kvm_pv_event_init(void)
> +{
> +	return kvm_arch_pv_event_init();
> +}
> +
> +static int __init enable_pv_event(void)
> +{
> +	if (pv_event) {
> +		if (kvm_pv_event_init())
> +			return 0;
> +
> +		kvm_pv_panicked_event_init();
> +	}
> +
> +	return 0;
> +}
> +arch_initcall(enable_pv_event);

Call the initialization code from kvm_guest_init, only one function is
necessary.

> +
>  void __init kvm_guest_init(void)
>  {
>  	int i;
> diff --git a/include/linux/kvm_para.h b/include/linux/kvm_para.h
> index 00a97bb..6fb6198 100644
> --- a/include/linux/kvm_para.h
> +++ b/include/linux/kvm_para.h
> @@ -10,4 +10,22 @@ static inline int kvm_para_has_feature(unsigned int feature)
>  		return 1;
>  	return 0;
>  }
> +
> +static inline int kvm_pv_has_feature(unsigned int feature)
> +{
> +	if (kvm_arch_pv_features() & (1UL << feature))
> +		return 1;
> +	return 0;
> +}
> +
> +static inline void kvm_pv_eject_event(unsigned int event)
> +{
> +	kvm_arch_pv_eject_event(event);
> +}
> +
> +static inline bool kvm_pv_event_enabled(void)
> +{
> +	return kvm_arch_pv_event_enabled();
> +}

No need for this helpers, as noted.

>  #endif /* __LINUX_KVM_PARA_H */
> diff --git a/include/uapi/linux/kvm_para.h b/include/uapi/linux/kvm_para.h
> index cea2c5c..c41ddce 100644
> --- a/include/uapi/linux/kvm_para.h
> +++ b/include/uapi/linux/kvm_para.h
> @@ -20,6 +20,12 @@
>  #define KVM_HC_FEATURES			3
>  #define KVM_HC_PPC_MAP_MAGIC_PAGE	4
>  
> +/* The bit of supported pv event */
> +#define KVM_PV_FEATURE_PANICKED	0
> +
> +/* The pv event value */
> +#define KVM_PV_EVENT_PANICKED	1
> +

This is a hypercall header. You want

arch/x86/include/asm/kvm_para.h

>  /*
>   * hypercalls use architecture specific
>   */
> diff --git a/kernel/panic.c b/kernel/panic.c
> index e1b2822..a764d2e 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -23,6 +23,7 @@
>  #include <linux/init.h>
>  #include <linux/nmi.h>
>  #include <linux/dmi.h>
> +#include <linux/kvm_para.h>
>  
>  #define PANIC_TIMER_STEP 100
>  #define PANIC_BLINK_SPD 18
> @@ -132,6 +133,9 @@ void panic(const char *fmt, ...)
>  	if (!panic_blink)
>  		panic_blink = no_blink;
>  
> +	if (kvm_pv_event_enabled())
> +		panic_timeout = 0;
> +

What is the rationale behind this?

Re: [PATCH v12 rebased 1/8] preserve cpu runstate

[Marcelo Tosatti]

On Wed, Jan 23, 2013 at 03:19:22PM +0800, Hu Tao wrote:
> This patch enables preservation of cpu runstate during save/load vm.
> So when a vm is restored from snapshot, the cpu runstate is restored,
> too.
> 
> See following example:
> 
> # save two vms: one is running, the other is paused
> (qemu) info status
> VM status: running
> (qemu) savevm running
> (qemu) stop
> (qemu) info status
> VM status: paused
> (qemu) savevm paused
> 
> # restore the one running
> (qemu) info status
> VM status: paused
> (qemu) loadvm running
> (qemu) info status
> VM status: running
> 
> # restore the one paused
> (qemu) loadvm paused
> (qemu) info status
> VM status: paused
> (qemu) cont
> (qemu)info status
> VM status: running
> 
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>

Lack of pause state on guest images is annoying. 

Fail to see why the panic feature depends on preservation of cpu
runstate.

>  include/sysemu/sysemu.h |  2 ++
>  migration.c             |  6 +-----
>  monitor.c               |  5 ++---
>  savevm.c                |  1 +
>  vl.c                    | 34 ++++++++++++++++++++++++++++++++++
>  5 files changed, 40 insertions(+), 8 deletions(-)
> 
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 337ce7d..7a69fde 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -19,6 +19,8 @@ extern uint8_t qemu_uuid[];
>  int qemu_uuid_parse(const char *str, uint8_t *uuid);
>  #define UUID_FMT "%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
>  
> +void save_run_state(void);
> +void load_run_state(void);
>  bool runstate_check(RunState state);
>  void runstate_set(RunState new_state);
>  int runstate_is_running(void);
> diff --git a/migration.c b/migration.c
> index 77c1971..f96cfd6 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -108,11 +108,7 @@ static void process_incoming_migration_co(void *opaque)
>      /* Make sure all file formats flush their mutable metadata */
>      bdrv_invalidate_cache_all();
>  
> -    if (autostart) {
> -        vm_start();
> -    } else {
> -        runstate_set(RUN_STATE_PAUSED);
> -    }
> +    load_run_state();
>  }
>  
>  static void enter_migration_coroutine(void *opaque)
> diff --git a/monitor.c b/monitor.c
> index 20bd19b..9381ed0 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2059,13 +2059,12 @@ void qmp_closefd(const char *fdname, Error **errp)
>  
>  static void do_loadvm(Monitor *mon, const QDict *qdict)
>  {
> -    int saved_vm_running  = runstate_is_running();
>      const char *name = qdict_get_str(qdict, "name");
>  
>      vm_stop(RUN_STATE_RESTORE_VM);
>  
> -    if (load_vmstate(name) == 0 && saved_vm_running) {
> -        vm_start();
> +    if (load_vmstate(name) == 0) {
> +        load_run_state();
>      }
>  }
>  
> diff --git a/savevm.c b/savevm.c
> index 304d1ef..10f1d56 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2112,6 +2112,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>      }
>  
>      saved_vm_running = runstate_is_running();
> +    save_run_state();
>      vm_stop(RUN_STATE_SAVE_VM);
>  
>      memset(sn, 0, sizeof(*sn));
> diff --git a/vl.c b/vl.c
> index 4ee1302..b0bcf1e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -520,6 +520,7 @@ static int default_driver_check(QemuOpts *opts, void *opaque)
>  /* QEMU state */
>  
>  static RunState current_run_state = RUN_STATE_PRELAUNCH;
> +static RunState saved_run_state = RUN_STATE_PRELAUNCH;
>  
>  typedef struct {
>      RunState from;
> @@ -543,6 +544,7 @@ static const RunStateTransition runstate_transitions_def[] = {
>      { RUN_STATE_PAUSED, RUN_STATE_FINISH_MIGRATE },
>  
>      { RUN_STATE_POSTMIGRATE, RUN_STATE_RUNNING },
> +    { RUN_STATE_POSTMIGRATE, RUN_STATE_PAUSED },
>      { RUN_STATE_POSTMIGRATE, RUN_STATE_FINISH_MIGRATE },
>  
>      { RUN_STATE_PRELAUNCH, RUN_STATE_RUNNING },
> @@ -553,6 +555,7 @@ static const RunStateTransition runstate_transitions_def[] = {
>      { RUN_STATE_FINISH_MIGRATE, RUN_STATE_POSTMIGRATE },
>  
>      { RUN_STATE_RESTORE_VM, RUN_STATE_RUNNING },
> +    { RUN_STATE_RESTORE_VM, RUN_STATE_PAUSED },
>  
>      { RUN_STATE_RUNNING, RUN_STATE_DEBUG },
>      { RUN_STATE_RUNNING, RUN_STATE_INTERNAL_ERROR },
> @@ -582,11 +585,39 @@ static const RunStateTransition runstate_transitions_def[] = {
>  
>  static bool runstate_valid_transitions[RUN_STATE_MAX][RUN_STATE_MAX];
>  
> +void save_run_state(void)
> +{
> +    saved_run_state = current_run_state;
> +}
> +
> +void load_run_state(void)
> +{
> +    if (saved_run_state == RUN_STATE_RUNNING) {
> +        vm_start();
> +    } else if (!runstate_check(saved_run_state)) {
> +        runstate_set(saved_run_state);
> +    } else {
> +        ; /* leave unchanged */
> +    }
> +}
> +
>  bool runstate_check(RunState state)
>  {
>      return current_run_state == state;
>  }
>  
> +static void runstate_save(QEMUFile *f, void *opaque)
> +{
> +    qemu_put_byte(f, saved_run_state);
> +}
> +
> +static int runstate_load(QEMUFile *f, void *opaque, int version_id)
> +{
> +    saved_run_state = qemu_get_byte(f);
> +
> +    return 0;
> +}

This breaks loading images without support for runstate information. 
Is it possible to overcome this limitation?

Would be happier if this patch could be debated on QEMU list.

Re: [PATCH v12 rebased 2/8] start vm after resetting it

[Marcelo Tosatti]

On Wed, Jan 23, 2013 at 03:19:23PM +0800, Hu Tao wrote:
> From: Wen Congyang <wency@cn.fujitsu.com>
> 
> The guest should run after resetting it, but it does not run if its
> old state is RUN_STATE_INTERNAL_ERROR or RUN_STATE_PAUSED.
> 
> We don't set runstate to RUN_STATE_PAUSED when resetting the guest,
> so the runstate will be changed from RUN_STATE_INTERNAL_ERROR or
> RUN_STATE_PAUSED to RUN_STATE_RUNNING(not RUN_STATE_PAUSED).

It appears the last hunk will automatically reset state from 
RUN_STATE_INTERNAL_ERROR to RUN_STATE_RUNNING ?

I suppose the transition table allows, from RUN_STATE_INTERNAL_ERROR:

<monitor> system_reset
<monitor> cont

To resume the machine?

> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>  include/block/block.h | 2 ++
>  qmp.c                 | 2 +-
>  vl.c                  | 7 ++++---
>  3 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index ffd1936..5e82ccb 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -366,6 +366,8 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs);
>  void bdrv_set_in_use(BlockDriverState *bs, int in_use);
>  int bdrv_in_use(BlockDriverState *bs);
>  
> +void iostatus_bdrv_it(void *opaque, BlockDriverState *bs);
> +
>  #ifdef CONFIG_LINUX_AIO
>  int raw_get_aio_fd(BlockDriverState *bs);
>  #else
> diff --git a/qmp.c b/qmp.c
> index 55b056b..5f1bed1 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -130,7 +130,7 @@ SpiceInfo *qmp_query_spice(Error **errp)
>  };
>  #endif
>  
> -static void iostatus_bdrv_it(void *opaque, BlockDriverState *bs)
> +void iostatus_bdrv_it(void *opaque, BlockDriverState *bs)
>  {
>      bdrv_iostatus_reset(bs);
>  }
> diff --git a/vl.c b/vl.c
> index b0bcf1e..1d2edaa 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -534,7 +534,7 @@ static const RunStateTransition runstate_transitions_def[] = {
>      { RUN_STATE_INMIGRATE, RUN_STATE_RUNNING },
>      { RUN_STATE_INMIGRATE, RUN_STATE_PAUSED },
>  
> -    { RUN_STATE_INTERNAL_ERROR, RUN_STATE_PAUSED },
> +    { RUN_STATE_INTERNAL_ERROR, RUN_STATE_RUNNING },
>      { RUN_STATE_INTERNAL_ERROR, RUN_STATE_FINISH_MIGRATE },
>  
>      { RUN_STATE_IO_ERROR, RUN_STATE_RUNNING },
> @@ -569,7 +569,7 @@ static const RunStateTransition runstate_transitions_def[] = {
>  
>      { RUN_STATE_SAVE_VM, RUN_STATE_RUNNING },
>  
> -    { RUN_STATE_SHUTDOWN, RUN_STATE_PAUSED },
> +    { RUN_STATE_SHUTDOWN, RUN_STATE_RUNNING },
>      { RUN_STATE_SHUTDOWN, RUN_STATE_FINISH_MIGRATE },
>  
>      { RUN_STATE_DEBUG, RUN_STATE_SUSPENDED },
> @@ -1951,7 +1951,8 @@ static bool main_loop_should_exit(void)
>          resume_all_vcpus();
>          if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
>              runstate_check(RUN_STATE_SHUTDOWN)) {
> -            runstate_set(RUN_STATE_PAUSED);
> +            bdrv_iterate(iostatus_bdrv_it, NULL);
> +            vm_start();
>          }
>      }
>      if (qemu_wakeup_requested()) {
> -- 
> 1.8.0.1.240.ge8a1f5a
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v12 rebased 1/8] preserve cpu runstate

[Hu Tao]

On Thu, Feb 07, 2013 at 11:45:34PM -0200, Marcelo Tosatti wrote:
> On Wed, Jan 23, 2013 at 03:19:22PM +0800, Hu Tao wrote:
> > This patch enables preservation of cpu runstate during save/load vm.
> > So when a vm is restored from snapshot, the cpu runstate is restored,
> > too.
> > 
> > See following example:
> > 
> > # save two vms: one is running, the other is paused
> > (qemu) info status
> > VM status: running
> > (qemu) savevm running
> > (qemu) stop
> > (qemu) info status
> > VM status: paused
> > (qemu) savevm paused
> > 
> > # restore the one running
> > (qemu) info status
> > VM status: paused
> > (qemu) loadvm running
> > (qemu) info status
> > VM status: running
> > 
> > # restore the one paused
> > (qemu) loadvm paused
> > (qemu) info status
> > VM status: paused
> > (qemu) cont
> > (qemu)info status
> > VM status: running
> > 
> > 
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> 
> Lack of pause state on guest images is annoying. 
> 
> Fail to see why the panic feature depends on preservation of cpu
> runstate.

To preserve the panic state if guest panic happens in the midway of
migration.

> 
> >  include/sysemu/sysemu.h |  2 ++
> >  migration.c             |  6 +-----
> >  monitor.c               |  5 ++---
> >  savevm.c                |  1 +
> >  vl.c                    | 34 ++++++++++++++++++++++++++++++++++
> >  5 files changed, 40 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> > index 337ce7d..7a69fde 100644
> > --- a/include/sysemu/sysemu.h
> > +++ b/include/sysemu/sysemu.h
> > @@ -19,6 +19,8 @@ extern uint8_t qemu_uuid[];
> >  int qemu_uuid_parse(const char *str, uint8_t *uuid);
> >  #define UUID_FMT "%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
> >  
> > +void save_run_state(void);
> > +void load_run_state(void);
> >  bool runstate_check(RunState state);
> >  void runstate_set(RunState new_state);
> >  int runstate_is_running(void);
> > diff --git a/migration.c b/migration.c
> > index 77c1971..f96cfd6 100644
> > --- a/migration.c
> > +++ b/migration.c
> > @@ -108,11 +108,7 @@ static void process_incoming_migration_co(void *opaque)
> >      /* Make sure all file formats flush their mutable metadata */
> >      bdrv_invalidate_cache_all();
> >  
> > -    if (autostart) {
> > -        vm_start();
> > -    } else {
> > -        runstate_set(RUN_STATE_PAUSED);
> > -    }
> > +    load_run_state();
> >  }
> >  
> >  static void enter_migration_coroutine(void *opaque)
> > diff --git a/monitor.c b/monitor.c
> > index 20bd19b..9381ed0 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -2059,13 +2059,12 @@ void qmp_closefd(const char *fdname, Error **errp)
> >  
> >  static void do_loadvm(Monitor *mon, const QDict *qdict)
> >  {
> > -    int saved_vm_running  = runstate_is_running();
> >      const char *name = qdict_get_str(qdict, "name");
> >  
> >      vm_stop(RUN_STATE_RESTORE_VM);
> >  
> > -    if (load_vmstate(name) == 0 && saved_vm_running) {
> > -        vm_start();
> > +    if (load_vmstate(name) == 0) {
> > +        load_run_state();
> >      }
> >  }
> >  
> > diff --git a/savevm.c b/savevm.c
> > index 304d1ef..10f1d56 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -2112,6 +2112,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
> >      }
> >  
> >      saved_vm_running = runstate_is_running();
> > +    save_run_state();
> >      vm_stop(RUN_STATE_SAVE_VM);
> >  
> >      memset(sn, 0, sizeof(*sn));
> > diff --git a/vl.c b/vl.c
> > index 4ee1302..b0bcf1e 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -520,6 +520,7 @@ static int default_driver_check(QemuOpts *opts, void *opaque)
> >  /* QEMU state */
> >  
> >  static RunState current_run_state = RUN_STATE_PRELAUNCH;
> > +static RunState saved_run_state = RUN_STATE_PRELAUNCH;
> >  
> >  typedef struct {
> >      RunState from;
> > @@ -543,6 +544,7 @@ static const RunStateTransition runstate_transitions_def[] = {
> >      { RUN_STATE_PAUSED, RUN_STATE_FINISH_MIGRATE },
> >  
> >      { RUN_STATE_POSTMIGRATE, RUN_STATE_RUNNING },
> > +    { RUN_STATE_POSTMIGRATE, RUN_STATE_PAUSED },
> >      { RUN_STATE_POSTMIGRATE, RUN_STATE_FINISH_MIGRATE },
> >  
> >      { RUN_STATE_PRELAUNCH, RUN_STATE_RUNNING },
> > @@ -553,6 +555,7 @@ static const RunStateTransition runstate_transitions_def[] = {
> >      { RUN_STATE_FINISH_MIGRATE, RUN_STATE_POSTMIGRATE },
> >  
> >      { RUN_STATE_RESTORE_VM, RUN_STATE_RUNNING },
> > +    { RUN_STATE_RESTORE_VM, RUN_STATE_PAUSED },
> >  
> >      { RUN_STATE_RUNNING, RUN_STATE_DEBUG },
> >      { RUN_STATE_RUNNING, RUN_STATE_INTERNAL_ERROR },
> > @@ -582,11 +585,39 @@ static const RunStateTransition runstate_transitions_def[] = {
> >  
> >  static bool runstate_valid_transitions[RUN_STATE_MAX][RUN_STATE_MAX];
> >  
> > +void save_run_state(void)
> > +{
> > +    saved_run_state = current_run_state;
> > +}
> > +
> > +void load_run_state(void)
> > +{
> > +    if (saved_run_state == RUN_STATE_RUNNING) {
> > +        vm_start();
> > +    } else if (!runstate_check(saved_run_state)) {
> > +        runstate_set(saved_run_state);
> > +    } else {
> > +        ; /* leave unchanged */
> > +    }
> > +}
> > +
> >  bool runstate_check(RunState state)
> >  {
> >      return current_run_state == state;
> >  }
> >  
> > +static void runstate_save(QEMUFile *f, void *opaque)
> > +{
> > +    qemu_put_byte(f, saved_run_state);
> > +}
> > +
> > +static int runstate_load(QEMUFile *f, void *opaque, int version_id)
> > +{
> > +    saved_run_state = qemu_get_byte(f);
> > +
> > +    return 0;
> > +}
> 
> This breaks loading images without support for runstate information. 
> Is it possible to overcome this limitation?

It's successful to load images without support for runstate information,
but trying to set run state to RUN_STATE_PRELAUNCH(the default value of
saved_run_state), which is invalid. The problem can be solved by setting
the default value of saved_run_state to RUN_STATE_RUNNING.

> 
> Would be happier if this patch could be debated on QEMU list.


-- 
Regards,
Hu Tao

Re: [PATCH v12 rebased 2/8] start vm after resetting it

[Hu Tao]

On Thu, Feb 07, 2013 at 11:50:28PM -0200, Marcelo Tosatti wrote:
> On Wed, Jan 23, 2013 at 03:19:23PM +0800, Hu Tao wrote:
> > From: Wen Congyang <wency@cn.fujitsu.com>
> > 
> > The guest should run after resetting it, but it does not run if its
> > old state is RUN_STATE_INTERNAL_ERROR or RUN_STATE_PAUSED.
> > 
> > We don't set runstate to RUN_STATE_PAUSED when resetting the guest,
> > so the runstate will be changed from RUN_STATE_INTERNAL_ERROR or
> > RUN_STATE_PAUSED to RUN_STATE_RUNNING(not RUN_STATE_PAUSED).
> 
> It appears the last hunk will automatically reset state from 
> RUN_STATE_INTERNAL_ERROR to RUN_STATE_RUNNING ?

Yes.

> 
> I suppose the transition table allows, from RUN_STATE_INTERNAL_ERROR:
> 
> <monitor> system_reset
> <monitor> cont
> 
> To resume the machine?

True.

I think the purpose of this patch is to always reset and _run_ the guest
by `system_reset', avoiding an additional `cont' following `system_reset'.

> 
> > 
> > Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> > ---
> >  include/block/block.h | 2 ++
> >  qmp.c                 | 2 +-
> >  vl.c                  | 7 ++++---
> >  3 files changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/block/block.h b/include/block/block.h
> > index ffd1936..5e82ccb 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -366,6 +366,8 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs);
> >  void bdrv_set_in_use(BlockDriverState *bs, int in_use);
> >  int bdrv_in_use(BlockDriverState *bs);
> >  
> > +void iostatus_bdrv_it(void *opaque, BlockDriverState *bs);
> > +
> >  #ifdef CONFIG_LINUX_AIO
> >  int raw_get_aio_fd(BlockDriverState *bs);
> >  #else
> > diff --git a/qmp.c b/qmp.c
> > index 55b056b..5f1bed1 100644
> > --- a/qmp.c
> > +++ b/qmp.c
> > @@ -130,7 +130,7 @@ SpiceInfo *qmp_query_spice(Error **errp)
> >  };
> >  #endif
> >  
> > -static void iostatus_bdrv_it(void *opaque, BlockDriverState *bs)
> > +void iostatus_bdrv_it(void *opaque, BlockDriverState *bs)
> >  {
> >      bdrv_iostatus_reset(bs);
> >  }
> > diff --git a/vl.c b/vl.c
> > index b0bcf1e..1d2edaa 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -534,7 +534,7 @@ static const RunStateTransition runstate_transitions_def[] = {
> >      { RUN_STATE_INMIGRATE, RUN_STATE_RUNNING },
> >      { RUN_STATE_INMIGRATE, RUN_STATE_PAUSED },
> >  
> > -    { RUN_STATE_INTERNAL_ERROR, RUN_STATE_PAUSED },
> > +    { RUN_STATE_INTERNAL_ERROR, RUN_STATE_RUNNING },
> >      { RUN_STATE_INTERNAL_ERROR, RUN_STATE_FINISH_MIGRATE },
> >  
> >      { RUN_STATE_IO_ERROR, RUN_STATE_RUNNING },
> > @@ -569,7 +569,7 @@ static const RunStateTransition runstate_transitions_def[] = {
> >  
> >      { RUN_STATE_SAVE_VM, RUN_STATE_RUNNING },
> >  
> > -    { RUN_STATE_SHUTDOWN, RUN_STATE_PAUSED },
> > +    { RUN_STATE_SHUTDOWN, RUN_STATE_RUNNING },
> >      { RUN_STATE_SHUTDOWN, RUN_STATE_FINISH_MIGRATE },
> >  
> >      { RUN_STATE_DEBUG, RUN_STATE_SUSPENDED },
> > @@ -1951,7 +1951,8 @@ static bool main_loop_should_exit(void)
> >          resume_all_vcpus();
> >          if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
> >              runstate_check(RUN_STATE_SHUTDOWN)) {
> > -            runstate_set(RUN_STATE_PAUSED);
> > +            bdrv_iterate(iostatus_bdrv_it, NULL);
> > +            vm_start();
> >          }
> >      }
> >      if (qemu_wakeup_requested()) {
> > -- 
> > 1.8.0.1.240.ge8a1f5a
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Regards,
Hu Tao

Re: [PATCH v12 rebased] kvm: notify host when the guest is panicked

[Hu Tao]

On Thu, Feb 07, 2013 at 11:39:47PM -0200, Marcelo Tosatti wrote:
> Hi,
> 
> On Wed, Jan 23, 2013 at 03:19:21PM +0800, Hu Tao wrote:
> > We can know the guest is panicked when the guest runs on xen.
> > But we do not have such feature on kvm.
> > 
> > Another purpose of this feature is: management app(for example:
> > libvirt) can do auto dump when the guest is panicked. If management
> > app does not do auto dump, the guest's user can do dump by hand if
> > he sees the guest is panicked.
> > 
> > We have three solutions to implement this feature:
> > 1. use vmcall
> > 2. use I/O port
> > 3. use virtio-serial.
> > 
> > We have decided to avoid touching hypervisor. The reason why I choose
> > choose the I/O port is:
> > 1. it is easier to implememt
> > 2. it does not depend any virtual device
> > 3. it can work when starting the kernel
> > 
> > Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  arch/ia64/kvm/irq.h                  | 19 +++++++++++++
> >  arch/powerpc/include/asm/kvm_para.h  | 18 ++++++++++++
> >  arch/s390/include/asm/kvm_para.h     | 19 +++++++++++++
> >  arch/x86/include/asm/kvm_para.h      | 20 ++++++++++++++
> >  arch/x86/include/uapi/asm/kvm_para.h |  2 ++
> >  arch/x86/kernel/kvm.c                | 53 ++++++++++++++++++++++++++++++++++++
> >  include/linux/kvm_para.h             | 18 ++++++++++++
> >  include/uapi/linux/kvm_para.h        |  6 ++++
> >  kernel/panic.c                       |  4 +++
> >  9 files changed, 159 insertions(+)
> > 
> > diff --git a/arch/ia64/kvm/irq.h b/arch/ia64/kvm/irq.h
> > index c0785a7..b3870f8 100644
> > --- a/arch/ia64/kvm/irq.h
> > +++ b/arch/ia64/kvm/irq.h
> > @@ -30,4 +30,23 @@ static inline int irqchip_in_kernel(struct kvm *kvm)
> >  	return 1;
> >  }
> >  
> > +static inline int kvm_arch_pv_event_init(void)
> > +{
> > +	return 0;
> > +}
> > +
> > +static inline unsigned int kvm_arch_pv_features(void)
> > +{
> > +	return 0;
> > +}
> > +
> > +static inline void kvm_arch_pv_eject_event(unsigned int event)
> > +{
> > +}
> > +
> > +static inline bool kvm_arch_pv_event_enabled(void)
> > +{
> > +	return false;
> > +}
> > +
> 
> The interface is x86 only, no need to touch other architectures.

OK.

> 
> >  #endif
> > diff --git a/arch/powerpc/include/asm/kvm_para.h b/arch/powerpc/include/asm/kvm_para.h
> > index 2b11965..17dd013 100644
> > --- a/arch/powerpc/include/asm/kvm_para.h
> > +++ b/arch/powerpc/include/asm/kvm_para.h
> > @@ -144,4 +144,22 @@ static inline bool kvm_check_and_clear_guest_paused(void)
> >  	return false;
> >  }
> >  
> > +static inline int kvm_arch_pv_event_init(void)
> > +{
> > +	return 0;
> > +}
> > +
> > +static inline unsigned int kvm_arch_pv_features(void)
> > +{
> > +	return 0;
> > +}
> > +
> > +static inline void kvm_arch_pv_eject_event(unsigned int event)
> > +{
> > +}
> > +
> > +static inline bool kvm_arch_pv_event_enabled(void)
> > +{
> > +	return false;
> > +}
> >  #endif /* __POWERPC_KVM_PARA_H__ */
> > diff --git a/arch/s390/include/asm/kvm_para.h b/arch/s390/include/asm/kvm_para.h
> > index e0f8423..81d87ec 100644
> > --- a/arch/s390/include/asm/kvm_para.h
> > +++ b/arch/s390/include/asm/kvm_para.h
> > @@ -154,4 +154,23 @@ static inline bool kvm_check_and_clear_guest_paused(void)
> >  	return false;
> >  }
> >  
> > +static inline int kvm_arch_pv_event_init(void)
> > +{
> > +	return 0;
> > +}
> > +
> > +static inline unsigned int kvm_arch_pv_features(void)
> > +{
> > +	return 0;
> > +}
> > +
> > +static inline void kvm_arch_pv_eject_event(unsigned int event)
> > +{
> > +}
> > +
> > +static inline bool kvm_arch_pv_event_enabled(void)
> > +{
> > +	return false;
> > +}
> > +
> >  #endif /* __S390_KVM_PARA_H */
> > --- a/arch/x86/include/asm/kvm_para.h
> > +++ b/arch/x86/include/asm/kvm_para.h
> > @@ -133,4 +133,24 @@ static inline void kvm_disable_steal_time(void)
> >  }
> >  #endif
> >  
> > +static inline int kvm_arch_pv_event_init(void)
> > +{
> > +	if (!request_region(KVM_PV_EVENT_PORT, 4, "KVM_PV_EVENT"))
> > +		return -1;
> > +
> > +	return 0;
> > +}
> 
> This should be in a driver in arch/x86/kernel/kvm-panic.c, or so.
> 
> > +
> > +static inline unsigned int kvm_arch_pv_features(void)
> > +{
> > +	return inl(KVM_PV_EVENT_PORT);
> > +}
> > +
> > +static inline void kvm_arch_pv_eject_event(unsigned int event)
> > +{
> > +	outl(event, KVM_PV_EVENT_PORT);
> > +}
> 
> > +
> > +bool kvm_arch_pv_event_enabled(void);
> > +
> >  #endif /* _ASM_X86_KVM_PARA_H */
> > diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h
> > index 06fdbd9..c15ef33 100644
> > --- a/arch/x86/include/uapi/asm/kvm_para.h
> > +++ b/arch/x86/include/uapi/asm/kvm_para.h
> > @@ -96,5 +96,7 @@ struct kvm_vcpu_pv_apf_data {
> >  #define KVM_PV_EOI_ENABLED KVM_PV_EOI_MASK
> >  #define KVM_PV_EOI_DISABLED 0x0
> >  
> > +#define KVM_PV_EVENT_PORT	(0x505UL)
> > +
> 
> No need for the ioport to be hard coded. What are the options to
> communicate an address to the guest? An MSR, via ACPI?

I'm not quite understanding here. By 'address', you mean an ioport?
how to communicate an address? (I have little knowledge about ACPI)

> 
> >  
> >  #endif /* _UAPI_ASM_X86_KVM_PARA_H */
> > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> > index 9c2bd8b..0aa7b3e 100644
> > --- a/arch/x86/kernel/kvm.c
> > +++ b/arch/x86/kernel/kvm.c
> > @@ -73,6 +73,20 @@ static int parse_no_kvmclock_vsyscall(char *arg)
> >  
> >  early_param("no-kvmclock-vsyscall", parse_no_kvmclock_vsyscall);
> >  
> > +static int pv_event = 1;
> > +static int parse_no_pv_event(char *arg)
> > +{
> > +	pv_event = 0;
> > +	return 0;
> > +}
> > +
> > +bool kvm_arch_pv_event_enabled(void)
> > +{
> > +	return !!pv_event;
> > +}
> > +
> > +early_param("no-pv-event", parse_no_pv_event);
> > +
> 
> "pv-event" is a bad name for an interface which is specific to notify
> panic events. Please use pv-panic everywhere.

panic event is one of the events supported. Can we keep the name?

> 
> >  static DEFINE_PER_CPU(struct kvm_vcpu_pv_apf_data, apf_reason) __aligned(64);
> >  static DEFINE_PER_CPU(struct kvm_steal_time, steal_time) __aligned(64);
> >  static int has_steal_clock = 0;
> > @@ -385,6 +399,17 @@ static struct notifier_block kvm_pv_reboot_nb = {
> >  	.notifier_call = kvm_pv_reboot_notify,
> >  };
> >  
> > +static int
> > +kvm_pv_panic_notify(struct notifier_block *nb, unsigned long code, void *unused)
> > +{
> > +	kvm_pv_eject_event(KVM_PV_EVENT_PANICKED);
> > +	return NOTIFY_DONE;
> > +}
> 
> Why 'eject' ?

changed to 'send'

> 
> > +
> > +static struct notifier_block kvm_pv_panic_nb = {
> > +	.notifier_call = kvm_pv_panic_notify,
> > +};
> > +
> >  static u64 kvm_steal_clock(int cpu)
> >  {
> >  	u64 steal;
> > @@ -462,6 +487,34 @@ static void __init kvm_apf_trap_init(void)
> >  	set_intr_gate(14, &async_page_fault);
> >  }
> >  
> > +static void __init kvm_pv_panicked_event_init(void)
> > +{
> > +	if (!kvm_para_available())
> > +		return;
> > +
> > +	if (kvm_pv_has_feature(KVM_PV_FEATURE_PANICKED))
> > +		atomic_notifier_chain_register(&panic_notifier_list,
> > +			&kvm_pv_panic_nb);
> > +}
> > +
> > +static inline int kvm_pv_event_init(void)
> > +{
> > +	return kvm_arch_pv_event_init();
> > +}
> > +
> > +static int __init enable_pv_event(void)
> > +{
> > +	if (pv_event) {
> > +		if (kvm_pv_event_init())
> > +			return 0;
> > +
> > +		kvm_pv_panicked_event_init();
> > +	}
> > +
> > +	return 0;
> > +}
> > +arch_initcall(enable_pv_event);
> 
> Call the initialization code from kvm_guest_init, only one function is
> necessary.

At the point of kvm_guest_init, rqeust_region (called by
kvm_pv_event_init) will block, so the guest kernel won't up.

> 
> > +
> >  void __init kvm_guest_init(void)
> >  {
> >  	int i;
> > diff --git a/include/linux/kvm_para.h b/include/linux/kvm_para.h
> > index 00a97bb..6fb6198 100644
> > --- a/include/linux/kvm_para.h
> > +++ b/include/linux/kvm_para.h
> > @@ -10,4 +10,22 @@ static inline int kvm_para_has_feature(unsigned int feature)
> >  		return 1;
> >  	return 0;
> >  }
> > +
> > +static inline int kvm_pv_has_feature(unsigned int feature)
> > +{
> > +	if (kvm_arch_pv_features() & (1UL << feature))
> > +		return 1;
> > +	return 0;
> > +}
> > +
> > +static inline void kvm_pv_eject_event(unsigned int event)
> > +{
> > +	kvm_arch_pv_eject_event(event);
> > +}
> > +
> > +static inline bool kvm_pv_event_enabled(void)
> > +{
> > +	return kvm_arch_pv_event_enabled();
> > +}
> 
> No need for this helpers, as noted.

OK.

> 
> >  #endif /* __LINUX_KVM_PARA_H */
> > diff --git a/include/uapi/linux/kvm_para.h b/include/uapi/linux/kvm_para.h
> > index cea2c5c..c41ddce 100644
> > --- a/include/uapi/linux/kvm_para.h
> > +++ b/include/uapi/linux/kvm_para.h
> > @@ -20,6 +20,12 @@
> >  #define KVM_HC_FEATURES			3
> >  #define KVM_HC_PPC_MAP_MAGIC_PAGE	4
> >  
> > +/* The bit of supported pv event */
> > +#define KVM_PV_FEATURE_PANICKED	0
> > +
> > +/* The pv event value */
> > +#define KVM_PV_EVENT_PANICKED	1
> > +
> 
> This is a hypercall header. You want
> 
> arch/x86/include/asm/kvm_para.h

OK.

> 
> >  /*
> >   * hypercalls use architecture specific
> >   */
> > diff --git a/kernel/panic.c b/kernel/panic.c
> > index e1b2822..a764d2e 100644
> > --- a/kernel/panic.c
> > +++ b/kernel/panic.c
> > @@ -23,6 +23,7 @@
> >  #include <linux/init.h>
> >  #include <linux/nmi.h>
> >  #include <linux/dmi.h>
> > +#include <linux/kvm_para.h>
> >  
> >  #define PANIC_TIMER_STEP 100
> >  #define PANIC_BLINK_SPD 18
> > @@ -132,6 +133,9 @@ void panic(const char *fmt, ...)
> >  	if (!panic_blink)
> >  		panic_blink = no_blink;
> >  
> > +	if (kvm_pv_event_enabled())
> > +		panic_timeout = 0;
> > +
> 
> What is the rationale behind this?

This is a hack to disable reset_on_panic if user enables
pv-event.

Re: [PATCH v12 rebased] kvm: notify host when the guest is panicked

[Marcelo Tosatti]

On Thu, Feb 28, 2013 at 04:54:25PM +0800, Hu Tao wrote:
> > > diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h
> > > index 06fdbd9..c15ef33 100644
> > > --- a/arch/x86/include/uapi/asm/kvm_para.h
> > > +++ b/arch/x86/include/uapi/asm/kvm_para.h
> > > @@ -96,5 +96,7 @@ struct kvm_vcpu_pv_apf_data {
> > >  #define KVM_PV_EOI_ENABLED KVM_PV_EOI_MASK
> > >  #define KVM_PV_EOI_DISABLED 0x0
> > >  
> > > +#define KVM_PV_EVENT_PORT	(0x505UL)
> > > +
> > 
> > No need for the ioport to be hard coded. What are the options to
> > communicate an address to the guest? An MSR, via ACPI?
> 
> I'm not quite understanding here. By 'address', you mean an ioport?
> how to communicate an address? (I have little knowledge about ACPI)

Yes, the ioport. The address of the ioport should not be fixed (for
example future emulated board could use that fixed ioport address,
0x505UL).

One option is to pass the address via an MSR. Yes, that is probably the
best option because there is no dependency on ACPI.

> > "pv-event" is a bad name for an interface which is specific to notify
> > panic events. Please use pv-panic everywhere.
> 
> panic event is one of the events supported. Can we keep the name?

> > Call the initialization code from kvm_guest_init, only one function is
> > necessary.
> 
> At the point of kvm_guest_init, rqeust_region (called by
> kvm_pv_event_init) will block, so the guest kernel won't up.

Why does it block?

> > >  #define PANIC_TIMER_STEP 100
> > >  #define PANIC_BLINK_SPD 18
> > > @@ -132,6 +133,9 @@ void panic(const char *fmt, ...)
> > >  	if (!panic_blink)
> > >  		panic_blink = no_blink;
> > >  
> > > +	if (kvm_pv_event_enabled())
> > > +		panic_timeout = 0;
> > > +
> > 
> > What is the rationale behind this?
> 
> This is a hack to disable reset_on_panic if user enables
> pv-event.

Condition it to kvm_pv_event_enabled() directly?

Re: [PATCH v12 rebased 2/8] start vm after resetting it

[Marcelo Tosatti]

On Wed, Feb 20, 2013 at 04:13:49PM +0800, Hu Tao wrote:
> On Thu, Feb 07, 2013 at 11:50:28PM -0200, Marcelo Tosatti wrote:
> > On Wed, Jan 23, 2013 at 03:19:23PM +0800, Hu Tao wrote:
> > > From: Wen Congyang <wency@cn.fujitsu.com>
> > > 
> > > The guest should run after resetting it, but it does not run if its
> > > old state is RUN_STATE_INTERNAL_ERROR or RUN_STATE_PAUSED.
> > > 
> > > We don't set runstate to RUN_STATE_PAUSED when resetting the guest,
> > > so the runstate will be changed from RUN_STATE_INTERNAL_ERROR or
> > > RUN_STATE_PAUSED to RUN_STATE_RUNNING(not RUN_STATE_PAUSED).
> > 
> > It appears the last hunk will automatically reset state from 
> > RUN_STATE_INTERNAL_ERROR to RUN_STATE_RUNNING ?
> 
> Yes.
> 
> > 
> > I suppose the transition table allows, from RUN_STATE_INTERNAL_ERROR:
> > 
> > <monitor> system_reset
> > <monitor> cont
> > 
> > To resume the machine?
> 
> True.
> 
> I think the purpose of this patch is to always reset and _run_ the guest
> by `system_reset', avoiding an additional `cont' following `system_reset'.

Unclear why its essential to the feature being proposed?

Re: [PATCH v12 rebased] kvm: notify host when the guest is panicked

[Gleb Natapov]

On Fri, Mar 01, 2013 at 09:03:12PM -0300, Marcelo Tosatti wrote:
> On Thu, Feb 28, 2013 at 04:54:25PM +0800, Hu Tao wrote:
> > > > diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h
> > > > index 06fdbd9..c15ef33 100644
> > > > --- a/arch/x86/include/uapi/asm/kvm_para.h
> > > > +++ b/arch/x86/include/uapi/asm/kvm_para.h
> > > > @@ -96,5 +96,7 @@ struct kvm_vcpu_pv_apf_data {
> > > >  #define KVM_PV_EOI_ENABLED KVM_PV_EOI_MASK
> > > >  #define KVM_PV_EOI_DISABLED 0x0
> > > >  
> > > > +#define KVM_PV_EVENT_PORT	(0x505UL)
> > > > +
> > > 
> > > No need for the ioport to be hard coded. What are the options to
> > > communicate an address to the guest? An MSR, via ACPI?
> > 
> > I'm not quite understanding here. By 'address', you mean an ioport?
> > how to communicate an address? (I have little knowledge about ACPI)
> 
> Yes, the ioport. The address of the ioport should not be fixed (for
> example future emulated board could use that fixed ioport address,
> 0x505UL).
> 
> One option is to pass the address via an MSR. Yes, that is probably the
> best option because there is no dependency on ACPI.
> 
Why dependency on ACPI is problematic? ACPI is the standard way on x86
to enumerate platform devices. Passing it through MSR makes this panic
device CPU interface which it is not. And since relying on #GP to detect
valid MSRs is not good interface we will have to guard it by cpuid bit.

--
			Gleb.

Re: [PATCH v12 rebased] kvm: notify host when the guest is panicked

[Marcelo Tosatti]

On Sun, Mar 03, 2013 at 03:00:22PM +0200, Gleb Natapov wrote:
> On Fri, Mar 01, 2013 at 09:03:12PM -0300, Marcelo Tosatti wrote:
> > On Thu, Feb 28, 2013 at 04:54:25PM +0800, Hu Tao wrote:
> > > > > diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h
> > > > > index 06fdbd9..c15ef33 100644
> > > > > --- a/arch/x86/include/uapi/asm/kvm_para.h
> > > > > +++ b/arch/x86/include/uapi/asm/kvm_para.h
> > > > > @@ -96,5 +96,7 @@ struct kvm_vcpu_pv_apf_data {
> > > > >  #define KVM_PV_EOI_ENABLED KVM_PV_EOI_MASK
> > > > >  #define KVM_PV_EOI_DISABLED 0x0
> > > > >  
> > > > > +#define KVM_PV_EVENT_PORT	(0x505UL)
> > > > > +
> > > > 
> > > > No need for the ioport to be hard coded. What are the options to
> > > > communicate an address to the guest? An MSR, via ACPI?
> > > 
> > > I'm not quite understanding here. By 'address', you mean an ioport?
> > > how to communicate an address? (I have little knowledge about ACPI)
> > 
> > Yes, the ioport. The address of the ioport should not be fixed (for
> > example future emulated board could use that fixed ioport address,
> > 0x505UL).
> > 
> > One option is to pass the address via an MSR. Yes, that is probably the
> > best option because there is no dependency on ACPI.
> > 
> Why dependency on ACPI is problematic? ACPI is the standard way on x86
> to enumerate platform devices. Passing it through MSR makes this panic
> device CPU interface which it is not. And since relying on #GP to detect
> valid MSRs is not good interface we will have to guard it by cpuid bit.
> 
> --
> 			Gleb.

KVM guest <-> KVM host interface is not dependent on ACPI, so far. Say,
its possible to use a Linux guest without ACPI and have KVM paravirt 
fully functional.

Re: [PATCH v12 rebased] kvm: notify host when the guest is panicked

[Gleb Natapov]

On Sun, Mar 03, 2013 at 07:29:53PM -0300, Marcelo Tosatti wrote:
> On Sun, Mar 03, 2013 at 03:00:22PM +0200, Gleb Natapov wrote:
> > On Fri, Mar 01, 2013 at 09:03:12PM -0300, Marcelo Tosatti wrote:
> > > On Thu, Feb 28, 2013 at 04:54:25PM +0800, Hu Tao wrote:
> > > > > > diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h
> > > > > > index 06fdbd9..c15ef33 100644
> > > > > > --- a/arch/x86/include/uapi/asm/kvm_para.h
> > > > > > +++ b/arch/x86/include/uapi/asm/kvm_para.h
> > > > > > @@ -96,5 +96,7 @@ struct kvm_vcpu_pv_apf_data {
> > > > > >  #define KVM_PV_EOI_ENABLED KVM_PV_EOI_MASK
> > > > > >  #define KVM_PV_EOI_DISABLED 0x0
> > > > > >  
> > > > > > +#define KVM_PV_EVENT_PORT	(0x505UL)
> > > > > > +
> > > > > 
> > > > > No need for the ioport to be hard coded. What are the options to
> > > > > communicate an address to the guest? An MSR, via ACPI?
> > > > 
> > > > I'm not quite understanding here. By 'address', you mean an ioport?
> > > > how to communicate an address? (I have little knowledge about ACPI)
> > > 
> > > Yes, the ioport. The address of the ioport should not be fixed (for
> > > example future emulated board could use that fixed ioport address,
> > > 0x505UL).
> > > 
> > > One option is to pass the address via an MSR. Yes, that is probably the
> > > best option because there is no dependency on ACPI.
> > > 
> > Why dependency on ACPI is problematic? ACPI is the standard way on x86
> > to enumerate platform devices. Passing it through MSR makes this panic
> > device CPU interface which it is not. And since relying on #GP to detect
> > valid MSRs is not good interface we will have to guard it by cpuid bit.
> > 
> > --
> > 			Gleb.
> 
> KVM guest <-> KVM host interface is not dependent on ACPI, so far. Say,
> its possible to use a Linux guest without ACPI and have KVM paravirt 
> fully functional.
This is not KVM guest <-> KVM host interface though. This is yet another
device. We could implement real impi device that have crash reporting
capability, but decided to go with something simpler. Without ACPI guest
will not be able to power down itself too, but this is not the reason
for us to introduce non-ACPI interface for power down.

--
			Gleb.

Re: [PATCH v12 rebased] kvm: notify host when the guest is panicked

[Gleb Natapov]

On Mon, Mar 04, 2013 at 05:43:48PM -0300, Marcelo Tosatti wrote:
> On Mon, Mar 04, 2013 at 07:49:13PM +0200, Gleb Natapov wrote:
> > On Sun, Mar 03, 2013 at 07:29:53PM -0300, Marcelo Tosatti wrote:
> > > On Sun, Mar 03, 2013 at 03:00:22PM +0200, Gleb Natapov wrote:
> > > > On Fri, Mar 01, 2013 at 09:03:12PM -0300, Marcelo Tosatti wrote:
> > > > > On Thu, Feb 28, 2013 at 04:54:25PM +0800, Hu Tao wrote:
> > > > > > > > diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h
> > > > > > > > index 06fdbd9..c15ef33 100644
> > > > > > > > --- a/arch/x86/include/uapi/asm/kvm_para.h
> > > > > > > > +++ b/arch/x86/include/uapi/asm/kvm_para.h
> > > > > > > > @@ -96,5 +96,7 @@ struct kvm_vcpu_pv_apf_data {
> > > > > > > >  #define KVM_PV_EOI_ENABLED KVM_PV_EOI_MASK
> > > > > > > >  #define KVM_PV_EOI_DISABLED 0x0
> > > > > > > >  
> > > > > > > > +#define KVM_PV_EVENT_PORT	(0x505UL)
> > > > > > > > +
> > > > > > > 
> > > > > > > No need for the ioport to be hard coded. What are the options to
> > > > > > > communicate an address to the guest? An MSR, via ACPI?
> > > > > > 
> > > > > > I'm not quite understanding here. By 'address', you mean an ioport?
> > > > > > how to communicate an address? (I have little knowledge about ACPI)
> > > > > 
> > > > > Yes, the ioport. The address of the ioport should not be fixed (for
> > > > > example future emulated board could use that fixed ioport address,
> > > > > 0x505UL).
> > > > > 
> > > > > One option is to pass the address via an MSR. Yes, that is probably the
> > > > > best option because there is no dependency on ACPI.
> > > > > 
> > > > Why dependency on ACPI is problematic? ACPI is the standard way on x86
> > > > to enumerate platform devices. Passing it through MSR makes this panic
> > > > device CPU interface which it is not. And since relying on #GP to detect
> > > > valid MSRs is not good interface we will have to guard it by cpuid bit.
> > > > 
> > > > --
> > > > 			Gleb.
> > > 
> > > KVM guest <-> KVM host interface is not dependent on ACPI, so far. Say,
> > > its possible to use a Linux guest without ACPI and have KVM paravirt 
> > > fully functional.
> > This is not KVM guest <-> KVM host interface though. This is yet another
> > device. We could implement real impi device that have crash reporting
> > capability, but decided to go with something simpler. Without ACPI guest
> > will not be able to power down itself too, but this is not the reason
> > for us to introduce non-ACPI interface for power down.
> 
> Sure (its more of an aesthetic/organizational point, i guess).
> 
> Anyway, one problem with ACPI is whether its initialized early enough
> (which is the whole point of PIO the x86 specific interface).
ACPI is needed pretty early in the boot process.

--
			Gleb.