From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LsKUa-0003WH-Dk for qemu-devel@nongnu.org; Fri, 10 Apr 2009 13:26:16 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LsKUV-0003VA-54 for qemu-devel@nongnu.org; Fri, 10 Apr 2009 13:26:15 -0400 Received: from [199.232.76.173] (port=42413 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LsKUU-0003Ux-R3 for qemu-devel@nongnu.org; Fri, 10 Apr 2009 13:26:10 -0400 Received: from fmmailgate03.web.de ([217.72.192.234]:34386) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1LsKUU-0006sA-2s for qemu-devel@nongnu.org; Fri, 10 Apr 2009 13:26:10 -0400 Received: from smtp05.web.de (fmsmtp05.dlan.cinetic.de [172.20.4.166]) by fmmailgate03.web.de (Postfix) with ESMTP id DF9CCFA42880 for ; Fri, 10 Apr 2009 19:26:08 +0200 (CEST) Received: from [88.66.126.154] (helo=[192.168.1.2]) by smtp05.web.de with asmtp (TLSv1:AES256-SHA:256) (WEB.DE 4.110 #277) id 1LsKUQ-0000kD-00 for qemu-devel@nongnu.org; Fri, 10 Apr 2009 19:26:06 +0200 Message-ID: <49DF812E.7050100@web.de> Date: Fri, 10 Apr 2009 19:26:06 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <1239374905.28083.21.camel@nibbler.dlib.indiana.edu> <49DF6313.4010800@web.de> <1239378381.28083.36.camel@nibbler.dlib.indiana.edu> In-Reply-To: <1239378381.28083.36.camel@nibbler.dlib.indiana.edu> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig86712A4323816982F8C9C98B" Sender: jan.kiszka@web.de Subject: [Qemu-devel] Re: [PATCH] two level table for IO port lookup [Patch updated] 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 This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig86712A4323816982F8C9C98B Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Brian Wheeler wrote: > On Fri, 2009-04-10 at 17:17 +0200, Jan Kiszka wrote: >> Brian Wheeler wrote: >>> The alpha architecture uses 24 bits for the io port address so this >>> patch adds a two level table and puts the IO port data into a >>> struct...because sizeof(void *) * 7 * 16777216 is nearly a 1G on my >>> workstation. >>> >>> I've set the alpha target to use a 12/12 split and everything else to= >>> use 8/8. =20 >>> >>> >>> >>> >>> Signed-off-by: Brian Wheeler >>> >=20 >>> + if(ioport[page]=3D=3DNULL) { >>> + ioport[page]=3Dcalloc((1<> As you use ioport_find also for guest-driven port access, doing >> allocation here is a bad idea. Consider a guest that probes the whole = io >> address space of your alpha box: you would end up with the same gig >> being allocated that you try to avoid with this approach. >> >> IOW: Only allocate on handler registration. On unsuccessful lookup, ju= st >> call the proper default handler. >> >=20 > Good point. This patch does that: there's an allocate flag which is > set only by the registration calls so only they will allocate memory. >=20 > Signed-off-by: Brian Wheeler >=20 >=20 > --- vl.c.orig 2009-04-10 10:01:52.000000000 -0400 > +++ vl.c 2009-04-10 11:40:27.000000000 -0400 > @@ -168,7 +168,7 @@ > //#define DEBUG_IOPORT > //#define DEBUG_NET > //#define DEBUG_SLIRP > - > +//#define DEBUG_IOPORT_FIND > =20 > #ifdef DEBUG_IOPORT > # define LOG_IOPORT(...) qemu_log_mask(CPU_LOG_IOPORT, ## __VA_ARGS__= ) > @@ -184,14 +184,30 @@ > /* Max number of bluetooth switches on the commandline. */ > #define MAX_BT_CMDLINE 10 > =20 > -/* XXX: use a two level table to limit memory usage */ > -#define MAX_IOPORTS 65536 > - > const char *bios_dir =3D CONFIG_QEMU_SHAREDIR; > const char *bios_name =3D 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]; > +}; Hmm, should we pad this struct to 8 pointers? > +typedef struct ioport ioport_t; > + > +#ifdef TARGET_ALPHA > +#define IOPORT_MAXBITS 24 > +#define IOPORT_PAGESIZE 12 I think IOPORT_PAGEBITS is a better name - the page size isn't 12 bytes. > +#else > +#define IOPORT_MAXBITS 16 > +#define IOPORT_PAGESIZE 8 I wonder if it wouldn't be a good idea to tune the page size (in bytes) to the host page size (or some small multiple of it), e.g. to 7 bits for 32-bit hosts with 4k pages (2^7 * sizeof() =3D 4096). That= way we could re-introduce initializing the page content to default handlers, dropping the null function check from the io access fast path. I think the reason this was once introduced is that lots of ioport pages with default but non-null content consumed real RAM while null pages typically don't do this on many OSes. But this two-level table should already address the issue at its root. > +#endif > + > +#define IOPORT_ENTRYMASK ((1< +#define IOPORT_PAGEMASK ~IOPORT_ENTRYMASK > +#define MAX_IOPORTS (1< + > +void *ioport[1<<(IOPORT_MAXBITS-IOPORT_PAGESIZE)]; Why not stick with "ioport_table" as name? > + > /* Note: drives_table[MAX_DRIVES] is a dummy block driver if none avai= lable > to store the VM snapshots */ > DriveInfo drives_table[MAX_DRIVES+1]; > @@ -288,6 +304,37 @@ > static IOPortReadFunc default_ioport_readb, default_ioport_readw, defa= ult_ioport_readl; > static IOPortWriteFunc default_ioport_writeb, default_ioport_writew, d= efault_ioport_writel; > =20 > + > +static inline ioport_t *ioport_find(uint32_t address, int allocate)=20 > +{ > + uint32_t page =3D (address & IOPORT_PAGEMASK) >> IOPORT_PAGESIZE; > + uint32_t entry =3D address & IOPORT_ENTRYMASK; > + if(address >=3D (1< + hw_error("Maximum port # for this architecture is %d. Port %d req= uested.", > + (1< + =20 > + if(ioport[page]=3D=3DNULL) { Please check CODING_STYLE for proper formatting. > + if(allocate) { > + ioport[page]=3Dcalloc((1< +#ifdef DEBUG_IOPORT_FIND > + printf("Initializing ioport page %d to: %p\n", page, ioport[page])= ; > +#endif > + } else { > + return NULL; > + } > + } > + ioport_t *p =3D (ioport_t *)(ioport[page] + entry * sizeof(ioport_t)= ); You can beautify this a lot by declaring ioport as "ioport_t *ioport[...]"... > + > +#ifdef DEBUG_IOPORT_FIND > + printf("port find %d: page=3D%d, address=3D%p, entry=3D%d, address=3D= %p\n",=20 > + address, page, ioport[page], entry, p); > + printf(" data: %p\n", p->opaque); > + printf(" read: %p, %p, %p\n", p->read[0], p->read[1], p->read[2]); > + printf(" write: %p, %p, %p\n", p->write[0], p->write[1], p->write[2]= ); > +#endif > + return p; > +} > + > static uint32_t ioport_read(int index, uint32_t address) > { > static IOPortReadFunc *default_func[3] =3D { > @@ -295,10 +342,16 @@ > default_ioport_readw, > default_ioport_readl > }; > - IOPortReadFunc *func =3D ioport_read_table[index][address]; > + ioport_t *p =3D ioport_find(address, 0); > + IOPortReadFunc *func =3D NULL; > + void *opaque =3D NULL; > + if(p) { > + func =3D p->read[index]; > + opaque =3D p->opaque; > + } > if (!func) > func =3D default_func[index]; > - return func(ioport_opaque[address], address); > + return func(opaque, address); > } > =20 > static void ioport_write(int index, uint32_t address, uint32_t data) > @@ -308,10 +361,16 @@ > default_ioport_writew, > default_ioport_writel > }; > - IOPortWriteFunc *func =3D ioport_write_table[index][address]; > + ioport_t *p =3D ioport_find(address, 0); > + IOPortWriteFunc *func =3D NULL; > + void *opaque =3D NULL; > + if(p) { > + func =3D p->write[index]; > + opaque =3D p->opaque; > + } > if (!func) > func =3D default_func[index]; > - func(ioport_opaque[address], address, data); > + func(opaque, address, data); > } > =20 > static uint32_t default_ioport_readb(void *opaque, uint32_t address) > @@ -378,10 +437,11 @@ > return -1; > } > for(i =3D start; i < start + length; i +=3D size) { > - ioport_read_table[bsize][i] =3D func; > - if (ioport_opaque[i] !=3D NULL && ioport_opaque[i] !=3D opaque= ) > + ioport_t *p =3D ioport_find(i, 1); > + p->read[bsize] =3D func; > + if (p->opaque !=3D NULL && p->opaque !=3D opaque) > hw_error("register_ioport_read: invalid opaque"); > - ioport_opaque[i] =3D opaque; > + p->opaque =3D opaque; > } > return 0; > } > @@ -403,10 +463,11 @@ > return -1; > } > for(i =3D start; i < start + length; i +=3D size) { > - ioport_write_table[bsize][i] =3D func; > - if (ioport_opaque[i] !=3D NULL && ioport_opaque[i] !=3D opaque= ) > + ioport_t *p =3D ioport_find(i, 1); > + p->write[bsize] =3D func; > + if (p->opaque !=3D NULL && p->opaque !=3D opaque) > hw_error("register_ioport_write: invalid opaque"); > - ioport_opaque[i] =3D opaque; > + p->opaque =3D opaque; > } > return 0; > } > @@ -416,15 +477,16 @@ > int i; > =20 > for(i =3D start; i < start + length; i++) { > - ioport_read_table[0][i] =3D default_ioport_readb; > - ioport_read_table[1][i] =3D default_ioport_readw; > - ioport_read_table[2][i] =3D default_ioport_readl; > - > - ioport_write_table[0][i] =3D default_ioport_writeb; > - ioport_write_table[1][i] =3D default_ioport_writew; > - ioport_write_table[2][i] =3D default_ioport_writel; > + ioport_t *p =3D ioport_find(i, 1); > + p->read[0] =3D default_ioport_readb; > + p->read[1] =3D default_ioport_readw; > + p->read[2] =3D default_ioport_readl; > + > + p->write[0] =3D default_ioport_writeb; > + p->write[1] =3D default_ioport_writew; > + p->write[2] =3D default_ioport_writel; > =20 > - ioport_opaque[i] =3D NULL; > + p->opaque =3D NULL; > } > } > =20 Jan --------------enig86712A4323816982F8C9C98B Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.9 (GNU/Linux) Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org iEYEARECAAYFAknfgS4ACgkQniDOoMHTA+l+QwCggcd2c1Whx4E9zn482+DoI7c+ XF4AniJAjE0dzyqoj0cWq515r0HebBky =ZWxI -----END PGP SIGNATURE----- --------------enig86712A4323816982F8C9C98B--