[PATCH v6] Add support for pvpanic pci device

[PATCH v6] Add support for pvpanic pci device

[Mihai Carabas]

This patchset adds support for pvpanic pci device.

v3:
- patch 1: made pvpanic isa device available only for PC, compile pvpanic-test
  only when isa device is present
- patch 2: fixed device id to 0x0011, used OBJECT_DECLARE_TYPE,
  PVPANIC_PCI_DEVICE, added VMSTATE_PCI_DEVICE, removed INTERFACE_PCIE_DEVICE
- patch 3: fixed documentation
- patch 4: add a qtest for pvpanic-pci

v4:
- added Rb/Ack on patches
- modify test case to include -action parameter that was recently added and also
  to be on par with the pvpanic ISA device testing

v5:
- added subsystem_vendor_id and subsystem_id needed for MS WHQL tests

v6:
- removed subsystem_vendor_id and subsystem_id as they are filed out by default
  if empty
- do not compile pvpanic-pci-test for ppc64 as we our tests do not support that
  platform

Mihai Carabas (4):
  hw/misc/pvpanic: split-out generic and bus dependent code
  hw/misc/pvpanic: add PCI interface support
  pvpanic : update pvpanic spec document
  tests/qtest: add a test case for pvpanic-pci

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

-- 
1.8.3.1

[PATCH v6 1/4] 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/i386/Kconfig           |  2 +-
 hw/misc/Kconfig           |  6 ++-
 hw/misc/meson.build       |  3 +-
 hw/misc/pvpanic-isa.c     | 94 +++++++++++++++++++++++++++++++++++++++++++++++
 hw/misc/pvpanic.c         | 85 +++---------------------------------------
 include/hw/misc/pvpanic.h | 23 +++++++++++-
 tests/qtest/meson.build   |  2 +-
 7 files changed, 130 insertions(+), 85 deletions(-)
 create mode 100644 hw/misc/pvpanic-isa.c

diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index eea059f..7f91f30 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -14,7 +14,7 @@ config PC
     imply ISA_DEBUG
     imply PARALLEL
     imply PCI_DEVICES
-    imply PVPANIC
+    imply PVPANIC_ISA
     imply QXL
     imply SEV
     imply SGA
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index cf18ac0..23bc978 100644
--- a/hw/misc/Kconfig
+++ b/hw/misc/Kconfig
@@ -121,9 +121,13 @@ config IOTKIT_SYSCTL
 config IOTKIT_SYSINFO
     bool
 
-config PVPANIC
+config PVPANIC_COMMON
+    bool
+
+config PVPANIC_ISA
     bool
     depends on ISA_BUS
