[PATCH v13 0/8] pv event interface between host and guest

[PATCH v13 0/8] pv event interface between host and guest

[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 when migration completes.

v12: http://lists.nongnu.org/archive/html/qemu-devel/2013-01/msg04120.html

changes from v12:
  - no DO_UPCASE
  - the interface is only for x86 now
  - request 4 bytes io range(hw/kvm_pv_event.c)
  - rebase to the latest tree

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                | 198 +++++++++++++++++++++++++++++++++++++++
 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, 314 insertions(+), 17 deletions(-)
 create mode 100644 docs/pv-event.txt
 create mode 100644 hw/kvm/pv_event.c

-- 
1.8.1.4

[PATCH v13] 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/x86/include/asm/kvm_para.h      | 26 +++++++++++++++++++++
 arch/x86/include/uapi/asm/kvm_para.h |  2 ++
 arch/x86/kernel/Makefile             |  2 +-
 arch/x86/kernel/kvm.c                | 44 ++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/kvm_panic.c          | 32 ++++++++++++++++++++++++++
 kernel/panic.c                       |  4 ++++
 6 files changed, 109 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/kernel/kvm_panic.c

diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
index 695399f..deae820 100644
--- a/arch/x86/include/asm/kvm_para.h
+++ b/arch/x86/include/asm/kvm_para.h
@@ -133,4 +133,30 @@ static inline void kvm_disable_steal_time(void)
 }
 #endif
 
+/* The bit of supported pv event */
+#define KVM_PV_FEATURE_PANICKED	0
+
+/* The pv event value */
+#define KVM_PV_EVENT_PANICKED	1
+
+static inline unsigned int kvm_arch_pv_features(void)
+{
+	return inl(KVM_PV_EVENT_PORT);
+}
+
+static inline unsigned int kvm_arch_pv_has_feature(unsigned int feature)
+{
+	if (kvm_arch_pv_features() & (1UL << feature))
+		return 1;
+	return 0;
+}
+
+static inline void kvm_arch_pv_send_event(unsigned int event)
+{
+	outl(event, KVM_PV_EVENT_PORT);
+}
+
+bool kvm_arch_pv_event_enabled(void);
+int kvm_arch_pv_event_init(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/Makefile b/arch/x86/kernel/Makefile
index 7bd3bd3..f0629b2 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -80,7 +80,7 @@ obj-$(CONFIG_DEBUG_RODATA_TEST)	+= test_rodata.o
 obj-$(CONFIG_DEBUG_NX_TEST)	+= test_nx.o
 obj-$(CONFIG_DEBUG_NMI_SELFTEST) += nmi_selftest.o
 
-obj-$(CONFIG_KVM_GUEST)		+= kvm.o kvmclock.o
+obj-$(CONFIG_KVM_GUEST)		+= kvm.o kvmclock.o kvm_panic.o
 obj-$(CONFIG_PARAVIRT)		+= paravirt.o paravirt_patch_$(BITS).o
 obj-$(CONFIG_PARAVIRT_SPINLOCKS)+= paravirt-spinlocks.o
 obj-$(CONFIG_PARAVIRT_CLOCK)	+= pvclock.o
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index b686a90..727ef91 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;
@@ -386,6 +400,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_arch_pv_send_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;
@@ -463,6 +488,23 @@ 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_arch_pv_has_feature(KVM_PV_FEATURE_PANICKED))
+		atomic_notifier_chain_register(&panic_notifier_list,
+			&kvm_pv_panic_nb);
+}
+
+static void enable_pv_event(void)
+{
+	if (pv_event) {
+		if (kvm_arch_pv_event_init())
+			return;
+
+		kvm_pv_panicked_event_init();
+	}
+}
+
 void __init kvm_guest_init(void)
 {
 	int i;
@@ -494,6 +536,8 @@ void __init kvm_guest_init(void)
 #else
 	kvm_guest_cpu_init();
 #endif
+
+	enable_pv_event();
 }
 
 static bool __init kvm_detect(void)
