[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: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH 10/15] smbios: Add a function to directly add an entry |
Date: |
Mon, 13 Apr 2015 09:00:39 +0200 |
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
> >>
- [Qemu-devel] [PATCH 08/15] ipmi: Add documentation, (continued)
- [Qemu-devel] [PATCH 08/15] ipmi: Add documentation, minyard, 2015/04/07
- [Qemu-devel] [PATCH 03/15] ipmi: Add a KCS low-level interface, minyard, 2015/04/07
- [Qemu-devel] [PATCH 11/15] pc: Postpone SMBIOS table installation to post machine init, minyard, 2015/04/07
- [Qemu-devel] [PATCH 04/15] ipmi: Add a BT low-level interface, minyard, 2015/04/07
- [Qemu-devel] [PATCH 09/15] ipmi: Add migration capability to the IPMI device., minyard, 2015/04/07
- [Qemu-devel] [PATCH 13/15] configure: Copy some items from default configs to target configs, minyard, 2015/04/07
- [Qemu-devel] [PATCH 10/15] smbios: Add a function to directly add an entry, minyard, 2015/04/07
- Re: [Qemu-devel] [PATCH 10/15] smbios: Add a function to directly add an entry, Michael S. Tsirkin, 2015/04/12
- Re: [Qemu-devel] [PATCH 10/15] smbios: Add a function to directly add an entry, Corey Minyard, 2015/04/12
- Re: [Qemu-devel] [PATCH 10/15] smbios: Add a function to directly add an entry,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [PATCH 10/15] smbios: Add a function to directly add an entry, Corey Minyard, 2015/04/13
- Re: [Qemu-devel] [PATCH 10/15] smbios: Add a function to directly add an entry, Paolo Bonzini, 2015/04/13
- Re: [Qemu-devel] [PATCH 10/15] smbios: Add a function to directly add an entry, Michael S. Tsirkin, 2015/04/14
- Re: [Qemu-devel] [PATCH 10/15] smbios: Add a function to directly add an entry, Corey Minyard, 2015/04/14
- Re: [Qemu-devel] [PATCH 10/15] smbios: Add a function to directly add an entry, Paolo Bonzini, 2015/04/14
- Re: [Qemu-devel] [PATCH 10/15] smbios: Add a function to directly add an entry, Michael S. Tsirkin, 2015/04/14
[Qemu-devel] [PATCH 12/15] ipmi: Add SMBIOS table entry, minyard, 2015/04/07
[Qemu-devel] [PATCH 06/15] ipmi: Add an external connection simulation interface, minyard, 2015/04/07
[Qemu-devel] [PATCH 07/15] ipmi: Add tests, minyard, 2015/04/07
[Qemu-devel] [PATCH 14/15] acpi: Add hooks for adding things to the SSDT table, minyard, 2015/04/07