diff mbox

[v8,3/3] hw/arm/virt: add dynamic sysbus device support

Message ID 1420474453-10058-4-git-send-email-eric.auger@linaro.org
State New
Headers show

Commit Message

Auger Eric Jan. 5, 2015, 4:14 p.m. UTC 
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>

---
v7 -> v8:
- rebase on 2.2.0
- in machvirt_init, create_platform_bus simply is added
  after the arm_load_kernel call instead of moving this latter.
  Related comment slighly reworded.
- Due to those changes I dropped Alex and Shannon's Reviewed-by

v6 -> v7:
Take into account Shannon comments:
- remove PLATFORM_BUS_FIRST_IRQ macro
- correct platform bus size to 0x400000
- add an additional comment in a15irqmap related to
  PLATFORM_BUS_NUM_IRQS

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

Conflicts:
	hw/arm/virt.c
---
 hw/arm/virt.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 68 insertions(+), 9 deletions(-)

Comments

Peter Maydell Jan. 6, 2015, 6:57 p.m. UTC | #1 
On 5 January 2015 at 16:14, 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.

> @@ -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
Auger Eric Jan. 7, 2015, 5:08 p.m. UTC | #2 
Hi Peter,
On 01/06/2015 07:57 PM, Peter Maydell wrote:
> On 5 January 2015 at 16:14, 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.
> 
>> @@ -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.
sure I will create another patch file dedicated to that
> 
>> +    [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.
Then I will start at 64.

About PLATFORM_BUS_NUM_IRQS= 20, I must acknowledge it is an arbitrary
figure. Alex used 10 in e500 as default value.

If I am not wrong a DMA330 can have up to 32 IRQ. It is another
candidate for VFIO. Would it be OK to use 32 instead of 20?

About memory, here we have space for 64 64kB pages. any other suggestions?

>  we have
>>  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?
Well I could deallocate in platform_bus_fdt_notify routine if you
recommend to do so.

Thanks for the review.

Eric
> 
> thanks
> -- PMM
>
diff mbox

Patch

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 2353440..40f4f0f 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,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 },
+    [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*/
 };
 
 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);
+    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;
@@ -658,6 +708,14 @@  static void machvirt_init(MachineState *machine)
     vbi->bootinfo.get_dtb = machvirt_dtb;
     vbi->bootinfo.firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
     arm_load_kernel(ARM_CPU(first_cpu), &vbi->bootinfo);
+
+    /*
+     * arm_load_kernel machine init done notifier registration must
+     * happen before the platform_bus_create call. In this latter,
+     * another notifier is registered which adds platform bus nodes.
+     * Notifiers are executed in registration reverse order.
+     */
+    create_platform_bus(vbi, pic);
 }
 
 static bool virt_get_secure(Object *obj, Error **errp)
@@ -696,6 +754,7 @@  static void virt_class_init(ObjectClass *oc, void *data)
     mc->desc = "ARM Virtual Machine",
     mc->init = machvirt_init;
     mc->max_cpus = 8;
+    mc->has_dynamic_sysbus = true;
 }
 
 static const TypeInfo machvirt_info = {