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

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

[Peter Maydell]

I'm trying to update omap_gpmc to (a) support OMAP3/Beagle features
and (b) use sysbus/qdev rather than ad-hocery, and I'm having
difficulty figuring out how it fits into the new memory API.

Specifically, omap_gpmc lets you attach up to 8 devices to its chip
selects, and has registers which specify base address and size for
each attached device. I want the attached device to be an arbitrary
sysbus device, because some boards use this (for instance the overo
board hangs a lan9118 off the gpmc, and the n8x0 connects up the
tusb6010; the other usual attached devices are nand and onenand).

This kind of "I want to manage the memory layout of a pile of other
things" seems like what the hierarchical memory API should provide,
but there's a bit of a difficulty here in that sysbus MMIOs aren't
necessarily MemoryRegions (and sysbus doesn't provide a "MemoryRegion*
sysbus_get_memoryregion(SysBusDevice dev, int regionnumber)"
anyway). How are we planning to move forward here? Will all sysbus
MMIOs eventually be converted to MemoryRegions?

Secondly, I'm not sure how to implement the gpmc size registers with
the memory API: memory_region_add_subregion() lets you specify the
offset you want to put the subregion at, but doesn't provide any way
to say "limit the size of the mapping to this many bytes, regardless
of how big the underlying device thinks it is".

(My pre-memory-API version of these changes implemented a new
sysbus_mmio_resize(), but you can't restrict the size of a
MemoryRegion based sysbus MMIO.)

So maybe I'm approaching the problem wrong -- how should I be doing
this?

[More detailed summary of how the GPMC base and size registers work:
the base register gives the top 6 bits of the base address, and there
is also a 6 bit mask register which effectively gives the size of the
region: the device is selected if (address[31:26] & mask) == base. If
the device is smaller than the specified size then it just repeats
throughout the region.  Note that you can specify funny masks that
give 'holes' in the region, although the TRM says not to do that
:-). Access to a region which was programmed to map to two different
chip selects generates a bus error instead. I don't think we need to
model the finer details of holes, repeat-through-region or overlap if
that is too tricky, though.]

thanks
-- PMM

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

[Avi Kivity]

On 08/02/2011 06:47 PM, Peter Maydell wrote:
> I'm trying to update omap_gpmc to (a) support OMAP3/Beagle features
> and (b) use sysbus/qdev rather than ad-hocery, and I'm having
> difficulty figuring out how it fits into the new memory API.
>
> Specifically, omap_gpmc lets you attach up to 8 devices to its chip
> selects, and has registers which specify base address and size for
> each attached device. I want the attached device to be an arbitrary
> sysbus device, because some boards use this (for instance the overo
> board hangs a lan9118 off the gpmc, and the n8x0 connects up the
> tusb6010; the other usual attached devices are nand and onenand).
>
> This kind of "I want to manage the memory layout of a pile of other
> things" seems like what the hierarchical memory API should provide,
> but there's a bit of a difficulty here in that sysbus MMIOs aren't
> necessarily MemoryRegions (and sysbus doesn't provide a "MemoryRegion*
> sysbus_get_memoryregion(SysBusDevice dev, int regionnumber)"
> anyway). How are we planning to move forward here? Will all sysbus
> MMIOs eventually be converted to MemoryRegions?

Yes.

>
> Secondly, I'm not sure how to implement the gpmc size registers with
> the memory API: memory_region_add_subregion() lets you specify the
> offset you want to put the subregion at, but doesn't provide any way
> to say "limit the size of the mapping to this many bytes, regardless
> of how big the underlying device thinks it is".

You can interpose you own container region:


  system_memory
      |
      +--- cs_region-0
              |
              +--- device-connected-to-that-region

cs-region-0 will clip anything under it.


>
> (My pre-memory-API version of these changes implemented a new
> sysbus_mmio_resize(), but you can't restrict the size of a
> MemoryRegion based sysbus MMIO.)
>
> So maybe I'm approaching the problem wrong -- how should I be doing
> this?

I don't think those devices should be connected to the sysbus (since 
they aren't on real hardware).  Connect them to your gpmc instead.  If 
the devices are already designed for sysbus, maybe we can dual-bus them, 
or make gpmc have eight sysbuses hanging off it.

>
> [More detailed summary of how the GPMC base and size registers work:
> the base register gives the top 6 bits of the base address, and there
> is also a 6 bit mask register which effectively gives the size of the
> region: the device is selected if (address[31:26]&  mask) == base. If
> the device is smaller than the specified size then it just repeats
> throughout the region.  Note that you can specify funny masks that
> give 'holes' in the region, although the TRM says not to do that
> :-). Access to a region which was programmed to map to two different
> chip selects generates a bus error instead. I don't think we need to
> model the finer details of holes, repeat-through-region or overlap if
> that is too tricky, though.]
>
> thanks
> -- PMM


-- 
error compiling committee.c: too many arguments to function

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

[Peter Maydell]

