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é <address@hidden> wrote:
> Signed-off-by: Peng Hao <address@hidden>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> [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("/address@hidden" 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