[PATCH v2] Add support for pvpanic pci device

[PATCH v2] Add support for pvpanic pci device

[Mihai Carabas]

This patchset adds support for pvpanic pci device. This is the second revision,
re-written after Peter Maydell's feedback:
- split the code in common, isa and pci
- obtained 0x101 device id per documenation from Gerd Hoffmann
- fix-up minor documentation issue

How to test this:
/usr/bin/qemu-system-aarch64 \
        -machine virt,gic-version=3 -device pvpanic-pci

After that you need to run a Linux kernel as guest, but you have to also apply
the patches I have sent for adding pci support for the pvpanic driver [1].

[1] https://lkml.org/lkml/2020/10/29/645



Mihai Carabas (3):
  hw/misc/pvpanic: split-out generic and bus dependent code
  hw/misc/pvpanic: add PCI interface support
  pvpanic : update pvpanic spec document

 docs/specs/pci-ids.txt    |  2 +
 docs/specs/pvpanic.txt    | 13 ++++++-
 hw/i386/Kconfig           |  1 -
 hw/misc/Kconfig           | 13 ++++++-
 hw/misc/meson.build       |  4 +-
 hw/misc/pvpanic-isa.c     | 95 +++++++++++++++++++++++++++++++++++++++++++++++
 hw/misc/pvpanic-pci.c     | 87 +++++++++++++++++++++++++++++++++++++++++++
 hw/misc/pvpanic.c         | 85 +++---------------------------------------
 include/hw/misc/pvpanic.h | 24 +++++++++++-
 include/hw/pci/pci.h      |  1 +
 tests/qtest/meson.build   |  2 +-
 11 files changed, 241 insertions(+), 86 deletions(-)
 create mode 100644 hw/misc/pvpanic-isa.c
 create mode 100644 hw/misc/pvpanic-pci.c

-- 
1.8.3.1

[PATCH 1/3] hw/misc/pvpanic: split-out generic and bus dependent code

[Mihai Carabas]

To ease the PCI device addition in next patches, split the code as follows:
- generic code (read/write/setup) is being kept in pvpanic.c
- ISA dependent code moved to pvpanic-isa.c

Also, rename:
- ISA_PVPANIC_DEVICE -> PVPANIC_ISA_DEVICE.
- TYPE_PVPANIC -> TYPE_PVPANIC_ISA.
- MemoryRegion io -> mr.
- pvpanic_ioport_* in pvpanic_*.

Update the build system with the new files and config structure.

Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
---
 hw/i386/Kconfig           |  1 -
 hw/misc/Kconfig           |  7 +++-
 hw/misc/meson.build       |  3 +-
 hw/misc/pvpanic-isa.c     | 95 +++++++++++++++++++++++++++++++++++++++++++++++
 hw/misc/pvpanic.c         | 85 +++---------------------------------------
 include/hw/misc/pvpanic.h | 23 +++++++++++-
 tests/qtest/meson.build   |  2 +-
 7 files changed, 131 insertions(+), 85 deletions(-)
 create mode 100644 hw/misc/pvpanic-isa.c

diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index eea059f..994fcaa 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -14,7 +14,6 @@ config PC
     imply ISA_DEBUG
     imply PARALLEL
     imply PCI_DEVICES
-    imply PVPANIC
     imply QXL
     imply SEV
     imply SGA
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index cf18ac0..b58e6fd 100644
--- a/hw/misc/Kconfig
+++ b/hw/misc/Kconfig
@@ -121,9 +121,14 @@ config IOTKIT_SYSCTL
 config IOTKIT_SYSINFO
     bool
 
-config PVPANIC
+config PVPANIC_COMMON
     bool
+
+config PVPANIC_ISA
+    bool
+    default y
     depends on ISA_BUS
+    select PVPANIC_COMMON
 
 config AUX
     bool
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index ce15ffc..8c828ad 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -13,6 +13,7 @@ softmmu_ss.add(when: 'CONFIG_EMC141X', if_true: files('emc141x.c'))
 softmmu_ss.add(when: 'CONFIG_UNIMP', if_true: files('unimp.c'))
 softmmu_ss.add(when: 'CONFIG_EMPTY_SLOT', if_true: files('empty_slot.c'))
 softmmu_ss.add(when: 'CONFIG_LED', if_true: files('led.c'))
+softmmu_ss.add(when: 'CONFIG_PVPANIC_COMMON', if_true: files('pvpanic.c'))
 
 # ARM devices
 softmmu_ss.add(when: 'CONFIG_PL310', if_true: files('arm_l2x0.c'))