diff --git a/arch/x86/kernel/kvm_panic.c b/arch/x86/kernel/kvm_panic.c
new file mode 100644
index 0000000..3df23b7
--- /dev/null
+++ b/arch/x86/kernel/kvm_panic.c
@@ -0,0 +1,32 @@
+/*
+ * KVM panic notification implementation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ * Copyright (C) 2013, Fujitsu, Inc.
+ *   Authors: Wen Congyang <wency@cn.fujitsu.com>
+ *   Authors: Hu Tao <hutao@cn.fujitsu.com>
+ */
+
+#include <uapi/asm/kvm_para.h>
+#include <linux/ioport.h>
+
+int kvm_arch_pv_event_init(void)
+{
+	if (!request_region(KVM_PV_EVENT_PORT, 4, "KVM_PV_EVENT"))
+		return -1;
+
+	return 0;
+}
diff --git a/kernel/panic.c b/kernel/panic.c
index e1b2822..06dd477 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_arch_pv_event_enabled())
+		panic_timeout = 0;
+
 	if (panic_timeout > 0) {
 		/*
 		 * Delay timeout seconds before rebooting the machine.
-- 
1.8.1.4

[PATCH v13 1/8] save/load 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 b19ec95..f121213 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 11725ae..c29830e 100644
--- a/migration.c
+++ b/migration.c
@@ -107,11 +107,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();
 }
 
 void process_incoming_migration(QEMUFile *f)
diff --git a/monitor.c b/monitor.c
index 32a6e74..bf974b4 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 a8a53ef..aa631eb 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2143,6 +2143,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 febd2ea..7991f2e 100644
--- a/vl.c
+++ b/vl.c
@@ -523,6 +523,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_RUNNING;
 
 typedef struct {
     RunState from;
@@ -546,6 +547,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 },
@@ -556,6 +558,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 },
@@ -585,11 +588,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;
@@ -599,6 +630,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.1.4

[PATCH v13 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 0f750d7..8effc1e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -376,6 +376,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 7991f2e..3d08e1a 100644
--- a/vl.c
+++ b/vl.c
@@ -537,7 +537,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 },
@@ -572,7 +572,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 },
@@ -2002,7 +2002,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.1.4

[PATCH v13 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..e9b082d 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..6c42923 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.1.4

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

[Hu Tao]

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

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 c29830e..fa17b82 100644
--- a/migration.c
+++ b/migration.c
@@ -698,6 +698,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 28b070f..8f1d138 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 3d08e1a..51d4922 100644
--- a/vl.c
+++ b/vl.c
@@ -536,6 +536,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 },
@@ -549,6 +550,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 },
@@ -559,6 +561,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 },
@@ -569,6 +572,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 },
 
@@ -583,6 +587,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 },
 };
 
@@ -2001,7 +2009,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.1.4

[PATCH v13 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 bf974b4..d65218d 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.1.4

[PATCH v13 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    | 198 +++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pc_piix.c         |   5 ++
 include/sysemu/kvm.h |   2 +
 kvm-stub.c           |   4 ++
 5 files changed, 210 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..5e68190
--- /dev/null
+++ b/hw/kvm/pv_event.c
@@ -0,0 +1,198 @@
+/*
+ * 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"
+#define PV_IOPORT(obj)      OBJECT_CHECK(PVIOPortState, (obj), PV_EVENT_DRIVER)
+
+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 = PV_IOPORT(dev);
+
+    if (pv_event_init(&s->conf) < 0) {
+        return -1;
+    }
+
+    memory_region_init_io(&s->ioport, &pv_io_ops, s, "pv_event", 4);
+    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 aa9cc81..24a9bf3 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 f2d97b5..97d3daf 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -296,4 +296,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 760aadc..f543fa2 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.1.4

[PATCH v13 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 24a9bf3..82a421a 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 797d992..1ad4041 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 51d4922..5b3a279 100644
--- a/vl.c
+++ b/vl.c
@@ -427,6 +427,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.1.4

[PATCH v13 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.1.4

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

[Jan Kiszka]

On 2013-02-28 13:13, 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).

I just wonder what will happen if I interrupted the guest via gdb and
then issue "monitor system_reset", also via gdb - common pattern if you
set a breakpoint on some BUG() or fault handler and then want to restart
the guest. Will the guest continue then while gdb thinks it is still
stopped? Likely, we do not differentiate between gdb-initiated stops and
the rest. Could you clarify?

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v13 1/8] save/load cpu runstate

[Eric Blake]

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

On 02/28/2013 05:13 AM, 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.

What happens if a management app wants to override the runstate when
restoring the domain?  I can think of several useful scenarios:

1. management app pauses the guest, then saves domain state and other
things (management state, or disk clones), then resumes the guest.
Later, the management wants to revert to the saved state, but have the
guest running right away.  I guess here, knowing that the guest was
saved in a paused state doesn't hurt, since the management app can
resume it right away.

2. management app saves domain state of a live guest, then copies that
state elsewhere.  In its new location, the management app wants to
investigate the state for forensic analysis - so even though the guest
remembers that it was running, management wants to start it paused.
Here, it is important that there must not be a window of time where the
guest can run, otherwise, the results are not reproducible.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [PATCH v13 1/8] save/load cpu runstate

[Hu Tao]

On Thu, Feb 28, 2013 at 02:12:37PM -0700, Eric Blake wrote:
> On 02/28/2013 05:13 AM, 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.
> 
> What happens if a management app wants to override the runstate when
> restoring the domain?  I can think of several useful scenarios:
> 
> 1. management app pauses the guest, then saves domain state and other
> things (management state, or disk clones), then resumes the guest.
> Later, the management wants to revert to the saved state, but have the
> guest running right away.  I guess here, knowing that the guest was
> saved in a paused state doesn't hurt, since the management app can
> resume it right away.
> 
> 2. management app saves domain state of a live guest, then copies that
> state elsewhere.  In its new location, the management app wants to
> investigate the state for forensic analysis - so even though the guest
> remembers that it was running, management wants to start it paused.
> Here, it is important that there must not be a window of time where the
> guest can run, otherwise, the results are not reproducible.

-S takes precedence in the case. But for in-migration, runstate is
loaded from src.

Re: [PATCH v13 1/8] save/load cpu runstate

[Eric Blake]

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

On 03/01/2013 12:36 AM, Hu Tao wrote:
> On Thu, Feb 28, 2013 at 02:12:37PM -0700, Eric Blake wrote:
>> On 02/28/2013 05:13 AM, 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.
>>
>> What happens if a management app wants to override the runstate when
>> restoring the domain?  I can think of several useful scenarios:
>>
>> 1. management app pauses the guest, then saves domain state and other
>> things (management state, or disk clones), then resumes the guest.
>> Later, the management wants to revert to the saved state, but have the
>> guest running right away.  I guess here, knowing that the guest was
>> saved in a paused state doesn't hurt, since the management app can
>> resume it right away.
>>
>> 2. management app saves domain state of a live guest, then copies that
>> state elsewhere.  In its new location, the management app wants to
>> investigate the state for forensic analysis - so even though the guest
>> remembers that it was running, management wants to start it paused.
>> Here, it is important that there must not be a window of time where the
>> guest can run, otherwise, the results are not reproducible.
> 
> -S takes precedence in the case. But for in-migration, runstate is
> loaded from src.

Given your answer, I think we're okay from the libvirt perspective.  My
biggest worry about a window where the guest runs unchecked is not a
problem, given that libvirt always uses -S on incoming migration.  In
turn, libvirt has its own mechanisms for tracking whether the outgoing
migration was started from a running state, along with API overrides to
let a user override whether libvirt will resume the guest on the
incoming migration side.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [PATCH v13 5/8] add a new qevent: QEVENT_GUEST_PANICKED

[Eric Blake]

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

On 02/28/2013 05:13 AM, Hu Tao wrote:
> 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(+)

Missing documentation in QMP/qmp-events.txt

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [PATCH v13 0/8] pv event interface between host and guest

[Gleb Natapov]

On Thu, Feb 28, 2013 at 08:13:10PM +0800, Hu Tao wrote:
> 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.
> 
What other event do you have in mind? Is interface generic enough to
accommodate future, yet unknown, events. It allows to pass only one
integer specifying even type, what if additional info is needed? My be
stop pretending that device is generic and make it do once thing but do
it well? For generic even passing interface (whatever it may be needed
for) much more powerful virtio should be used.

On implementation itself I do not understand why is this kvm specific.
The only thing that makes it so is that you hook device initialization
into guest kvm initialization code, but this is obviously incorrect.
What stops QEMU tcg or Xen from reusing the same device for the same
purpose except the artificial limitation in a guest.

Reading data from a random ioports is not how you discover platform
devices in 21 century (and the data you read from unassigned port is not
guarantied to be zero, it may depend on QEMU version), you use ACPI for
that and Marcelo already pointed that to you. Having little knowledge of
ACPI (we all do) is not a good reason to not doing it. We probably need
to reserve QEMU specific ACPI Plug and Play hardware ID to define our own
devices. After that you will be able to create device with _HID(QEMU0001)
in DSDT that supplies address information (ioport to use) and capability
supported. Guest uses acpi_get_devices() to discover a platform device by
its name (QEMU0001).  Then you put the driver for the platform device
into drivers/platform/x86/ and QEMU/kvm/Xen all will be able to use it.

On QEMU side of things I cannot comment much on how QOMified the device
is (it should be), I hope other reviews will verify it, but I noticed
that device is only initialized for PIIX, what about Q35?

--
			Gleb.

Re: [PATCH v13 1/8] save/load cpu runstate

[Paolo Bonzini]

Il 28/02/2013 13:13, Hu Tao ha scritto:
> 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.

I don't think this feature is worth breaking backwards migration
compatibility.  It is usually handled at a higher-level (management,
like libvirt).

Please make this a separate patch.

Paolo

> 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 b19ec95..f121213 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 11725ae..c29830e 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -107,11 +107,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();
>  }
>  
>  void process_incoming_migration(QEMUFile *f)
> diff --git a/monitor.c b/monitor.c
> index 32a6e74..bf974b4 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 a8a53ef..aa631eb 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2143,6 +2143,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 febd2ea..7991f2e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -523,6 +523,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_RUNNING;
>  
>  typedef struct {
>      RunState from;
> @@ -546,6 +547,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 },
> @@ -556,6 +558,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 },
> @@ -585,11 +588,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;
> @@ -599,6 +630,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 */
> 

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

[Paolo Bonzini]

Il 28/02/2013 13:13, Hu Tao ha scritto:
> 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).

This is also debatable.  In particular, restarting an INTERNAL_ERROR
guest makes it harder to inspect the state at the time of the failure.

INTERNAL_ERROR should never happen, let's separate this patch too.

Paolo

Re: [PATCH v13 4/8] add a new runstate: RUN_STATE_GUEST_PANICKED

[Paolo Bonzini]

