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

[Paolo Bonzini]

Il 04/06/2013 15:24, Paolo Bonzini ha scritto:
> 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}.
> 
>>>> 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

A closer look at the code, and a better grep command, changed this down to:

    hw/display/cirrus_vga.c:2
    hw/display/qxl.c:1
    hw/display/vga-isa.c:1
    hw/display/vga.c:6
    hw/misc/vfio.c:8

(plus the ones quoted below) where all the calls in cirrus, qxl and vfio
could be removed realtively easily.  In particular, VGA card
implementations do not use pci_register_vga yet, because it's a new API.

So, with further refactoring, it could be brought down to 1 or 2 calls
in hw/display/vga.c and one in vga-isa.c.  hw/display/vga.c needs the
memory_region_set_owner calls, both because of optimization tricks, and
because the code tries to cover both ISA and PCI.  vga-isa.c needs it
because VGAs are special ISA devices, the only ones that do MMIO.

Now, doing all the refactoring may not be worthwhile, but it shows that
the abstraction is even less leaky than it sounds.

I asked Alex Williamson to read the thread and share his opinion.
Interestingly, he had a different mental model of building the memory
regions (passing them to PCI core early rather than late, and that's why
VFIO needed 8 calls in this series).  So I believe his input will be useful.

Paolo

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