@@ -97,7 +98,7 @@ softmmu_ss.add(when: 'CONFIG_IOTKIT_SYSINFO', if_true: files('iotkit-sysinfo.c')
 softmmu_ss.add(when: 'CONFIG_ARMSSE_CPUID', if_true: files('armsse-cpuid.c'))
 softmmu_ss.add(when: 'CONFIG_ARMSSE_MHU', if_true: files('armsse-mhu.c'))
 
-softmmu_ss.add(when: 'CONFIG_PVPANIC', if_true: files('pvpanic.c'))
+softmmu_ss.add(when: 'CONFIG_PVPANIC_ISA', if_true: files('pvpanic-isa.c'))
 softmmu_ss.add(when: 'CONFIG_AUX', if_true: files('auxbus.c'))
 softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_scu.c', 'aspeed_sdmc.c', 'aspeed_xdma.c'))
 softmmu_ss.add(when: 'CONFIG_MSF2', if_true: files('msf2-sysreg.c'))
diff --git a/hw/misc/pvpanic-isa.c b/hw/misc/pvpanic-isa.c
new file mode 100644
index 0000000..6ebcd4e
--- /dev/null
+++ b/hw/misc/pvpanic-isa.c
@@ -0,0 +1,95 @@
+/*
+ * 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 "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "sysemu/runstate.h"
+
+#include "hw/nvram/fw_cfg.h"
+#include "hw/qdev-properties.h"
+#include "hw/misc/pvpanic.h"
+#include "qom/object.h"
+#include "hw/isa/isa.h"
+
+typedef struct PVPanicISAState PVPanicISAState;
+DECLARE_INSTANCE_CHECKER(PVPanicISAState, PVPANIC_ISA_DEVICE,
+                         TYPE_PVPANIC_ISA)
+
+/*
+ * PVPanicISAState for ISA device and
+ * use ioport.
+ */
+struct PVPanicISAState {
+    ISADevice parent_obj;
+
+    uint16_t ioport;
+    PVPanicState pvpanic;
+};
+
+static void pvpanic_isa_initfn(Object *obj)
+{
+    PVPanicISAState *s = PVPANIC_ISA_DEVICE(obj);
+
+    pvpanic_setup_io(&s->pvpanic, DEVICE(s), 1);
+}
+
+static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp)
+{
+    ISADevice *d = ISA_DEVICE(dev);
+    PVPanicISAState *s = PVPANIC_ISA_DEVICE(dev);
+    FWCfgState *fw_cfg = fw_cfg_find();
+    uint16_t *pvpanic_port;
+
+    if (!fw_cfg) {
+        return;
+    }
+
+    pvpanic_port = g_malloc(sizeof(*pvpanic_port));
+    *pvpanic_port = cpu_to_le16(s->ioport);
+    fw_cfg_add_file(fw_cfg, "etc/pvpanic-port", pvpanic_port,
+                    sizeof(*pvpanic_port));
+
+    isa_register_ioport(d, &s->mr, s->ioport);
+}
+
+static Property pvpanic_isa_properties[] = {
+    DEFINE_PROP_UINT16(PVPANIC_IOPORT_PROP, PVPanicISAState, ioport, 0x505),
+    DEFINE_PROP_UINT8("events", PVPanicState, events, PVPANIC_PANICKED | PVPANIC_CRASHLOADED),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void pvpanic_isa_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = pvpanic_isa_realizefn;
+    device_class_set_props(dc, pvpanic_isa_properties);
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+}
+
+static TypeInfo pvpanic_isa_info = {
+    .name          = TYPE_PVPANIC_ISA,
+    .parent        = TYPE_ISA_DEVICE,
+    .instance_size = sizeof(PVPanicISAState),
+    .instance_init = pvpanic_isa_initfn,
+    .class_init    = pvpanic_isa_class_init,
+};
+
+static void pvpanic_register_types(void)
+{
+    type_register_static(&pvpanic_isa_info);
+}
+
+type_init(pvpanic_register_types)
diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index 35d6797..e2cb4a5 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -22,18 +22,6 @@
 #include "hw/misc/pvpanic.h"
 #include "qom/object.h"
 
-/* The bit of supported pv event, TODO: include uapi header and remove this */
-#define PVPANIC_F_PANICKED      0
-#define PVPANIC_F_CRASHLOADED   1
-
-/* The pv event value */
-#define PVPANIC_PANICKED        (1 << PVPANIC_F_PANICKED)
-#define PVPANIC_CRASHLOADED     (1 << PVPANIC_F_CRASHLOADED)
-
-typedef struct PVPanicState PVPanicState;
-DECLARE_INSTANCE_CHECKER(PVPanicState, ISA_PVPANIC_DEVICE,
-                         TYPE_PVPANIC)
-
 static void handle_event(int event)
 {
     static bool logged;
@@ -54,90 +42,29 @@ static void handle_event(int event)
     }
 }
 