Il 28/02/2013 13:13, Hu Tao ha scritto:
> The guest will be in this state when it is panicked.
> 
> 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 c29830e..fa17b82 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -698,6 +698,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 28b070f..8f1d138 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 3d08e1a..51d4922 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -536,6 +536,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 },

Is this a consequence of the first patch?

>      { RUN_STATE_INTERNAL_ERROR, RUN_STATE_RUNNING },
>      { RUN_STATE_INTERNAL_ERROR, RUN_STATE_FINISH_MIGRATE },
> @@ -549,6 +550,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 },

Impossible.  GUEST_PANICKED requires an instruction to be executed in
the guest, so it should first go to RUNNING.

>      { RUN_STATE_PRELAUNCH, RUN_STATE_RUNNING },
>      { RUN_STATE_PRELAUNCH, RUN_STATE_FINISH_MIGRATE },
> @@ -559,6 +561,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 },

Is it also for the first patch?

>      { RUN_STATE_RUNNING, RUN_STATE_DEBUG },
>      { RUN_STATE_RUNNING, RUN_STATE_INTERNAL_ERROR },
> @@ -569,6 +572,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 },

This one is obviously ok.

>      { RUN_STATE_SAVE_VM, RUN_STATE_RUNNING },
>  
> @@ -583,6 +587,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 },

Like SHUTDOWN, it should go first to PAUSED and then to RUNNING.  A
GUEST_PANICKED -> RUNNING transition is not possible.  You're seeing it
because you lack the addition of GUEST_PANICKED here:

        if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
            runstate_check(RUN_STATE_SHUTDOWN)) {
            runstate_set(RUN_STATE_PAUSED);
        }

I think you should first move the INTERNAL_ERROR || SHUTDOWN checks to a
separate function, so that you can then add GUEST_PANICKED.

Paolo

>      { RUN_STATE_MAX, RUN_STATE_MAX },
>  };
>  
> @@ -2001,7 +2009,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();
>          }
> 

Re: [PATCH v13 5/8] add a new qevent: QEVENT_GUEST_PANICKED

[Paolo Bonzini]

Il 28/02/2013 13:13, Hu Tao ha scritto:
> 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 bf974b4..d65218d 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)
>  
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [PATCH v13 6/8] introduce a new qom device to deal with panicked event

[Paolo Bonzini]

Il 28/02/2013 13:13, Hu Tao ha scritto:
> 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.

There's nothing KVM-specific about this, no?

Let's call the device hw/pvevent.c, and the device isa-pvevent like
isa-debugcon and isa-debugexit.

Paolo

> 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    | 198 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/pc_piix.c         |   5 ++
>  include/sysemu/kvm.h |   2 +
>  kvm-stub.c           |   4 ++
>  5 files changed, 210 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..5e68190
> --- /dev/null
> +++ b/hw/kvm/pv_event.c
> @@ -0,0 +1,198 @@
> +/*
> + * 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"
> +#define PV_IOPORT(obj)      OBJECT_CHECK(PVIOPortState, (obj), PV_EVENT_DRIVER)
> +
> +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 = PV_IOPORT(dev);
> +
> +    if (pv_event_init(&s->conf) < 0) {
> +        return -1;
> +    }
> +
> +    memory_region_init_io(&s->ioport, &pv_io_ops, s, "pv_event", 4);
> +    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 aa9cc81..24a9bf3 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 f2d97b5..97d3daf 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -296,4 +296,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 760aadc..f543fa2 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)
> +{
> +}
> 

Re: [PATCH v13 7/8] allower the user to disable pv event support

[Paolo Bonzini]

Il 28/02/2013 13:13, Hu Tao ha scritto:
> 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(-)

We cannot add -machine suboptions for everything we add to a random
machine type.  I think it's better to do one of the following:

1) not add it to the machine by default.  Let libvirt add it with -device.

2) add a property to the piix3 ISA bridges (in hw/piix_pci.c) to
enable/disable it.  It can be set with -global PIIX3.pv_event=on/off.

In the second case, if you set it to on by default, you will also need
to add the property in hw/pc.h, to disable the device in older machine
types for backwards-compatibility.

Paolo

> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 24a9bf3..82a421a 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 797d992..1ad4041 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 51d4922..5b3a279 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -427,6 +427,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 */ }
>      },
> 

Re: [PATCH v13 0/8] pv event interface between host and guest

[Paolo Bonzini]

Il 03/03/2013 10:17, Gleb Natapov ha scritto:
> On Thu, Feb 28, 2013 at 08:13:10PM +0800, Hu Tao wrote:
>> 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.
>>
> What other event do you have in mind? Is interface generic enough to
> accommodate future, yet unknown, events. It allows to pass only one
> integer specifying even type, what if additional info is needed? My be
> stop pretending that device is generic and make it do once thing but do
> it well? For generic even passing interface (whatever it may be needed
> for) much more powerful virtio should be used.
> 
> On implementation itself I do not understand why is this kvm specific.
> The only thing that makes it so is that you hook device initialization
> into guest kvm initialization code, but this is obviously incorrect.
> What stops QEMU tcg or Xen from reusing the same device for the same
> purpose except the artificial limitation in a guest.

Agreed.

> Reading data from a random ioports is not how you discover platform
> devices in 21 century (and the data you read from unassigned port is not
> guarantied to be zero, it may depend on QEMU version), you use ACPI for
> that and Marcelo already pointed that to you. Having little knowledge of
> ACPI (we all do) is not a good reason to not doing it. We probably need
> to reserve QEMU specific ACPI Plug and Play hardware ID to define our own
> devices. After that you will be able to create device with _HID(QEMU0001)
> in DSDT that supplies address information (ioport to use) and capability
> supported.

Please also document this HID in a new file in the QEMU docs/ directory.

> Guest uses acpi_get_devices() to discover a platform device by
> its name (QEMU0001).  Then you put the driver for the platform device
> into drivers/platform/x86/ and QEMU/kvm/Xen all will be able to use it.

Just to clarify it for Hu Tao, the read from a random ioport is how the
ACPI code will detect presence of the device.

Something like this should work (in SeaBIOS's src/acpi-dsdt-isa.dsl):

    Device(PEVT) {
        Name(_HID, EisaId("QEMU0001"))
        OperationRegion(PEOR, SystemIO, 0x505, 0x01)
        Field(PEOR, ByteAcc, NoLock, Preserve) {
            PEPT,   8,
        }

        Method(_STA, 0, NotSerialized) {
            Store(PEPT, Local0)
            If (LEqual(Local0, Zero)) {
                Return (0x00)
            } Else {
                Return (0x0F)
            }
        }

        Name(_CRS, ResourceTemplate() {
            IO(Decode16, 0x505, 0x505, 0x01, 0x01)
        })
    }

Please test this with a QEMU option like "-M pc-1.4".  The device should
_not_ be detected if you're doing it right.

> On QEMU side of things I cannot comment much on how QOMified the device
> is (it should be),

Please make the device target-independent.  It can be used on non-x86
architectures that have I/O ports.  You should make the port
configurable using a property (DEFINE_PROPERTY_INT16 or something like
that), with a default of 0x505.

All the panicked_action is not necessary in my opinion.  We have it for
watchdogs, but that's really a legacy thing.  Let libvirt do it, and
always make the guest panic perform the PANICKED_PAUSE action.

If you do it properly, a lot (really a lot) of code will go away.

> I hope other reviews will verify it, but I noticed
> that device is only initialized for PIIX, what about Q35?

Yup.

Paolo

Re: [PATCH v13 0/8] pv event interface between host and guest

[Gleb Natapov]

On Mon, Mar 04, 2013 at 11:05:37AM +0100, Paolo Bonzini wrote:
> > Guest uses acpi_get_devices() to discover a platform device by
> > its name (QEMU0001).  Then you put the driver for the platform device
> > into drivers/platform/x86/ and QEMU/kvm/Xen all will be able to use it.
> 
> Just to clarify it for Hu Tao, the read from a random ioport is how the
> ACPI code will detect presence of the device.
> 
Actually no (at least in the long run, for the first version it may be
OK). Since we want to move DSDT generation into QEMU if device will not
be present QEMU will not generate corresponded Device() in DSDT, or it
will generate it with _STA() { Return (0x00)} hard coded. Seabios can do
the same if we will pass it info about device presence via fw_cfg. Not
sure Kevin will like it now when we plan to move DSDT into QEMU anyway :)

