[Qemu-devel] Re: [PATCH] two level table for IO port lookup [Patch updated]

[Jan Kiszka]

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.  
>>>
>>>
>>>
>>>
>>> Signed-off-by: Brian Wheeler <address@hidden>
>>>
> 
>>> +  if(ioport[page]==NULL) {
>>> +    ioport[page]=calloc((1<<IOPORT_PAGESIZE), sizeof(ioport_t));
>> 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, just
>> call the proper default handler.
>>
> 
> 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.
> 
> Signed-off-by: Brian Wheeler <address@hidden>
> 
> 
> --- 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
>  
>  #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
>  
> -/* 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];
> +};

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(<padded-ioport>) = 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<<IOPORT_PAGESIZE)-1)
> +#define IOPORT_PAGEMASK ~IOPORT_ENTRYMASK
> +#define MAX_IOPORTS (1<<IOPORT_MAXBITS)
> +
> +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 available
>     to store the VM snapshots */
>  DriveInfo drives_table[MAX_DRIVES+1];
> @@ -288,6 +304,37 @@
>  static IOPortReadFunc default_ioport_readb, default_ioport_readw, 
> default_ioport_readl;
>  static IOPortWriteFunc default_ioport_writeb, default_ioport_writew, 
> default_ioport_writel;
>  
> +
> +static inline ioport_t *ioport_find(uint32_t address, int allocate) 
> +{
> +  uint32_t page = (address & IOPORT_PAGEMASK) >> IOPORT_PAGESIZE;
> +  uint32_t entry = address & IOPORT_ENTRYMASK;
> +  if(address >= (1<<IOPORT_MAXBITS))
> +    hw_error("Maximum port # for this architecture is %d.  Port %d 
> requested.",
> +          (1<<IOPORT_MAXBITS)-1, address);

This check shouldn't be performed in the fast path (io access), at least
not unconditionally. The CPU models already have to take care to not
invoke this service with invalid arguments.

BTW, you tend to use 2 spaces as indention. The QEMU style is 4 spaces,
please fix.

> +  
> +  if(ioport[page]==NULL) {

Please check CODING_STYLE for proper formatting.

> +    if(allocate) {
> +      ioport[page]=calloc((1<<IOPORT_PAGESIZE), sizeof(ioport_t));

If calloc fails, qemu will crash. Use qemu_mallocz instead, it fails
gracefully in case of OOM.

> +#ifdef DEBUG_IOPORT_FIND
> +    printf("Initializing ioport page %d to: %p\n", page, ioport[page]);
> +#endif
> +    } else {
> +      return NULL;
> +    }
> +  }
> +  ioport_t *p = (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=%d, address=%p, entry=%d, address=%p\n", 
> +      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] = {
> @@ -295,10 +342,16 @@
>          default_ioport_readw,
>          default_ioport_readl
>      };
> -    IOPortReadFunc *func = ioport_read_table[index][address];
> +    ioport_t *p = ioport_find(address, 0);
> +    IOPortReadFunc *func = NULL;
> +    void *opaque = NULL;
> +    if(p) {
> +      func = p->read[index];
> +      opaque = p->opaque;
> +    }
>      if (!func)
>          func = default_func[index];
> -    return func(ioport_opaque[address], address);
> +    return func(opaque, address);
>  }
>  
>  static void ioport_write(int index, uint32_t address, uint32_t data)
> @@ -308,10 +361,16 @@
>          default_ioport_writew,
>          default_ioport_writel
>      };
> -    IOPortWriteFunc *func = ioport_write_table[index][address];
> +    ioport_t *p = ioport_find(address, 0);
> +    IOPortWriteFunc *func = NULL;
> +    void *opaque = NULL;
> +    if(p) {
> +      func = p->write[index];
> +      opaque = p->opaque;
> +    }
>      if (!func)
>          func = default_func[index];
> -    func(ioport_opaque[address], address, data);
> +    func(opaque, address, data);
>  }
>  
>  static uint32_t default_ioport_readb(void *opaque, uint32_t address)
> @@ -378,10 +437,11 @@
>          return -1;
>      }
>      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);
> +        p->read[bsize] = func;
> +        if (p->opaque != NULL && p->opaque != opaque)
>              hw_error("register_ioport_read: invalid opaque");
> -        ioport_opaque[i] = opaque;
> +        p->opaque = opaque;
>      }
>      return 0;
>  }
> @@ -403,10 +463,11 @@
>          return -1;
>      }
>      for(i = start; i < start + length; i += size) {
> -        ioport_write_table[bsize][i] = func;
> -        if (ioport_opaque[i] != NULL && ioport_opaque[i] != opaque)
> +        ioport_t *p = ioport_find(i, 1);
> +        p->write[bsize] = func;
> +        if (p->opaque != NULL && p->opaque != opaque)
>              hw_error("register_ioport_write: invalid opaque");
> -        ioport_opaque[i] = opaque;
> +        p->opaque = opaque;
>      }
>      return 0;
>  }
> @@ -416,15 +477,16 @@
>      int i;
>  
>      for(i = start; i < start + length; i++) {
> -        ioport_read_table[0][i] = default_ioport_readb;
> -        ioport_read_table[1][i] = default_ioport_readw;
> -        ioport_read_table[2][i] = default_ioport_readl;
> -
> -        ioport_write_table[0][i] = default_ioport_writeb;
> -        ioport_write_table[1][i] = default_ioport_writew;
> -        ioport_write_table[2][i] = default_ioport_writel;
> +        ioport_t *p = ioport_find(i, 1);
> +        p->read[0] = default_ioport_readb;
> +        p->read[1] = default_ioport_readw;
> +        p->read[2] = default_ioport_readl;
> +
> +        p->write[0] = default_ioport_writeb;
> +        p->write[1] = default_ioport_writew;
> +        p->write[2] = default_ioport_writel;
>  
> -        ioport_opaque[i] = NULL;
> +        p->opaque = NULL;
>      }
>  }
>  

Jan

signature.asc
Description: OpenPGP digital signature