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

[Brian Wheeler]

On Fri, 2009-04-10 at 19:26 +0200, Jan Kiszka wrote:
> Brian Wheeler wrote:
> > On Fri, 2009-04-10 at 17:17 +0200, Jan Kiszka wrote:
> >> Brian Wheeler wrote:
> > +
> > +struct ioport {
> > +   void *opaque;
> > +   IOPortReadFunc *read[3];
> > +   IOPortWriteFunc *write[3];
> > +};
> 
> Hmm, should we pad this struct to 8 pointers?
> 

Probably.  I've done that in my working patch.


> > +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.
> 

Yes, its a better name.


> > +#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.
> 

I'm going to have to defer to someone else on this, because I don't
know.  I tried the patch with other values and it worked ok, so if we
decide to tune it later it shouldn't require any code changes.  Using 8
bits for the entry size gives a table size of 8K, so it is two x86-32
pages which isn't bad. 

On the topic of the extra null checks, I've rewritten it so non-allocate
find with a null page returns a pointer to a default ioport_t -- that
way ioport_find never returns null and the check isn't needed.


> > +#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?


Yep, that's a better name.

> > +
> > +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.
> 

I put it into the #ifdef DEBUG_IOPORT_FIND block, so it'll only get
called if there's debugging in process.


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

I blame emacs.  I've fixed it.


> > +  
> > +  if(ioport[page]==NULL) {
> 
> Please check CODING_STYLE for proper formatting.
> 

Ok, I've got through the patch and tried to find all of the coding style
issues.

> > +    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.
> 

Done


> > +#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[...]"...
> 

Yeah, my C is really rusty so I knew what I wanted to do, but I couldn't
remember how to do it so it was pretty.

ioport is a pointer to the table of ioport_t entries.  How do I declare
and use it?  If I declare ioport as:

ioport_t *ioport[1 << (IOPORT_MAXBITS-IOPORT_PAGEBITS)];

then I can take out the cast, but how do I get the table entry since I
have to dereference the ioport[page] to get to the array?


Here's the updated patch:

Signed-off-by: Brian Wheeler <address@hidden>

--- vl.c.orig   2009-04-10 10:01:52.000000000 -0400
+++ vl.c        2009-04-10 14:52:07.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,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;
+
+#ifdef TARGET_ALPHA
+#define IOPORT_MAXBITS 24
+#define IOPORT_PAGEBITS 12
+#else
+#define IOPORT_MAXBITS 16
+#define IOPORT_PAGEBITS 8
+#endif
+
+#define IOPORT_ENTRYMASK ((1<<IOPORT_PAGEBITS)-1)
+#define IOPORT_PAGEMASK ~IOPORT_ENTRYMASK
+#define MAX_IOPORTS (1<<IOPORT_MAXBITS)
+
+void *ioport_table[1<<(IOPORT_MAXBITS-IOPORT_PAGEBITS)];
+
 /* 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,30 +305,67 @@
 static IOPortReadFunc default_ioport_readb, default_ioport_readw, 
default_ioport_readl;
 static IOPortWriteFunc default_ioport_writeb, default_ioport_writew, 
default_ioport_writel;
 
+
+static ioport_t ioport_default_func = {
+    NULL,
+    {default_ioport_readb, default_ioport_readw, default_ioport_readl},
+    {default_ioport_writeb, default_ioport_writew, default_ioport_writel},
+    NULL
+};
+
+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
+    if (address >= (1<<IOPORT_MAXBITS)) {
+       hw_error("Maximum port # for this architecture is %d.  "
+                "Port %d requested.", (1<<IOPORT_MAXBITS)-1, address);
+    }
+#endif
+    if (ioport_table[page] == NULL) {
+       if (allocate) {
+           ioport_table[page]=qemu_mallocz((1<<IOPORT_PAGEBITS) * 
+                                           sizeof(ioport_t));
+#ifdef DEBUG_IOPORT_FIND
+           printf("Initializing ioport page %d to: %p\n", page, 
ioport_table[page]);
+#endif
+       } else {
+           return &ioport_default_func;
+       }
+    }
+    ioport_t *p = (ioport_table[page] + entry * sizeof(ioport_t));
+  
+#ifdef DEBUG_IOPORT_FIND
+    printf("port find %d:  page=%d, address=%p, entry=%d, address=%p\n", 
+          address, page, ioport_table[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] = {
-        default_ioport_readb,
-        default_ioport_readw,
-        default_ioport_readl
-    };
-    IOPortReadFunc *func = ioport_read_table[index][address];
-    if (!func)
-        func = default_func[index];
-    return func(ioport_opaque[address], address);
+    ioport_t *p = ioport_find(address, 0);
+    IOPortReadFunc *func = p->read[index];
+    void *opaque = p->opaque;
+    if (!func) {
+        func = ioport_default_func.read[index];
+    }
+    return func(opaque, address);
 }
 
 static void ioport_write(int index, uint32_t address, uint32_t data)
 {
-    static IOPortWriteFunc *default_func[3] = {
-        default_ioport_writeb,
-        default_ioport_writew,
-        default_ioport_writel
-    };
-    IOPortWriteFunc *func = ioport_write_table[index][address];
-    if (!func)
-        func = default_func[index];
-    func(ioport_opaque[address], address, data);
+    ioport_t *p = ioport_find(address, 0);
+    IOPortWriteFunc *func = p->write[index];
+    void *opaque = p->opaque;
+    if (!func) {
+        func = ioport_default_func.write[index];
+    }
+    func(opaque, address, data);
 }
 
 static uint32_t default_ioport_readb(void *opaque, uint32_t address)
@@ -378,10 +432,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 +458,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 +472,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;
     }
 }