Re: [Qemu-devel] [PATCH V11 6/8] hw/arm/virt: add configure interface for pvpanic-mmio

Re: [Qemu-devel] [PATCH V11 6/8] hw/arm/virt: add configure interface for pvpanic-mmio

[Andrew Jones]

On Tue, Dec 04, 2018 at 03:26:47AM +0800, Peng Hao wrote:
> Add configure interface for pvpanic-mmio device in virt machine.
> 
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> ---
>  hw/arm/virt.c         | 23 +++++++++++++++++++++++
>  include/hw/arm/virt.h |  1 +
>  2 files changed, 24 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index a4541fa..fdd3f20 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1655,6 +1655,20 @@ static void virt_set_its(Object *obj, bool value, Error **errp)
>      vms->its = value;
>  }
>  
> +static bool virt_get_pvpanic(Object *obj, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    return vms->pvpanic;
> +}
> +
> +static void virt_set_pvpanic(Object *obj, bool value, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    vms->pvpanic = value;
> +}
> +
>  static char *virt_get_gic_version(Object *obj, Error **errp)
>  {
>      VirtMachineState *vms = VIRT_MACHINE(obj);
> @@ -1884,6 +1898,15 @@ static void virt_3_1_instance_init(Object *obj)
>                                      "Valid values are none and smmuv3",
>                                      NULL);
>  
> +    /* Default disallows pvpanic-mmio instantiation */
> +    vms->pvpanic = false;
> +    object_property_add_bool(obj, "pvpanic", virt_get_pvpanic,
> +                             virt_set_pvpanic, NULL);
> +    object_property_set_description(obj, "pvpanic",
> +                                    "Set on/off to enable/disable "
> +                                    "PVPANIC MMIO device",
> +                                    NULL);
> +
>      vms->memmap = a15memmap;
>      vms->irqmap = a15irqmap;
>  }
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 937c124..7d6d1c0 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -113,6 +113,7 @@ typedef struct {
>      bool highmem;
>      bool highmem_ecam;
>      bool its;
> +    bool pvpanic;

This hunk should be squashed into 4/8 and the respective hunks of 7/8
should be squashed into 4/8 and 5/8.

Thanks,
drew

>      bool virt;
>      int32_t gic_version;
>      VirtIOMMUType iommu;
> -- 
> 1.8.3.1
> 

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

[Andrew Jones]

On Tue, Dec 04, 2018 at 03:26:45AM +0800, Peng Hao 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                   | 22 ++++++++++++++++++++++
>  include/hw/arm/virt.h           |  1 +
>  3 files changed, 24 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..a4541fa 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, \
> @@ -143,6 +144,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 +192,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 +1551,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
>

With the change pointed out in 6/8

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

Re: [Qemu-devel] [PATCH V11 6/8] hw/arm/virt: add configure interface for pvpanic-mmio

[Andrew Jones]

On Mon, Dec 03, 2018 at 12:18:36PM +0100, Andrew Jones wrote:
> On Tue, Dec 04, 2018 at 03:26:47AM +0800, Peng Hao wrote:
> > Add configure interface for pvpanic-mmio device in virt machine.
> > 
> > Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> > ---
> >  hw/arm/virt.c         | 23 +++++++++++++++++++++++
> >  include/hw/arm/virt.h |  1 +
> >  2 files changed, 24 insertions(+)
> > 
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index a4541fa..fdd3f20 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1655,6 +1655,20 @@ static void virt_set_its(Object *obj, bool value, Error **errp)
> >      vms->its = value;
> >  }
> >  
> > +static bool virt_get_pvpanic(Object *obj, Error **errp)
> > +{
> > +    VirtMachineState *vms = VIRT_MACHINE(obj);
> > +
> > +    return vms->pvpanic;
> > +}
> > +
> > +static void virt_set_pvpanic(Object *obj, bool value, Error **errp)
> > +{
> > +    VirtMachineState *vms = VIRT_MACHINE(obj);
> > +
> > +    vms->pvpanic = value;
> > +}
> > +
> >  static char *virt_get_gic_version(Object *obj, Error **errp)
> >  {
> >      VirtMachineState *vms = VIRT_MACHINE(obj);
> > @@ -1884,6 +1898,15 @@ static void virt_3_1_instance_init(Object *obj)
> >                                      "Valid values are none and smmuv3",
> >                                      NULL);
> >  
> > +    /* Default disallows pvpanic-mmio instantiation */
> > +    vms->pvpanic = false;
> > +    object_property_add_bool(obj, "pvpanic", virt_get_pvpanic,
> > +                             virt_set_pvpanic, NULL);
> > +    object_property_set_description(obj, "pvpanic",
> > +                                    "Set on/off to enable/disable "
> > +                                    "PVPANIC MMIO device",
> > +                                    NULL);
> > +
> >      vms->memmap = a15memmap;
> >      vms->irqmap = a15irqmap;
> >  }
> > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > index 937c124..7d6d1c0 100644
> > --- a/include/hw/arm/virt.h
> > +++ b/include/hw/arm/virt.h
> > @@ -113,6 +113,7 @@ typedef struct {
> >      bool highmem;
> >      bool highmem_ecam;
> >      bool its;
> > +    bool pvpanic;
> 
> This hunk should be squashed into 4/8 and the respective hunks of 7/8
> should be squashed into 4/8 and 5/8.
>

With the above changes

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

Re: [Qemu-devel] [PATCH V11 0/8] add pvpanic mmio support

[Peter Maydell]

On Mon, 3 Dec 2018 at 11:04, Peng Hao <peng.hao2@zte.com.cn> wrote:
>
> 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

I would still prefer to see a more detailed examination of whether
we can do this with a PCI device before we commit to taking the
MMIO version into the virt board.

thanks
-- PMM

[Qemu-devel] [PATCH V11 0/8] 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.

v10 --> v11
     change configure interface in virt machine configure parameters.

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 (6):
  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 interface for pvpanic-mmio
  hw/misc/pvpanic: realize the configure interface
  pvpanic : update pvpanic document

 default-configs/arm-softmmu.mak |  1 +
 docs/specs/pvpanic.txt          | 15 ++++++-
 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 V11 1/8] 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 V11 2/8] 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.

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/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 V11 3/8] hw/misc/pvpanic: Add the MMIO interface

