Re: [Qemu-devel] [PATCH v2, Ping] SMBIOS: Upgrade Type17 to v2.3, add Type2

[Laszlo Ersek]

On 02/19/14 21:40, Gabriel L. Somlo wrote:
> On Wed, Feb 19, 2014 at 10:59:34AM +0100, Gerd Hoffmann wrote:
>>   Hi,
>>
>>> However, when I compare unmodified SMBIOS against what I get when
>>> supplying the patched binary table via command line, I get this:
>>
>> As Laszlo already sayed: one table per file.
> 
> So I gave up on that relatively quickly, as there's no easy and
> convenient way to "harvest" a binary of just one table type from
> a host that works the way I want it... :(
> 
>>>> If seabios finds a table provided by qemu it used it, otherwise it
>>>> (possibly) generates its own.  So we can smoothly switch over to qemu,
>>>> table-by-table.  You can have qemu provide type2+type17 tables, and
>>>> leave everything else as-is.  And when doing it in qemu it is easy to do
>>>> it for new machine types only.
>>>
>>> I could try to hack at the QEMU smbios source file to try to find
>>> where the problem lies (at least why handover to SeaBIOS doesn't work
>>> as expected), but I'm not sure providing command line flags for
>>> inputting each record type individually is a scalable way to move
>>> forward.
>>
>> Agree.  qemu should simply autogenerate the entries (where it can).
>> i.e. basically port seabios smbios_init_type_17 function to qemu, then
>> hook the result into the smbios_entries array.  The code to do that is
>> in smbios_entry_add().  You probably want to factor that out ino a small
>> helper function which is then called by both smbios_entry_add() and the
>> type17 init function.
> 
> So I tried the patch below (hardcoded Type 2 initialization). The
> guest (tried this on Fedora-20 live) still can't see a Type 2 entry.
> 
> Besides, I think smbios_maybe_add_str() looks wrong: It looks like
> it's trying to paste a string into a given type structure at the
> field offset, but I think the field should actually be an *index*
> into a concatenated set of zero-terminated strings tacked on to the
> end of the type struct (see load_str_field_* macros in
> seabios src/fw/smbios.c).
> 
> What am I missing ?

I'm responding halfway from memory. You won't like the answer.

Basically when you call smbios_maybe_add_str() and smbios_add_field(),
qemu only prepares a field-wise (SMBIOS_FIELD_ENTRY) patch, to be passed
down via fw_cfg. The offset of the affected field is *always* the
in-table (formatted-area) offset.

It is up to the boot firmware to process these field-wise patches
cleverly. Ie. the boot firmware must explicitly look for specific field
offsets, and *know* whether each offset identifies an in-table
(formatted-area) field -- in which case the patching is done in the
formatted area --, or the offset identifies a string index field -- in
which case the patching must happen in the unformatted area.

For example, compare the Type 0 fields (a) "bios version" and (b) "bios
major release". In qemu:

(a)
smbios_maybe_add_str(0,
                     offsetof(struct smbios_type_0, bios_version_str),
                     type0.version);
  smbios_add_field(type, offset, data, strlen(data) + 1)

vs.

(b)
  smbios_add_field(0,
                   offsetof(struct smbios_type_0,
                            system_bios_major_release),
                   &type0.major, 1);

The SMBIOS_FIELD_ENTRY records that these prepare look just the same,
even though (a) is in the unformatted area (the in-table part is just a
string index), while (b) is in the formatted area.

Now look at how smbios processes them. In smbios_init_type_0() (and all
other such functions), first all of the unformatted (string) fields are
retrieved with calls to load_str_field_with_default(), using explicit
field offsets.

load_str_field_with_default() will append the contents of the field at
the end of the unformatted area, while also setting up the in-table
string index to point to it. (See p->field = ++str_index.) So this
covers (a) bios_version_str.

After these have been processed, set_field_with_default() calls are
made, again with explicit offsets, however these overwrite in-table
(formatted area) fields with the fw_cfg field-wise patches. This covers
(b) system_bios_major_release.

Therefore, qemu doesn't care at all, but the firmware must know which
offset corresponds to a string index and which to an in-table
(formatted) field.

If you want to export new fields for an existing table, or else a
completely new table *but* field-wise, then you need to encode this kind
of "field info" in SeaBIOS. (IOW, add defaults for them, and implement
field-specific patching.)

If you want to export a new table whole-sale (SMBIOS_TABLE_ENTRY), then
you don't have to modify SeaBIOS. However, in qemu:
- you either need to pass single-table blobs on the qemu command line, or
- patch qemu so that it internally prepares the *complete* table for the
boot firmware (meaning you must encode knowledge about formatted vs.
unformatted fields in qemu -- you must set up the unformatted area and
the string indices yourself). (*)

You have to encode the formatted vs. unformatted knowledge *somewhere*.
You can push it around (qemu command line, qemu code, seabios code), but
you have to encode it explicitly somewhere.

(Writing the SMBIOS patches for OVMF (type0 and type1) took me three
days of *uninterrupted* misery ^W coding.)

Preferably, (*) should be implemented, because then SeaBIOS and OVMF can
both profit immediately. O:-)

Laszlo