From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1M9iSS-0003Wt-US for qemu-devel@nongnu.org; Thu, 28 May 2009 12:27:56 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1M9iSN-0003Vn-Q2 for qemu-devel@nongnu.org; Thu, 28 May 2009 12:27:56 -0400 Received: from [199.232.76.173] (port=52889 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1M9iSN-0003Vk-MS for qemu-devel@nongnu.org; Thu, 28 May 2009 12:27:51 -0400 Received: from e1.ny.us.ibm.com ([32.97.182.141]:58221) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1M9iSN-0005ak-AB for qemu-devel@nongnu.org; Thu, 28 May 2009 12:27:51 -0400 Received: from d01relay04.pok.ibm.com (d01relay04.pok.ibm.com [9.56.227.236]) by e1.ny.us.ibm.com (8.13.1/8.13.1) with ESMTP id n4SGNhEq022060 for ; Thu, 28 May 2009 12:23:43 -0400 Received: from d01av03.pok.ibm.com (d01av03.pok.ibm.com [9.56.224.217]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v9.2) with ESMTP id n4SGRma8152008 for ; Thu, 28 May 2009 12:27:48 -0400 Received: from d01av03.pok.ibm.com (loopback [127.0.0.1]) by d01av03.pok.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id n4SGRlPL004186 for ; Thu, 28 May 2009 12:27:48 -0400 Message-ID: <4A1EBBFB.5050502@us.ibm.com> Date: Thu, 28 May 2009 12:29:47 -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> <4A1EA849.3010107@us.ibm.com> <1243525878.25164.39.camel@bling> In-Reply-To: <1243525878.25164.39.camel@bling> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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 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.