> Something like this should work (in SeaBIOS's src/acpi-dsdt-isa.dsl):
> 
>     Device(PEVT) {
>         Name(_HID, EisaId("QEMU0001"))
>         OperationRegion(PEOR, SystemIO, 0x505, 0x01)
>         Field(PEOR, ByteAcc, NoLock, Preserve) {
>             PEPT,   8,
>         }
> 
>         Method(_STA, 0, NotSerialized) {
>             Store(PEPT, Local0)
>             If (LEqual(Local0, Zero)) {
>                 Return (0x00)
>             } Else {
>                 Return (0x0F)
>             }
>         }
> 
>         Name(_CRS, ResourceTemplate() {
>             IO(Decode16, 0x505, 0x505, 0x01, 0x01)
>         })
>     }
> 
> Please test this with a QEMU option like "-M pc-1.4".  The device should
> _not_ be detected if you're doing it right.
> 

--
			Gleb.

Re: [PATCH v13 0/8] pv event interface between host and guest

[Gleb Natapov]

On Mon, Mar 04, 2013 at 11:28:05AM +0100, Paolo Bonzini wrote:
> Il 04/03/2013 11:21, Gleb Natapov ha scritto:
> >> > Just to clarify it for Hu Tao, the read from a random ioport is how the
> >> > ACPI code will detect presence of the device.
> >> > 
> > Actually no (at least in the long run, for the first version it may be
> > OK).
> 
> Agreed.
> 
> > Since we want to move DSDT generation into QEMU if device will not
> > be present QEMU will not generate corresponded Device() in DSDT, or it
> > will generate it with _STA() { Return (0x00)} hard coded.
> 
> Yes, this would be good.
> 
> > Seabios can do
> > the same if we will pass it info about device presence via fw_cfg.
> 
> True, but I don't like this a lot.  I don't like splitting decisions
> between SeaBIOS and the DSDT, you end up touching code all over the
> place and writing ASL is simpler than patching---even with all the
> machinery that we have.
That's the main argument in favor of moving DSDT into QEMU regardless
of this patch series, but as long as we have it in Seabios, have
infrastructure for patching and use it for many things already I do not
see why avoiding it.

>                          It is also simpler to move ASL from SeaBIOS to
> OVMF and/or viceversa.  I don't recall what was the opposition to a
> fw_cfg driver directly in the DSDT, but I think this would be a good
> usage for it.
> 
Basically fw_cfg was not designed with this in mind. It was really meant
to be simple interface for FW running one one CPU to use. You probably
may do locking with AML too to guaranty atomic access, but things get
complicated. Also may option that was added lately use file interface
(since this is what Kevin prefers) and manipulating strings in AML is
probably not what we want.

> Splitting it between QEMU and DSDT is a bit better, since you have to
> touch QEMU anyway to implement the feature.
> 
> Anyhow, this does not apply to the next submission of this series.  I
> think we can agree to the compromise of using ACPI but still read the
> port in _STA.
> 
If you want to make ioport configurable I do not see how can we avoid
patching.

--
			Gleb.

Re: [PATCH v13 0/8] pv event interface between host and guest

[Paolo Bonzini]

Il 04/03/2013 11:43, Gleb Natapov ha scritto:
> > Anyhow, this does not apply to the next submission of this series.  I
> > think we can agree to the compromise of using ACPI but still read the
> > port in _STA.
> 
> If you want to make ioport configurable I do not see how can we avoid
> patching.

I want to make the ioport configurable in the device, but the PIIX and
ICH9 (which are what the DSDT is written for) will always use port 0x505.

You can configure a different iobase for your serial ports, the guest
can still use them but not discover them via ACPI.  This is the same thing.

Paolo

Re: [PATCH v13 0/8] pv event interface between host and guest

[Gleb Natapov]

On Mon, Mar 04, 2013 at 11:49:07AM +0100, Paolo Bonzini wrote:
> Il 04/03/2013 11:43, Gleb Natapov ha scritto:
> > > Anyhow, this does not apply to the next submission of this series.  I
> > > think we can agree to the compromise of using ACPI but still read the
> > > port in _STA.
> > 
> > If you want to make ioport configurable I do not see how can we avoid
> > patching.
> 
> I want to make the ioport configurable in the device, but the PIIX and
> ICH9 (which are what the DSDT is written for) will always use port 0x505.
> 
But the device is not part of PIIX or ICH9. It is additional device that
may or may not be present depending on a command line. So what if
someone configures debugcon or debugexit to use this port? We can always
blame the users, but I fill that we are making unnecessary compromises.

> You can configure a different iobase for your serial ports, the guest
> can still use them but not discover them via ACPI.  This is the same thing.
> 
Probably we should patch DSDT too when it will be in QEMU :) of force
iobase to spec values if device is used as part of a chipset.

--
			Gleb.

Re: [PATCH v13 0/8] pv event interface between host and guest

[Paolo Bonzini]

Il 04/03/2013 11:59, Gleb Natapov ha scritto:
> > I want to make the ioport configurable in the device, but the PIIX and
> > ICH9 (which are what the DSDT is written for) will always use port 0x505.
> 
> But the device is not part of PIIX or ICH9.

So is kvmclock, or kvmvapic.  I think it makes sense to add this device
to PIIX or ICH9 since it is an ISA device.

> It is additional device that
> may or may not be present depending on a command line. So what if
> someone configures debugcon or debugexit to use this port?

I haven't checked if debug{con,exit} will pass the _STA test, but if
they do, the user will get a Ctrl-A or respectively an exit of QEMU when
the guest panics.

What if someone configures debugcon on port 0x3f8?  Some guest will use
it, some will not.

> We can always
> blame the users, but I fill that we are making unnecessary compromises.

Once we choose an ISA device, where the user has full control of the
address space, we already know we'll have to accept compromises.  I
don't think this compromise is particularly bad: do discovery via ACPI
(nice), accept that the user can trick the AML (ugly).

Paolo

> > You can configure a different iobase for your serial ports, the guest
> > can still use them but not discover them via ACPI.  This is the same thing.
> 
> Probably we should patch DSDT too when it will be in QEMU :) of force
> iobase to spec values if device is used as part of a chipset.

Re: [PATCH v13 0/8] pv event interface between host and guest

[Gleb Natapov]

On Mon, Mar 04, 2013 at 12:10:58PM +0100, Paolo Bonzini wrote:
> Il 04/03/2013 11:59, Gleb Natapov ha scritto:
> > > I want to make the ioport configurable in the device, but the PIIX and
> > > ICH9 (which are what the DSDT is written for) will always use port 0x505.
> > 
> > But the device is not part of PIIX or ICH9.
> 
> So is kvmclock, or kvmvapic.  I think it makes sense to add this device
> to PIIX or ICH9 since it is an ISA device.
> 
Those are CPU interfaces, not chipset. fw_cfg or our PIIX ACPI additions would
be better examples, but since they are always present and non configurable they
are in a different category.

