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

[Qemu-devel] [PATCH v16 0/6] 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 v15:

  - document improvements
  - use SSDT to patch pvpanic ioport

v15: http://lists.nongnu.org/archive/html/qemu-devel/2013-03/msg03630.html

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: add document of pvpanic
  pc_piix, pc_q35: export fw_cfg
  pvpanic: pass configurable ioport to seabios

 QMP/qmp-events.txt        |  14 +++++
 docs/specs/pvpanic.txt    |  37 +++++++++++++
 hw/Makefile.objs          |   1 +
 hw/fw_cfg.h               |   2 +
 hw/i386/pc_piix.c         |   7 +--
 hw/i386/pc_q35.c          |   9 ++--
 hw/pc.h                   |   6 +++
 hw/pvpanic.c              | 130 ++++++++++++++++++++++++++++++++++++++++++++++
 include/monitor/monitor.h |   1 +
 include/sysemu/sysemu.h   |   1 +
 monitor.c                 |   1 +
 qapi-schema.json          |   5 +-
 qmp.c                     |   3 +-
 vl.c                      |  13 ++++-
 14 files changed, 219 insertions(+), 11 deletions(-)
 create mode 100644 docs/specs/pvpanic.txt
 create mode 100644 hw/pvpanic.c

-- 
1.8.1.4

[Qemu-devel] [PATCH v16 1/6] 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 f629a24..294d339 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 52eacca..271f386 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 v16 2/6] 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 4ec1db9..a46d5ae 100644
--- a/monitor.c
+++ b/monitor.c
@@ -484,6 +484,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 v16 3/6] 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/Makefile.objs |   1 +
 hw/pvpanic.c     | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+)
 create mode 100644 hw/pvpanic.c

diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index d0b2ecb..12ac32a 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -216,4 +216,5 @@ obj-$(CONFIG_KVM) += ivshmem.o
 obj-$(CONFIG_LINUX) += vfio_pci.o
 endif
 
+common-obj-y += pvpanic.o
 endif
diff --git a/hw/pvpanic.c b/hw/pvpanic.c
new file mode 100644
index 0000000..ff0a116
--- /dev/null
+++ b/hw/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.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 v16 4/6] 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 v16 5/6] pc_piix, pc_q35: export fw_cfg

[Hu Tao]

This lets other devices pass informations to seabios.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 hw/i386/pc_piix.c | 7 ++++---
 hw/i386/pc_q35.c  | 9 ++++++---
 hw/pc.h           | 6 ++++++
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 0abc9f1..7a9d9a8 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 };
 
+void *fw_cfg_piix;
+
 /* PC hardware initialisation */
 static void pc_init1(MemoryRegion *system_memory,
                      MemoryRegion *system_io,
@@ -84,7 +86,6 @@ static void pc_init1(MemoryRegion *system_memory,
     MemoryRegion *ram_memory;
     MemoryRegion *pci_memory;
     MemoryRegion *rom_memory;
-    void *fw_cfg = NULL;
 
     pc_cpus_init(cpu_model);
     pc_acpi_init("acpi-dsdt.aml");
@@ -112,7 +113,7 @@ static void pc_init1(MemoryRegion *system_memory,
 
     /* allocate ram and load rom/bios */
     if (!xen_enabled()) {
-        fw_cfg = pc_memory_init(system_memory,
+        fw_cfg_piix = pc_memory_init(system_memory,
                        kernel_filename, kernel_cmdline, initrd_filename,
                        below_4g_mem_size, above_4g_mem_size,
                        rom_memory, &ram_memory);
@@ -210,7 +211,7 @@ static void pc_init1(MemoryRegion *system_memory,
         /* TODO: Populate SPD eeprom data.  */
         smbus = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100,
                               gsi[9], *smi_irq,
-                              kvm_enabled(), fw_cfg);
+                              kvm_enabled(), fw_cfg_piix);
         smbus_eeprom_init(smbus, 8, NULL, 0);
     }
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 4f5f347..ce7798c 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
 
+void *fw_cfg_q35;
+
 /* 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)
@@ -111,9 +113,10 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
 
     /* allocate ram and load rom/bios */
     if (!xen_enabled()) {
-        pc_memory_init(get_system_memory(), kernel_filename, kernel_cmdline,
-                       initrd_filename, below_4g_mem_size, above_4g_mem_size,
-                       rom_memory, &ram_memory);
+        fw_cfg_q35 = pc_memory_init(get_system_memory(), kernel_filename,
+                                    kernel_cmdline, initrd_filename,
+                                    below_4g_mem_size, above_4g_mem_size,
+                                    rom_memory, &ram_memory);
     }
 
     /* irq lines */
diff --git a/hw/pc.h b/hw/pc.h
index 8e1dd4c..d42b76b 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -145,6 +145,12 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
 extern PCIDevice *piix4_dev;
 int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
 
+/* pc_piix.c */
+extern void *fw_cfg_piix;
+
+/* pc_q35.c */
+extern void *fw_cfg_q35;
+
 /* vga.c */
 enum vga_retrace_method {
     VGA_RETRACE_DUMB,
-- 
1.8.1.4

[Qemu-devel] [PATCH v16 6/6] 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/fw_cfg.h  |  2 ++
 hw/pvpanic.c | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/hw/fw_cfg.h b/hw/fw_cfg.h
index 05c8df1..07cc941 100644
--- a/hw/fw_cfg.h
+++ b/hw/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
diff --git a/hw/pvpanic.c b/hw/pvpanic.c
index ff0a116..94c3b25 100644
--- a/hw/pvpanic.c
+++ b/hw/pvpanic.c
@@ -17,6 +17,8 @@
 #include <monitor/monitor.h>
 #include <sysemu/sysemu.h>
 #include <sysemu/kvm.h>
+#include "hw/fw_cfg.h"
+#include "hw/pc.h"
 
 /* The bit of supported pv event */
 #define PVPANIC_F_PANICKED      0
@@ -83,6 +85,18 @@ static int pvpanic_isa_initfn(ISADevice *dev)
     memory_region_init_io(&s->io, &pvpanic_ops, s, "pvpanic", 1);
     isa_register_ioport(dev, &s->io, s->ioport);
 
+    if (fw_cfg_piix) {
+        fw_cfg_add_file(fw_cfg_piix, "etc/pvpanic-port",
+                        g_memdup(&s->ioport, sizeof(s->ioport)),
+                        sizeof(s->ioport));
+    }
+
+    if (fw_cfg_q35) {
+        fw_cfg_add_file(fw_cfg_q35, "etc/pvpanic-port",
+                        g_memdup(&s->ioport, sizeof(s->ioport)),
+                        sizeof(s->ioport));
+    }
+
     return 0;
 }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH v16] Add pvpanic device driver

[Hu Tao]

pvpanic device is used to notify host(qemu) when guest panic happens.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 src/acpi.c        |  3 +++
 src/ssdt-misc.dsl | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/src/acpi.c b/src/acpi.c
index bc4d8ea..fe504f0 100644
--- a/src/acpi.c
+++ b/src/acpi.c
@@ -534,6 +534,9 @@ build_ssdt(void)
         ssdt_ptr[acpi_pci64_valid[0]] = 0;
     }
 
+    int pvpanic_port = romfile_loadint("etc/pvpanic-port", 0x0);
+    *(u16 *)(ssdt_ptr + *ssdt_isa_pest) = pvpanic_port;
+
     ssdt_ptr += sizeof(ssdp_misc_aml);
 
     // build Scope(_SB_) header
diff --git a/src/ssdt-misc.dsl b/src/ssdt-misc.dsl
index 679422b..acc850e 100644
--- a/src/ssdt-misc.dsl
+++ b/src/ssdt-misc.dsl
@@ -55,4 +55,50 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
             Zero   /* reserved */
         })
     }