[Peng Hao]

Add pvpanic new type "TYPE_PVPANIC_MMIO"

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
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 V11 4/8] 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                   | 22 ++++++++++++++++++++++
 include/hw/arm/virt.h           |  1 +
 3 files changed, 24 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..a4541fa 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, \
@@ -143,6 +144,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 +192,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 +1551,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 V11 5/8] hw/arm/virt: add pvpanic device in virt acpi table

[Peng Hao]

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

Reviewed-by: Andrew Jones <drjones@redhat.com>
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..4215ca6 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 V11 6/8] hw/arm/virt: add configure interface for pvpanic-mmio

[Peng Hao]

Add configure interface for pvpanic-mmio device in virt machine.

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

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index a4541fa..fdd3f20 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1655,6 +1655,20 @@ static void virt_set_its(Object *obj, bool value, Error **errp)
     vms->its = value;
 }
 
+static bool virt_get_pvpanic(Object *obj, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(obj);
+
+    return vms->pvpanic;
+}
+
+static void virt_set_pvpanic(Object *obj, bool value, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(obj);
+
+    vms->pvpanic = value;
+}
+
 static char *virt_get_gic_version(Object *obj, Error **errp)
 {
     VirtMachineState *vms = VIRT_MACHINE(obj);
@@ -1884,6 +1898,15 @@ static void virt_3_1_instance_init(Object *obj)
                                     "Valid values are none and smmuv3",
                                     NULL);
 
+    /* Default disallows pvpanic-mmio instantiation */
+    vms->pvpanic = false;
+    object_property_add_bool(obj, "pvpanic", virt_get_pvpanic,
+                             virt_set_pvpanic, NULL);
+    object_property_set_description(obj, "pvpanic",
+                                    "Set on/off to enable/disable "
+                                    "PVPANIC MMIO device",
+                                    NULL);
+
     vms->memmap = a15memmap;
     vms->irqmap = a15irqmap;
 }
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 937c124..7d6d1c0 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -113,6 +113,7 @@ typedef struct {
     bool highmem;
     bool highmem_ecam;
     bool its;
+    bool pvpanic;
     bool virt;
     int32_t gic_version;
     VirtIOMMUType iommu;
-- 
1.8.3.1

[Qemu-devel] [PATCH V11 7/8] hw/arm/virt: use the configure interface

[Peng Hao]

Use the configure interface for pvpanic-mmio.

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

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 4215ca6..4990a0d 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -785,7 +785,10 @@ 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 (vms->pvpanic) {
+        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 fdd3f20..a63e9e1 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -198,6 +198,9 @@ static void create_pvpanic_device(const VirtMachineState *vms)
     hwaddr base = vms->memmap[VIRT_PVPANIC].base;
     hwaddr size = vms->memmap[VIRT_PVPANIC].size;
 
+    if (!vms->pvpanic) {
+        return;
+    }
     sysbus_create_simple(TYPE_PVPANIC_MMIO, base, NULL);
 
     nodename = g_strdup_printf("/pvpanic-mmio@%" PRIx64, base);
-- 
1.8.3.1

[Qemu-devel] [PATCH V11 8/8] pvpanic : update pvpanic document

[Peng Hao]

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

Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 docs/specs/pvpanic.txt | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/docs/specs/pvpanic.txt b/docs/specs/pvpanic.txt
index c7bbacc..994f080 100644
--- a/docs/specs/pvpanic.txt
+++ b/docs/specs/pvpanic.txt
@@ -1,7 +1,7 @@
 PVPANIC DEVICE
 ==============
 
-pvpanic device is a simulated ISA device, through which a guest panic
+pvpanic device is a simulated device, through which a guest panic
 event is sent to qemu, and a QMP event is generated. This allows
 management apps (e.g. libvirt) to be notified and respond to the event.
 
@@ -9,6 +9,9 @@ The management app has the option of waiting for GUEST_PANICKED events,
 and/or polling for guest-panicked RunState, to learn when the pvpanic
 device has fired a panic event.
 
+The pvpanic device can be implemented as an ISA device (using IOPORT),
+or, 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], where currently only the first byte is used.
+
 ACPI Interface
 --------------
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH V11 0/8] add pvpanic mmio support

