Re: [Qemu-devel] [PATCH v4 26/39] pcnet: convert to memory API

[Michael S. Tsirkin]

On Tue, Aug 09, 2011 at 03:44:35PM +0300, Avi Kivity wrote:
> On 08/09/2011 03:42 PM, Michael S. Tsirkin wrote:
> >On Tue, Aug 09, 2011 at 09:52:17AM +0300, Avi Kivity wrote:
> >>  On 08/09/2011 09:55 AM, Bob Breuer wrote:
> >>  >>    static void lance_cleanup(VLANClientState *nc)
> >>  >>   @@ -117,13 +116,11 @@ static int lance_init(SysBusDevice *dev)
> >>  >>        SysBusPCNetState *d = FROM_SYSBUS(SysBusPCNetState, dev);
> >>  >>        PCNetState *s =&d->state;
> >>  >>
> >>  >>   -    s->mmio_index =
> >>  >>   -        cpu_register_io_memory(lance_mem_read, lance_mem_write, d,
> >>  >>   -                               DEVICE_NATIVE_ENDIAN);
> >>  >>   +    memory_region_init_io(&s->mmio,&lance_mem_ops, s, "lance-mmio", 
> >> 4);
> >>  >
> >>  >You've switched up d and s here, so anything that tries to talk to the
> >>  >ethernet, such as a sparc32 guest, will now cause Qemu to segfault.
> >>  >
> >>  >
> >>
> >>  Good catch; will post a fix.
> >>
> >>  Maybe keeping the opaque wasn't such a good idea.
> >
> >Yes, we typically can get from the mmio to the device state
> >using container_of.
> >
> >
> 
> But in some cases, we can't, and the it's a pain having to wrap
> MemoryRegion in another structure containing an opaque.

I guess, even though that wrapping structure would
use a proper type, not an opaque.

> Maybe a good compromise is to:
> 
>   - keep MemoryRegion::opaque
>   - pass a MemoryRegion *mr to callbacks instead of opaque
>   - use container_of() when possible
>   - use mr->opaque otherwise

Right. This even saves a memory dereference when opaque is
unused.

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