Re: [Qemu-devel] [PATCH v3 6/6] hw/arm/virt: Support dynamically spawned sysbus devices

[Eric Auger]

On 09/09/2014 01:11 PM, Paolo Bonzini wrote:
> Il 09/09/2014 09:54, Eric Auger ha scritto:
>> Allows sysbus devices to be instantiated from command line by
>> using -device option
>>
>> Signed-off-by: Alexander Graf <address@hidden>
>> Signed-off-by: Eric Auger <address@hidden>
>>
>> ---
>>
>> v2 -> v3
>> - renaming of arm_platform_bus_create_devtree and arm_load_dtb
>> - add copyright in hw/arm/dyn_sysbus_devtree.c
>>
>> v1 -> v2:
>> - remove useless vfio-platform.h include file
>> - s/MACHVIRT_PLATFORM_HOLE/MACHVIRT_PLATFORM_SIZE
>> - use dyn_sysbus_binding and dyn_sysbus_devtree
>> - dynamic sysbus platform buse size shrinked to 4MB and
>>   moved between RTC and MMIO
>>
>> v1:
>>
>> Inspired from what Alex Graf did in ppc e500
>> https://lists.gnu.org/archive/html/qemu-ppc/2014-07/msg00012.html
>> ---
>>  hw/arm/dyn_sysbus_devtree.c | 26 +++++++++++++++++++
>>  hw/arm/virt.c               | 62 
>> +++++++++++++++++++++++++++++++++++++++++++--
>>  2 files changed, 86 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/arm/dyn_sysbus_devtree.c b/hw/arm/dyn_sysbus_devtree.c
>> index 6375024..61e5b5f 100644
>> --- a/hw/arm/dyn_sysbus_devtree.c
>> +++ b/hw/arm/dyn_sysbus_devtree.c
>> @@ -1,7 +1,30 @@
>> +/*
>> + * ARM Platform Bus device tree generation helpers 
>> + *
>> + * Copyright (c) 2014 Linaro Limited
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2 or later, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along 
>> with
>> + * this program.  If not, see <http://www.gnu.org/licenses/>.
>> + *
>> + */
>> +
>>  #include "hw/arm/dyn_sysbus_devtree.h"
>>  #include "qemu/error-report.h"
>>  #include "sysemu/device_tree.h"
>>  
>> +/**
>> + * arm_sysbus_device_create_devtree - create the node of devices
>> + * attached to the platform bus
>> + */
>>  static int arm_sysbus_device_create_devtree(Object *obj, void *opaque)
>>  {
>>      PlatformDevtreeData *data = opaque;
>> @@ -27,6 +50,9 @@ static int arm_sysbus_device_create_devtree(Object *obj, 
>> void *opaque)
>>      return 0;
>>  }
>>  
>> +/**
>> + * arm_platform_bus_create_devtree - create the platform bus node
>> + */
>>  void arm_platform_bus_create_devtree(DynSysbusParams *params,
>>                                   void *fdt, const char *intc)
>>  {
> 
> All this should go in patch 2.  For the documentation comments, please
> follow the model of include/hw/memory.h (including putting the
> documentation in the header for public functions).
> 
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 9085b88..16acf44 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -40,6 +40,8 @@
>>  #include "exec/address-spaces.h"
>>  #include "qemu/bitops.h"
>>  #include "qemu/error-report.h"
>> +#include "hw/misc/dyn_sysbus_binding.h"
>> +#include "hw/arm/dyn_sysbus_devtree.h"
>>  
>>  #define NUM_VIRTIO_TRANSPORTS 32
>>  
>> @@ -57,6 +59,14 @@
>>  #define GIC_FDT_IRQ_PPI_CPU_START 8
>>  #define GIC_FDT_IRQ_PPI_CPU_WIDTH 8
>>  
>> +#define MACHVIRT_PLATFORM_BASE         0x9400000
>> +#define MACHVIRT_PLATFORM_SIZE         (4ULL * 1024 * 1024) /* 4 MB */
>> +#define MACHVIRT_PLATFORM_PAGE_SHIFT   12
>> +#define MACHVIRT_PLATFORM_SIZE_PAGES   (MACHVIRT_PLATFORM_SIZE >> \
>> +                                    MACHVIRT_PLATFORM_PAGE_SHIFT)
>> +#define MACHVIRT_PLATFORM_FIRST_IRQ    48
>> +#define MACHVIRT_PLATFORM_NUM_IRQS     20
>> +
>>  enum {
>>      VIRT_FLASH,
>>      VIRT_MEM,
>> @@ -66,6 +76,7 @@ enum {
>>      VIRT_UART,
>>      VIRT_MMIO,
>>      VIRT_RTC,
>> +    VIRT_PLATFORM,
>>  };
>>  
>>  typedef struct MemMapEntry {
>> @@ -105,16 +116,27 @@ static const MemMapEntry a15memmap[] = {
>>      [VIRT_GIC_CPU] =    { 0x08010000, 0x00010000 },
>>      [VIRT_UART] =       { 0x09000000, 0x00001000 },
>>      [VIRT_RTC] =        { 0x09010000, 0x00001000 },
>> +    [VIRT_PLATFORM] = {MACHVIRT_PLATFORM_BASE , MACHVIRT_PLATFORM_SIZE},
>>      [VIRT_MMIO] =       { 0x0a000000, 0x00000200 },
>>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size 
>> */
>>      /* 0x10000000 .. 0x40000000 reserved for PCI */
>> -    [VIRT_MEM] =        { 0x40000000, 30ULL * 1024 * 1024 * 1024 },
>> +    [VIRT_MEM] = { 0x40000000, 30ULL * 1024 * 1024 * 1024 },
>>  };
>>  
>>  static const int a15irqmap[] = {
>>      [VIRT_UART] = 1,
>>      [VIRT_RTC] = 2,
>>      [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
>> +    [VIRT_PLATFORM] = MACHVIRT_PLATFORM_FIRST_IRQ,
>> +};
>> +
>> +static DynSysbusParams dyn_sysbus_platform_params = {
>> +    .has_platform_bus = true,
>> +    .platform_bus_base = MACHVIRT_PLATFORM_BASE,
>> +    .platform_bus_size = MACHVIRT_PLATFORM_SIZE,
>> +    .platform_bus_first_irq = MACHVIRT_PLATFORM_FIRST_IRQ,
>> +    .platform_bus_num_irqs = MACHVIRT_PLATFORM_NUM_IRQS,
>> +    .page_shift = MACHVIRT_PLATFORM_PAGE_SHIFT,
>>  };
>>  
>>  static VirtBoardInfo machines[] = {
>> @@ -458,6 +480,18 @@ static void create_virtio_devices(const VirtBoardInfo 
>> *vbi, qemu_irq *pic)
>>      fdt_add_virtio_nodes(vbi);
>>  }
>>  
>> +static void machvirt_prep_device_tree(VirtBoardInfo *vbi)
>> +{
>> +    create_fdt(vbi);
>> +    fdt_add_timer_nodes(vbi);
>> +    fdt_add_cpu_nodes(vbi);
>> +    fdt_add_psci_node(vbi);
>> +    fdt_add_gic_node(vbi);
>> +    fdt_add_uart_node(vbi);
>> +    fdt_add_rtc_node(vbi);
>> +    fdt_add_virtio_nodes(vbi);
>> +}
>> +
>>  static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
>>  {
>>      const VirtBoardInfo *board = (const VirtBoardInfo *)binfo;
>> @@ -466,14 +500,28 @@ static void *machvirt_dtb(const struct arm_boot_info 
>> *binfo, int *fdt_size)
>>      return board->fdt;
>>  }
>>  
>> +static void machvirt_reset_device_tree(void *opaque)
>> +{
>> +    VirtBoardInfo *board = (VirtBoardInfo *)opaque;
>> +    struct arm_boot_info *info = &board->bootinfo;
>> +    hwaddr dtb_start = QEMU_ALIGN_UP(info->initrd_start + info->initrd_size,
>> +                                     4096);
>> +    machvirt_prep_device_tree(board);
>> +    arm_platform_bus_create_devtree(&dyn_sysbus_platform_params,
>> +                                board->fdt, "/intc");
> 
> Why do you have to recreate the device tree every time (as opposed to
> just doing the load)?  And if arm_load_dtb used rom_add_blob_fixed
> instead of cpu_physical_memory_write, you wouldn't need a reset hook at all.
+
"
We need to do this anyway, because currently we don't do
anything to ensure the DTB hangs around for the OS to
find again on reset, even in the simple "just boot Linux"
setups.

-- PMM
"

Hi Paolo, Peter, Ard,

I come back to this topic since I must acknowledge I do not have a good
understanding of what to do here.

My goal is to add some dt nodes (dynamic sysbus ones) after the first
device tree was created by the machine init function. at machine init
the relevant info to build the dynamic sysbus nodes are not yet
available.  I registered a reset handler to recreate the device tree
including the new nodes.

The arm_load_dtb implementation recently changed and now uses
rom_add_blob_fixed - as Paolo recommended - instead of
cpu_physical_memory_write. But now when the reset handler is called I
now get an HW error: "ROM images must be loaded at startup".

Please could you advise about how to implement what I did before with
this new implementation.

Thank you in advance

Best Regards

Eric
> 
> Paolo
> 
>> +    arm_load_dtb(dtb_start, info);
>> +}
>> +
>>  static void machvirt_init(MachineState *machine)
>>  {
>> -    qemu_irq pic[NUM_IRQS];
>> +    qemu_irq *pic = g_new(qemu_irq, NUM_IRQS);
>>      MemoryRegion *sysmem = get_system_memory();
>>      int n;
>>      MemoryRegion *ram = g_new(MemoryRegion, 1);
>>      const char *cpu_model = machine->cpu_model;
>>      VirtBoardInfo *vbi;
>> +    DynSysbusNotifier *notifier;
>>  
>>      if (!cpu_model) {
>>          cpu_model = "cortex-a15";
>> @@ -547,6 +595,13 @@ static void machvirt_init(MachineState *machine)
>>       */
>>      create_virtio_devices(vbi, pic);
>>  
>> +    notifier = g_new(DynSysbusNotifier, 1);
>> +    notifier->notifier.notify = platform_bus_init_notify;
>> +    notifier->address_space_mem = sysmem;
>> +    notifier->mpic = pic;
>> +    notifier->params = dyn_sysbus_platform_params;
>> +    qemu_add_machine_init_done_notifier(&notifier->notifier);
>> +
>>      vbi->bootinfo.ram_size = machine->ram_size;
>>      vbi->bootinfo.kernel_filename = machine->kernel_filename;
>>      vbi->bootinfo.kernel_cmdline = machine->kernel_cmdline;
>> @@ -556,6 +611,8 @@ static void machvirt_init(MachineState *machine)
>>      vbi->bootinfo.loader_start = vbi->memmap[VIRT_MEM].base;
>>      vbi->bootinfo.get_dtb = machvirt_dtb;
>>      arm_load_kernel(ARM_CPU(first_cpu), &vbi->bootinfo);
>> +
>> +    qemu_register_reset(machvirt_reset_device_tree, vbi);
>>  }
>>  
>>  static QEMUMachine machvirt_a15_machine = {
>> @@ -563,6 +620,7 @@ static QEMUMachine machvirt_a15_machine = {
>>      .desc = "ARM Virtual Machine",
>>      .init = machvirt_init,
>>      .max_cpus = 8,
>> +    .has_dynamic_sysbus = true,
>>  };
>>  
>>  static void machvirt_machine_init(void)
>>
>