On 2 August 2011 16:58, Avi Kivity <avi@redhat.com> wrote:
> On 08/02/2011 06:47 PM, Peter Maydell wrote:
>> This kind of "I want to manage the memory layout of a pile of other
>> things" seems like what the hierarchical memory API should provide,
>> but there's a bit of a difficulty here in that sysbus MMIOs aren't
>> necessarily MemoryRegions (and sysbus doesn't provide a "MemoryRegion*
>> sysbus_get_memoryregion(SysBusDevice dev, int regionnumber)"
>> anyway). How are we planning to move forward here? Will all sysbus
>> MMIOs eventually be converted to MemoryRegions?
>
> Yes.

Cool. I guess this means struct SysBusDevice's struct { ... } mmio[]
will turn into a MemoryRegion* mmio[] ?

>> Secondly, I'm not sure how to implement the gpmc size registers with
>> the memory API: memory_region_add_subregion() lets you specify the
>> offset you want to put the subregion at, but doesn't provide any way
>> to say "limit the size of the mapping to this many bytes, regardless
>> of how big the underlying device thinks it is".
>
> You can interpose you own container region:
>
>
>  system_memory
>     |
>     +--- cs_region-0
>             |
>             +--- device-connected-to-that-region
>
> cs-region-0 will clip anything under it.

OK, and when we change the size of CS0 we delete the container
region, create a new one of the new size and reconnect the
device-region to the new container? That's a bit clumsy but it
will work.

>> So maybe I'm approaching the problem wrong -- how should I be doing
>> this?
>
> I don't think those devices should be connected to the sysbus (since they
> aren't on real hardware).  Connect them to your gpmc instead.  If the
> devices are already designed for sysbus, maybe we can dual-bus them, or make
> gpmc have eight sysbuses hanging off it.

Actually I think in an ideal world omap_gpmc_attach() would
simply take a MemoryRegion* :
 void omap_gpmc_attach(DeviceState *gpmc, int cs,
                       MemoryRegion *subdevice)

(for "NOR-like" devices, with a second separate one for NAND-like
devices:
 void omap_gpmc_attach_nand(DeviceState *gpmc, int cs,
                            DeviceState *nanddevice)
...because for a NAND like device we need to do nand_setpins(),
nand_setio(), etc, but for a NOR-like device we just need to map
its memory.)

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

Then in some future new-qemu-object-model world when devices
just directly expose their MemoryRegions it will be easy to
just pass mydevice->registers to omap_gpmc_attach().

[That is, the only reason I'm passing SysBus objects around
is that at the moment that is the only useful abstraction we
have for saying "I'm an arbitrary device object and I provide
some GPIO pins and some memory mappable regions". MemoryRegion*
allows me to pass around a memory mappable region in a more
direct way than having to pass a (SysBus*, mmio_index) tuple.]

-- PMM

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

[Avi Kivity]

On 08/02/2011 08:21 PM, Peter Maydell wrote:
> On 2 August 2011 16:58, Avi Kivity<avi@redhat.com>  wrote:
> >  On 08/02/2011 06:47 PM, Peter Maydell wrote:
> >>  This kind of "I want to manage the memory layout of a pile of other
> >>  things" seems like what the hierarchical memory API should provide,
> >>  but there's a bit of a difficulty here in that sysbus MMIOs aren't
> >>  necessarily MemoryRegions (and sysbus doesn't provide a "MemoryRegion*
> >>  sysbus_get_memoryregion(SysBusDevice dev, int regionnumber)"
> >>  anyway). How are we planning to move forward here? Will all sysbus
> >>  MMIOs eventually be converted to MemoryRegions?
> >
> >  Yes.
>
> Cool. I guess this means struct SysBusDevice's struct { ... } mmio[]
> will turn into a MemoryRegion* mmio[] ?
>

Not all.  Some use a single mmio region to map multiple subregions at 
unrelated addresses, so we have to use a callback.  If someone takes the 
time to split them into multiple regions, we can remove the callbacks (I 
think there are two or three users).  However, it has to be done by 
someone who understands the implications; I limited myself to mechanical 
transformations.

It's particularly annoying as the mmio regions are magically mapped to a 
vararg constructor function, or something.

> >>  Secondly, I'm not sure how to implement the gpmc size registers with
> >>  the memory API: memory_region_add_subregion() lets you specify the
> >>  offset you want to put the subregion at, but doesn't provide any way
> >>  to say "limit the size of the mapping to this many bytes, regardless
> >>  of how big the underlying device thinks it is".
> >
> >  You can interpose you own container region:
> >
> >
> >    system_memory
> >       |
> >       +--- cs_region-0
> >               |
> >               +--- device-connected-to-that-region
> >
> >  cs-region-0 will clip anything under it.
>
> OK, and when we change the size of CS0 we delete the container
> region, create a new one of the new size and reconnect the
> device-region to the new container? That's a bit clumsy but it
> will work.

Yes.  I plan to introduce more general dynamic clipping, but only after 
the dust settles.

> >>  So maybe I'm approaching the problem wrong -- how should I be doing
> >>  this?
> >
> >  I don't think those devices should be connected to the sysbus (since they
> >  aren't on real hardware).  Connect them to your gpmc instead.  If the
> >  devices are already designed for sysbus, maybe we can dual-bus them, or make
> >  gpmc have eight sysbuses hanging off it.
>
> Actually I think in an ideal world omap_gpmc_attach() would
> simply take a MemoryRegion* :
>   void omap_gpmc_attach(DeviceState *gpmc, int cs,
>                         MemoryRegion *subdevice)

