[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 06/17] sysbus: add sysbus_pass_mmio
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH v2 06/17] sysbus: add sysbus_pass_mmio |
Date: |
Tue, 04 Jun 2013 16:27:44 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130514 Thunderbird/17.0.6 |
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
- [Qemu-devel] [PATCH v2 02/17] memory: add ref/unref, (continued)
- [Qemu-devel] [PATCH v2 02/17] memory: add ref/unref, Paolo Bonzini, 2013/06/04
- [Qemu-devel] [PATCH v2 04/17] exec: add a reference to the region returned by address_space_translate, Paolo Bonzini, 2013/06/04
- [Qemu-devel] [PATCH v2 05/17] pci: set owner for BARs, Paolo Bonzini, 2013/06/04
- [Qemu-devel] [PATCH v2 07/17] sysbus: set owner for MMIO regions, Paolo Bonzini, 2013/06/04
- [Qemu-devel] [PATCH v2 06/17] sysbus: add sysbus_pass_mmio, Paolo Bonzini, 2013/06/04
- Re: [Qemu-devel] [PATCH v2 06/17] sysbus: add sysbus_pass_mmio, Peter Maydell, 2013/06/04
- Re: [Qemu-devel] [PATCH v2 06/17] sysbus: add sysbus_pass_mmio, Paolo Bonzini, 2013/06/04
- Re: [Qemu-devel] [PATCH v2 06/17] sysbus: add sysbus_pass_mmio, Peter Maydell, 2013/06/04
- Re: [Qemu-devel] [PATCH v2 06/17] sysbus: add sysbus_pass_mmio, Paolo Bonzini, 2013/06/04
- Re: [Qemu-devel] [PATCH v2 06/17] sysbus: add sysbus_pass_mmio, Peter Maydell, 2013/06/04
- Re: [Qemu-devel] [PATCH v2 06/17] sysbus: add sysbus_pass_mmio,
Paolo Bonzini <=
- Re: [Qemu-devel] [PATCH v2 06/17] sysbus: add sysbus_pass_mmio, Peter Maydell, 2013/06/04
- Re: [Qemu-devel] [PATCH v2 06/17] sysbus: add sysbus_pass_mmio, Paolo Bonzini, 2013/06/04
- Re: [Qemu-devel] [PATCH v2 06/17] sysbus: add sysbus_pass_mmio, Paolo Bonzini, 2013/06/04
- Re: [Qemu-devel] [PATCH v2 06/17] sysbus: add sysbus_pass_mmio, Alex Williamson, 2013/06/04
- Re: [Qemu-devel] [PATCH v2 06/17] sysbus: add sysbus_pass_mmio, Paolo Bonzini, 2013/06/04
[Qemu-devel] [PATCH v2 08/17] acpi: add memory_region_set_owner calls, Paolo Bonzini, 2013/06/04
[Qemu-devel] [PATCH v2 10/17] isa/portio: allow setting an owner, Paolo Bonzini, 2013/06/04
[Qemu-devel] [PATCH v2 11/17] vga: add memory_region_set_owner calls, Paolo Bonzini, 2013/06/04
[Qemu-devel] [PATCH v2 09/17] misc: add memory_region_set_owner calls, Paolo Bonzini, 2013/06/04
[Qemu-devel] [PATCH v2 13/17] vfio: add memory_region_set_owner calls, Paolo Bonzini, 2013/06/04