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

[Michael S. Tsirkin]

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, 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.
> 
> 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
> >>