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

[Alexander Graf]

On 02.07.14 00:50, Scott Wood wrote:
> 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.

Good idea. The only thing I'll leave here is the page_shift. The fact 
that we only allocate in 4k chunks is inherently implementation specific.

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

I don't think we have to worry about e500v1. I'll change it :).

> > +#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.

True - renamed to "size".

> > +
> >   /* 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?

"id" is just a linear counter over all devices in the platform bus so 
that if you need to have a unique identifier, you can have one.

"irq_start" is the offset of the first mpic irq that's connected to the 
platform bus.

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

It's not yet - it's only the stub that allows to plug in specific device 
code that then generates device tree nodes :).

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

Fair enough, will remove it :)

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

Renamed to "platform_bus" :). In fact, I'll go through the file with a 
small sweeper and try to make sure we're reasonably consistent in our 
naming.


Alex