Re: [Qemu-devel] [PATCH v6 3/3] hw/arm/virt: add dynamic sysbus device support

[Eric Auger <eric.auger@linaro.org>]

On 12/16/2014 06:41 AM, Shannon Zhao wrote:
> On 2014/12/9 18:29, Eric Auger wrote:
>> Allows sysbus devices to be instantiated from command line by
>> using -device option. Machvirt creates a platform bus at init.
>> The dynamic sysbus devices are attached to this platform bus device.
>>
>> The platform bus device registers a machine init done notifier
>> whose role will be to bind the dynamic sysbus devices. Indeed
>> dynamic sysbus devices are created after machine init.
>>
>> machvirt also registers a notifier that will build the device
>> tree nodes for the platform bus and its children dynamic sysbus
>> devices.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>> v5 -> v6:
>> - Take into account Peter's comments:
>>   - platform_bus_params initialized from vbi->memmap and vbi->irqmap.
>>     As a consequence, const is removed. Also alignment in a15memmap
>>     is slightly changed.
>>   - ARMPlatformBusSystemParams handle removed from create_platform_bus
>>     prototype
>> - arm_load_kernel has become a machine init done notifier registration.
>>   It must be called before platform_bus_create to guarantee the correct
>>   notifier execution sequence
>>
>> v4 -> v5:
>> - platform_bus_params becomes static const
>> - reword comment in create_platform_bus
>> - reword the commit message
>>
>> v3 -> v4:
>> - use platform bus object, instantiated in create_platform_bus
>> - device tree generation for platform bus and children dynamic
>>   sysbus devices is no more handled at reset but in a
>>   machine_init_done_notifier (due to the change in implementaion
>>   of ARM load dtb using rom_add_blob_fixed).
>> - device tree enhancement now takes into account the case of
>>   user provided dtb. Before the user dtb was overwritten which
>>   was wrong. However in case the dtb is provided by the user,
>>   dynamic sysbus nodes are not added there.
>> - renaming of MACHVIRT_PLATFORM defines
>> - MACHVIRT_PLATFORM_PAGE_SHIFT and SIZE_PAGES not needed anymore,
>>   hence removed.
>> - DynSysbusParams struct renamed into ARMPlatformBusSystemParams
>>   and above params removed.
>> - separation of dt creation and QEMU binding is not mandated anymore
>>   since the device tree is not created from scratch anymore. Instead
>>   the modify_dtb function is used.
>> - create_platform_bus registers another machine init done notifier
>>   to start VFIO IRQ handling. This latter executes after the
>>   dynamic sysbus device binding.
>>
>> 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
>>
>> Conflicts:
>> 	hw/arm/sysbus-fdt.c
>> ---
>>  hw/arm/virt.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 80 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 314e55b..366083a 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -42,6 +42,8 @@
>>  #include "exec/address-spaces.h"
>>  #include "qemu/bitops.h"
>>  #include "qemu/error-report.h"
>> +#include "hw/arm/sysbus-fdt.h"
>> +#include "hw/platform-bus.h"
>>  
>>  #define NUM_VIRTIO_TRANSPORTS 32
>>  
>> @@ -59,6 +61,9 @@
>>  #define GIC_FDT_IRQ_PPI_CPU_START 8
>>  #define GIC_FDT_IRQ_PPI_CPU_WIDTH 8
>>  
>> +#define PLATFORM_BUS_FIRST_IRQ    48
> 
> This macro is not used any more, right?
Hi Shannon,