+    select PVPANIC_COMMON
 
 config AUX
     bool
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 607cd38..edaaec2 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'))
@@ -98,7 +99,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..27113ab
--- /dev/null
+++ b/hw/misc/pvpanic-isa.c
@@ -0,0 +1,94 @@
+/*
+ * 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"
+
+OBJECT_DECLARE_SIMPLE_TYPE(PVPanicISAState, PVPANIC_ISA_DEVICE)
+
+/*
+ * 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);
+    PVPanicState *ps = &s->pvpanic;
+    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, &ps->mr, s->ioport);
+}
+
+static Property pvpanic_isa_properties[] = {
+    DEFINE_PROP_UINT16(PVPANIC_IOPORT_PROP, PVPanicISAState, ioport, 0x505),
+    DEFINE_PROP_UINT8("events", PVPanicISAState, pvpanic.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_DEVICE,
+    .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..abc9dde 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_DEVICE "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_DEVICE, NULL);
     if (!o) {
         return 0;
     }
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 16d0462..0e85343 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_ISA') ? ['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 v6 2/4] 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>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
---
 docs/specs/pci-ids.txt    |  1 +
 hw/misc/Kconfig           |  6 +++
 hw/misc/meson.build       |  1 +
 hw/misc/pvpanic-pci.c     | 94 +++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/misc/pvpanic.h |  1 +
 include/hw/pci/pci.h      |  1 +
 6 files changed, 104 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..5e407a6 100644
--- a/docs/specs/pci-ids.txt
+++ b/docs/specs/pci-ids.txt
@@ -64,6 +64,7 @@ PCI devices (other than virtio):
 1b36:000d  PCI xhci usb host adapter
 1b36:000f  mdpy (mdev sample device), linux/samples/vfio-mdev/mdpy.c
 1b36:0010  PCIe NVMe device (-device nvme)
+1b36:0011  PCI PVPanic device (-device pvpanic-pci)
 
 All these devices are documented in docs/specs.
 
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index 23bc978..19c216f 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
     depends on ISA_BUS
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index edaaec2..6292839 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -100,6 +100,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..d629639
--- /dev/null
+++ b/hw/misc/pvpanic-pci.c
@@ -0,0 +1,94 @@
+/*
+ * 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 "migration/vmstate.h"
+#include "hw/misc/pvpanic.h"
+#include "qom/object.h"
+#include "hw/pci/pci.h"
+
+OBJECT_DECLARE_SIMPLE_TYPE(PVPanicPCIState, PVPANIC_PCI_DEVICE)
+
+/*
+ * PVPanicPCIState for PCI device
+ */
+typedef struct PVPanicPCIState {
+    PCIDevice dev;
+    PVPanicState pvpanic;
+} PVPanicPCIState;
+
+static const VMStateDescription vmstate_pvpanic_pci = {
+    .name = "pvpanic-pci",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_PCI_DEVICE(dev, PVPanicPCIState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void pvpanic_pci_realizefn(PCIDevice *dev, Error **errp)
+{
+    PVPanicPCIState *s = PVPANIC_PCI_DEVICE(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", PVPanicPCIState, pvpanic.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;
+    dc->vmsd = &vmstate_pvpanic_pci;
+
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+}
+
+static TypeInfo pvpanic_pci_info = {
+    .name          = TYPE_PVPANIC_PCI_DEVICE,
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(PVPanicPCIState),
+    .class_init    = pvpanic_pci_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { 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 abc9dde..ca3c5bb 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_DEVICE "pvpanic"
+#define TYPE_PVPANIC_PCI_DEVICE "pvpanic-pci"
 
 #define PVPANIC_IOPORT_PROP "ioport"
 
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 259f9c9..66db084 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -107,6 +107,7 @@ extern bool pci_available;
 #define PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE 0x000e
 #define PCI_DEVICE_ID_REDHAT_MDPY        0x000f
 #define PCI_DEVICE_ID_REDHAT_NVME        0x0010
+#define PCI_DEVICE_ID_REDHAT_PVPANIC     0x0011
 #define PCI_DEVICE_ID_REDHAT_QXL         0x0100
 
 #define FMT_PCIBUS                      PRIx64
-- 
1.8.3.1

[PATCH v6 3/4] 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 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..8afcde1 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, 1 byte long. Any machine with a PCI bus
+can enable a pvpanic device by adding '-device pvpanic-pci' to the command
+line.
+
 ACPI Interface
 --------------
 
-- 
1.8.3.1

[PATCH v6 4/4] tests/qtest: add a test case for pvpanic-pci

[Mihai Carabas]

Add a test case for pvpanic-pci device. The scenario is the same as pvpanic
ISA device, but is using the PCI bus.

Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
Acked-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
---
 tests/qtest/meson.build        |  1 +
 tests/qtest/pvpanic-pci-test.c | 94 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 95 insertions(+)
 create mode 100644 tests/qtest/pvpanic-pci-test.c

diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 0e85343..7ccdf02 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -34,6 +34,7 @@ qtests_i386 = \
    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_ISA') ? ['pvpanic-test'] : []) +              \
+  (config_all_devices.has_key('CONFIG_PVPANIC_PCI') ? ['pvpanic-pci-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'] : []) +                  \
diff --git a/tests/qtest/pvpanic-pci-test.c b/tests/qtest/pvpanic-pci-test.c
new file mode 100644
index 0000000..24b33c6
--- /dev/null
+++ b/tests/qtest/pvpanic-pci-test.c
@@ -0,0 +1,94 @@
+/*
+ * QTest testcase for PV Panic PCI 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 "libqos/libqtest.h"
+#include "qapi/qmp/qdict.h"
+#include "libqos/pci.h"
+#include "libqos/pci-pc.h"
+#include "hw/pci/pci_regs.h"
+
+static void test_panic_nopause(void)
+{
+    uint8_t val;
+    QDict *response, *data;
+    QTestState *qts;
+    QPCIBus *pcibus;
+    QPCIDevice *dev;
+    QPCIBar bar;
+
+    qts = qtest_init("-device pvpanic-pci,addr=04.0 -action panic=none");
+    pcibus = qpci_new_pc(qts, NULL);
+    dev = qpci_device_find(pcibus, QPCI_DEVFN(0x4, 0x0));
+    qpci_device_enable(dev);
+    bar = qpci_iomap(dev, 0, NULL);
+
+    qpci_memread(dev, bar, 0, &val, sizeof(val));
+    g_assert_cmpuint(val, ==, 3);
+
+    val = 1;
+    qpci_memwrite(dev, bar, 0, &val, sizeof(val));
+
+    response = qtest_qmp_eventwait_ref(qts, "GUEST_PANICKED");
+    g_assert(qdict_haskey(response, "data"));
+    data = qdict_get_qdict(response, "data");
+    g_assert(qdict_haskey(data, "action"));
+    g_assert_cmpstr(qdict_get_str(data, "action"), ==, "run");
+    qobject_unref(response);
+
+    qtest_quit(qts);
+}
+
+static void test_panic(void)
+{
+    uint8_t val;
+    QDict *response, *data;
+    QTestState *qts;
+    QPCIBus *pcibus;
+    QPCIDevice *dev;
+    QPCIBar bar;
+
+    qts = qtest_init("-device pvpanic-pci,addr=04.0 -action panic=pause");
+    pcibus = qpci_new_pc(qts, NULL);
+    dev = qpci_device_find(pcibus, QPCI_DEVFN(0x4, 0x0));
+    qpci_device_enable(dev);
+    bar = qpci_iomap(dev, 0, NULL);
+
+    qpci_memread(dev, bar, 0, &val, sizeof(val));
+    g_assert_cmpuint(val, ==, 3);
+
+    val = 1;
+    qpci_memwrite(dev, bar, 0, &val, sizeof(val));
+
+    response = qtest_qmp_eventwait_ref(qts, "GUEST_PANICKED");
+    g_assert(qdict_haskey(response, "data"));
+    data = qdict_get_qdict(response, "data");
+    g_assert(qdict_haskey(data, "action"));
+    g_assert_cmpstr(qdict_get_str(data, "action"), ==, "pause");
+    qobject_unref(response);
+
+    qtest_quit(qts);
+}
+
+int main(int argc, char **argv)
+{
+    int ret;
+
+    g_test_init(&argc, &argv, NULL);
+    qtest_add_func("/pvpanic-pci/panic", test_panic);
+    qtest_add_func("/pvpanic-pci/panic-nopause", test_panic_nopause);
+
+    ret = g_test_run();
+
+    return ret;
+}
-- 
1.8.3.1

Re: [PATCH v6] Add support for pvpanic pci device

[Peter Maydell]

On Wed, 27 Jan 2021 at 15:46, Mihai Carabas <mihai.carabas@oracle.com> wrote:
>
> This patchset adds support for pvpanic pci device.
>
> v3:
> - patch 1: made pvpanic isa device available only for PC, compile pvpanic-test
>   only when isa device is present
> - patch 2: fixed device id to 0x0011, used OBJECT_DECLARE_TYPE,
>   PVPANIC_PCI_DEVICE, added VMSTATE_PCI_DEVICE, removed INTERFACE_PCIE_DEVICE
> - patch 3: fixed documentation
> - patch 4: add a qtest for pvpanic-pci
>
> v4:
> - added Rb/Ack on patches
> - modify test case to include -action parameter that was recently added and also
>   to be on par with the pvpanic ISA device testing
>
> v5:
> - added subsystem_vendor_id and subsystem_id needed for MS WHQL tests
>
> v6:
> - removed subsystem_vendor_id and subsystem_id as they are filed out by default
>   if empty
> - do not compile pvpanic-pci-test for ppc64 as we our tests do not support that
>   platform

Why doesn't the test work on PPC ?

thanks
-- PMM

Re: [PATCH v6] Add support for pvpanic pci device

[Mihai Carabas]

La 27.01.2021 19:47, Peter Maydell a scris:
> On Wed, 27 Jan 2021 at 15:46, Mihai Carabas <mihai.carabas@oracle.com> wrote:
>> This patchset adds support for pvpanic pci device.
>>
>> v3:
>> - patch 1: made pvpanic isa device available only for PC, compile pvpanic-test
>>    only when isa device is present
>> - patch 2: fixed device id to 0x0011, used OBJECT_DECLARE_TYPE,
>>    PVPANIC_PCI_DEVICE, added VMSTATE_PCI_DEVICE, removed INTERFACE_PCIE_DEVICE
>> - patch 3: fixed documentation
>> - patch 4: add a qtest for pvpanic-pci
>>
>> v4:
>> - added Rb/Ack on patches
>> - modify test case to include -action parameter that was recently added and also
>>    to be on par with the pvpanic ISA device testing
>>
>> v5:
>> - added subsystem_vendor_id and subsystem_id needed for MS WHQL tests
>>
>> v6:
>> - removed subsystem_vendor_id and subsystem_id as they are filed out by default
>>    if empty
>> - do not compile pvpanic-pci-test for ppc64 as we our tests do not support that
>>    platform
> Why doesn't the test work on PPC ?

I have to re-write the test suite in order to use a PCI bus specific to 
pp64 platform. If we want to have test cases for ppc64, I can do this 
with additional logic.

Thank you,
Mihai

Re: [PATCH v6] Add support for pvpanic pci device

[Peter Maydell]

On Wed, 27 Jan 2021 at 15:46, Mihai Carabas <mihai.carabas@oracle.com> wrote:
>
> This patchset adds support for pvpanic pci device.
>



Applied to target-arm.next, thanks.

-- PMM

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

[Philippe Mathieu-Daudé]

Hi,

(old patch merged as commit d097b3dcb62)

On 27/1/21 15:59, Mihai Carabas 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>
> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
> ---
>   docs/specs/pci-ids.txt    |  1 +
>   hw/misc/Kconfig           |  6 +++
>   hw/misc/meson.build       |  1 +
>   hw/misc/pvpanic-pci.c     | 94 +++++++++++++++++++++++++++++++++++++++++++++++
>   include/hw/misc/pvpanic.h |  1 +
>   include/hw/pci/pci.h      |  1 +
>   6 files changed, 104 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..5e407a6 100644
> --- a/docs/specs/pci-ids.txt
> +++ b/docs/specs/pci-ids.txt
> @@ -64,6 +64,7 @@ PCI devices (other than virtio):
>   1b36:000d  PCI xhci usb host adapter
>   1b36:000f  mdpy (mdev sample device), linux/samples/vfio-mdev/mdpy.c
>   1b36:0010  PCIe NVMe device (-device nvme)
> +1b36:0011  PCI PVPanic device (-device pvpanic-pci)
>   
>   All these devices are documented in docs/specs.
>   
> diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
> index 23bc978..19c216f 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
>       depends on ISA_BUS
> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> index edaaec2..6292839 100644
> --- a/hw/misc/meson.build
> +++ b/hw/misc/meson.build
> @@ -100,6 +100,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..d629639
> --- /dev/null
> +++ b/hw/misc/pvpanic-pci.c
> @@ -0,0 +1,94 @@
> +/*
> + * 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 "migration/vmstate.h"
> +#include "hw/misc/pvpanic.h"
> +#include "qom/object.h"
> +#include "hw/pci/pci.h"
> +
> +OBJECT_DECLARE_SIMPLE_TYPE(PVPanicPCIState, PVPANIC_PCI_DEVICE)
> +
> +/*
> + * PVPanicPCIState for PCI device
> + */
> +typedef struct PVPanicPCIState {
> +    PCIDevice dev;
> +    PVPanicState pvpanic;
> +} PVPanicPCIState;
> +
> +static const VMStateDescription vmstate_pvpanic_pci = {
> +    .name = "pvpanic-pci",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_PCI_DEVICE(dev, PVPanicPCIState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void pvpanic_pci_realizefn(PCIDevice *dev, Error **errp)
> +{
> +    PVPanicPCIState *s = PVPANIC_PCI_DEVICE(dev);
> +    PVPanicState *ps = &s->pvpanic;
> +
> +    pvpanic_setup_io(&s->pvpanic, DEVICE(s), 2);

Why registering 2-bytes of I/O when the underlying device is 1-byte
wide? Is this to allow 16-bit I/O instructions?

> +
> +    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &ps->mr);
> +}
> +
> +static Property pvpanic_pci_properties[] = {
> +    DEFINE_PROP_UINT8("events", PVPanicPCIState, pvpanic.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;
> +    dc->vmsd = &vmstate_pvpanic_pci;
> +
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +}
> +
> +static TypeInfo pvpanic_pci_info = {
> +    .name          = TYPE_PVPANIC_PCI_DEVICE,
> +    .parent        = TYPE_PCI_DEVICE,
> +    .instance_size = sizeof(PVPanicPCIState),
> +    .class_init    = pvpanic_pci_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { 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 abc9dde..ca3c5bb 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_DEVICE "pvpanic"
> +#define TYPE_PVPANIC_PCI_DEVICE "pvpanic-pci"
>   
>   #define PVPANIC_IOPORT_PROP "ioport"
>   
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 259f9c9..66db084 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -107,6 +107,7 @@ extern bool pci_available;
>   #define PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE 0x000e
>   #define PCI_DEVICE_ID_REDHAT_MDPY        0x000f
>   #define PCI_DEVICE_ID_REDHAT_NVME        0x0010
> +#define PCI_DEVICE_ID_REDHAT_PVPANIC     0x0011
>   #define PCI_DEVICE_ID_REDHAT_QXL         0x0100
>   
>   #define FMT_PCIBUS                      PRIx64

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

[Gustavo Romero]

Hi Phil,

On 11/18/25 11:16, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> (old patch merged as commit d097b3dcb62)
> 
> On 27/1/21 15:59, Mihai Carabas 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>
>> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
>> ---
>>   docs/specs/pci-ids.txt    |  1 +
>>   hw/misc/Kconfig           |  6 +++
>>   hw/misc/meson.build       |  1 +
>>   hw/misc/pvpanic-pci.c     | 94 +++++++++++++++++++++++++++++++++++++++++++++++
>>   include/hw/misc/pvpanic.h |  1 +
>>   include/hw/pci/pci.h      |  1 +
>>   6 files changed, 104 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..5e407a6 100644
>> --- a/docs/specs/pci-ids.txt
>> +++ b/docs/specs/pci-ids.txt
>> @@ -64,6 +64,7 @@ PCI devices (other than virtio):
>>   1b36:000d  PCI xhci usb host adapter
>>   1b36:000f  mdpy (mdev sample device), linux/samples/vfio-mdev/mdpy.c
>>   1b36:0010  PCIe NVMe device (-device nvme)
>> +1b36:0011  PCI PVPanic device (-device pvpanic-pci)
>>   All these devices are documented in docs/specs.
>> diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
>> index 23bc978..19c216f 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
>>       depends on ISA_BUS
>> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
>> index edaaec2..6292839 100644
>> --- a/hw/misc/meson.build
>> +++ b/hw/misc/meson.build
>> @@ -100,6 +100,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..d629639
>> --- /dev/null
>> +++ b/hw/misc/pvpanic-pci.c
>> @@ -0,0 +1,94 @@
>> +/*
>> + * 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 "migration/vmstate.h"
>> +#include "hw/misc/pvpanic.h"
>> +#include "qom/object.h"
>> +#include "hw/pci/pci.h"
>> +
>> +OBJECT_DECLARE_SIMPLE_TYPE(PVPanicPCIState, PVPANIC_PCI_DEVICE)
>> +
>> +/*
>> + * PVPanicPCIState for PCI device
>> + */
>> +typedef struct PVPanicPCIState {
>> +    PCIDevice dev;
>> +    PVPanicState pvpanic;
>> +} PVPanicPCIState;
>> +
>> +static const VMStateDescription vmstate_pvpanic_pci = {
>> +    .name = "pvpanic-pci",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_PCI_DEVICE(dev, PVPanicPCIState),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static void pvpanic_pci_realizefn(PCIDevice *dev, Error **errp)
>> +{
>> +    PVPanicPCIState *s = PVPANIC_PCI_DEVICE(dev);
>> +    PVPanicState *ps = &s->pvpanic;
>> +
>> +    pvpanic_setup_io(&s->pvpanic, DEVICE(s), 2);
> 
> Why registering 2-bytes of I/O when the underlying device is 1-byte
> wide? Is this to allow 16-bit I/O instructions?

TL;DR: I think that 2 should actually be 16 :)

First, IMO, pvpanic_setup_io() should be called pvpanic_setup_mmio() because
the registered memory region in question is of type PCI_BASE_ADDRESS_SPACE_MEMORY,
not of type PCI_BASE_ADDRESS_SPACE_IO, hence the BAR associated to
this memory region (ps->mr) will map a MMIO region, not an I/O region.

But I think I see why it looks confusing to me: QEMU puts both a Memory Space
and an I/O Space (PCI terms here) into pci_dev->io_regions[] (which is ok).
The former defines a MMIO region and the latter an I/O (x86 old-style) region.

I'm just saying that in case you were thinking of inb/outb/insw/outsw &
friends when you said "to allow 16-bit I/O instructions", which are mean
to work with PCI_BASE_ADDRESS_SPACE_IO, not with PCI_BASE_ADDRESS_SPACE_MEMORY
regions, where normal CPU load/store instructions (mov, ldr, str, ld, std, etc.)
should be used instead.

For such normal a CPU loads/stores the granularity would be, in general, 1 byte,
so CPU can load/store 1 byte or more on most PCI devices (in MMIO regions), hence:

       .impl = {
           .min_access_size = 1,
           .max_access_size = 1,
       },


looks correct to me.

That said, I understand that pvpanic_setup_io is defining a 2 bytes
PCI_BASE_ADDRESS_SPACE_MEMORY (Memory Space) and the minimum size of this
region should actually be 16 bytes, not 2 bytes.

If you take a look at the PCI spec 3.0, in section 6.2.5.1, "Address Maps",
it says, about "Base Address Register for Memory":

"A 32-bit register can be implemented to support a single memory size that
is a power of 2 from 16 bytes to 2 GB."

So the minimum size of a Memory Space is 16 bytes, because bits from 0 to 3
in the BAR are not used for address decoding purposes (they define the type
of the space, number of bits in the address space, etc.).

Also, in pci_register_bar() the size is used to set the wmask for the
BAR register:

wmask = ~(size - 1);

and with size = 2, if my math is right, gives 0xfffffffe, which is invalid
(it should be 0xfffffff0 for the minimum size: bits from 0 to 3 should
be zeroed (falgs) and bit 4 and upwards should be one. The first bit set as
1 starting from the least significant bit determines the size of the MMIO region,
and in 0xfffffff0 bit 4 is the first one that is 1, which kind tells us the
minimum size should really be 16 bytes for a MMIO region).

So that's my 2 cents on it :)


Cheers,
Gustavo

>> +
>> +    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &ps->mr);
>> +}
>> +
>> +static Property pvpanic_pci_properties[] = {
>> +    DEFINE_PROP_UINT8("events", PVPanicPCIState, pvpanic.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;
>> +    dc->vmsd = &vmstate_pvpanic_pci;
>> +
>> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>> +}
>> +
>> +static TypeInfo pvpanic_pci_info = {
>> +    .name          = TYPE_PVPANIC_PCI_DEVICE,
>> +    .parent        = TYPE_PCI_DEVICE,
>> +    .instance_size = sizeof(PVPanicPCIState),
>> +    .class_init    = pvpanic_pci_class_init,
>> +    .interfaces = (InterfaceInfo[]) {
>> +        { 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 abc9dde..ca3c5bb 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_DEVICE "pvpanic"
>> +#define TYPE_PVPANIC_PCI_DEVICE "pvpanic-pci"
>>   #define PVPANIC_IOPORT_PROP "ioport"
>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>> index 259f9c9..66db084 100644
>> --- a/include/hw/pci/pci.h
>> +++ b/include/hw/pci/pci.h
>> @@ -107,6 +107,7 @@ extern bool pci_available;
>>   #define PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE 0x000e
>>   #define PCI_DEVICE_ID_REDHAT_MDPY        0x000f
>>   #define PCI_DEVICE_ID_REDHAT_NVME        0x0010
>> +#define PCI_DEVICE_ID_REDHAT_PVPANIC     0x0011
>>   #define PCI_DEVICE_ID_REDHAT_QXL         0x0100
>>   #define FMT_PCIBUS                      PRIx64
> 
>