diff mbox series

[v3,4/4] hw/arm/virt: Use the pvpanic device

Message ID 20181018130434.23237-5-philmd@redhat.com (mailing list archive)
State New, archived
Headers show
Series hw/misc: Add a MMIO interface to the pvpanic device | expand

Commit Message

Philippe Mathieu-Daudé Oct. 18, 2018, 1:04 p.m. UTC 
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(-)

Comments

Peter Maydell Oct. 18, 2018, 1:37 p.m. UTC | #1 
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
Peng Hao Oct. 18, 2018, 4:07 p.m. UTC | #2 
>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
diff mbox series

Patch

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,