[Qemu-devel] [PATCH V10 0/9] add pvpanic mmio support

[Qemu-devel] [PATCH V10 0/9] add pvpanic mmio support

[Peng Hao]

The first patches are simple cleanups:
     - patch 1 move the pvpanic device with the 'ocmmon objects' so we compile
               it once for the x86/arm/aarch64 archs,
     - patch 2 simply renames ISA fields/definitions to generic ones.

     Then instead of add/use the MMIO pvpanic device in the virt machine in an
     unique patch, I split it in two distinct patches:
     - patch 3 uses Peng Hao's work, but add the MMIO interface to the existing
        device (no logical change).
     - patch 4 is Peng Hao's work in the virt machine (no logical change).
     - patch 5 add pvpanic device in acpi table in virt machine
     v2 from Peng Hao is:
     https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg03433.html

v3 --> v4
     patch 1,2 no modification.
     patch 3, add TYPE_PANIC_MMIO for distinguishing different bus device,
              virt + isa_pvpanic will abnormally terminate virtual machine.
     patch 4, "pvpanic,mmio" --> "qemu,pvpanic-mmio".
     patch 5, newly added.

v4 --> v5
     patch 1,2 no modification.
     patch 3 delete PvpanicCommonState structure.
     patch 4 VIRT_PVPANIC_MMIO --> VIRT_PVPANIC
             correct VIRT_PVPANIC's overlap start address
     patch 5 no modification.

v5 --> v6
     add document.

v6 --> v7
     patch 5 modify device name from "PANC" to "PEVT".
     patch 6 modify document description.

v7 --> v8
     add configure interface for pvpanic-mmio

v8 --> v9
     revert "moving structure definition to header file"
     because of compile error in x86.

v9 --> v10
     Modify document.
     Repair missing header files.
     
the kernel part of the series:
     https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git/log/?h=char-misc-testing
     misc/pvpanic: remove a redundant comma
     misc/pvpanic: convert to SPDX license tags
     misc/pvpanic: change header file sort style
     misc/pvpanic: remove unnecessary header file
     misc/pvpanic : break dependency on ACPI
     misc/pvpanic : grouping ACPI related stuff
     misc/pvpanic: add support to get pvpanic device info FDT
     dt-bindings: misc/pvpanic: add document for pvpanic-mmio
     misc/pvpanic: add MMIO support
     misc/pvpanic: simplify the code using acpi_dev_resource_io
     pvpanic: move pvpanic to misc as common driver  

Philippe Mathieu-Daudé (2):
  hw/misc/pvpanic: Build the pvpanic device in $(common-obj)
  hw/misc/pvpanic: Cosmetic renaming

Peng Hao (7):
  hw/misc/pvpanic: Add the MMIO interface
  hw/arm/virt: Use the pvpanic device
  hw/arm/virt: add pvpanic device in virt acpi table
  hw/misc/pvpanic: add configure query interface
  hw/misc/pvpanic: preparing for adding configure interface
  hw/misc/pvpanic: realize the configure interface
  pvpanic : update pvpanic document

 default-configs/arm-softmmu.mak |  1 +
 docs/specs/pvpanic.txt          | 15 ++++++-
 hw/arm/sysbus-fdt.c             |  2 +
 hw/arm/virt-acpi-build.c        | 17 ++++++++
 hw/arm/virt.c                   | 23 ++++++++++-
 hw/misc/Makefile.objs           |  2 +-
 hw/misc/pvpanic.c               | 87 +++++++++++++++++++++++++++++++++--------
 include/hw/arm/virt.h           |  1 +
 include/hw/misc/pvpanic.h       |  6 +++
 9 files changed, 134 insertions(+), 20 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH V10 1/9] hw/misc/pvpanic: Build the pvpanic device in $(common-obj)

[Peng Hao]

From: Philippe Mathieu-Daudé <philmd@redhat.com>

The 'pvpanic' ISA device can be use by any machine with an ISA bus.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 hw/misc/Makefile.objs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 680350b..c387ce4 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -8,6 +8,7 @@ common-obj-$(CONFIG_ISA_TESTDEV) += pc-testdev.o
 common-obj-$(CONFIG_PCI_TESTDEV) += pci-testdev.o
 common-obj-$(CONFIG_EDU) += edu.o
 common-obj-$(CONFIG_PCA9552) += pca9552.o
+common-obj-$(CONFIG_PVPANIC) += pvpanic.o
 
 common-obj-y += unimp.o
 common-obj-$(CONFIG_FW_CFG_DMA) += vmcoreinfo.o
@@ -70,7 +71,6 @@ obj-$(CONFIG_IOTKIT_SECCTL) += iotkit-secctl.o
 obj-$(CONFIG_IOTKIT_SYSCTL) += iotkit-sysctl.o
 obj-$(CONFIG_IOTKIT_SYSINFO) += iotkit-sysinfo.o
 
-obj-$(CONFIG_PVPANIC) += pvpanic.o
 obj-$(CONFIG_AUX) += auxbus.o
 obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o aspeed_sdmc.o
 obj-$(CONFIG_MSF2) += msf2-sysreg.o
-- 
1.8.3.1

[Qemu-devel] [PATCH V10 2/9] hw/misc/pvpanic: Cosmetic renaming

[Peng Hao]

From: Philippe Mathieu-Daudé <philmd@redhat.com>

