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

[Peter Maydell]

On 5 January 2015 at 16:14, 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.

> @@ -59,6 +61,8 @@
>  #define GIC_FDT_IRQ_PPI_CPU_START 8
>  #define GIC_FDT_IRQ_PPI_CPU_WIDTH 8
>
> +#define PLATFORM_BUS_NUM_IRQS     20
> +
>  enum {
>      VIRT_FLASH,
>      VIRT_MEM,
> @@ -69,8 +73,11 @@ enum {
>      VIRT_MMIO,
>      VIRT_RTC,
>      VIRT_FW_CFG,
> +    VIRT_PLATFORM_BUS,
>  };
>
> +static ARMPlatformBusSystemParams platform_bus_params;
> +
>  typedef struct MemMapEntry {
>      hwaddr base;
>      hwaddr size;
> @@ -119,24 +126,26 @@ typedef struct {
>   */
>  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_FW_CFG] =     { 0x09020000, 0x0000000a },
> -    [VIRT_MMIO] =       { 0x0a000000, 0x00000200 },
> +    [VIRT_GIC_DIST] =       { 0x08000000, 0x00010000 },
> +    [VIRT_GIC_CPU] =        { 0x08010000, 0x00010000 },
> +    [VIRT_UART] =           { 0x09000000, 0x00001000 },
> +    [VIRT_RTC] =            { 0x09010000, 0x00001000 },
> +    [VIRT_FW_CFG] =         { 0x09020000, 0x0000000a },

Please don't re-indent unrelated lines in the same patch: it makes
it very hard to tell whether any of them have actually changed.

> +    [VIRT_PLATFORM_BUS] =   { 0x09400000, 0x00400000 },
> +    [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, /* .. to 48 + PLATFORM_BUS_NUM_IRQS -1*/

Missing spaces again (also use '...to' for consistency with line above).

>  };

This patch generally looks OK, so I think the major question is:
are we happy with this memory and IRQ usage?

Why 20 for PLATFORM_BUS_NUM_IRQS? It seems a funny number.
Starting the platform bus IRQs at 48 means there is no scope
at all for raising NUM_VIRTIO_TRANSPORTS later, which is
not really what I had in mind, though in fact it seems
unlikely that we'll ever really want to do that given the
imminent advent of PCI. Still, I don't think it would
hurt to start at 64.

>  static VirtBoardInfo machines[] = {
> @@ -556,6 +565,47 @@ static void create_fw_cfg(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);

...does the arm_register_platform_bus_fdt_creator() function
implicitly agree to g_free() the pointer it's passed when it's
done with it, or are we just leaking this?

thanks
-- PMM