[Qemu-devel] [PATCH v18 0/7] Add pvpanic device to deal with guest panic event

[Qemu-devel] [PATCH v18 0/7] Add pvpanic device to deal with guest panic event

[Hu Tao]

This series introduces a new simulated device, pvpanic, to notify
qemu when guest panic event happens.

Along with this series, there are two patches to add seabios ACPI
driver and kernel ACPI driver for the device, respectively.

Tested with:

  - qemu(kvm)/qemu(tcg)
  - qemu piix/q35
  - default ioport/custom ioport

Changes from v17:

  - create pvpanic device by default for machine 1.5
  - rebase on top of latest git tree(hw directories changed a lot)
  - integrate Christian's patch for s390

v17: https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg01028.html


Christian Borntraeger (1):
  Wire up disabled wait a panicked event on s390

Hu Tao (6):
  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
  pvpanic: pass configurable ioport to seabios
  pvpanic: add document of pvpanic
  pvpanic: create pvpanic by default for machine 1.5

 QMP/qmp-events.txt        |  14 +++++
 docs/specs/pvpanic.txt    |  37 +++++++++++++
 hw/i386/pc_piix.c         |  16 +++++-
 hw/i386/pc_q35.c          |  15 ++++-
 hw/misc/Makefile.objs     |   2 +
 hw/misc/pvpanic.c         | 137 ++++++++++++++++++++++++++++++++++++++++++++++
 hw/nvram/fw_cfg.c         |   8 ++-
 include/hw/i386/pc.h      |   3 +
 include/hw/nvram/fw_cfg.h |   2 +
 include/monitor/monitor.h |   1 +
 include/sysemu/sysemu.h   |   1 +
 monitor.c                 |   1 +
 qapi-schema.json          |   5 +-
 qmp.c                     |   3 +-
 target-s390x/kvm.c        |  17 +++++-
 vl.c                      |  13 ++++-
 16 files changed, 263 insertions(+), 12 deletions(-)
 create mode 100644 docs/specs/pvpanic.txt
 create mode 100644 hw/misc/pvpanic.c

-- 
1.8.1.4

[Qemu-devel] [PATCH v18 1/7] 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>
---
 include/sysemu/sysemu.h |  1 +
 qapi-schema.json        |  5 ++++-
 qmp.c                   |  3 +--
 vl.c                    | 13 +++++++++++--
 4 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 6578782..b6fffc8 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -22,6 +22,7 @@ int qemu_uuid_parse(const char *str, uint8_t *uuid);
 bool runstate_check(RunState state);
 void runstate_set(RunState new_state);
 int runstate_is_running(void);
+bool runstate_needs_reset(void);
 typedef struct vm_change_state_entry VMChangeStateEntry;
 typedef void VMChangeStateHandler(void *opaque, int running, RunState state);
 
diff --git a/qapi-schema.json b/qapi-schema.json
index db542f6..09ddfb5 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -174,11 +174,14 @@
 # @suspended: guest is suspended (ACPI S3)
 #
 # @watchdog: the watchdog action is configured to pause and has been triggered
+#
+# @guest-panicked: guest has been panicked as a result of guest OS panic
 ##
 { '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 55b056b..a1ebb5d 100644
--- a/qmp.c
+++ b/qmp.c
@@ -149,8 +149,7 @@ void qmp_cont(Error **errp)
 {
     Error *local_err = NULL;
 
-    if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
-               runstate_check(RUN_STATE_SHUTDOWN)) {
+    if (runstate_needs_reset()) {
         error_set(errp, QERR_RESET_REQUIRED);
         return;
     } else if (runstate_check(RUN_STATE_SUSPENDED)) {
diff --git a/vl.c b/vl.c
index d694a90..5f3cf96 100644
--- a/vl.c
+++ b/vl.c
@@ -596,6 +596,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 },
 
@@ -610,6 +611,8 @@ 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_PAUSED },
+
     { RUN_STATE_MAX, RUN_STATE_MAX },
 };
 
@@ -651,6 +654,13 @@ int runstate_is_running(void)
     return runstate_check(RUN_STATE_RUNNING);
 }
 
+bool runstate_needs_reset(void)
+{
+    return runstate_check(RUN_STATE_INTERNAL_ERROR) ||
+        runstate_check(RUN_STATE_SHUTDOWN) ||
+        runstate_check(RUN_STATE_GUEST_PANICKED);
+}
+
 StatusInfo *qmp_query_status(Error **errp)
 {
     StatusInfo *info = g_malloc0(sizeof(*info));
@@ -2007,8 +2017,7 @@ static bool main_loop_should_exit(void)
         cpu_synchronize_all_states();
         qemu_system_reset(VMRESET_REPORT);
         resume_all_vcpus();
-        if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
-            runstate_check(RUN_STATE_SHUTDOWN)) {
+        if (runstate_needs_reset()) {
             runstate_set(RUN_STATE_PAUSED);
         }
     }
-- 
1.8.1.4

[Qemu-devel] [PATCH v18 2/7] add a new qevent: QEVENT_GUEST_PANICKED

[Hu Tao]

This event will be emited when qemu detects guest panic.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 QMP/qmp-events.txt        | 14 ++++++++++++++
 include/monitor/monitor.h |  1 +
 monitor.c                 |  1 +
 3 files changed, 16 insertions(+)

diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index dcc826d..92fe5fb 100644
--- a/QMP/qmp-events.txt
+++ b/QMP/qmp-events.txt
@@ -446,3 +446,17 @@ Example:
 
 Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is
 followed respectively by the RESET, SHUTDOWN, or STOP events.
+
+GUEST_PANICKED
+--------------
+
+Emitted when guest OS panic is detected.
+
+Data:
+
+- "action": Action that has been taken (json-string, currently always "pause").
+
+Example:
+
+{ "event": "GUEST_PANICKED",
+     "data": { "action": "pause" } }
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index b868760..1a6cfcf 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -46,6 +46,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 c897e80..444e297 100644
--- a/monitor.c
+++ b/monitor.c
@@ -496,6 +496,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

[Qemu-devel] [PATCH v18 3/7] introduce a new qom device to deal with panicked event

[Hu Tao]

pvpanic device is used to send guest panic event from guest to qemu.

When guest panic happens, pvpanic device driver will write a event
number to IO port 0x505(which is the IO port occupied by pvpanic device,
by default). On receiving the event, pvpanic device will pause guest
cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 hw/misc/Makefile.objs |   2 +
 hw/misc/pvpanic.c     | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 118 insertions(+)
 create mode 100644 hw/misc/pvpanic.c

diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 03699c3..d72ea83 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -38,3 +38,5 @@ obj-$(CONFIG_OMAP) += omap_tap.o
 obj-$(CONFIG_PXA2XX) += pxa2xx_pcmcia.o
 obj-$(CONFIG_SLAVIO) += slavio_misc.o
 obj-$(CONFIG_ZYNQ) += zynq_slcr.o
+
+common-obj-y += pvpanic.o
diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
new file mode 100644
index 0000000..5118fd7
--- /dev/null
+++ b/hw/misc/pvpanic.c
@@ -0,0 +1,116 @@
+/*
+ * QEMU simulated pvpanic device.
+ *
+ * Copyright Fujitsu, Corp. 2013
+ *
+ * Authors:
+ *     Wen Congyang <wency@cn.fujitsu.com>
+ *     Hu Tao <hutao@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 <qapi/qmp/qobject.h>
+#include <qapi/qmp/qjson.h>
+#include <monitor/monitor.h>
+#include <sysemu/sysemu.h>
+#include <sysemu/kvm.h>
+
+/* The bit of supported pv event */
+#define PVPANIC_F_PANICKED      0
+
+/* The pv event value */
+#define PVPANIC_PANICKED        (1 << PVPANIC_F_PANICKED)
+
+#define TYPE_ISA_PVPANIC_DEVICE    "pvpanic"
+#define ISA_PVPANIC_DEVICE(obj)    \
+    OBJECT_CHECK(PVPanicState, (obj), TYPE_ISA_PVPANIC_DEVICE)
+
+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 handle_event(int event)
+{
+    if (event == PVPANIC_PANICKED) {
+        panicked_mon_event("pause");
+        vm_stop(RUN_STATE_GUEST_PANICKED);
+        return;
+    }
+}
+
+#include "hw/isa/isa.h"
+
+typedef struct PVPanicState {
+    ISADevice parent_obj;
+
+    MemoryRegion io;
+    uint16_t ioport;
+} PVPanicState;
+
+/* return supported events on read */
+static uint64_t pvpanic_ioport_read(void *opaque, hwaddr addr, unsigned size)
+{
+    return PVPANIC_PANICKED;
+}
+
+static void pvpanic_ioport_write(void *opaque, hwaddr addr, uint64_t val,
+                                 unsigned size)
+{
+    handle_event(val);
+}
+
+static const MemoryRegionOps pvpanic_ops = {
+    .read = pvpanic_ioport_read,
+    .write = pvpanic_ioport_write,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+static int pvpanic_isa_initfn(ISADevice *dev)
+{
+    PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
+
+    memory_region_init_io(&s->io, &pvpanic_ops, s, "pvpanic", 1);
+    isa_register_ioport(dev, &s->io, s->ioport);
+
+    return 0;
+}
+
+static Property pvpanic_isa_properties[] = {
+    DEFINE_PROP_UINT16("ioport", PVPanicState, ioport, 0x505),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void pvpanic_isa_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    ISADeviceClass *ic = ISA_DEVICE_CLASS(klass);
+
+    ic->init = pvpanic_isa_initfn;
+    dc->no_user = 1;
+    dc->props = pvpanic_isa_properties;
+}
+
+static TypeInfo pvpanic_isa_info = {
+    .name          = TYPE_ISA_PVPANIC_DEVICE,
+    .parent        = TYPE_ISA_DEVICE,
+    .instance_size = sizeof(PVPanicState),
+    .class_init    = pvpanic_isa_class_init,
+};
+
+static void pvpanic_register_types(void)
+{
+    type_register_static(&pvpanic_isa_info);
+}
+
+type_init(pvpanic_register_types)
-- 
1.8.1.4

[Qemu-devel] [PATCH v18 4/7] pvpanic: pass configurable ioport to seabios

[Hu Tao]

This lets seabios patch the corresponding SSDT entry.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 hw/misc/pvpanic.c         | 14 ++++++++++++++
 hw/nvram/fw_cfg.c         |  8 +++++++-
 include/hw/nvram/fw_cfg.h |  2 ++
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index 5118fd7..b2fd413 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -18,6 +18,8 @@
 #include <sysemu/sysemu.h>
 #include <sysemu/kvm.h>
 
+#include "hw/nvram/fw_cfg.h"
+
 /* The bit of supported pv event */
 #define PVPANIC_F_PANICKED      0
 
@@ -79,10 +81,22 @@ static const MemoryRegionOps pvpanic_ops = {
 static int pvpanic_isa_initfn(ISADevice *dev)
 {
     PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
+    static bool port_configured;
+    void *fw_cfg;
 
     memory_region_init_io(&s->io, &pvpanic_ops, s, "pvpanic", 1);
     isa_register_ioport(dev, &s->io, s->ioport);
 
+    if (!port_configured) {
+        fw_cfg = object_resolve_path("/machine/fw_cfg", NULL);
+        if (fw_cfg) {
+            fw_cfg_add_file(fw_cfg, "etc/pvpanic-port",
+                            g_memdup(&s->ioport, sizeof(s->ioport)),
+                            sizeof(s->ioport));
+            port_configured = true;
+        }
+    }
+
     return 0;
 }
 
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 97bba87..1a7e49c 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -489,11 +489,17 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
     dev = qdev_create(NULL, "fw_cfg");
     qdev_prop_set_uint32(dev, "ctl_iobase", ctl_port);
     qdev_prop_set_uint32(dev, "data_iobase", data_port);
-    qdev_init_nofail(dev);
     d = SYS_BUS_DEVICE(dev);
 
     s = DO_UPCAST(FWCfgState, busdev.qdev, dev);
 
+    if (!object_resolve_path("/machine/fw_cfg", NULL)) {
+        object_property_add_child(qdev_get_machine(), "fw_cfg", OBJECT(s),
+                                  NULL);
+    }
+
+    qdev_init_nofail(dev);
+
     if (ctl_addr) {
         sysbus_mmio_map(d, 0, ctl_addr);
     }
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 05c8df1..07cc941 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -1,6 +1,8 @@
 #ifndef FW_CFG_H
 #define FW_CFG_H
 
+#include "exec/hwaddr.h"
+
 #define FW_CFG_SIGNATURE        0x00
 #define FW_CFG_ID               0x01
 #define FW_CFG_UUID             0x02
-- 
1.8.1.4

[Qemu-devel] [PATCH v18 5/7] pvpanic: add document of pvpanic

[Hu Tao]

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

diff --git a/docs/specs/pvpanic.txt b/docs/specs/pvpanic.txt
new file mode 100644
index 0000000..d72d667
--- /dev/null
+++ b/docs/specs/pvpanic.txt
@@ -0,0 +1,37 @@
+PVPANIC DEVICE
+==============
+
+pvpanic device is a simulated ISA device, through which a guest panic
+event is sent to qemu, and a QMP event is generated. This allows
+management apps (e.g. libvirt) to be notified and respond to the event.
+
+The management app has the option of waiting for GUEST_PANICKED events,
+and/or polling for guest-panicked RunState, to learn when the pvpanic
+device has fired a panic event.
+
+ISA Interface
+-------------
+
+pvpanic uses port 0x505 to receive a panic event from the guest. On
+write, bit 0 is set to indicate guest panic has happened. On read, bit
+0 is set to indicate guest panic notification is supported. Remaining
+bits are reserved, and should be written as 0, and ignored on read.
+
+ACPI Interface
+--------------
+
+pvpanic device is defined with ACPI ID "QEMU0001". Custom methods:
+
+RDPT:       To determine whether guest panic notification is supported.
+Arguments:  None
+Return:     Returns a byte, bit 0 set to indicate guest panic
+            notification is supported. Other bits are reserved and
+            should be ignored.
+
+WRPT:       To send a guest panic event
+Arguments:  Arg0 is a byte, with bit 0 set to indicate guest panic has
+            happened. Other bits are reserved and should be cleared.
+Return:     None
+
+The ACPI device will automatically refer to the right port in case it
+is modified.
-- 
1.8.1.4

[Qemu-devel] [PATCH v18 6/7] pvpanic: create pvpanic by default for machine 1.5

[Hu Tao]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 hw/i386/pc_piix.c    | 16 ++++++++++++++--
 hw/i386/pc_q35.c     | 15 ++++++++++++++-
 hw/misc/pvpanic.c    |  7 +++++++
 include/hw/i386/pc.h |  3 +++
 4 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index cff8013..8c0643a 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -54,6 +54,8 @@ static const int ide_iobase[MAX_IDE_BUS] = { 0x1f0, 0x170 };
 static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
 static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
 
+static bool has_pvpanic = true;
+
 /* PC hardware initialisation */
 static void pc_init1(MemoryRegion *system_memory,
                      MemoryRegion *system_io,
@@ -217,6 +219,10 @@ static void pc_init1(MemoryRegion *system_memory,
     if (pci_enabled) {
         pc_pci_device_init(pci_bus);
     }
+
+    if (has_pvpanic) {
+        pvpanic_init(isa_bus);
+    }
 }
 
 static void pc_init_pci(QEMUMachineInitArgs *args)
@@ -234,10 +240,16 @@ static void pc_init_pci(QEMUMachineInitArgs *args)
              initrd_filename, cpu_model, 1, 1);
 }
 
+static void pc_init_pci_1_4(QEMUMachineInitArgs *args)
+{
+    has_pvpanic = false;
+    pc_init_pci(args);
+}
+
 static void pc_init_pci_1_3(QEMUMachineInitArgs *args)
 {
     enable_compat_apic_id_mode();
-    pc_init_pci(args);
+    pc_init_pci_1_4(args);
 }
 
 /* PC machine init function for pc-0.14 to pc-1.2 */
@@ -308,7 +320,7 @@ static QEMUMachine pc_i440fx_machine_v1_5 = {
 static QEMUMachine pc_i440fx_machine_v1_4 = {
     .name = "pc-i440fx-1.4",
     .desc = "Standard PC (i440FX + PIIX, 1996)",
-    .init = pc_init_pci,
+    .init = pc_init_pci_1_4,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
         PC_COMPAT_1_4,
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 6ac1a89..cbdbf77 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -45,6 +45,8 @@
 /* ICH9 AHCI has 6 ports */
 #define MAX_SATA_PORTS     6
 
+static bool has_pvpanic = true;
+
 /* set CMOS shutdown status register (index 0xF) as S3_resume(0xFE)
  *    BIOS will read it and start S3 resume at POST Entry */
 static void pc_cmos_set_s3_resume(void *opaque, int irq, int level)
@@ -207,8 +209,19 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
     if (pci_enabled) {
         pc_pci_device_init(host_bus);
     }
+
+    if (has_pvpanic) {
+        pvpanic_init(isa_bus);
+    }
+}
+
+static void pc_q35_init_1_4(QEMUMachineInitArgs *args)
+{
+    has_pvpanic = false;
+    pc_q35_init(args);
 }
 
+
 static QEMUMachine pc_q35_machine_v1_5 = {
     .name = "pc-q35-1.5",
     .alias = "q35",
@@ -221,7 +234,7 @@ static QEMUMachine pc_q35_machine_v1_5 = {
 static QEMUMachine pc_q35_machine_v1_4 = {
     .name = "pc-q35-1.4",
     .desc = "Standard PC (Q35 + ICH9, 2009)",
-    .init = pc_q35_init,
+    .init = pc_q35_init_1_4,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
         PC_COMPAT_1_4,
diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index b2fd413..59160bb 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -19,6 +19,7 @@
 #include <sysemu/kvm.h>
 
 #include "hw/nvram/fw_cfg.h"
+#include "hw/i386/pc.h"
 
 /* The bit of supported pv event */
 #define PVPANIC_F_PANICKED      0
@@ -100,6 +101,12 @@ static int pvpanic_isa_initfn(ISADevice *dev)
     return 0;
 }
 
+int pvpanic_init(ISABus *bus)
+{
+    isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE);
+    return 0;
+}
+
 static Property pvpanic_isa_properties[] = {
     DEFINE_PROP_UINT16("ioport", PVPanicState, ioport, 0x505),
     DEFINE_PROP_END_OF_LIST(),
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 5d40914..37b9fd1 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -178,6 +178,9 @@ static inline bool isa_ne2000_init(ISABus *bus, int base, int irq, NICInfo *nd)
 /* pc_sysfw.c */
 void pc_system_firmware_init(MemoryRegion *rom_memory);
 
+/* pvpanic.c */
+int pvpanic_init(ISABus *bus);
+
 /* e820 types */
 #define E820_RAM        1
 #define E820_RESERVED   2
-- 
1.8.1.4

[Qemu-devel] [PATCH v18 7/7] Wire up disabled wait a panicked event on s390

[Hu Tao]

From: Christian Borntraeger <borntraeger@de.ibm.com>

On s390 the disabled wait state indicates a state of attention.
For example Linux uses that state after a panic. Lets
put the system into panicked state.

An alternative implementation would be to state
disabled-wait <address> instead of pause in the action field.
(e.g. z/OS, z/VM and other classic OSes use the address of the
disabled wait to indicate an error code).

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 target-s390x/kvm.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 644f484..0c111f0 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -34,6 +34,8 @@
 #include "sysemu/kvm.h"
 #include "cpu.h"
 #include "sysemu/device_tree.h"
+#include "qapi/qmp/qjson.h"
+#include "monitor/monitor.h"
 
 /* #define DEBUG_KVM */
 
@@ -705,9 +707,18 @@ static int handle_intercept(S390CPU *cpu)
             r = handle_instruction(cpu, run);
             break;
         case ICPT_WAITPSW:
-            if (s390_del_running_cpu(cpu) == 0 &&
-                is_special_wait_psw(cs)) {
-                qemu_system_shutdown_request();
+            /* disabled wait, since enabled wait is handled in kernel */
+            if (s390_del_running_cpu(cpu) == 0) {
+                if (is_special_wait_psw(cs)) {
+                    qemu_system_shutdown_request();
+                } else {
+                    QObject *data;
+
+                    data = qobject_from_jsonf("{ 'action': %s }", "pause");
+                    monitor_protocol_event(QEVENT_GUEST_PANICKED, data);
+                    qobject_decref(data);
+                    vm_stop(RUN_STATE_GUEST_PANICKED);
+                }
             }
             r = EXCP_HALTED;
             break;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v18 0/7] Add pvpanic device to deal with guest panic event

[Paolo Bonzini]

Il 10/04/2013 05:33, Hu Tao ha scritto:
> This series introduces a new simulated device, pvpanic, to notify
> qemu when guest panic event happens.
> 
> Along with this series, there are two patches to add seabios ACPI
> driver and kernel ACPI driver for the device, respectively.
> 
> Tested with:
> 
>   - qemu(kvm)/qemu(tcg)
>   - qemu piix/q35
>   - default ioport/custom ioport
> 
> Changes from v17:
> 
>   - create pvpanic device by default for machine 1.5
>   - rebase on top of latest git tree(hw directories changed a lot)
>   - integrate Christian's patch for s390
> 
> v17: https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg01028.html
> 
> 
> Christian Borntraeger (1):
>   Wire up disabled wait a panicked event on s390
> 
> Hu Tao (6):
>   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
>   pvpanic: pass configurable ioport to seabios
>   pvpanic: add document of pvpanic
>   pvpanic: create pvpanic by default for machine 1.5
> 
>  QMP/qmp-events.txt        |  14 +++++
>  docs/specs/pvpanic.txt    |  37 +++++++++++++
>  hw/i386/pc_piix.c         |  16 +++++-
>  hw/i386/pc_q35.c          |  15 ++++-
>  hw/misc/Makefile.objs     |   2 +
>  hw/misc/pvpanic.c         | 137 ++++++++++++++++++++++++++++++++++++++++++++++
>  hw/nvram/fw_cfg.c         |   8 ++-
>  include/hw/i386/pc.h      |   3 +
>  include/hw/nvram/fw_cfg.h |   2 +
>  include/monitor/monitor.h |   1 +
>  include/sysemu/sysemu.h   |   1 +
>  monitor.c                 |   1 +
>  qapi-schema.json          |   5 +-
>  qmp.c                     |   3 +-
>  target-s390x/kvm.c        |  17 +++++-
>  vl.c                      |  13 ++++-
>  16 files changed, 263 insertions(+), 12 deletions(-)
>  create mode 100644 docs/specs/pvpanic.txt
>  create mode 100644 hw/misc/pvpanic.c
> 

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

Re: [Qemu-devel] [PATCH v18 3/7] introduce a new qom device to deal with panicked event

[Markus Armbruster]

Hu Tao <hutao@cn.fujitsu.com> writes:

> pvpanic device is used to send guest panic event from guest to qemu.
>
> When guest panic happens, pvpanic device driver will write a event
> number to IO port 0x505(which is the IO port occupied by pvpanic device,
> by default). On receiving the event, pvpanic device will pause guest
> cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.
>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  hw/misc/Makefile.objs |   2 +
>  hw/misc/pvpanic.c     | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 118 insertions(+)
>  create mode 100644 hw/misc/pvpanic.c
>
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index 03699c3..d72ea83 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -38,3 +38,5 @@ obj-$(CONFIG_OMAP) += omap_tap.o
>  obj-$(CONFIG_PXA2XX) += pxa2xx_pcmcia.o
>  obj-$(CONFIG_SLAVIO) += slavio_misc.o
>  obj-$(CONFIG_ZYNQ) += zynq_slcr.o
> +
> +common-obj-y += pvpanic.o
> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
> new file mode 100644
> index 0000000..5118fd7
> --- /dev/null
> +++ b/hw/misc/pvpanic.c
> @@ -0,0 +1,116 @@
> +/*
> + * QEMU simulated pvpanic device.
> + *
> + * Copyright Fujitsu, Corp. 2013
> + *
> + * Authors:
> + *     Wen Congyang <wency@cn.fujitsu.com>
> + *     Hu Tao <hutao@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 <qapi/qmp/qobject.h>
> +#include <qapi/qmp/qjson.h>
> +#include <monitor/monitor.h>
> +#include <sysemu/sysemu.h>
> +#include <sysemu/kvm.h>
> +
> +/* The bit of supported pv event */
> +#define PVPANIC_F_PANICKED      0
> +
> +/* The pv event value */
> +#define PVPANIC_PANICKED        (1 << PVPANIC_F_PANICKED)
> +
> +#define TYPE_ISA_PVPANIC_DEVICE    "pvpanic"
> +#define ISA_PVPANIC_DEVICE(obj)    \
> +    OBJECT_CHECK(PVPanicState, (obj), TYPE_ISA_PVPANIC_DEVICE)
> +
> +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 handle_event(int event)
> +{
> +    if (event == PVPANIC_PANICKED) {
> +        panicked_mon_event("pause");
> +        vm_stop(RUN_STATE_GUEST_PANICKED);
> +        return;
> +    }
> +}

I've asked these questions before, if you answered them, I missed it:

1. Your event value looks like it encodes multiple events as bits.  Only
one bit is defined so far (PVEVENT_F_PANICKED).  But you recognize this
bit only when the other bits are all off.  Why?  Won't we regret this if
we ever want to define additional bits?

2. You silently ignore unrecognized event values.  Shouldn't we log
them?

> +
> +#include "hw/isa/isa.h"
> +
> +typedef struct PVPanicState {
> +    ISADevice parent_obj;
> +
> +    MemoryRegion io;
> +    uint16_t ioport;
> +} PVPanicState;
> +
> +/* return supported events on read */
> +static uint64_t pvpanic_ioport_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    return PVPANIC_PANICKED;
> +}
> +
> +static void pvpanic_ioport_write(void *opaque, hwaddr addr, uint64_t val,
> +                                 unsigned size)
> +{
> +    handle_event(val);
> +}
> +
> +static const MemoryRegionOps pvpanic_ops = {
> +    .read = pvpanic_ioport_read,
> +    .write = pvpanic_ioport_write,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> +
> +static int pvpanic_isa_initfn(ISADevice *dev)
> +{
> +    PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
> +
> +    memory_region_init_io(&s->io, &pvpanic_ops, s, "pvpanic", 1);
> +    isa_register_ioport(dev, &s->io, s->ioport);
> +
> +    return 0;
> +}
> +
> +static Property pvpanic_isa_properties[] = {
> +    DEFINE_PROP_UINT16("ioport", PVPanicState, ioport, 0x505),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void pvpanic_isa_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    ISADeviceClass *ic = ISA_DEVICE_CLASS(klass);
> +
> +    ic->init = pvpanic_isa_initfn;
> +    dc->no_user = 1;
> +    dc->props = pvpanic_isa_properties;
> +}
> +
> +static TypeInfo pvpanic_isa_info = {
> +    .name          = TYPE_ISA_PVPANIC_DEVICE,
> +    .parent        = TYPE_ISA_DEVICE,
> +    .instance_size = sizeof(PVPanicState),
> +    .class_init    = pvpanic_isa_class_init,
> +};
> +
> +static void pvpanic_register_types(void)
> +{
> +    type_register_static(&pvpanic_isa_info);
> +}
> +
> +type_init(pvpanic_register_types)

Re: [Qemu-devel] [PATCH v18 3/7] introduce a new qom device to deal with panicked event

[Hu Tao]

Hi,

On Wed, Apr 10, 2013 at 01:54:04PM +0200, Markus Armbruster wrote:
> Hu Tao <hutao@cn.fujitsu.com> writes:
> 
> > pvpanic device is used to send guest panic event from guest to qemu.
> >
> > When guest panic happens, pvpanic device driver will write a event
> > number to IO port 0x505(which is the IO port occupied by pvpanic device,
> > by default). On receiving the event, pvpanic device will pause guest
> > cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.
> >
> > Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  hw/misc/Makefile.objs |   2 +
> >  hw/misc/pvpanic.c     | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 118 insertions(+)
> >  create mode 100644 hw/misc/pvpanic.c
> >
> > diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> > index 03699c3..d72ea83 100644
> > --- a/hw/misc/Makefile.objs
> > +++ b/hw/misc/Makefile.objs
> > @@ -38,3 +38,5 @@ obj-$(CONFIG_OMAP) += omap_tap.o
> >  obj-$(CONFIG_PXA2XX) += pxa2xx_pcmcia.o
> >  obj-$(CONFIG_SLAVIO) += slavio_misc.o
> >  obj-$(CONFIG_ZYNQ) += zynq_slcr.o
> > +
> > +common-obj-y += pvpanic.o
> > diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
> > new file mode 100644
> > index 0000000..5118fd7
> > --- /dev/null
> > +++ b/hw/misc/pvpanic.c
> > @@ -0,0 +1,116 @@
> > +/*
> > + * QEMU simulated pvpanic device.
> > + *
> > + * Copyright Fujitsu, Corp. 2013
> > + *
> > + * Authors:
> > + *     Wen Congyang <wency@cn.fujitsu.com>
> > + *     Hu Tao <hutao@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 <qapi/qmp/qobject.h>
> > +#include <qapi/qmp/qjson.h>
> > +#include <monitor/monitor.h>
> > +#include <sysemu/sysemu.h>
> > +#include <sysemu/kvm.h>
> > +
> > +/* The bit of supported pv event */
> > +#define PVPANIC_F_PANICKED      0
> > +
> > +/* The pv event value */
> > +#define PVPANIC_PANICKED        (1 << PVPANIC_F_PANICKED)
> > +
> > +#define TYPE_ISA_PVPANIC_DEVICE    "pvpanic"
> > +#define ISA_PVPANIC_DEVICE(obj)    \
> > +    OBJECT_CHECK(PVPanicState, (obj), TYPE_ISA_PVPANIC_DEVICE)
> > +
> > +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 handle_event(int event)
> > +{
> > +    if (event == PVPANIC_PANICKED) {
> > +        panicked_mon_event("pause");
> > +        vm_stop(RUN_STATE_GUEST_PANICKED);
> > +        return;
> > +    }
> > +}
> 
> I've asked these questions before, if you answered them, I missed it:

Sorry, I must have missed them.

> 
> 1. Your event value looks like it encodes multiple events as bits.  Only
> one bit is defined so far (PVEVENT_F_PANICKED).  But you recognize this

It was the intention to support multiple events, but Gleb suggested to do
only panic event. So pvpanic device supports only panic event.

> bit only when the other bits are all off.  Why?  Won't we regret this if
> we ever want to define additional bits?

Other bits are reserved now, and must be written as 0. (see patch 5/7)
If we define these bits in the further for whatever purposes, we have to
update code here.

> 
> 2. You silently ignore unrecognized event values.  Shouldn't we log
> them?

See above.


-- 
Regards,
Hu Tao

Re: [Qemu-devel] [PATCH v18 3/7] introduce a new qom device to deal with panicked event

[Markus Armbruster]

Hu Tao <hutao@cn.fujitsu.com> writes:

> Hi,
>
> On Wed, Apr 10, 2013 at 01:54:04PM +0200, Markus Armbruster wrote:
>> Hu Tao <hutao@cn.fujitsu.com> writes:
>> 
>> > pvpanic device is used to send guest panic event from guest to qemu.
>> >
>> > When guest panic happens, pvpanic device driver will write a event
>> > number to IO port 0x505(which is the IO port occupied by pvpanic device,
>> > by default). On receiving the event, pvpanic device will pause guest
>> > cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.
>> >
>> > Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
>> > ---
>> >  hw/misc/Makefile.objs |   2 +
>> >  hw/misc/pvpanic.c | 116
>> > ++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  2 files changed, 118 insertions(+)
>> >  create mode 100644 hw/misc/pvpanic.c
>> >
>> > diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
>> > index 03699c3..d72ea83 100644
>> > --- a/hw/misc/Makefile.objs
>> > +++ b/hw/misc/Makefile.objs
>> > @@ -38,3 +38,5 @@ obj-$(CONFIG_OMAP) += omap_tap.o
>> >  obj-$(CONFIG_PXA2XX) += pxa2xx_pcmcia.o
>> >  obj-$(CONFIG_SLAVIO) += slavio_misc.o
>> >  obj-$(CONFIG_ZYNQ) += zynq_slcr.o
>> > +
>> > +common-obj-y += pvpanic.o
>> > diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
>> > new file mode 100644
>> > index 0000000..5118fd7
>> > --- /dev/null
>> > +++ b/hw/misc/pvpanic.c
>> > @@ -0,0 +1,116 @@
>> > +/*
>> > + * QEMU simulated pvpanic device.
>> > + *
>> > + * Copyright Fujitsu, Corp. 2013
>> > + *
>> > + * Authors:
>> > + *     Wen Congyang <wency@cn.fujitsu.com>
>> > + *     Hu Tao <hutao@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 <qapi/qmp/qobject.h>
>> > +#include <qapi/qmp/qjson.h>
>> > +#include <monitor/monitor.h>
>> > +#include <sysemu/sysemu.h>
>> > +#include <sysemu/kvm.h>
>> > +
>> > +/* The bit of supported pv event */
>> > +#define PVPANIC_F_PANICKED      0
>> > +
>> > +/* The pv event value */
>> > +#define PVPANIC_PANICKED        (1 << PVPANIC_F_PANICKED)
>> > +
>> > +#define TYPE_ISA_PVPANIC_DEVICE    "pvpanic"
>> > +#define ISA_PVPANIC_DEVICE(obj)    \
>> > +    OBJECT_CHECK(PVPanicState, (obj), TYPE_ISA_PVPANIC_DEVICE)
>> > +
>> > +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 handle_event(int event)
>> > +{
>> > +    if (event == PVPANIC_PANICKED) {
>> > +        panicked_mon_event("pause");
>> > +        vm_stop(RUN_STATE_GUEST_PANICKED);
>> > +        return;
>> > +    }
>> > +}
>> 
>> I've asked these questions before, if you answered them, I missed it:
>
> Sorry, I must have missed them.
>
>> 
>> 1. Your event value looks like it encodes multiple events as bits.  Only
>> one bit is defined so far (PVEVENT_F_PANICKED).  But you recognize this
>
> It was the intention to support multiple events, but Gleb suggested to do
> only panic event. So pvpanic device supports only panic event.
>
>> bit only when the other bits are all off.  Why?  Won't we regret this if
>> we ever want to define additional bits?
>
> Other bits are reserved now, and must be written as 0. (see patch 5/7)
> If we define these bits in the further for whatever purposes, we have to
> update code here.

Not only that, we have to make sure all combinations of old/new device
and old/new guest device drivers work.

>> 2. You silently ignore unrecognized event values.  Shouldn't we log
>> them?
>
> See above.

PATCH 5/7 describes your your device's guest ABI:

    pvpanic uses port 0x505 to receive a panic event from the guest. On
    write, bit 0 is set to indicate guest panic has happened. On read, bit
    0 is set to indicate guest panic notification is supported. Remaining
    bits are reserved, and should be written as 0, and ignored on read.

For what it's worth, real hardware usually doesn't work that way.  It
usually ignores the bits it doesn't implement, and recognizes the bits
it implements regardless of what's written to the others.

Let's explore extending the device: add another event, and encode it as
bit 1.  Then read returns 3, and the guest may write 0, 1, 2 or 3.

New device, old guest device driver: driver reads 3.  A scrupulously
correct driver masks out the bits it doesn't understand (3 & 1), gets 1
as it expects, and continues to work as before.  A sloppy driver may not
check the bits; also continues to work as before.  A confused driver may
choke on the value 3, and fail, most probably in a way that's visible in
dmesg.

Old device, new guest device driver: driver reads 1, masks out the bits
it doesn't understand (1 & 3), gets 1.  Say the driver wants to report
both events.  A well-behaved driver recognizes that the device doesn't
understand the new event, and writes 1.  Works.  A sloppy driver may
write 3.  Your device ignores that write silently.

I don't like that.  I'd prefer it to work like real hardware usually
does: recognize bit 0 even when other bits are set.

ABI description becomes:

    pvpanic exposes a single I/O port, by default 0x505.  Each bit of
    the port corresponds to an event.

    On read, the bits recognized by the device are set.  Software should
    ignore bits it doesn't recognize.

    On write, the bits not recognized by the device are ignored.
    Software should set only bits both itself and the device recognize.

    Currently, only bit 0 is recognized.  Setting it indicates a guest
    panic has happened.

Regardless of whether you make the device ignore unrecognized bits or
not, please consider logging when a guest sets unrecognized bits.  That
way we can catch misbehaving guest device drivers much more easily.
Recommend to log only the first occurence, to prevent denial of service.

Re: [Qemu-devel] [PATCH v18 5/7] pvpanic: add document of pvpanic

[Markus Armbruster]

Hu Tao <hutao@cn.fujitsu.com> writes:

> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  docs/specs/pvpanic.txt | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 docs/specs/pvpanic.txt
>
> diff --git a/docs/specs/pvpanic.txt b/docs/specs/pvpanic.txt
> new file mode 100644
> index 0000000..d72d667
> --- /dev/null
> +++ b/docs/specs/pvpanic.txt
> @@ -0,0 +1,37 @@
> +PVPANIC DEVICE
> +==============
> +
> +pvpanic device is a simulated ISA device, through which a guest panic
> +event is sent to qemu, and a QMP event is generated. This allows
> +management apps (e.g. libvirt) to be notified and respond to the event.
> +
> +The management app has the option of waiting for GUEST_PANICKED events,
> +and/or polling for guest-panicked RunState, to learn when the pvpanic
> +device has fired a panic event.
> +
> +ISA Interface
> +-------------
> +
> +pvpanic uses port 0x505 to receive a panic event from the guest. On
> +write, bit 0 is set to indicate guest panic has happened. On read, bit
> +0 is set to indicate guest panic notification is supported. Remaining
> +bits are reserved, and should be written as 0, and ignored on read.
> +
> +ACPI Interface
> +--------------
> +
> +pvpanic device is defined with ACPI ID "QEMU0001". Custom methods:
> +
> +RDPT:       To determine whether guest panic notification is supported.
> +Arguments:  None
> +Return:     Returns a byte, bit 0 set to indicate guest panic
> +            notification is supported. Other bits are reserved and
> +            should be ignored.
> +
> +WRPT:       To send a guest panic event
> +Arguments:  Arg0 is a byte, with bit 0 set to indicate guest panic has
> +            happened. Other bits are reserved and should be cleared.
> +Return:     None
> +
> +The ACPI device will automatically refer to the right port in case it
> +is modified.

Here, you implicitly state that the port is configurable.  Section "ISA
Interface" sounds like it was fixed to 0x505.  It's actually
configurable.  Rephrase it a bit there?  See my reply to 3/7 for a
possible wording.

Re: [Qemu-devel] [PATCH v18 3/7] introduce a new qom device to deal with panicked event

[Hu Tao]

Hi,

Sorry for the delay.

On Thu, Apr 11, 2013 at 10:52:02AM +0200, Markus Armbruster wrote:
> Hu Tao <hutao@cn.fujitsu.com> writes:
> 
> > Hi,
> >
> > On Wed, Apr 10, 2013 at 01:54:04PM +0200, Markus Armbruster wrote:
> >> Hu Tao <hutao@cn.fujitsu.com> writes:
> >> 
> >> > pvpanic device is used to send guest panic event from guest to qemu.
> >> >
> >> > When guest panic happens, pvpanic device driver will write a event
> >> > number to IO port 0x505(which is the IO port occupied by pvpanic device,
> >> > by default). On receiving the event, pvpanic device will pause guest
> >> > cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.
> >> >
> >> > Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> >> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> >> > ---
> >> >  hw/misc/Makefile.objs |   2 +
> >> >  hw/misc/pvpanic.c | 116
> >> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> >> >  2 files changed, 118 insertions(+)
> >> >  create mode 100644 hw/misc/pvpanic.c
> >> >
> >> > diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> >> > index 03699c3..d72ea83 100644
> >> > --- a/hw/misc/Makefile.objs
> >> > +++ b/hw/misc/Makefile.objs
> >> > @@ -38,3 +38,5 @@ obj-$(CONFIG_OMAP) += omap_tap.o
> >> >  obj-$(CONFIG_PXA2XX) += pxa2xx_pcmcia.o
> >> >  obj-$(CONFIG_SLAVIO) += slavio_misc.o
> >> >  obj-$(CONFIG_ZYNQ) += zynq_slcr.o
> >> > +
> >> > +common-obj-y += pvpanic.o
> >> > diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
> >> > new file mode 100644
> >> > index 0000000..5118fd7
> >> > --- /dev/null
> >> > +++ b/hw/misc/pvpanic.c
> >> > @@ -0,0 +1,116 @@
> >> > +/*
> >> > + * QEMU simulated pvpanic device.
> >> > + *
> >> > + * Copyright Fujitsu, Corp. 2013
> >> > + *
> >> > + * Authors:
> >> > + *     Wen Congyang <wency@cn.fujitsu.com>
> >> > + *     Hu Tao <hutao@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 <qapi/qmp/qobject.h>
> >> > +#include <qapi/qmp/qjson.h>
> >> > +#include <monitor/monitor.h>
> >> > +#include <sysemu/sysemu.h>
> >> > +#include <sysemu/kvm.h>
> >> > +
> >> > +/* The bit of supported pv event */
> >> > +#define PVPANIC_F_PANICKED      0
> >> > +
> >> > +/* The pv event value */
> >> > +#define PVPANIC_PANICKED        (1 << PVPANIC_F_PANICKED)
> >> > +
> >> > +#define TYPE_ISA_PVPANIC_DEVICE    "pvpanic"
> >> > +#define ISA_PVPANIC_DEVICE(obj)    \
> >> > +    OBJECT_CHECK(PVPanicState, (obj), TYPE_ISA_PVPANIC_DEVICE)
> >> > +
> >> > +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 handle_event(int event)
> >> > +{
> >> > +    if (event == PVPANIC_PANICKED) {
> >> > +        panicked_mon_event("pause");
> >> > +        vm_stop(RUN_STATE_GUEST_PANICKED);
> >> > +        return;
> >> > +    }
> >> > +}
> >> 
> >> I've asked these questions before, if you answered them, I missed it:
> >
> > Sorry, I must have missed them.
> >
> >> 
> >> 1. Your event value looks like it encodes multiple events as bits.  Only
> >> one bit is defined so far (PVEVENT_F_PANICKED).  But you recognize this
> >
> > It was the intention to support multiple events, but Gleb suggested to do
> > only panic event. So pvpanic device supports only panic event.
> >
> >> bit only when the other bits are all off.  Why?  Won't we regret this if
> >> we ever want to define additional bits?
> >
> > Other bits are reserved now, and must be written as 0. (see patch 5/7)
> > If we define these bits in the further for whatever purposes, we have to
> > update code here.
> 
> Not only that, we have to make sure all combinations of old/new device
> and old/new guest device drivers work.
> 
> >> 2. You silently ignore unrecognized event values.  Shouldn't we log
> >> them?
> >
> > See above.
> 
> PATCH 5/7 describes your your device's guest ABI:
> 
>     pvpanic uses port 0x505 to receive a panic event from the guest. On
>     write, bit 0 is set to indicate guest panic has happened. On read, bit
>     0 is set to indicate guest panic notification is supported. Remaining
>     bits are reserved, and should be written as 0, and ignored on read.
> 
> For what it's worth, real hardware usually doesn't work that way.  It
> usually ignores the bits it doesn't implement, and recognizes the bits
> it implements regardless of what's written to the others.
> 
> Let's explore extending the device: add another event, and encode it as
> bit 1.  Then read returns 3, and the guest may write 0, 1, 2 or 3.
> 
> New device, old guest device driver: driver reads 3.  A scrupulously
> correct driver masks out the bits it doesn't understand (3 & 1), gets 1
> as it expects, and continues to work as before.  A sloppy driver may not
> check the bits; also continues to work as before.  A confused driver may
> choke on the value 3, and fail, most probably in a way that's visible in
> dmesg.
> 
> Old device, new guest device driver: driver reads 1, masks out the bits
> it doesn't understand (1 & 3), gets 1.  Say the driver wants to report
> both events.  A well-behaved driver recognizes that the device doesn't
> understand the new event, and writes 1.  Works.  A sloppy driver may
> write 3.  Your device ignores that write silently.

Thanks for the detailed analysis. Let's make pvpanic work in the way
real hardware does:

>From b4c7755011916c6fa8cfaaf6ecc52e6a8cae7ea5 Mon Sep 17 00:00:00 2001
From: Hu Tao <hutao@cn.fujitsu.com>
Date: Mon, 15 Apr 2013 10:07:07 +0800
Subject: [PATCH] introduce a new qom device to deal with panicked event

pvpanic device is used to send guest panic event from guest to qemu.

When guest panic happens, pvpanic device driver will write a event
number to IO port 0x505(which is the IO port occupied by pvpanic device,
by default). On receiving the event, pvpanic device will pause guest
cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 hw/misc/Makefile.objs |   2 +
 hw/misc/pvpanic.c     | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 125 insertions(+)
 create mode 100644 hw/misc/pvpanic.c

diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 03699c3..d72ea83 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -38,3 +38,5 @@ obj-$(CONFIG_OMAP) += omap_tap.o
 obj-$(CONFIG_PXA2XX) += pxa2xx_pcmcia.o
 obj-$(CONFIG_SLAVIO) += slavio_misc.o
 obj-$(CONFIG_ZYNQ) += zynq_slcr.o
+
+common-obj-y += pvpanic.o
diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
new file mode 100644
index 0000000..ab4a545
--- /dev/null
+++ b/hw/misc/pvpanic.c
@@ -0,0 +1,123 @@
+/*
+ * QEMU simulated pvpanic device.
+ *
+ * Copyright Fujitsu, Corp. 2013
+ *
+ * Authors:
+ *     Wen Congyang <wency@cn.fujitsu.com>
+ *     Hu Tao <hutao@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 <qapi/qmp/qobject.h>
+#include <qapi/qmp/qjson.h>
+#include <monitor/monitor.h>
+#include <sysemu/sysemu.h>
+#include <sysemu/kvm.h>
+
+/* The bit of supported pv event */
+#define PVPANIC_F_PANICKED      0
+
+/* The pv event value */
+#define PVPANIC_PANICKED        (1 << PVPANIC_F_PANICKED)
+
+#define TYPE_ISA_PVPANIC_DEVICE    "pvpanic"
+#define ISA_PVPANIC_DEVICE(obj)    \
+    OBJECT_CHECK(PVPanicState, (obj), TYPE_ISA_PVPANIC_DEVICE)
+
+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 handle_event(int event)
+{
+    static bool logged = false;
+
+    if (event & ~PVPANIC_PANICKED && !logged) {
+        fprintf(stderr, "pvpanic: unknown event %#x.\n", event);
+        logged = true;
+    }
+
+    if (event & PVPANIC_PANICKED) {
+        panicked_mon_event("pause");
+        vm_stop(RUN_STATE_GUEST_PANICKED);
+        return;
+    }
+}
+
+#include "hw/isa/isa.h"
+
+typedef struct PVPanicState {
+    ISADevice parent_obj;
+
+    MemoryRegion io;
+    uint16_t ioport;
+} PVPanicState;
+
+/* return supported events on read */
+static uint64_t pvpanic_ioport_read(void *opaque, hwaddr addr, unsigned size)
+{
+    return PVPANIC_PANICKED;
+}
+
+static void pvpanic_ioport_write(void *opaque, hwaddr addr, uint64_t val,
+                                 unsigned size)
+{
+    handle_event(val);
+}
+
+static const MemoryRegionOps pvpanic_ops = {
+    .read = pvpanic_ioport_read,
+    .write = pvpanic_ioport_write,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+static int pvpanic_isa_initfn(ISADevice *dev)
+{
+    PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
+
+    memory_region_init_io(&s->io, &pvpanic_ops, s, "pvpanic", 1);
+    isa_register_ioport(dev, &s->io, s->ioport);
+
+    return 0;
+}
+
+static Property pvpanic_isa_properties[] = {
+    DEFINE_PROP_UINT16("ioport", PVPanicState, ioport, 0x505),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void pvpanic_isa_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    ISADeviceClass *ic = ISA_DEVICE_CLASS(klass);
+
+    ic->init = pvpanic_isa_initfn;
+    dc->no_user = 1;
+    dc->props = pvpanic_isa_properties;
+}
+
+static TypeInfo pvpanic_isa_info = {
+    .name          = TYPE_ISA_PVPANIC_DEVICE,
+    .parent        = TYPE_ISA_DEVICE,
+    .instance_size = sizeof(PVPanicState),
+    .class_init    = pvpanic_isa_class_init,
+};
+
+static void pvpanic_register_types(void)
+{
+    type_register_static(&pvpanic_isa_info);
+}
+
+type_init(pvpanic_register_types)
-- 
1.8.1.4


> 
> I don't like that.  I'd prefer it to work like real hardware usually
> does: recognize bit 0 even when other bits are set.
> 
> ABI description becomes:
> 
>     pvpanic exposes a single I/O port, by default 0x505.  Each bit of
>     the port corresponds to an event.
> 
>     On read, the bits recognized by the device are set.  Software should
>     ignore bits it doesn't recognize.
> 
>     On write, the bits not recognized by the device are ignored.
>     Software should set only bits both itself and the device recognize.
> 
>     Currently, only bit 0 is recognized.  Setting it indicates a guest
>     panic has happened.

I adopt your words here, with a slight change:

>From 758f69bb37fa0fb8567a480258cc8cc998c4d693 Mon Sep 17 00:00:00 2001
From: Hu Tao <hutao@cn.fujitsu.com>
Date: Mon, 15 Apr 2013 10:07:07 +0800
Subject: [PATCH] pvpanic: add document of pvpanic

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

diff --git a/docs/specs/pvpanic.txt b/docs/specs/pvpanic.txt
new file mode 100644
index 0000000..c7bbacc
--- /dev/null
+++ b/docs/specs/pvpanic.txt
@@ -0,0 +1,39 @@
+PVPANIC DEVICE
+==============
+
+pvpanic device is a simulated ISA device, through which a guest panic
+event is sent to qemu, and a QMP event is generated. This allows
+management apps (e.g. libvirt) to be notified and respond to the event.
+
+The management app has the option of waiting for GUEST_PANICKED events,
+and/or polling for guest-panicked RunState, to learn when the pvpanic
+device has fired a panic event.
+
+ISA Interface
+-------------
+
+pvpanic exposes a single I/O port, by default 0x505. On read, the bits
+recognized by the device are set. Software should ignore bits it doesn't
+recognize. On write, the bits not recognized by the device are ignored.
+Software should set only bits both itself and the device recognize.
+Currently, only bit 0 is recognized, setting it indicates a guest panic
+has happened.
+
+ACPI Interface
+--------------
+
+pvpanic device is defined with ACPI ID "QEMU0001". Custom methods:
+
+RDPT:       To determine whether guest panic notification is supported.
+Arguments:  None
+Return:     Returns a byte, bit 0 set to indicate guest panic
+            notification is supported. Other bits are reserved and
+            should be ignored.
+
+WRPT:       To send a guest panic event
+Arguments:  Arg0 is a byte, with bit 0 set to indicate guest panic has
+            happened. Other bits are reserved and should be cleared.
+Return:     None
+
+The ACPI device will automatically refer to the right port in case it
+is modified.
-- 
1.8.1.4


(If no objection to the changes, I'll sent v19)

> 
> Regardless of whether you make the device ignore unrecognized bits or
> not, please consider logging when a guest sets unrecognized bits.  That
> way we can catch misbehaving guest device drivers much more easily.
> Recommend to log only the first occurence, to prevent denial of service.