Re: [Qemu-devel] [RFC v4 04/12] isa: Allow to un-assign I/O ports

[Gleb Natapov]

On Sun, Jun 12, 2011 at 05:32:57PM +0200, Andreas Färber wrote:
> Am 12.06.2011 um 15:48 schrieb Gleb Natapov:
> 
> >On Sun, Jun 12, 2011 at 01:59:51PM +0200, Andreas Färber wrote:
> >>Am 09.06.2011 um 17:03 schrieb Markus Armbruster:
> >>
> >>>Andreas Färber <address@hidden> writes:
> >>>
> >>>>Signed-off-by: Andreas Färber <address@hidden>
> >>>>---
> >>>>hw/isa-bus.c |   14 ++++++++++++++
> >>>>hw/isa.h     |    1 +
> >>>>2 files changed, 15 insertions(+), 0 deletions(-)
> >>>>
> >>>>diff --git a/hw/isa-bus.c b/hw/isa-bus.c
> >>>>index d258932..1f64673 100644
> >>>>--- a/hw/isa-bus.c
> >>>>+++ b/hw/isa-bus.c
> >>>>@@ -105,6 +105,20 @@ void isa_init_ioport(ISADevice *dev,
> >>>>uint16_t ioport)
> >>>>  isa_init_ioport_range(dev, ioport, 1);
> >>>>}
> >>>>
> >>>>+void isa_discard_ioport_range(ISADevice *dev, uint16_t start,
> >>>>uint16_t length)
> >>>>+{
> >>>>+    int i, j;
> >>>>+    for (i = 0; i < dev->nioports; i++) {
> >>>>+        if (dev->ioports[i] == start) {
> >>>>+            for (j = 0; j < dev->nioports - i; j++) {
> >>>>+                dev->ioports[i + j] = dev->ioports[i +
> >>>>length + j];
> >>>>+            }
> >>>>+            dev->nioports -= length;
> >>>>+            break;
> >>>>+        }
> >>>>+    }
> >>>>+}
> >>>>+
> >>>
> >>>"discard"?  It's the opposite of isa_init_ioport_range(), name
> >>>should
> >>>make that obvious.  "uninit"?
> >>
> >>"uninit" felt wrong.
> >>
> >>>Note: this only affects the I/O-port bookkeeping within
> >>>ISADevice, not
> >>>the actual I/O port handler registration.  Any use must be
> >>>accompanied
> >>>by a matching handler de-registration.  Just like any use of
> >>>isa_init_ioport_range() must be accompanied by matching
> >>>register_ioport_FOO()s.  You can thank Gleb for this mess.
> >>
> >>Right, I didn't spot any wrong usage though.
> >>
> >>So what about cleaning up the mess and doing
> >>isa_[un]assign_ioport_range(), wrapping the ioport.c functions?
> >>The overhead of calling it up to six times for the different widths
> >>and read/write would seem negligible as a startup cost. And for
> >>pc87312 we don't seriously have to care about performance imo.
> >>
> >Ideally every ioport registration should be moved to use IORange. I
> >tried to move all isa devices to it, but it is complicated for two
> >reasons. First not every device is qdevified and second some devises
> >can be instantiated as isa device and non-isa device and they do
> >ioport
> >registration in a common code. With your approach you will have to
> >duplicate ioport registration code for isa and non-isa case for such
> >devices and code duplication is not good.
> 
> I'm not trying to duplicate anything! What I've been seeing is that
> many of the ISA devices I've touched in this series first register
> the I/O ports and then associate them with the ISADevice, in
> ISA-only code. So the numbers *are* duplicated and I'm proposing to
> consolidate this into one call.
> 
> The existing "init" would stay, so that code with disseparate ISA
> and common parts (like IDE) remains unaffected.
> 
OK, if you think that it is less of a "mess" this way. In a perfect
world you would have something like:

void init_ioport(DeviceState *dev, IORange *r, const IORangeOps *ops,
                     uint64_t base, uint64_t len)

And it will do correct thing for each device type.

--
                        Gleb.