Re: [Qemu-devel] [PATCH v3] Two-Level IOPort Lookup

[Brian Wheeler]

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 <address@hidden>
> > 
> > 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<<IOPORT_MAXBITS)) {
> > +   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