+
+    External(\_SB.PCI0, DeviceObj)
+    External(\_SB.PCI0.ISA, DeviceObj)
+
+    Scope(\_SB.PCI0.ISA) {
+        Device(PEVT) {
+            Name(_HID, "QEMU0001")
+            /* PEST will be patched to be Zero if no such device */
+            ACPI_EXTRACT_NAME_WORD_CONST ssdt_isa_pest
+            Name(PEST, 0xFFFF)
+            OperationRegion(PEOR, SystemIO, PEST, 0x01)
+            Field(PEOR, ByteAcc, NoLock, Preserve) {
+                PEPT,   8,
+            }
+
+            Method(_STA, 0, NotSerialized) {
+                Store(PEST, Local0)
+                If (LEqual(Local0, Zero)) {
+                    Return (0x00)
+                } Else {
+                    Return (0x0F)
+                }
+            }
+
+            Method(RDPT, 0, NotSerialized) {
+                Store(PEPT, Local0)
+                Return (Local0)
+            }
+
+            Method(WRPT, 1, NotSerialized) {
+                Store(Arg0, PEPT)
+            }
+
+            Name(_CRS, ResourceTemplate() {
+                IO(Decode16, 0x00, 0x00, 0x01, 0x01, IO)
+            })
+
+            CreateWordField(_CRS, IO._MIN, IOMN)
+            CreateWordField(_CRS, IO._MAX, IOMX)
+
+            Method(_INI, 0, NotSerialized) {
+                Store(PEST, IOMN)
+                Store(PEST, IOMX)
+            }
+        }
+    }
 }
-- 
1.8.1.4

[PATCH v16] pvpanic: pvpanic device driver

[Hu Tao]

pvpanic device is a qemu simulated device through which guest panic
event is sent to host.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---

ref: http://lists.nongnu.org/archive/html/qemu-devel/2013-03/msg05297.html

 drivers/platform/x86/Kconfig   |   7 +++
 drivers/platform/x86/Makefile  |   2 +
 drivers/platform/x86/pvpanic.c | 115 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 124 insertions(+)
 create mode 100644 drivers/platform/x86/pvpanic.c

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 3338437..527ed04 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -781,4 +781,11 @@ config APPLE_GMUX
 	  graphics as well as the backlight. Currently only backlight
 	  control is supported by the driver.
 
+config PVPANIC
+	tristate "pvpanic device support"
+	depends on ACPI
+	---help---
+	  This driver provides support for pvpanic device, which is a qemu
+	  simulated device through which guest panic event is sent to host.
+
 endif # X86_PLATFORM_DEVICES
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index ace2b38..ef0ec74 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -51,3 +51,5 @@ obj-$(CONFIG_INTEL_OAKTRAIL)	+= intel_oaktrail.o
 obj-$(CONFIG_SAMSUNG_Q10)	+= samsung-q10.o
 obj-$(CONFIG_APPLE_GMUX)	+= apple-gmux.o
 obj-$(CONFIG_CHROMEOS_LAPTOP)	+= chromeos_laptop.o
