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

[Scott Wood <scottwood@freescale.com>]

On Wed, 2014-07-02 at 19:30 +0200, Alexander Graf wrote:
> On 02.07.14 19:26, Scott Wood wrote:
> > On Wed, 2014-07-02 at 19:12 +0200, Alexander Graf wrote:
> >> On 02.07.14 00:50, Scott Wood wrote:
> >>> 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 :).
> > We theoretically support it elsewhere...  Once parameterized, it
> > shouldn't be hard to base the address for this, CCSRBAR, and similar
> > things on whether MAS7 is supported.
> 
> It gets parametrized in the machine file, CPU selection comes after 
> machine selection. So parameterizing it doesn't really solve it.

Why can't e500plat_init() look at args->cpu_model?  Or the
parameterization could take two sets of addresses, one for a 32-bit
layout and one for a 36-bit layout.  It might make sense to make this a
user-settable parameter; some OSes might not be able to handle a 36-bit
layout (or might not be as efficient at handling it) even on e500v2.
Many of the e500v2 boards can be built for either a 32 or 36 bit address
layout in U-Boot.

> However, again, I don't think we have to worry about it.

It's not a huge worry, but it'd be nice to not break it gratuitously.
If we do break it we should explicitly disallow e500v1 with e500plat.

> >>>> @@ -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.
> > OK, but why is that here but no irq_end, and no address range?  How do
> > allocations from the irq range happen?
> 
> There are 2 phases:
> 
>    1) Device association with the machine
>    2) Device tree generation
> 
> The allocation of IRQ ranges happens during the association phase. That 
> phase also updates all the hints in the devices to reflect their current 
> IRQ (and MMIO) mappings. The device tree generation phase only needs to 
> read those bits then - and add the IRQ offset to get from the "platform 
> bus IRQ range" to "MPIC IRQ range".

I think the answer to my original question is that irqs are allocated
based on zero because they go in an array, while memory regions are
allocated with their actual addresses because they don't.

> >>>> +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 :).
> > How does the plugging in work?
> >
> > It looks like all this does is increment id.
> 
> I'm not sure I understand. The plugging in is different code :). This 
> really only does increment an id. Maybe I'll just remove it if it 
> confuses you?

My confusion is that it is called sysbus_device_create_devtree(), not
sysbus_device_alloc_id().  Am I missing some sort of virtual function
mechanism here that would allow this function to be replaced?

/me looks at patch 6/6 again

Oh, you just add to this function in future patches.  I was expecting
something fancier given the QOM stuff and my misunderstanding about what
file patch 6/6 was touching. :-)

-Scott