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

[Corey Minyard <minyard@acm.org>]

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