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

[Beth Kon <eak@us.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 2359 bytes --]

Alex Williamson wrote:
> 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.
>   
Binaries don't get added more than once with my patch, as long as the 
"proper" number of binaries is entered (e.g., number of table 4 entries 
matches number of cpus specified). But I think your approach has an 
advantage if an improper number of tables is entered. If one table 4 
binary is entered and -smp 2 is specified, my patch would create 2 
identical tables and yours would only create one. Wrong in both cases, 
but yours matches what the user entered.

This leads to the question of whether qemu should verify that 
command-line smbios tables make "sense" (e.g. number of type 4 tables == 
smp_cpus).  The SMBIOS spec has many cases where multiples of a table 
are required. Covering all of them would be very burdensome with little 
or no payback. But I think it is worth putting in a check for table 4, 
since it is relatively simple and is pretty likely to be specified on 
the command line.

One other nit I fixed was detection of a bad filename.

So if you agree, I'd like to submit your rombios32.c patch along with my 
table 4 check patch (combined here for simplicity).

[-- Attachment #2: smbios_fix.patch --]
[-- Type: text/x-patch, Size: 2812 bytes --]

diff --git a/hw/smbios.c b/hw/smbios.c
index ced90ce..a8b52c7 100644
--- a/hw/smbios.c
+++ b/hw/smbios.c
@@ -173,8 +173,8 @@ int smbios_entry_add(const char *t)
         struct smbios_table *table;
         int size = get_image_size(buf);
 
-        if (size < sizeof(struct smbios_structure_header)) {
-            fprintf(stderr, "Cannot read smbios file %s", buf);
+        if (size == -1 || size < sizeof(struct smbios_structure_header)) {
+            fprintf(stderr, "Cannot read smbios file %s\n", buf);
             exit(1);
         }
 
@@ -196,6 +196,9 @@ int smbios_entry_add(const char *t)
 
         header = (struct smbios_structure_header *)(table->data);
         smbios_check_collision(header->type, SMBIOS_TABLE_ENTRY);
+        if (header->type == 4) {
+            smbios_type4_count++;
+        }
 
         smbios_entries_len += sizeof(*table) + size;
         (*(uint16_t *)smbios_entries) =
diff --git a/kvm/bios/rombios32.c b/kvm/bios/rombios32.c
index cbd5f15..f3e75f8 100755
--- a/kvm/bios/rombios32.c
+++ b/kvm/bios/rombios32.c
@@ -2531,13 +2531,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)
diff --git a/pc-bios/bios.bin b/pc-bios/bios.bin
index 70bd7ad..50d5365 100644
Binary files a/pc-bios/bios.bin and b/pc-bios/bios.bin differ
diff --git a/sysemu.h b/sysemu.h
index 47d001e..0b982ed 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -115,6 +115,7 @@ extern int rtc_td_hack;
 extern int alt_grab;
 extern int usb_enabled;
 extern int smp_cpus;
+extern int smbios_type4_count;
 extern int cursor_hide;
 extern int graphic_rotate;
 extern int no_quit;
diff --git a/vl.c b/vl.c
index db8265b..c9cc5b7 100644
--- a/vl.c
+++ b/vl.c
@@ -246,6 +246,7 @@ int singlestep = 0;
 const char *assigned_devices[MAX_DEV_ASSIGN_CMDLINE];
 int assigned_devices_index;
 int smp_cpus = 1;
+int smbios_type4_count = 0;
 const char *vnc_display;
 int acpi_enabled = 1;
 int no_hpet = 0;
@@ -5775,6 +5776,14 @@ int main(int argc, char **argv, char **envp)
         }
     }
 
+#ifdef TARGET_I386
+    if (smbios_type4_count && smbios_type4_count != smp_cpus) {
+         fprintf(stderr,
+                 "count of SMBIOS type 4 tables != SMP CPUs specified.\n");
+         exit(1);
+    }
+#endif
+
 #if defined(CONFIG_KVM) && defined(CONFIG_KQEMU)
     if (kvm_allowed && kqemu_allowed) {
         fprintf(stderr,