Yup.

> (for "NOR-like" devices, with a second separate one for NAND-like
> devices:
>   void omap_gpmc_attach_nand(DeviceState *gpmc, int cs,
>                              DeviceState *nanddevice)
> ...because for a NAND like device we need to do nand_setpins(),
> nand_setio(), etc, but for a NOR-like device we just need to map
> its memory.)
>
> 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?

> Then in some future new-qemu-object-model world when devices
> just directly expose their MemoryRegions it will be easy to
> just pass mydevice->registers to omap_gpmc_attach().
>
> [That is, the only reason I'm passing SysBus objects around
> is that at the moment that is the only useful abstraction we
> have for saying "I'm an arbitrary device object and I provide
> some GPIO pins and some memory mappable regions". MemoryRegion*
> allows me to pass around a memory mappable region in a more
> direct way than having to pass a (SysBus*, mmio_index) tuple.]

I think I see.  Perhaps you're describing qdev/MemoryRegion integration.

(vote starts now for renaming MemoryRegion -> Memory; or perhaps Aspace 
for Address Space).

-- 
error compiling committee.c: too many arguments to function

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

[Jan Kiszka]

On 2011-08-02 19:21, Peter Maydell wrote:
> On 2 August 2011 16:58, Avi Kivity <avi@redhat.com> wrote:
>> On 08/02/2011 06:47 PM, Peter Maydell wrote:
>>> This kind of "I want to manage the memory layout of a pile of other
>>> things" seems like what the hierarchical memory API should provide,
>>> but there's a bit of a difficulty here in that sysbus MMIOs aren't
>>> necessarily MemoryRegions (and sysbus doesn't provide a "MemoryRegion*
>>> sysbus_get_memoryregion(SysBusDevice dev, int regionnumber)"
>>> anyway). How are we planning to move forward here? Will all sysbus
>>> MMIOs eventually be converted to MemoryRegions?
>>
>> Yes.
> 
> Cool. I guess this means struct SysBusDevice's struct { ... } mmio[]
> will turn into a MemoryRegion* mmio[] ?
> 
>>> Secondly, I'm not sure how to implement the gpmc size registers with
>>> the memory API: memory_region_add_subregion() lets you specify the
>>> offset you want to put the subregion at, but doesn't provide any way
>>> to say "limit the size of the mapping to this many bytes, regardless
>>> of how big the underlying device thinks it is".
>>
>> You can interpose you own container region:
>>
>>
>>  system_memory
>>     |
>>     +--- cs_region-0
>>             |
>>             +--- device-connected-to-that-region
>>
>> cs-region-0 will clip anything under it.
> 
> OK, and when we change the size of CS0 we delete the container
> region, create a new one of the new size and reconnect the
> device-region to the new container? That's a bit clumsy but it
> will work.

That's why I was asking for a memory_region_update service + region
description via some struct, not (only) via function arguments.

> 
>>> So maybe I'm approaching the problem wrong -- how should I be doing
>>> this?
>>
>> I don't think those devices should be connected to the sysbus (since they
>> aren't on real hardware).  Connect them to your gpmc instead.  If the
>> devices are already designed for sysbus, maybe we can dual-bus them, or make
>> gpmc have eight sysbuses hanging off it.
> 
> Actually I think in an ideal world omap_gpmc_attach() would
> simply take a MemoryRegion* :
>  void omap_gpmc_attach(DeviceState *gpmc, int cs,
>                        MemoryRegion *subdevice)
> 
> (for "NOR-like" devices, with a second separate one for NAND-like
> devices:
>  void omap_gpmc_attach_nand(DeviceState *gpmc, int cs,
>                             DeviceState *nanddevice)
> ...because for a NAND like device we need to do nand_setpins(),
> nand_setio(), etc, but for a NOR-like device we just need to map
> its memory.)
> 
> 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).
> 
> Then in some future new-qemu-object-model world when devices
> just directly expose their MemoryRegions it will be easy to
> just pass mydevice->registers to omap_gpmc_attach().
> 
> [That is, the only reason I'm passing SysBus objects around
> is that at the moment that is the only useful abstraction we
> have for saying "I'm an arbitrary device object and I provide
> some GPIO pins and some memory mappable regions". MemoryRegion*
> allows me to pass around a memory mappable region in a more
> direct way than having to pass a (SysBus*, mmio_index) tuple.]

And that's why we need GPIO/IRQ services at qdev level, not exclusively
for sysbus club members.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

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

[Peter Maydell]

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);

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

>> [That is, the only reason I'm passing SysBus objects around
>> is that at the moment that is the only useful abstraction we
>> have for saying "I'm an arbitrary device object and I provide
>> some GPIO pins and some memory mappable regions". MemoryRegion*
>> allows me to pass around a memory mappable region in a more
>> direct way than having to pass a (SysBus*, mmio_index) tuple.]
>
> I think I see.  Perhaps you're describing qdev/MemoryRegion integration.

