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

[Alexander Graf <agraf@suse.de>]

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