To ease the MMIO device addition in the next patch, rename:
- ISA_PVPANIC_DEVICE -> PVPANIC_ISA_DEVICE.
- MemoryRegion io -> mr.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 hw/misc/pvpanic.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index 9d8961b..0f23a67 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -25,8 +25,8 @@
 /* The pv event value */
 #define PVPANIC_PANICKED        (1 << PVPANIC_F_PANICKED)
 
-#define ISA_PVPANIC_DEVICE(obj)    \
-    OBJECT_CHECK(PVPanicState, (obj), TYPE_PVPANIC)
+#define PVPANIC_ISA_DEVICE(obj)    \
+    OBJECT_CHECK(PVPanicISAState, (obj), TYPE_PVPANIC)
 
 static void handle_event(int event)
 {
@@ -45,12 +45,16 @@ static void handle_event(int event)
 
 #include "hw/isa/isa.h"
 
-typedef struct PVPanicState {
+/* PVPanicISAState for ISA device and
+ * use ioport.
+ */
+typedef struct PVPanicISAState {
     ISADevice parent_obj;
-
-    MemoryRegion io;
+    /*< private>*/
     uint16_t ioport;
-} PVPanicState;
+    /*<public>*/
+    MemoryRegion mr;
+} PVPanicISAState;
 
 /* return supported events on read */
 static uint64_t pvpanic_ioport_read(void *opaque, hwaddr addr, unsigned size)
@@ -75,15 +79,15 @@ static const MemoryRegionOps pvpanic_ops = {
 
 static void pvpanic_isa_initfn(Object *obj)
 {
-    PVPanicState *s = ISA_PVPANIC_DEVICE(obj);
+    PVPanicISAState *s = PVPANIC_ISA_DEVICE(obj);
 
-    memory_region_init_io(&s->io, OBJECT(s), &pvpanic_ops, s, "pvpanic", 1);
+    memory_region_init_io(&s->mr, 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);
+    PVPanicISAState *s = PVPANIC_ISA_DEVICE(dev);
     FWCfgState *fw_cfg = fw_cfg_find();
     uint16_t *pvpanic_port;
 
@@ -96,11 +100,11 @@ static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp)
     fw_cfg_add_file(fw_cfg, "etc/pvpanic-port", pvpanic_port,
                     sizeof(*pvpanic_port));
 
-    isa_register_ioport(d, &s->io, s->ioport);
+    isa_register_ioport(d, &s->mr, s->ioport);
 }
 
 static Property pvpanic_isa_properties[] = {
-    DEFINE_PROP_UINT16(PVPANIC_IOPORT_PROP, PVPanicState, ioport, 0x505),
+    DEFINE_PROP_UINT16(PVPANIC_IOPORT_PROP, PVPanicISAState, ioport, 0x505),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -116,7 +120,7 @@ static void pvpanic_isa_class_init(ObjectClass *klass, void *data)
 static TypeInfo pvpanic_isa_info = {
     .name          = TYPE_PVPANIC,
     .parent        = TYPE_ISA_DEVICE,
-    .instance_size = sizeof(PVPanicState),
+    .instance_size = sizeof(PVPanicISAState),
     .instance_init = pvpanic_isa_initfn,
     .class_init    = pvpanic_isa_class_init,
 };
-- 
1.8.3.1

[Qemu-devel] [PATCH V10 3/9] hw/misc/pvpanic: Add the MMIO interface

[Peng Hao]

Add pvpanic new type "TYPE_PVPANIC_MMIO"

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 hw/misc/pvpanic.c         | 50 +++++++++++++++++++++++++++++++++++++++++++----
 include/hw/misc/pvpanic.h |  1 +
 2 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index 0f23a67..c9382a8 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -2,10 +2,12 @@
  * QEMU simulated pvpanic device.
  *
  * Copyright Fujitsu, Corp. 2013
+ * Copyright (c) 2018 ZTE Ltd.
  *
  * Authors:
  *     Wen Congyang <wency@cn.fujitsu.com>
  *     Hu Tao <hutao@cn.fujitsu.com>
+ *     Peng Hao <peng.hao2@zte.com.cn>
  *
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.
@@ -28,6 +30,9 @@
 #define PVPANIC_ISA_DEVICE(obj)    \
     OBJECT_CHECK(PVPanicISAState, (obj), TYPE_PVPANIC)
 
+#define PVPANIC_MMIO_DEVICE(obj)    \
+    OBJECT_CHECK(PVPanicMMIOState, (obj), TYPE_PVPANIC_MMIO)
+
 static void handle_event(int event)
 {
     static bool logged;
@@ -56,21 +61,32 @@ typedef struct PVPanicISAState {
     MemoryRegion mr;
 } PVPanicISAState;
 
+/* PVPanicMMIOState for sysbus device and
+ * use mmio.
+ */
+typedef struct PVPanicMMIOState {
+    SysBusDevice parent_obj;
+    /*<private>*/
+
+    /* public */
+    MemoryRegion mr;
+} PVPanicMMIOState;
+
 /* 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)
 {
     return PVPANIC_PANICKED;
 }
 
-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,
@@ -125,9 +141,35 @@ static TypeInfo pvpanic_isa_info = {
     .class_init    = pvpanic_isa_class_init,
 };
 
+static void pvpanic_mmio_initfn(Object *obj)
+{
+    PVPanicMMIOState *s = PVPANIC_MMIO_DEVICE(obj);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+
+    memory_region_init_io(&s->mr, OBJECT(s), &pvpanic_ops, s,
+                          TYPE_PVPANIC_MMIO, 2);
+    sysbus_init_mmio(sbd, &s->mr);
+}
+
+static void pvpanic_mmio_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+}
+
+static TypeInfo pvpanic_mmio_info = {
+    .name          = TYPE_PVPANIC_MMIO,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(PVPanicMMIOState),
+    .instance_init = pvpanic_mmio_initfn,
+    .class_init    = pvpanic_mmio_class_init,
+};
+
 static void pvpanic_register_types(void)
 {
     type_register_static(&pvpanic_isa_info);
+    type_register_static(&pvpanic_mmio_info);
 }
 
 type_init(pvpanic_register_types)
diff --git a/include/hw/misc/pvpanic.h b/include/hw/misc/pvpanic.h
index 1ee071a..f1a05b2 100644
--- a/include/hw/misc/pvpanic.h
+++ b/include/hw/misc/pvpanic.h
@@ -15,6 +15,7 @@
 #define HW_MISC_PVPANIC_H
 
 #define TYPE_PVPANIC "pvpanic"
+#define TYPE_PVPANIC_MMIO "pvpanic-mmio"
 
 #define PVPANIC_IOPORT_PROP "ioport"
 
-- 
1.8.3.1

[Qemu-devel] [PATCH V10 4/9] hw/arm/virt: Use the pvpanic device

[Peng Hao]

Add pvpanic device in arm virt machine.

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 default-configs/arm-softmmu.mak |  1 +
 hw/arm/virt.c                   | 21 +++++++++++++++++++++
 include/hw/arm/virt.h           |  1 +
 3 files changed, 23 insertions(+)

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index 2420491..50345df 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -159,3 +159,4 @@ CONFIG_PCI_DESIGNWARE=y
 CONFIG_STRONGARM=y
 CONFIG_HIGHBANK=y
 CONFIG_MUSICPAL=y
+CONFIG_PVPANIC=y
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index a2b8d8f..f2cb5de 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -143,6 +143,7 @@ static const MemMapEntry a15memmap[] = {
     [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
     [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
     [VIRT_SMMU] =               { 0x09050000, 0x00020000 },
+    [VIRT_PVPANIC] =            { 0x09070000, 0x00000002 },
     [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
@@ -190,6 +191,24 @@ static bool cpu_type_valid(const char *cpu)
     return false;
 }
 
+static void create_pvpanic_device(const VirtMachineState *vms)
+{
+     char *nodename;
+     hwaddr base = vms->memmap[VIRT_PVPANIC].base;
+     hwaddr size = vms->memmap[VIRT_PVPANIC].size;
+
+     sysbus_create_simple(TYPE_PVPANIC_MMIO, base, NULL);
+
+     nodename = g_strdup_printf("/pvpanic-mmio@%" PRIx64, base);
+     qemu_fdt_add_subnode(vms->fdt, nodename);
+     qemu_fdt_setprop_string(vms->fdt, nodename,
+                             "compatible", "qemu,pvpanic-mmio");
+     qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
+                                  2, base, 2, size);
+
+     g_free(nodename);
+}
+
 static void create_fdt(VirtMachineState *vms)
 {
     void *fdt = create_device_tree(&vms->fdt_size);
@@ -1531,6 +1550,8 @@ static void machvirt_init(MachineState *machine)
 
     create_flash(vms, sysmem, secure_sysmem ? secure_sysmem : sysmem);
 
+    create_pvpanic_device(vms);
+
     create_gic(vms, pic);
 
     fdt_add_pmu_nodes(vms);
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 4cc57a7..937c124 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -66,6 +66,7 @@ enum {
     VIRT_GIC_REDIST,
     VIRT_GIC_REDIST2,
     VIRT_SMMU,
+    VIRT_PVPANIC,
     VIRT_UART,
     VIRT_MMIO,
     VIRT_RTC,
-- 
1.8.3.1

[Qemu-devel] [PATCH V10 5/9] hw/arm/virt: add pvpanic device in virt acpi table

[Peng Hao]

Add pvpanic device in virt acpi table, so when kenrel command line
uses acpi=force, kernel can get info from acpi table.

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 hw/arm/virt-acpi-build.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 5785fb6..2adba60 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -84,6 +84,20 @@ static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap,
     aml_append(scope, dev);
 }
 
+static void acpi_dsdt_add_pvpanic(Aml *scope, const MemMapEntry *pvpanic_memmap)
+{
+    Aml *dev = aml_device("PEVT");
+    aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0001")));
+    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
+
+    Aml *crs = aml_resource_template();
+    aml_append(crs, aml_memory32_fixed(pvpanic_memmap->base,
+                                       pvpanic_memmap->size, AML_READ_WRITE));
+
+    aml_append(dev, aml_name_decl("_CRS", crs));
+    aml_append(scope, dev);
+}
+
 static void acpi_dsdt_add_fw_cfg(Aml *scope, const MemMapEntry *fw_cfg_memmap)
 {
     Aml *dev = aml_device("FWCF");
@@ -771,6 +785,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
                        (irqmap[VIRT_UART] + ARM_SPI_BASE));
     acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
+    acpi_dsdt_add_pvpanic(scope, &memmap[VIRT_PVPANIC]);
     acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
     acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
                     (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
-- 
1.8.3.1

[Qemu-devel] [PATCH V10 6/9] hw/misc/pvpanic: add configure query interface

[Peng Hao]

Add configure query interface for pvpanic-mmio.

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 include/hw/misc/pvpanic.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/hw/misc/pvpanic.h b/include/hw/misc/pvpanic.h
index f1a05b2..dc042cf 100644
--- a/include/hw/misc/pvpanic.h
+++ b/include/hw/misc/pvpanic.h
@@ -2,10 +2,12 @@
  * QEMU simulated pvpanic device.
  *
  * Copyright Fujitsu, Corp. 2013
+ * Copyright ZTE Ltd. 2018
  *
  * Authors:
  *     Wen Congyang <wency@cn.fujitsu.com>
  *     Hu Tao <hutao@cn.fujitsu.com>
+ *     Peng Hao <peng.hao2@zte.com.cn>
  *
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.
@@ -28,4 +30,9 @@ static inline uint16_t pvpanic_port(void)
     return object_property_get_uint(o, PVPANIC_IOPORT_PROP, NULL);
 }
 
+static inline Object *pvpanic_mmio(void)
+{
+    return object_resolve_path_type("", TYPE_PVPANIC_MMIO, NULL);
+}
+
 #endif
-- 
1.8.3.1

[Qemu-devel] [PATCH V10 7/9] hw/misc/pvpanic: preparing for adding configure interface

[Peng Hao]

Prepare for pvpanic-mmio configure interface.

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 hw/arm/sysbus-fdt.c |  2 ++
 hw/arm/virt.c       |  2 ++
 hw/misc/pvpanic.c   | 11 +++++++++--
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index ad698d4..34577f3 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -38,6 +38,7 @@
 #include "hw/vfio/vfio-amd-xgbe.h"
 #include "hw/display/ramfb.h"
 #include "hw/arm/fdt.h"
+#include "hw/misc/pvpanic.h"
 
 /*
  * internal struct that contains the information to create dynamic
@@ -459,6 +460,7 @@ static const BindingEntry bindings[] = {
     VFIO_PLATFORM_BINDING("amd,xgbe-seattle-v1a", add_amd_xgbe_fdt_node),
 #endif
     TYPE_BINDING(TYPE_RAMFB_DEVICE, no_fdt_node),
+    TYPE_BINDING(TYPE_PVPANIC_MMIO, no_fdt_node),
     TYPE_BINDING("", NULL), /* last element */
 };
 
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index f2cb5de..1fd5941 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -59,6 +59,7 @@
 #include "qapi/visitor.h"
 #include "standard-headers/linux/input.h"
 #include "hw/arm/smmuv3.h"
+#include "hw/misc/pvpanic.h"
 
 #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
     static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
@@ -1783,6 +1784,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_CALXEDA_XGMAC);
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_AMD_XGBE);
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
+    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PVPANIC_MMIO);
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_PLATFORM);
     mc->block_default_type = IF_VIRTIO;
     mc->no_cdrom = 1;
diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index c9382a8..b6b5c89 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -67,7 +67,7 @@ typedef struct PVPanicISAState {
 typedef struct PVPanicMMIOState {
     SysBusDevice parent_obj;
     /*<private>*/
-
+    uint32_t base;
     /* public */
     MemoryRegion mr;
 } PVPanicMMIOState;
@@ -151,10 +151,17 @@ static void pvpanic_mmio_initfn(Object *obj)
     sysbus_init_mmio(sbd, &s->mr);
 }
 
+static Property pvpanic_mmio_properties[] = {
+    DEFINE_PROP_UINT32("mmio", PVPanicMMIOState, base, 0x09070000),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void pvpanic_mmio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-
+
+    dc->user_creatable = true;
+    dc->props = pvpanic_mmio_properties;
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH V10 8/9] hw/misc/pvpanic: realize the configure interface

[Peng Hao]

Add configure interface for pvpanic-mmio. In qemu command line
use -device pvpanic-mmio to enable the device.

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 hw/arm/virt-acpi-build.c | 5 ++++-
 hw/arm/virt.c            | 7 +++----
 hw/misc/pvpanic.c        | 1 +
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 2adba60..d29d229 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -45,6 +45,7 @@
 #include "hw/arm/virt.h"
 #include "sysemu/numa.h"
 #include "kvm_arm.h"
