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

[Alexander Graf <agraf@suse.de>]

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 <agraf@suse.de>
>> ---
>>   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