-#include "hw/isa/isa.h"
-
-struct PVPanicState {
-    ISADevice parent_obj;
-
-    MemoryRegion io;
-    uint16_t ioport;
-    uint8_t events;
-};
-
 /* return supported events on read */
-static uint64_t pvpanic_ioport_read(void *opaque, hwaddr addr, unsigned size)
+static uint64_t pvpanic_read(void *opaque, hwaddr addr, unsigned size)
 {
     PVPanicState *pvp = opaque;
     return pvp->events;
 }
 
-static void pvpanic_ioport_write(void *opaque, hwaddr addr, uint64_t val,
+static void pvpanic_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,
+    .read = pvpanic_read,
+    .write = pvpanic_write,
     .impl = {
         .min_access_size = 1,
         .max_access_size = 1,
     },
 };
 
-static void pvpanic_isa_initfn(Object *obj)
-{
-    PVPanicState *s = ISA_PVPANIC_DEVICE(obj);
-
-    memory_region_init_io(&s->io, OBJECT(s), &pvpanic_ops, s, "pvpanic", 1);
-}
-
-static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp)
-{
-    ISADevice *d = ISA_DEVICE(dev);
-    PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
-    FWCfgState *fw_cfg = fw_cfg_find();
-    uint16_t *pvpanic_port;
-
-    if (!fw_cfg) {
-        return;
-    }
-
-    pvpanic_port = g_malloc(sizeof(*pvpanic_port));
-    *pvpanic_port = cpu_to_le16(s->ioport);
-    fw_cfg_add_file(fw_cfg, "etc/pvpanic-port", pvpanic_port,
-                    sizeof(*pvpanic_port));
-
-    isa_register_ioport(d, &s->io, s->ioport);
-}
-
-static Property pvpanic_isa_properties[] = {
-    DEFINE_PROP_UINT16(PVPANIC_IOPORT_PROP, PVPanicState, ioport, 0x505),
-    DEFINE_PROP_UINT8("events", PVPanicState, events, PVPANIC_PANICKED | PVPANIC_CRASHLOADED),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
-static void pvpanic_isa_class_init(ObjectClass *klass, void *data)
+void pvpanic_setup_io(PVPanicState *s, DeviceState *dev, unsigned size)
 {
-    DeviceClass *dc = DEVICE_CLASS(klass);
-
-    dc->realize = pvpanic_isa_realizefn;
-    device_class_set_props(dc, pvpanic_isa_properties);
-    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+    memory_region_init_io(&s->mr, OBJECT(dev), &pvpanic_ops, s, "pvpanic", size);
 }
-
-static TypeInfo pvpanic_isa_info = {
-    .name          = TYPE_PVPANIC,
-    .parent        = TYPE_ISA_DEVICE,
-    .instance_size = sizeof(PVPanicState),
-    .instance_init = pvpanic_isa_initfn,
-    .class_init    = pvpanic_isa_class_init,
-};
-
-static void pvpanic_register_types(void)
-{
-    type_register_static(&pvpanic_isa_info);
-}
-
-type_init(pvpanic_register_types)
diff --git a/include/hw/misc/pvpanic.h b/include/hw/misc/pvpanic.h
index ae0c818..691cf61 100644
--- a/include/hw/misc/pvpanic.h
+++ b/include/hw/misc/pvpanic.h
@@ -17,13 +17,32 @@
 
 #include "qom/object.h"
 
-#define TYPE_PVPANIC "pvpanic"
+#define TYPE_PVPANIC_ISA "pvpanic"
 
 #define PVPANIC_IOPORT_PROP "ioport"
 
+/* The bit of supported pv event, TODO: include uapi header and remove this */
+#define PVPANIC_F_PANICKED      0
+#define PVPANIC_F_CRASHLOADED   1
+
+/* The pv event value */
+#define PVPANIC_PANICKED        (1 << PVPANIC_F_PANICKED)
+#define PVPANIC_CRASHLOADED     (1 << PVPANIC_F_CRASHLOADED)
+
+/*
+ * PVPanicState for any device type
+ */
+typedef struct PVPanicState PVPanicState;
+struct PVPanicState {
+    MemoryRegion mr;
+    uint8_t events;
+};
+
+void pvpanic_setup_io(PVPanicState *s, DeviceState *dev, unsigned size);
+
 static inline uint16_t pvpanic_port(void)
 {
-    Object *o = object_resolve_path_type("", TYPE_PVPANIC, NULL);
+    Object *o = object_resolve_path_type("", TYPE_PVPANIC_ISA, NULL);
     if (!o) {
         return 0;
     }
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 6a67c53..d7b5a9e 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -33,7 +33,7 @@ qtests_i386 = \
   (config_host.has_key('CONFIG_LINUX') and                                                  \
    config_all_devices.has_key('CONFIG_ISA_IPMI_BT') ? ['ipmi-bt-test'] : []) +              \
   (config_all_devices.has_key('CONFIG_WDT_IB700') ? ['wdt_ib700-test'] : []) +              \
-  (config_all_devices.has_key('CONFIG_PVPANIC') ? ['pvpanic-test'] : []) +                  \
+  (config_all_devices.has_key('CONFIG_PVPANIC_COMMON') ? ['pvpanic-test'] : []) +           \
   (config_all_devices.has_key('CONFIG_HDA') ? ['intel-hda-test'] : []) +                    \
   (config_all_devices.has_key('CONFIG_I82801B11') ? ['i82801b11-test'] : []) +             \
   (config_all_devices.has_key('CONFIG_IOH3420') ? ['ioh3420-test'] : []) +                  \
-- 
1.8.3.1

[PATCH 2/3] hw/misc/pvpanic: add PCI interface support

[Mihai Carabas]

Add PCI interface support for PVPANIC device. Create a new file pvpanic-pci.c
where the PCI specific routines reside and update the build system with the new
files and config structure.

Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
---
 docs/specs/pci-ids.txt    |  2 ++
 hw/misc/Kconfig           |  6 ++++
 hw/misc/meson.build       |  1 +
 hw/misc/pvpanic-pci.c     | 87 +++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/misc/pvpanic.h |  1 +
 include/hw/pci/pci.h      |  1 +
 6 files changed, 98 insertions(+)
 create mode 100644 hw/misc/pvpanic-pci.c

diff --git a/docs/specs/pci-ids.txt b/docs/specs/pci-ids.txt
index abbdbca..191681d 100644
--- a/docs/specs/pci-ids.txt
+++ b/docs/specs/pci-ids.txt
@@ -68,3 +68,5 @@ PCI devices (other than virtio):
 All these devices are documented in docs/specs.
 
 The 0100 device ID is used for the QXL video card device.
+
+The 0101 device ID is used for the PVPanic device.
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index b58e6fd..aca7ffb 100644
--- a/hw/misc/Kconfig
+++ b/hw/misc/Kconfig
@@ -124,6 +124,12 @@ config IOTKIT_SYSINFO
 config PVPANIC_COMMON
     bool
 
+config PVPANIC_PCI
+    bool
+    default y if PCI_DEVICES
+    depends on PCI
+    select PVPANIC_COMMON
+
 config PVPANIC_ISA
     bool
     default y
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 8c828ad..f686019 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -99,6 +99,7 @@ softmmu_ss.add(when: 'CONFIG_ARMSSE_CPUID', if_true: files('armsse-cpuid.c'))
 softmmu_ss.add(when: 'CONFIG_ARMSSE_MHU', if_true: files('armsse-mhu.c'))
 
 softmmu_ss.add(when: 'CONFIG_PVPANIC_ISA', if_true: files('pvpanic-isa.c'))
+softmmu_ss.add(when: 'CONFIG_PVPANIC_PCI', if_true: files('pvpanic-pci.c'))
 softmmu_ss.add(when: 'CONFIG_AUX', if_true: files('auxbus.c'))
 softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_scu.c', 'aspeed_sdmc.c', 'aspeed_xdma.c'))
 softmmu_ss.add(when: 'CONFIG_MSF2', if_true: files('msf2-sysreg.c'))
diff --git a/hw/misc/pvpanic-pci.c b/hw/misc/pvpanic-pci.c
new file mode 100644
index 0000000..173909a
--- /dev/null
+++ b/hw/misc/pvpanic-pci.c
@@ -0,0 +1,87 @@
+/*
+ * QEMU simulated PCI pvpanic device.
+ *
+ * Copyright (C) 2020 Oracle
+ *
+ * Authors:
+ *     Mihai Carabas <mihai.carabas@oracle.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 "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "sysemu/runstate.h"
+
+#include "hw/nvram/fw_cfg.h"
+#include "hw/qdev-properties.h"
+#include "hw/misc/pvpanic.h"
+#include "qom/object.h"
+#include "hw/pci/pci.h"
+
+typedef struct PVPanicPCIState PVPanicPCIState;
+DECLARE_INSTANCE_CHECKER(PVPanicPCIState, PVPANIC_PCI_DEVICE,
+                         TYPE_PVPANIC_PCI)
+
+/*
+ * PVPanicPCIState for PCI device
+ */
+typedef struct PVPanicPCIState {
+    PCIDevice dev;
+    PVPanicState pvpanic;
+} PVPanicPCIState;
+
+/* pvpanic pci device*/
+
+static void pvpanic_pci_realizefn(PCIDevice *dev, Error **errp)
+{
+    PVPanicPCIState *s = DO_UPCAST(PVPanicPCIState, dev, dev);
+    PVPanicState *ps = &s->pvpanic;
+
+    pvpanic_setup_io(&s->pvpanic, DEVICE(s), 2);
+
+    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &ps->mr);
+}
+
+static Property pvpanic_pci_properties[] = {
+    DEFINE_PROP_UINT8("events", PVPanicState, events, PVPANIC_PANICKED | PVPANIC_CRASHLOADED),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void pvpanic_pci_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *pc = PCI_DEVICE_CLASS(klass);
+
+    device_class_set_props(dc, pvpanic_pci_properties);
+
+    pc->realize = pvpanic_pci_realizefn;
+    pc->vendor_id = PCI_VENDOR_ID_REDHAT;
+    pc->device_id = PCI_DEVICE_ID_REDHAT_PVPANIC;
+    pc->revision = 1;
+    pc->class_id = PCI_CLASS_SYSTEM_OTHER;
+
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+}
+
+static TypeInfo pvpanic_pci_info = {
+    .name          = TYPE_PVPANIC_PCI,
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(PVPanicPCIState),
+    .class_init    = pvpanic_pci_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { INTERFACE_PCIE_DEVICE },
+        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+        { }
+    }
+};
+
+static void pvpanic_register_types(void)
+{
+    type_register_static(&pvpanic_pci_info);
+}
+
+type_init(pvpanic_register_types);
diff --git a/include/hw/misc/pvpanic.h b/include/hw/misc/pvpanic.h
index 691cf61..de1d0a9 100644
--- a/include/hw/misc/pvpanic.h
+++ b/include/hw/misc/pvpanic.h
@@ -18,6 +18,7 @@
 #include "qom/object.h"
 
 #define TYPE_PVPANIC_ISA "pvpanic"
