From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59922) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c4qyu-000746-Hr for qemu-devel@nongnu.org; Thu, 10 Nov 2016 10:09:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c4qyr-00082V-CP for qemu-devel@nongnu.org; Thu, 10 Nov 2016 10:09:36 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55800) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c4qyr-00081u-4K for qemu-devel@nongnu.org; Thu, 10 Nov 2016 10:09:33 -0500 Date: Thu, 10 Nov 2016 17:09:30 +0200 From: "Michael S. Tsirkin" Message-ID: <20161110170840-mutt-send-email-mst@kernel.org> References: <20160906082833.25428-1-lma@suse.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160906082833.25428-1-lma@suse.com> Subject: Re: [Qemu-devel] [PATCH] smbios: Add 1 terminator if there is any string field defined in given table. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Lin Ma Cc: imammedo@redhat.com, qemu-devel@nongnu.org, famz@redhat.com, lersek@redhat.com On Tue, Sep 06, 2016 at 04:28:33PM +0800, Lin Ma wrote: > If user specifies binary file on command line to load smbios entries, then > will get error messages while decoding them in guest. > > Reproducer: > 1. dump a smbios table to a binary file from host or guest.(says table 1) > 2. load the binary file through command line: 'qemu -smbios file=...'. > 3. perform 'dmidecode' or 'dmidecode -t 1' in guest. > > It reports 'Invalid entry length...' because qemu doesn't add terminator(s) for > the table correctly. > For smbios tables which have string field provided, qemu should add 1 terminator. > For smbios tables which dont have string field provided, qemu should add 2. > > This patch fixed the issue. > > Signed-off-by: Lin Ma Seems to make sense superficially Acked-by: Michael S. Tsirkin Fam, would you like to take this? > --- > hw/smbios/smbios.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++ > include/hw/smbios/smbios.h | 44 +++++++++++++++++++++++ > 2 files changed, 134 insertions(+) > > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c > index 74c7102..6293bc5 100644 > --- a/hw/smbios/smbios.c > +++ b/hw/smbios/smbios.c > @@ -885,6 +885,9 @@ void smbios_entry_add(QemuOpts *opts) > { > const char *val; > > + int i, terminator_count = 2, table_str_field_count = 0; > + int *tables_str_field_offset = NULL; > + > assert(!smbios_immutable); > > val = qemu_opt_get(opts, "file"); > @@ -926,7 +929,94 @@ void smbios_entry_add(QemuOpts *opts) > smbios_type4_count++; > } > > + switch (header->type) { > + case 0: > + tables_str_field_offset = g_malloc0(sizeof(int) * \ > + TYPE_0_STR_FIELD_COUNT); > + tables_str_field_offset = (int []){\ > + TYPE_0_STR_FIELD_OFFSET_VENDOR, \ > + TYPE_0_STR_FIELD_OFFSET_BIOS_VERSION, \ > + TYPE_0_STR_FIELD_OFFSET_BIOS_RELEASE_DATE}; > + table_str_field_count = sizeof(tables_str_field_offset) / \ > + sizeof(tables_str_field_offset[0]); > + break; > + case 1: > + tables_str_field_offset = g_malloc0(sizeof(int) * \ > + TYPE_1_STR_FIELD_COUNT); > + tables_str_field_offset = (int []){ > + TYPE_1_STR_FIELD_OFFSET_MANUFACTURER, \ > + TYPE_1_STR_FIELD_OFFSET_PRODUCT, \ > + TYPE_1_STR_FIELD_OFFSET_VERSION, \ > + TYPE_1_STR_FIELD_OFFSET_SERIAL, \ > + TYPE_1_STR_FIELD_OFFSET_SKU, \ > + TYPE_1_STR_FIELD_OFFSET_FAMILY}; > + table_str_field_count = sizeof(tables_str_field_offset) / \ > + sizeof(tables_str_field_offset[0]); > + break; > + case 2: > + tables_str_field_offset = g_malloc0(sizeof(int) * \ > + TYPE_2_STR_FIELD_COUNT); > + tables_str_field_offset = (int []){\ > + TYPE_2_STR_FIELD_OFFSET_MANUFACTURER, \ > + TYPE_2_STR_FIELD_OFFSET_PRODUCT, \ > + TYPE_2_STR_FIELD_OFFSET_VERSION, \ > + TYPE_2_STR_FIELD_OFFSET_SERIAL, \ > + TYPE_2_STR_FIELD_OFFSET_ASSET, \ > + TYPE_2_STR_FIELD_OFFSET_LOCATION}; > + table_str_field_count = sizeof(tables_str_field_offset) / \ > + sizeof(tables_str_field_offset[0]); > + break; > + case 3: > + tables_str_field_offset = g_malloc0(sizeof(int) * \ > + TYPE_3_STR_FIELD_COUNT); > + tables_str_field_offset = (int []){\ > + TYPE_3_STR_FIELD_OFFSET_MANUFACTURER, \ > + TYPE_3_STR_FIELD_OFFSET_VERSION, \ > + TYPE_3_STR_FIELD_OFFSET_SERIAL, \ > + TYPE_3_STR_FIELD_OFFSET_ASSET, \ > + TYPE_3_STR_FIELD_OFFSET_SKU}; > + table_str_field_count = sizeof(tables_str_field_offset) / \ > + sizeof(tables_str_field_offset[0]); > + break; > + case 4: > + tables_str_field_offset = g_malloc0(sizeof(int) * \ > + TYPE_4_STR_FIELD_COUNT); > + tables_str_field_offset = (int []){\ > + TYPE_4_STR_FIELD_OFFSET_SOCKET, \ > + TYPE_4_STR_FIELD_OFFSET_PROCESSOR_MANUFACTURER, \ > + TYPE_4_STR_FIELD_OFFSET_PROCESSOR_VERSION, \ > + TYPE_4_STR_FIELD_OFFSET_SERIAL, \ > + TYPE_4_STR_FIELD_OFFSET_ASSET, \ > + TYPE_4_STR_FIELD_OFFSET_PART}; > + table_str_field_count = sizeof(tables_str_field_offset) / \ > + sizeof(tables_str_field_offset[0]); > + break; > + case 17: > + tables_str_field_offset = g_malloc0(sizeof(int) * \ > + TYPE_17_STR_FIELD_COUNT); > + tables_str_field_offset = (int []){\ > + TYPE_17_STR_FIELD_OFFSET_DEVICE_LOCATOR, \ > + TYPE_17_STR_FIELD_OFFSET_BANK_LOCATOR, \ > + TYPE_17_STR_FIELD_OFFSET_MANUFACTURER, \ > + TYPE_17_STR_FIELD_OFFSET_SERIAL, \ > + TYPE_17_STR_FIELD_OFFSET_ASSET, \ > + TYPE_17_STR_FIELD_OFFSET_PART}; > + table_str_field_count = sizeof(tables_str_field_offset) / \ > + sizeof(tables_str_field_offset[0]); > + break; > + default: > + break; > + } > + > + for (i = 0; i < table_str_field_count; i++) { > + if (*(uint8_t *)(smbios_tables + tables_str_field_offset[i]) > 0) { > + terminator_count = 1; > + break; > + } > + } > + > smbios_tables_len += size; > + smbios_tables_len += terminator_count; > if (size > smbios_table_max) { > smbios_table_max = size; > } > diff --git a/include/hw/smbios/smbios.h b/include/hw/smbios/smbios.h > index 1cd53cc..6d59c3d 100644 > --- a/include/hw/smbios/smbios.h > +++ b/include/hw/smbios/smbios.h > @@ -267,4 +267,48 @@ void smbios_get_tables(const struct smbios_phys_mem_area *mem_array, > const unsigned int mem_array_size, > uint8_t **tables, size_t *tables_len, > uint8_t **anchor, size_t *anchor_len); > + > +#define TYPE_0_STR_FIELD_OFFSET_VENDOR 0x4 > +#define TYPE_0_STR_FIELD_OFFSET_BIOS_VERSION 0x5 > +#define TYPE_0_STR_FIELD_OFFSET_BIOS_RELEASE_DATE 0x8 > +#define TYPE_0_STR_FIELD_COUNT 3 > + > +#define TYPE_1_STR_FIELD_OFFSET_MANUFACTURER 0x4 > +#define TYPE_1_STR_FIELD_OFFSET_PRODUCT 0x5 > +#define TYPE_1_STR_FIELD_OFFSET_VERSION 0x6 > +#define TYPE_1_STR_FIELD_OFFSET_SERIAL 0x7 > +#define TYPE_1_STR_FIELD_OFFSET_SKU 0x19 > +#define TYPE_1_STR_FIELD_OFFSET_FAMILY 0x1a > +#define TYPE_1_STR_FIELD_COUNT 6 > + > +#define TYPE_2_STR_FIELD_OFFSET_MANUFACTURER 0x4 > +#define TYPE_2_STR_FIELD_OFFSET_PRODUCT 0x5 > +#define TYPE_2_STR_FIELD_OFFSET_VERSION 0x6 > +#define TYPE_2_STR_FIELD_OFFSET_SERIAL 0x7 > +#define TYPE_2_STR_FIELD_OFFSET_ASSET 0x8 > +#define TYPE_2_STR_FIELD_OFFSET_LOCATION 0xa > +#define TYPE_2_STR_FIELD_COUNT 6 > + > +#define TYPE_3_STR_FIELD_OFFSET_MANUFACTURER 0x4 > +#define TYPE_3_STR_FIELD_OFFSET_VERSION 0x6 > +#define TYPE_3_STR_FIELD_OFFSET_SERIAL 0x7 > +#define TYPE_3_STR_FIELD_OFFSET_ASSET 0x8 > +#define TYPE_3_STR_FIELD_OFFSET_SKU 0x14 > +#define TYPE_3_STR_FIELD_COUNT 5 > + > +#define TYPE_4_STR_FIELD_OFFSET_SOCKET 0x4 > +#define TYPE_4_STR_FIELD_OFFSET_PROCESSOR_MANUFACTURER 0x7 > +#define TYPE_4_STR_FIELD_OFFSET_PROCESSOR_VERSION 0x10 > +#define TYPE_4_STR_FIELD_OFFSET_SERIAL 0x20 > +#define TYPE_4_STR_FIELD_OFFSET_ASSET 0x21 > +#define TYPE_4_STR_FIELD_OFFSET_PART 0x22 > +#define TYPE_4_STR_FIELD_COUNT 6 > + > +#define TYPE_17_STR_FIELD_OFFSET_DEVICE_LOCATOR 0x10 > +#define TYPE_17_STR_FIELD_OFFSET_BANK_LOCATOR 0x11 > +#define TYPE_17_STR_FIELD_OFFSET_MANUFACTURER 0x17 > +#define TYPE_17_STR_FIELD_OFFSET_SERIAL 0x18 > +#define TYPE_17_STR_FIELD_OFFSET_ASSET 0x19 > +#define TYPE_17_STR_FIELD_OFFSET_PART 0x1a > +#define TYPE_17_STR_FIELD_COUNT 6 > #endif /* QEMU_SMBIOS_H */ > -- > 2.9.2