qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Corey Minyard
Subject: Re: [Qemu-devel] [PATCH 10/15] smbios: Add a function to directly add an entry
Date: Sun, 12 Apr 2015 20:26:46 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]