+#define TYPE_PVPANIC_PCI "pvpanic-pci"
 
 #define PVPANIC_IOPORT_PROP "ioport"
 
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 259f9c9..e9512ab 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -108,6 +108,7 @@ extern bool pci_available;
 #define PCI_DEVICE_ID_REDHAT_MDPY        0x000f
 #define PCI_DEVICE_ID_REDHAT_NVME        0x0010
 #define PCI_DEVICE_ID_REDHAT_QXL         0x0100
+#define PCI_DEVICE_ID_REDHAT_PVPANIC     0x0101
 
 #define FMT_PCIBUS                      PRIx64
 
-- 
1.8.3.1

[PATCH 3/3] pvpanic : update pvpanic spec document

[Mihai Carabas]

Add pvpanic PCI device support details in docs/specs/pvpanic.txt.

Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
---
 docs/specs/pvpanic.txt | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/docs/specs/pvpanic.txt b/docs/specs/pvpanic.txt
index a90fbca..5ddc8df 100644
--- a/docs/specs/pvpanic.txt
+++ b/docs/specs/pvpanic.txt
@@ -1,7 +1,7 @@
 PVPANIC DEVICE
 ==============
 
-pvpanic device is a simulated ISA device, through which a guest panic
+pvpanic device is a simulated 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.
 
