hw/misc: Add a MMIO interface to the pvpanic device

PATCH In review Philippe Mathieu-Daudé 3 participants 13 messages Oct 18, 2018 – Oct 25, 2018 1 message not in the archive

lore

Share & tools

lore

Philippe Mathieu-Daudé

author

2018-10-18 13:04 UTC #0 lore raw

v3 cover hw/misc: Add a MMIO interface to the pvpanic device

Hi, this series takes Peng Hao's previous work but rather than adding yet another device, simply add the MMIO interface to the current device (which only implements the I/O port access).

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).

v2 from Peng Hao is:
https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg03433.html

Regards,

Phil.

Philippe Mathieu-Daudé (4):
hw/misc/pvpanic: Build the pvpanic device in $(common-obj) hw/misc/pvpanic: Cosmetic renaming hw/misc/pvpanic: Add the MMIO interface hw/arm/virt: Use the pvpanic device

 default-configs/arm-softmmu.mak |  2 +-
 hw/arm/virt.c                   | 21 ++++++++++
 hw/misc/Makefile.objs           |  2 +-
 hw/misc/pvpanic.c               | 68 +++++++++++++++++++++++++++++----
 include/hw/arm/virt.h           |  1 +
 5 files changed, 84 insertions(+), 10 deletions(-)

Signature · 1 line

2.17.2

Philippe Mathieu-Daudé

author

2018-10-18 13:04 UTC #1 lore raw

PATCH v3 1/4 hw/misc/pvpanic: Build the pvpanic device in $(common-obj)

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

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/misc/Makefile.objs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

hw/misc/Makefile.objs +1 -1

index 6d50b03cfd..74e7300f30 100644
@@ -11,6 +11,7 @@ common-obj-$(CONFIG_PCA9552) += pca9552.o
 
 common-obj-y += unimp.o
 common-obj-$(CONFIG_FW_CFG_DMA) += vmcoreinfo.o
+common-obj-$(CONFIG_PVPANIC) += pvpanic.o
 
 # ARM devices
 common-obj-$(CONFIG_PL310) += arm_l2x0.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_HYPERV_TESTDEV) += hyperv_testdev.o
 obj-$(CONFIG_AUX) += auxbus.o
 obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o aspeed_sdmc.o

Signature · 1 line

2.17.2

Philippe Mathieu-Daudé

author

2018-10-18 13:04 UTC #2 lore raw

PATCH v3 2/4 hw/misc/pvpanic: Cosmetic renaming

In reply to Philippe Mathieu-Daudé

To ease the MMIO device addition in the next patch, rename:
- ISA_PVPANIC_DEVICE -> PVPANIC (this just returns a generic Object),
- ISADevice parent_obj -> isadev,
- MemoryRegion io -> mr.

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/misc/pvpanic.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

hw/misc/pvpanic.c +9 -7

index 9d8961ba0c..4f552e1533 100644
@@ -25,7 +25,7 @@
 /* The pv event value */
 #define PVPANIC_PANICKED        (1 << PVPANIC_F_PANICKED)
 
-#define ISA_PVPANIC_DEVICE(obj)    \
+#define PVPANIC(obj)    \
     OBJECT_CHECK(PVPanicState, (obj), TYPE_PVPANIC)
 
 static void handle_event(int event)

@@ -46,9 +46,11 @@ static void handle_event(int event)
 #include "hw/isa/isa.h"
 
 typedef struct PVPanicState {
-    ISADevice parent_obj;
+    /*< private >*/
+    ISADevice isadev;
 
-    MemoryRegion io;
+    /*< public >*/
+    MemoryRegion mr;
     uint16_t ioport;
 } PVPanicState;

