| Message ID | 1428436304-24044-11-git-send-email-minyard@acm.org |
|---|---|
| State | New |
| Headers | show |
On Tue, Apr 07, 2015 at 02:51:39PM -0500, minyard@acm.org wrote: > From: Corey Minyard <cminyard@mvista.com> > > There was no way to directly add a table entry to the SMBIOS table, > even though the BIOS supports this. So add a function to do this. > This is in preparation for the IPMI handler adding it's SMBIOS table > entry. > > Signed-off-by: Corey Minyard <cminyard@mvista.com> Do we have to use a callback for this? It seems that if smbios_table_entry_add just added the table itself to some array or a linked list, then devices could just call smbios_table_entry_add instead of registering a handler. And smbios_get_tables would scan that and get the tables. On systems without smbios, this function could be a nop stub. Did I miss something? > --- > hw/i386/smbios.c | 149 ++++++++++++++++++++++++++++++----------------- > include/hw/i386/smbios.h | 13 +++++ > 2 files changed, 110 insertions(+), 52 deletions(-) > > diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c > index 1341e02..fe15325 100644 > --- a/hw/i386/smbios.c > +++ b/hw/i386/smbios.c > @@ -831,6 +831,25 @@ static void smbios_entry_point_setup(void) > ep.structure_table_address = cpu_to_le32(0); > } > > +struct smbios_device_handler { > + void (*handle_device_table)(void *opaque); > + void *opaque; > + struct smbios_device_handler *prev; > +}; > +static struct smbios_device_handler *device_handlers; > + > +void smbios_register_device_table_handler(void (*handle_device_table) > + (void *opaque), > + void *opaque) > +{ > + struct smbios_device_handler *handler = g_malloc(sizeof(*handler)); > + > + handler->handle_device_table = handle_device_table; > + handler->opaque = opaque; > + handler->prev = device_handlers; > + device_handlers = handler; > +} > + > void smbios_get_tables(uint8_t **tables, size_t *tables_len, > uint8_t **anchor, size_t *anchor_len) > { > @@ -875,6 +894,14 @@ void smbios_get_tables(uint8_t **tables, size_t *tables_len, > } > > smbios_build_type_32_table(); > + > + while (device_handlers) { > + struct smbios_device_handler *handler = device_handlers; > + device_handlers = handler->prev; > + handler->handle_device_table(handler->opaque); > + g_free(handler); > + } > + > smbios_build_type_127_table(); > > smbios_validate_table(); > @@ -898,6 +925,71 @@ static void save_opt(const char **dest, QemuOpts *opts, const char *name) > } > } > > +int smbios_table_entry_add(void *data, int size, bool append_zeros) > +{ > + struct smbios_structure_header *header; > + struct smbios_table *table; /* legacy mode only */ > + > + /* > + * NOTE: standard double '\0' terminator expected, per smbios spec. > + * (except in legacy mode, where the second '\0' is implicit and > + * will be inserted by the BIOS). > + */ > + if (append_zeros) { > + size += 2; > + } > + smbios_tables = g_realloc(smbios_tables, smbios_tables_len + size); > + header = (struct smbios_structure_header *)(smbios_tables + > + smbios_tables_len); > + > + memcpy(header, data, size); > + > + if (test_bit(header->type, have_fields_bitmap)) { > + error_report("can't load type %d struct, fields already specified!", > + header->type); > + exit(1); > + } > + set_bit(header->type, have_binfile_bitmap); > + > + if (header->type == 4) { > + smbios_type4_count++; > + } > + > + smbios_tables_len += size; > + if (size > smbios_table_max) { > + smbios_table_max = size; > + } > + smbios_table_cnt++; > + > + /* add a copy of the newly loaded blob to legacy smbios_entries */ > + /* NOTE: This code runs before smbios_set_defaults(), so we don't > + * yet know which mode (legacy vs. aggregate-table) will be > + * required. We therefore add the binary blob to both legacy > + * (smbios_entries) and aggregate (smbios_tables) tables, and > + * delete the one we don't need from smbios_set_defaults(), > + * once we know which machine version has been requested. > + */ > + if (!smbios_entries) { > + smbios_entries_len = sizeof(uint16_t); > + smbios_entries = g_malloc0(smbios_entries_len); > + } > + if (append_zeros) { > + size -= 1; /* The BIOS adds the second zero in legacy mode. */ > + } > + smbios_entries = g_realloc(smbios_entries, smbios_entries_len + > + size + sizeof(*table)); > + table = (struct smbios_table *)(smbios_entries + smbios_entries_len); > + table->header.type = SMBIOS_TABLE_ENTRY; > + table->header.length = cpu_to_le16(sizeof(*table) + size); > + memcpy(table->data, header, size); > + smbios_entries_len += sizeof(*table) + size; > + (*(uint16_t *)smbios_entries) = > + cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1); > + /* end: add a copy of the newly loaded blob to legacy smbios_entries */ > + > + return 0; > +} > + > void smbios_entry_add(QemuOpts *opts) > { > Error *local_err = NULL; > @@ -907,9 +999,8 @@ void smbios_entry_add(QemuOpts *opts) > > val = qemu_opt_get(opts, "file"); > if (val) { > - struct smbios_structure_header *header; > int size; > - struct smbios_table *table; /* legacy mode only */ > + uint8_t *data; > > qemu_opts_validate(opts, qemu_smbios_file_opts, &local_err); > if (local_err) { > @@ -923,60 +1014,14 @@ void smbios_entry_add(QemuOpts *opts) > exit(1); > } > > - /* > - * NOTE: standard double '\0' terminator expected, per smbios spec. > - * (except in legacy mode, where the second '\0' is implicit and > - * will be inserted by the BIOS). > - */ > - smbios_tables = g_realloc(smbios_tables, smbios_tables_len + size); > - header = (struct smbios_structure_header *)(smbios_tables + > - smbios_tables_len); > - > - if (load_image(val, (uint8_t *)header) != size) { > + data = g_malloc(size); > + if (load_image(val, data) != size) { > error_report("Failed to load SMBIOS file %s", val); > exit(1); > } > > - if (test_bit(header->type, have_fields_bitmap)) { > - error_report("can't load type %d struct, fields already specified!", > - header->type); > - exit(1); > - } > - set_bit(header->type, have_binfile_bitmap); > - > - if (header->type == 4) { > - smbios_type4_count++; > - } > - > - smbios_tables_len += size; > - if (size > smbios_table_max) { > - smbios_table_max = size; > - } > - smbios_table_cnt++; > - > - /* add a copy of the newly loaded blob to legacy smbios_entries */ > - /* NOTE: This code runs before smbios_set_defaults(), so we don't > - * yet know which mode (legacy vs. aggregate-table) will be > - * required. We therefore add the binary blob to both legacy > - * (smbios_entries) and aggregate (smbios_tables) tables, and > - * delete the one we don't need from smbios_set_defaults(), > - * once we know which machine version has been requested. > - */ > - if (!smbios_entries) { > - smbios_entries_len = sizeof(uint16_t); > - smbios_entries = g_malloc0(smbios_entries_len); > - } > - smbios_entries = g_realloc(smbios_entries, smbios_entries_len + > - size + sizeof(*table)); > - table = (struct smbios_table *)(smbios_entries + smbios_entries_len); > - table->header.type = SMBIOS_TABLE_ENTRY; > - table->header.length = cpu_to_le16(sizeof(*table) + size); > - memcpy(table->data, header, size); > - smbios_entries_len += sizeof(*table) + size; > - (*(uint16_t *)smbios_entries) = > - cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1); > - /* end: add a copy of the newly loaded blob to legacy smbios_entries */ > - > + smbios_table_entry_add(data, size, false); > + g_free(data); > return; > } > > diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h > index d2850be..656327d 100644 > --- a/include/hw/i386/smbios.h > +++ b/include/hw/i386/smbios.h > @@ -27,6 +27,19 @@ void smbios_get_tables(uint8_t **tables, size_t *tables_len, > uint8_t **anchor, size_t *anchor_len); > > /* > + * Add an external entry to the SMBIOS table. Can only be called > + * from a registered device table handler. > + */ > +int smbios_table_entry_add(void *data, int size, bool append_zeros); > + > +/* > + * When constructing SMBIOS tables, call a function at the end of the > + * add process to allow devices to add their own SMBIOS table entries. > + */ > +void smbios_register_device_table_handler(void (*handle_device_table) > + (void *opaque), > + void *opaque); > +/* > * SMBIOS spec defined tables > */ > > -- > 1.8.3.1 >
On 04/12/2015 11:05 AM, Michael S. Tsirkin wrote: > On Tue, Apr 07, 2015 at 02:51:39PM -0500, minyard@acm.org wrote: >> From: Corey Minyard <cminyard@mvista.com> >> >> There was no way to directly add a table entry to the SMBIOS table, >> even though the BIOS supports this. So add a function to do this. >> This is in preparation for the IPMI handler adding it's SMBIOS table >> entry. >> >> Signed-off-by: Corey Minyard <cminyard@mvista.com> > Do we have to use a callback for this? > > It seems that if smbios_table_entry_add just added the table > itself to some array or a linked list, then devices could just call > smbios_table_entry_add instead of registering a handler. > > And smbios_get_tables would scan that and get the tables. Well, there are many ways you could handle this, and it doesn't seem much different in the end to me. You either have a list of items to add to the table or a list of callbacks to fill in the data. I made this the same as the ACPI code, which you have to have as a callback if you are adding it to a common SSDT. Plus it reduced the amount of dynamic allocation required. > > On systems without smbios, this function could be a nop stub. One nice things about the callbacks is you can eliminate any stubs. I find stubs kind of ugly, personally. -corey > > Did I miss something? > >> --- >> hw/i386/smbios.c | 149 ++++++++++++++++++++++++++++++----------------- >> include/hw/i386/smbios.h | 13 +++++ >> 2 files changed, 110 insertions(+), 52 deletions(-) >> >> diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c >> index 1341e02..fe15325 100644 >> --- a/hw/i386/smbios.c >> +++ b/hw/i386/smbios.c >> @@ -831,6 +831,25 @@ static void smbios_entry_point_setup(void) >> ep.structure_table_address = cpu_to_le32(0); >> } >> >> +struct smbios_device_handler { >> + void (*handle_device_table)(void *opaque); >> + void *opaque; >> + struct smbios_device_handler *prev; >> +}; >> +static struct smbios_device_handler *device_handlers; >> + >> +void smbios_register_device_table_handler(void (*handle_device_table) >> + (void *opaque), >> + void *opaque) >> +{ >> + struct smbios_device_handler *handler = g_malloc(sizeof(*handler)); >> + >> + handler->handle_device_table = handle_device_table; >> + handler->opaque = opaque; >> + handler->prev = device_handlers; >> + device_handlers = handler; >> +} >> + >> void smbios_get_tables(uint8_t **tables, size_t *tables_len, >> uint8_t **anchor, size_t *anchor_len) >> { >> @@ -875,6 +894,14 @@ void smbios_get_tables(uint8_t **tables, size_t *tables_len, >> } >> >> smbios_build_type_32_table(); >> + >> + while (device_handlers) { >> + struct smbios_device_handler *handler = device_handlers; >> + device_handlers = handler->prev; >> + handler->handle_device_table(handler->opaque); >> + g_free(handler); >> + } >> + >> smbios_build_type_127_table(); >> >> smbios_validate_table(); >> @@ -898,6 +925,71 @@ static void save_opt(const char **dest, QemuOpts *opts, const char *name) >> } >> } >> >> +int smbios_table_entry_add(void *data, int size, bool append_zeros) >> +{ >> + struct smbios_structure_header *header; >> + struct smbios_table *table; /* legacy mode only */ >> + >> + /* >> + * NOTE: standard double '\0' terminator expected, per smbios spec. >> + * (except in legacy mode, where the second '\0' is implicit and >> + * will be inserted by the BIOS). >> + */ >> + if (append_zeros) { >> + size += 2; >> + } >> + smbios_tables = g_realloc(smbios_tables, smbios_tables_len + size); >> + header = (struct smbios_structure_header *)(smbios_tables + >> + smbios_tables_len); >> + >> + memcpy(header, data, size); >> + >> + if (test_bit(header->type, have_fields_bitmap)) { >> + error_report("can't load type %d struct, fields already specified!", >> + header->type); >> + exit(1); >> + } >> + set_bit(header->type, have_binfile_bitmap); >> + >> + if (header->type == 4) { >> + smbios_type4_count++; >> + } >> + >> + smbios_tables_len += size; >> + if (size > smbios_table_max) { >> + smbios_table_max = size; >> + } >> + smbios_table_cnt++; >> + >> + /* add a copy of the newly loaded blob to legacy smbios_entries */ >> + /* NOTE: This code runs before smbios_set_defaults(), so we don't >> + * yet know which mode (legacy vs. aggregate-table) will be >> + * required. We therefore add the binary blob to both legacy >> + * (smbios_entries) and aggregate (smbios_tables) tables, and >> + * delete the one we don't need from smbios_set_defaults(), >> + * once we know which machine version has been requested. >> + */ >> + if (!smbios_entries) { >> + smbios_entries_len = sizeof(uint16_t); >> + smbios_entries = g_malloc0(smbios_entries_len); >> + } >> + if (append_zeros) { >> + size -= 1; /* The BIOS adds the second zero in legacy mode. */ >> + } >> + smbios_entries = g_realloc(smbios_entries, smbios_entries_len + >> + size + sizeof(*table)); >> + table = (struct smbios_table *)(smbios_entries + smbios_entries_len); >> + table->header.type = SMBIOS_TABLE_ENTRY; >> + table->header.length = cpu_to_le16(sizeof(*table) + size); >> + memcpy(table->data, header, size); >> + smbios_entries_len += sizeof(*table) + size; >> + (*(uint16_t *)smbios_entries) = >> + cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1); >> + /* end: add a copy of the newly loaded blob to legacy smbios_entries */ >> + >> + return 0; >> +} >> + >> void smbios_entry_add(QemuOpts *opts) >> { >> Error *local_err = NULL; >> @@ -907,9 +999,8 @@ void smbios_entry_add(QemuOpts *opts) >> >> val = qemu_opt_get(opts, "file"); >> if (val) { >> - struct smbios_structure_header *header; >> int size; >> - struct smbios_table *table; /* legacy mode only */ >> + uint8_t *data; >> >> qemu_opts_validate(opts, qemu_smbios_file_opts, &local_err); >> if (local_err) { >> @@ -923,60 +1014,14 @@ void smbios_entry_add(QemuOpts *opts) >> exit(1); >> } >> >> - /* >> - * NOTE: standard double '\0' terminator expected, per smbios spec. >> - * (except in legacy mode, where the second '\0' is implicit and >> - * will be inserted by the BIOS). >> - */ >> - smbios_tables = g_realloc(smbios_tables, smbios_tables_len + size); >> - header = (struct smbios_structure_header *)(smbios_tables + >> - smbios_tables_len); >> - >> - if (load_image(val, (uint8_t *)header) != size) { >> + data = g_malloc(size); >> + if (load_image(val, data) != size) { >> error_report("Failed to load SMBIOS file %s", val); >> exit(1); >> } >> >> - if (test_bit(header->type, have_fields_bitmap)) { >> - error_report("can't load type %d struct, fields already specified!", >> - header->type); >> - exit(1); >> - } >> - set_bit(header->type, have_binfile_bitmap); >> - >> - if (header->type == 4) { >> - smbios_type4_count++; >> - } >> - >> - smbios_tables_len += size; >> - if (size > smbios_table_max) { >> - smbios_table_max = size; >> - } >> - smbios_table_cnt++; >> - >> - /* add a copy of the newly loaded blob to legacy smbios_entries */ >> - /* NOTE: This code runs before smbios_set_defaults(), so we don't >> - * yet know which mode (legacy vs. aggregate-table) will be >> - * required. We therefore add the binary blob to both legacy >> - * (smbios_entries) and aggregate (smbios_tables) tables, and >> - * delete the one we don't need from smbios_set_defaults(), >> - * once we know which machine version has been requested. >> - */ >> - if (!smbios_entries) { >> - smbios_entries_len = sizeof(uint16_t); >> - smbios_entries = g_malloc0(smbios_entries_len); >> - } >> - smbios_entries = g_realloc(smbios_entries, smbios_entries_len + >> - size + sizeof(*table)); >> - table = (struct smbios_table *)(smbios_entries + smbios_entries_len); >> - table->header.type = SMBIOS_TABLE_ENTRY; >> - table->header.length = cpu_to_le16(sizeof(*table) + size); >> - memcpy(table->data, header, size); >> - smbios_entries_len += sizeof(*table) + size; >> - (*(uint16_t *)smbios_entries) = >> - cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1); >> - /* end: add a copy of the newly loaded blob to legacy smbios_entries */ >> - >> + smbios_table_entry_add(data, size, false); >> + g_free(data); >> return; >> } >> >> diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h >> index d2850be..656327d 100644 >> --- a/include/hw/i386/smbios.h >> +++ b/include/hw/i386/smbios.h >> @@ -27,6 +27,19 @@ void smbios_get_tables(uint8_t **tables, size_t *tables_len, >> uint8_t **anchor, size_t *anchor_len); >> >> /* >> + * Add an external entry to the SMBIOS table. Can only be called >> + * from a registered device table handler. >> + */ >> +int smbios_table_entry_add(void *data, int size, bool append_zeros); >> + >> +/* >> + * When constructing SMBIOS tables, call a function at the end of the >> + * add process to allow devices to add their own SMBIOS table entries. >> + */ >> +void smbios_register_device_table_handler(void (*handle_device_table) >> + (void *opaque), >> + void *opaque); >> +/* >> * SMBIOS spec defined tables >> */ >> >> -- >> 1.8.3.1 >>
On Sun, Apr 12, 2015 at 08:26:46PM -0500, Corey Minyard wrote: > On 04/12/2015 11:05 AM, Michael S. Tsirkin wrote: > > On Tue, Apr 07, 2015 at 02:51:39PM -0500, minyard@acm.org wrote: > >> From: Corey Minyard <cminyard@mvista.com> > >> > >> There was no way to directly add a table entry to the SMBIOS table, > >> even though the BIOS supports this. So add a function to do this. > >> This is in preparation for the IPMI handler adding it's SMBIOS table > >> entry. > >> > >> Signed-off-by: Corey Minyard <cminyard@mvista.com> > > Do we have to use a callback for this? > > > > It seems that if smbios_table_entry_add just added the table > > itself to some array or a linked list, then devices could just call > > smbios_table_entry_add instead of registering a handler. > > > > And smbios_get_tables would scan that and get the tables. > > Well, there are many ways you could handle this, and it doesn't seem > much different in the end to me. You either have a list of items to add > to the table or a list of callbacks to fill in the data. > > I made this the same as the ACPI code, which you have to have as a > callback if you are adding it to a common SSDT. Not really I think. > Plus it reduced the > amount of dynamic allocation required. Not sure why this matters though. > > > > On systems without smbios, this function could be a nop stub. > > One nice things about the callbacks is you can eliminate any stubs. I > find stubs kind of ugly, personally. > > -corey You still need a stub for the register function, don't you? > > > > Did I miss something? > > > >> --- > >> hw/i386/smbios.c | 149 ++++++++++++++++++++++++++++++----------------- > >> include/hw/i386/smbios.h | 13 +++++ > >> 2 files changed, 110 insertions(+), 52 deletions(-) > >> > >> diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c > >> index 1341e02..fe15325 100644 > >> --- a/hw/i386/smbios.c > >> +++ b/hw/i386/smbios.c > >> @@ -831,6 +831,25 @@ static void smbios_entry_point_setup(void) > >> ep.structure_table_address = cpu_to_le32(0); > >> } > >> > >> +struct smbios_device_handler { > >> + void (*handle_device_table)(void *opaque); > >> + void *opaque; > >> + struct smbios_device_handler *prev; > >> +}; > >> +static struct smbios_device_handler *device_handlers; > >> + > >> +void smbios_register_device_table_handler(void (*handle_device_table) > >> + (void *opaque), > >> + void *opaque) > >> +{ > >> + struct smbios_device_handler *handler = g_malloc(sizeof(*handler)); > >> + > >> + handler->handle_device_table = handle_device_table; > >> + handler->opaque = opaque; > >> + handler->prev = device_handlers; > >> + device_handlers = handler; > >> +} > >> + > >> void smbios_get_tables(uint8_t **tables, size_t *tables_len, > >> uint8_t **anchor, size_t *anchor_len) > >> { > >> @@ -875,6 +894,14 @@ void smbios_get_tables(uint8_t **tables, size_t *tables_len, > >> } > >> > >> smbios_build_type_32_table(); > >> + > >> + while (device_handlers) { > >> + struct smbios_device_handler *handler = device_handlers; > >> + device_handlers = handler->prev; > >> + handler->handle_device_table(handler->opaque); > >> + g_free(handler); > >> + } > >> + > >> smbios_build_type_127_table(); > >> > >> smbios_validate_table(); > >> @@ -898,6 +925,71 @@ static void save_opt(const char **dest, QemuOpts *opts, const char *name) > >> } > >> } > >> > >> +int smbios_table_entry_add(void *data, int size, bool append_zeros) > >> +{ > >> + struct smbios_structure_header *header; > >> + struct smbios_table *table; /* legacy mode only */ > >> + > >> + /* > >> + * NOTE: standard double '\0' terminator expected, per smbios spec. > >> + * (except in legacy mode, where the second '\0' is implicit and > >> + * will be inserted by the BIOS). > >> + */ > >> + if (append_zeros) { > >> + size += 2; > >> + } > >> + smbios_tables = g_realloc(smbios_tables, smbios_tables_len + size); > >> + header = (struct smbios_structure_header *)(smbios_tables + > >> + smbios_tables_len); > >> + > >> + memcpy(header, data, size); > >> + > >> + if (test_bit(header->type, have_fields_bitmap)) { > >> + error_report("can't load type %d struct, fields already specified!", > >> + header->type); > >> + exit(1); > >> + } > >> + set_bit(header->type, have_binfile_bitmap); > >> + > >> + if (header->type == 4) { > >> + smbios_type4_count++; > >> + } > >> + > >> + smbios_tables_len += size; > >> + if (size > smbios_table_max) { > >> + smbios_table_max = size; > >> + } > >> + smbios_table_cnt++; > >> + > >> + /* add a copy of the newly loaded blob to legacy smbios_entries */ > >> + /* NOTE: This code runs before smbios_set_defaults(), so we don't > >> + * yet know which mode (legacy vs. aggregate-table) will be > >> + * required. We therefore add the binary blob to both legacy > >> + * (smbios_entries) and aggregate (smbios_tables) tables, and > >> + * delete the one we don't need from smbios_set_defaults(), > >> + * once we know which machine version has been requested. > >> + */ > >> + if (!smbios_entries) { > >> + smbios_entries_len = sizeof(uint16_t); > >> + smbios_entries = g_malloc0(smbios_entries_len); > >> + } > >> + if (append_zeros) { > >> + size -= 1; /* The BIOS adds the second zero in legacy mode. */ > >> + } > >> + smbios_entries = g_realloc(smbios_entries, smbios_entries_len + > >> + size + sizeof(*table)); > >> + table = (struct smbios_table *)(smbios_entries + smbios_entries_len); > >> + table->header.type = SMBIOS_TABLE_ENTRY; > >> + table->header.length = cpu_to_le16(sizeof(*table) + size); > >> + memcpy(table->data, header, size); > >> + smbios_entries_len += sizeof(*table) + size; > >> + (*(uint16_t *)smbios_entries) = > >> + cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1); > >> + /* end: add a copy of the newly loaded blob to legacy smbios_entries */ > >> + > >> + return 0; > >> +} > >> + > >> void smbios_entry_add(QemuOpts *opts) > >> { > >> Error *local_err = NULL; > >> @@ -907,9 +999,8 @@ void smbios_entry_add(QemuOpts *opts) > >> > >> val = qemu_opt_get(opts, "file"); > >> if (val) { > >> - struct smbios_structure_header *header; > >> int size; > >> - struct smbios_table *table; /* legacy mode only */ > >> + uint8_t *data; > >> > >> qemu_opts_validate(opts, qemu_smbios_file_opts, &local_err); > >> if (local_err) { > >> @@ -923,60 +1014,14 @@ void smbios_entry_add(QemuOpts *opts) > >> exit(1); > >> } > >> > >> - /* > >> - * NOTE: standard double '\0' terminator expected, per smbios spec. > >> - * (except in legacy mode, where the second '\0' is implicit and > >> - * will be inserted by the BIOS). > >> - */ > >> - smbios_tables = g_realloc(smbios_tables, smbios_tables_len + size); > >> - header = (struct smbios_structure_header *)(smbios_tables + > >> - smbios_tables_len); > >> - > >> - if (load_image(val, (uint8_t *)header) != size) { > >> + data = g_malloc(size); > >> + if (load_image(val, data) != size) { > >> error_report("Failed to load SMBIOS file %s", val); > >> exit(1); > >> } > >> > >> - if (test_bit(header->type, have_fields_bitmap)) { > >> - error_report("can't load type %d struct, fields already specified!", > >> - header->type); > >> - exit(1); > >> - } > >> - set_bit(header->type, have_binfile_bitmap); > >> - > >> - if (header->type == 4) { > >> - smbios_type4_count++; > >> - } > >> - > >> - smbios_tables_len += size; > >> - if (size > smbios_table_max) { > >> - smbios_table_max = size; > >> - } > >> - smbios_table_cnt++; > >> - > >> - /* add a copy of the newly loaded blob to legacy smbios_entries */ > >> - /* NOTE: This code runs before smbios_set_defaults(), so we don't > >> - * yet know which mode (legacy vs. aggregate-table) will be > >> - * required. We therefore add the binary blob to both legacy > >> - * (smbios_entries) and aggregate (smbios_tables) tables, and > >> - * delete the one we don't need from smbios_set_defaults(), > >> - * once we know which machine version has been requested. > >> - */ > >> - if (!smbios_entries) { > >> - smbios_entries_len = sizeof(uint16_t); > >> - smbios_entries = g_malloc0(smbios_entries_len); > >> - } > >> - smbios_entries = g_realloc(smbios_entries, smbios_entries_len + > >> - size + sizeof(*table)); > >> - table = (struct smbios_table *)(smbios_entries + smbios_entries_len); > >> - table->header.type = SMBIOS_TABLE_ENTRY; > >> - table->header.length = cpu_to_le16(sizeof(*table) + size); > >> - memcpy(table->data, header, size); > >> - smbios_entries_len += sizeof(*table) + size; > >> - (*(uint16_t *)smbios_entries) = > >> - cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1); > >> - /* end: add a copy of the newly loaded blob to legacy smbios_entries */ > >> - > >> + smbios_table_entry_add(data, size, false); > >> + g_free(data); > >> return; > >> } > >> > >> diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h > >> index d2850be..656327d 100644 > >> --- a/include/hw/i386/smbios.h > >> +++ b/include/hw/i386/smbios.h > >> @@ -27,6 +27,19 @@ void smbios_get_tables(uint8_t **tables, size_t *tables_len, > >> uint8_t **anchor, size_t *anchor_len); > >> > >> /* > >> + * Add an external entry to the SMBIOS table. Can only be called > >> + * from a registered device table handler. > >> + */ > >> +int smbios_table_entry_add(void *data, int size, bool append_zeros); > >> + > >> +/* > >> + * When constructing SMBIOS tables, call a function at the end of the > >> + * add process to allow devices to add their own SMBIOS table entries. > >> + */ > >> +void smbios_register_device_table_handler(void (*handle_device_table) > >> + (void *opaque), > >> + void *opaque); > >> +/* > >> * SMBIOS spec defined tables > >> */ > >> > >> -- > >> 1.8.3.1 > >>
On 04/13/2015 02:00 AM, Michael S. Tsirkin wrote: > On Sun, Apr 12, 2015 at 08:26:46PM -0500, Corey Minyard wrote: >> On 04/12/2015 11:05 AM, Michael S. Tsirkin wrote: >>> On Tue, Apr 07, 2015 at 02:51:39PM -0500, minyard@acm.org wrote: >>>> From: Corey Minyard <cminyard@mvista.com> >>>> >>>> There was no way to directly add a table entry to the SMBIOS table, >>>> even though the BIOS supports this. So add a function to do this. >>>> This is in preparation for the IPMI handler adding it's SMBIOS table >>>> entry. >>>> >>>> Signed-off-by: Corey Minyard <cminyard@mvista.com> >>> Do we have to use a callback for this? >>> >>> It seems that if smbios_table_entry_add just added the table >>> itself to some array or a linked list, then devices could just call >>> smbios_table_entry_add instead of registering a handler. >>> >>> And smbios_get_tables would scan that and get the tables. >> Well, there are many ways you could handle this, and it doesn't seem >> much different in the end to me. You either have a list of items to add >> to the table or a list of callbacks to fill in the data. >> >> I made this the same as the ACPI code, which you have to have as a >> callback if you are adding it to a common SSDT. > Not really I think. The AML functions require that you have a tree to attach what you are adding. If you did your own SSDT, you wouldn't need a callback. You could add a binary blob that gets put into the SSDT, but I think that would require adding some AML functions. > >> Plus it reduced the >> amount of dynamic allocation required. > Not sure why this matters though. I'm a kernel guy and we are insane about this kind of stuff :). >>> On systems without smbios, this function could be a nop stub. >> One nice things about the callbacks is you can eliminate any stubs. I >> find stubs kind of ugly, personally. >> >> -corey > You still need a stub for the register function, don't you? It depends on how you design it. I had already redesigned it based on Paulo's comments to add the firmware to a holding list in the base IPMI code, then when the callbacks came from SMBIOS and ACPI the code would iterate that list and add the table entries. No stubs required. Creating a separate SSDT and adding the list to ACPI/SMBIOS code is not an issue, either, and stubs aren't a big deal. Thanks, -corey >>> Did I miss something? >>> >>>> --- >>>> hw/i386/smbios.c | 149 ++++++++++++++++++++++++++++++----------------- >>>> include/hw/i386/smbios.h | 13 +++++ >>>> 2 files changed, 110 insertions(+), 52 deletions(-) >>>> >>>> diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c >>>> index 1341e02..fe15325 100644 >>>> --- a/hw/i386/smbios.c >>>> +++ b/hw/i386/smbios.c >>>> @@ -831,6 +831,25 @@ static void smbios_entry_point_setup(void) >>>> ep.structure_table_address = cpu_to_le32(0); >>>> } >>>> >>>> +struct smbios_device_handler { >>>> + void (*handle_device_table)(void *opaque); >>>> + void *opaque; >>>> + struct smbios_device_handler *prev; >>>> +}; >>>> +static struct smbios_device_handler *device_handlers; >>>> + >>>> +void smbios_register_device_table_handler(void (*handle_device_table) >>>> + (void *opaque), >>>> + void *opaque) >>>> +{ >>>> + struct smbios_device_handler *handler = g_malloc(sizeof(*handler)); >>>> + >>>> + handler->handle_device_table = handle_device_table; >>>> + handler->opaque = opaque; >>>> + handler->prev = device_handlers; >>>> + device_handlers = handler; >>>> +} >>>> + >>>> void smbios_get_tables(uint8_t **tables, size_t *tables_len, >>>> uint8_t **anchor, size_t *anchor_len) >>>> { >>>> @@ -875,6 +894,14 @@ void smbios_get_tables(uint8_t **tables, size_t *tables_len, >>>> } >>>> >>>> smbios_build_type_32_table(); >>>> + >>>> + while (device_handlers) { >>>> + struct smbios_device_handler *handler = device_handlers; >>>> + device_handlers = handler->prev; >>>> + handler->handle_device_table(handler->opaque); >>>> + g_free(handler); >>>> + } >>>> + >>>> smbios_build_type_127_table(); >>>> >>>> smbios_validate_table(); >>>> @@ -898,6 +925,71 @@ static void save_opt(const char **dest, QemuOpts *opts, const char *name) >>>> } >>>> } >>>> >>>> +int smbios_table_entry_add(void *data, int size, bool append_zeros) >>>> +{ >>>> + struct smbios_structure_header *header; >>>> + struct smbios_table *table; /* legacy mode only */ >>>> + >>>> + /* >>>> + * NOTE: standard double '\0' terminator expected, per smbios spec. >>>> + * (except in legacy mode, where the second '\0' is implicit and >>>> + * will be inserted by the BIOS). >>>> + */ >>>> + if (append_zeros) { >>>> + size += 2; >>>> + } >>>> + smbios_tables = g_realloc(smbios_tables, smbios_tables_len + size); >>>> + header = (struct smbios_structure_header *)(smbios_tables + >>>> + smbios_tables_len); >>>> + >>>> + memcpy(header, data, size); >>>> + >>>> + if (test_bit(header->type, have_fields_bitmap)) { >>>> + error_report("can't load type %d struct, fields already specified!", >>>> + header->type); >>>> + exit(1); >>>> + } >>>> + set_bit(header->type, have_binfile_bitmap); >>>> + >>>> + if (header->type == 4) { >>>> + smbios_type4_count++; >>>> + } >>>> + >>>> + smbios_tables_len += size; >>>> + if (size > smbios_table_max) { >>>> + smbios_table_max = size; >>>> + } >>>> + smbios_table_cnt++; >>>> + >>>> + /* add a copy of the newly loaded blob to legacy smbios_entries */ >>>> + /* NOTE: This code runs before smbios_set_defaults(), so we don't >>>> + * yet know which mode (legacy vs. aggregate-table) will be >>>> + * required. We therefore add the binary blob to both legacy >>>> + * (smbios_entries) and aggregate (smbios_tables) tables, and >>>> + * delete the one we don't need from smbios_set_defaults(), >>>> + * once we know which machine version has been requested. >>>> + */ >>>> + if (!smbios_entries) { >>>> + smbios_entries_len = sizeof(uint16_t); >>>> + smbios_entries = g_malloc0(smbios_entries_len); >>>> + } >>>> + if (append_zeros) { >>>> + size -= 1; /* The BIOS adds the second zero in legacy mode. */ >>>> + } >>>> + smbios_entries = g_realloc(smbios_entries, smbios_entries_len + >>>> + size + sizeof(*table)); >>>> + table = (struct smbios_table *)(smbios_entries + smbios_entries_len); >>>> + table->header.type = SMBIOS_TABLE_ENTRY; >>>> + table->header.length = cpu_to_le16(sizeof(*table) + size); >>>> + memcpy(table->data, header, size); >>>> + smbios_entries_len += sizeof(*table) + size; >>>> + (*(uint16_t *)smbios_entries) = >>>> + cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1); >>>> + /* end: add a copy of the newly loaded blob to legacy smbios_entries */ >>>> + >>>> + return 0; >>>> +} >>>> + >>>> void smbios_entry_add(QemuOpts *opts) >>>> { >>>> Error *local_err = NULL; >>>> @@ -907,9 +999,8 @@ void smbios_entry_add(QemuOpts *opts) >>>> >>>> val = qemu_opt_get(opts, "file"); >>>> if (val) { >>>> - struct smbios_structure_header *header; >>>> int size; >>>> - struct smbios_table *table; /* legacy mode only */ >>>> + uint8_t *data; >>>> >>>> qemu_opts_validate(opts, qemu_smbios_file_opts, &local_err); >>>> if (local_err) { >>>> @@ -923,60 +1014,14 @@ void smbios_entry_add(QemuOpts *opts) >>>> exit(1); >>>> } >>>> >>>> - /* >>>> - * NOTE: standard double '\0' terminator expected, per smbios spec. >>>> - * (except in legacy mode, where the second '\0' is implicit and >>>> - * will be inserted by the BIOS). >>>> - */ >>>> - smbios_tables = g_realloc(smbios_tables, smbios_tables_len + size); >>>> - header = (struct smbios_structure_header *)(smbios_tables + >>>> - smbios_tables_len); >>>> - >>>> - if (load_image(val, (uint8_t *)header) != size) { >>>> + data = g_malloc(size); >>>> + if (load_image(val, data) != size) { >>>> error_report("Failed to load SMBIOS file %s", val); >>>> exit(1); >>>> } >>>> >>>> - if (test_bit(header->type, have_fields_bitmap)) { >>>> - error_report("can't load type %d struct, fields already specified!", >>>> - header->type); >>>> - exit(1); >>>> - } >>>> - set_bit(header->type, have_binfile_bitmap); >>>> - >>>> - if (header->type == 4) { >>>> - smbios_type4_count++; >>>> - } >>>> - >>>> - smbios_tables_len += size; >>>> - if (size > smbios_table_max) { >>>> - smbios_table_max = size; >>>> - } >>>> - smbios_table_cnt++; >>>> - >>>> - /* add a copy of the newly loaded blob to legacy smbios_entries */ >>>> - /* NOTE: This code runs before smbios_set_defaults(), so we don't >>>> - * yet know which mode (legacy vs. aggregate-table) will be >>>> - * required. We therefore add the binary blob to both legacy >>>> - * (smbios_entries) and aggregate (smbios_tables) tables, and >>>> - * delete the one we don't need from smbios_set_defaults(), >>>> - * once we know which machine version has been requested. >>>> - */ >>>> - if (!smbios_entries) { >>>> - smbios_entries_len = sizeof(uint16_t); >>>> - smbios_entries = g_malloc0(smbios_entries_len); >>>> - } >>>> - smbios_entries = g_realloc(smbios_entries, smbios_entries_len + >>>> - size + sizeof(*table)); >>>> - table = (struct smbios_table *)(smbios_entries + smbios_entries_len); >>>> - table->header.type = SMBIOS_TABLE_ENTRY; >>>> - table->header.length = cpu_to_le16(sizeof(*table) + size); >>>> - memcpy(table->data, header, size); >>>> - smbios_entries_len += sizeof(*table) + size; >>>> - (*(uint16_t *)smbios_entries) = >>>> - cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1); >>>> - /* end: add a copy of the newly loaded blob to legacy smbios_entries */ >>>> - >>>> + smbios_table_entry_add(data, size, false); >>>> + g_free(data); >>>> return; >>>> } >>>> >>>> diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h >>>> index d2850be..656327d 100644 >>>> --- a/include/hw/i386/smbios.h >>>> +++ b/include/hw/i386/smbios.h >>>> @@ -27,6 +27,19 @@ void smbios_get_tables(uint8_t **tables, size_t *tables_len, >>>> uint8_t **anchor, size_t *anchor_len); >>>> >>>> /* >>>> + * Add an external entry to the SMBIOS table. Can only be called >>>> + * from a registered device table handler. >>>> + */ >>>> +int smbios_table_entry_add(void *data, int size, bool append_zeros); >>>> + >>>> +/* >>>> + * When constructing SMBIOS tables, call a function at the end of the >>>> + * add process to allow devices to add their own SMBIOS table entries. >>>> + */ >>>> +void smbios_register_device_table_handler(void (*handle_device_table) >>>> + (void *opaque), >>>> + void *opaque); >>>> +/* >>>> * SMBIOS spec defined tables >>>> */ >>>> >>>> -- >>>> 1.8.3.1 >>>>
On 13/04/2015 18:34, Corey Minyard wrote: > > > I made this the same as the ACPI code, which you have to have as a > > > callback if you are adding it to a common SSDT. > > > > Not really I think. > > The AML functions require that you have a tree to attach what you are > adding. If you did your own SSDT, you wouldn't need a callback. You > could add a binary blob that gets put into the SSDT, but I think that > would require adding some AML functions. I very much prefer the callback idea. Long term it could be used by more devices and possibly it could be turned into an AMLProvider QOM interface. Then the ACPI builder could iterate on all QOM devices and just ask which of them can provide some AML. Also, tables are rebuilt when the firmware loads them, and handing in a blob makes it harder to achieve this on-the-fly modification, compared to callbacks. Paolo
On Mon, Apr 13, 2015 at 06:40:46PM +0200, Paolo Bonzini wrote: > > > On 13/04/2015 18:34, Corey Minyard wrote: > > > > I made this the same as the ACPI code, which you have to have as a > > > > callback if you are adding it to a common SSDT. > > > > > > Not really I think. > > > > The AML functions require that you have a tree to attach what you are > > adding. If you did your own SSDT, you wouldn't need a callback. You > > could add a binary blob that gets put into the SSDT, but I think that > > would require adding some AML functions. > > I very much prefer the callback idea. Long term it could be used by > more devices and possibly it could be turned into an AMLProvider QOM > interface. Then the ACPI builder could iterate on all QOM devices and > just ask which of them can provide some AML. Yes, that would make sense. Devices which have a static AML could provide a static AML property, with very little code, those that have dynamic AML - dynamic AML property with more code. But I don't see callbacks as a step in that direction - more like code that will have to be ripped out later. I was looking for ways to remove dependencies for this patchset, not add them. > Also, tables are rebuilt when the firmware loads them, and handing in a > blob makes it harder to achieve this on-the-fly modification, compared > to callbacks. > > Paolo Well that's not true for smbios, is it?
On Mon, Apr 13, 2015 at 11:34:49AM -0500, Corey Minyard wrote: > On 04/13/2015 02:00 AM, Michael S. Tsirkin wrote: > > On Sun, Apr 12, 2015 at 08:26:46PM -0500, Corey Minyard wrote: > >> On 04/12/2015 11:05 AM, Michael S. Tsirkin wrote: > >>> On Tue, Apr 07, 2015 at 02:51:39PM -0500, minyard@acm.org wrote: > >>>> From: Corey Minyard <cminyard@mvista.com> > >>>> > >>>> There was no way to directly add a table entry to the SMBIOS table, > >>>> even though the BIOS supports this. So add a function to do this. > >>>> This is in preparation for the IPMI handler adding it's SMBIOS table > >>>> entry. > >>>> > >>>> Signed-off-by: Corey Minyard <cminyard@mvista.com> > >>> Do we have to use a callback for this? > >>> > >>> It seems that if smbios_table_entry_add just added the table > >>> itself to some array or a linked list, then devices could just call > >>> smbios_table_entry_add instead of registering a handler. > >>> > >>> And smbios_get_tables would scan that and get the tables. > >> Well, there are many ways you could handle this, and it doesn't seem > >> much different in the end to me. You either have a list of items to add > >> to the table or a list of callbacks to fill in the data. > >> > >> I made this the same as the ACPI code, which you have to have as a > >> callback if you are adding it to a common SSDT. > > Not really I think. > > The AML functions require that you have a tree to attach what you are > adding. If you did your own SSDT, you wouldn't need a callback. You > could add a binary blob that gets put into the SSDT, but I think that > would require adding some AML functions. Oh, you are using functions from aml-build.c. I forgot that they can only be used within ssdt ATM. Maybe that's worth fixing. I need to ponder this a bit.
On 04/14/2015 01:31 AM, Michael S. Tsirkin wrote: > On Mon, Apr 13, 2015 at 06:40:46PM +0200, Paolo Bonzini wrote: >> >> On 13/04/2015 18:34, Corey Minyard wrote: >>>>> I made this the same as the ACPI code, which you have to have as a >>>>> callback if you are adding it to a common SSDT. >>>> Not really I think. >>> The AML functions require that you have a tree to attach what you are >>> adding. If you did your own SSDT, you wouldn't need a callback. You >>> could add a binary blob that gets put into the SSDT, but I think that >>> would require adding some AML functions. >> I very much prefer the callback idea. Long term it could be used by >> more devices and possibly it could be turned into an AMLProvider QOM >> interface. Then the ACPI builder could iterate on all QOM devices and >> just ask which of them can provide some AML. > Yes, that would make sense. Devices which have a static > AML could provide a static AML property, with very little code, > those that have dynamic AML - dynamic AML property with more code. > > But I don't see callbacks as a step in that direction - > more like code that will have to be ripped out later. > > I was looking for ways to remove dependencies for this patchset, > not add them. I don't see a dependency difference between the two. Either way we are adding something to the ACPI code to store these things, and adding something to i386 (and arm64, when that happens) to get these entries from ACPI and add them to the firmware. >> Also, tables are rebuilt when the firmware loads them, and handing in a >> blob makes it harder to achieve this on-the-fly modification, compared >> to callbacks. >> >> Paolo > Well that's not true for smbios, is it? Both tables are dynamically built at runtime. The SMBIOS table is fixed size, but the contents will vary depending on what the user specifies about IPMI. -corey
On 14/04/2015 08:31, Michael S. Tsirkin wrote: > On Mon, Apr 13, 2015 at 06:40:46PM +0200, Paolo Bonzini wrote: >> >> >> On 13/04/2015 18:34, Corey Minyard wrote: >>>>> I made this the same as the ACPI code, which you have to have as a >>>>> callback if you are adding it to a common SSDT. >>>> >>>> Not really I think. >>> >>> The AML functions require that you have a tree to attach what you are >>> adding. If you did your own SSDT, you wouldn't need a callback. You >>> could add a binary blob that gets put into the SSDT, but I think that >>> would require adding some AML functions. >> >> I very much prefer the callback idea. Long term it could be used by >> more devices and possibly it could be turned into an AMLProvider QOM >> interface. Then the ACPI builder could iterate on all QOM devices and >> just ask which of them can provide some AML. > > Yes, that would make sense. Devices which have a static > AML could provide a static AML property, with very little code, > those that have dynamic AML - dynamic AML property with more code. Why complicate the ACPI hooks unnecessarily? The generic code is written to support dynamic AML, why should devices support both? This seems like premature optimization. > I was looking for ways to remove dependencies for this patchset, > not add them. I'm not sure which dependencies these are. >> Also, tables are rebuilt when the firmware loads them, and handing in a >> blob makes it harder to achieve this on-the-fly modification, compared >> to callbacks. > > Well that's not true for smbios, is it? No, it's not. For smbios, I agree that passing a static table to the API (i.e. having smbios_register_device_table instead of smbios_register_device_table_handler, and calling smbios_table_entry_add directly from smbios_get_tables) is also a valid choice. Paolo
diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c index 1341e02..fe15325 100644 --- a/hw/i386/smbios.c +++ b/hw/i386/smbios.c @@ -831,6 +831,25 @@ static void smbios_entry_point_setup(void) ep.structure_table_address = cpu_to_le32(0); } +struct smbios_device_handler { + void (*handle_device_table)(void *opaque); + void *opaque; + struct smbios_device_handler *prev; +}; +static struct smbios_device_handler *device_handlers; + +void smbios_register_device_table_handler(void (*handle_device_table) + (void *opaque), + void *opaque) +{ + struct smbios_device_handler *handler = g_malloc(sizeof(*handler)); + + handler->handle_device_table = handle_device_table; + handler->opaque = opaque; + handler->prev = device_handlers; + device_handlers = handler; +} + void smbios_get_tables(uint8_t **tables, size_t *tables_len, uint8_t **anchor, size_t *anchor_len) { @@ -875,6 +894,14 @@ void smbios_get_tables(uint8_t **tables, size_t *tables_len, } smbios_build_type_32_table(); + + while (device_handlers) { + struct smbios_device_handler *handler = device_handlers; + device_handlers = handler->prev; + handler->handle_device_table(handler->opaque); + g_free(handler); + } + smbios_build_type_127_table(); smbios_validate_table(); @@ -898,6 +925,71 @@ static void save_opt(const char **dest, QemuOpts *opts, const char *name) } } +int smbios_table_entry_add(void *data, int size, bool append_zeros) +{ + struct smbios_structure_header *header; + struct smbios_table *table; /* legacy mode only */ + + /* + * NOTE: standard double '\0' terminator expected, per smbios spec. + * (except in legacy mode, where the second '\0' is implicit and + * will be inserted by the BIOS). + */ + if (append_zeros) { + size += 2; + } + smbios_tables = g_realloc(smbios_tables, smbios_tables_len + size); + header = (struct smbios_structure_header *)(smbios_tables + + smbios_tables_len); + + memcpy(header, data, size); + + if (test_bit(header->type, have_fields_bitmap)) { + error_report("can't load type %d struct, fields already specified!", + header->type); + exit(1); + } + set_bit(header->type, have_binfile_bitmap); + + if (header->type == 4) { + smbios_type4_count++; + } + + smbios_tables_len += size; + if (size > smbios_table_max) { + smbios_table_max = size; + } + smbios_table_cnt++; + + /* add a copy of the newly loaded blob to legacy smbios_entries */ + /* NOTE: This code runs before smbios_set_defaults(), so we don't + * yet know which mode (legacy vs. aggregate-table) will be + * required. We therefore add the binary blob to both legacy + * (smbios_entries) and aggregate (smbios_tables) tables, and + * delete the one we don't need from smbios_set_defaults(), + * once we know which machine version has been requested. + */ + if (!smbios_entries) { + smbios_entries_len = sizeof(uint16_t); + smbios_entries = g_malloc0(smbios_entries_len); + } + if (append_zeros) { + size -= 1; /* The BIOS adds the second zero in legacy mode. */ + } + smbios_entries = g_realloc(smbios_entries, smbios_entries_len + + size + sizeof(*table)); + table = (struct smbios_table *)(smbios_entries + smbios_entries_len); + table->header.type = SMBIOS_TABLE_ENTRY; + table->header.length = cpu_to_le16(sizeof(*table) + size); + memcpy(table->data, header, size); + smbios_entries_len += sizeof(*table) + size; + (*(uint16_t *)smbios_entries) = + cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1); + /* end: add a copy of the newly loaded blob to legacy smbios_entries */ + + return 0; +} + void smbios_entry_add(QemuOpts *opts) { Error *local_err = NULL; @@ -907,9 +999,8 @@ void smbios_entry_add(QemuOpts *opts) val = qemu_opt_get(opts, "file"); if (val) { - struct smbios_structure_header *header; int size; - struct smbios_table *table; /* legacy mode only */ + uint8_t *data; qemu_opts_validate(opts, qemu_smbios_file_opts, &local_err); if (local_err) { @@ -923,60 +1014,14 @@ void smbios_entry_add(QemuOpts *opts) exit(1); } - /* - * NOTE: standard double '\0' terminator expected, per smbios spec. - * (except in legacy mode, where the second '\0' is implicit and - * will be inserted by the BIOS). - */ - smbios_tables = g_realloc(smbios_tables, smbios_tables_len + size); - header = (struct smbios_structure_header *)(smbios_tables + - smbios_tables_len); - - if (load_image(val, (uint8_t *)header) != size) { + data = g_malloc(size); + if (load_image(val, data) != size) { error_report("Failed to load SMBIOS file %s", val); exit(1); } - if (test_bit(header->type, have_fields_bitmap)) { - error_report("can't load type %d struct, fields already specified!", - header->type); - exit(1); - } - set_bit(header->type, have_binfile_bitmap); - - if (header->type == 4) { - smbios_type4_count++; - } - - smbios_tables_len += size; - if (size > smbios_table_max) { - smbios_table_max = size; - } - smbios_table_cnt++; - - /* add a copy of the newly loaded blob to legacy smbios_entries */ - /* NOTE: This code runs before smbios_set_defaults(), so we don't - * yet know which mode (legacy vs. aggregate-table) will be - * required. We therefore add the binary blob to both legacy - * (smbios_entries) and aggregate (smbios_tables) tables, and - * delete the one we don't need from smbios_set_defaults(), - * once we know which machine version has been requested. - */ - if (!smbios_entries) { - smbios_entries_len = sizeof(uint16_t); - smbios_entries = g_malloc0(smbios_entries_len); - } - smbios_entries = g_realloc(smbios_entries, smbios_entries_len + - size + sizeof(*table)); - table = (struct smbios_table *)(smbios_entries + smbios_entries_len); - table->header.type = SMBIOS_TABLE_ENTRY; - table->header.length = cpu_to_le16(sizeof(*table) + size); - memcpy(table->data, header, size); - smbios_entries_len += sizeof(*table) + size; - (*(uint16_t *)smbios_entries) = - cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1); - /* end: add a copy of the newly loaded blob to legacy smbios_entries */ - + smbios_table_entry_add(data, size, false); + g_free(data); return; } diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h index d2850be..656327d 100644 --- a/include/hw/i386/smbios.h +++ b/include/hw/i386/smbios.h @@ -27,6 +27,19 @@ void smbios_get_tables(uint8_t **tables, size_t *tables_len, uint8_t **anchor, size_t *anchor_len); /* + * Add an external entry to the SMBIOS table. Can only be called + * from a registered device table handler. + */ +int smbios_table_entry_add(void *data, int size, bool append_zeros); + +/* + * When constructing SMBIOS tables, call a function at the end of the + * add process to allow devices to add their own SMBIOS table entries. + */ +void smbios_register_device_table_handler(void (*handle_device_table) + (void *opaque), + void *opaque); +/* * SMBIOS spec defined tables */