+#include "hw/misc/pvpanic.h"
 
 #define ARM_SPI_BASE 32
 #define ACPI_POWER_BUTTON_DEVICE "PWRB"
@@ -785,7 +786,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
                        (irqmap[VIRT_UART] + ARM_SPI_BASE));
     acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
-    acpi_dsdt_add_pvpanic(scope, &memmap[VIRT_PVPANIC]);
+    if (pvpanic_mmio()) {
+        acpi_dsdt_add_pvpanic(scope, &memmap[VIRT_PVPANIC]);
+    }
     acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
     acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
                     (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 1fd5941..9cddf36 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -198,8 +198,6 @@ static void create_pvpanic_device(const VirtMachineState *vms)
      hwaddr base = vms->memmap[VIRT_PVPANIC].base;
      hwaddr size = vms->memmap[VIRT_PVPANIC].size;
 
-     sysbus_create_simple(TYPE_PVPANIC_MMIO, base, NULL);
-
      nodename = g_strdup_printf("/pvpanic-mmio@%" PRIx64, base);
      qemu_fdt_add_subnode(vms->fdt, nodename);
      qemu_fdt_setprop_string(vms->fdt, nodename,
@@ -1330,6 +1328,9 @@ void virt_machine_done(Notifier *notifier, void *data)
     struct arm_boot_info *info = &vms->bootinfo;
     AddressSpace *as = arm_boot_address_space(cpu, info);
 
+    if (pvpanic_mmio()) {
+        create_pvpanic_device(vms);
+    }
     /*
      * If the user provided a dtb, we assume the dynamic sysbus nodes
      * already are integrated there. This corresponds to a use case where
@@ -1551,8 +1552,6 @@ static void machvirt_init(MachineState *machine)
 
     create_flash(vms, sysmem, secure_sysmem ? secure_sysmem : sysmem);
 
-    create_pvpanic_device(vms);
-
     create_gic(vms, pic);
 
     fdt_add_pmu_nodes(vms);
diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index b6b5c89..6c4a79f 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -149,6 +149,7 @@ static void pvpanic_mmio_initfn(Object *obj)
     memory_region_init_io(&s->mr, OBJECT(s), &pvpanic_ops, s,
                           TYPE_PVPANIC_MMIO, 2);
     sysbus_init_mmio(sbd, &s->mr);
+    sysbus_mmio_map(sbd, 0, s->base);
 }
 
 static Property pvpanic_mmio_properties[] = {
-- 
1.8.3.1

[Qemu-devel] [PATCH V10 9/9] pvpanic : update pvpanic document

[Peng Hao]

Add mmio support info in docs/specs/pvpanic.txt.

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 docs/specs/pvpanic.txt | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/docs/specs/pvpanic.txt b/docs/specs/pvpanic.txt
index c7bbacc..b1beea3 100644
--- a/docs/specs/pvpanic.txt
+++ b/docs/specs/pvpanic.txt
@@ -1,14 +1,17 @@
 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
+pvpanic device is a simulated ISA/SYSBUS 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.
 
+The pvpanic device can be implemented as an ISA device (using IOPORT), 
+or, since qemu 4.0, as a SYSBUS device (using MMIO).
+
 ISA Interface
 -------------
 
@@ -19,6 +22,13 @@ Software should set only bits both itself and the device recognize.
 Currently, only bit 0 is recognized, setting it indicates a guest panic
 has happened.
 
+SYSBUS Interface
+----------------
+
+The SYSBUS interface is similar to the ISA interface except that it uses
+MMIO. For example, the arm virt machine could put the pvpanic device at
+[0x9070000, 0x9070001] and currently only the first byte is used.
+
 ACPI Interface
 --------------
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH V10 2/9] hw/misc/pvpanic: Cosmetic renaming

[Peter Maydell]

On Wed, 28 Nov 2018 at 03:49, Peng Hao <peng.hao2@zte.com.cn> wrote:
>
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> To ease the MMIO device addition in the next patch, rename:
> - ISA_PVPANIC_DEVICE -> PVPANIC_ISA_DEVICE.
> - MemoryRegion io -> mr.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH V10 3/9] hw/misc/pvpanic: Add the MMIO interface

[Peter Maydell]

On Wed, 28 Nov 2018 at 03:50, Peng Hao <peng.hao2@zte.com.cn> wrote:
>
> Add pvpanic new type "TYPE_PVPANIC_MMIO"
>
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> ---
>  hw/misc/pvpanic.c         | 50 +++++++++++++++++++++++++++++++++++++++++++----
>  include/hw/misc/pvpanic.h |  1 +
>  2 files changed, 47 insertions(+), 4 deletions(-)
>


> +static void pvpanic_mmio_initfn(Object *obj)
> +{
> +    PVPanicMMIOState *s = PVPANIC_MMIO_DEVICE(obj);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +
> +    memory_region_init_io(&s->mr, OBJECT(s), &pvpanic_ops, s,
> +                          TYPE_PVPANIC_MMIO, 2);

Why did you choose to make the memory region 2 bytes wide?
The ISA i/o port interface seems to be a 1 byte wide one.

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH V10 4/9] hw/arm/virt: Use the pvpanic device

[Peter Maydell]

On Wed, 28 Nov 2018 at 03:50, Peng Hao <peng.hao2@zte.com.cn> wrote:
>
> Add pvpanic device in arm virt machine.
>
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> ---
>  default-configs/arm-softmmu.mak |  1 +
>  hw/arm/virt.c                   | 21 +++++++++++++++++++++
>  include/hw/arm/virt.h           |  1 +
>  3 files changed, 23 insertions(+)
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index 2420491..50345df 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -159,3 +159,4 @@ CONFIG_PCI_DESIGNWARE=y
>  CONFIG_STRONGARM=y
>  CONFIG_HIGHBANK=y
>  CONFIG_MUSICPAL=y
> +CONFIG_PVPANIC=y
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index a2b8d8f..f2cb5de 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -143,6 +143,7 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
>      [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
>      [VIRT_SMMU] =               { 0x09050000, 0x00020000 },
> +    [VIRT_PVPANIC] =            { 0x09070000, 0x00000002 },
>      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> @@ -190,6 +191,24 @@ static bool cpu_type_valid(const char *cpu)
>      return false;
>  }
>
> +static void create_pvpanic_device(const VirtMachineState *vms)
> +{
> +     char *nodename;
> +     hwaddr base = vms->memmap[VIRT_PVPANIC].base;
> +     hwaddr size = vms->memmap[VIRT_PVPANIC].size;
> +
> +     sysbus_create_simple(TYPE_PVPANIC_MMIO, base, NULL);
> +
> +     nodename = g_strdup_printf("/pvpanic-mmio@%" PRIx64, base);
> +     qemu_fdt_add_subnode(vms->fdt, nodename);
> +     qemu_fdt_setprop_string(vms->fdt, nodename,
> +                             "compatible", "qemu,pvpanic-mmio");
> +     qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
> +                                  2, base, 2, size);
> +
> +     g_free(nodename);
> +}

Just a note that the binding documentation for this still doesn't
seem to be in the mainline kernel git repo yet; I won't take
the patchset until it is.

thanks
-- PMM

Re: [Qemu-devel] [PATCH V10 5/9] hw/arm/virt: add pvpanic device in virt acpi table

[Peter Maydell]

On Wed, 28 Nov 2018 at 03:50, Peng Hao <peng.hao2@zte.com.cn> wrote:
>
> Add pvpanic device in virt acpi table, so when kenrel command line

typo: "kernel". I'll leave it to the ACPI experts to review the
meat of this patch.

> uses acpi=force, kernel can get info from acpi table.
>
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> ---
>  hw/arm/virt-acpi-build.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 5785fb6..2adba60 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -84,6 +84,20 @@ static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap,
>      aml_append(scope, dev);
>  }
>
> +static void acpi_dsdt_add_pvpanic(Aml *scope, const MemMapEntry *pvpanic_memmap)
> +{
> +    Aml *dev = aml_device("PEVT");
> +    aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0001")));
> +    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> +
> +    Aml *crs = aml_resource_template();
> +    aml_append(crs, aml_memory32_fixed(pvpanic_memmap->base,
> +                                       pvpanic_memmap->size, AML_READ_WRITE));
> +
> +    aml_append(dev, aml_name_decl("_CRS", crs));
> +    aml_append(scope, dev);
> +}
> +
>  static void acpi_dsdt_add_fw_cfg(Aml *scope, const MemMapEntry *fw_cfg_memmap)
>  {
>      Aml *dev = aml_device("FWCF");
> @@ -771,6 +785,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
>      acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> +    acpi_dsdt_add_pvpanic(scope, &memmap[VIRT_PVPANIC]);
>      acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
>      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
>                      (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
> --
> 1.8.3.1
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH V10 8/9] hw/misc/pvpanic: realize the configure interface

[Peter Maydell]

On Wed, 28 Nov 2018 at 03:50, Peng Hao <peng.hao2@zte.com.cn> wrote:
>
> Add configure interface for pvpanic-mmio. In qemu command line
> use -device pvpanic-mmio to enable the device.
>
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> ---
>  hw/arm/virt-acpi-build.c | 5 ++++-
>  hw/arm/virt.c            | 7 +++----
>  hw/misc/pvpanic.c        | 1 +
>  3 files changed, 8 insertions(+), 5 deletions(-)


> @@ -1330,6 +1328,9 @@ void virt_machine_done(Notifier *notifier, void *data)
>      struct arm_boot_info *info = &vms->bootinfo;
>      AddressSpace *as = arm_boot_address_space(cpu, info);
>
> +    if (pvpanic_mmio()) {
> +        create_pvpanic_device(vms);
> +    }
>      /*
>       * If the user provided a dtb, we assume the dynamic sysbus nodes
>       * already are integrated there. This corresponds to a use case where
> @@ -1551,8 +1552,6 @@ static void machvirt_init(MachineState *machine)
>
>      create_flash(vms, sysmem, secure_sysmem ? secure_sysmem : sysmem);
>
> -    create_pvpanic_device(vms);
> -

Please don't make changes in on patch of a patchset
and then undo them in a later patch. It makes the
patchset harder to read and review.

thanks
-- PMM

Re: [Qemu-devel] [PATCH V10 7/9] hw/misc/pvpanic: preparing for adding configure interface

[Peter Maydell]

On Wed, 28 Nov 2018 at 03:50, Peng Hao <peng.hao2@zte.com.cn> wrote:
>
> Prepare for pvpanic-mmio configure interface.
>
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> ---
>  hw/arm/sysbus-fdt.c |  2 ++
>  hw/arm/virt.c       |  2 ++
>  hw/misc/pvpanic.c   | 11 +++++++++--
>  3 files changed, 13 insertions(+), 2 deletions(-)

This looks rather odd. The sysbus-fdt.c code is for
handling devices that we must pass through from the
host system (including pulling in device tree fragments
from the host). The pvpanic device isn't a host pass
through device so I don't think it should be touching
that code at all. It's just a simple MMIO device.

I suspect the reason you've done this is that you're
trying to get "-device pvpanic" to work on the command
line. I recommend not trying to get that to work.
MMIO devices are not pluggable devices, and -device
is not expected to work with them.

thanks
-- PMM

Re: [Qemu-devel] [PATCH V10 7/9] hw/misc/pvpanic: preparing for adding configure interface

[Peter Maydell]

On Fri, 30 Nov 2018 at 15:56, Peter Maydell <peter.maydell@linaro.org> wrote:
> I suspect the reason you've done this is that you're
> trying to get "-device pvpanic" to work on the command
> line. I recommend not trying to get that to work.
> MMIO devices are not pluggable devices, and -device
> is not expected to work with them.

If you do want a pluggable pvpanic device for the virt
board then you should implement it as a PCI device,
incidentally.

thanks
-- PMM

Re: [Qemu-devel] [PATCH V10 9/9] pvpanic : update pvpanic document

[Andrew Jones]

On Wed, Nov 28, 2018 at 08:12:52PM +0800, Peng Hao wrote:
> Add mmio support info in docs/specs/pvpanic.txt.
> 
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> ---
>  docs/specs/pvpanic.txt | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/specs/pvpanic.txt b/docs/specs/pvpanic.txt
> index c7bbacc..b1beea3 100644
> --- a/docs/specs/pvpanic.txt
> +++ b/docs/specs/pvpanic.txt
> @@ -1,14 +1,17 @@
>  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
> +pvpanic device is a simulated ISA/SYSBUS device, through which a guest

Why add the ISA/SYSBUS here? It's not really the best use of '/', IMO,
I'd just drop that as the ISA and SYSBUS is pointed out later.

> +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.
>  
> +The pvpanic device can be implemented as an ISA device (using IOPORT), 
> +or, since qemu 4.0, as a SYSBUS device (using MMIO).
> +
>  ISA Interface
>  -------------
>  
> @@ -19,6 +22,13 @@ Software should set only bits both itself and the device recognize.
>  Currently, only bit 0 is recognized, setting it indicates a guest panic
>  has happened.
>  
> +SYSBUS Interface
> +----------------
> +
> +The SYSBUS interface is similar to the ISA interface except that it uses
> +MMIO. For example, the arm virt machine could put the pvpanic device at
> +[0x9070000, 0x9070001] and currently only the first byte is used.

instead of '] and currently'. I'd write '], where currently'

> +
>  ACPI Interface
>  --------------
>  
> -- 
> 1.8.3.1
> 
> 

Otherwise 

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [Qemu-devel] [PATCH V10 5/9] hw/arm/virt: add pvpanic device in virt acpi table

[Andrew Jones]

On Wed, Nov 28, 2018 at 08:12:48PM +0800, Peng Hao wrote:
> Add pvpanic device in virt acpi table, so when kenrel command line
> uses acpi=force, kernel can get info from acpi table.
> 
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> ---
>  hw/arm/virt-acpi-build.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 5785fb6..2adba60 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -84,6 +84,20 @@ static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap,
>      aml_append(scope, dev);
>  }
>  
> +static void acpi_dsdt_add_pvpanic(Aml *scope, const MemMapEntry *pvpanic_memmap)
> +{
> +    Aml *dev = aml_device("PEVT");
> +    aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0001")));
> +    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> +
> +    Aml *crs = aml_resource_template();
> +    aml_append(crs, aml_memory32_fixed(pvpanic_memmap->base,
> +                                       pvpanic_memmap->size, AML_READ_WRITE));
> +
> +    aml_append(dev, aml_name_decl("_CRS", crs));
> +    aml_append(scope, dev);
> +}
> +
>  static void acpi_dsdt_add_fw_cfg(Aml *scope, const MemMapEntry *fw_cfg_memmap)
>  {
>      Aml *dev = aml_device("FWCF");
> @@ -771,6 +785,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
>      acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> +    acpi_dsdt_add_pvpanic(scope, &memmap[VIRT_PVPANIC]);
>      acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
>      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
>                      (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
> -- 
> 1.8.3.1
> 
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [Qemu-devel] [PATCH V10 7/9] hw/misc/pvpanic: preparing for adding configure interface

[Andrew Jones]

On Fri, Nov 30, 2018 at 03:57:13PM +0000, Peter Maydell wrote:
> On Fri, 30 Nov 2018 at 15:56, Peter Maydell <peter.maydell@linaro.org> wrote:
> > I suspect the reason you've done this is that you're
> > trying to get "-device pvpanic" to work on the command
> > line. I recommend not trying to get that to work.
> > MMIO devices are not pluggable devices, and -device
> > is not expected to work with them.
> 
> If you do want a pluggable pvpanic device for the virt
> board then you should implement it as a PCI device,
> incidentally.
>

We'd have to allocate it a PCI device ID, but I guess that's OK as
there are plenty of IDs left for 1b36. I'm not sure it's worth it
though. Phil asked that this device by user creatable, but maybe
that's not necessary. Maybe we just need a mach-virt compat boolean
and then to always provide this device for 4.0 and later machines?

Thanks,
drew

Re: [Qemu-devel] [PATCH V10 7/9] hw/misc/pvpanic: preparing for adding configure interface

[Peter Maydell]

On Fri, 30 Nov 2018 at 16:14, Andrew Jones <drjones@redhat.com> wrote:
>
> On Fri, Nov 30, 2018 at 03:57:13PM +0000, Peter Maydell wrote:
> > On Fri, 30 Nov 2018 at 15:56, Peter Maydell <peter.maydell@linaro.org> wrote:
> > > I suspect the reason you've done this is that you're
> > > trying to get "-device pvpanic" to work on the command
> > > line. I recommend not trying to get that to work.
> > > MMIO devices are not pluggable devices, and -device
> > > is not expected to work with them.
> >
> > If you do want a pluggable pvpanic device for the virt
> > board then you should implement it as a PCI device,
> > incidentally.
> >
>
> We'd have to allocate it a PCI device ID, but I guess that's OK as
> there are plenty of IDs left for 1b36. I'm not sure it's worth it
> though. Phil asked that this device by user creatable, but maybe
> that's not necessary. Maybe we just need a mach-virt compat boolean
> and then to always provide this device for 4.0 and later machines?

Yes, if it's just an mmio device then we should either:
 * default to providing it, with the usual flag to say "don't create
   in older virt board versions", and also a machine property to
   disable it manually (like we do with the ITS)
 * default to not providing it at all, and have a machine
   property to enable it (like we do with the IOMMU)
Which is all doable, but every time we do that it makes the
virt board code that extra bit more complicated (we have
half a dozen machine properties on it already).

This kind of thing is why a PCI device is cleaner -- it just
works on any machine with a PCI controller, it by default is
something that the user can provide if they want to and not if
they don't, and it's not a custom UI that management layers
might need to special-case. The guest does need to do a bit
of PCI probing and setup initially, but then it can just leave
the MMIO BAR permanently mapped and write to that address
when the guest panics.

thanks
-- PMM

Re: [Qemu-devel] [PATCH V10 7/9] hw/misc/pvpanic: preparing for adding configure interface

[peng.hao2]

>On Wed, 28 Nov 2018 at 03:50, Peng Hao <peng.hao2@zte.com.cn> wrote:
>>
>> Prepare for pvpanic-mmio configure interface.
>>
>> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
>> ---
>>  hw/arm/sysbus-fdt.c |  2 ++
>>  hw/arm/virt.c       |  2 ++
>>  hw/misc/pvpanic.c   | 11 +++++++++--
>>  3 files changed, 13 insertions(+), 2 deletions(-)
>
>This looks rather odd. The sysbus-fdt.c code is for
>handling devices that we must pass through from the
>host system (including pulling in device tree fragments
>from the host). The pvpanic device isn't a host pass
>through device so I don't think it should be touching
>that code at all. It's just a simple MMIO device.
>
>I suspect the reason you've done this is that you're
>trying to get "-device pvpanic" to work on the command
>line. I recommend not trying to get that to work.
>MMIO devices are not pluggable devices, and -device
>is not expected to work with them.
>
yes, I use -device pvpanic-mmio. Thank you for pointing this out.
 I also felt a little odd when I realized it. But I didn't think about the reason.
I think I can use a way like iommu, and I've thought about it in doing my 
current way.
thanks.
>thanks
>-- PMM

Re: [Qemu-devel] [PATCH V10 7/9] hw/misc/pvpanic: preparing foradding configure interface

[peng.hao2]

>On Fri, 30 Nov 2018 at 16:14, Andrew Jones <drjones@redhat.com> wrote:
>>
>> On Fri, Nov 30, 2018 at 03:57:13PM +0000, Peter Maydell wrote:
>> > On Fri, 30 Nov 2018 at 15:56, Peter Maydell <peter.maydell@linaro.org> wrote:
>> > > I suspect the reason you've done this is that you're
>> > > trying to get "-device pvpanic" to work on the command
>> > > line. I recommend not trying to get that to work.
>> > > MMIO devices are not pluggable devices, and -device
>> > > is not expected to work with them.
>> >
>> > If you do want a pluggable pvpanic device for the virt
>> > board then you should implement it as a PCI device,
>> > incidentally.
>> >
>>
>> We'd have to allocate it a PCI device ID, but I guess that's OK as
>> there are plenty of IDs left for 1b36. I'm not sure it's worth it
>> though. Phil asked that this device by user creatable, but maybe
>> that's not necessary. Maybe we just need a mach-virt compat boolean
>> and then to always provide this device for 4.0 and later machines?
>
>Yes, if it's just an mmio device then we should either:
>* default to providing it, with the usual flag to say "don't create
>in older virt board versions", and also a machine property to
>disable it manually (like we do with the ITS)
>* default to not providing it at all, and have a machine
>property to enable it (like we do with the IOMMU)
>Which is all doable, but every time we do that it makes the
>virt board code that extra bit more complicated (we have
>half a dozen machine properties on it already).
>
>This kind of thing is why a PCI device is cleaner -- it just
>works on any machine with a PCI controller, it by default is
>something that the user can provide if they want to and not if
>they don't, and it's not a custom UI that management layers
>might need to special-case. The guest does need to do a bit
>of PCI probing and setup initially, but then it can just leave
>the MMIO BAR permanently mapped and write to that address
>when the guest panics.
>
Usually, it doesn't matter if there is this pvpanic device or not, so I 
haven't considered configurability from the beginning.
 I didn't expect you to strongly recommend the use of PCI devices. 
Doing so is not just going to roll back some of the code in the kernel... 
I'll think about your suggestions.
thanks.
>thanks
-- PMM

Re: [Qemu-devel] [PATCH V10 7/9] hw/misc/pvpanic: preparing foradding configure interface

[Peter Maydell]

On Sat, 1 Dec 2018 at 09:28, <peng.hao2@zte.com.cn> wrote:
> Peter Maydell wrote:
> >This kind of thing is why a PCI device is cleaner -- it just
> >works on any machine with a PCI controller, it by default is
> >something that the user can provide if they want to and not if
> >they don't, and it's not a custom UI that management layers
> >might need to special-case. The guest does need to do a bit
> >of PCI probing and setup initially, but then it can just leave
> >the MMIO BAR permanently mapped and write to that address
> >when the guest panics.

> Usually, it doesn't matter if there is this pvpanic device or not, so I
> haven't considered configurability from the beginning.

It matters to people who don't want it, because it increases
the attack surface presented to the guest.

>  I didn't expect you to strongly recommend the use of PCI devices.
> Doing so is not just going to roll back some of the code in the kernel...
> I'll think about your suggestions.

My view on this hasn't changed since October:
https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg03861.html
if this really really needs to be an MMIO device then it can
be done, but a PCI device is a much cleaner interface which
is more widely usable than just the virt board.

thanks
-- PMM