From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=48841 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PAgmV-0005Ud-5t for qemu-devel@nongnu.org; Tue, 26 Oct 2010 06:29:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PAgmL-0005s4-Ep for qemu-devel@nongnu.org; Tue, 26 Oct 2010 06:29:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60933) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PAgmL-0005rf-0u for qemu-devel@nongnu.org; Tue, 26 Oct 2010 06:29:17 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o9QATG2w013820 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 26 Oct 2010 06:29:16 -0400 Date: Tue, 26 Oct 2010 12:29:14 +0200 From: Gleb Natapov Subject: Re: [Qemu-devel] [PATCH 1/2] Keep track of ISA ports ISA device is using in qdev. Message-ID: <20101026102914.GO2343@redhat.com> References: <1287928933-11423-1-git-send-email-gleb@redhat.com> <1287928933-11423-2-git-send-email-gleb@redhat.com> <20101025165803.GB31633@redhat.com> <20101025181540.GB877@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org On Tue, Oct 26, 2010 at 11:49:26AM +0200, Markus Armbruster wrote: > Gleb Natapov writes: > > > On Mon, Oct 25, 2010 at 08:01:13PM +0200, Markus Armbruster wrote: > >> Gleb Natapov writes: > >> > >> > On Mon, Oct 25, 2010 at 05:06:51PM +0200, Markus Armbruster wrote: > >> >> Gleb Natapov writes: > >> >> > >> >> > Prevent two devices from claiming the same io port. > >> >> > >> >> Really? > >> >> > >> >> Your new check for double-claim is in the new isa_init_ioport(), which > >> >> is for ISADevice only. Thus, only qdevified ISA devices can opt for > >> >> this protection, by calling isa_init_ioport(). It doesn't protect from > >> >> devices who don't or can't opt in, such as PCI devices. > >> >> > >> > I didn't claim different. This obviously works only for ISA qdev > >> > devices. > >> > >> I read "Prevent two devices from claiming the same io port" as such. > >> > > And have you read subject with that? > > Yes. Nevertheless, I found it a bit misleading, so I commented. > Well, I hope it is clear now. > >> >> Anyway, we already check for double-claim in > >> >> register_ioport_{read,write}(). The check has issues --- hw_error() is > >> >> wrong there for hot plug. But it's where the check should be, isn't it? > >> > You don't like double-claim checking? Forget about it (all 3 lines > >> > of code). The real point of the patch is to have ISA resources used > >> > by devices to be stored in common place (ISADevice) which allows > >> > get_dev_path() to be implemented. > >> > >> We already track I/O port use, in ioport.c. I don't like that > >> duplicated. Even less so if the dupe catches fewer double-claims than > >> the original. > > Consider it removed although we do keep track of irqs there and this > > tracking is also incomplete. Why is it there? > > Where's "there"? > In ISADevice. Look for "isabus->assigned" in isa-bus.c. > Let's not add to the code duplication if we can help it. It's bad > enough as it is. > > >> Perhaps your new function should wrap around register_ioport_*() instead > >> of supplementing it. > > register_ioport_*() is disconnected from qdev in general and from ISADEvice > > in particular, so how "wrap around register_ioport_*()" will help me to > > have get_dev_path() for ISABus is beyond my understanding. > > I was too terse, sorry. > > Maybe we should have functions for ISADevices to register ISA resources. > Functions that are *not* disconnected from qdev. Instead of code like > > register_ioport_write(pm_io_base, 64, 2, pm_ioport_writew, s); > register_ioport_read(pm_io_base, 64, 2, pm_ioport_readw, s); > register_ioport_write(pm_io_base, 64, 4, pm_ioport_writel, s); > register_ioport_read(pm_io_base, 64, 4, pm_ioport_readl, s); > isa_init_ioport_range(dev, pm_ioport_base, 64); > > or, with Avi's proposed interface > > ioport_register(&s->ioport, pm_io_base, 64); > isa_init_ioport_range(dev, pm_ioport_base, 64); > > we'd get something like > > isa_ioport_register(dev, &s->ioport, pm_io_base, 64); > > Isn't that neater? > Neater, but out of scope for my work. Obviously all resource management should go via qdev at the end. > Since some PCI devices also register ISA resources, we'd have to > offer functions for them as well, properly integrated. > > Now, I didn't mean to ask you to do all that as a precondition for > getting your patch in. I was just observing. OK. -- Gleb.