Re: [Qemu-ppc] [Qemu-devel] [PATCH 5/6] PPC: e500: Support dynamically spawned sysbus devices

[Scott Wood]

On Tue, 2014-07-01 at 23:49 +0200, Alexander Graf wrote:
> For e500 our approach to supporting dynamically spawned sysbus devices is to
> create a simple bus from the guest's point of view within which we map those
> devices dynamically.
> 
> We allocate memory regions always within the "platform" hole in address
> space and map IRQs to predetermined IRQ lines that are reserved for platform
> device usage.
> 
> This maps really nicely into device tree logic, so we can just tell the
> guest about our virtual simple bus in device tree as well.
> 
> Signed-off-by: Alexander Graf <address@hidden>
> ---
>  hw/ppc/e500.c     | 251 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/ppc/e500.h     |   1 +
>  hw/ppc/e500plat.c |   1 +
>  3 files changed, 253 insertions(+)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index bb2e75f..bf704b0 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -36,6 +36,7 @@
>  #include "exec/address-spaces.h"
>  #include "qemu/host-utils.h"
>  #include "hw/pci-host/ppce500.h"
> +#include "qemu/error-report.h"
>  
>  #define EPAPR_MAGIC                (0x45504150)
>  #define BINARY_DEVICE_TREE_FILE    "mpc8544ds.dtb"
> @@ -47,6 +48,14 @@
>  
>  #define RAM_SIZES_ALIGN            (64UL << 20)
>  
> +#define E500_PLATFORM_BASE         0xF0000000ULL

Should the IRQ and address range be parameterized?  Even if the platform
bus is going to be restricted to only e500plat, it seems like it would
be good to keep all assumptions that are e500plat-specific inside the
e500plat file.

Plus, let's please not hardcode any more addresses that are going to be
a problem for giving guests a large amount of RAM (yes, CCSRBAR is also
blocking that, but that has a TODO to parameterize).  How about
0xf00000000ULL?  Unless of course we're emulating an e500v1, which
doesn't support 36-bit physical addressing.  Parameterization would help
there as well.

> +#define E500_PLATFORM_HOLE         (128ULL * 1024 * 1024) /* 128 MB */
> +#define E500_PLATFORM_PAGE_SHIFT   12
> +#define E500_PLATFORM_HOLE_PAGES   (E500_PLATFORM_HOLE >> \
> +                                    E500_PLATFORM_PAGE_SHIFT)
> +#define E500_PLATFORM_FIRST_IRQ    5
> +#define E500_PLATFORM_NUM_IRQS     10

What is the "hole"?  If that's meant to be the size to go along with
E500_PLATFORM_BASE, that seems like odd terminology.

> +
>  /* TODO: parameterize */
>  #define MPC8544_CCSRBAR_BASE       0xE0000000ULL
>  #define MPC8544_CCSRBAR_SIZE       0x00100000ULL
> @@ -122,6 +131,77 @@ static void dt_serial_create(void *fdt, unsigned long 
> long offset,
>      }
>  }
>  
> +typedef struct PlatformDevtreeData {
> +    void *fdt;
> +    const char *mpic;
> +    int irq_start;
> +    const char *node;
> +    int id;
> +} PlatformDevtreeData;

What is id?  How does irq_start work?

> +static int sysbus_device_create_devtree(Object *obj, void *opaque)
> +{
> +    PlatformDevtreeData *data = opaque;
> +    Object *dev;
> +    SysBusDevice *sbdev;
> +    bool matched = false;
> +
> +    dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE);
> +    sbdev = (SysBusDevice *)dev;
> +
> +    if (!sbdev) {
> +        /* Container, traverse it for children */
> +        return object_child_foreach(obj, sysbus_device_create_devtree, data);
> +    }
> +
> +    if (matched) {
> +        data->id++;
> +    } else {
> +        error_report("Device %s is not supported by this machine yet.",
> +                     qdev_fw_name(DEVICE(dev)));
> +        exit(1);
> +    }
> +
> +    return 0;
> +}

It's not clear to me how this function is creating a device tree node.

> +
> +static void platform_create_devtree(void *fdt, const char *node, uint64_t 
> addr,
> +                                    const char *mpic, int irq_start,
> +                                    int nr_irqs)
> +{
> +    const char platcomp[] = "qemu,platform\0simple-bus";
> +    PlatformDevtreeData data;
> +    Object *container;
> +
> +    /* Create a /platform node that we can put all devices into */
> +
> +    qemu_fdt_add_subnode(fdt, node);
> +    qemu_fdt_setprop(fdt, node, "compatible", platcomp, sizeof(platcomp));
> +    qemu_fdt_setprop_string(fdt, node, "device_type", "platform");

Where did this device_type come from?

device_type is deprecated and new uses should not be introduced.

> diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
> index 08b25fa..3a588ed 100644
> --- a/hw/ppc/e500.h
> +++ b/hw/ppc/e500.h
> @@ -11,6 +11,7 @@ typedef struct PPCE500Params {
>      void (*fixup_devtree)(struct PPCE500Params *params, void *fdt);
>  
>      int mpic_version;
> +    bool has_platform;
>  } PPCE500Params;

It would be clearer to refer to this as "platform bus", "platform
devices", or similar rather than just "platform" -- the latter is
confusing relative to existing use of the word "platform", such as
e500plat.c.

-Scott