+
+obj-$(CONFIG_PVPANIC)           += pvpanic.o
diff --git a/drivers/platform/x86/pvpanic.c b/drivers/platform/x86/pvpanic.c
new file mode 100644
index 0000000..e6ea595
--- /dev/null
+++ b/drivers/platform/x86/pvpanic.c
@@ -0,0 +1,115 @@
+/*
+ *  pvpanic.c - pvpanic Device Support
+ *
+ *  Copyright (C) 2013 Fujitsu.
+ *
+ *  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, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <acpi/acpi_bus.h>
+#include <acpi/acpi_drivers.h>
+
+MODULE_AUTHOR("Hu Tao <hutao@cn.fujitsu.com>");
+MODULE_DESCRIPTION("pvpanic device driver");
+MODULE_LICENSE("GPL");
+
+static int pvpanic_add(struct acpi_device *device);
+static int pvpanic_remove(struct acpi_device *device);
+
+static const struct acpi_device_id pvpanic_device_ids[] = {
+	{ "QEMU0001", 0},
+	{ "", 0},
+};
+MODULE_DEVICE_TABLE(acpi, pvpanic_device_ids);
+
+#define PVPANIC_PANICKED	(1 << 0)
+
+static acpi_handle handle;
+
+static struct acpi_driver pvpanic_driver = {
+	.name =		"pvpanic",
+	.class =	"QEMU",
+	.ids =		pvpanic_device_ids,
+	.ops =		{
+				.add =		pvpanic_add,
+				.remove =	pvpanic_remove,
+			},
+	.owner =	THIS_MODULE,
+};
+
+static void
+pvpanic_send_event(unsigned int event)
+{
+	union acpi_object arg;
+	struct acpi_object_list arg_list;
+
+	if (!handle)
+		return;
+
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = event;
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+
+	acpi_evaluate_object(handle, "WRPT", &arg_list, NULL);
+}
+
+static int
+pvpanic_panic_notify(struct notifier_block *nb, unsigned long code,
+		     void *unused)
+{
+	pvpanic_send_event(PVPANIC_PANICKED);
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block pvpanic_panic_nb = {
+	.notifier_call = pvpanic_panic_notify,
+};
+
+static int pvpanic_add(struct acpi_device *device)
+{
+	acpi_status status;
+	u64 ret;
+
+	status = acpi_evaluate_integer(device->handle, "_STA", NULL,
+				       &ret);
+
+	if (ACPI_FAILURE(status) || !ret)
+		return -ENODEV;
+
+	handle = device->handle;
+	atomic_notifier_chain_register(&panic_notifier_list,
+				       &pvpanic_panic_nb);
+
+	return 0;
+}
+
+static int pvpanic_remove(struct acpi_device *device)
+{
+
+	atomic_notifier_chain_unregister(&panic_notifier_list,
+					 &pvpanic_panic_nb);
+	handle = NULL;
+	return 0;
+}
+
+module_acpi_driver(pvpanic_driver);
-- 
1.8.1.4

[Qemu-devel] [PATCH v16] pvpanic: pvpanic device driver

[Hu Tao]

pvpanic device is a qemu simulated device through which guest panic
event is sent to host.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---

ref: http://lists.nongnu.org/archive/html/qemu-devel/2013-03/msg05297.html

 drivers/platform/x86/Kconfig   |   7 +++
 drivers/platform/x86/Makefile  |   2 +
 drivers/platform/x86/pvpanic.c | 115 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 124 insertions(+)
 create mode 100644 drivers/platform/x86/pvpanic.c

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 3338437..527ed04 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -781,4 +781,11 @@ config APPLE_GMUX
 	  graphics as well as the backlight. Currently only backlight
 	  control is supported by the driver.
 
+config PVPANIC
+	tristate "pvpanic device support"
+	depends on ACPI
+	---help---
+	  This driver provides support for pvpanic device, which is a qemu
+	  simulated device through which guest panic event is sent to host.
+
 endif # X86_PLATFORM_DEVICES
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index ace2b38..ef0ec74 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -51,3 +51,5 @@ obj-$(CONFIG_INTEL_OAKTRAIL)	+= intel_oaktrail.o
 obj-$(CONFIG_SAMSUNG_Q10)	+= samsung-q10.o
 obj-$(CONFIG_APPLE_GMUX)	+= apple-gmux.o
 obj-$(CONFIG_CHROMEOS_LAPTOP)	+= chromeos_laptop.o
+
+obj-$(CONFIG_PVPANIC)           += pvpanic.o
diff --git a/drivers/platform/x86/pvpanic.c b/drivers/platform/x86/pvpanic.c
new file mode 100644
index 0000000..e6ea595
--- /dev/null
+++ b/drivers/platform/x86/pvpanic.c
@@ -0,0 +1,115 @@
+/*
+ *  pvpanic.c - pvpanic Device Support
+ *
+ *  Copyright (C) 2013 Fujitsu.
+ *
+ *  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, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <acpi/acpi_bus.h>
+#include <acpi/acpi_drivers.h>
+
+MODULE_AUTHOR("Hu Tao <hutao@cn.fujitsu.com>");
+MODULE_DESCRIPTION("pvpanic device driver");
+MODULE_LICENSE("GPL");
+
+static int pvpanic_add(struct acpi_device *device);
+static int pvpanic_remove(struct acpi_device *device);
+
+static const struct acpi_device_id pvpanic_device_ids[] = {
+	{ "QEMU0001", 0},
+	{ "", 0},
+};
+MODULE_DEVICE_TABLE(acpi, pvpanic_device_ids);
+
+#define PVPANIC_PANICKED	(1 << 0)
+
+static acpi_handle handle;
+
+static struct acpi_driver pvpanic_driver = {
+	.name =		"pvpanic",
+	.class =	"QEMU",
+	.ids =		pvpanic_device_ids,
+	.ops =		{
+				.add =		pvpanic_add,
+				.remove =	pvpanic_remove,
+			},
+	.owner =	THIS_MODULE,
+};
+
+static void
+pvpanic_send_event(unsigned int event)
+{
+	union acpi_object arg;
+	struct acpi_object_list arg_list;
+
+	if (!handle)
+		return;
+
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = event;
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+
+	acpi_evaluate_object(handle, "WRPT", &arg_list, NULL);
+}
+
+static int
+pvpanic_panic_notify(struct notifier_block *nb, unsigned long code,
+		     void *unused)
+{
+	pvpanic_send_event(PVPANIC_PANICKED);
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block pvpanic_panic_nb = {
+	.notifier_call = pvpanic_panic_notify,
+};
+
+static int pvpanic_add(struct acpi_device *device)
+{
+	acpi_status status;
+	u64 ret;
+
+	status = acpi_evaluate_integer(device->handle, "_STA", NULL,
+				       &ret);
+
+	if (ACPI_FAILURE(status) || !ret)
+		return -ENODEV;
+
+	handle = device->handle;
+	atomic_notifier_chain_register(&panic_notifier_list,
+				       &pvpanic_panic_nb);
+
+	return 0;
+}
+
+static int pvpanic_remove(struct acpi_device *device)
+{
+
+	atomic_notifier_chain_unregister(&panic_notifier_list,
+					 &pvpanic_panic_nb);
+	handle = NULL;
+	return 0;
+}
+
+module_acpi_driver(pvpanic_driver);
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v16 5/6] pc_piix, pc_q35: export fw_cfg

[Paolo Bonzini]

Il 29/03/2013 09:16, Hu Tao ha scritto:
> diff --git a/hw/pc.h b/hw/pc.h
> index 8e1dd4c..d42b76b 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -145,6 +145,12 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
>  extern PCIDevice *piix4_dev;
>  int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
>  
> +/* pc_piix.c */
> +extern void *fw_cfg_piix;
> +
> +/* pc_q35.c */
> +extern void *fw_cfg_q35;
> +
>  /* vga.c */
>  enum vga_retrace_method {
>      VGA_RETRACE_DUMB,
> 

Hu, this is not what I suggested in the review...

Please do not ignore comments without answer, otherwise we'll just run
in circles.

Paolo

Re: [Qemu-devel] [PATCH v16] Add pvpanic device driver

[Paolo Bonzini]

Il 29/03/2013 09:18, Hu Tao ha scritto:
> pvpanic device is used to notify host(qemu) when guest panic happens.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  src/acpi.c        |  3 +++
>  src/ssdt-misc.dsl | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+)
> 
> diff --git a/src/acpi.c b/src/acpi.c
> index bc4d8ea..fe504f0 100644
> --- a/src/acpi.c
> +++ b/src/acpi.c
> @@ -534,6 +534,9 @@ build_ssdt(void)
>          ssdt_ptr[acpi_pci64_valid[0]] = 0;
>      }
>  
> +    int pvpanic_port = romfile_loadint("etc/pvpanic-port", 0x0);
> +    *(u16 *)(ssdt_ptr + *ssdt_isa_pest) = pvpanic_port;
> +
>      ssdt_ptr += sizeof(ssdp_misc_aml);
>  
>      // build Scope(_SB_) header
> diff --git a/src/ssdt-misc.dsl b/src/ssdt-misc.dsl
> index 679422b..acc850e 100644
> --- a/src/ssdt-misc.dsl
> +++ b/src/ssdt-misc.dsl
> @@ -55,4 +55,50 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
>              Zero   /* reserved */
>          })
>      }
> +
> +    External(\_SB.PCI0, DeviceObj)
> +    External(\_SB.PCI0.ISA, DeviceObj)
> +
> +    Scope(\_SB.PCI0.ISA) {
> +        Device(PEVT) {
> +            Name(_HID, "QEMU0001")
> +            /* PEST will be patched to be Zero if no such device */
> +            ACPI_EXTRACT_NAME_WORD_CONST ssdt_isa_pest
> +            Name(PEST, 0xFFFF)
> +            OperationRegion(PEOR, SystemIO, PEST, 0x01)
> +            Field(PEOR, ByteAcc, NoLock, Preserve) {
> +                PEPT,   8,
> +            }
> +
> +            Method(_STA, 0, NotSerialized) {
> +                Store(PEST, Local0)
> +                If (LEqual(Local0, Zero)) {
> +                    Return (0x00)
> +                } Else {
> +                    Return (0x0F)
> +                }
> +            }
> +
> +            Method(RDPT, 0, NotSerialized) {
> +                Store(PEPT, Local0)
> +                Return (Local0)
> +            }
> +
> +            Method(WRPT, 1, NotSerialized) {
> +                Store(Arg0, PEPT)
> +            }
> +
> +            Name(_CRS, ResourceTemplate() {
> +                IO(Decode16, 0x00, 0x00, 0x01, 0x01, IO)
> +            })
> +
> +            CreateWordField(_CRS, IO._MIN, IOMN)
> +            CreateWordField(_CRS, IO._MAX, IOMX)
> +
> +            Method(_INI, 0, NotSerialized) {
> +                Store(PEST, IOMN)
> +                Store(PEST, IOMX)
> +            }
> +        }
> +    }
>  }
> 

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