[peng.hao2]

>On Mon, 3 Dec 2018 at 11:04, Peng Hao <peng.hao2@zte.com.cn> wrote:
>>
>> 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
>
>I would still prefer to see a more detailed examination of whether
>we can do this with a PCI device before we commit to taking the
>MMIO version into the virt board.

I'm sorry I thought I had sent an email. yesterday when I wrote an email to 
explain the reason, I was interrupted and forgot to send it out.

Now the pvpanic device is implemented as a mmio device or an ACPI device in the kernel, 
and only one device can be seen at the same time. If the kernel parses FDT first, then pvpanic  
is a mmio device. The kernel parses ACPI table first(and virtual machine is configured with ACPI), 
and pvpanic  is an ACPI device.
If pvpanic is implemented as a PCI device, then the PCI device must still be seen when the ACPI table 
is first parsed by the kernel, because ACPI device relies on the mmio space of the PCI device.
Mmio devices can be thought of as just an address space rather than a device in the strict sense.

Secondly, I don't want it to be a pluggable device. If the user deletes the device by mistake, it may lead to unpredictable results.

thanks.
>thanks
>-- PMM

Re: [Qemu-devel] [PATCH V11 0/8] add pvpanic mmio support

[Peter Maydell]

On Tue, 4 Dec 2018 at 00:41, <peng.hao2@zte.com.cn> wrote:
>
> >I would still prefer to see a more detailed examination of whether
> >we can do this with a PCI device before we commit to taking the
> >MMIO version into the virt board.
>
> I'm sorry I thought I had sent an email. yesterday when I wrote an email to
> explain the reason, I was interrupted and forgot to send it out.
>
> Now the pvpanic device is implemented as a mmio device or an ACPI device in the kernel,
> and only one device can be seen at the same time. If the kernel parses FDT first, then pvpanic
> is a mmio device. The kernel parses ACPI table first(and virtual machine is configured with ACPI),
> and pvpanic  is an ACPI device.
> If pvpanic is implemented as a PCI device, then the PCI device must still be seen when the ACPI table
> is first parsed by the kernel, because ACPI device relies on the mmio space of the PCI device.
> Mmio devices can be thought of as just an address space rather than a device in the strict sense.

I'm afraid I don't understand. If it's a PCI device then
it does not need to be listed in the device tree or the
ACPI tables at all, because it is probeable by the guest.
This also significantly simplifies the changes needed in QEMU.

> Secondly, I don't want it to be a pluggable device. If the user
> deletes the device by mistake, it may lead to unpredictable results.

If the user deletes the PCI device they're using for their
disk or networking this will also lead to unpredictable
results. We expect users not to randomly unplug things from
their system if they want it to continue to work. In any
case your guest driver can easily handle the unplug: the
guest would then just lose the ability to notify on panic,
falling back to as if the pvpanic device had never been
present.

thanks
-- PMM

Re: [Qemu-devel] [PATCH V11 0/8] add pvpanic mmio support

[Andrew Jones]

On Tue, Dec 04, 2018 at 09:40:07AM +0000, Peter Maydell wrote:
> On Tue, 4 Dec 2018 at 00:41, <peng.hao2@zte.com.cn> wrote:
> >
> > >I would still prefer to see a more detailed examination of whether
> > >we can do this with a PCI device before we commit to taking the
> > >MMIO version into the virt board.
> >
> > I'm sorry I thought I had sent an email. yesterday when I wrote an email to
> > explain the reason, I was interrupted and forgot to send it out.
> >
> > Now the pvpanic device is implemented as a mmio device or an ACPI device in the kernel,
> > and only one device can be seen at the same time. If the kernel parses FDT first, then pvpanic
> > is a mmio device. The kernel parses ACPI table first(and virtual machine is configured with ACPI),
> > and pvpanic  is an ACPI device.
> > If pvpanic is implemented as a PCI device, then the PCI device must still be seen when the ACPI table
> > is first parsed by the kernel, because ACPI device relies on the mmio space of the PCI device.
> > Mmio devices can be thought of as just an address space rather than a device in the strict sense.
> 
> I'm afraid I don't understand. If it's a PCI device then
> it does not need to be listed in the device tree or the
> ACPI tables at all, because it is probeable by the guest.
> This also significantly simplifies the changes needed in QEMU.
> 
> > Secondly, I don't want it to be a pluggable device. If the user
> > deletes the device by mistake, it may lead to unpredictable results.
> 
> If the user deletes the PCI device they're using for their
> disk or networking this will also lead to unpredictable
> results. We expect users not to randomly unplug things from
> their system if they want it to continue to work. In any
> case your guest driver can easily handle the unplug: the
> guest would then just lose the ability to notify on panic,
> falling back to as if the pvpanic device had never been
> present.
>

To muddy the waters a bit more, while I'm not opposed to this device
being a PCI device, there is a chance that someone will still want a
platform-mmio version as well. I'm not sure how everything will
eventually fall into place, but I've seen some super minimal guest
configs proposed for the VMs-used-like-containers use cases, even
configs that choose to use virtio-mmio over virtio-pci, and then not
provide a PCI bus at all to the vm.

