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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 04/06/2013 14:36, Peter Maydell ha scritto:
> On 4 June 2013 13:31, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 04/06/2013 14:24, Peter Maydell ha scritto:
>>> On 4 June 2013 13:13, Paolo Bonzini <pbonzini@redhat.com> 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}.

>>> Please just make reference counting work properly with passing
>>> MemoryRegion*s around.
>>
>> Do you have any idea that doesn't require touch 800 invocation of the
>> region creation functions?
> 
> 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.  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.

There are an order of magnitude less calls to memory_region_set_owner
than to memory_region_init_*.  Changing four places suffices to get
ownership for 97% of the devices (309 files in hw/ call
memory_region_init*, 9 devices call memory_region_set_owner):

  hw/core/sysbus.c:1
  hw/isa/isa-bus.c:1
  hw/pci/pci.c:1
  ioport.c:2

Of the remaining calls, 2/3 of them are concentrated in a handful of
devices:

  hw/display/cirrus_vga.c:7
  hw/display/vga.c:4
  hw/i386/kvm/pci-assign.c:7
  hw/misc/vfio.c:8

and all the others could probably be refactored away, and are all for PC
devices, other targets are unaffected (I did review them and sysbus
catches everything):

  hw/acpi/ich9.c:1
  hw/acpi/piix4.c:4
  hw/char/serial-pci.c:1
  hw/isa/apm.c:1
  hw/misc/pc-testdev.c:4


To me, it seems a pretty good abstraction.

Paolo