> > It is additional device that
> > may or may not be present depending on a command line. So what if
> > someone configures debugcon or debugexit to use this port?
> 
> I haven't checked if debug{con,exit} will pass the _STA test, but if
> they do, the user will get a Ctrl-A or respectively an exit of QEMU when
> the guest panics.
> 
> What if someone configures debugcon on port 0x3f8?  Some guest will use
> it, some will not.
> 
Qemu should fail to start since conflict will be detected during
initialization.

> > We can always
> > blame the users, but I fill that we are making unnecessary compromises.
> 
> Once we choose an ISA device, where the user has full control of the
> address space, we already know we'll have to accept compromises.  I
> don't think this compromise is particularly bad: do discovery via ACPI
> (nice), accept that the user can trick the AML (ugly).
> 
Why would have we accept compromises, we may, but I disagree that it
is necessary? If user configures conflicting ports QEMU will detect
it during init, if configuration is correct DSDT should provide enough
information for guests to use configured devices.

--
			Gleb.

Re: [PATCH v13 0/8] pv event interface between host and guest

[Paolo Bonzini]

Il 04/03/2013 12:20, Gleb Natapov ha scritto:
> On Mon, Mar 04, 2013 at 12:10:58PM +0100, Paolo Bonzini wrote:
>>> It is additional device that
>>> may or may not be present depending on a command line. So what if
>>> someone configures debugcon or debugexit to use this port?
>>
>> I haven't checked if debug{con,exit} will pass the _STA test, but if
>> they do, the user will get a Ctrl-A or respectively an exit of QEMU when
>> the guest panics.
>>
>> What if someone configures debugcon on port 0x3f8?  Some guest will use
>> it, some will not.
>>
> Qemu should fail to start since conflict will be detected during
> initialization.

Not if you _remove_ the serial port and place debugcon at 0x3f8.

Same here, you can remove the panic event port and add debugcon at
0x505.  That's the problematic case.  But if the user goes to that
length, I think we can honestly say we don't care.

Paolo

>>> We can always
>>> blame the users, but I fill that we are making unnecessary compromises.
>>
>> Once we choose an ISA device, where the user has full control of the
>> address space, we already know we'll have to accept compromises.  I
>> don't think this compromise is particularly bad: do discovery via ACPI
>> (nice), accept that the user can trick the AML (ugly).
>
> Why would have we accept compromises, we may, but I disagree that it
> is necessary? If user configures conflicting ports QEMU will detect
> it during init, if configuration is correct DSDT should provide enough
> information for guests to use configured devices.
> 
> --
> 			Gleb.
> 

Re: [PATCH v13 0/8] pv event interface between host and guest

[Gleb Natapov]

On Mon, Mar 04, 2013 at 12:35:08PM +0100, Paolo Bonzini wrote:
> Il 04/03/2013 12:20, Gleb Natapov ha scritto:
> > On Mon, Mar 04, 2013 at 12:10:58PM +0100, Paolo Bonzini wrote:
> >>> It is additional device that
> >>> may or may not be present depending on a command line. So what if
> >>> someone configures debugcon or debugexit to use this port?
> >>
> >> I haven't checked if debug{con,exit} will pass the _STA test, but if
> >> they do, the user will get a Ctrl-A or respectively an exit of QEMU when
> >> the guest panics.
> >>
> >> What if someone configures debugcon on port 0x3f8?  Some guest will use
> >> it, some will not.
> >>
> > Qemu should fail to start since conflict will be detected during
> > initialization.
> 
> Not if you _remove_ the serial port and place debugcon at 0x3f8.
> 
That will no longer be PIIX chipset. And still, if user will not use
"buggy" guest that assumes that serial is at 0x3f8 or she configures
guest to not use serial, everything will be fine.

> Same here, you can remove the panic event port and add debugcon at
> 0x505.  That's the problematic case.  But if the user goes to that
> length, I think we can honestly say we don't care.
IMO there is a big difference between well know serial ISA ports and
PIO ports we allocate for our devices. Later have to be discoverable
without resorting to probing. On CPU level we do the same with CPUID
bits instead of relaying on MSRs #GP. On KVM API level we do the same
with capabilities instead of relying on ioctls returning errors. This
is not different.

--
			Gleb.

Re: [PATCH v13 0/8] pv event interface between host and guest

[Paolo Bonzini]

Il 04/03/2013 12:52, Gleb Natapov ha scritto:
> > Same here, you can remove the panic event port and add debugcon at
> > 0x505.  That's the problematic case.  But if the user goes to that
> > length, I think we can honestly say we don't care.
>
> IMO there is a big difference between well know serial ISA ports and
> PIO ports we allocate for our devices. Later have to be discoverable
> without resorting to probing. On CPU level we do the same with CPUID
> bits instead of relaying on MSRs #GP. On KVM API level we do the same
> with capabilities instead of relying on ioctls returning errors. This
> is not different.

Ok, I see your point now.  Yes, this is a good reason why patching is
better in the long run.

Paolo

Re: [PATCH v13 1/8] save/load cpu runstate

[Hu Tao]

On Mon, Mar 04, 2013 at 10:30:48AM +0100, Paolo Bonzini wrote:
> Il 28/02/2013 13:13, Hu Tao ha scritto:
> > 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.
> 
> I don't think this feature is worth breaking backwards migration
> compatibility.  It is usually handled at a higher-level (management,
> like libvirt).

If guest panic happens during migration, runstate will still be running
on destination host without this patch. But, it does be a problem to break
backwards migration compatibility.

> 
> Please make this a separate patch.

Sure.

> 
> Paolo
> 
> > 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 b19ec95..f121213 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 11725ae..c29830e 100644
> > --- a/migration.c
> > +++ b/migration.c
> > @@ -107,11 +107,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();
> >  }
> >  
> >  void process_incoming_migration(QEMUFile *f)
> > diff --git a/monitor.c b/monitor.c
> > index 32a6e74..bf974b4 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 a8a53ef..aa631eb 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -2143,6 +2143,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 febd2ea..7991f2e 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -523,6 +523,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_RUNNING;
> >  
> >  typedef struct {
> >      RunState from;
> > @@ -546,6 +547,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 },
> > @@ -556,6 +558,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 },
> > @@ -585,11 +588,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;
> > @@ -599,6 +630,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 */
> > 

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

[Hu Tao]

On Thu, Feb 28, 2013 at 02:23:42PM +0100, Jan Kiszka wrote:
> On 2013-02-28 13:13, 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).
> 
> I just wonder what will happen if I interrupted the guest via gdb and
> then issue "monitor system_reset", also via gdb - common pattern if you
> set a breakpoint on some BUG() or fault handler and then want to restart
> the guest. Will the guest continue then while gdb thinks it is still
> stopped? Likely, we do not differentiate between gdb-initiated stops and
> the rest. Could you clarify?

Guest won't continue unless issue gdb "continue". Anyway, I'll seperate
this patch, as Paolo requested.

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

[Hu Tao]

On Mon, Mar 04, 2013 at 10:32:17AM +0100, Paolo Bonzini wrote:
> Il 28/02/2013 13:13, Hu Tao ha scritto:
> > 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).
> 
> This is also debatable.  In particular, restarting an INTERNAL_ERROR
> guest makes it harder to inspect the state at the time of the failure.
> 
> INTERNAL_ERROR should never happen, let's separate this patch too.