yes you're right. I am going to remove it.
> 
>> +#define PLATFORM_BUS_NUM_IRQS     20
>> +
>>  enum {
>>      VIRT_FLASH,
>>      VIRT_MEM,
>> @@ -68,8 +73,11 @@ enum {
>>      VIRT_UART,
>>      VIRT_MMIO,
>>      VIRT_RTC,
>> +    VIRT_PLATFORM_BUS,
>>  };
>>  
>> +static ARMPlatformBusSystemParams platform_bus_params;
>> +
>>  typedef struct MemMapEntry {
>>      hwaddr base;
>>      hwaddr size;
>> @@ -100,23 +108,25 @@ typedef struct VirtBoardInfo {
>>   */
>>  static const MemMapEntry a15memmap[] = {
>>      /* Space up to 0x8000000 is reserved for a boot ROM */
>> -    [VIRT_FLASH] =      {          0, 0x08000000 },
>> -    [VIRT_CPUPERIPHS] = { 0x08000000, 0x00020000 },
>> +    [VIRT_FLASH] =          {          0, 0x08000000 },
>> +    [VIRT_CPUPERIPHS] =     { 0x08000000, 0x00020000 },
>>      /* GIC distributor and CPU interfaces sit inside the CPU peripheral space */
>> -    [VIRT_GIC_DIST] =   { 0x08000000, 0x00010000 },
>> -    [VIRT_GIC_CPU] =    { 0x08010000, 0x00010000 },
>> -    [VIRT_UART] =       { 0x09000000, 0x00001000 },
>> -    [VIRT_RTC] =        { 0x09010000, 0x00001000 },
>> -    [VIRT_MMIO] =       { 0x0a000000, 0x00000200 },
>> +    [VIRT_GIC_DIST] =       { 0x08000000, 0x00010000 },
>> +    [VIRT_GIC_CPU] =        { 0x08010000, 0x00010000 },
>> +    [VIRT_UART] =           { 0x09000000, 0x00001000 },
>> +    [VIRT_RTC] =            { 0x09010000, 0x00001000 },
>> +    [VIRT_PLATFORM_BUS] =   { 0x09400000, 0x04000000 },
> 
> The number of platform_bus_size is not right. Maybe it should be 0x00400000.

you're fully correct! thanks for spotting this mistake.

Best Regards

Eric
> 
> Thanks,
> Shannon
> 
>> +    [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_BUS] = 48,
>>  };
>>  
>>  static VirtBoardInfo machines[] = {
>> @@ -519,6 +529,47 @@ static void create_flash(const VirtBoardInfo *vbi)
>>      g_free(nodename);
>>  }
>>  
>> +static void create_platform_bus(VirtBoardInfo *vbi, qemu_irq *pic)
>> +{
>> +    DeviceState *dev;
>> +    SysBusDevice *s;
>> +    int i;
>> +    ARMPlatformBusFdtParams *fdt_params = g_new(ARMPlatformBusFdtParams, 1);
>> +    MemoryRegion *sysmem = get_system_memory();
>> +
>> +    platform_bus_params.platform_bus_base = vbi->memmap[VIRT_PLATFORM_BUS].base;
>> +    platform_bus_params.platform_bus_size = vbi->memmap[VIRT_PLATFORM_BUS].size;
>> +    platform_bus_params.platform_bus_first_irq = vbi->irqmap[VIRT_PLATFORM_BUS];
>> +    platform_bus_params.platform_bus_num_irqs = PLATFORM_BUS_NUM_IRQS;
>> +
>> +    fdt_params->system_params = &platform_bus_params;
>> +    fdt_params->binfo = &vbi->bootinfo;
>> +    fdt_params->intc = "/intc";
>> +    /*
>> +     * register a machine init done notifier that creates the device tree
>> +     * nodes of the platform bus and its children dynamic sysbus devices
>> +     */
>> +    arm_register_platform_bus_fdt_creator(fdt_params);
>> +
>> +    dev = qdev_create(NULL, TYPE_PLATFORM_BUS_DEVICE);
>> +    dev->id = TYPE_PLATFORM_BUS_DEVICE;
>> +    qdev_prop_set_uint32(dev, "num_irqs",
>> +        platform_bus_params.platform_bus_num_irqs);
>> +    qdev_prop_set_uint32(dev, "mmio_size",
>> +        platform_bus_params.platform_bus_size);
>> +    qdev_init_nofail(dev);
>> +    s = SYS_BUS_DEVICE(dev);
>> +
>> +    for (i = 0; i < platform_bus_params.platform_bus_num_irqs; i++) {
>> +        int irqn = platform_bus_params.platform_bus_first_irq + i;
>> +        sysbus_connect_irq(s, i, pic[irqn]);
>> +    }
>> +
>> +    memory_region_add_subregion(sysmem,
>> +                                platform_bus_params.platform_bus_base,
>> +                                sysbus_mmio_get_region(s, 0));
>> +}
>> +
>>  static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
>>  {
>>      const VirtBoardInfo *board = (const VirtBoardInfo *)binfo;
>> @@ -581,6 +632,24 @@ static void machvirt_init(MachineState *machine)
>>  
>>          object_property_set_bool(cpuobj, true, "realized", NULL);
>>      }
>> +
>> +    /*
>> +     * register the arm_load_kernel machine init done notifier
>> +     * before platform_bus_create call.
>> +     * There, arm_register_platform_bus_fdt_creator registers another
>> +     * notifier that adds platform bus nodes. Notifiers are executed
>> +     * in registration reverse order.
>> +     */
>> +    vbi->bootinfo.ram_size = machine->ram_size;
>> +    vbi->bootinfo.kernel_filename = machine->kernel_filename;
>> +    vbi->bootinfo.kernel_cmdline = machine->kernel_cmdline;
>> +    vbi->bootinfo.initrd_filename = machine->initrd_filename;
>> +    vbi->bootinfo.nb_cpus = smp_cpus;
>> +    vbi->bootinfo.board_id = -1;
>> +    vbi->bootinfo.loader_start = vbi->memmap[VIRT_MEM].base;
>> +    vbi->bootinfo.get_dtb = machvirt_dtb;
>> +    arm_load_kernel(ARM_CPU(first_cpu), &vbi->bootinfo);
>> +
>>      fdt_add_timer_nodes(vbi);
>>      fdt_add_cpu_nodes(vbi);
>>      fdt_add_psci_node(vbi);
>> @@ -604,15 +673,8 @@ static void machvirt_init(MachineState *machine)
>>       */
>>      create_virtio_devices(vbi, pic);
>>  
>> -    vbi->bootinfo.ram_size = machine->ram_size;
>> -    vbi->bootinfo.kernel_filename = machine->kernel_filename;
>> -    vbi->bootinfo.kernel_cmdline = machine->kernel_cmdline;
>> -    vbi->bootinfo.initrd_filename = machine->initrd_filename;
>> -    vbi->bootinfo.nb_cpus = smp_cpus;
>> -    vbi->bootinfo.board_id = -1;
>> -    vbi->bootinfo.loader_start = vbi->memmap[VIRT_MEM].base;
>> -    vbi->bootinfo.get_dtb = machvirt_dtb;
>> -    arm_load_kernel(ARM_CPU(first_cpu), &vbi->bootinfo);
>> +    create_platform_bus(vbi, pic);
>> +
>>  }
>>  
>>  static QEMUMachine machvirt_a15_machine = {
>> @@ -620,6 +682,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)
>>
> 
>