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

[Peter Maydell]

On 30 November 2014 at 18:19, Eric Auger <address@hidden> 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 <address@hidden>
> Signed-off-by: Eric Auger <address@hidden>
>
> ---
> 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.

>      [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.

-- PMM