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

[peng.hao2]

>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.
>
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("/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.
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