@@ -75,15 +77,15 @@ static const MemoryRegionOps pvpanic_ops = {
 
 static void pvpanic_isa_initfn(Object *obj)
 {
-    PVPanicState *s = ISA_PVPANIC_DEVICE(obj);
+    PVPanicState *s = PVPANIC(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);
+    PVPanicState *s = PVPANIC(dev);
     FWCfgState *fw_cfg = fw_cfg_find();
     uint16_t *pvpanic_port;

@@ -96,7 +98,7 @@ 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[] = {

Signature · 1 line

2.17.2

Philippe Mathieu-Daudé

author

2018-10-18 13:05 UTC #3 lore raw

PATCH v3 3/4 hw/misc/pvpanic: Add the MMIO interface

In reply to Philippe Mathieu-Daudé

Signed-off-by Peng Hao <[email protected]>

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

[PMD: Use TYPE_PVPANIC definition, split in 2 patches]

---
Peng: I hope this is now more obvious how you could reuse the pvpanic device.

 hw/misc/pvpanic.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

hw/misc/pvpanic.c +51 -1

index 4f552e1533..b81c5fa633 100644
@@ -2,10 +2,12 @@
  * QEMU simulated pvpanic device.
  *
  * Copyright Fujitsu, Corp. 2013
+ * Copyright (c) 2018 ZTE Ltd.
  *
  * Authors:
  *     Wen Congyang <[email protected]>
  *     Hu Tao <[email protected]>
+ *     Peng Hao <[email protected]>
  *
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.

@@ -47,7 +49,10 @@ static void handle_event(int event)
 
 typedef struct PVPanicState {
     /*< private >*/
-    ISADevice isadev;
+    union {
+        ISADevice isadev;
+        SysBusDevice busdev;
+    };
 
     /*< public >*/
     MemoryRegion mr;

@@ -123,9 +128,54 @@ static TypeInfo pvpanic_isa_info = {
     .class_init    = pvpanic_isa_class_init,
 };
 
+static uint64_t pvpanic_mmio_read(void *opaque, hwaddr addr, unsigned size)
+{
+    return -1;
+}
+
+static void pvpanic_mmio_write(void *opaque, hwaddr addr, uint64_t value,
+                                 unsigned size)
+{
+   handle_event(value);
+}
+
+static const MemoryRegionOps pvpanic_mmio_ops = {
+    .read = pvpanic_mmio_read,
+    .write = pvpanic_mmio_write,
+    .impl = {
+        .max_access_size = 1,
+    },
+};
+
+static void pvpanic_mmio_initfn(Object *obj)
+{
+    PVPanicState *s = PVPANIC(obj);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+
+    memory_region_init_io(&s->mr, OBJECT(s), &pvpanic_mmio_ops, s,
+                          TYPE_PVPANIC, 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,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(PVPanicState),
+    .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)

Signature · 1 line

2.17.2

Peter Maydell

2018-10-18 13:08 UTC #4 lore raw

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

Quoted 27 lines · Philippe Mathieu-Daudé

Signed-off-by Peng Hao <[email protected]>

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

[PMD: Use TYPE_PVPANIC definition, split in 2 patches]

---
Peng: I hope this is now more obvious how you could reuse the pvpanic device.

 hw/misc/pvpanic.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

hw/misc/pvpanic.c +2 -0

index 4f552e1533..b81c5fa633 100644
@@ -2,10 +2,12 @@
  * QEMU simulated pvpanic device.
  *
  * Copyright Fujitsu, Corp. 2013
+ * Copyright (c) 2018 ZTE Ltd.
  *
  * Authors:
  *     Wen Congyang <[email protected]>
  *     Hu Tao <[email protected]>
+ *     Peng Hao <[email protected]>
  *
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.

hw/misc/pvpanic.c static void handle_event(int event) · line 55

Quoted 8 lines

hw/misc/pvpanic.c +4 -1

@@ -47,7 +49,10 @@ static void handle_event(int event)
 
 typedef struct PVPanicState {
     /*< private >*/
-    ISADevice isadev;
+    union {
+        ISADevice isadev;
+        SysBusDevice busdev;
+    };

This field is the parent-type for the QOM object, so I don't think it makes sense for it to be a union. Any one QOM object should have a single distinct parent type. (There are other examples of "some more or less similar device has ISA and SysBus or PCI and SysBus variations" which should provide some models to copy from.

thanks
-- PMM

Philippe Mathieu-Daudé

author

2018-10-18 13:19 UTC #5 lore raw

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

Quoted 29 lines · Peter Maydell

Quoted 28 lines · Philippe Mathieu-Daudé

Signed-off-by Peng Hao <[email protected]>

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

[PMD: Use TYPE_PVPANIC definition, split in 2 patches]

---
Peng: I hope this is now more obvious how you could reuse the pvpanic device.

 hw/misc/pvpanic.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

hw/misc/pvpanic.c +2 -0

index 4f552e1533..b81c5fa633 100644
@@ -2,10 +2,12 @@
  * QEMU simulated pvpanic device.
  *
  * Copyright Fujitsu, Corp. 2013
+ * Copyright (c) 2018 ZTE Ltd.
  *
  * Authors:
  *     Wen Congyang <[email protected]>
  *     Hu Tao <[email protected]>
+ *     Peng Hao <[email protected]>
  *
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.

@@ -47,7 +49,10 @@ static void handle_event(int event)

Quoted 14 lines

hw/misc/pvpanic.c static void handle_event(int event) · line 55
Quoted 7 lines
hw/misc/pvpanic.c +4 -1
 typedef struct PVPanicState {
     /*< private >*/
-    ISADevice isadev;
+    union {
+        ISADevice isadev;
+        SysBusDevice busdev;
+    };
This field is the parent-type for the QOM object, so I don't think it makes sense for it to be a union. Any one QOM object should have a single distinct parent type. (There are other examples of "some more or less similar device has ISA and SysBus or PCI and SysBus variations" which should provide some models to copy from.

OK, I understand as I can use "Device parent_obj" instead of that union? But then we want:

pvpanic_isa_info.instance_size =
sizeof(PVPanic_Common_State) + sizeof(ISADevice)

pvpanic_mmio_info.instance_size =
sizeof(PVPanic_Common_State) + sizeof(SysBusDevice)

So to avoid union, I have to use:

    typedef struct PVPanicCommonState {
        MemoryRegion mr;
        uint16_t ioport;
    } PVPanicCommonState;

    typedef struct PVPanicISAState {
        /*< private >*/
        ISADevice isadev;

        /*< public >*/
        PVPanicCommonState common;
    } PVPanicISAState;

    #define PVPANIC_ISA(obj)    \
        OBJECT_CHECK(PVPanicISAState, (obj), TYPE_PVPANIC)

    typedef struct PVPanicMMIOState {
        /*< private >*/
        SysBusDevice busdev;

        /*< public >*/
        PVPanicCommonState common;
    } PVPanicMMIOState;

    #define PVPANIC_MMIO(obj)    \
        OBJECT_CHECK(PVPanicMMIOState, (obj), TYPE_PVPANIC)

And later:

-->8--
@@ -92,3 +104,3 @@ static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp)

     ISADevice *d = ISA_DEVICE(dev);
-    PVPanicState *s = PVPANIC(dev);
+    PVPanicState *s = PVPANIC_ISA(dev);
     FWCfgState *fw_cfg = fw_cfg_find();

@@ -125,3 +137,3 @@ static TypeInfo pvpanic_isa_info = {
.parent = TYPE_ISA_DEVICE,
- .instance_size = sizeof(PVPanicState),
+ .instance_size = sizeof(PVPanicISAState),
.instance_init = pvpanic_isa_initfn,
@@ -151,3 +163,3 @@ static void pvpanic_mmio_initfn(Object *obj)
{
- PVPanicState *s = PVPANIC(obj);

+    PVPanicState *s = PVPANIC_MMIO(obj);
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);

@@ -169,3 +181,3 @@ static TypeInfo pvpanic_mmio_info = {
.parent = TYPE_SYS_BUS_DEVICE,
- .instance_size = sizeof(PVPanicState),
+ .instance_size = sizeof(PVPanicMMIOState),
.instance_init = pvpanic_mmio_initfn,
---

Am I correct?

Thanks,

Phil.

Peter Maydell

2018-10-18 13:28 UTC #6 lore raw

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

Quoted 28 lines · Philippe Mathieu-Daudé

Quoted 6 lines · Peter Maydell
This field is the parent-type for the QOM object, so I don't think it makes sense for it to be a union. Any one QOM object should have a single distinct parent type. (There are other examples of "some more or less similar device has ISA and SysBus or PCI and SysBus variations" which should provide some models to copy from.
So to avoid union, I have to use:
    typedef struct PVPanicCommonState {
        MemoryRegion mr;
        uint16_t ioport;
    } PVPanicCommonState;

    typedef struct PVPanicISAState {
        /*< private >*/
        ISADevice isadev;

        /*< public >*/
        PVPanicCommonState common;
    } PVPanicISAState;

    #define PVPANIC_ISA(obj)    \
        OBJECT_CHECK(PVPanicISAState, (obj), TYPE_PVPANIC)

    typedef struct PVPanicMMIOState {
        /*< private >*/

Quoted 9 lines

        SysBusDevice busdev;

        /*< public >*/
        PVPanicCommonState common;
    } PVPanicMMIOState;

    #define PVPANIC_MMIO(obj)    \
        OBJECT_CHECK(PVPanicMMIOState, (obj), TYPE_PVPANIC)

Something like that, I think. Basically you have two distinct types, an ISA PVPanic and a SysBus PVPanic, which have straightforward inheritance (the ISA PVPanic is-a ISADevice, the SysBus PVPanic is-a SysBusdevice). Both have-a PVPanic common state.

See eg hw/display/sm501.c, or hw/net/ne2000{,-isa}.c.

thanks
-- PMM

[email protected]

2018-10-18 16:13 UTC #7 lore raw

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

In reply to Philippe Mathieu-Daudé

Quoted 7 lines

Signed-off-by Peng Hao <[email protected]>
Signed-off-by Philippe Mathieu-Daudé <[email protected]>
[PMD: Use TYPE_PVPANIC definition, split in 2 patches]
---
Peng: I hope this is now more obvious how you could reuse the pvpanic device.

Thanks. I will think about your suggestions and patches.

Quoted 2 lines

hw/misc/pvpanic.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 51 insertions(+), 1 deletion(-)

Philippe Mathieu-Daudé

author

2018-10-18 13:05 UTC #8 lore raw

PATCH v3 4/4 hw/arm/virt: Use the pvpanic device

In reply to Philippe Mathieu-Daudé

Signed-off-by Peng Hao <[email protected]>

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

[PMD: Use TYPE_PVPANIC definition, split in 2 patches]

---
 default-configs/arm-softmmu.mak |  2 +-
 hw/arm/virt.c                   | 21 +++++++++++++++++++++
 include/hw/arm/virt.h           |  1 +
 3 files changed, 23 insertions(+), 1 deletion(-)

default-configs/arm-softmmu.mak +1 -1

index 2420491aac..def07fa095 100644
@@ -43,7 +43,7 @@ CONFIG_USB_MUSB=y
 CONFIG_USB_EHCI_SYSBUS=y
 CONFIG_PLATFORM_BUS=y
 CONFIG_VIRTIO_MMIO=y
-
+CONFIG_PVPANIC=y
 CONFIG_ARM11MPCORE=y
 CONFIG_A9MPCORE=y
 CONFIG_A15MPCORE=y

hw/arm/virt.c +21 -0

index 9f677825f9..40469e6785 100644
@@ -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, \

@@ -140,6 +141,7 @@ static const MemMapEntry a15memmap[] = {
     [VIRT_UART] =               { 0x09000000, 0x00001000 },
     [VIRT_RTC] =                { 0x09010000, 0x00001000 },
     [VIRT_FW_CFG] =             { 0x09020000, 0x00000018 },
+    [VIRT_PVPANIC_MMIO] =       { 0x09020018, 0x00000002 },
     [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
     [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
     [VIRT_SMMU] =               { 0x09050000, 0x00020000 },

@@ -802,6 +804,23 @@ static void create_gpio(const VirtMachineState *vms, qemu_irq *pic)
     g_free(nodename);
 }
 
+static void create_pvpanic_device(const VirtMachineState *vms)
+{
+    char *nodename;
+    hwaddr base = vms->memmap[VIRT_PVPANIC_MMIO].base;
+    hwaddr size = vms->memmap[VIRT_PVPANIC_MMIO].size;
+
+    sysbus_create_simple(TYPE_PVPANIC, 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", "pvpanic,mmio");
+    qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
+                                 2, base, 2, size);
+    g_free(nodename);
+}
+
 static void create_virtio_devices(const VirtMachineState *vms, qemu_irq *pic)
 {
     int i;

@@ -1548,6 +1567,8 @@ static void machvirt_init(MachineState *machine)
 
     create_pcie(vms, pic);
 
+    create_pvpanic_device(vms);
+
     create_gpio(vms, pic);
 
     /* Create mmio transports, so the user can create virtio backends

include/hw/arm/virt.h +1 -0

index 4cc57a7ef6..61cc3109b2 100644
@@ -70,6 +70,7 @@ enum {
     VIRT_MMIO,
     VIRT_RTC,
     VIRT_FW_CFG,
+    VIRT_PVPANIC_MMIO,
     VIRT_PCIE,
     VIRT_PCIE_MMIO,
     VIRT_PCIE_PIO,

Signature · 1 line

2.17.2

Peter Maydell

2018-10-18 13:37 UTC #9 lore raw

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

Quoted 35 lines · Philippe Mathieu-Daudé

Signed-off-by Peng Hao <[email protected]>

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

[PMD: Use TYPE_PVPANIC definition, split in 2 patches]

---
 default-configs/arm-softmmu.mak |  2 +-
 hw/arm/virt.c                   | 21 +++++++++++++++++++++
 include/hw/arm/virt.h           |  1 +
 3 files changed, 23 insertions(+), 1 deletion(-)

default-configs/arm-softmmu.mak +1 -1

index 2420491aac..def07fa095 100644
@@ -43,7 +43,7 @@ CONFIG_USB_MUSB=y
 CONFIG_USB_EHCI_SYSBUS=y
 CONFIG_PLATFORM_BUS=y
 CONFIG_VIRTIO_MMIO=y
-
+CONFIG_PVPANIC=y
 CONFIG_ARM11MPCORE=y
 CONFIG_A9MPCORE=y
 CONFIG_A15MPCORE=y

hw/arm/virt.c +1 -0

index 9f677825f9..40469e6785 100644
@@ -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, \

hw/arm/virt.c static const MemMapEntry a15memmap[] = { · line 144

Quoted 4 lines

hw/arm/virt.c +1 -0

@@ -140,6 +141,7 @@ static const MemMapEntry a15memmap[] = {
     [VIRT_UART] =               { 0x09000000, 0x00001000 },
     [VIRT_RTC] =                { 0x09010000, 0x00001000 },
     [VIRT_FW_CFG] =             { 0x09020000, 0x00000018 },
+    [VIRT_PVPANIC_MMIO] =       { 0x09020018, 0x00000002 },

This shouldn't be in the same physical 64K page as the preceding device:
we carefully keep them all separate so that the guest can give them separate MMU permissions if it wants.

Quoted 11 lines

hw/arm/virt.c +4 -0

     [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
     [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
     [VIRT_SMMU] =               { 0x09050000, 0x00020000 },

@@ -802,6 +804,23 @@ static void create_gpio(const VirtMachineState *vms, qemu_irq *pic)
     g_free(nodename);
 }
 
+static void create_pvpanic_device(const VirtMachineState *vms)
+{
+    char *nodename;
+    hwaddr base = vms->memmap[VIRT_PVPANIC_MMIO].base;

hw/arm/virt.c static void create_gpio(const VirtMachineState *vms, qemu_irq *pic) · line 820

Quoted 10 lines

hw/arm/virt.c +10 -0

+    hwaddr size = vms->memmap[VIRT_PVPANIC_MMIO].size;
+
+    sysbus_create_simple(TYPE_PVPANIC, 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", "pvpanic,mmio");
+    qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
+                                 2, base, 2, size);

This doesn't seem to be an official DT binding. It would need to be accepted upstream (ie in the kernel's Documentation/devicetree/bindings/) before we could add the device in QEMU.

What about an ACPI table entry?

Does this really need to be an MMIO device, rather than a PCI device? Being a PCI device would avoid all of these issues:
* having to specify a dt binding
* having to specify an ACPI binding
* having to either have the thing always present, or some custom way for the user to enable/disable it
plus it means it's available for other boards and architectures than just the Arm virt board.

I'd also like some confirmation from folks more familiar with the current state of the art in guest-to-management-layer communication that pvpanic is still the recommended way to achieve this goal, and hasn't been obsoleted by something else.

thanks
-- PMM

[email protected]

2018-10-18 16:07 UTC #10 lore raw

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

Quoted 33 lines · Philippe Mathieu-Daudé

Quoted 32 lines

Signed-off-by Peng Hao <[email protected]>

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

[PMD: Use TYPE_PVPANIC definition, split in 2 patches]

---
 default-configs/arm-softmmu.mak |  2 +-
 hw/arm/virt.c                   | 21 +++++++++++++++++++++
 include/hw/arm/virt.h           |  1 +
 3 files changed, 23 insertions(+), 1 deletion(-)

default-configs/arm-softmmu.mak +1 -1

index 2420491aac..def07fa095 100644
@@ -43,7 +43,7 @@ CONFIG_USB_MUSB=y
 CONFIG_USB_EHCI_SYSBUS=y
 CONFIG_PLATFORM_BUS=y
 CONFIG_VIRTIO_MMIO=y
-
+CONFIG_PVPANIC=y
 CONFIG_ARM11MPCORE=y
 CONFIG_A9MPCORE=y
 CONFIG_A15MPCORE=y

hw/arm/virt.c +1 -0

index 9f677825f9..40469e6785 100644
@@ -59,6 +59,7 @@
 #include "qapi/visitor.h"
 #include "standard-headers/linux/input.h"
 #include "hw/arm/smmuv3.h"
+#include "hw/misc/pvpanic.h"

Quoted 12 lines

hw/arm/virt.c static const MemMapEntry a15memmap[] = { · line 144
Quoted 7 lines
hw/arm/virt.c +1 -0
 #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
     static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
@@ -140,6 +141,7 @@ static const MemMapEntry a15memmap[] = {
     [VIRT_UART] =               { 0x09000000, 0x00001000 },
     [VIRT_RTC] =                { 0x09010000, 0x00001000 },
     [VIRT_FW_CFG] =             { 0x09020000, 0x00000018 },
+    [VIRT_PVPANIC_MMIO] =       { 0x09020018, 0x00000002 },
This shouldn't be in the same physical 64K page as the preceding device:
we carefully keep them all separate so that the guest can give them separate MMU permissions if it wants.

I never notice that. Thanks.

Quoted 17 lines

hw/arm/virt.c +10 -0

     [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
     [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
     [VIRT_SMMU] =               { 0x09050000, 0x00020000 },

@@ -802,6 +804,23 @@ static void create_gpio(const VirtMachineState *vms, qemu_irq *pic)
     g_free(nodename);
 }
 
+static void create_pvpanic_device(const VirtMachineState *vms)
+{
+    char *nodename;
+    hwaddr base = vms->memmap[VIRT_PVPANIC_MMIO].base;
+    hwaddr size = vms->memmap[VIRT_PVPANIC_MMIO].size;
+
+    sysbus_create_simple(TYPE_PVPANIC, base, NULL);
+
+    nodename = g_strdup_printf("/pvpanic-mmio@%" PRIx64, base);
+    qemu_fdt_add_subnode(vms->fdt, nodename);

Quoted 8 lines

hw/arm/virt.c static void create_gpio(const VirtMachineState *vms, qemu_irq *pic) · line 820
Quoted 4 lines
hw/arm/virt.c +4 -0
+    qemu_fdt_setprop_string(vms->fdt, nodename,
+                            "compatible", "pvpanic,mmio");
+    qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
+                                 2, base, 2, size);
This doesn't seem to be an official DT binding. It would need to be accepted upstream (ie in the kernel's Documentation/devicetree/bindings/) before we could add the device in QEMU.

The patch I submitted to the kernel about pvpanic-mmio driver is also pointing out the same problem. I can use "qemu,pvpanic-mmio" ?

Quoted 2 lines

What about an ACPI table entry?

I must add an ACPI table entry for pvpanic-mmio?
I tested whether adding a ACPI entry does not seem to affect the function of pvpanic. now I emulate the pvpanic-mmio device as a sysbus device and use fdt to send mmio address info.

Quoted 9 lines

Does this really need to be an MMIO device, rather than a PCI device? Being a PCI device would avoid all of these issues:
* having to specify a dt binding
* having to specify an ACPI binding
* having to either have the thing always present, or some custom way for the user to enable/disable it
plus it means it's available for other boards and architectures than just the Arm virt board.

I thought about pci device , If so, it seems that I can only use ACPI to pass MMIO information, just like
pvpanic isa device did. I don't want to rely on ACPI like pvpanic isa device . I can use fdt to transmit mmio info to kernel for pci device ? another reason, the function of pvpanic is simple.

Thanks.

Quoted 7 lines

I'd also like some confirmation from folks more familiar with the current state of the art in guest-to-management-layer communication that pvpanic is still the recommended way to achieve this goal, and hasn't been obsoleted by something else.
thanks
-- PMM

Philippe Mathieu-Daudé

author

2018-10-18 13:09 UTC #11 lore raw

In reply to Philippe Mathieu-Daudé

Quoted 18 lines · Philippe Mathieu-Daudé

Hi, this series takes Peng Hao's previous work but rather than adding yet another device, simply add the MMIO interface to the current device (which only implements the I/O port access).
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).
v2 from Peng Hao is:
https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg03433.html

Quoted 7 lines

Regards,
Phil.
Philippe Mathieu-Daudé (4):
hw/misc/pvpanic: Build the pvpanic device in $(common-obj) hw/misc/pvpanic: Cosmetic renaming

Oops I failed when rebasing, these two are supposed to be from

Peng Hao

with below my S-o-b:
[PMD: Use TYPE_PVPANIC definition, split in 2 patches,
improved patch subject]

Quoted 10 lines

hw/misc/pvpanic: Add the MMIO interface
hw/arm/virt: Use the pvpanic device

 default-configs/arm-softmmu.mak |  2 +-
 hw/arm/virt.c                   | 21 ++++++++++
 hw/misc/Makefile.objs           |  2 +-
 hw/misc/pvpanic.c               | 68 +++++++++++++++++++++++++++++----
 include/hw/arm/virt.h           |  1 +
 5 files changed, 84 insertions(+), 10 deletions(-)

[email protected]

2018-10-25 05:47 UTC #12 lore raw

Re: [Qemu-devel] [PATCH v3 0/4] hw/misc: Add a MMIO interface to the pvpanicdevice

Quoted 23 lines · Philippe Mathieu-Daudé

Quoted 22 lines
Hi, this series takes Peng Hao's previous work but rather than adding yet another device, simply add the MMIO interface to the current device (which only implements the I/O port access).
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).
v2 from Peng Hao is:
https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg03433.html
Regards,
Phil.

Quoted 8 lines

Quoted 3 lines
Philippe Mathieu-Daudé (4):
hw/misc/pvpanic: Build the pvpanic device in $(common-obj) hw/misc/pvpanic: Cosmetic renaming
Oops I failed when rebasing, these two are supposed to be from
Peng Hao

now it can works.
In refactoring code, there is still a need to add a type to distinguish different bus device. I add a type TYPE_PVPANIC_MMIO for sysbus pvpanic device and
orignal TYPE_PVPANIC for isa pvpanic deivce.

bindling-DT "qemu,pvpanic-mmio" in kernel is in review. I will resubmit patches.

Thanks.

Quoted 14 lines

with below my S-o-b:
[PMD: Use TYPE_PVPANIC definition, split in 2 patches, improved patch subject]
Quoted 10 lines
hw/misc/pvpanic: Add the MMIO interface
hw/arm/virt: Use the pvpanic device
 default-configs/arm-softmmu.mak |  2 +-
 hw/arm/virt.c                   | 21 ++++++++++
 hw/misc/Makefile.objs           |  2 +-
 hw/misc/pvpanic.c               | 68 +++++++++++++++++++++++++++++----
 include/hw/arm/virt.h           |  1 +
 5 files changed, 84 insertions(+), 10 deletions(-)