From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1M9h9J-0001K9-GK for qemu-devel@nongnu.org; Thu, 28 May 2009 11:04:05 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1M9h9F-0001JV-Lk for qemu-devel@nongnu.org; Thu, 28 May 2009 11:04:05 -0400 Received: from [199.232.76.173] (port=53933 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1M9h9F-0001JP-8X for qemu-devel@nongnu.org; Thu, 28 May 2009 11:04:01 -0400 Received: from mx20.gnu.org ([199.232.41.8]:39899) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1M9h9E-0003r8-FF for qemu-devel@nongnu.org; Thu, 28 May 2009 11:04:00 -0400 Received: from e1.ny.us.ibm.com ([32.97.182.141]) by mx20.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1M9h9B-0004La-M8 for qemu-devel@nongnu.org; Thu, 28 May 2009 11:03:58 -0400 Received: from d01relay07.pok.ibm.com (d01relay07.pok.ibm.com [9.56.227.147]) by e1.ny.us.ibm.com (8.13.1/8.13.1) with ESMTP id n4SExln0022884 for ; Thu, 28 May 2009 10:59:47 -0400 Received: from d03av04.boulder.ibm.com (d03av04.boulder.ibm.com [9.17.195.170]) by d01relay07.pok.ibm.com (8.13.8/8.13.8/NCO v9.2) with ESMTP id n4SF3nDZ1528038 for ; Thu, 28 May 2009 11:03:49 -0400 Received: from d03av04.boulder.ibm.com (loopback [127.0.0.1]) by d03av04.boulder.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id n4SF3juL005407 for ; Thu, 28 May 2009 09:03:46 -0600 Message-ID: <4A1EA849.3010107@us.ibm.com> Date: Thu, 28 May 2009 11:05:45 -0400 From: Beth Kon MIME-Version: 1.0 Subject: Re: [Qemu-devel] Re: [PATCH] Correct SMBIOS handling of multiple tables References: <1243460525-25570-1-git-send-email-eak@us.ibm.com> <1243466249.20591.64.camel@lappy> <4A1DD659.7090604@us.ibm.com> <1243474701.15223.12.camel@2710p.home> In-Reply-To: <1243474701.15223.12.camel@2710p.home> Content-Type: multipart/mixed; boundary="------------060905030209050500020101" List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: bochs-developers@lists.sourceforge.net, qemu-devel@nongnu.org This is a multi-part message in MIME format. --------------060905030209050500020101 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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). --------------060905030209050500020101 Content-Type: text/x-patch; name="smbios_fix.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="smbios_fix.patch" 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, --------------060905030209050500020101--