Sure.

> 
> Paolo

Re: [PATCH v13 4/8] add a new runstate: RUN_STATE_GUEST_PANICKED

[Hu Tao]

On Mon, Mar 04, 2013 at 10:40:15AM +0100, Paolo Bonzini wrote:
> Il 28/02/2013 13:13, Hu Tao ha scritto:
> > The guest will be in this state when it is panicked.
> > 
> > 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 c29830e..fa17b82 100644
> > --- a/migration.c
> > +++ b/migration.c
> > @@ -698,6 +698,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 28b070f..8f1d138 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 3d08e1a..51d4922 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -536,6 +536,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 },
> 
> Is this a consequence of the first patch?

Yes.

> 
> >      { RUN_STATE_INTERNAL_ERROR, RUN_STATE_RUNNING },
> >      { RUN_STATE_INTERNAL_ERROR, RUN_STATE_FINISH_MIGRATE },
> > @@ -549,6 +550,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 },
> 
> Impossible.  GUEST_PANICKED requires an instruction to be executed in
> the guest, so it should first go to RUNNING.
> 
> >      { RUN_STATE_PRELAUNCH, RUN_STATE_RUNNING },
> >      { RUN_STATE_PRELAUNCH, RUN_STATE_FINISH_MIGRATE },
> > @@ -559,6 +561,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 },
> 
> Is it also for the first patch?

Yes.

> 
> >      { RUN_STATE_RUNNING, RUN_STATE_DEBUG },
> >      { RUN_STATE_RUNNING, RUN_STATE_INTERNAL_ERROR },
> > @@ -569,6 +572,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 },
> 
> This one is obviously ok.
> 
> >      { RUN_STATE_SAVE_VM, RUN_STATE_RUNNING },
> >  
> > @@ -583,6 +587,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 },
> 
> Like SHUTDOWN, it should go first to PAUSED and then to RUNNING.  A
> GUEST_PANICKED -> RUNNING transition is not possible.  You're seeing it
> because you lack the addition of GUEST_PANICKED here:
> 
>         if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
>             runstate_check(RUN_STATE_SHUTDOWN)) {
>             runstate_set(RUN_STATE_PAUSED);
>         }
> 
> I think you should first move the INTERNAL_ERROR || SHUTDOWN checks to a
> separate function, so that you can then add GUEST_PANICKED.

Will 

         if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
             runstate_check(RUN_STATE_SHUTDOWN) ||
             runstate_check(RUN_STATE_GUEST_PANICKED)) {
             runstate_set(RUN_STATE_PAUSED);
         }

be OK? Or I must be misunderstanding you.

> 
> Paolo
> 
> >      { RUN_STATE_MAX, RUN_STATE_MAX },
> >  };
> >  
> > @@ -2001,7 +2009,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();
> >          }
> > 

Re: [PATCH v13 5/8] add a new qevent: QEVENT_GUEST_PANICKED

[Hu Tao]

On Fri, Mar 01, 2013 at 09:31:47AM -0700, Eric Blake wrote:
> On 02/28/2013 05:13 AM, Hu Tao wrote:
> > 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(+)
> 
> Missing documentation in QMP/qmp-events.txt

Thanks, will add it.

Re: [PATCH v13 1/8] save/load cpu runstate

[Paolo Bonzini]

Il 05/03/2013 03:33, Hu Tao ha scritto:
> On Mon, Mar 04, 2013 at 10:30:48AM +0100, Paolo Bonzini wrote:
>> Il 28/02/2013 13:13, Hu Tao ha scritto:
>>> 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.
>>
>> I don't think this feature is worth breaking backwards migration
>> compatibility.  It is usually handled at a higher-level (management,
>> like libvirt).
> 
> If guest panic happens during migration, runstate will still be running
> on destination host without this patch. But, it does be a problem to break
> backwards migration compatibility.

Yes, but the source management (libvirt for example) will have the
occasion to observe the change and cancel the migration or forward the
information on the wire.  There are already similar reasons why libvirt
always starts guests with -S.

Paolo

Re: [PATCH v13 4/8] add a new runstate: RUN_STATE_GUEST_PANICKED

[Paolo Bonzini]

Il 05/03/2013 04:17, Hu Tao ha scritto:
> Will 
> 
>          if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
>              runstate_check(RUN_STATE_SHUTDOWN) ||
>              runstate_check(RUN_STATE_GUEST_PANICKED)) {
>              runstate_set(RUN_STATE_PAUSED);
>          }
> 
> be OK? Or I must be misunderstanding you.
> 

Please move

       return (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
               runstate_check(RUN_STATE_SHUTDOWN) ||
               runstate_check(RUN_STATE_GUEST_PANICKED));

to a separate function (runstate_needs_reset for example), so that you
can reuse it in the two or three places that need it.

Paolo

Re: [PATCH v13 0/8] pv event interface between host and guest

[Hu Tao]

On Sun, Mar 03, 2013 at 11:17:38AM +0200, Gleb Natapov wrote:
> On Thu, Feb 28, 2013 at 08:13:10PM +0800, Hu Tao wrote:
> > 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.
> > 
> What other event do you have in mind? Is interface generic enough to
> accommodate future, yet unknown, events. It allows to pass only one
> integer specifying even type, what if additional info is needed? My be

guest crash, lockup, or warning.[1] But the first purpose is to do panic
notification(panic event). Since at the point the guest is panicked, I
think it's better to keep the interface as simple as possible.

[1] http://wiki.qemu.org/Features/PVCrashDetection

> stop pretending that device is generic and make it do once thing but do

you mean make the interface just do panic notification?

> it well? For generic even passing interface (whatever it may be needed
> for) much more powerful virtio should be used.
> 
> On implementation itself I do not understand why is this kvm specific.
> The only thing that makes it so is that you hook device initialization
> into guest kvm initialization code, but this is obviously incorrect.
> What stops QEMU tcg or Xen from reusing the same device for the same
> purpose except the artificial limitation in a guest.
> 
> Reading data from a random ioports is not how you discover platform
> devices in 21 century (and the data you read from unassigned port is not
> guarantied to be zero, it may depend on QEMU version), you use ACPI for
> that and Marcelo already pointed that to you. Having little knowledge of
> ACPI (we all do) is not a good reason to not doing it. We probably need
> to reserve QEMU specific ACPI Plug and Play hardware ID to define our own

Do we have to request the ID from some orgnazation?

> devices. After that you will be able to create device with _HID(QEMU0001)

QMU0001, I think. EISA ID requires it to have only 3 letters for PNP ID.

> in DSDT that supplies address information (ioport to use) and capability
> supported. Guest uses acpi_get_devices() to discover a platform device by
> its name (QEMU0001).  Then you put the driver for the platform device
> into drivers/platform/x86/ and QEMU/kvm/Xen all will be able to use it.

Thanks for the information!

> 
> On QEMU side of things I cannot comment much on how QOMified the device
> is (it should be), I hope other reviews will verify it, but I noticed
> that device is only initialized for PIIX, what about Q35?
> 
> --
> 			Gleb.

Re: [PATCH v13 0/8] pv event interface between host and guest

[Hu Tao]

Hi,

