[PATCH v2, Ping] SMBIOS: Upgrade Type17 to v2.3, add Type2
Add v2.3 fields to Type 17 (Memory Device) structure. Add Type 2 (Baseboard) structure. "About This Mac" on OS X guests will crash and restart the GUI if Type 17 structures are not compliant with at least v2.3 of the SMBIOS/DMI spec. OS X 10.7 and 10.8 will panic during boot if a Type 2 (Baseboard) structure is not present in the SMBIOS. Signed-off-by: Gabriel Somlo <somlo@cmu.edu> --- On Fri, Feb 07, 2014 at 04:37:58PM +0100, Paolo Bonzini wrote:
Il 06/02/2014 14:38, Gabriel L. Somlo ha scritto:
On Wed, Feb 05, 2014 at 08:02:25PM -0500, Kevin O'Connor wrote:
Thanks. In general, though, it is preferred to make smbios changes at the QEMU layer. Indeed, going forward, I'd like to see all the smbios stuff moved up to QEMU.
Is there something in these two patches that can't be done in QEMU?
But anyhow, right now QEMU seems to be making relatively minor tweaks to something that's firmly at home in SeaBIOS, which is why I sent my patches to the latter...
Yeah, that's correct. There's really no particular hook in QEMU to do this.
Would it be OK to apply this in SeaBIOS now, so that 1. it can be useful until whenever SMBIOS gets transferred/absorbed into QEMU, and 2. it won't fall through the cracks when that transition happens ? Thanks much, Gabriel src/fw/smbios.c | 42 ++++++++++++++++++++++++++++++++++++++++++ src/std/smbios.h | 22 ++++++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/src/fw/smbios.c b/src/fw/smbios.c index 55c662a..5b76468 100644 --- a/src/fw/smbios.c +++ b/src/fw/smbios.c @@ -251,6 +251,41 @@ smbios_init_type_1(void *start) return end; } +/* Type 2 -- Base Board */ +static void * +smbios_init_type_2(void *start) +{ + struct smbios_type_2 *p = (struct smbios_type_2 *)start; + char *end = (char *)start + sizeof(struct smbios_type_2); + size_t size; + int str_index = 0; + + p->header.type = 2; + p->header.length = sizeof(struct smbios_type_2); + p->header.handle = 0x200; + + load_str_field_with_default(2, manufacturer_str, BUILD_APPNAME); + load_str_field_or_skip(2, product_str); + load_str_field_or_skip(2, version_str); + load_str_field_or_skip(2, serial_number_str); + load_str_field_or_skip(2, asset_tag_number_str); + load_str_field_or_skip(2, location_str); + + set_field_with_default(2, feature_flags, 0x01); /* Motherboard */ + set_field_with_default(2, chassis_handle, 0x300); /* T3 System Enclosure */ + set_field_with_default(2, board_type, 0x0a); /* Motherboard */ + set_field_with_default(2, contained_element_count, 0); + + *end = 0; + end++; + if (!str_index) { + *end = 0; + end++; + } + + return end; +} + /* Type 3 -- System Enclosure */ static void * smbios_init_type_3(void *start) @@ -417,6 +452,12 @@ smbios_init_type_17(void *start, u32 size_mb, int instance) set_field_with_default(17, memory_type, 0x07); /* RAM */ set_field_with_default(17, type_detail, 0); + set_field_with_default(17, speed, 0); /* unknown */ + load_str_field_or_skip(17, manufacturer_str); + load_str_field_or_skip(17, serial_number_str); + load_str_field_or_skip(17, asset_tag_number_str); + load_str_field_or_skip(17, part_number_str); + *end = 0; end++; if (!str_index) { @@ -540,6 +581,7 @@ smbios_setup(void) add_struct(0, p); add_struct(1, p); + add_struct(2, p); add_struct(3, p); int cpu_num; diff --git a/src/std/smbios.h b/src/std/smbios.h index 0513716..86a4c57 100644 --- a/src/std/smbios.h +++ b/src/std/smbios.h @@ -59,6 +59,22 @@ struct smbios_type_1 { u8 family_str; } PACKED; +/* SMBIOS type 2 - Base Board */ +struct smbios_type_2 { + struct smbios_structure_header header; + u8 manufacturer_str; + u8 product_str; + u8 version_str; + u8 serial_number_str; + u8 asset_tag_number_str; + u8 feature_flags; + u8 location_str; + u16 chassis_handle; + u8 board_type; + u8 contained_element_count; + // contained elements follow +} PACKED; + /* SMBIOS type 3 - System Enclosure (v2.3) */ struct smbios_type_3 { struct smbios_structure_header header; @@ -127,6 +143,12 @@ struct smbios_type_17 { u8 bank_locator_str; u8 memory_type; u16 type_detail; + /* v2.3 fields: */ + u16 speed; + u8 manufacturer_str; + u8 serial_number_str; + u8 asset_tag_number_str; + u8 part_number_str; } PACKED; /* SMBIOS type 19 - Memory Array Mapped Address */ -- 1.8.1.4
On Mon, Feb 17, 2014 at 11:09:48AM -0500, Gabriel L. Somlo wrote:
On Fri, Feb 07, 2014 at 04:37:58PM +0100, Paolo Bonzini wrote:
Il 06/02/2014 14:38, Gabriel L. Somlo ha scritto:
On Wed, Feb 05, 2014 at 08:02:25PM -0500, Kevin O'Connor wrote:
Thanks. In general, though, it is preferred to make smbios changes at the QEMU layer. Indeed, going forward, I'd like to see all the smbios stuff moved up to QEMU.
Is there something in these two patches that can't be done in QEMU?
But anyhow, right now QEMU seems to be making relatively minor tweaks to something that's firmly at home in SeaBIOS, which is why I sent my patches to the latter...
Yeah, that's correct. There's really no particular hook in QEMU to do this.
Would it be OK to apply this in SeaBIOS now, so that 1. it can be useful until whenever SMBIOS gets transferred/absorbed into QEMU, and 2. it won't fall through the cracks when that transition happens ?
Unfortunately, if we change the smbios in SeaBIOS, it will show up on all machine types that use the new version of SeaBIOS. We've had issues with this type of change before as various OSes react differently to the change. Gerd, what's your take on this change? -Kevin
On Mo, 2014-02-17 at 15:33 -0500, Kevin O'Connor wrote:
On Mon, Feb 17, 2014 at 11:09:48AM -0500, Gabriel L. Somlo wrote:
On Fri, Feb 07, 2014 at 04:37:58PM +0100, Paolo Bonzini wrote:
Il 06/02/2014 14:38, Gabriel L. Somlo ha scritto:
On Wed, Feb 05, 2014 at 08:02:25PM -0500, Kevin O'Connor wrote:
Thanks. In general, though, it is preferred to make smbios changes at the QEMU layer. Indeed, going forward, I'd like to see all the smbios stuff moved up to QEMU.
Is there something in these two patches that can't be done in QEMU?
But anyhow, right now QEMU seems to be making relatively minor tweaks to something that's firmly at home in SeaBIOS, which is why I sent my patches to the latter...
Yeah, that's correct. There's really no particular hook in QEMU to do this.
Would it be OK to apply this in SeaBIOS now, so that 1. it can be useful until whenever SMBIOS gets transferred/absorbed into QEMU, and 2. it won't fall through the cracks when that transition happens ?
Unfortunately, if we change the smbios in SeaBIOS, it will show up on all machine types that use the new version of SeaBIOS. We've had issues with this type of change before as various OSes react differently to the change.
Gerd, what's your take on this change?
I think we can do this in qemu, without touching seabios at all. The currently used fw_cfg interface allows to pass both individual fields and complete tables. In practice the individual fields interface is used for type0 and type1 table fields. I think the only way to pass complete tables is 'qemu -smbios file=<pregenerated-blob-here>'. 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. cheers, Gerd
On Tue, Feb 18, 2014 at 11:21:33AM +0100, Gerd Hoffmann wrote:
Unfortunately, if we change the smbios in SeaBIOS, it will show up on all machine types that use the new version of SeaBIOS. We've had issues with this type of change before as various OSes react differently to the change.
Gerd, what's your take on this change?
I think we can do this in qemu, without touching seabios at all. The currently used fw_cfg interface allows to pass both individual fields and complete tables. In practice the individual fields interface is used for type0 and type1 table fields. I think the only way to pass complete tables is 'qemu -smbios file=<pregenerated-blob-here>'.
Using Fedora 20 live, I collected the SMBIOS table from the guest using "dmidecode --dump-bin", with the unpatched SeaBIOS (dmidecode_pc.bin), SeaBIOS with my patch applied (dmidecode_mac.bin), and again with unpatched SeaBIOS but with "-smbios file=dmidecode_mac.bin" on the QEMU command line (dmidecode_cmdline.bin). I then ran "dmidecode --from-dump" on the three binaries I collected, and here's what I got: $ diff dmi_pc.txt dmi_mac.txt 2c2 < Reading SMBIOS/DMI data from file dmidecode_pc.bin. ---
Reading SMBIOS/DMI data from file dmidecode_mac.bin. 4c4 < 10 structures occupying 298 bytes.
11 structures occupying 326 bytes. 29a30,43 Handle 0x0200, DMI type 2, 15 bytes Base Board Information Manufacturer: Bochs Product Name: Not Specified Version: Not Specified Serial Number: Not Specified Asset Tag: Not Specified Features: Board is a hosting board Location In Chassis: Not Specified Chassis Handle: 0x0300 Type: Motherboard Contained Object Handles: 0
73c87 < Handle 0x1100, DMI type 17, 21 bytes ---
Handle 0x1100, DMI type 17, 27 bytes 85a100,104 Speed: Unknown Manufacturer: Not Specified Serial Number: Not Specified Asset Tag: Not Specified Part Number: Not Specified
This looks as expected (we have a new Type 2 entry, and the Type 17 entry now has version 2.3 fields where it didn't use to before). However, when I compare unmodified SMBIOS against what I get when supplying the patched binary table via command line, I get this: $ diff dmi_pc.txt dmi_cmdline.txt 2c2 < Reading SMBIOS/DMI data from file dmidecode_pc.bin. ---
Reading SMBIOS/DMI data from file dmidecode_cmdline.bin. 4c4 < 10 structures occupying 298 bytes.
11 structures occupying 657 bytes. 108,109c108,120 < Handle 0x7F00, DMI type 127, 4 bytes < End Of Table
Handle 0x5F4D, DMI type 95, 83 bytes Unknown Type Header and Data: 5F 53 4D 5F 32 1F 02 04 4B 00 00 00 00 00 00 00 5F 44 4D 49 5F D2 46 01 20 00 00 00 0B 00 24 00 00 18 00 00 01 02 00 E8 03 00 08 00 00 00 00 00 00 00 00 04 01 00 FF FF 42 6F 63 68 73 00 42 6F 63 68 73 00 30 31 2F 30 31 2F 32 30 31 31 00 00 01 1B 00 Strings: ....
Invalid entry length (0). DMI table is broken! Stop.
No Type 2, no extra fields for Type 17, and a corrupt table to boot.
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.
Sending a patch against QEMU would have definitely been my first choice, by a wide margin :) But after studying the hw/i386/smbios.c source file in QEMU for a while, I walked away with the impression that all it really tries to do is edit a few of the Type 0 and Type 1 fields, and that the hand-over logic between QEMU and SeaBIOS is not ready for prime time yet. So I sent the patch to SeaBIOS, where it seems to do what I want it to :) 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. Perhaps if there were a DMI/SMBIOS compiler (the reverse action of "dmidecode --from-dump", something that would take a text "source" table and generate a .bin from it), we could focus on getting the "-smbios file=<foo>" bit working correctly, and we could provide instructions in the docs on how users can build their own smbios tables. But I couldn't find anything out there that would "compile" a smbios table from some type of human-readable (ascii) form... Any thoughts ? Thanks, --Gabriel PS. I tried the patched SeaBIOS (with v2.3 type17 and added type2) on XP, Windows7, and Linux, and all of them seemed happy and none of them seemed to mind... Just sayin' ;)
On Tue, Feb 18, 2014 at 02:17:29PM -0500, Gabriel L. Somlo wrote:
Sending a patch against QEMU would have definitely been my first choice, by a wide margin :) But after studying the hw/i386/smbios.c source file in QEMU for a while, I walked away with the impression that all it really tries to do is edit a few of the Type 0 and Type 1 fields, and that the hand-over logic between QEMU and SeaBIOS is not ready for prime time yet.
The current mechanism for passing smbios from QEMU to SeaBIOS is horrible.
So I sent the patch to SeaBIOS, where it seems to do what I want it to :)
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.
In my opinion, generating the entire smbios table in QEMU and using the "romfile_loader" mechanism (see seabios src/fw/romfile_loader.c) would be the preferred solution. I understand that this is more than you signed up for.
Perhaps if there were a DMI/SMBIOS compiler (the reverse action of "dmidecode --from-dump", something that would take a text "source" table and generate a .bin from it), we could focus on getting the "-smbios file=<foo>" bit working correctly, and we could provide instructions in the docs on how users can build their own smbios tables.
But I couldn't find anything out there that would "compile" a smbios table from some type of human-readable (ascii) form...
Any thoughts ?
Thanks, --Gabriel
PS. I tried the patched SeaBIOS (with v2.3 type17 and added type2) on XP, Windows7, and Linux, and all of them seemed happy and none of them seemed to mind... Just sayin' ;)
Thanks for running tests. One thing that has bitten us in the past is OSes re-running license checks and/or popping up "new hardware" notifications on bios table changes. -Kevin
Hi,
In my opinion, generating the entire smbios table in QEMU and using the "romfile_loader" mechanism (see seabios src/fw/romfile_loader.c) would be the preferred solution. I understand that this is more than you signed up for.
Yes, this is where we should end up long-term. For the time being I think it is fine to use the existing mechanism to switch over table by table, and once we have the code to generate all tables in qemu we can switch over to an interface where we simply pass all tables as single blob. I think we don't even need the romfile_loader, or do have smbios tables pointers which need to be fixed up? cheers, Gerd
Add v2.3 fields to Type 4 and 17 structures, to comply with the major/minor smbios spec version (2.4) advertised in the smbios entry point structure. Signed-off-by: Gabriel Somlo <somlo@cmu.edu> --- On Tue, Feb 18, 2014 at 03:02:16PM -0500, Kevin O'Connor wrote:
Thanks for running tests. One thing that has bitten us in the past is OSes re-running license checks and/or popping up "new hardware" notifications on bios table changes.
Completely orthogonal to Type 2 (Base Board) and to the QEMU/smbios conversation: Since SeaBIOS sets up the SMBIOS entry point structure like this: ep->smbios_major_version = 2; ep->smbios_minor_version = 4; shouldn't all type structures contain all fields required by v2.4 of the spec, which means the type 17 struct missing v2.3 fields is actually a bug ? I had a feeling I should have kept the patches separate :) Along these lines, we're also missing the following fields for Type 4: serial_number_str, asset_tag_number_str, part_number_str I get that a new Type 2 suddenly showing up *might* confuse some guest OSs, but what do you think about the type 4 and type 17 v2.3 fields ? Thanks, Gabriel src/fw/smbios.c | 10 ++++++++++ src/std/smbios.h | 10 ++++++++++ 2 files changed, 20 insertions(+) diff --git a/src/fw/smbios.c b/src/fw/smbios.c index 55c662a..f0cdd5c 100644 --- a/src/fw/smbios.c +++ b/src/fw/smbios.c @@ -344,6 +344,10 @@ smbios_init_type_4(void *start, unsigned int cpu_number) p->l2_cache_handle = 0xffff; p->l3_cache_handle = 0xffff; + load_str_field_or_skip(4, serial_number_str); + load_str_field_or_skip(4, asset_tag_number_str); + load_str_field_or_skip(4, part_number_str); + *end = 0; end++; if (!str_index) { @@ -417,6 +421,12 @@ smbios_init_type_17(void *start, u32 size_mb, int instance) set_field_with_default(17, memory_type, 0x07); /* RAM */ set_field_with_default(17, type_detail, 0); + set_field_with_default(17, speed, 0); /* unknown */ + load_str_field_or_skip(17, manufacturer_str); + load_str_field_or_skip(17, serial_number_str); + load_str_field_or_skip(17, asset_tag_number_str); + load_str_field_or_skip(17, part_number_str); + *end = 0; end++; if (!str_index) { diff --git a/src/std/smbios.h b/src/std/smbios.h index 0513716..7ae3968 100644 --- a/src/std/smbios.h +++ b/src/std/smbios.h @@ -96,6 +96,10 @@ struct smbios_type_4 { u16 l1_cache_handle; u16 l2_cache_handle; u16 l3_cache_handle; + /* v2.3 fields: */ + u8 serial_number_str; + u8 asset_tag_number_str; + u8 part_number_str; } PACKED; /* SMBIOS type 16 - Physical Memory Array @@ -127,6 +131,12 @@ struct smbios_type_17 { u8 bank_locator_str; u8 memory_type; u16 type_detail; + /* v2.3 fields: */ + u16 speed; + u8 manufacturer_str; + u8 serial_number_str; + u8 asset_tag_number_str; + u8 part_number_str; } PACKED; /* SMBIOS type 19 - Memory Array Mapped Address */ -- 1.8.1.4
On 02/18/14 20:17, Gabriel L. Somlo wrote:
On Tue, Feb 18, 2014 at 11:21:33AM +0100, Gerd Hoffmann wrote:
Using Fedora 20 live, I collected the SMBIOS table from the guest using "dmidecode --dump-bin", with the unpatched SeaBIOS (dmidecode_pc.bin), SeaBIOS with my patch applied (dmidecode_mac.bin), and again with unpatched SeaBIOS but with "-smbios file=dmidecode_mac.bin" on the QEMU command line (dmidecode_cmdline.bin).
[...]
However, when I compare unmodified SMBIOS against what I get when supplying the patched binary table via command line, I get this:
$ diff dmi_pc.txt dmi_cmdline.txt 2c2 < Reading SMBIOS/DMI data from file dmidecode_pc.bin. ---
Reading SMBIOS/DMI data from file dmidecode_cmdline.bin. 4c4 < 10 structures occupying 298 bytes.
11 structures occupying 657 bytes. 108,109c108,120 < Handle 0x7F00, DMI type 127, 4 bytes < End Of Table
Handle 0x5F4D, DMI type 95, 83 bytes Unknown Type Header and Data: 5F 53 4D 5F 32 1F 02 04 4B 00 00 00 00 00 00 00 5F 44 4D 49 5F D2 46 01 20 00 00 00 0B 00 24 00 00 18 00 00 01 02 00 E8 03 00 08 00 00 00 00 00 00 00 00 04 01 00 FF FF 42 6F 63 68 73 00 42 6F 63 68 73 00 30 31 2F 30 31 2F 32 30 31 31 00 00 01 1B 00 Strings: ....
Invalid entry length (0). DMI table is broken! Stop.
No Type 2, no extra fields for Type 17, and a corrupt table to boot.
I had tested this qemu interface with my OVMF SMBIOS patches. It works. (I used a Type 3 table.) The problem in this case is that you can't just pass in a raw dump from dmidecode. You need to prefix it with "smbios_header": struct smbios_table { struct smbios_header header; uint8_t data[]; } QEMU_PACKED; struct smbios_header { uint16_t length; uint8_t type; } QEMU_PACKED; You need to set "type" to 1 (SMBIOS_TABLE_ENTRY), and set "length" so that it covers the entire "smbios_table" struct (ie. both header and payload, where payload is your SMBIOS table). "length" is little endian. Laszlo
On 02/18/14 22:08, Laszlo Ersek wrote:
On 02/18/14 20:17, Gabriel L. Somlo wrote:
On Tue, Feb 18, 2014 at 11:21:33AM +0100, Gerd Hoffmann wrote:
Using Fedora 20 live, I collected the SMBIOS table from the guest using "dmidecode --dump-bin", with the unpatched SeaBIOS (dmidecode_pc.bin), SeaBIOS with my patch applied (dmidecode_mac.bin), and again with unpatched SeaBIOS but with "-smbios file=dmidecode_mac.bin" on the QEMU command line (dmidecode_cmdline.bin).
[...]
However, when I compare unmodified SMBIOS against what I get when supplying the patched binary table via command line, I get this:
$ diff dmi_pc.txt dmi_cmdline.txt 2c2 < Reading SMBIOS/DMI data from file dmidecode_pc.bin. ---
Reading SMBIOS/DMI data from file dmidecode_cmdline.bin. 4c4 < 10 structures occupying 298 bytes.
11 structures occupying 657 bytes. 108,109c108,120 < Handle 0x7F00, DMI type 127, 4 bytes < End Of Table
Handle 0x5F4D, DMI type 95, 83 bytes Unknown Type Header and Data: 5F 53 4D 5F 32 1F 02 04 4B 00 00 00 00 00 00 00 5F 44 4D 49 5F D2 46 01 20 00 00 00 0B 00 24 00 00 18 00 00 01 02 00 E8 03 00 08 00 00 00 00 00 00 00 00 04 01 00 FF FF 42 6F 63 68 73 00 42 6F 63 68 73 00 30 31 2F 30 31 2F 32 30 31 31 00 00 01 1B 00 Strings: ....
Invalid entry length (0). DMI table is broken! Stop.
No Type 2, no extra fields for Type 17, and a corrupt table to boot.
I had tested this qemu interface with my OVMF SMBIOS patches. It works. (I used a Type 3 table.)
The problem in this case is that you can't just pass in a raw dump from dmidecode. You need to prefix it with "smbios_header":
struct smbios_table { struct smbios_header header; uint8_t data[]; } QEMU_PACKED;
struct smbios_header { uint16_t length; uint8_t type; } QEMU_PACKED;
You need to set "type" to 1 (SMBIOS_TABLE_ENTRY), and set "length" so that it covers the entire "smbios_table" struct (ie. both header and payload, where payload is your SMBIOS table). "length" is little endian.
Oh wait, scratch that, qemu should do this for you. But, I retested the thing (with my original Type 3 file), and it still seems to work. -smbios file=/home/virt-images/smbios/type3 000000 03 14 00 03 01 01 00 00 00 03 03 03 02 00 00 00 000010 00 00 00 00 52 65 64 20 48 61 74 00 00 Dumped in the OVMF guest: Handle 0x0000, DMI type 3, 20 bytes Chassis Information Manufacturer: Red Hat Type: Other Lock: Not Present Version: Not Specified Serial Number: Not Specified Asset Tag: Not Specified Boot-up State: Safe Power Supply State: Safe Thermal State: Safe Security Status: Unknown OEM Information: 0x00000000 Height: Unspecified Number Of Power Cords: Unspecified What was the exact command line you used? ... Ah, I think I understand. When you issued dmidecode --dump-bin, that dumped *all* of the tables. (--dump-bin is mutually exclusive with --type.) You can't pass multiple tables with one '-smbios file=XXX qemu option. You need separate binary dumps (one per table), and should pass them with one -smbios file=XXX option each. Thanks Laszlo
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.
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. cheers, Gerd
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 ? Thanks, --Gabriel 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; }
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 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
- update type 4 and 17 structures to v2.3 of the smbios spec - add type 2 structure definition Signed-off-by: Gabriel Somlo <somlo@cmu.edu> --- This patch changes none of the current functionality. It simply documents a few missing fields and adds the definition for an optional type, so I'm hoping it will not be too controversial :) Thanks, Gabriel include/hw/i386/smbios.h | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h index 18fb970..4e0d9c9 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; @@ -79,7 +95,7 @@ struct smbios_type_3 { // contained elements follow } QEMU_PACKED; -/* SMBIOS type 4 - Processor Information (v2.0) */ +/* SMBIOS type 4 - Processor Information (v2.3) */ struct smbios_type_4 { struct smbios_structure_header header; uint8_t socket_designation_str; @@ -97,6 +113,10 @@ struct smbios_type_4 { uint16_t l1_cache_handle; uint16_t l2_cache_handle; uint16_t l3_cache_handle; + uint8_t serial_number_str; + uint8_t asset_tag_number_str; + uint8_t part_number_str; + } QEMU_PACKED; /* SMBIOS type 16 - Physical Memory Array @@ -111,7 +131,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 +147,11 @@ struct smbios_type_17 { uint8_t bank_locator_str; uint8_t memory_type; uint16_t type_detail; + 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 */ -- 1.8.1.4
- deprecate passing individual smbios fields (SMBIOS_FIELD_ENTRY) - added functionality to build full tables instead, and pass them to the bios via fw_cfg as SMBIOS_TABLE_ENTRY blobs - added code to optionally build type 2 smbios table expected by OS X Signed-off-by: Gabriel Somlo <somlo@cmu.edu> --- First off, Kevin, Gerd, Laszlo: thanks much for helping me understand the current code. With your explanations, and after staring at it patiently for a few hours, it started coming into focus :) So, currently, QEMU sends tables acquired from binary files via the command line as SMBIOS_TABLE_ENTRY fw_cfg blobs, and individual fields via smaller SMBIOS_FIELD_ENTRY blobs. SeaBIOS looks for the whole-table blobs first, and if none are found, attempts to build the table using field blobs as default values if available. With this patch, I'm changing QEMU to only ever send whole-table blobs (SMBIOS_TABLE_ENTRY) to SeaBIOS. This applies currently to Type 0 and Type 1 tables, and from what I could test, there's no user-visible change in functionality. I've also added code to (optionally) generate a Type 2 table. Tables are generated only if full binary blobs were NOT given via the command line. Optional tables are only generated if fields for them are mentioned on the command line. Kevin and Gerd: I'd like to get your acked-by for this patch (or any future revision): you're the two people who've been editing src/fw/smbios.c in SeaBIOS, and while I didn't directly cut'n'paste anything from there, I did in fact stare at it very intently, and I'd like to get your OK with the lgplv3 -> gplv2+ difference in licensing between SeaBIOS and, respectively, QEMU. We're still passing individual structure types one per blob, with SeaBIOS doing the final assembly. In the future we might consider putting it all together in QEMU, and submitting it as a third blob type, but that would require coordinating with SeaBIOS, and I'd like to at least figure out how to build all required table types first. But even before that, I'd like to get feedback as early as possible, before I get too attached to any code changes I'm making :) Thanks much, Gabriel hw/i386/pc_piix.c | 12 +- hw/i386/pc_q35.c | 8 +- hw/i386/smbios.c | 312 +++++++++++++++++++++++++++++++---------------- include/hw/i386/smbios.h | 4 +- 4 files changed, 220 insertions(+), 116 deletions(-) diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index d5dc1ef..36b1b9c 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -60,7 +60,7 @@ static const int ide_irq[MAX_IDE_BUS] = { 14, 15 }; static bool has_pci_info; static bool has_acpi_build = true; -static bool smbios_type1_defaults = true; +static bool smbios_defaults = true; /* Make sure that guest addresses aligned at 1Gbyte boundaries get mapped to * host addresses aligned at 1Gbyte boundaries. This way we can use 1GByte * pages in the host. @@ -143,9 +143,9 @@ static void pc_init1(QEMUMachineInitArgs *args, guest_info->has_pci_info = has_pci_info; guest_info->isapc_ram_fw = !pci_enabled; - if (smbios_type1_defaults) { + if (smbios_defaults) { /* These values are guest ABI, do not change */ - smbios_set_type1_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)", + smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)", args->machine->name); } @@ -258,7 +258,7 @@ static void pc_init_pci(QEMUMachineInitArgs *args) static void pc_compat_1_7(QEMUMachineInitArgs *args) { - smbios_type1_defaults = false; + smbios_defaults = false; gigabyte_align = false; } @@ -337,7 +337,7 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args) { has_pci_info = false; has_acpi_build = false; - smbios_type1_defaults = false; + smbios_defaults = false; disable_kvm_pv_eoi(); enable_compat_apic_id_mode(); pc_init1(args, 1, 0); @@ -347,7 +347,7 @@ static void pc_init_isa(QEMUMachineInitArgs *args) { has_pci_info = false; has_acpi_build = false; - smbios_type1_defaults = false; + smbios_defaults = false; if (!args->cpu_model) { args->cpu_model = "486"; } diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index a7f6260..dfcc252 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -50,7 +50,7 @@ static bool has_pci_info; static bool has_acpi_build = true; -static bool smbios_type1_defaults = true; +static bool smbios_defaults = true; /* Make sure that guest addresses aligned at 1Gbyte boundaries get mapped to * host addresses aligned at 1Gbyte boundaries. This way we can use 1GByte * pages in the host. @@ -130,9 +130,9 @@ static void pc_q35_init(QEMUMachineInitArgs *args) guest_info->isapc_ram_fw = false; guest_info->has_acpi_build = has_acpi_build; - if (smbios_type1_defaults) { + if (smbios_defaults) { /* These values are guest ABI, do not change */ - smbios_set_type1_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)", + smbios_set_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)", args->machine->name); } @@ -242,7 +242,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args) static void pc_compat_1_7(QEMUMachineInitArgs *args) { - smbios_type1_defaults = false; + smbios_defaults = false; gigabyte_align = false; } diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c index e8f41ad..39051b1 100644 --- a/hw/i386/smbios.c +++ b/hw/i386/smbios.c @@ -29,13 +29,6 @@ struct smbios_header { uint8_t type; } QEMU_PACKED; -struct smbios_field { - struct smbios_header header; - uint8_t type; - uint16_t offset; - uint8_t data[]; -} QEMU_PACKED; - struct smbios_table { struct smbios_header header; uint8_t data[]; @@ -49,11 +42,8 @@ static size_t smbios_entries_len; static int smbios_type4_count = 0; static bool smbios_immutable; -static struct { - bool seen; - int headertype; - Location loc; -} first_opt[2]; +static DECLARE_BITMAP(have_binfile_bitmap, 128); +static DECLARE_BITMAP(have_fields_bitmap, 128); static struct { const char *vendor, *version, *date; @@ -66,6 +56,10 @@ static struct { /* uuid is in qemu_uuid[] */ } type1; +static struct { + const char *manufacturer, *product, *version, *serial, *asset, *location; +} type2; + static QemuOptsList qemu_smbios_opts = { .name = "smbios", .head = QTAILQ_HEAD_INITIALIZER(qemu_smbios_opts.head), @@ -149,6 +143,39 @@ static const QemuOptDesc qemu_smbios_type1_opts[] = { { /* end of list */ } }; +static const QemuOptDesc qemu_smbios_type2_opts[] = { + { + .name = "type", + .type = QEMU_OPT_NUMBER, + .help = "SMBIOS element type", + },{ + .name = "manufacturer", + .type = QEMU_OPT_STRING, + .help = "manufacturer name", + },{ + .name = "product", + .type = QEMU_OPT_STRING, + .help = "product name", + },{ + .name = "version", + .type = QEMU_OPT_STRING, + .help = "version number", + },{ + .name = "serial", + .type = QEMU_OPT_STRING, + .help = "serial number", + },{ + .name = "asset", + .type = QEMU_OPT_STRING, + .help = "asset tag number", + },{ + .name = "location", + .type = QEMU_OPT_STRING, + .help = "location in chassis", + }, + { /* end of list */ } +}; + static void smbios_register_config(void) { qemu_add_opts(&qemu_smbios_opts); @@ -164,117 +191,169 @@ static void smbios_validate_table(void) } } -/* - * To avoid unresolvable overlaps in data, don't allow both - * tables and fields for the same smbios type. - */ -static void smbios_check_collision(int type, int entry) +#define SMBIOS_BUILD_TABLE_PRE(tbl_type, tbl_handle, tbl_required) \ + struct smbios_table *t; \ + struct smbios_type_##tbl_type *p; \ + int str_index = 0; \ + do { \ + /* skip if we got (a) binary file(s) for this type, or */ \ + /* if this type is optional and wasn't requested explicitly */ \ + if (test_bit(tbl_type, have_binfile_bitmap) || \ + !(tbl_required || test_bit(tbl_type, have_fields_bitmap))) { \ + return; \ + } \ + \ + /* initialize fw_cfg smbios element count */ \ + 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 + \ + sizeof(*t) + sizeof(*p)); \ + \ + t = (struct smbios_table *)(smbios_entries + smbios_entries_len); \ + p = (struct smbios_type_##tbl_type *)(t + 1); \ + \ + smbios_entries_len += sizeof(*t) + sizeof(*p); \ + \ + p->header.type = tbl_type; \ + p->header.length = sizeof(*p); \ + p->header.handle = tbl_handle; \ + } while (0) + +#define SMBIOS_TABLE_SET_STR(field, value) \ + do { \ + int len; \ + if (value != NULL && (len = strlen(value) + 1) > 1) { \ + smbios_entries = g_realloc(smbios_entries, \ + smbios_entries_len + len); \ + memcpy(smbios_entries + smbios_entries_len, value, len); \ + smbios_entries_len += len; \ + p->field = ++str_index; \ + } else { \ + p->field = 0; \ + } \ + } while (0) + +#define SMBIOS_BUILD_TABLE_POST \ + do { \ + /* add empty string if no strings defined in table */ \ + /* (NOTE: terminating \0 currently handled by fw_cfg/seabios) */ \ + if (str_index == 0) { \ + smbios_entries = g_realloc(smbios_entries, \ + smbios_entries_len + 1); \ + *(smbios_entries + smbios_entries_len) = 0; \ + smbios_entries_len += 1; \ + } \ + \ + t->header.type = SMBIOS_TABLE_ENTRY; \ + t->header.length = smbios_entries_len - \ + ((uint8_t *)t - smbios_entries); \ + \ + /* update fw_cfg smbios element count */ \ + *(uint16_t *)smbios_entries += 1; \ + } while (0) + +static void smbios_build_type_0_table(void) { - if (type < ARRAY_SIZE(first_opt)) { - if (first_opt[type].seen) { - if (first_opt[type].headertype != entry) { - error_report("Can't mix file= and type= for same type"); - loc_push_restore(&first_opt[type].loc); - error_report("This is the conflicting setting"); - loc_pop(&first_opt[type].loc); - exit(1); - } - } else { - first_opt[type].seen = true; - first_opt[type].headertype = entry; - loc_save(&first_opt[type].loc); - } - } -} + SMBIOS_BUILD_TABLE_PRE(0, 0x000, 1); /* required */ -static void smbios_add_field(int type, int offset, const void *data, size_t len) -{ - struct smbios_field *field; + SMBIOS_TABLE_SET_STR(vendor_str, type0.vendor); + SMBIOS_TABLE_SET_STR(bios_version_str, type0.version); - 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 + - sizeof(*field) + len); - field = (struct smbios_field *)(smbios_entries + smbios_entries_len); - field->header.type = SMBIOS_FIELD_ENTRY; - field->header.length = cpu_to_le16(sizeof(*field) + len); - - field->type = type; - field->offset = cpu_to_le16(offset); - memcpy(field->data, data, len); - - smbios_entries_len += sizeof(*field) + len; - (*(uint16_t *)smbios_entries) = - cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1); -} + p->bios_starting_address_segment = 0xE800; /* hardcoded in SeaBIOS */ -static void smbios_maybe_add_str(int type, int offset, const char *data) -{ - if (data) { - smbios_add_field(type, offset, data, strlen(data) + 1); - } -} + SMBIOS_TABLE_SET_STR(bios_release_date_str, type0.date); + + p->bios_rom_size = 0; /* hardcoded in SeaBIOS with FIXME comment */ + + /* BIOS characteristics not supported */ + memset(p->bios_characteristics, 0, 8); + p->bios_characteristics[0] = 0x08; + + /* Enable targeted content distribution (needed for SVVP, per SeaBIOS) */ + p->bios_characteristics_extension_bytes[0] = 0; + p->bios_characteristics_extension_bytes[1] = 4; -static void smbios_build_type_0_fields(void) -{ - smbios_maybe_add_str(0, offsetof(struct smbios_type_0, vendor_str), - type0.vendor); - smbios_maybe_add_str(0, offsetof(struct smbios_type_0, bios_version_str), - type0.version); - smbios_maybe_add_str(0, offsetof(struct smbios_type_0, - bios_release_date_str), - type0.date); if (type0.have_major_minor) { - smbios_add_field(0, offsetof(struct smbios_type_0, - system_bios_major_release), - &type0.major, 1); - smbios_add_field(0, offsetof(struct smbios_type_0, - system_bios_minor_release), - &type0.minor, 1); + p->system_bios_major_release = type0.major; + p->system_bios_minor_release = type0.minor; + } else { + p->system_bios_major_release = 0; + p->system_bios_minor_release = 0; } + + /* hardcoded in SeaBIOS */ + p->embedded_controller_major_release = 0xFF; + p->embedded_controller_minor_release = 0xFF; + + SMBIOS_BUILD_TABLE_POST; } -static void smbios_build_type_1_fields(void) +static void smbios_build_type_1_table(void) { - smbios_maybe_add_str(1, offsetof(struct smbios_type_1, manufacturer_str), - type1.manufacturer); - smbios_maybe_add_str(1, offsetof(struct smbios_type_1, product_name_str), - type1.product); - smbios_maybe_add_str(1, offsetof(struct smbios_type_1, version_str), - type1.version); - smbios_maybe_add_str(1, offsetof(struct smbios_type_1, serial_number_str), - type1.serial); - smbios_maybe_add_str(1, offsetof(struct smbios_type_1, sku_number_str), - type1.sku); - smbios_maybe_add_str(1, offsetof(struct smbios_type_1, family_str), - type1.family); + SMBIOS_BUILD_TABLE_PRE(1, 0x100, 1); /* required */ + + SMBIOS_TABLE_SET_STR(manufacturer_str, type1.manufacturer); + SMBIOS_TABLE_SET_STR(product_name_str, type1.product); + SMBIOS_TABLE_SET_STR(version_str, type1.version); + SMBIOS_TABLE_SET_STR(serial_number_str, type1.serial); if (qemu_uuid_set) { - smbios_add_field(1, offsetof(struct smbios_type_1, uuid), - qemu_uuid, 16); + memcpy(p->uuid, qemu_uuid, 16); + } else { + memset(p->uuid, 0, 16); } + p->wake_up_type = 0x06; /* power switch */ + SMBIOS_TABLE_SET_STR(sku_number_str, type1.sku); + SMBIOS_TABLE_SET_STR(family_str, type1.family); + + SMBIOS_BUILD_TABLE_POST; } -void smbios_set_type1_defaults(const char *manufacturer, - const char *product, const char *version) +static void smbios_build_type_2_table(void) { - if (!type1.manufacturer) { - type1.manufacturer = manufacturer; - } - if (!type1.product) { - type1.product = product; - } - if (!type1.version) { - type1.version = version; + SMBIOS_BUILD_TABLE_PRE(2, 0x200, 0); /* optional */ + + SMBIOS_TABLE_SET_STR(manufacturer_str, type2.manufacturer); + SMBIOS_TABLE_SET_STR(product_str, type2.product); + SMBIOS_TABLE_SET_STR(version_str, type2.version); + SMBIOS_TABLE_SET_STR(serial_number_str, type2.serial); + SMBIOS_TABLE_SET_STR(asset_tag_number_str, type2.asset); + p->feature_flags = 0x01; /* Motherboard */ + SMBIOS_TABLE_SET_STR(location_str, type2.location); + p->chassis_handle = 0x300; /* Type 3 (System enclosure) */ + p->board_type = 0x0A; /* Motherboard */ + p->contained_element_count = 0; + + SMBIOS_BUILD_TABLE_POST; +} + +#define SMBIOS_SET_DEFAULT(field, value) \ + if (!field) { \ + field = value; \ } + +void smbios_set_defaults(const char *manufacturer, + const char *product, const char *version) +{ + SMBIOS_SET_DEFAULT(type0.vendor, manufacturer); + SMBIOS_SET_DEFAULT(type0.version, version); + SMBIOS_SET_DEFAULT(type0.date, "01/01/2014"); + SMBIOS_SET_DEFAULT(type1.manufacturer, manufacturer); + SMBIOS_SET_DEFAULT(type1.product, product); + SMBIOS_SET_DEFAULT(type1.version, version); + SMBIOS_SET_DEFAULT(type2.manufacturer, manufacturer); + SMBIOS_SET_DEFAULT(type2.product, product); + SMBIOS_SET_DEFAULT(type2.version, version); } uint8_t *smbios_get_table(size_t *length) { if (!smbios_immutable) { - smbios_build_type_0_fields(); - smbios_build_type_1_fields(); + smbios_build_type_0_table(); + smbios_build_type_1_table(); + smbios_build_type_2_table(); smbios_validate_table(); smbios_immutable = true; } @@ -332,7 +411,14 @@ void smbios_entry_add(QemuOpts *opts) } header = (struct smbios_structure_header *)(table->data); - smbios_check_collision(header->type, SMBIOS_TABLE_ENTRY); + + if (test_bit(header->type, have_fields_bitmap)) { + error_report("Can't add binary type %d table! " + "(fields already specified)", header->type); + exit(1); + } + set_bit(header->type, have_binfile_bitmap); + if (header->type == 4) { smbios_type4_count++; } @@ -347,7 +433,12 @@ void smbios_entry_add(QemuOpts *opts) if (val) { unsigned long type = strtoul(val, NULL, 0); - smbios_check_collision(type, SMBIOS_FIELD_ENTRY); + if (test_bit(type, have_binfile_bitmap)) { + error_report("Can't add fields for type %ld table! " + "(binary file already loaded)", type); + exit(1); + } + set_bit(type, have_fields_bitmap); switch (type) { case 0: @@ -391,6 +482,19 @@ void smbios_entry_add(QemuOpts *opts) qemu_uuid_set = true; } return; + case 2: + qemu_opts_validate(opts, qemu_smbios_type2_opts, &local_err); + if (local_err) { + error_report("%s", error_get_pretty(local_err)); + exit(1); + } + save_opt(&type2.manufacturer, opts, "manufacturer"); + save_opt(&type2.product, opts, "product"); + save_opt(&type2.version, opts, "version"); + save_opt(&type2.serial, opts, "serial"); + save_opt(&type2.asset, opts, "asset"); + save_opt(&type2.location, opts, "location"); + return; default: error_report("Don't know how to build fields for SMBIOS type %ld", type); diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h index 4e0d9c9..5d8c657 100644 --- a/include/hw/i386/smbios.h +++ b/include/hw/i386/smbios.h @@ -16,8 +16,8 @@ #include "qemu/option.h" void smbios_entry_add(QemuOpts *opts); -void smbios_set_type1_defaults(const char *manufacturer, - const char *product, const char *version); +void smbios_set_defaults(const char *manufacturer, + const char *product, const char *version); uint8_t *smbios_get_table(size_t *length); /* -- 1.8.1.4
On Tue, Mar 04, 2014 at 02:20:06PM -0500, Gabriel L. Somlo wrote:
Kevin and Gerd: I'd like to get your acked-by for this patch (or any future revision): you're the two people who've been editing src/fw/smbios.c in SeaBIOS, and while I didn't directly cut'n'paste anything from there, I did in fact stare at it very intently, and I'd like to get your OK with the lgplv3 -> gplv2+ difference in licensing between SeaBIOS and, respectively, QEMU.
I'm okay with licensing the seabios src/fw/smbios.c file as GPLv2+. -Kevin
On Di, 2014-03-04 at 14:20 -0500, Gabriel L. Somlo wrote:
- deprecate passing individual smbios fields (SMBIOS_FIELD_ENTRY) - added functionality to build full tables instead, and pass them to the bios via fw_cfg as SMBIOS_TABLE_ENTRY blobs - added code to optionally build type 2 smbios table expected by OS X
Great.
Kevin and Gerd: I'd like to get your acked-by for this patch (or any future revision): you're the two people who've been editing src/fw/smbios.c in SeaBIOS, and while I didn't directly cut'n'paste anything from there, I did in fact stare at it very intently, and I'd like to get your OK with the lgplv3 -> gplv2+ difference in licensing between SeaBIOS and, respectively, QEMU.
Fine with me.
-static bool smbios_type1_defaults = true; +static bool smbios_defaults = true; /* Make sure that guest addresses aligned at 1Gbyte boundaries get mapped to * host addresses aligned at 1Gbyte boundaries. This way we can use 1GByte * pages in the host. @@ -143,9 +143,9 @@ static void pc_init1(QEMUMachineInitArgs *args, guest_info->has_pci_info = has_pci_info; guest_info->isapc_ram_fw = !pci_enabled;
- if (smbios_type1_defaults) { + if (smbios_defaults) { /* These values are guest ABI, do not change */ - smbios_set_type1_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)", + smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)", args->machine->name); }
Need to be careful here. smbios_type1_defaults handles machine type compatibility. qemu version 1.7 (and older) don't set any smbios fields by default. Current qemu behaves simliar with -M pc-i440fx-1.7 (+older) for compatibility reasons, and smbios_type1_defaults controls that. Given that we don't have a stable release which sets smbios defaults I think it is ok to do it this way (assuming it is merged before the 2.0 release). Splitting the patches would be nice for review (add table generator / switch type0 / switch type1 / remove field support / add type2). Maybe we should continue to use the individual fields interface for 1.7 machine types, for best backward compatibility. Overall it looks good to me, don't see any major issues. cheers, Gerd
On Wed, Mar 05, 2014 at 11:59:44AM +0100, Gerd Hoffmann wrote:
-static bool smbios_type1_defaults = true; +static bool smbios_defaults = true; /* Make sure that guest addresses aligned at 1Gbyte boundaries get mapped to * host addresses aligned at 1Gbyte boundaries. This way we can use 1GByte * pages in the host. @@ -143,9 +143,9 @@ static void pc_init1(QEMUMachineInitArgs *args, guest_info->has_pci_info = has_pci_info; guest_info->isapc_ram_fw = !pci_enabled;
- if (smbios_type1_defaults) { + if (smbios_defaults) { /* These values are guest ABI, do not change */ - smbios_set_type1_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)", + smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)", args->machine->name); }
Need to be careful here. smbios_type1_defaults handles machine type compatibility. qemu version 1.7 (and older) don't set any smbios fields by default. Current qemu behaves simliar with -M pc-i440fx-1.7 (+older) for compatibility reasons, and smbios_type1_defaults controls that.
I was just reusing the type_1 defaults for more than just type_1 tables, so I felt I should rename it to reflect this new, more general purpose. It's still setting type_1 defaults if/when necessary. I might also be misunderstanding your point, sorry if that's the case...
Given that we don't have a stable release which sets smbios defaults I think it is ok to do it this way (assuming it is merged before the 2.0 release).
Splitting the patches would be nice for review (add table generator / switch type0 / switch type1 / remove field support / add type2).
Yeah, it did occur to me that the diff looked horribly incomprehensible, so I'll split it all out into smaller, hopefully much more intelligible bits :)
Maybe we should continue to use the individual fields interface for 1.7 machine types, for best backward compatibility.
I'm trying to wrap my head around how I'd distinguish between fields on the command line that are to be passed on as individual fields, vs. fields that should be used to build a full table...
Overall it looks good to me, don't see any major issues.
Thanks, I'll re-send as a series of smaller patches, hopefully with a few additional table types being built locally. --Gabriel
On Mi, 2014-03-05 at 09:48 -0500, Gabriel L. Somlo wrote:
On Wed, Mar 05, 2014 at 11:59:44AM +0100, Gerd Hoffmann wrote:
-static bool smbios_type1_defaults = true; +static bool smbios_defaults = true; /* Make sure that guest addresses aligned at 1Gbyte boundaries get mapped to * host addresses aligned at 1Gbyte boundaries. This way we can use 1GByte * pages in the host. @@ -143,9 +143,9 @@ static void pc_init1(QEMUMachineInitArgs *args, guest_info->has_pci_info = has_pci_info; guest_info->isapc_ram_fw = !pci_enabled;
- if (smbios_type1_defaults) { + if (smbios_defaults) { /* These values are guest ABI, do not change */ - smbios_set_type1_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)", + smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)", args->machine->name); }
Need to be careful here. smbios_type1_defaults handles machine type compatibility. qemu version 1.7 (and older) don't set any smbios fields by default. Current qemu behaves simliar with -M pc-i440fx-1.7 (+older) for compatibility reasons, and smbios_type1_defaults controls that.
I was just reusing the type_1 defaults for more than just type_1 tables, so I felt I should rename it to reflect this new, more general purpose. It's still setting type_1 defaults if/when necessary.
Ok, let me try explain again with some more background info. Qemu tries to enable guest visible changes and sometimes new features (depends on the feature) only in new machine types. If you install a guest with (for example) virt-manager, libvirt will store the current machine type in the guest xml (which is pc-i440fx-1.7 on the stable branch). When qemu is upgrated to the upcoming 2.0 release, the guest will continue to be started with -M pc-i440fx-1.7 and the expectation is that the guest will not see any changes. There are two reasons for that: * First, we want avoid confusing the guest and triggering unwanted actions such as windows wanting be reactivated due to hardware changes. * Second, for live migration compatibility. New features might need additional state to be stored, so we turn them off for compatibility. You don't need to worry about live migration, but the smbios stuff clearly goes into the guest-visible change category. smbios_type1_defaults is one of the compatibility controls. It is false for 1.7 (+older) machine types and true otherwise. So when mucking with it we need to be careful to not break the compatibility stuff. So, if we manage to get the patches into shape in time for qemu 2.0 your way to do that is fine. We are pretty close to the 2.0 freeze though, so maybe we should better plan for post-2.0 anyway, especially as you plan to add more tables. If the patches get merged after 2.0, for 2.1, then we'll need a separate variable as we'll have two compatibility modes (for 1.7+older and 2.0) then.
Maybe we should continue to use the individual fields interface for 1.7 machine types, for best backward compatibility.
I'm trying to wrap my head around how I'd distinguish between fields on the command line that are to be passed on as individual fields, vs. fields that should be used to build a full table...
Assuming the patches get in after 2.0. Then we'll have some new compat variable, say smbios_generate_tables (false for 2.0+older, true otherwise). And in the code: if (smbios_generate_tables) { /* generate complete type1 table and pass it on */ } else { /* use existing code to pass individual type1 fields */ } The compat stuff is a bit messy at times, but I hope it is more clear now ... cheers, Gerd
On Thu, Mar 06, 2014 at 10:03:32AM +0100, Gerd Hoffmann wrote:
You don't need to worry about live migration, but the smbios stuff clearly goes into the guest-visible change category.
smbios_type1_defaults is one of the compatibility controls. It is false for 1.7 (+older) machine types and true otherwise. So when mucking with it we need to be careful to not break the compatibility stuff.
So, if we manage to get the patches into shape in time for qemu 2.0 your way to do that is fine. We are pretty close to the 2.0 freeze though, so maybe we should better plan for post-2.0 anyway, especially as you plan to add more tables.
Well, the only other type I'm personally interested in is 17 (memory device), because SeaBIOS doesn't build its smbios-v2.3 fields, even though it advertises v2.4 in its entry point data structure. And OS X is calling us out on that bluff... :) Type 2 (base board) is required during boot by OS X 10.7 and 10.8 (strangely though, not by 10.6 or 10.9). But if it were only about Type 2, I'd probably have left it at passing the binary table in via the command line :) What really convinced me to go for all this additional work was Laszlo's suggestion that this might help if/when we try to start trying to use UEFI/tianocore/ovmf instead of SeaBIOS. Thanks, --Gabriel
On 03/06/14 17:09, Gabriel L. Somlo wrote:
On Thu, Mar 06, 2014 at 10:03:32AM +0100, Gerd Hoffmann wrote:
You don't need to worry about live migration, but the smbios stuff clearly goes into the guest-visible change category.
smbios_type1_defaults is one of the compatibility controls. It is false for 1.7 (+older) machine types and true otherwise. So when mucking with it we need to be careful to not break the compatibility stuff.
So, if we manage to get the patches into shape in time for qemu 2.0 your way to do that is fine. We are pretty close to the 2.0 freeze though, so maybe we should better plan for post-2.0 anyway, especially as you plan to add more tables.
Well, the only other type I'm personally interested in is 17 (memory device), because SeaBIOS doesn't build its smbios-v2.3 fields, even though it advertises v2.4 in its entry point data structure. And OS X is calling us out on that bluff... :)
Type 2 (base board) is required during boot by OS X 10.7 and 10.8 (strangely though, not by 10.6 or 10.9). But if it were only about Type 2, I'd probably have left it at passing the binary table in via the command line :)
What really convinced me to go for all this additional work was Laszlo's suggestion that this might help if/when we try to start trying to use UEFI/tianocore/ovmf instead of SeaBIOS.
Let me be a bit more precise... :) Moving SMBIOS generation from SeaBIOS to qemu (similarly to ACPI) would benefit: - SeaBIOS (IIRC Kevin had implied his preference for this), - OVMF (no need to play catch-up field-wise), - other boot firmware. I think I didn't suggest using OVMF *instead of* SeaBIOS. :) In any case, I think if you can pull of this migration of SMBIOS tables, that would be a huge service to the community. I should have reviewed your series but it seemed hard, and I didn't have to "look very far" for other work :), so I postponed it. Then Gerd said "please split it up into smaller patches", which I can only agree with! :) Thanks! Laszlo
participants (4)
-
Gabriel L. Somlo -
Gerd Hoffmann -
Kevin O'Connor -
Laszlo Ersek