Re: [PATCH v16] pvpanic: pvpanic device driver

[Paolo Bonzini]

Il 29/03/2013 09:34, Hu Tao ha scritto:
> pvpanic device is a qemu simulated device through which guest panic
> event is sent to host.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
> 
> ref: http://lists.nongnu.org/archive/html/qemu-devel/2013-03/msg05297.html
> 
>  drivers/platform/x86/Kconfig   |   7 +++
>  drivers/platform/x86/Makefile  |   2 +
>  drivers/platform/x86/pvpanic.c | 115 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 124 insertions(+)
>  create mode 100644 drivers/platform/x86/pvpanic.c
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 3338437..527ed04 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -781,4 +781,11 @@ config APPLE_GMUX
>  	  graphics as well as the backlight. Currently only backlight
>  	  control is supported by the driver.
>  
> +config PVPANIC
> +	tristate "pvpanic device support"
> +	depends on ACPI
> +	---help---
> +	  This driver provides support for pvpanic device, which is a qemu
> +	  simulated device through which guest panic event is sent to host.
> +
>  endif # X86_PLATFORM_DEVICES
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index ace2b38..ef0ec74 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -51,3 +51,5 @@ obj-$(CONFIG_INTEL_OAKTRAIL)	+= intel_oaktrail.o
>  obj-$(CONFIG_SAMSUNG_Q10)	+= samsung-q10.o
>  obj-$(CONFIG_APPLE_GMUX)	+= apple-gmux.o
>  obj-$(CONFIG_CHROMEOS_LAPTOP)	+= chromeos_laptop.o
> +
> +obj-$(CONFIG_PVPANIC)           += pvpanic.o
> diff --git a/drivers/platform/x86/pvpanic.c b/drivers/platform/x86/pvpanic.c
> new file mode 100644
> index 0000000..e6ea595
> --- /dev/null
> +++ b/drivers/platform/x86/pvpanic.c
> @@ -0,0 +1,115 @@
> +/*
> + *  pvpanic.c - pvpanic Device Support
> + *
> + *  Copyright (C) 2013 Fujitsu.
> + *
> + *  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, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/types.h>
> +#include <acpi/acpi_bus.h>
> +#include <acpi/acpi_drivers.h>
> +
> +MODULE_AUTHOR("Hu Tao <hutao@cn.fujitsu.com>");
> +MODULE_DESCRIPTION("pvpanic device driver");
> +MODULE_LICENSE("GPL");
> +
> +static int pvpanic_add(struct acpi_device *device);
> +static int pvpanic_remove(struct acpi_device *device);
> +
> +static const struct acpi_device_id pvpanic_device_ids[] = {
> +	{ "QEMU0001", 0},
> +	{ "", 0},
> +};
> +MODULE_DEVICE_TABLE(acpi, pvpanic_device_ids);
> +
> +#define PVPANIC_PANICKED	(1 << 0)
> +
> +static acpi_handle handle;
> +
> +static struct acpi_driver pvpanic_driver = {
> +	.name =		"pvpanic",
> +	.class =	"QEMU",
> +	.ids =		pvpanic_device_ids,
> +	.ops =		{
> +				.add =		pvpanic_add,
> +				.remove =	pvpanic_remove,
> +			},
> +	.owner =	THIS_MODULE,
> +};
> +
> +static void
> +pvpanic_send_event(unsigned int event)
> +{
> +	union acpi_object arg;
> +	struct acpi_object_list arg_list;
> +
> +	if (!handle)
> +		return;
> +
> +	arg.type = ACPI_TYPE_INTEGER;
> +	arg.integer.value = event;
> +
> +	arg_list.count = 1;
> +	arg_list.pointer = &arg;
> +
> +	acpi_evaluate_object(handle, "WRPT", &arg_list, NULL);
> +}
> +
> +static int
> +pvpanic_panic_notify(struct notifier_block *nb, unsigned long code,
> +		     void *unused)
> +{
> +	pvpanic_send_event(PVPANIC_PANICKED);
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block pvpanic_panic_nb = {
> +	.notifier_call = pvpanic_panic_notify,
> +};
> +
> +static int pvpanic_add(struct acpi_device *device)
> +{
> +	acpi_status status;
> +	u64 ret;
> +
> +	status = acpi_evaluate_integer(device->handle, "_STA", NULL,
> +				       &ret);
> +
> +	if (ACPI_FAILURE(status) || !ret)
> +		return -ENODEV;

Please return if (ret & 0x0B) != 0x0B.  This checks:

Bit 0 - Set if the device is present.
Bit 1 - Set if the device is enabled and decoding its resources.
Bit 3 - Set if the device is functioning properly.

Otherwise looks good.

> +
> +	handle = device->handle;
> +	atomic_notifier_chain_register(&panic_notifier_list,
> +				       &pvpanic_panic_nb);
> +
> +	return 0;
> +}
> +
> +static int pvpanic_remove(struct acpi_device *device)
> +{
> +
> +	atomic_notifier_chain_unregister(&panic_notifier_list,
> +					 &pvpanic_panic_nb);
> +	handle = NULL;
> +	return 0;
> +}
> +
> +module_acpi_driver(pvpanic_driver);
> 

Re: [PATCH v16] pvpanic: pvpanic device driver

[Paolo Bonzini]

Il 29/03/2013 09:34, Hu Tao ha scritto:
> pvpanic device is a qemu simulated device through which guest panic
> event is sent to host.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
> 
> ref: http://lists.nongnu.org/archive/html/qemu-devel/2013-03/msg05297.html
> 
>  drivers/platform/x86/Kconfig   |   7 +++
>  drivers/platform/x86/Makefile  |   2 +
>  drivers/platform/x86/pvpanic.c | 115 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 124 insertions(+)
>  create mode 100644 drivers/platform/x86/pvpanic.c
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 3338437..527ed04 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -781,4 +781,11 @@ config APPLE_GMUX
>  	  graphics as well as the backlight. Currently only backlight
>  	  control is supported by the driver.
>  
> +config PVPANIC
> +	tristate "pvpanic device support"
> +	depends on ACPI
> +	---help---
> +	  This driver provides support for pvpanic device, which is a qemu
> +	  simulated device through which guest panic event is sent to host.
> +
>  endif # X86_PLATFORM_DEVICES
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index ace2b38..ef0ec74 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -51,3 +51,5 @@ obj-$(CONFIG_INTEL_OAKTRAIL)	+= intel_oaktrail.o
>  obj-$(CONFIG_SAMSUNG_Q10)	+= samsung-q10.o
>  obj-$(CONFIG_APPLE_GMUX)	+= apple-gmux.o
>  obj-$(CONFIG_CHROMEOS_LAPTOP)	+= chromeos_laptop.o
> +
> +obj-$(CONFIG_PVPANIC)           += pvpanic.o
> diff --git a/drivers/platform/x86/pvpanic.c b/drivers/platform/x86/pvpanic.c
> new file mode 100644
> index 0000000..e6ea595
> --- /dev/null
> +++ b/drivers/platform/x86/pvpanic.c
> @@ -0,0 +1,115 @@
> +/*
> + *  pvpanic.c - pvpanic Device Support
> + *
> + *  Copyright (C) 2013 Fujitsu.
> + *
> + *  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, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/types.h>
> +#include <acpi/acpi_bus.h>
> +#include <acpi/acpi_drivers.h>
> +
> +MODULE_AUTHOR("Hu Tao <hutao@cn.fujitsu.com>");
> +MODULE_DESCRIPTION("pvpanic device driver");
> +MODULE_LICENSE("GPL");
> +
> +static int pvpanic_add(struct acpi_device *device);
> +static int pvpanic_remove(struct acpi_device *device);
> +
> +static const struct acpi_device_id pvpanic_device_ids[] = {
> +	{ "QEMU0001", 0},
> +	{ "", 0},
> +};
> +MODULE_DEVICE_TABLE(acpi, pvpanic_device_ids);
> +
> +#define PVPANIC_PANICKED	(1 << 0)
> +
> +static acpi_handle handle;
> +
> +static struct acpi_driver pvpanic_driver = {
> +	.name =		"pvpanic",
> +	.class =	"QEMU",
> +	.ids =		pvpanic_device_ids,
> +	.ops =		{
> +				.add =		pvpanic_add,
> +				.remove =	pvpanic_remove,
> +			},
> +	.owner =	THIS_MODULE,
> +};
> +
> +static void
> +pvpanic_send_event(unsigned int event)
> +{
> +	union acpi_object arg;
> +	struct acpi_object_list arg_list;
> +
> +	if (!handle)
> +		return;
> +
> +	arg.type = ACPI_TYPE_INTEGER;
> +	arg.integer.value = event;
> +
> +	arg_list.count = 1;
> +	arg_list.pointer = &arg;
> +
> +	acpi_evaluate_object(handle, "WRPT", &arg_list, NULL);
> +}
> +
> +static int
> +pvpanic_panic_notify(struct notifier_block *nb, unsigned long code,
> +		     void *unused)
> +{
> +	pvpanic_send_event(PVPANIC_PANICKED);
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block pvpanic_panic_nb = {
> +	.notifier_call = pvpanic_panic_notify,
> +};
> +
> +static int pvpanic_add(struct acpi_device *device)
> +{
> +	acpi_status status;
> +	u64 ret;
> +
> +	status = acpi_evaluate_integer(device->handle, "_STA", NULL,
> +				       &ret);
> +
> +	if (ACPI_FAILURE(status) || !ret)
> +		return -ENODEV;

Please return if (ret & 0x0B) != 0x0B.  This checks:

Bit 0 - Set if the device is present.
Bit 1 - Set if the device is enabled and decoding its resources.
Bit 3 - Set if the device is functioning properly.

Otherwise looks good.

> +
> +	handle = device->handle;
> +	atomic_notifier_chain_register(&panic_notifier_list,
> +				       &pvpanic_panic_nb);
> +
> +	return 0;
> +}
> +
> +static int pvpanic_remove(struct acpi_device *device)
> +{
> +
> +	atomic_notifier_chain_unregister(&panic_notifier_list,
> +					 &pvpanic_panic_nb);
> +	handle = NULL;
> +	return 0;
> +}
> +
> +module_acpi_driver(pvpanic_driver);
> 

Re: [Qemu-devel] [PATCH v16] pvpanic: pvpanic device driver

[Paolo Bonzini]

Il 29/03/2013 09:34, Hu Tao ha scritto:
> pvpanic device is a qemu simulated device through which guest panic
> event is sent to host.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
> 
> ref: http://lists.nongnu.org/archive/html/qemu-devel/2013-03/msg05297.html
> 
>  drivers/platform/x86/Kconfig   |   7 +++
>  drivers/platform/x86/Makefile  |   2 +
>  drivers/platform/x86/pvpanic.c | 115 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 124 insertions(+)
>  create mode 100644 drivers/platform/x86/pvpanic.c
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 3338437..527ed04 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -781,4 +781,11 @@ config APPLE_GMUX
>  	  graphics as well as the backlight. Currently only backlight
>  	  control is supported by the driver.
>  
> +config PVPANIC
> +	tristate "pvpanic device support"
> +	depends on ACPI
> +	---help---
> +	  This driver provides support for pvpanic device, which is a qemu
> +	  simulated device through which guest panic event is sent to host.
> +
>  endif # X86_PLATFORM_DEVICES
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index ace2b38..ef0ec74 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -51,3 +51,5 @@ obj-$(CONFIG_INTEL_OAKTRAIL)	+= intel_oaktrail.o
>  obj-$(CONFIG_SAMSUNG_Q10)	+= samsung-q10.o
>  obj-$(CONFIG_APPLE_GMUX)	+= apple-gmux.o
>  obj-$(CONFIG_CHROMEOS_LAPTOP)	+= chromeos_laptop.o
> +
> +obj-$(CONFIG_PVPANIC)           += pvpanic.o
> diff --git a/drivers/platform/x86/pvpanic.c b/drivers/platform/x86/pvpanic.c
> new file mode 100644
> index 0000000..e6ea595
> --- /dev/null
> +++ b/drivers/platform/x86/pvpanic.c
> @@ -0,0 +1,115 @@
> +/*
> + *  pvpanic.c - pvpanic Device Support
> + *
> + *  Copyright (C) 2013 Fujitsu.
> + *
> + *  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, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/types.h>
> +#include <acpi/acpi_bus.h>
> +#include <acpi/acpi_drivers.h>
> +
> +MODULE_AUTHOR("Hu Tao <hutao@cn.fujitsu.com>");
> +MODULE_DESCRIPTION("pvpanic device driver");
> +MODULE_LICENSE("GPL");
> +
> +static int pvpanic_add(struct acpi_device *device);
> +static int pvpanic_remove(struct acpi_device *device);
> +
> +static const struct acpi_device_id pvpanic_device_ids[] = {
> +	{ "QEMU0001", 0},
> +	{ "", 0},
> +};
> +MODULE_DEVICE_TABLE(acpi, pvpanic_device_ids);
> +
> +#define PVPANIC_PANICKED	(1 << 0)
> +
> +static acpi_handle handle;
> +
> +static struct acpi_driver pvpanic_driver = {
> +	.name =		"pvpanic",
> +	.class =	"QEMU",
> +	.ids =		pvpanic_device_ids,
> +	.ops =		{
> +				.add =		pvpanic_add,
> +				.remove =	pvpanic_remove,
> +			},
> +	.owner =	THIS_MODULE,
> +};
> +
> +static void
> +pvpanic_send_event(unsigned int event)
> +{
> +	union acpi_object arg;
> +	struct acpi_object_list arg_list;
> +
> +	if (!handle)
> +		return;
> +
> +	arg.type = ACPI_TYPE_INTEGER;
> +	arg.integer.value = event;
> +
> +	arg_list.count = 1;
> +	arg_list.pointer = &arg;
> +
> +	acpi_evaluate_object(handle, "WRPT", &arg_list, NULL);
> +}
> +
> +static int
> +pvpanic_panic_notify(struct notifier_block *nb, unsigned long code,
> +		     void *unused)
> +{
> +	pvpanic_send_event(PVPANIC_PANICKED);
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block pvpanic_panic_nb = {
> +	.notifier_call = pvpanic_panic_notify,
> +};
> +
> +static int pvpanic_add(struct acpi_device *device)
> +{
> +	acpi_status status;
> +	u64 ret;
> +
> +	status = acpi_evaluate_integer(device->handle, "_STA", NULL,
> +				       &ret);
> +
> +	if (ACPI_FAILURE(status) || !ret)
> +		return -ENODEV;

Please return if (ret & 0x0B) != 0x0B.  This checks:

Bit 0 - Set if the device is present.
Bit 1 - Set if the device is enabled and decoding its resources.
Bit 3 - Set if the device is functioning properly.

Otherwise looks good.

> +
> +	handle = device->handle;
> +	atomic_notifier_chain_register(&panic_notifier_list,
> +				       &pvpanic_panic_nb);
> +
> +	return 0;
> +}
> +
> +static int pvpanic_remove(struct acpi_device *device)
> +{
> +
> +	atomic_notifier_chain_unregister(&panic_notifier_list,
> +					 &pvpanic_panic_nb);
> +	handle = NULL;
> +	return 0;
> +}
> +
> +module_acpi_driver(pvpanic_driver);
> 

Re: [Qemu-devel] [SeaBIOS] [PATCH v16] Add pvpanic device driver

[Kevin O'Connor]

On Fri, Mar 29, 2013 at 04:18:44PM +0800, Hu Tao wrote:
> pvpanic device is used to notify host(qemu) when guest panic happens.

Thanks.  However, we're planning a move of ACPI tables from SeaBIOS to
QEMU.  I think this should wait until after the move.

-Kevin

Re: [Qemu-devel] [SeaBIOS] [PATCH v16] Add pvpanic device driver

[Paolo Bonzini]

Il 29/03/2013 14:33, Kevin O'Connor ha scritto:
> On Fri, Mar 29, 2013 at 04:18:44PM +0800, Hu Tao wrote:
>> pvpanic device is used to notify host(qemu) when guest panic happens.
> 
> Thanks.  However, we're planning a move of ACPI tables from SeaBIOS to
> QEMU.  I think this should wait until after the move.

The device should be in QEMU 1.5, and the SSDT probably will still be in
SeaBIOS by then (and might even be the last to move, since it's quite
complex and dynamic).  I don't think it is fair to block this patch on
those grounds...

Paolo

Re: [Qemu-devel] [SeaBIOS] [PATCH v16] Add pvpanic device driver

[Kevin O'Connor]

On Fri, Mar 29, 2013 at 02:49:12PM +0100, Paolo Bonzini wrote:
> Il 29/03/2013 14:33, Kevin O'Connor ha scritto:
> > On Fri, Mar 29, 2013 at 04:18:44PM +0800, Hu Tao wrote:
> >> pvpanic device is used to notify host(qemu) when guest panic happens.
> > 
> > Thanks.  However, we're planning a move of ACPI tables from SeaBIOS to
> > QEMU.  I think this should wait until after the move.
> 
> The device should be in QEMU 1.5, and the SSDT probably will still be in
> SeaBIOS by then (and might even be the last to move, since it's quite
> complex and dynamic).  I don't think it is fair to block this patch on
> those grounds...

What is the user visible impact of not having a panic device?

My main concern is that the patch creates a new fw_cfg channel between
qemu and seabios thats sole purpose is to alter the OS visible ACPI
tables.  These types of QEMU->SeaBIOS interfaces are fragile and are
(in sum) quite complex.

-Kevin

Re: [Qemu-devel] [SeaBIOS] [PATCH v16] Add pvpanic device driver

[Paolo Bonzini]

Il 30/03/2013 14:20, Kevin O'Connor ha scritto:
> On Fri, Mar 29, 2013 at 02:49:12PM +0100, Paolo Bonzini wrote:
>> Il 29/03/2013 14:33, Kevin O'Connor ha scritto:
>>> On Fri, Mar 29, 2013 at 04:18:44PM +0800, Hu Tao wrote:
>>>> pvpanic device is used to notify host(qemu) when guest panic happens.
>>>
>>> Thanks.  However, we're planning a move of ACPI tables from SeaBIOS to
>>> QEMU.  I think this should wait until after the move.
>>
>> The device should be in QEMU 1.5, and the SSDT probably will still be in
>> SeaBIOS by then (and might even be the last to move, since it's quite
>> complex and dynamic).  I don't think it is fair to block this patch on
>> those grounds...
> 
> What is the user visible impact of not having a panic device?

Management cannot restart or dump the guest after a panic, and cannot
log that a panic happened (as opposed to a random watchdog event).

> My main concern is that the patch creates a new fw_cfg channel between
> qemu and seabios thats sole purpose is to alter the OS visible ACPI
> tables.  These types of QEMU->SeaBIOS interfaces are fragile and are
> (in sum) quite complex.

Yes, this is true.

Paolo

Re: [Qemu-devel] [SeaBIOS] [PATCH v16] Add pvpanic device driver

[Gleb Natapov]

On Sat, Mar 30, 2013 at 09:20:09AM -0400, Kevin O'Connor wrote:
> On Fri, Mar 29, 2013 at 02:49:12PM +0100, Paolo Bonzini wrote:
> > Il 29/03/2013 14:33, Kevin O'Connor ha scritto:
> > > On Fri, Mar 29, 2013 at 04:18:44PM +0800, Hu Tao wrote:
> > >> pvpanic device is used to notify host(qemu) when guest panic happens.
> > > 
> > > Thanks.  However, we're planning a move of ACPI tables from SeaBIOS to
> > > QEMU.  I think this should wait until after the move.
> > 
> > The device should be in QEMU 1.5, and the SSDT probably will still be in
> > SeaBIOS by then (and might even be the last to move, since it's quite
> > complex and dynamic).  I don't think it is fair to block this patch on
> > those grounds...
> 
> What is the user visible impact of not having a panic device?
> 
> My main concern is that the patch creates a new fw_cfg channel between
> qemu and seabios thats sole purpose is to alter the OS visible ACPI
> tables.  These types of QEMU->SeaBIOS interfaces are fragile and are
> (in sum) quite complex.
> 
The patch uses existing channel between qemu and seabios, one
romfile_loadint() is all it takes. We already have number of interfaces
to change OS visible ACPI tables, that's why we want to move ACPI table
creation to QEMU in the first place. It is unfortunate to start blocking
features now before we have an alternative. When ACPI table creation
will move into QEMU the code in this patch will be dropped along with
all the other code that serves similar purpose.

--
			Gleb.

Re: [Qemu-devel] [PATCH v16 5/6] pc_piix, pc_q35: export fw_cfg

[Hu Tao]

On Fri, Mar 29, 2013 at 10:28:07AM +0100, Paolo Bonzini wrote:
> Il 29/03/2013 09:16, Hu Tao ha scritto:
> > diff --git a/hw/pc.h b/hw/pc.h
> > index 8e1dd4c..d42b76b 100644
> > --- a/hw/pc.h
> > +++ b/hw/pc.h
> > @@ -145,6 +145,12 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
> >  extern PCIDevice *piix4_dev;
> >  int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
> >  
> > +/* pc_piix.c */
> > +extern void *fw_cfg_piix;
> > +
> > +/* pc_q35.c */
> > +extern void *fw_cfg_q35;
> > +
> >  /* vga.c */
> >  enum vga_retrace_method {
> >      VGA_RETRACE_DUMB,
> > 
> 
> Hu, this is not what I suggested in the review...
> 
> Please do not ignore comments without answer, otherwise we'll just run
> in circles.

Sorry about that.

I have to float up object_property_add_child() because fw_cfg has path
'/machine/unattached/device[4]' (in my test) by default, the path seems
to vary so we can use it.

Please review, if no problem I'll post v17 shortly:

>From 5071f4bc6ff640637826d8ed7af9668d7bc1a642 Mon Sep 17 00:00:00 2001
From: Hu Tao <hutao@cn.fujitsu.com>
Date: Mon, 1 Apr 2013 15:51:59 +0800
Subject: [PATCH v16.1 5/5] pvpanic: pass configurable ioport to seabios

This lets seabios patch the corresponding SSDT entry.

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

diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
index 63a1998..712856c 100644
--- a/hw/fw_cfg.c
+++ b/hw/fw_cfg.c
@@ -489,11 +489,15 @@ 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/hw/fw_cfg.h b/hw/fw_cfg.h
index 05c8df1..07cc941 100644
--- a/hw/fw_cfg.h
+++ b/hw/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
diff --git a/hw/pvpanic.c b/hw/pvpanic.c
index ff0a116..4384831 100644
--- a/hw/pvpanic.c
+++ b/hw/pvpanic.c
@@ -18,6 +18,8 @@
 #include <sysemu/sysemu.h>
 #include <sysemu/kvm.h>
 
+#include "hw/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 = false;
+    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;
 }
 