On Mon, Mar 04, 2013 at 11:05:37AM +0100, Paolo Bonzini wrote:
> Il 03/03/2013 10:17, Gleb Natapov ha scritto:
> > On Thu, Feb 28, 2013 at 08:13:10PM +0800, Hu Tao wrote:
> >> 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.
> >>
> > What other event do you have in mind? Is interface generic enough to
> > accommodate future, yet unknown, events. It allows to pass only one
> > integer specifying even type, what if additional info is needed? My be
> > stop pretending that device is generic and make it do once thing but do
> > it well? For generic even passing interface (whatever it may be needed
> > for) much more powerful virtio should be used.
> > 
> > On implementation itself I do not understand why is this kvm specific.
> > The only thing that makes it so is that you hook device initialization
> > into guest kvm initialization code, but this is obviously incorrect.
> > What stops QEMU tcg or Xen from reusing the same device for the same
> > purpose except the artificial limitation in a guest.
> 
> Agreed.
> 
> > Reading data from a random ioports is not how you discover platform
> > devices in 21 century (and the data you read from unassigned port is not
> > guarantied to be zero, it may depend on QEMU version), you use ACPI for
> > that and Marcelo already pointed that to you. Having little knowledge of
> > ACPI (we all do) is not a good reason to not doing it. We probably need
> > to reserve QEMU specific ACPI Plug and Play hardware ID to define our own
> > devices. After that you will be able to create device with _HID(QEMU0001)
> > in DSDT that supplies address information (ioport to use) and capability
> > supported.
> 
> Please also document this HID in a new file in the QEMU docs/ directory.
> 
> > Guest uses acpi_get_devices() to discover a platform device by
> > its name (QEMU0001).  Then you put the driver for the platform device
> > into drivers/platform/x86/ and QEMU/kvm/Xen all will be able to use it.
> 
> Just to clarify it for Hu Tao, the read from a random ioport is how the
> ACPI code will detect presence of the device.
> 
> Something like this should work (in SeaBIOS's src/acpi-dsdt-isa.dsl):
> 
>     Device(PEVT) {
>         Name(_HID, EisaId("QEMU0001"))
>         OperationRegion(PEOR, SystemIO, 0x505, 0x01)
>         Field(PEOR, ByteAcc, NoLock, Preserve) {
>             PEPT,   8,
>         }
> 
>         Method(_STA, 0, NotSerialized) {
>             Store(PEPT, Local0)
>             If (LEqual(Local0, Zero)) {
>                 Return (0x00)
>             } Else {
>                 Return (0x0F)
>             }
>         }

IIUC, here _STA reads from ioport 0x505, if the result is 0, then the
device is not present. Otherwise, the device is present. But as Gleb
said, ''the data you read from unassigned port is not guarantied to be
zero, it may depend on QEMU version''. What should I do to tell if the
device is present or not correctly?

> 
>         Name(_CRS, ResourceTemplate() {
>             IO(Decode16, 0x505, 0x505, 0x01, 0x01)
>         })
>     }
> 
> Please test this with a QEMU option like "-M pc-1.4".  The device should
> _not_ be detected if you're doing it right.

because there is no corresponding device driver?

> 
> > On QEMU side of things I cannot comment much on how QOMified the device
> > is (it should be),
> 
> Please make the device target-independent.  It can be used on non-x86
> architectures that have I/O ports.  You should make the port
> configurable using a property (DEFINE_PROPERTY_INT16 or something like
> that), with a default of 0x505.

OK.

> 
> All the panicked_action is not necessary in my opinion.  We have it for
> watchdogs, but that's really a legacy thing.  Let libvirt do it, and
> always make the guest panic perform the PANICKED_PAUSE action.

OK.

> 
> If you do it properly, a lot (really a lot) of code will go away.

Thanks!

> 
> > I hope other reviews will verify it, but I noticed
> > that device is only initialized for PIIX, what about Q35?
> 
> Yup.
> 
> Paolo

Re: [PATCH v13 4/8] add a new runstate: RUN_STATE_GUEST_PANICKED

[Hu Tao]

On Tue, Mar 05, 2013 at 09:26:18AM +0100, Paolo Bonzini wrote:
> Il 05/03/2013 04:17, Hu Tao ha scritto:
> > Will 
> > 
> >          if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
> >              runstate_check(RUN_STATE_SHUTDOWN) ||
> >              runstate_check(RUN_STATE_GUEST_PANICKED)) {
> >              runstate_set(RUN_STATE_PAUSED);
> >          }
> > 
> > be OK? Or I must be misunderstanding you.
> > 
> 
> Please move
> 
>        return (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
>                runstate_check(RUN_STATE_SHUTDOWN) ||
>                runstate_check(RUN_STATE_GUEST_PANICKED));
> 
> to a separate function (runstate_needs_reset for example), so that you
> can reuse it in the two or three places that need it.

See it now. Thanks!

Re: [PATCH v13 0/8] pv event interface between host and guest

[Paolo Bonzini]