I think qdev devices need to be able to expose MemoryRegions
as first class named 'properties' or 'plugs' or 'sockets' or
whatever we want to call them, yes. (Ditto gpio/irq, which at
the moment we can kind of expose but not by name.)

-- PMM

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

[Avi Kivity]

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.

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

> >>  [That is, the only reason I'm passing SysBus objects around
> >>  is that at the moment that is the only useful abstraction we
> >>  have for saying "I'm an arbitrary device object and I provide
> >>  some GPIO pins and some memory mappable regions". MemoryRegion*
> >>  allows me to pass around a memory mappable region in a more
> >>  direct way than having to pass a (SysBus*, mmio_index) tuple.]
> >
> >  I think I see.  Perhaps you're describing qdev/MemoryRegion integration.
>
> I think qdev devices need to be able to expose MemoryRegions
> as first class named 'properties' or 'plugs' or 'sockets' or
> whatever we want to call them, yes. (Ditto gpio/irq, which at
> the moment we can kind of expose but not by name.)

Let's hope some sucker gets volunteered into it.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

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

[Avi Kivity]

On 08/02/2011 09:07 PM, Jan Kiszka wrote:
> >>
> >>
> >>   system_memory
> >>      |
> >>      +--- cs_region-0
> >>              |
> >>              +--- device-connected-to-that-region
> >>
> >>  cs-region-0 will clip anything under it.
> >
> >  OK, and when we change the size of CS0 we delete the container
> >  region, create a new one of the new size and reconnect the
> >  device-region to the new container? That's a bit clumsy but it
> >  will work.
>
> That's why I was asking for a memory_region_update service + region
> description via some struct, not (only) via function arguments.

Maybe we can do something like

   memory_region_init(&myregion, (MemoryDesc) {
       .size = blah,
       .foo = bar,
       })

and use the same structure for updates.

>  And that's why we need GPIO/IRQ services at qdev level, not exclusively
>  for sysbus club members.

Is there any sysbus thing which doesn't need to become a qdev thing?  I'd guess that the only sysbus property that doesn't need to become part of qdev is its singletonness.


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

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

[Peter Maydell]

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 doesn't need it,
all we need to pass it is a MemoryRegion *. 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.

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

-- PMM

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

[Anthony Liguori]

On 08/02/2011 01: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);
>
> 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*.

Hrm, this looks like badness to me.

You're effectively using MemoryRegions to implement an ad-hoc interface. 
  This is not what MemoryRegions are meant to do though.  You want 
something like:

class WhateverDevice : public Device, implements SimpleDevice
{
    MemoryRegion *get_memory_region(void);
};

class OmapGmc : public Device
{
    SimpleDevice *slots[8];
};

In qdev of today, you should implement something other than SysBus as a 
base class and make OmapGmc a bus.

>
>>> [That is, the only reason I'm passing SysBus objects around
>>> is that at the moment that is the only useful abstraction we
>>> have for saying "I'm an arbitrary device object and I provide
>>> some GPIO pins and some memory mappable regions". MemoryRegion*
>>> allows me to pass around a memory mappable region in a more
>>> direct way than having to pass a (SysBus*, mmio_index) tuple.]
>>
>> I think I see.  Perhaps you're describing qdev/MemoryRegion integration.
>
> I think qdev devices need to be able to expose MemoryRegions
> as first class named 'properties' or 'plugs' or 'sockets' or
> whatever we want to call them, yes. (Ditto gpio/irq, which at
> the moment we can kind of expose but not by name.)

I disagree in this case.  I think MemoryRegion is a bit too low level of 
a connecting point.  I think an interface would be a stronger interface 
to use.

What's the relationship between the omap_gpmc and the devices in real 
hardware?  Are the devices designed to connect to the GPMC explicitly 
via a common set of pins?  Is there an intermediate bridge chip or 
something?

Regards,

Anthony Liguori

>
> -- PMM
>

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

[Anthony Liguori]

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
>

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

[Anthony Liguori]

