From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:42836) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QVl1h-0005pS-Ul for qemu-devel@nongnu.org; Sun, 12 Jun 2011 09:48:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QVl1g-0000xP-9q for qemu-devel@nongnu.org; Sun, 12 Jun 2011 09:48:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:7688) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QVl1f-0000wV-Sr for qemu-devel@nongnu.org; Sun, 12 Jun 2011 09:48:28 -0400 Date: Sun, 12 Jun 2011 16:48:24 +0300 From: Gleb Natapov Message-ID: <20110612134824.GK491@redhat.com> References: <4DEF2F25.5070104@redhat.com> <1307559319-16183-1-git-send-email-andreas.faerber@web.de> <1307559319-16183-2-git-send-email-andreas.faerber@web.de> <1307559319-16183-3-git-send-email-andreas.faerber@web.de> <1307559319-16183-4-git-send-email-andreas.faerber@web.de> <1307559319-16183-5-git-send-email-andreas.faerber@web.de> <8A9CB932-9F44-4B19-BFB2-8B1267F990BC@web.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: <8A9CB932-9F44-4B19-BFB2-8B1267F990BC@web.de> Subject: Re: [Qemu-devel] [RFC v4 04/12] isa: Allow to un-assign I/O ports List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?utf-8?Q?F=C3=A4rber?= Cc: Gerd Hoffmann , =?utf-8?B?SGVydsOp?= Poussineau , Markus Armbruster , qemu-devel Developers On Sun, Jun 12, 2011 at 01:59:51PM +0200, Andreas F=C3=A4rber wrote: > Am 09.06.2011 um 17:03 schrieb Markus Armbruster: >=20 > >Andreas F=C3=A4rber writes: > > > >>Signed-off-by: Andreas F=C3=A4rber > >>--- > >>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 =3D 0; i < dev->nioports; i++) { > >>+ if (dev->ioports[i] =3D=3D start) { > >>+ for (j =3D 0; j < dev->nioports - i; j++) { > >>+ dev->ioports[i + j] =3D dev->ioports[i + length + j]; > >>+ } > >>+ dev->nioports -=3D length; > >>+ break; > >>+ } > >>+ } > >>+} > >>+ > > > >"discard"? It's the opposite of isa_init_ioport_range(), name should > >make that obvious. "uninit"? >=20 > "uninit" felt wrong. >=20 > >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. >=20 > Right, I didn't spot any wrong usage though. >=20 > 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. >=20 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=20 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. It is always easier to call something a mess instead of actually fixing it. -- Gleb.