Re: [Qemu-devel] Re: [PATCH] Correct SMBIOS handling of multiple tables

[Alex Williamson <alex.williamson@hp.com>]

On Wed, 2009-05-27 at 20:10 -0400, Beth Kon wrote:
> Without this patch, the current code has a problem even in the case of 
> default tables (nothing specified on command line). Following the code 
> path for add_struct(4, p, cpu_num) (for example) add_struct is called 
> once for each cpu. add_struct calls smbios_load_external, and the first 
> thing that does is check used_bitmap. The first pass through for table 4 
> works. But if the vm is smp, the second pass through doesn't work 
> because used_bitmap reports that a table 4 entry was already created, so 
> smbios_load_external returns a 1 and add_struct becomes a noop in 
> effect. I think this patch does what you intended, to override default 
> creation with external creation if specified on the command line, and to 
> get only one complete set of tables for each type.

Oh, I see the problem now, thanks for the explanation.  I don't think
that's the right fix though.  If we remove the duplicate check at the
start of smbios_load_external() and the user passes a binary, we might
add it more than once.  For instance the case where the user specifies
'-smp 2' and passes 2 type 4 entries, they would end up with 4 type 4
entries.

Don't we just need to only mark the bitmap if we load something
externally?  Something like this:

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
--

diff --git a/bios/rombios32.c b/bios/rombios32.c
index f861f81..c869798 100644
--- a/bios/rombios32.c
+++ b/bios/rombios32.c
@@ -2554,13 +2554,14 @@ smbios_load_external(int type, char **p, unsigned *nr_structs,
             *max_struct_size = *p - (char *)header;
     }
 
-    /* Mark that we've reported on this type */
-    used_bitmap[(type >> 6) & 0x3] |= (1ULL << (type & 0x3f));
+    if (start != *p) {
+        /* Mark that we've reported on this type */
+        used_bitmap[(type >> 6) & 0x3] |= (1ULL << (type & 0x3f));
+        return 1;
+    }
 
-    return (start != *p);
-#else /* !BX_QEMU */
+#endif /* !BX_QEMU */
     return 0;
-#endif
 }
 
 void smbios_init(void)