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

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

[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

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

-- 
2.17.2

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

[Philippe Mathieu-Daudé]

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

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 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 6d50b03cfd..74e7300f30 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -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
-- 
2.17.2

[Qemu-devel] [PATCH v3 2/4] hw/misc/pvpanic: Cosmetic renaming

[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é <philmd@redhat.com>
---
 hw/misc/pvpanic.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index 9d8961ba0c..4f552e1533 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -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[] = {
-- 
2.17.2

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

[Philippe Mathieu-Daudé]

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
[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(-)

diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index 4f552e1533..b81c5fa633 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.
@@ -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)
-- 
2.17.2

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

[Philippe Mathieu-Daudé]

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
[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(-)

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index 2420491aac..def07fa095 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -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
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 9f677825f9..40469e6785 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, \
@@ -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
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 4cc57a7ef6..61cc3109b2 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -70,6 +70,7 @@ enum {
     VIRT_MMIO,
     VIRT_RTC,
     VIRT_FW_CFG,
+    VIRT_PVPANIC_MMIO,
     VIRT_PCIE,
     VIRT_PCIE_MMIO,
     VIRT_PCIE_PIO,
-- 
2.17.2

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

[Peter Maydell]

On 18 October 2018 at 14:04, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> [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(-)
>
> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
> index 4f552e1533..b81c5fa633 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.
> @@ -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

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

[Philippe Mathieu-Daudé]

On 18/10/2018 15:04, Philippe Mathieu-Daudé wrote:
> 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

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]

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

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

[Philippe Mathieu-Daudé]

On 18/10/2018 15:08, Peter Maydell wrote:
> On 18 October 2018 at 14:04, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> [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(-)
>>
>> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
>> index 4f552e1533..b81c5fa633 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.
>> @@ -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.

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.

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

[Peter Maydell]

On 18 October 2018 at 14:19, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> On 18/10/2018 15:08, Peter Maydell wrote:
>> 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 >*/
>         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

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

[Peter Maydell]

On 18 October 2018 at 14:04, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> [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(-)
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index 2420491aac..def07fa095 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -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
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 9f677825f9..40469e6785 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, \
> @@ -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.

>      [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);

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

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

[peng.hao2]

>On 18 October 2018 at 14:04, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> [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(-)
>>
>> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
>> index 2420491aac..def07fa095 100644
>> --- a/default-configs/arm-softmmu.mak
>> +++ b/default-configs/arm-softmmu.mak
>> @@ -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
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 9f677825f9..40469e6785 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, \
>> @@ -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.

>>      [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);
>
>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" ?
>
>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.
>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.
>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

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

[peng.hao2]

>
>Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>[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.
>hw/misc/pvpanic.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++-
>1 file changed, 51 insertions(+), 1 deletion(-)

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

[peng.hao2]

>On 18/10/2018 15:04, Philippe Mathieu-Daudé wrote:
>> 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
>
>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.

>with below my S-o-b:
>[PMD: Use TYPE_PVPANIC definition, split in 2 patches,
>improved patch subject]
>
>>   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(-)
>>