| Message ID | 20140219204030.GA25087@ERROL.INI.CMU.EDU |
|---|---|
| State | New |
| Headers | show |
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
On Mi, 2014-02-19 at 15:40 -0500, 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... :( "dmidecode --type 2" ? > 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 ? smbios_maybe_add_str puts a string into the "transfer field to seabios" list, and seabios generates the table using it. When generating the complete table in qemu it indeed doesn't fulfill the purpose ... cheers, Gerd
On Thu, Feb 20, 2014 at 04:27:27PM +0100, Gerd Hoffmann wrote: > On Mi, 2014-02-19 at 15:40 -0500, Gabriel L. Somlo wrote: > > 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... :( > > "dmidecode --type 2" ? That doesn't work with --dump-bin, to get me something I could just load with "-smbios file=<foo>". On Wed, Feb 19, 2014 at 11:20:37PM +0100, Laszlo Ersek wrote: > 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:-) At this point, the question is "cut'n'paste from SeaBIOS" vs. rewrite the table generation routines from scratch in QEMU? I remember ACPI was mostly cut'n'pasted into QEMU, so would that be an OK starting point for SMBIOS as well ? I looked at licenses, and the SeaBIOS smbios.c file is gplv3, whereas the current smbios.c in QEMU is some combination of gplv2 and gplv2+. Any thoughts on whether that's a problem, legally speaking ? Thanks, --Gabriel
On Thu, Feb 20, 2014 at 10:38:04AM -0500, Gabriel L. Somlo wrote: > On Thu, Feb 20, 2014 at 04:27:27PM +0100, Gerd Hoffmann wrote: > > On Mi, 2014-02-19 at 15:40 -0500, Gabriel L. Somlo wrote: > > > 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... :( > > > > "dmidecode --type 2" ? > > That doesn't work with --dump-bin, to get me something I could just > load with "-smbios file=<foo>". > > On Wed, Feb 19, 2014 at 11:20:37PM +0100, Laszlo Ersek wrote: > > 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:-) > > At this point, the question is "cut'n'paste from SeaBIOS" vs. rewrite > the table generation routines from scratch in QEMU? I remember ACPI > was mostly cut'n'pasted into QEMU, so would that be an OK starting > point for SMBIOS as well ? I looked at licenses, and the SeaBIOS > smbios.c file is gplv3, whereas the current smbios.c in QEMU is some > combination of gplv2 and gplv2+. Any thoughts on whether that's a > problem, legally speaking ? s/gplv3/lgplv3/, but same question :) Thanks, --G
On Thu, Feb 20, 2014 at 10:42:02AM -0500, Gabriel L. Somlo wrote: > On Thu, Feb 20, 2014 at 10:38:04AM -0500, Gabriel L. Somlo wrote: > > On Thu, Feb 20, 2014 at 04:27:27PM +0100, Gerd Hoffmann wrote: > > > On Mi, 2014-02-19 at 15:40 -0500, Gabriel L. Somlo wrote: > > > > 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... :( > > > > > > "dmidecode --type 2" ? > > > > That doesn't work with --dump-bin, to get me something I could just > > load with "-smbios file=<foo>". But real men compile their binary SMBIOS tables by hand: echo -ne "\x02\x0f\x00\x02\x00\x00\x00\x00\x00\x01\x00\x00\x03\x0a\x00\x00\x00" > type2.bin This seems to work quite allright, at least for now :) --Gabriel
On 02/20/14 16:38, Gabriel L. Somlo wrote: > On Thu, Feb 20, 2014 at 04:27:27PM +0100, Gerd Hoffmann wrote: >> On Mi, 2014-02-19 at 15:40 -0500, Gabriel L. Somlo wrote: >>> 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... :( >> >> "dmidecode --type 2" ? > > That doesn't work with --dump-bin, to get me something I could just > load with "-smbios file=<foo>". > > On Wed, Feb 19, 2014 at 11:20:37PM +0100, Laszlo Ersek wrote: >> 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:-) > > At this point, the question is "cut'n'paste from SeaBIOS" vs. rewrite > the table generation routines from scratch in QEMU? I remember ACPI > was mostly cut'n'pasted into QEMU, so would that be an OK starting > point for SMBIOS as well ? I looked at licenses, and the SeaBIOS > smbios.c file is gplv3, whereas the current smbios.c in QEMU is some > combination of gplv2 and gplv2+. Any thoughts on whether that's a > problem, legally speaking ? I have no clue about the technical feasibility. You'll probably only know if you try it :) Regarding licensing, IIRC Michael ported the ACPI code by: - posting the qemu patch on both qemu-devel and the seabios list, - CC'd all people who ever modified the seabios files being ported (this is easy with "git log") - asked all CC'd people to respond with an Acked-by if they agreed to relicense their seabios contribs. Laszlo
diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h index 18fb970..3d2749b 100644 --- a/include/hw/i386/smbios.h +++ b/include/hw/i386/smbios.h @@ -60,6 +60,22 @@ struct smbios_type_1 { uint8_t family_str; } QEMU_PACKED; +/* SMBIOS type 2 - Base Board */ +struct smbios_type_2 { + struct smbios_structure_header header; + uint8_t manufacturer_str; + uint8_t product_str; + uint8_t version_str; + uint8_t serial_number_str; + uint8_t asset_tag_number_str; + uint8_t feature_flags; + uint8_t location_str; + uint16_t chassis_handle; + uint8_t board_type; + uint8_t contained_element_count; + // contained elements follow +} QEMU_PACKED; + /* SMBIOS type 3 - System Enclosure (v2.3) */ struct smbios_type_3 { struct smbios_structure_header header; @@ -111,7 +127,7 @@ struct smbios_type_16 { uint16_t memory_error_information_handle; uint16_t number_of_memory_devices; } QEMU_PACKED; -/* SMBIOS type 17 - Memory Device +/* SMBIOS type 17 - Memory Device (v2.3) * Associated with one type 19 */ struct smbios_type_17 { @@ -127,6 +143,12 @@ struct smbios_type_17 { uint8_t bank_locator_str; uint8_t memory_type; uint16_t type_detail; + // v2.3 fields: + uint16_t speed; + uint8_t manufacturer_str; + uint8_t serial_number_str; + uint8_t asset_tag_number_str; + uint8_t part_number_str; } QEMU_PACKED; /* SMBIOS type 19 - Memory Array Mapped Address */ diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c index e8f41ad..e875493 100644 --- a/hw/i386/smbios.c +++ b/hw/i386/smbios.c @@ -270,11 +270,41 @@ void smbios_set_type1_defaults(const char *manufacturer, } } +static void smbios_build_type_2_fields(void) +{ + uint8_t f_flags = 0x01, b_type = 0x0a; /* Motherboard / Hosting Board */ + uint8_t elem_cnt = 0; /* None */ + uint16_t chassis_hndl = 0x300; /* Type 3 System Enclosure */ + + smbios_maybe_add_str(2, offsetof(struct smbios_type_2, manufacturer_str), + "abcxyz"); + smbios_maybe_add_str(2, offsetof(struct smbios_type_2, product_str), + NULL); + smbios_maybe_add_str(2, offsetof(struct smbios_type_2, version_str), + NULL); + smbios_maybe_add_str(2, offsetof(struct smbios_type_2, serial_number_str), + NULL); + smbios_maybe_add_str(2, offsetof(struct smbios_type_2, + asset_tag_number_str), + NULL); + smbios_add_field(2, offsetof(struct smbios_type_2, feature_flags), + &f_flags, sizeof(f_flags)); + smbios_maybe_add_str(2, offsetof(struct smbios_type_2, location_str), + NULL); + smbios_add_field(2, offsetof(struct smbios_type_2, chassis_handle), + &chassis_hndl, sizeof(chassis_hndl)); + smbios_add_field(2, offsetof(struct smbios_type_2, board_type), + &b_type, sizeof(b_type)); + smbios_add_field(2, offsetof(struct smbios_type_2, contained_element_count), + &elem_cnt, sizeof(elem_cnt)); +} + uint8_t *smbios_get_table(size_t *length) { if (!smbios_immutable) { smbios_build_type_0_fields(); smbios_build_type_1_fields(); + smbios_build_type_2_fields(); smbios_validate_table(); smbios_immutable = true; }