@@ -9,6 +9,9 @@ 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.
 
+The pvpanic device can be implemented as an ISA device (using IOPORT) or as a
+PCI device.
+
 ISA Interface
 -------------
 
@@ -24,6 +27,14 @@ bit 1: a guest panic has happened and will be handled by the guest;
        the host should record it or report it, but should not affect
        the execution of the guest.
 
+PCI Interface
+-------------
+
+The PCI interface is similar to the ISA interface except that it uses an MMIO
+address space provided by its BAR0, 2 bytes long. Any machine with a PCI
+device can enable a pvpanic device by adding '-device pvpanic-pci' to the
+command line.
+
 ACPI Interface
 --------------
 
-- 
1.8.3.1

Re: [PATCH 2/3] hw/misc/pvpanic: add PCI interface support

[Gerd Hoffmann]

> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 259f9c9..e9512ab 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -108,6 +108,7 @@ extern bool pci_available;
>  #define PCI_DEVICE_ID_REDHAT_MDPY        0x000f
>  #define PCI_DEVICE_ID_REDHAT_NVME        0x0010
>  #define PCI_DEVICE_ID_REDHAT_QXL         0x0100
> +#define PCI_DEVICE_ID_REDHAT_PVPANIC     0x0101

qxl has 0x100 for historical reasons, otherwise devices are simply
enumerated upwards.  NVME has 0x10, so PVPANIC should simply take
the next free which is 0x11.  Didn't I mention that in my last mail?

