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

[Peter Maydell]

On 4 June 2013 14:24, Paolo Bonzini <address@hidden> wrote:
> Il 04/06/2013 14:36, Peter Maydell ha scritto:
>> On 4 June 2013 13:31, Paolo Bonzini <address@hidden> wrote:
>>> Il 04/06/2013 14:24, Peter Maydell ha scritto:
>>>> On 4 June 2013 13:13, Paolo Bonzini <address@hidden> wrote:
>>>> This is much less flexible than just using sysbus_mmio_get_region(),
>>>> because it only lets you pass the whole set of MMIOs from the
>>>> other device through, not just the ones you want.
>>>
>>> How is this different from sysbus_pass_irq?
>>
>> sysbus_pass_irq is also an annoyingly inflexible function.
>> With MMIOs we have the advantage of being able to do better.
>
> I prefer consistency to useless flexibility.
>
> The day someone will need it, they can add sysbus_pass_one_{irq,mmio}.

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 think that would be a straightforward and easy to understand
>> way to define the ownership rules so I would much rather we
>> did that. I really don't like the way your current patch
>> is doing something complicated in an attempt to avoid this.
>
> They are straightforward, documented, and the wide majority of the
> devices need not care at all about them.

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"? This is just yoking together two separate
things. And it seems wrong that devices shouldn't care about
memory region ownership -- devices *are* the memory region
owners typically. Memory regions should just have owners
always from the start, if we need them to have owners.

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

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

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.

thanks
-- PMM