Maybe this series and the current kernel series can be allowed to
continue as they are, and if later there's a demand for a pci version,
it could just be yet another variant added later?

Thanks,
drew

Re: [Qemu-devel] [PATCH V11 0/8] add pvpanic mmio support

[Daniel P. Berrangé]

On Mon, Dec 03, 2018 at 06:50:03PM +0000, Peter Maydell wrote:
> On Mon, 3 Dec 2018 at 11:04, Peng Hao <peng.hao2@zte.com.cn> wrote:
> >
> > 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
> 
> I would still prefer to see a more detailed examination of whether
> we can do this with a PCI device before we commit to taking the
> MMIO version into the virt board.

The original design rational for using an I/O port is mentioned here,
though it is quite brief:

  http://lists.gnu.org/archive/html/qemu-devel/2012-10/msg04361.html

[quote]
We have three solutions to implement this feature:
1. use vmcall
2. use I/O port
3. use virtio-serial.

We have decided to avoid touching hypervisor. The reason why I choose
choose the I/O port is:
1. it is easier to implememt
2. it does not depend any virtual device
3. it can work when starting the kernel
[/quote]

Later postings then exposed the port via ACPI

  http://lists.nongnu.org/archive/html/qemu-devel/2013-03/msg02293.html

After it had merged there were some changes and the question of turning
it into a PCI device was raised. Paolo was concerned that the guest OS
is in an unknown state (arbitrary locks held, data structures corrupt,
etc) when panic is fired, so simplicity of the I/O port was desirable:

  https://lists.gnu.org/archive/html/qemu-devel/2013-08/msg03309.html

Anthony countered that even a PCI device could simply do an outb() in
config space:

  https://lists.gnu.org/archive/html/qemu-devel/2013-08/msg03325.html

So it is not clear using a PCI device is in fact a problem in terms of
reliability at time of firing.


Perhaps more relevant is the question of how easily it can be initialized,
as that affects whether it can be used for panics during very early boot,
or from firmware:

  https://lists.gnu.org/archive/html/qemu-devel/2013-08/msg03296.html

Finally, there is also the point that PCI slots are precious, and this
is something to be enabled out of the box on all VMs, so you'd be removing
one extra PCI slot from general usage. Thus mgmt apps would need to start
adding PCI bridges sooner. Not a blocker but something to bear in mind
when weighing up options.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH V11 0/8] add pvpanic mmio support

[Peter Maydell]

On Tue, 4 Dec 2018 at 12:05, Andrew Jones <drjones@redhat.com> wrote:
> To muddy the waters a bit more, while I'm not opposed to this device
> being a PCI device, there is a chance that someone will still want a
> platform-mmio version as well. I'm not sure how everything will
> eventually fall into place, but I've seen some super minimal guest
> configs proposed for the VMs-used-like-containers use cases, even
> configs that choose to use virtio-mmio over virtio-pci, and then not
> provide a PCI bus at all to the vm.
>
> Maybe this series and the current kernel series can be allowed to
> continue as they are, and if later there's a demand for a pci version,
> it could just be yet another variant added later?

I like the PCI device idea because it avoids adding yet
another complication to the virt board. Adding the MMIO
variant now and the PCI device later doesn't let us avoid
the complication or even remove it in the future.

The last I heard we were still thinking about deprecating
virtio-mmio entirely.

thanks
-- PMM

Re: [Qemu-devel] [PATCH V11 0/8] add pvpanic mmio support

[Peter Maydell]

On Tue, 4 Dec 2018 at 12:47, Daniel P. Berrangé <berrange@redhat.com> wrote:
> After it had merged there were some changes and the question of turning
> it into a PCI device was raised. Paolo was concerned that the guest OS
> is in an unknown state (arbitrary locks held, data structures corrupt,
> etc) when panic is fired, so simplicity of the I/O port was desirable:
>
>   https://lists.gnu.org/archive/html/qemu-devel/2013-08/msg03309.html
>
> Anthony countered that even a PCI device could simply do an outb() in
> config space:
>
>   https://lists.gnu.org/archive/html/qemu-devel/2013-08/msg03325.html
>
> So it is not clear using a PCI device is in fact a problem in terms of
> reliability at time of firing.

...and if we'd done it that way in the first place for x86 then
we wouldn't be having to do anything at all now for Arm.
That suggests to me that we should do it that way now, and then we
can avoid having to do a bunch of extra development work for the
next architecture, or the next interesting Arm board model.

> Perhaps more relevant is the question of how easily it can be initialized,
> as that affects whether it can be used for panics during very early boot,
> or from firmware:
>
>   https://lists.gnu.org/archive/html/qemu-devel/2013-08/msg03296.html

Mmm, firmware and early boot panic is potentially an interesting
use case. Does UEFI actually make use of it, though? A quick look
at the kernel source for the x86 pvpanic driver suggests it doesn't
take any particular steps to ensure that it is initialized early,
though I guess acpi drivers probably init before PCI.

I notice also that there's a mention in that thread that the pvpanic
ACPI table entry on x86 resulted in unhelpful Windows notifications
about new devices it didn't understand. Is that going to be an issue
for Arm with this mmio pvpanic ?