thanks,
  Gerd

Re: [PATCH 2/3] hw/misc/pvpanic: add PCI interface support

[Mihai Carabas]

> On 22 Dec 2020, at 17:30, Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> 
>> 
>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>> index 259f9c9..e9512ab 100644
>> --- a/include/hw/pci/pci.h
>> +++ b/include/hw/pci/pci.h
>> @@ -108,6 +108,7 @@ extern bool pci_available;
>> #define PCI_DEVICE_ID_REDHAT_MDPY        0x000f
>> #define PCI_DEVICE_ID_REDHAT_NVME        0x0010
>> #define PCI_DEVICE_ID_REDHAT_QXL         0x0100
>> +#define PCI_DEVICE_ID_REDHAT_PVPANIC     0x0101
> 
> qxl has 0x100 for historical reasons, otherwise devices are simply
> enumerated upwards.  NVME has 0x10, so PVPANIC should simply take
> the next free which is 0x11.  Didn't I mention that in my last mail?
> 
> thanks,
>  Gerd
Sorry, my mistake. I did not read your email carefully. Consider this modification done.

Waiting for other feedback a couple of days and I will send a new revision with this update.

Thank you,
Mihai
> 

Re: [PATCH 3/3] pvpanic : update pvpanic spec document

[Peter Maydell]

On Fri, 18 Dec 2020 at 13:36, Mihai Carabas <mihai.carabas@oracle.com> wrote:
>
> Add pvpanic PCI device support details in docs/specs/pvpanic.txt.
>
> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
> ---
>  docs/specs/pvpanic.txt | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/docs/specs/pvpanic.txt b/docs/specs/pvpanic.txt
> index a90fbca..5ddc8df 100644
> --- a/docs/specs/pvpanic.txt
> +++ b/docs/specs/pvpanic.txt
> @@ -1,7 +1,7 @@
>  PVPANIC DEVICE
>  ==============
>
> -pvpanic device is a simulated ISA device, through which a guest panic
> +pvpanic device is a simulated 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.
>
> @@ -9,6 +9,9 @@ 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.
>
> +The pvpanic device can be implemented as an ISA device (using IOPORT) or as a
> +PCI device.
> +
>  ISA Interface
>  -------------
>
> @@ -24,6 +27,14 @@ bit 1: a guest panic has happened and will be handled by the guest;
>         the host should record it or report it, but should not affect
>         the execution of the guest.
>
> +PCI Interface
> +-------------
> +
> +The PCI interface is similar to the ISA interface except that it uses an MMIO
> +address space provided by its BAR0, 2 bytes long. Any machine with a PCI
> +device can enable a pvpanic device by adding '-device pvpanic-pci' to the
> +command line.

Why is it 2 bytes long, when both the ISA and the ACPI interfaces are
only a single byte? What size reads and writes are accepted?

thanks
-- PMM

Re: [PATCH 1/3] hw/misc/pvpanic: split-out generic and bus dependent code

[Peter Maydell]

On Fri, 18 Dec 2020 at 13:36, Mihai Carabas <mihai.carabas@oracle.com> wrote:
>
> To ease the PCI device addition in next patches, split the code as follows:
> - generic code (read/write/setup) is being kept in pvpanic.c
> - ISA dependent code moved to pvpanic-isa.c
>
> Also, rename:
> - ISA_PVPANIC_DEVICE -> PVPANIC_ISA_DEVICE.
> - TYPE_PVPANIC -> TYPE_PVPANIC_ISA.
> - MemoryRegion io -> mr.
> - pvpanic_ioport_* in pvpanic_*.
>
> Update the build system with the new files and config structure.
>
> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
> ---
>  hw/i386/Kconfig           |  1 -
>  hw/misc/Kconfig           |  7 +++-
>  hw/misc/meson.build       |  3 +-
>  hw/misc/pvpanic-isa.c     | 95 +++++++++++++++++++++++++++++++++++++++++++++++
>  hw/misc/pvpanic.c         | 85 +++---------------------------------------
>  include/hw/misc/pvpanic.h | 23 +++++++++++-
>  tests/qtest/meson.build   |  2 +-
>  7 files changed, 131 insertions(+), 85 deletions(-)
>  create mode 100644 hw/misc/pvpanic-isa.c

Hi; couple of minor review issues below but mostly this looks good.

> diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
> index eea059f..994fcaa 100644
> --- a/hw/i386/Kconfig
> +++ b/hw/i386/Kconfig
> @@ -14,7 +14,6 @@ config PC
>      imply ISA_DEBUG
>      imply PARALLEL
>      imply PCI_DEVICES
> -    imply PVPANIC
>      imply QXL
>      imply SEV
>      imply SGA

Why is it ok for this imply line to just go away rather
than changing to "imply PVPANIC_ISA" ? I think the answer is
the "default y" in the Kconfig file below, but really that
is changing behaviour (making PVPANIC available on all
guests with ISA, not just PC) -- so it ought to be done
in a separate patch, so this one can be purely refactoring
with no user-visible changes.

> diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
> index cf18ac0..b58e6fd 100644
> --- a/hw/misc/Kconfig
> +++ b/hw/misc/Kconfig
> @@ -121,9 +121,14 @@ config IOTKIT_SYSCTL
>  config IOTKIT_SYSINFO
>      bool
>
> -config PVPANIC
> +config PVPANIC_COMMON
>      bool
> +
> +config PVPANIC_ISA
> +    bool
> +    default y
>      depends on ISA_BUS
> +    select PVPANIC_COMMON
>
>  config AUX
>      bool



> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 6a67c53..d7b5a9e 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -33,7 +33,7 @@ qtests_i386 = \
>    (config_host.has_key('CONFIG_LINUX') and                                                  \
>     config_all_devices.has_key('CONFIG_ISA_IPMI_BT') ? ['ipmi-bt-test'] : []) +              \
>    (config_all_devices.has_key('CONFIG_WDT_IB700') ? ['wdt_ib700-test'] : []) +              \
> -  (config_all_devices.has_key('CONFIG_PVPANIC') ? ['pvpanic-test'] : []) +                  \
> +  (config_all_devices.has_key('CONFIG_PVPANIC_COMMON') ? ['pvpanic-test'] : []) +           \
>    (config_all_devices.has_key('CONFIG_HDA') ? ['intel-hda-test'] : []) +                    \
>    (config_all_devices.has_key('CONFIG_I82801B11') ? ['i82801b11-test'] : []) +             \
>    (config_all_devices.has_key('CONFIG_IOH3420') ? ['ioh3420-test'] : []) +                  \

The pvpanic-test test only tests the ISA device, so I think it
either needs to be only built for CONFIG_PVPANIC_ISA, or
the tests themselves within the pvpanic-test.c file would need to
be updated to somehow skip if the QEMU under test didn't have
the ISA pvpanic device. The former seems easier.

While I'm talking about tests, it would be nice to have a
basic test of the new pvpanic-pci device too. If you put that
in its own pvpanic-pci-test.c file then you can make that one
be dependent on CONFIG_PVPANIC_PCI.

thanks
-- PMM

Re: [PATCH 2/3] hw/misc/pvpanic: add PCI interface support

[Peter Maydell]

