Re: [Qemu-devel] [PATCH v2 06/17] sysbus: add sysbus_pass_mmio

[Paolo Bonzini]

Il 04/06/2013 16:11, Peter Maydell ha scritto:
> We've already got a working implementation, in the shape
> of sysbus_mmio_get_region(). This is exactly the right way
> to do this API -- we have one API which says "give me a
> MemoryRegion*" and one which says "I have a MemoryRegion*,
> please expose it". All I'm asking you to do is not break it.

I can add a conditional to sysbus_add_mmio if you prefer.  I think it's
uglier, but I can live with it.

> I still don't understand them. Why should "hey, please use
> this MMIO region as a PCI BAR" imply "and by the way set the
> ownership"? Why does "here's an MMIO region which should be
> exposed to users of this device" imply "and by the way
> set the ownership"?

Because both places are _already_ tying a MemoryRegion to a device.

>>  By contrast, changing 800
>> invocations of the functions would be impossible to review seriously, it
>> would have to be redone when boards are qdev/QOM-ified, would be worse
>> for submitters of new boards.
> 
> If it's not clear who ought to be the owner when mmio_init_region
> is called then there are problems anyway.

It is clear, but this doesn't make a mechanical-but-not-quite patch easy
to review.

It's not that I cannot do it.  I simply believe it is a worse choice.

>> There are an order of magnitude less calls to memory_region_set_owner
>> than to memory_region_init_*.
> 
> That's because you've optimised for "minimise number of places
> to put calls". The downside is it's much harder to review new
> patches. An owner parameter to the mmio_init_region* functions
> means (a) it's impossible to forget to set the owner (b) it's
> easy to check when looking at the patch whether the owner is
> appropriate (c) you don't have to worry about weird cases
> where something else might try to set the owner later.

That's true, I cannot deny that.

> As a concrete example, if somebody submitted cirrus_vga
> as a new driver, I have no idea how to tell that it needs
> to set the owner for its memory regions, when 99% of
> other devices don't. I think this is going to result in
> "forgot to set owner" bugs.

Because cirrus is adding regions directly to address_space_memory/io.
As documented:

 * The device must set the owner itself
 * only if it uses memory_region_add_subregion directly on some address
 * space, or after the parent region is passed to the bus (for example
 * dynamically while the device runs).

Paolo