> Finally, there is also the point that PCI slots are precious, and this
> is something to be enabled out of the box on all VMs, so you'd be removing
> one extra PCI slot from general usage. Thus mgmt apps would need to start
> adding PCI bridges sooner. Not a blocker but something to bear in mind
> when weighing up options.

Space in the 'virt' memory map for random MMIO devices is also a limited
resource, especially if you want something in the <4GB space.

thanks
-- PMM

Re: [Qemu-devel] [PATCH V11 0/8] add pvpanic mmio support

[Paolo Bonzini]

On 04/12/18 13:59, Peter Maydell wrote:
> On Tue, 4 Dec 2018 at 12:47, Daniel P. Berrangé <berrange@redhat.com> wrote:
>> After it had merged there were some changes and the question of turning
>> it into a PCI device was raised. Paolo was concerned that the guest OS
>> is in an unknown state (arbitrary locks held, data structures corrupt,
>> etc) when panic is fired, so simplicity of the I/O port was desirable:
>>
>>   https://lists.gnu.org/archive/html/qemu-devel/2013-08/msg03309.html

(Actually that was Marcelo, I was just relaying the message).

>> Anthony countered that even a PCI device could simply do an outb() in
>> config space:
>>
>>   https://lists.gnu.org/archive/html/qemu-devel/2013-08/msg03325.html
>>
>> So it is not clear using a PCI device is in fact a problem in terms of
>> reliability at time of firing.
> 
> ...and if we'd done it that way in the first place for x86 then
> we wouldn't be having to do anything at all now for Arm.
> That suggests to me that we should do it that way now, and then we
> can avoid having to do a bunch of extra development work for the
> next architecture, or the next interesting Arm board model.

Is there any case where we have anything but ISA and MMIO?  (We have
8250 which is ISA, PCI and MMIO, but that's kind of special because PCI
is only there for hotpluggability.  pvpanic hotplug is not interesting).

Also, while reusing code in general is nice, sometimes there are
platform-specific ways to do it.  For ARM, for example, would it make
sense to use an HVC/SMC that "extends" the PSCI, and pass the number in
the PSCI device tree node?

