Re: [Qemu-devel] [PATCH 1/2] Type-safe ioport callbacks

[Anthony Liguori]

On 10/26/2010 12:18 PM, Avi Kivity wrote:
>  On 10/26/2010 05:09 PM, Blue Swirl wrote:
> > On Tue, Oct 26, 2010 at 8:05 AM, Avi Kivity<address@hidden>  wrote:
> > >    On 10/25/2010 08:38 PM, Blue Swirl wrote:
> > >>
> > >> >
> > >> >    I don't really see why we need registration; 
> > cpu_register_io() takes
> > >> >    function pointers, a size, and an opaque, and gives an 
> > integer handle
> > >> >  in
> > >> >    return.  With the IOPort object approach, you set up the 
> > IOPort with
> > >> >    function pointers, size is implied, and the opaque is derived 
> > using
> > >> >    container_of(); the handle is simply the address of the object.
> > >>
> > >>  With the handle, we can separate setting up the structures at device
> > >>  level, and mapping the object using only the handle at bus or other
> > >>  higher level. Can this be done with the object approach?
> > >
> > >  I believe so.  The handle is simply an indirect pointer, no?
> >
> > Yes, but then the object should also contain size information. That
> > should not be needed for mapping at higher level.
>
> Sorry, I don't follow your meaning.
>
> When I said "size is implied" I meant that the IOPort object has a 
> separate function pointer for sizes 1, 2, and 4, so it 
> ioport_register() doesn't need a size parameter.  But I don't see how 
> that relates to your comment.

Yeah, I don't think it makes sense to combine "this is how to dispatch 
I/O" with "this is a region of I/O address space".

I think an IORegion should contain an IOPort structure though.  I think 
the name needs rethinking.

Maybe:

struct PortIOHandler;
struct MemoryIOHandler;

And it would be good to add a memory callback to this series too.

Regards,

Anthony Liguori

> > >>  The purpose of that patch series was to perform the separation 
> > for PCI
> > >>  BARs. I wasn't so happy with the series, so I never pushed.
> > >
> > >  In fact I think an IOPort is even more suitable; if we need 
> > additional
> > >  attributes we can use a derived object:
> > >
> > >  struct PCIIOPort {
> > >      IOPort ioport;
> > >      /* additional fields */
> > >  };
> >
> > One issue with my series was that it would be great if the devices
> > just had some BAR structures (used by PCI layer to map the devices)
> > inside PCI/qdev structures, but I invented that too late. Maybe this
> > can be addressed in your design?
>
> It looks to be orthogonal.  It would be great to have a BAR object; 
> that object can then use your API, my API, or the existing API.