On 08/02/2011 01:07 PM, Jan Kiszka wrote:
> On 2011-08-02 19:21, Peter Maydell wrote:
>> On 2 August 2011 16:58, Avi Kivity<avi@redhat.com>  wrote:
>>> On 08/02/2011 06:47 PM, Peter Maydell wrote:
>>>> This kind of "I want to manage the memory layout of a pile of other
>>>> things" seems like what the hierarchical memory API should provide,
>>>> but there's a bit of a difficulty here in that sysbus MMIOs aren't
>>>> necessarily MemoryRegions (and sysbus doesn't provide a "MemoryRegion*
>>>> sysbus_get_memoryregion(SysBusDevice dev, int regionnumber)"
>>>> anyway). How are we planning to move forward here? Will all sysbus
>>>> MMIOs eventually be converted to MemoryRegions?
>>>
>>> Yes.
>>
>> Cool. I guess this means struct SysBusDevice's struct { ... } mmio[]
>> will turn into a MemoryRegion* mmio[] ?
>>
>>>> Secondly, I'm not sure how to implement the gpmc size registers with
>>>> the memory API: memory_region_add_subregion() lets you specify the
>>>> offset you want to put the subregion at, but doesn't provide any way
>>>> to say "limit the size of the mapping to this many bytes, regardless
>>>> of how big the underlying device thinks it is".
>>>
>>> You can interpose you own container region:
>>>
>>>
>>>   system_memory
>>>      |
>>>      +--- cs_region-0
>>>              |
>>>              +--- device-connected-to-that-region
>>>
>>> cs-region-0 will clip anything under it.
>>
>> OK, and when we change the size of CS0 we delete the container
>> region, create a new one of the new size and reconnect the
>> device-region to the new container? That's a bit clumsy but it
>> will work.
>
> That's why I was asking for a memory_region_update service + region
> description via some struct, not (only) via function arguments.
>
>>
>>>> So maybe I'm approaching the problem wrong -- how should I be doing
>>>> this?
>>>
>>> I don't think those devices should be connected to the sysbus (since they
>>> aren't on real hardware).  Connect them to your gpmc instead.  If the
>>> devices are already designed for sysbus, maybe we can dual-bus them, or make
>>> gpmc have eight sysbuses hanging off it.
>>
>> Actually I think in an ideal world omap_gpmc_attach() would
>> simply take a MemoryRegion* :
>>   void omap_gpmc_attach(DeviceState *gpmc, int cs,
>>                         MemoryRegion *subdevice)
>>
>> (for "NOR-like" devices, with a second separate one for NAND-like
>> devices:
>>   void omap_gpmc_attach_nand(DeviceState *gpmc, int cs,
>>                              DeviceState *nanddevice)
>> ...because for a NAND like device we need to do nand_setpins(),
>> nand_setio(), etc, but for a NOR-like device we just need to map
>> its memory.)
>>
>> 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).
>>
>> Then in some future new-qemu-object-model world when devices
>> just directly expose their MemoryRegions it will be easy to
>> just pass mydevice->registers to omap_gpmc_attach().
>>
>> [That is, the only reason I'm passing SysBus objects around
>> is that at the moment that is the only useful abstraction we
>> have for saying "I'm an arbitrary device object and I provide
>> some GPIO pins and some memory mappable regions". MemoryRegion*
>> allows me to pass around a memory mappable region in a more
>> direct way than having to pass a (SysBus*, mmio_index) tuple.]
>
> And that's why we need GPIO/IRQ services at qdev level, not exclusively
> for sysbus club members.

The qdev level should be the common base that makes sense for *all* qdev 
devices.  IRQ management does not belong in DeviceState because what you 
do for a simple LCD is not what you do for an MSI-X capable PCI device.

This is what QOM properties tries to address.  It should be possible to 
create a simple device, and register plugs/sockets for GPIO pins without 
pushing GPIO knowledge into the base class.

In a QDev world, the right approach is to have a GpioDevice base class 
that implements this sort of logic for devices where it makes sense. 
That's what SysBusDevice sort of wants to be but it somehow ended up as 
yet another base class for everything.

Regards,

Anthony Liguori

> Jan
>

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

[Avi Kivity]

On 08/02/2011 10: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 doesn't need it,
> all we need to pass it is a MemoryRegion *. 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.

The way I see it:

    cpu --> sysbus ---> gpmc ---> [ devices ]

If qdev supports memory regions, we can model this directly.  If not, we 
go via sysbus:

    cpu -> sysbus -> gpmc ---> [ sysbus ---> device ]s

(where the various sysbuses are all different instances)

having both the devices and gpmc sit on the same sysbus doesn't make 
sense to me.

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

So the machine model gives the device its bus (either a qbus embedded in 
gpmc, or a sysbus embedded in gpmc), and the device registers itself in 
its bus.  The machine model doesn't need to stitch individual memory 
regions.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

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

[Peter Maydell]

On 2 August 2011 21:56, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Hrm, this looks like badness to me.
>
> You're effectively using MemoryRegions to implement an ad-hoc interface.

As you'll see below, the hardware intreface is somewhat ad-hoc :-)

>  This is not what MemoryRegions are meant to do though.  You want something
> like:
>
> class WhateverDevice : public Device, implements SimpleDevice
> {
>   MemoryRegion *get_memory_region(void);
> };
>
> class OmapGmc : public Device
> {
>   SimpleDevice *slots[8];
> };
>
> In qdev of today, you should implement something other than SysBus as a base
> class and make OmapGmc a bus.

Doesn't this then preclude connecting the memory region of an
existing sysbus device model into the OmapGpmc bus? After all,
it wouldn't be implementing SimpleDevice.

> What's the relationship between the omap_gpmc and the devices in real
> hardware?  Are the devices designed to connect to the GPMC explicitly via a
> common set of pins?  Is there an intermediate bridge chip or something?

The external interface (ie the one to the outside of the OMAP3 SOC,
into which a board can wire devices) looks like this:

 gpmc_a[10:1]   output    Address (note no bit zero!)
 gpmc_d[15:0]   i/o       Data
 gpmc_ncs[7:0]  output    Chip selects
 gpmc_nadv_ale  output    Address valid (or address latch enable for NAND)
 gpmc_noe_nre   output    Output enable (or read enable for NAND)
 gpmc_nwe       output    Write enable
 gpmc_nbe0_cle  output    Lower byte enable (also command latch enable for NAND)
 gpmc_nbe1      output    Byte 1 enable
 gpmc_nwp       output    Write protect
 gpmc_io_dir    output    gpmc_d[15:0] direction control (input vs output)