Related to this, is there a more or less "standard" watchdog device on
ARM that could be added to virt?  There is the SBSA watchdog, but it's
ugly for implementation in KVM because it counts down with frequency
equal to CNTFRQ (which I'm not sure if QEMU has access too, and also it
doesn't play well with live migration).

> I notice also that there's a mention in that thread that the pvpanic
> ACPI table entry on x86 resulted in unhelpful Windows notifications
> about new devices it didn't understand. Is that going to be an issue
> for Arm with this mmio pvpanic ?

Yes, it is probably the same as for x86.

Paolo

Re: [Qemu-devel] [PATCH V11 0/8] add pvpanic mmio support

[Peter Maydell]

On Tue, 4 Dec 2018 at 13:30, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 04/12/18 13:59, Peter Maydell wrote:
> > ...and if we'd done it that way in the first place for x86 then
> > we wouldn't be having to do anything at all now for Arm.
> > That suggests to me that we should do it that way now, and then we
> > can avoid having to do a bunch of extra development work for the
> > next architecture, or the next interesting Arm board model.
>
> Is there any case where we have anything but ISA and MMIO?  (We have
> 8250 which is ISA, PCI and MMIO, but that's kind of special because PCI
> is only there for hotpluggability.  pvpanic hotplug is not interesting).

The point about PCI is that it is the same everywhere and
discoverable, and easy for the user to add to the system or not.
MMIO requires extra work for every board model that we want to
put the device into, plus extra on both kernel and QEMU side
for every system description mechanism (ACPI, dtb, whatever
some future architecture might use), even if we have the basic
"mmio pvpanic" device code already.

> Also, while reusing code in general is nice, sometimes there are
> platform-specific ways to do it.  For ARM, for example, would it make
> sense to use an HVC/SMC that "extends" the PSCI, and pass the number in
> the PSCI device tree node?

If you want a hypercall then these days the arm HVC calling convention
includes mechanisms for discoverably determining whether a particular
hypercall is supported, so you wouldn't need to pass anything in the
ACPI or dtb. But I didn't get the impression that anybody wanted a
hypercall for this particularly.

> Related to this, is there a more or less "standard" watchdog device on
> ARM that could be added to virt?  There is the SBSA watchdog, but it's
> ugly for implementation in KVM because it counts down with frequency
> equal to CNTFRQ (which I'm not sure if QEMU has access too, and also it
> doesn't play well with live migration).

The i6300esb is PCI, presumably that would work?

> > I notice also that there's a mention in that thread that the pvpanic
> > ACPI table entry on x86 resulted in unhelpful Windows notifications
> > about new devices it didn't understand. Is that going to be an issue
> > for Arm with this mmio pvpanic ?
>
> Yes, it is probably the same as for x86.

I guess we need to find out if that is a problem before we can
merge this, then.

thanks
-- PMM

Re: [Qemu-devel] [PATCH V11 0/8] add pvpanic mmio support

[Daniel P. Berrangé]

On Tue, Dec 04, 2018 at 12:59:51PM +0000, Peter Maydell wrote:
> On Tue, 4 Dec 2018 at 12:47, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > After it had merged there were some changes and the question of turning
> > it into a PCI device was raised. Paolo was concerned that the guest OS
> > is in an unknown state (arbitrary locks held, data structures corrupt,
> > etc) when panic is fired, so simplicity of the I/O port was desirable:
> >
> >   https://lists.gnu.org/archive/html/qemu-devel/2013-08/msg03309.html
> >
> > Anthony countered that even a PCI device could simply do an outb() in
> > config space:
> >
> >   https://lists.gnu.org/archive/html/qemu-devel/2013-08/msg03325.html
> >
> > So it is not clear using a PCI device is in fact a problem in terms of
> > reliability at time of firing.
> 
> ...and if we'd done it that way in the first place for x86 then
> we wouldn't be having to do anything at all now for Arm.
> That suggests to me that we should do it that way now, and then we
> can avoid having to do a bunch of extra development work for the
> next architecture, or the next interesting Arm board model.

On s390 there's always a panic notifier mechanism as it is a
integral part of the architecture.

On PowerPC, pSeries guests have access to a panic notifier provided
by the firmware.

On x86, as well as pvpanic, there is also a paravirtualized
option defined by the HyperV extensions "hv_crash"

IIUC a PCI based solution would be usable on x86, s390, powerpc (pseries),
aarch64 (virt) and eventually riscv (virt). Of those, it is only
aarch64 and riscv that lack a panic notifier solution today.

I feel like we've already lost from the pov of a standardized solution,
but that doesn't mean we shouldn't still consider using PCI if it does
look like the best otion for arm/riscv.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH V11 0/8] add pvpanic mmio support

[Paolo Bonzini]

On 04/12/18 14:43, Peter Maydell wrote:
> The point about PCI is that it is the same everywhere and
> discoverable, and easy for the user to add to the system or not.
> MMIO requires extra work for every board model that we want to
> put the device into, plus extra on both kernel and QEMU side
> for every system description mechanism (ACPI, dtb, whatever
> some future architecture might use), even if we have the basic
> "mmio pvpanic" device code already.

Looks like dtb is becoming a standard?  Even RISC-V switched from their
own system description to device tree.  Anyway, this is not too
important.  I agree with you about discoverability, on the other hand if
we could have something defined by the vendor rather than QEMU it would
be even better.  (Even better would be something that distro kernels
already have support for, but that would be asking too much probably).

>> Also, while reusing code in general is nice, sometimes there are
>> platform-specific ways to do it.  For ARM, for example, would it make
>> sense to use an HVC/SMC that "extends" the PSCI, and pass the number in
>> the PSCI device tree node?
> 
> If you want a hypercall then these days the arm HVC calling convention
> includes mechanisms for discoverably determining whether a particular
> hypercall is supported, so you wouldn't need to pass anything in the
> ACPI or dtb. But I didn't get the impression that anybody wanted a
> hypercall for this particularly.

Not for x86, where each hypervisor has its own hypercall number and even
its calling convention.  But for ARM it already makes more sense.

>> Related to this, is there a more or less "standard" watchdog device on
>> ARM that could be added to virt?  There is the SBSA watchdog, but it's
>> ugly for implementation in KVM because it counts down with frequency
>> equal to CNTFRQ (which I'm not sure if QEMU has access too, and also it
>> doesn't play well with live migration).
> 
> The i6300esb is PCI, presumably that would work?

Yeah, I was wondering if there was something in PrimeCell.  I found
SP805 exists now.

>>> I notice also that there's a mention in that thread that the pvpanic
>>> ACPI table entry on x86 resulted in unhelpful Windows notifications
>>> about new devices it didn't understand. Is that going to be an issue
>>> for Arm with this mmio pvpanic ?
>>
>> Yes, it is probably the same as for x86.
> 
> I guess we need to find out if that is a problem before we can
> merge this, then.

As long as the pvpanic device is not added by default it's okay.

Paolo

Re: [Qemu-devel] [PATCH V11 0/8] add pvpanic mmio support

[Michael S. Tsirkin]

On Tue, Dec 04, 2018 at 02:53:26PM +0100, Paolo Bonzini wrote:
> On 04/12/18 14:43, Peter Maydell wrote:
> > The point about PCI is that it is the same everywhere and
> > discoverable, and easy for the user to add to the system or not.
> > MMIO requires extra work for every board model that we want to
> > put the device into, plus extra on both kernel and QEMU side
> > for every system description mechanism (ACPI, dtb, whatever
> > some future architecture might use), even if we have the basic
> > "mmio pvpanic" device code already.
> 
> Looks like dtb is becoming a standard?  Even RISC-V switched from their
> own system description to device tree.  Anyway, this is not too
> important.  I agree with you about discoverability, on the other hand if
> we could have something defined by the vendor rather than QEMU it would
> be even better.  (Even better would be something that distro kernels
> already have support for, but that would be asking too much probably).
> 
> >> Also, while reusing code in general is nice, sometimes there are
> >> platform-specific ways to do it.  For ARM, for example, would it make
> >> sense to use an HVC/SMC that "extends" the PSCI, and pass the number in
> >> the PSCI device tree node?
> > 
> > If you want a hypercall then these days the arm HVC calling convention
> > includes mechanisms for discoverably determining whether a particular
> > hypercall is supported, so you wouldn't need to pass anything in the
> > ACPI or dtb. But I didn't get the impression that anybody wanted a
> > hypercall for this particularly.
> 
> Not for x86, where each hypervisor has its own hypercall number and even
> its calling convention.  But for ARM it already makes more sense.
> 
> >> Related to this, is there a more or less "standard" watchdog device on
> >> ARM that could be added to virt?  There is the SBSA watchdog, but it's
> >> ugly for implementation in KVM because it counts down with frequency
> >> equal to CNTFRQ (which I'm not sure if QEMU has access too, and also it
> >> doesn't play well with live migration).
> > 
> > The i6300esb is PCI, presumably that would work?
> 
> Yeah, I was wondering if there was something in PrimeCell.  I found
> SP805 exists now.
> 
> >>> I notice also that there's a mention in that thread that the pvpanic
> >>> ACPI table entry on x86 resulted in unhelpful Windows notifications
> >>> about new devices it didn't understand. Is that going to be an issue
> >>> for Arm with this mmio pvpanic ?
> >>
> >> Yes, it is probably the same as for x86.
> > 
> > I guess we need to find out if that is a problem before we can
> > merge this, then.
> 
> As long as the pvpanic device is not added by default it's okay.
> 
> Paolo

It isn't too bad, you just include a driver and then it's happy.

Isn't a device just for panic going too far though?
Should it be some kind of PV device that we
can use to add more pv stuff in the future, with
a panic option?

-- 
MST

Re: [Qemu-devel] [PATCH V11 0/8] add pvpanic mmio support

[peng.hao2]

>On Tue, 4 Dec 2018 at 00:41, <peng.hao2@zte.com.cn> wrote:
>>
>> >I would still prefer to see a more detailed examination of whether
>> >we can do this with a PCI device before we commit to taking the
>> >MMIO version into the virt board.
>>
>> I'm sorry I thought I had sent an email. yesterday when I wrote an email to
>> explain the reason, I was interrupted and forgot to send it out.
>>
>> Now the pvpanic device is implemented as a mmio device or an ACPI device in the kernel,
>> and only one device can be seen at the same time. If the kernel parses FDT first, then pvpanic
>> is a mmio device. The kernel parses ACPI table first(and virtual machine is configured with ACPI),
>> and pvpanic  is an ACPI device.
>> If pvpanic is implemented as a PCI device, then the PCI device must still be seen when the ACPI table
>> is first parsed by the kernel, because ACPI device relies on the mmio space of the PCI device.
>> Mmio devices can be thought of as just an address space rather than a device in the strict sense.
>
>I'm afraid I don't understand. If it's a PCI device then
>it does not need to be listed in the device tree or the
>ACPI tables at all, because it is probeable by the guest.
>This also significantly simplifies the changes needed in QEMU.
>

It is precisely because PCI devices can not be controlled by FDT or ACPI tables, 
I do not want to implement it as a pci device.
X86/pvpanic is implemented as ISA device in QEMU and ACPI device in kernel. 
My implementation extends the implementation of x86/pvpanic, and a large of x86/pvpanic 
codes are reused.If PCI devices are implemented in qemu, then ACPI devices and PCI 
devices may appear simultaneously in the kernel. This would add both devices to the 
crash notifier list, which is odd. I want to see only one device at any time. Of course, many
architectures can use PCI devices, but we are currently reusing x86/pvpanic code as much 
as possible in qemu and kernel , rather than reimplementing it. At the same time, 
backward compatibility also needs to be considered.

                                     pvpanic in guest kernel
ARM:   ACPI table         acpi device
            FDT                  mmio device  (start guest bypassing uefi)
x86      ACPI table         acpi device

>> Secondly, I don't want it to be a pluggable device. If the user
>> deletes the device by mistake, it may lead to unpredictable results.
>
>If the user deletes the PCI device they're using for their
>disk or networking this will also lead to unpredictable
>results. We expect users not to randomly unplug things from
>their system if they want it to continue to work. In any
>case your guest driver can easily handle the unplug: the
>guest would then just lose the ability to notify on panic,
>falling back to as if the pvpanic device had never been
>present.

If two devices can exist simultaneously by modifying the code,
 then because ACPI devices rely on a PCI device, if PCI devices are dynamically
 unplugged, ACPI device will not work when panic is triggered.

thanks.
>
>thanks
>-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH V11 0/8] add pvpanic mmio support

[Peter Maydell]

On Wed, 5 Dec 2018 at 00:28, <peng.hao2@zte.com.cn> wrote:
>
> >I'm afraid I don't understand. If it's a PCI device then
> >it does not need to be listed in the device tree or the
> >ACPI tables at all, because it is probeable by the guest.
> >This also significantly simplifies the changes needed in QEMU.
> >
>
> It is precisely because PCI devices can not be controlled by FDT or ACPI tables,
> I do not want to implement it as a pci device.
> X86/pvpanic is implemented as ISA device in QEMU and ACPI device in kernel.
> My implementation extends the implementation of x86/pvpanic, and a large of x86/pvpanic
> codes are reused.If PCI devices are implemented in qemu, then ACPI devices and PCI
> devices may appear simultaneously in the kernel. This would add both devices to the
> crash notifier list, which is odd. I want to see only one device at any time.

Yes, certainly we only need one pvpanic device. If it's implemented
as a PCI device, then that's what appears. We don't need and
would not implement the MMIO version. On x86 a user could
in theory use the command line to request both ISA and PCI
pvpanic devices. That would not be very sensible, but there
are lots of QEMU command lines the user can request that
don't make sense.

> Of course, many
> architectures can use PCI devices, but we are currently reusing x86/pvpanic code as much
> as possible in qemu and kernel , rather than reimplementing it. At the same time,
> backward compatibility also needs to be considered.
>
>                                      pvpanic in guest kernel
> ARM:   ACPI table         acpi device
>             FDT                  mmio device  (start guest bypassing uefi)
> x86      ACPI table         acpi device

For Arm, there is no backward compatibility issue, as we have
not yet implemented or shipped anything.

> >> Secondly, I don't want it to be a pluggable device. If the user
> >> deletes the device by mistake, it may lead to unpredictable results.
> >
> >If the user deletes the PCI device they're using for their
> >disk or networking this will also lead to unpredictable
> >results. We expect users not to randomly unplug things from
> >their system if they want it to continue to work. In any
> >case your guest driver can easily handle the unplug: the
> >guest would then just lose the ability to notify on panic,
> >falling back to as if the pvpanic device had never been
> >present.
>
> If two devices can exist simultaneously by modifying the code,
>  then because ACPI devices rely on a PCI device, if PCI devices are dynamically
>  unplugged, ACPI device will not work when panic is triggered.

If somebody modifies the code to QEMU or the guest kernel
such that something breaks, that's their issue to deal with.
My proposal is that we would ship:
 * a QEMU with a PCI pvpanic device (which you could plug in
   if you wanted it)
 * no changes to the Arm virt board, so nothing in the ACPI
   or device tree
 * no "mmio pvpanic" device

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH V11 0/8] add pvpanic mmio support

[peng.hao2]

>On Wed, 5 Dec 2018 at 00:28, <peng.hao2@zte.com.cn> wrote:
>>
>> >I'm afraid I don't understand. If it's a PCI device then
>> >it does not need to be listed in the device tree or the
>> >ACPI tables at all, because it is probeable by the guest.
>> >This also significantly simplifies the changes needed in QEMU.
>> >
>>
>> It is precisely because PCI devices can not be controlled by FDT or ACPI tables,
>> I do not want to implement it as a pci device.
>> X86/pvpanic is implemented as ISA device in QEMU and ACPI device in kernel.
>> My implementation extends the implementation of x86/pvpanic, and a large of x86/pvpanic
>> codes are reused.If PCI devices are implemented in qemu, then ACPI devices and PCI
>> devices may appear simultaneously in the kernel. This would add both devices to the
>> crash notifier list, which is odd. I want to see only one device at any time.
>
>Yes, certainly we only need one pvpanic device. If it's implemented
>as a PCI device, then that's what appears. We don't need and
>would not implement the MMIO version. On x86 a user could
>in theory use the command line to request both ISA and PCI
>pvpanic devices. That would not be very sensible, but there
>are lots of QEMU command lines the user can request that
>don't make sense.
>
>> Of course, many
>> architectures can use PCI devices, but we are currently reusing x86/pvpanic code as much
>> as possible in qemu and kernel , rather than reimplementing it. At the same time,
>> backward compatibility also needs to be considered.
>>
>>                                      pvpanic in guest kernel
>> ARM:   ACPI table         acpi device
>>             FDT                  mmio device  (start guest bypassing uefi)
>> x86      ACPI table         acpi device
>
>For Arm, there is no backward compatibility issue, as we have
>not yet implemented or shipped anything.
>

Sorry, the expression is not clear enough. I want to say that x86 needs backward 
compatibility if we intend to reuse the code of x86/pvpanic.

>> >> Secondly, I don't want it to be a pluggable device. If the user
>> >> deletes the device by mistake, it may lead to unpredictable results.
>> >
>> >If the user deletes the PCI device they're using for their
>> >disk or networking this will also lead to unpredictable
>> >results. We expect users not to randomly unplug things from
>> >their system if they want it to continue to work. In any
>> >case your guest driver can easily handle the unplug: the
>> >guest would then just lose the ability to notify on panic,
>> >falling back to as if the pvpanic device had never been
>> >present.
>>
>> If two devices can exist simultaneously by modifying the code,
>>  then because ACPI devices rely on a PCI device, if PCI devices are dynamically
>>  unplugged, ACPI device will not work when panic is triggered.
>
>If somebody modifies the code to QEMU or the guest kernel
>such that something breaks, that's their issue to deal with.
>My proposal is that we would ship:
>* a QEMU with a PCI pvpanic device (which you could plug in
>if you wanted it)
>* no changes to the Arm virt board, so nothing in the ACPI
>or device tree
>* no "mmio pvpanic" device

ok, I will try it.
thanks.
>
>thanks
>-- PMM