Re: [Qemu-devel] modelling omap_gpmc with the hierarchical memory API

[Anthony Liguori <anthony@codemonkey.ws>]

On 08/02/2011 02:38 PM, Peter Maydell wrote:
> On 2 August 2011 20:11, Avi Kivity<avi@redhat.com>  wrote:
>> On 08/02/2011 09:21 PM, Peter Maydell wrote:
>>>
>>> On 2 August 2011 19:05, Avi Kivity<avi@redhat.com>    wrote:
>>>>   On 08/02/2011 08:21 PM, Peter Maydell wrote:
>>>>>   So I think we just need a sysbus_mmio_get_memoryregion()
>>>>>   (and convert the devices I need to attach to use memory
>>>>>   regions, and live with not being able to attach unconverted
>>>>>   devices).
>>>>
>>>>   I don't follow - why do we need get_memoryregion? who would call it?
>>>
>>> The machine model would call it. So you do something like
>>>   DeviceState *dev = qdev_create(NULL, "whatever");
>>>   /* Note the parallel here to the existing
>>>    *   sysbus_mmio_map(sysbus_from_qdev(dev), mmio_idx, addr);
>>>    */
>>>   MemoryRegion *mr =
>>> sysbus_mmio_get_memoryregion(sysbus_from_qdev(dev), mmio_idx);
>>>   omap_gpmc_attach(gpmc, 7, mr);
>>
>> This is where the gpmc provides the sysbus.  It doesn't need to call
>> get_memoryregion() on itself.
>
> Why should the gpmc provide a sysbus?

It shouldn't.  The bus model is broken in qdev so it has to to make it 
work today.

> It doesn't need it,
> all we need to pass it is a MemoryRegion *.

But the MemoryRegion * is your bus protocol.  While it's useful to have 
the flexibility to make arbitrary connections for things like Pins, it's 
better to use higher level interfaces that are more strongly typed.

That prevents silly thinks like trying to connect the MemoryRegion of an 
E1000 PCI card to the OMAP GPMC.

> A bus would
> imply multiple different things that could all sit on it
> at different addresses, whereas if gpmc provided 8 different
> sysbuses they'd each have either 0 or 1 child always at
> address 0.

You don't want a bus here all children are equal.  You want:

class GPMC : public Device
{
    GPMCChild *slots[8];
};

I feel your pain, but today the way we do this is by having a bus and 
moving the addressing info into the child as a qdev property.

>
>>> ie the machine model is where we wire up the subdevices
>>> to the gpmc, and at the machine model level what you have is
>>> a pointer to an entire device, so you need to be able to
>>> convert the (sysbus*, mmio_index) tuple to a MemoryRegion*.
>>
>> I believe that it is in general unnecessary.  A device hands its bus a
>> memory region, and the bus does with it what it will (generally mapping it
>> into a container, and presenting the container to a parent bus).
>>   get_memoryregion() implies a third party.
>
> The third party here is the machine model. The machine model
> owns and instantiates both gpmc and the subdevice. It wants
> to wire them up. In the same way that you can use qdev_get_gpio_in
> and qdev_connect_gpio_out to connect a gpio line from one thing
> to another, you need to be able to connect a memory region
> from one thing to another.

I don't think GPIO is a good example to use because it suffers from the 
same problem (too low level of an interface).

Regards,

Anthony Liguori

>
> -- PMM
>