Re: [Qemu-devel] [PATCH 10/15] smbios: Add a function to directly add an entry

[Michael S. Tsirkin]

On Tue, Apr 07, 2015 at 02:51:39PM -0500, address@hidden wrote:
> From: Corey Minyard <address@hidden>
> 
> 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 <address@hidden>

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
>