From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LtNba-0007yd-BC for qemu-devel@nongnu.org; Mon, 13 Apr 2009 10:57:50 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LtNbV-0007tt-TX for qemu-devel@nongnu.org; Mon, 13 Apr 2009 10:57:49 -0400 Received: from [199.232.76.173] (port=40309 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LtNbV-0007te-CC for qemu-devel@nongnu.org; Mon, 13 Apr 2009 10:57:45 -0400 Received: from belushi.uits.indiana.edu ([129.79.1.188]:47388) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1LtNbU-0004Ql-Sl for qemu-devel@nongnu.org; Mon, 13 Apr 2009 10:57:45 -0400 Received: from mail-relay.iu.edu (burns.uits.indiana.edu [129.79.1.202]) by belushi.uits.indiana.edu (8.14.2/8.13.8/IU Messaging Team) with ESMTP id n3DEvha7025664 for ; Mon, 13 Apr 2009 10:57:43 -0400 Received: from [129.79.35.119] (nibbler.dlib.indiana.edu [129.79.35.119]) (authenticated bits=0) by mail-relay.iu.edu (8.14.2/8.13.8/IU Messaging Team Submission) with ESMTP id n3DEvg1u015079 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Mon, 13 Apr 2009 10:57:42 -0400 Subject: Re: [Qemu-devel] [PATCH v3] Two-Level IOPort Lookup From: Brian Wheeler In-Reply-To: References: <1239631898.27750.26.camel@nibbler.dlib.indiana.edu> Content-Type: text/plain Date: Mon, 13 Apr 2009 10:57:42 -0400 Message-Id: <1239634662.27750.33.camel@nibbler.dlib.indiana.edu> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org On Mon, 2009-04-13 at 18:40 +0400, malc wrote: > On Mon, 13 Apr 2009, Brian Wheeler wrote: > > > I've fixed a few things: > > * my pointer arithmetic was gross looking, so I fixed it > > * added an allocated page count for stats when debugging > > > > > > On my platform (x86-64) the in-tree implementation allocates 3.5M for > > the PC target. With this implementation the PC target consumes 2K of > > pointer table + 256K of malloc'd memory. > > > > For the Alpha target, statically allocating all 24-bits worth of ioport > > tables is about 1G of ram. With this patch, it uses 32K for the pointer > > table and 512K of malloc'd memory > > > > Is there anything else I need to look at before this patch is worthy of > > inclusion? > > > > > > > > Signed-off-by: Brian Wheeler > > > > Index: vl.c > > =================================================================== > > --- vl.c (revision 7097) > > +++ vl.c (working copy) > > @@ -168,8 +168,8 @@ > > //#define DEBUG_IOPORT > > //#define DEBUG_NET > > //#define DEBUG_SLIRP > > +//#define DEBUG_IOPORT_FIND > > > > - > > #ifdef DEBUG_IOPORT > > # define LOG_IOPORT(...) qemu_log_mask(CPU_LOG_IOPORT, ## __VA_ARGS__) > > #else > > @@ -184,14 +184,31 @@ > > /* Max number of bluetooth switches on the commandline. */ > > #define MAX_BT_CMDLINE 10 > > > > -/* XXX: use a two level table to limit memory usage */ > > -#define MAX_IOPORTS 65536 > > - > > const char *bios_dir = CONFIG_QEMU_SHAREDIR; > > const char *bios_name = NULL; > > -static void *ioport_opaque[MAX_IOPORTS]; > > -static IOPortReadFunc *ioport_read_table[3][MAX_IOPORTS]; > > -static IOPortWriteFunc *ioport_write_table[3][MAX_IOPORTS]; > > + > > +struct ioport { > > + void *opaque; > > + IOPortReadFunc *read[3]; > > + IOPortWriteFunc *write[3]; > > + void *pad; > > +}; > > +typedef struct ioport ioport_t; > > Please do not use _t. > Ah, I missed that part in the CODING_STYLE document. I've now called it IOPort > [..snip..] > > > +static inline ioport_t *ioport_find(uint32_t address, int allocate) > > +{ > > + uint32_t page = (address & IOPORT_PAGEMASK) >> IOPORT_PAGEBITS; > > + uint32_t entry = address & IOPORT_ENTRYMASK; > > +#ifdef DEBUG_IOPORT_FIND > > + static int page_count = 0; > > + if (address >= (1< > + hw_error("Maximum port # for this architecture is %d. " > ^ extra space? > > [..snip..] > Not really, since the next line is a continuation of the string (to meet the 80-char-wide rule) and that ends a sentence. > > for(i = start; i < start + length; i += size) { > > - ioport_read_table[bsize][i] = func; > > - if (ioport_opaque[i] != NULL && ioport_opaque[i] != opaque) > > + ioport_t *p = ioport_find(i, 1); > > Indentation appears messed up. > > [..snip..] > Sigh, yeah. A new patch is forthcoming. Brian