-- 
1.8.1.4

Re: [Qemu-devel] [SeaBIOS] [PATCH v16] Add pvpanic device driver

[Kevin O'Connor]

On Sun, Mar 31, 2013 at 05:34:10PM +0300, Gleb Natapov wrote:
> On Sat, Mar 30, 2013 at 09:20:09AM -0400, Kevin O'Connor wrote:
> > On Fri, Mar 29, 2013 at 02:49:12PM +0100, Paolo Bonzini wrote:
> > > Il 29/03/2013 14:33, Kevin O'Connor ha scritto:
> > > > On Fri, Mar 29, 2013 at 04:18:44PM +0800, Hu Tao wrote:
> > > >> pvpanic device is used to notify host(qemu) when guest panic happens.
> > > > 
> > > > Thanks.  However, we're planning a move of ACPI tables from SeaBIOS to
> > > > QEMU.  I think this should wait until after the move.
> > > 
> > > The device should be in QEMU 1.5, and the SSDT probably will still be in
> > > SeaBIOS by then (and might even be the last to move, since it's quite
> > > complex and dynamic).  I don't think it is fair to block this patch on
> > > those grounds...
> > 
> > What is the user visible impact of not having a panic device?
> > 
> > My main concern is that the patch creates a new fw_cfg channel between
> > qemu and seabios thats sole purpose is to alter the OS visible ACPI
> > tables.  These types of QEMU->SeaBIOS interfaces are fragile and are
> > (in sum) quite complex.
> > 
> The patch uses existing channel between qemu and seabios, one
> romfile_loadint() is all it takes. We already have number of interfaces
> to change OS visible ACPI tables, that's why we want to move ACPI table
> creation to QEMU in the first place. It is unfortunate to start blocking
> features now before we have an alternative. When ACPI table creation
> will move into QEMU the code in this patch will be dropped along with
> all the other code that serves similar purpose.

Hi Gleb,

If there is a general consensus that this feature is important then
we'll go forward with adding it as is.  To be clear though, my
preference would be to go forward with moving ACPI tables into QEMU,
and then add this stuff on top of that.  If no one beats me to it,
I'll send some initial patches myself.

If we do merge this feature as is, it will also require a SeaBIOS
release (followed by a SeaBIOS pull into QEMU).  There weren't any
short-term plans for a new SeaBIOS release - if this feature demands
that then we need to start planning it now.

-Kevin

Re: [Qemu-devel] [SeaBIOS] [PATCH v16] Add pvpanic device driver

[Gleb Natapov]

On Mon, Apr 01, 2013 at 08:22:57PM -0400, Kevin O'Connor wrote:
> On Sun, Mar 31, 2013 at 05:34:10PM +0300, Gleb Natapov wrote:
> > On Sat, Mar 30, 2013 at 09:20:09AM -0400, Kevin O'Connor wrote:
> > > On Fri, Mar 29, 2013 at 02:49:12PM +0100, Paolo Bonzini wrote:
> > > > Il 29/03/2013 14:33, Kevin O'Connor ha scritto:
> > > > > On Fri, Mar 29, 2013 at 04:18:44PM +0800, Hu Tao wrote:
> > > > >> pvpanic device is used to notify host(qemu) when guest panic happens.
> > > > > 
> > > > > Thanks.  However, we're planning a move of ACPI tables from SeaBIOS to
> > > > > QEMU.  I think this should wait until after the move.
> > > > 
> > > > The device should be in QEMU 1.5, and the SSDT probably will still be in
> > > > SeaBIOS by then (and might even be the last to move, since it's quite
> > > > complex and dynamic).  I don't think it is fair to block this patch on
> > > > those grounds...
> > > 
> > > What is the user visible impact of not having a panic device?
> > > 
> > > My main concern is that the patch creates a new fw_cfg channel between
> > > qemu and seabios thats sole purpose is to alter the OS visible ACPI
> > > tables.  These types of QEMU->SeaBIOS interfaces are fragile and are
> > > (in sum) quite complex.
> > > 
> > The patch uses existing channel between qemu and seabios, one
> > romfile_loadint() is all it takes. We already have number of interfaces
> > to change OS visible ACPI tables, that's why we want to move ACPI table
> > creation to QEMU in the first place. It is unfortunate to start blocking
> > features now before we have an alternative. When ACPI table creation
> > will move into QEMU the code in this patch will be dropped along with
> > all the other code that serves similar purpose.
> 
> Hi Gleb,
> 
> If there is a general consensus that this feature is important then
> we'll go forward with adding it as is.  To be clear though, my
> preference would be to go forward with moving ACPI tables into QEMU,
> and then add this stuff on top of that.  If no one beats me to it,
> I'll send some initial patches myself.
> 
If we can accomplish the move before next major QEMU release we do not
need this new fw_cfg file obviously. Paolo thinks this is not feasible,
I haven't followed this work to close to have informed opinion.

> If we do merge this feature as is, it will also require a SeaBIOS
> release (followed by a SeaBIOS pull into QEMU).  There weren't any
> short-term plans for a new SeaBIOS release - if this feature demands
> that then we need to start planning it now.
> 
It is always good to have latest BIOS with new QEMU release IMO, so lest
plan it regardless.

--
			Gleb.

Re: [Qemu-devel] [SeaBIOS] [PATCH v16] Add pvpanic device driver

[Kevin O'Connor]

On Tue, Apr 02, 2013 at 12:07:46PM +0300, Gleb Natapov wrote:
> On Mon, Apr 01, 2013 at 08:22:57PM -0400, Kevin O'Connor wrote:
> > On Sun, Mar 31, 2013 at 05:34:10PM +0300, Gleb Natapov wrote:
> > > On Sat, Mar 30, 2013 at 09:20:09AM -0400, Kevin O'Connor wrote:
> > > The patch uses existing channel between qemu and seabios, one
> > > romfile_loadint() is all it takes. We already have number of interfaces
> > > to change OS visible ACPI tables, that's why we want to move ACPI table
> > > creation to QEMU in the first place. It is unfortunate to start blocking
> > > features now before we have an alternative. When ACPI table creation
> > > will move into QEMU the code in this patch will be dropped along with
> > > all the other code that serves similar purpose.
> > 
> > If there is a general consensus that this feature is important then
> > we'll go forward with adding it as is.  To be clear though, my
> > preference would be to go forward with moving ACPI tables into QEMU,
> > and then add this stuff on top of that.  If no one beats me to it,
> > I'll send some initial patches myself.
> > 
> If we can accomplish the move before next major QEMU release we do not
> need this new fw_cfg file obviously. Paolo thinks this is not feasible,
> I haven't followed this work to close to have informed opinion.

I was hoping I'd get a chance to submit some QEMU patches for this
before the soft-freeze, but unfortunately I have not been able to.
Since I don't want to hold up features, I remove my earlier objection
and I'm okay with committing this to SeaBIOS.

Hu Tao - if the QEMU part of the pvpanic series is committed to QEMU
I'll commit the corresponding SeaBIOS parts.

Apologies for the delay.
-Kevin

Re: [Qemu-devel] [SeaBIOS] [PATCH v16] Add pvpanic device driver

[Hu Tao]

On Tue, Apr 09, 2013 at 08:37:16PM -0400, Kevin O'Connor wrote:
> On Tue, Apr 02, 2013 at 12:07:46PM +0300, Gleb Natapov wrote:
> > On Mon, Apr 01, 2013 at 08:22:57PM -0400, Kevin O'Connor wrote:
> > > On Sun, Mar 31, 2013 at 05:34:10PM +0300, Gleb Natapov wrote:
> > > > On Sat, Mar 30, 2013 at 09:20:09AM -0400, Kevin O'Connor wrote:
> > > > The patch uses existing channel between qemu and seabios, one
> > > > romfile_loadint() is all it takes. We already have number of interfaces
> > > > to change OS visible ACPI tables, that's why we want to move ACPI table
> > > > creation to QEMU in the first place. It is unfortunate to start blocking
> > > > features now before we have an alternative. When ACPI table creation
> > > > will move into QEMU the code in this patch will be dropped along with
> > > > all the other code that serves similar purpose.
> > > 
> > > If there is a general consensus that this feature is important then
> > > we'll go forward with adding it as is.  To be clear though, my
> > > preference would be to go forward with moving ACPI tables into QEMU,
> > > and then add this stuff on top of that.  If no one beats me to it,
> > > I'll send some initial patches myself.
> > > 
> > If we can accomplish the move before next major QEMU release we do not
> > need this new fw_cfg file obviously. Paolo thinks this is not feasible,
> > I haven't followed this work to close to have informed opinion.
> 
> I was hoping I'd get a chance to submit some QEMU patches for this
> before the soft-freeze, but unfortunately I have not been able to.
> Since I don't want to hold up features, I remove my earlier objection
> and I'm okay with committing this to SeaBIOS.

Glad to hear that!

> 
> Hu Tao - if the QEMU part of the pvpanic series is committed to QEMU
> I'll commit the corresponding SeaBIOS parts.

Thanks a lot!

-- 
Regards,
Hu Tao