...which you can then use in a number of different modes by programming
the GPMC appropriately:
 * multiplexed address-and-data NOR-like device:
   gpmc_d[15:0] are a 16 bit data bus and also used for addressing:
   gpmc_a[10:1] + gpmc_d[15:0] together give an address bus A26..A1
   (no A0, so you can't byte-address, only 16-bit-word address)
 * non-multiplexed NOR like device:
   gpmc_d[15:0] are a 16 bit data bus only
   gpmc_a[10:1] are A10..A1 address bus (again, no A0)
 * 16 bit NAND device wired up to gpmc_d[15:0], with the NAND
   control pins being gpmc_nadv_ale etc as above
 * 8 bit NAND device, as 16 bit NAND but only using gpmc_d[7:0]

Assuming we aren't going to actually try to model this at a pin
level, this disentangles into talking to one of two interfaces:
 * a NAND device (which at the moment nand.c implements via
   nand_setpins/nand_getpins/nand_setio/nand_getio)
 * a random addressable memory region

Typically in the latter case the device we're talking to will
also provide some gpio or irq signals, which will be routed in
a totally different direction having nothing to do with the GPMC.
So it's important that we should be able to take an existing
device model and route its registers through the GPMC and its
GPIO elsewhere.

-- PMM

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

[Avi Kivity]

On 08/03/2011 12:06 AM, Anthony Liguori wrote:
>
> The qdev level should be the common base that makes sense for *all* 
> qdev devices.  IRQ management does not belong in DeviceState because 
> what you do for a simple LCD is not what you do for an MSI-X capable 
> PCI device.
>
> This is what QOM properties tries to address.  It should be possible 
> to create a simple device, and register plugs/sockets for GPIO pins 
> without pushing GPIO knowledge into the base class.
>
> In a QDev world, the right approach is to have a GpioDevice base class 
> that implements this sort of logic for devices where it makes sense. 
> That's what SysBusDevice sort of wants to be but it somehow ended up 
> as yet another base class for everything.
>

Doesn't this end up requiring multiple inheritance, and a ton of 
boilerplate in addition?

I'm in favour of throwing everything into qdev so it's easy to use.  If 
a device doesn't have gpio, why, 0 is a valid integer.


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

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

[Avi Kivity]

On 08/03/2011 12:28 AM, Peter Maydell wrote:
> On 2 August 2011 21:56, Anthony Liguori<anthony@codemonkey.ws>  wrote:
> >  Hrm, this looks like badness to me.
> >
> >  You're effectively using MemoryRegions to implement an ad-hoc interface.
>
> As you'll see below, the hardware intreface is somewhat ad-hoc :-)
>

IMO it fits perfectly into a qdev which contains 8 qbuses, each of which 
is connected to another qdev.

The plan would be:
- fold sysbus into qdev
- improve qdev/qbus memory region support
- reimplement PCI BARs atop qdev regions

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

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

[Peter Maydell]

On 2 August 2011 22:48, Avi Kivity <avi@redhat.com> wrote:
> On 08/03/2011 12:28 AM, Peter Maydell wrote:
>> As you'll see below, the hardware intreface is somewhat ad-hoc :-)
>
> IMO it fits perfectly into a qdev which contains 8 qbuses, each of which is
> connected to another qdev.

Yeah. I don't much like the way qdev forces everything into
this bus-based mould, but on the other hand I'd like to get
some of this stuff landed this year, and it's not that awful
a fit in this case, I guess.

-- PMM

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

[Anthony Liguori]

On 08/02/2011 04:28 PM, Peter Maydell wrote:
> On 2 August 2011 21:56, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>> Hrm, this looks like badness to me.
>>
>> You're effectively using MemoryRegions to implement an ad-hoc interface.
>
> As you'll see below, the hardware intreface is somewhat ad-hoc :-)
>
>>   This is not what MemoryRegions are meant to do though.  You want something
>> like:
>>
>> class WhateverDevice : public Device, implements SimpleDevice
>> {
>>    MemoryRegion *get_memory_region(void);
>> };
>>
>> class OmapGmc : public Device
>> {
>>    SimpleDevice *slots[8];
>> };
>>
>> In qdev of today, you should implement something other than SysBus as a base
>> class and make OmapGmc a bus.
>
> Doesn't this then preclude connecting the memory region of an
> existing sysbus device model into the OmapGpmc bus? After all,
> it wouldn't be implementing SimpleDevice.

Yes, that's a feature not a bug.

>> What's the relationship between the omap_gpmc and the devices in real
>> hardware?  Are the devices designed to connect to the GPMC explicitly via a
>> common set of pins?  Is there an intermediate bridge chip or something?
>
> The external interface (ie the one to the outside of the OMAP3 SOC,
> into which a board can wire devices) looks like this:
>
>   gpmc_a[10:1]   output    Address (note no bit zero!)
>   gpmc_d[15:0]   i/o       Data
>   gpmc_ncs[7:0]  output    Chip selects
>   gpmc_nadv_ale  output    Address valid (or address latch enable for NAND)
>   gpmc_noe_nre   output    Output enable (or read enable for NAND)
>   gpmc_nwe       output    Write enable
>   gpmc_nbe0_cle  output    Lower byte enable (also command latch enable for NAND)
>   gpmc_nbe1      output    Byte 1 enable
>   gpmc_nwp       output    Write protect
>   gpmc_io_dir    output    gpmc_d[15:0] direction control (input vs output)
>
> ...which you can then use in a number of different modes by programming
> the GPMC appropriately:
>   * multiplexed address-and-data NOR-like device:
>     gpmc_d[15:0] are a 16 bit data bus and also used for addressing:
>     gpmc_a[10:1] + gpmc_d[15:0] together give an address bus A26..A1
>     (no A0, so you can't byte-address, only 16-bit-word address)
>   * non-multiplexed NOR like device:
>     gpmc_d[15:0] are a 16 bit data bus only
>     gpmc_a[10:1] are A10..A1 address bus (again, no A0)
>   * 16 bit NAND device wired up to gpmc_d[15:0], with the NAND
>     control pins being gpmc_nadv_ale etc as above
>   * 8 bit NAND device, as 16 bit NAND but only using gpmc_d[7:0]
>
> Assuming we aren't going to actually try to model this at a pin
> level, this disentangles into talking to one of two interfaces:
>   * a NAND device (which at the moment nand.c implements via
>     nand_setpins/nand_getpins/nand_setio/nand_getio)
>   * a random addressable memory region
>

So you basically have a OMAP3 SOC bus.  You can't connect devices 
directly to it, you need some sort of bridge to bridge existing devices 
to this bus.

It's no different than something like ISA on the classic PC.

> Typically in the latter case the device we're talking to will
> also provide some gpio or irq signals, which will be routed in
> a totally different direction having nothing to do with the GPMC.
> So it's important that we should be able to take an existing
> device model and route its registers through the GPMC and its
> GPIO elsewhere.

So I think the modelling disconnect here is that you're trying to model 
the wire connections to hook up arbitrary devices to a OMAP SOC.

Unless you're modelling each pin (which I don't think you're doing), I 
think it makes more sense to treat this as a bus and model accordingly.

Regards,

Anthony Liguori

>
> -- PMM
>

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

[Anthony Liguori]

On 08/02/2011 04:48 PM, Avi Kivity wrote:
> On 08/03/2011 12:28 AM, Peter Maydell wrote:
>> On 2 August 2011 21:56, Anthony Liguori<anthony@codemonkey.ws> wrote:
>> > Hrm, this looks like badness to me.
>> >
>> > You're effectively using MemoryRegions to implement an ad-hoc
>> interface.
>>
>> As you'll see below, the hardware intreface is somewhat ad-hoc :-)
>>
>
> IMO it fits perfectly into a qdev which contains 8 qbuses, each of which
> is connected to another qdev.

Do you mean a qdev that is a bus and accepts up to 8 devices as children?

Regards,

Anthony Liguori

> The plan would be:
> - fold sysbus into qdev
> - improve qdev/qbus memory region support
> - reimplement PCI BARs atop qdev regions
>

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

[Anthony Liguori]

On 08/02/2011 04:29 PM, Avi Kivity wrote:
> On 08/03/2011 12:06 AM, Anthony Liguori wrote:
>>
>> The qdev level should be the common base that makes sense for *all*
>> qdev devices. IRQ management does not belong in DeviceState because
>> what you do for a simple LCD is not what you do for an MSI-X capable
>> PCI device.
>>
>> This is what QOM properties tries to address. It should be possible to
>> create a simple device, and register plugs/sockets for GPIO pins
>> without pushing GPIO knowledge into the base class.
>>
>> In a QDev world, the right approach is to have a GpioDevice base class
>> that implements this sort of logic for devices where it makes sense.
>> That's what SysBusDevice sort of wants to be but it somehow ended up
>> as yet another base class for everything.
>>
>
> Doesn't this end up requiring multiple inheritance, and a ton of
> boilerplate in addition?

I don't see a ton of boiler plate nor do I see multiple inheritance. 
Either you have devices that live on a bus that interact with other 
devices via the bus interface or you have devices that interact through 
Pin interactions.

But pushing everything into qdev is wrong.  This is a classic trap with 
object oriented modelling.  Inheritance is supposed to model is-a 
relationships, not "may-implement in some subclasses".  Not only does it 
cause unnecessary bloat, it causes brittleness.

What in the world would DeviceState::num_gpio_out mean for a PCI device? 
  It doesn't make sense at all which means it doesn't belong in DeviceState.

Unless we're modelling the pin inputs and outputs for every device that 
we possibly support..  But we're in for a world of hurt if that's what 
our goals are.  Connecting PCI devices to their busses will be, 
interesting, to say the least :-)

Regards,

Anthony Liguori


>
> I'm in favour of throwing everything into qdev so it's easy to use. If a
> device doesn't have gpio, why, 0 is a valid integer.
>
>

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

[Avi Kivity]

On 08/03/2011 05:26 AM, Anthony Liguori wrote:
> On 08/02/2011 04:48 PM, Avi Kivity wrote:
>> On 08/03/2011 12:28 AM, Peter Maydell wrote:
>>> On 2 August 2011 21:56, Anthony Liguori<anthony@codemonkey.ws> wrote:
>>> > Hrm, this looks like badness to me.
>>> >
>>> > You're effectively using MemoryRegions to implement an ad-hoc
>>> interface.
>>>
>>> As you'll see below, the hardware intreface is somewhat ad-hoc :-)
>>>
>>
>> IMO it fits perfectly into a qdev which contains 8 qbuses, each of which
>> is connected to another qdev.
>
> Do you mean a qdev that is a bus and accepts up to 8 devices as children?

I guess that works too.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

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

[Avi Kivity]

On 08/03/2011 05:33 AM, Anthony Liguori wrote:
> On 08/02/2011 04:29 PM, Avi Kivity wrote:
>> On 08/03/2011 12:06 AM, Anthony Liguori wrote:
>>>
>>> The qdev level should be the common base that makes sense for *all*
>>> qdev devices. IRQ management does not belong in DeviceState because
>>> what you do for a simple LCD is not what you do for an MSI-X capable
>>> PCI device.
>>>
>>> This is what QOM properties tries to address. It should be possible to
>>> create a simple device, and register plugs/sockets for GPIO pins
>>> without pushing GPIO knowledge into the base class.
>>>
>>> In a QDev world, the right approach is to have a GpioDevice base class
>>> that implements this sort of logic for devices where it makes sense.
>>> That's what SysBusDevice sort of wants to be but it somehow ended up
>>> as yet another base class for everything.
>>>
>>
>> Doesn't this end up requiring multiple inheritance, and a ton of
>> boilerplate in addition?
>
> I don't see a ton of boiler plate nor do I see multiple inheritance. 
> Either you have devices that live on a bus that interact with other 
> devices via the bus interface or you have devices that interact 
> through Pin interactions.

If you have a GpioDevice to for devices/buses with gpio and a FooDevice 
for devices/buses with foo, how do you do a device that has both gpio 
and foo?

>
> But pushing everything into qdev is wrong.  This is a classic trap 
> with object oriented modelling.  Inheritance is supposed to model is-a 
> relationships, not "may-implement in some subclasses".  Not only does 
> it cause unnecessary bloat, it causes brittleness.
>
> What in the world would DeviceState::num_gpio_out mean for a PCI device?

Nothing.  It's always zero.

> It doesn't make sense at all which means it doesn't belong in 
> DeviceState.

What if the common subset is empty?

> Unless we're modelling the pin inputs and outputs for every device 
> that we possibly support..  But we're in for a world of hurt if that's 
> what our goals are.  Connecting PCI devices to their busses will be, 
> interesting, to say the least :-)