Il 06/03/2013 09:56, Hu Tao ha scritto:
>> > 
>> > Something like this should work (in SeaBIOS's src/acpi-dsdt-isa.dsl):
>> > 
>> >     Device(PEVT) {
>> >         Name(_HID, EisaId("QEMU0001"))
>> >         OperationRegion(PEOR, SystemIO, 0x505, 0x01)
>> >         Field(PEOR, ByteAcc, NoLock, Preserve) {
>> >             PEPT,   8,
>> >         }
>> > 
>> >         Method(_STA, 0, NotSerialized) {
>> >             Store(PEPT, Local0)
>> >             If (LEqual(Local0, Zero)) {
>> >                 Return (0x00)
>> >             } Else {
>> >                 Return (0x0F)
>> >             }
>> >         }
> IIUC, here _STA reads from ioport 0x505, if the result is 0, then the
> device is not present. Otherwise, the device is present. But as Gleb
> said, ''the data you read from unassigned port is not guarantied to be
> zero, it may depend on QEMU version''. What should I do to tell if the
> device is present or not correctly?

The firmware is tied to the QEMU version, so you can rely on unassigned
ports returning zero.

Later we can change this to use fw-cfg.

Paolo

Re: [Qemu-devel] [PATCH v13 0/8] pv event interface between host and guest

[li guang]

在 2013-03-06三的 10:07 +0100，Paolo Bonzini写道：
> Il 06/03/2013 09:56, Hu Tao ha scritto:
> >> > 
> >> > Something like this should work (in SeaBIOS's src/acpi-dsdt-isa.dsl):
> >> > 
> >> >     Device(PEVT) {
> >> >         Name(_HID, EisaId("QEMU0001"))
> >> >         OperationRegion(PEOR, SystemIO, 0x505, 0x01)
> >> >         Field(PEOR, ByteAcc, NoLock, Preserve) {
> >> >             PEPT,   8,
> >> >         }
> >> > 
> >> >         Method(_STA, 0, NotSerialized) {
> >> >             Store(PEPT, Local0)
> >> >             If (LEqual(Local0, Zero)) {
> >> >                 Return (0x00)
> >> >             } Else {
> >> >                 Return (0x0F)
> >> >             }
> >> >         }
> > IIUC, here _STA reads from ioport 0x505, if the result is 0, then the
> > device is not present. Otherwise, the device is present. But as Gleb
> > said, ''the data you read from unassigned port is not guarantied to be
> > zero, it may depend on QEMU version''. What should I do to tell if the
> > device is present or not correctly?
> 
> The firmware is tied to the QEMU version, so you can rely on unassigned
> ports returning zero.
> 

but what if we happen to transfer data end by 0x0,
here, will this device(QEMU0001) disappear?
(by this asl code snippet, I think it will)
so, if we transfer data(not 0x0) device appear,
then, it will disappear(if come across 0x0),
can this happen?
or should we use another status io port e.g.
0x506 to handle this condition?

> Later we can change this to use fw-cfg.
> 
> Paolo
> 
> 

Re: [PATCH v13 0/8] pv event interface between host and guest

[Gleb Natapov]

On Wed, Mar 06, 2013 at 04:46:58PM +0800, Hu Tao wrote:
> On Sun, Mar 03, 2013 at 11:17:38AM +0200, Gleb Natapov wrote:
> > On Thu, Feb 28, 2013 at 08:13:10PM +0800, Hu Tao wrote:
> > > 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.
> > > 
> > What other event do you have in mind? Is interface generic enough to
> > accommodate future, yet unknown, events. It allows to pass only one
> > integer specifying even type, what if additional info is needed? My be
> 
> guest crash, lockup, or warning.[1] But the first purpose is to do panic
> notification(panic event). Since at the point the guest is panicked, I
> think it's better to keep the interface as simple as possible.
> 
> [1] http://wiki.qemu.org/Features/PVCrashDetection
> 
> > stop pretending that device is generic and make it do once thing but do
> 
> you mean make the interface just do panic notification?
> 
Yes.

> > it well? For generic even passing interface (whatever it may be needed
> > for) much more powerful virtio should be used.
> > 
> > On implementation itself I do not understand why is this kvm specific.
> > The only thing that makes it so is that you hook device initialization
> > into guest kvm initialization code, but this is obviously incorrect.
> > What stops QEMU tcg or Xen from reusing the same device for the same
> > purpose except the artificial limitation in a guest.
> > 
> > Reading data from a random ioports is not how you discover platform
> > devices in 21 century (and the data you read from unassigned port is not
> > guarantied to be zero, it may depend on QEMU version), you use ACPI for
> > that and Marcelo already pointed that to you. Having little knowledge of
> > ACPI (we all do) is not a good reason to not doing it. We probably need
> > to reserve QEMU specific ACPI Plug and Play hardware ID to define our own
> 
> Do we have to request the ID from some orgnazation?
> 
A Plug and Play ID or ACPI ID can be obtained by sending e-mail to pnpid@microsoft.com.

> > devices. After that you will be able to create device with _HID(QEMU0001)
> 
> QMU0001, I think. EISA ID requires it to have only 3 letters for PNP ID.
Right, but I am not sure we need EISA ID here. Why not ACPI ID like in
the example from the spec:

Name (_HID, "MSFT0003") // Vendor-defined device

--
			Gleb.

Re: [PATCH v13 0/8] pv event interface between host and guest

[Gleb Natapov]

On Wed, Mar 06, 2013 at 10:07:31AM +0100, Paolo Bonzini wrote:
> Il 06/03/2013 09:56, Hu Tao ha scritto:
> >> > 
> >> > Something like this should work (in SeaBIOS's src/acpi-dsdt-isa.dsl):
> >> > 
> >> >     Device(PEVT) {
> >> >         Name(_HID, EisaId("QEMU0001"))
> >> >         OperationRegion(PEOR, SystemIO, 0x505, 0x01)
> >> >         Field(PEOR, ByteAcc, NoLock, Preserve) {
> >> >             PEPT,   8,
> >> >         }
> >> > 
> >> >         Method(_STA, 0, NotSerialized) {
> >> >             Store(PEPT, Local0)
> >> >             If (LEqual(Local0, Zero)) {
> >> >                 Return (0x00)
> >> >             } Else {
> >> >                 Return (0x0F)
> >> >             }
> >> >         }
> > IIUC, here _STA reads from ioport 0x505, if the result is 0, then the
> > device is not present. Otherwise, the device is present. But as Gleb
> > said, ''the data you read from unassigned port is not guarantied to be
> > zero, it may depend on QEMU version''. What should I do to tell if the
> > device is present or not correctly?
> 
> The firmware is tied to the QEMU version, so you can rely on unassigned
> ports returning zero.
> 
> Later we can change this to use fw-cfg.
> 
I thought we agreed to do it from the start :)

--
			Gleb.

Re: [PATCH v13 0/8] pv event interface between host and guest

[Paolo Bonzini]

> On Wed, Mar 06, 2013 at 10:07:31AM +0100, Paolo Bonzini wrote:
> > Il 06/03/2013 09:56, Hu Tao ha scritto:
> > >> > 
> > >> > Something like this should work (in SeaBIOS's
> > >> > src/acpi-dsdt-isa.dsl):
> > >> > 
> > >> >     Device(PEVT) {
> > >> >         Name(_HID, EisaId("QEMU0001"))
> > >> >         OperationRegion(PEOR, SystemIO, 0x505, 0x01)
> > >> >         Field(PEOR, ByteAcc, NoLock, Preserve) {
> > >> >             PEPT,   8,
> > >> >         }
> > >> > 
> > >> >         Method(_STA, 0, NotSerialized) {
> > >> >             Store(PEPT, Local0)
> > >> >             If (LEqual(Local0, Zero)) {
> > >> >                 Return (0x00)
> > >> >             } Else {
> > >> >                 Return (0x0F)
> > >> >             }
> > >> >         }
> > > IIUC, here _STA reads from ioport 0x505, if the result is 0, then the
> > > device is not present. Otherwise, the device is present. But as Gleb
> > > said, ''the data you read from unassigned port is not guarantied to be
> > > zero, it may depend on QEMU version''. What should I do to tell if the
> > > device is present or not correctly?
> > 
> > The firmware is tied to the QEMU version, so you can rely on
> > unassigned ports returning zero.
> > 
> > Later we can change this to use fw-cfg.
> 
> I thought we agreed to do it from the start :)

Then Hu will need to patch the _STA method.

Paolo

Re: [PATCH v13 0/8] pv event interface between host and guest

[Gleb Natapov]

On Wed, Mar 06, 2013 at 04:48:17AM -0500, Paolo Bonzini wrote:
> 
> > On Wed, Mar 06, 2013 at 10:07:31AM +0100, Paolo Bonzini wrote:
> > > Il 06/03/2013 09:56, Hu Tao ha scritto:
> > > >> > 
> > > >> > Something like this should work (in SeaBIOS's
> > > >> > src/acpi-dsdt-isa.dsl):
> > > >> > 
> > > >> >     Device(PEVT) {
> > > >> >         Name(_HID, EisaId("QEMU0001"))
> > > >> >         OperationRegion(PEOR, SystemIO, 0x505, 0x01)
> > > >> >         Field(PEOR, ByteAcc, NoLock, Preserve) {
> > > >> >             PEPT,   8,
> > > >> >         }
> > > >> > 
> > > >> >         Method(_STA, 0, NotSerialized) {
> > > >> >             Store(PEPT, Local0)
> > > >> >             If (LEqual(Local0, Zero)) {
> > > >> >                 Return (0x00)
> > > >> >             } Else {
> > > >> >                 Return (0x0F)
> > > >> >             }
> > > >> >         }
> > > > IIUC, here _STA reads from ioport 0x505, if the result is 0, then the
> > > > device is not present. Otherwise, the device is present. But as Gleb
> > > > said, ''the data you read from unassigned port is not guarantied to be
> > > > zero, it may depend on QEMU version''. What should I do to tell if the
> > > > device is present or not correctly?
> > > 
> > > The firmware is tied to the QEMU version, so you can rely on
> > > unassigned ports returning zero.
> > > 
> > > Later we can change this to use fw-cfg.
> > 
> > I thought we agreed to do it from the start :)
> 
> Then Hu will need to patch the _STA method.
> 
_STA and _CRS.

--
			Gleb.