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

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

On 12/05/2014 05:36 PM, Peter Maydell wrote:
> On 30 November 2014 at 18:19, Eric Auger <eric.auger@linaro.org> 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>
>>
>> ---
>> 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 | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 57 insertions(+)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 314e55b..37326a9 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,11 @@
>>  #define GIC_FDT_IRQ_PPI_CPU_START 8
>>  #define GIC_FDT_IRQ_PPI_CPU_WIDTH 8
>>
>> +#define PLATFORM_BUS_BASE         0x9400000
>> +#define PLATFORM_BUS_SIZE         (4ULL * 1024 * 1024)
>> +#define PLATFORM_BUS_FIRST_IRQ    48
>> +#define PLATFORM_BUS_NUM_IRQS     20
>> +
>>  enum {
>>      VIRT_FLASH,
>>      VIRT_MEM,
>> @@ -68,6 +75,7 @@ enum {
>>      VIRT_UART,
>>      VIRT_MMIO,
>>      VIRT_RTC,
>> +    VIRT_PLATFORM_BUS,
>>  };
>>
>>  typedef struct MemMapEntry {
>> @@ -107,6 +115,7 @@ static const MemMapEntry a15memmap[] = {
>>      [VIRT_GIC_CPU] =    { 0x08010000, 0x00010000 },
>>      [VIRT_UART] =       { 0x09000000, 0x00001000 },
>>      [VIRT_RTC] =        { 0x09010000, 0x00001000 },
>> +    [VIRT_PLATFORM_BUS] = {PLATFORM_BUS_BASE , PLATFORM_BUS_SIZE},
> 
> This makes it pretty hard to read this -- you should use
> the raw 0x numbers here. Anywhere else that wants to know
> the base address etc should fish it out of the memory
> map at runtime, as we do with the other devices.
Hi Peter

ok. As a side effect platform_bus_params will not be a const anymore,
previously recommended by Alex.
> 
>>      [VIRT_MMIO] =       { 0x0a000000, 0x00000200 },
>>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>>      /* 0x10000000 .. 0x40000000 reserved for PCI */
>> @@ -117,6 +126,14 @@ static const int a15irqmap[] = {
>>      [VIRT_UART] = 1,
>>      [VIRT_RTC] = 2,
>>      [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
>> +    [VIRT_PLATFORM_BUS] = PLATFORM_BUS_FIRST_IRQ,
> 
> Similarly with interrupt numbers.
ok
Thanks
Eric
> 
> -- PMM
>