qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Beth Kon
Subject: Re: [Qemu-devel] Re: [PATCH] Correct SMBIOS handling of multiple tables
Date: Thu, 28 May 2009 12:29:47 -0400
User-agent: Thunderbird 2.0.0.21 (X11/20090318)

Alex Williamson wrote:
On Thu, 2009-05-28 at 11:05 -0400, Beth Kon wrote:
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.

I think that makes sense, we can add other checks as needed.

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

Patch comment below....

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
+

I'm not thrilled with adding globals and externs in unrelated parts of
the code.  I think we can keep this all internal to smbios.c if we make
a new smbios_validate_table() function that gets called from
smbios_get_table().  Thanks,

Alex


Yep, I agree. I didn't like that either. Your solution is better. I'll change that and submit the userspace part and, as discussed, you can submit the rombios32.c change.





reply via email to

[Prev in Thread] Current Thread [Next in Thread]