We use the high level functions to model address/data/irq/decode/etc. 
and gpio for the stuff that falls between the chairs.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

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

[Peter Maydell]

On 3 August 2011 03:25, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 08/02/2011 04:28 PM, Peter Maydell wrote:
>> Typically in the latter case the device we're talking to will
>> also provide some gpio or irq signals, which will be routed in
>> a totally different direction having nothing to do with the GPMC.
>> So it's important that we should be able to take an existing
>> device model and route its registers through the GPMC and its
>> GPIO elsewhere.
>
> So I think the modelling disconnect here is that you're trying to model the
> wire connections to hook up arbitrary devices to a OMAP SOC.

Yes. This is how hardware works... If our device model lets
us wire up connections at a fairly low level, then we can always
implement more strongly typed and more bus like connections in
terms of that. If we only provide strongly typed connections then
it's not possible to model lower level wiring in terms of those.

This is IMHO exactly why so many things end up as sysbus devices --
they're the closest thing we have to "just get out of my way and
let me connect this device model up the way it needs to be connected".

> Unless you're modelling each pin (which I don't think you're doing), I think
> it makes more sense to treat this as a bus and model accordingly.

I think the "bus" like thing is the "I am providing an addressible
section of memory" part. I think we need that as a low level
fundamental kind of connection in the same way as a single GPIO
signal is a low level fundamental connection. At the moment the
closest thing we have to that is to model it as a complete sysbus,
which seems like overkill (we don't want the GPIO bits of sysbus
for just the memory connection) but will work.

[*] I did convince myself that you do want a sysbus bus on each
chip select, because in theory in the hardware you could put an
external address decoder on the gpmc_a[] bus and hang multiple
devices off one chipselect. So that's a sysbus, really.

-- PMM

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

[Avi Kivity]

On 08/03/2011 12:10 PM, Peter Maydell wrote:
> [*] I did convince myself that you do want a sysbus bus on each
> chip select, because in theory in the hardware you could put an
> external address decoder on the gpmc_a[] bus and hang multiple
> devices off one chipselect. So that's a sysbus, really.

No, you've added a device which is yet another bus.

-- 
error compiling committee.c: too many arguments to function