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 <andreas.faer...@web.de> writes:
> >>>
> >>>>Signed-off-by: Andreas Färber <andreas.faer...@web.de>
> >>>>---
> >>>>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.
