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

[Alexander Graf]

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

I'd prefer if we don't overparameterize - it'll just become a headache 
further down. Today we don't explicitly disallow anything anywhere - you 
could theoretically stick a G3 into e500plat. I don't see why we should 
start with heavy sanity checks now :).

Plus, the machine works just fine today if you don't pass in -device 
eTSEC. It's not like we're moving all devices to the new "platform bus".

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

Memory regions are allocated based on zero as well, they get mapped into 
a subregion. From a device's point of view, the regions for MMIO and 
IRQs that it sees all start at 0 relative 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 :).
> > > 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?

I've removed the id bit - hope that makes it more obvious :).

> /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. :-)

Heh, yeah, nothing impressively fancy :).


Alex