On Fri, 18 Dec 2020 at 13:36, Mihai Carabas <mihai.carabas@oracle.com> wrote:
>
> Add PCI interface support for PVPANIC device. Create a new file pvpanic-pci.c
> where the PCI specific routines reside and update the build system with the new
> files and config structure.
>
> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
> ---
>  docs/specs/pci-ids.txt    |  2 ++
>  hw/misc/Kconfig           |  6 ++++
>  hw/misc/meson.build       |  1 +
>  hw/misc/pvpanic-pci.c     | 87 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/misc/pvpanic.h |  1 +
>  include/hw/pci/pci.h      |  1 +
>  6 files changed, 98 insertions(+)
>  create mode 100644 hw/misc/pvpanic-pci.c
>
> diff --git a/docs/specs/pci-ids.txt b/docs/specs/pci-ids.txt
> index abbdbca..191681d 100644
> --- a/docs/specs/pci-ids.txt
> +++ b/docs/specs/pci-ids.txt
> @@ -68,3 +68,5 @@ PCI devices (other than virtio):
>  All these devices are documented in docs/specs.
>
>  The 0100 device ID is used for the QXL video card device.
> +
> +The 0101 device ID is used for the PVPanic device.
> diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
> index b58e6fd..aca7ffb 100644
> --- a/hw/misc/Kconfig
> +++ b/hw/misc/Kconfig
> @@ -124,6 +124,12 @@ config IOTKIT_SYSINFO
>  config PVPANIC_COMMON
>      bool
>
> +config PVPANIC_PCI
> +    bool
> +    default y if PCI_DEVICES
> +    depends on PCI
> +    select PVPANIC_COMMON
> +
>  config PVPANIC_ISA
>      bool
>      default y
> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> index 8c828ad..f686019 100644
> --- a/hw/misc/meson.build
> +++ b/hw/misc/meson.build
> @@ -99,6 +99,7 @@ softmmu_ss.add(when: 'CONFIG_ARMSSE_CPUID', if_true: files('armsse-cpuid.c'))
>  softmmu_ss.add(when: 'CONFIG_ARMSSE_MHU', if_true: files('armsse-mhu.c'))
>
>  softmmu_ss.add(when: 'CONFIG_PVPANIC_ISA', if_true: files('pvpanic-isa.c'))
> +softmmu_ss.add(when: 'CONFIG_PVPANIC_PCI', if_true: files('pvpanic-pci.c'))
>  softmmu_ss.add(when: 'CONFIG_AUX', if_true: files('auxbus.c'))
>  softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_scu.c', 'aspeed_sdmc.c', 'aspeed_xdma.c'))
>  softmmu_ss.add(when: 'CONFIG_MSF2', if_true: files('msf2-sysreg.c'))
> diff --git a/hw/misc/pvpanic-pci.c b/hw/misc/pvpanic-pci.c
> new file mode 100644
> index 0000000..173909a
> --- /dev/null
> +++ b/hw/misc/pvpanic-pci.c
> @@ -0,0 +1,87 @@
> +/*
> + * QEMU simulated PCI pvpanic device.
> + *
> + * Copyright (C) 2020 Oracle
> + *
> + * Authors:
> + *     Mihai Carabas <mihai.carabas@oracle.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 "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "sysemu/runstate.h"
> +
> +#include "hw/nvram/fw_cfg.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/misc/pvpanic.h"
> +#include "qom/object.h"
> +#include "hw/pci/pci.h"
> +
> +typedef struct PVPanicPCIState PVPanicPCIState;
> +DECLARE_INSTANCE_CHECKER(PVPanicPCIState, PVPANIC_PCI_DEVICE,
> +                         TYPE_PVPANIC_PCI)

The doc comment for the DECLARE_INSTANCE_CHECKER() macro
says "Direct usage of this macro should be avoided, and
the complete OBJECT_DECLARE_TYPE macro is recommended instead."

> +
> +/*
> + * PVPanicPCIState for PCI device
> + */
> +typedef struct PVPanicPCIState {
> +    PCIDevice dev;
> +    PVPanicState pvpanic;
> +} PVPanicPCIState;
> +
> +/* pvpanic pci device*/

Missing space before '*/', but the comment isn't really telling
the reader anything, so you could just delete it.

> +
> +static void pvpanic_pci_realizefn(PCIDevice *dev, Error **errp)
> +{
> +    PVPanicPCIState *s = DO_UPCAST(PVPanicPCIState, dev, dev);

Since this is a QOM device, better to use the QOM cast rather
than DO_UPCAST():

   PVPanicPCIState *s = PVPANIC_PCI_DEVICE(dev);

> +    PVPanicState *ps = &s->pvpanic;
> +
> +    pvpanic_setup_io(&s->pvpanic, DEVICE(s), 2);

Why 2 bytes?

> +    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &ps->mr);
> +}

> +static void pvpanic_pci_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    PCIDeviceClass *pc = PCI_DEVICE_CLASS(klass);
> +
> +    device_class_set_props(dc, pvpanic_pci_properties);
> +
> +    pc->realize = pvpanic_pci_realizefn;
> +    pc->vendor_id = PCI_VENDOR_ID_REDHAT;
> +    pc->device_id = PCI_DEVICE_ID_REDHAT_PVPANIC;
> +    pc->revision = 1;
> +    pc->class_id = PCI_CLASS_SYSTEM_OTHER;
> +
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +}

You also need to set the dc->vmsd to a VMState for this
device. The ISA pvpanic didn't need one because the pvpanic
device itself has no variable state and an ISA device
doesn't either, but PCI devices do have guest-modifiable
state, so you need a VMState structure that uses a
VMSTATE_PCI_DEVICE() line to ensure it gets saved and
restored on migration.

> +static TypeInfo pvpanic_pci_info = {
> +    .name          = TYPE_PVPANIC_PCI,
> +    .parent        = TYPE_PCI_DEVICE,
> +    .instance_size = sizeof(PVPanicPCIState),
> +    .class_init    = pvpanic_pci_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { INTERFACE_PCIE_DEVICE },
> +        { INTERFACE_CONVENTIONAL_PCI_DEVICE },

I'm not very familiar with the PCI code, but are we definitely
doing enough to be able to claim to be a PCIe device ?

> +        { }
> +    }
> +};
> +
> +static void pvpanic_register_types(void)
> +{
> +    type_register_static(&pvpanic_pci_info);
> +}
> +
> +type_init(pvpanic_register_types);

thanks
-- PMM