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

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

[Gabriel L. Somlo]

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

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

[Kevin O'Connor]

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

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

[Gerd Hoffmann]

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

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

[Gabriel L. Somlo]

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' ;)

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

[Kevin O'Connor]

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

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

[Gerd Hoffmann]

  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

[Qemu-devel] [PATCH v3] SMBIOS: Update Type 4, 17 structs to v2.3 of the spec

[Gabriel L. Somlo]

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

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

[Laszlo Ersek]

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

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

[Laszlo Ersek]

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

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

[Gerd Hoffmann]

  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

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

[Gabriel L. Somlo]

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;
     }

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

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

[Gerd Hoffmann]

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

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

[Gabriel L. Somlo]

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

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

[Gabriel L. Somlo]

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

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

[Gabriel L. Somlo]

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

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

[Laszlo Ersek]

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

[Qemu-devel] [PATCH 1/2] QEMU: SMBIOS: Update all structs to v2.3; Add type 2 struct

[Gabriel L. Somlo]

  - 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

[Qemu-devel] [PATCH 2/2 (RFC)] QEMU: SMBIOS: Build full smbios tables

[Gabriel L. Somlo]

   - 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

Re: [Qemu-devel] [PATCH 2/2 (RFC)] QEMU: SMBIOS: Build full smbios tables

[Kevin O'Connor]

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

Re: [Qemu-devel] [PATCH 2/2 (RFC)] QEMU: SMBIOS: Build full smbios tables

[Gerd Hoffmann]

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

Re: [Qemu-devel] [PATCH 2/2 (RFC)] QEMU: SMBIOS: Build full smbios tables

[Gabriel L. Somlo]

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

Re: [Qemu-devel] [PATCH 2/2 (RFC)] QEMU: SMBIOS: Build full smbios tables

[Gerd Hoffmann]

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

Re: [Qemu-devel] [PATCH 2/2 (RFC)] QEMU: SMBIOS: Build full smbios tables

[Gabriel L. Somlo]

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

Re: [Qemu-devel] [PATCH 2/2 (RFC)] QEMU: SMBIOS: Build full smbios tables

[Laszlo Ersek]

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

Re: [Qemu-devel] QEMU: SMBIOS: Build full smbios tables

[Gabriel L. Somlo]

On Thu, Mar 06, 2014 at 06:04:32PM +0100, Laszlo Ersek wrote:
> > 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. :)

Sorry if I unintentionally misrepresented what you said, I was trying
to paraphrase precisely what you just said above :)

> 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! :)

I'll send the current set of seven patches, which basically replace
the way types 0 and 1 are handled with building full tables, and adds
the option to request that a type 2 table be built.

I'd like to get the rest of what SeaBIOS builds by default (types 3+)
in before the 2.0 freeze (BTW, how much longer do I have for that?),
but while I start hacking on that I figured I'd help you help me by
making it easy to review what I have so far... :)

Thanks again,
--Gabriel

Re: [Qemu-devel] QEMU: SMBIOS: Build full smbios tables

[Laszlo Ersek]

On 03/09/14 03:57, Gabriel L. Somlo wrote:

> I'll send the current set of seven patches, which basically replace
> the way types 0 and 1 are handled with building full tables, and adds
> the option to request that a type 2 table be built.
> 
> I'd like to get the rest of what SeaBIOS builds by default (types 3+)
> in before the 2.0 freeze (BTW, how much longer do I have for that?),

The hard freeze is on March 12th. <http://wiki.qemu.org/Planning/2.0>

> but while I start hacking on that I figured I'd help you help me by
> making it easy to review what I have so far... :)

My review queue is too long again, so I might not be overly quick...

Thanks
Laszlo

Re: [Qemu-devel] QEMU: SMBIOS: Build full smbios tables

[Gabriel L. Somlo]

On Sun, Mar 09, 2014 at 08:18:11PM +0100, Laszlo Ersek wrote:
> On 03/09/14 03:57, Gabriel L. Somlo wrote:
> > I'd like to get the rest of what SeaBIOS builds by default (types 3+)
> > in before the 2.0 freeze (BTW, how much longer do I have for that?),
> 
> The hard freeze is on March 12th. <http://wiki.qemu.org/Planning/2.0>

I guess that's doable, unless reviews also have to be done by then, in
which case it's out of my hands :)

> > but while I start hacking on that I figured I'd help you help me by
> > making it easy to review what I have so far... :)
> 
> My review queue is too long again, so I might not be overly quick...

Tangentially related, do any of you know of the best/fastest way to
grab the "CPUX86State *env" of (one of) the x86 CPU (about to be)
configured for the current guest ?

I have to populate these type 4 fields like so:

	processor_id[0] = env->cpuid_version;
	processor_id[1] = env=>features[FEAT_1_EDX];

and I'm a bit lost as to where (and whether) this stuff is available
from anywhere at the time the smbios initialization code is being
executed...

SeaBIOS has the luxury of executing the CPUID instruction from within
guest context and using the returned %eax and %edx, but I'm hoping
there's a shortcut from the host side :)

Thanks,
--Gabriel

[Qemu-devel] [PATCH 1/7] SMBIOS: Update all table definitions to smbios spec v2.3

[Gabriel L. Somlo]

Table definitions for types 4 and 17 are only up to v2.0,
so add fields specified in smbios v2.3, as expected (and
advertised) by the SeaBIOS smbios entry point structure.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 include/hw/i386/smbios.h | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
index 18fb970..de1da87 100644
--- a/include/hw/i386/smbios.h
+++ b/include/hw/i386/smbios.h
@@ -79,7 +79,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 +97,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 +115,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 +131,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

[Qemu-devel] [PATCH 2/7] SMBIOS: Rename smbios_set_type1_defaults() for more general use

[Gabriel L. Somlo]

Subsequent patches will utilize this function to set defaults for
more smbios types than just type 1, so the function name should
reflect this.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/i386/pc_piix.c        | 12 ++++++------
 hw/i386/pc_q35.c         |  8 ++++----
 hw/i386/smbios.c         |  4 ++--
 include/hw/i386/smbios.h |  4 ++--
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index ae1699d..ef2d062 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);
     }
 
@@ -264,7 +264,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;
 }
 
@@ -343,7 +343,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);
@@ -353,7 +353,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..89dc070 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -256,8 +256,8 @@ static void smbios_build_type_1_fields(void)
     }
 }
 
-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)
 {
     if (!type1.manufacturer) {
         type1.manufacturer = manufacturer;
diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
index de1da87..a7ec973 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

[Qemu-devel] [PATCH 3/7] SMBIOS: Streamline setting smbios defaults with macro

[Gabriel L. Somlo]

The function smbios_set_defaults() uses a repeating code pattern
for each field. This patch replaces that pattern with a macro.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/i386/smbios.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 89dc070..f4ee7b4 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -256,18 +256,17 @@ static void smbios_build_type_1_fields(void)
     }
 }
 
+#define SMBIOS_SET_DEFAULT(field, value)                                  \
+    if (!field) {                                                         \
+        field = value;                                                    \
+    }
+
 void smbios_set_defaults(const char *manufacturer,
                          const char *product, const char *version)
 {
-    if (!type1.manufacturer) {
-        type1.manufacturer = manufacturer;
-    }
-    if (!type1.product) {
-        type1.product = product;
-    }
-    if (!type1.version) {
-        type1.version = version;
-    }
+    SMBIOS_SET_DEFAULT(type1.manufacturer, manufacturer);
+    SMBIOS_SET_DEFAULT(type1.product, product);
+    SMBIOS_SET_DEFAULT(type1.version, version);
 }
 
 uint8_t *smbios_get_table(size_t *length)
-- 
1.8.1.4

[Qemu-devel] [PATCH 4/7] SMBIOS: Replace type collision check mechanism with bitmaps

[Gabriel L. Somlo]

Replace existing smbios_check_collision() functionality with
a pair of bitmaps: have_binfile_bitmap and have_fields_bitmap.
Bits corresponding to each smbios type are set by smbios_entry_add(),
which also uses the bitmaps to ensure that binary blobs and field
values are never accepted for the same type.

These bitmaps will also be used in the future to decide whether
or not to build a full table for a given smbios type.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/i386/smbios.c         | 51 ++++++++++++++++++++----------------------------
 include/hw/i386/smbios.h |  2 ++
 2 files changed, 23 insertions(+), 30 deletions(-)

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index f4ee7b4..6889332 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -49,11 +49,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, SMBIOS_MAX_TYPE+1);
+static DECLARE_BITMAP(have_fields_bitmap, SMBIOS_MAX_TYPE+1);
 
 static struct {
     const char *vendor, *version, *date;
@@ -164,29 +161,6 @@ 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)
-{
-    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);
-        }
-    }
-}
-
 static void smbios_add_field(int type, int offset, const void *data, size_t len)
 {
     struct smbios_field *field;
@@ -331,7 +305,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++;
         }
@@ -346,7 +327,17 @@ void smbios_entry_add(QemuOpts *opts)
     if (val) {
         unsigned long type = strtoul(val, NULL, 0);
 
-        smbios_check_collision(type, SMBIOS_FIELD_ENTRY);
+        if (type > SMBIOS_MAX_TYPE) {
+            error_report("smbios type (%ld) out of range!", type);
+            exit(1);
+        }
+
+        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:
diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
index a7ec973..8b63441 100644
--- a/include/hw/i386/smbios.h
+++ b/include/hw/i386/smbios.h
@@ -15,6 +15,8 @@
 
 #include "qemu/option.h"
 
+#define SMBIOS_MAX_TYPE 127
+
 void smbios_entry_add(QemuOpts *opts);
 void smbios_set_defaults(const char *manufacturer,
                          const char *product, const char *version);
-- 
1.8.1.4

[Qemu-devel] [PATCH 5/7] SMBIOS: Add code to build full smbios tables

[Gabriel L. Somlo]

This patch adds a set of macros which build full smbios tables
of a given type, including the logic to decide whether a given
table type should be built or not.

To illustrate this new functionality, we introduce and optionally
build a table of type 2 (base board), which is required by some
versions of OS X (10.7 and 10.8).

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/i386/smbios.c         | 158 +++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/i386/smbios.h |  16 +++++
 2 files changed, 174 insertions(+)

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 6889332..9679e06 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -48,6 +48,7 @@ static uint8_t *smbios_entries;
 static size_t smbios_entries_len;
 static int smbios_type4_count = 0;
 static bool smbios_immutable;
+static bool smbios_build_tables;
 
 static DECLARE_BITMAP(have_binfile_bitmap, SMBIOS_MAX_TYPE+1);
 static DECLARE_BITMAP(have_fields_bitmap, SMBIOS_MAX_TYPE+1);
@@ -63,6 +64,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),
@@ -146,6 +151,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);
@@ -161,6 +199,90 @@ static void smbios_validate_table(void)
     }
 }
 
+static bool smbios_skip_table(uint8_t type, bool required_table)
+{
+    if (test_bit(type, have_binfile_bitmap)) {
+        return true; /* user provided their own binary blob(s) */
+    }
+    if (test_bit(type, have_fields_bitmap)) {
+        return false; /* user provided fields via command line */
+    }
+    if (smbios_build_tables && required_table) {
+        return false; /* we're building tables, and this one's required */
+    }
+    return true;
+}
+
+#define SMBIOS_BUILD_TABLE_PRE(tbl_type, tbl_handle, tbl_required)        \
+    struct smbios_table *w;                                               \
+    struct smbios_type_##tbl_type *t;                                     \
+    size_t w_off, t_off; /* wrapper, table offsets into smbios_entries */ \
+    int str_index = 0;                                                    \
+    do {                                                                  \
+        /* should we skip building this table ? */                        \
+        if (smbios_skip_table(tbl_type, tbl_required)) {                  \
+            return;                                                       \
+        }                                                                 \
+                                                                          \
+        /* initialize fw_cfg smbios element count */                      \
+        if (!smbios_entries) {                                            \
+            smbios_entries_len = sizeof(uint16_t);                        \
+            smbios_entries = g_malloc0(smbios_entries_len);               \
+        }                                                                 \
+                                                                          \
+        /* use offsets of wrapper w and table t within smbios_entries  */ \
+        /* (pointers must be updated after each realloc)               */ \
+        w_off = smbios_entries_len;                                       \
+        t_off = w_off + sizeof(*w);                                       \
+        smbios_entries_len = t_off + sizeof(*t);                          \
+        smbios_entries = g_realloc(smbios_entries, smbios_entries_len);   \
+        w = (struct smbios_table *)(smbios_entries + w_off);              \
+        t = (struct smbios_type_##tbl_type *)(smbios_entries + t_off);    \
+                                                                          \
+        w->header.type = SMBIOS_TABLE_ENTRY;                              \
+        w->header.length = sizeof(*w) + sizeof(*t);                       \
+                                                                          \
+        t->header.type = tbl_type;                                        \
+        t->header.length = sizeof(*t);                                    \
+        t->header.handle = tbl_handle;                                    \
+    } while (0)
+
+#define SMBIOS_TABLE_SET_STR(tbl_type, 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;                                    \
+            /* update pointer(s) post-realloc */                          \
+            w = (struct smbios_table *)(smbios_entries + w_off);          \
+            t = (struct smbios_type_##tbl_type *)(smbios_entries + t_off);\
+            t->field = ++str_index;                                       \
+            w->header.length += len;                                      \
+        } else {                                                          \
+            t->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;                                      \
+            /* update pointer(s) post-realloc */                          \
+            w = (struct smbios_table *)(smbios_entries + w_off);          \
+            w->header.length += 1;                                        \
+        }                                                                 \
+                                                                          \
+        /* update fw_cfg smbios element count */                          \
+        *(uint16_t *)smbios_entries += 1;                                 \
+    } while (0)
+
 static void smbios_add_field(int type, int offset, const void *data, size_t len)
 {
     struct smbios_field *field;
@@ -230,6 +352,24 @@ static void smbios_build_type_1_fields(void)
     }
 }
 
+static void smbios_build_type_2_table(void)
+{
+    SMBIOS_BUILD_TABLE_PRE(2, 0x200, false); /* optional */
+
+    SMBIOS_TABLE_SET_STR(2, manufacturer_str, type2.manufacturer);
+    SMBIOS_TABLE_SET_STR(2, product_str, type2.product);
+    SMBIOS_TABLE_SET_STR(2, version_str, type2.version);
+    SMBIOS_TABLE_SET_STR(2, serial_number_str, type2.serial);
+    SMBIOS_TABLE_SET_STR(2, asset_tag_number_str, type2.asset);
+    t->feature_flags = 0x01; /* Motherboard */
+    SMBIOS_TABLE_SET_STR(2, location_str, type2.location);
+    t->chassis_handle = 0x300; /* Type 3 (System enclosure) */
+    t->board_type = 0x0A; /* Motherboard */
+    t->contained_element_count = 0;
+
+    SMBIOS_BUILD_TABLE_POST;
+}
+
 #define SMBIOS_SET_DEFAULT(field, value)                                  \
     if (!field) {                                                         \
         field = value;                                                    \
@@ -238,14 +378,19 @@ static void smbios_build_type_1_fields(void)
 void smbios_set_defaults(const char *manufacturer,
                          const char *product, const char *version)
 {
+    smbios_build_tables = true;
     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_2_table();
         smbios_build_type_0_fields();
         smbios_build_type_1_fields();
         smbios_validate_table();
@@ -381,6 +526,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 8b63441..dfa5104 100644
--- a/include/hw/i386/smbios.h
+++ b/include/hw/i386/smbios.h
@@ -62,6 +62,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;
-- 
1.8.1.4

[Qemu-devel] [PATCH 6/7] SMBIOS: Build full tables for types 0 and 1

[Gabriel L. Somlo]

If defaults are set by piix or q35, or fields are given on the
command line, build full smbios tables for types 0 and 1 instead
of sending individual fields to the BIOS via fw_cfg.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/i386/smbios.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 9679e06..e7df372 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -352,6 +352,62 @@ static void smbios_build_type_1_fields(void)
     }
 }
 
+static void smbios_build_type_0_table(void)
+{
+    SMBIOS_BUILD_TABLE_PRE(0, 0x000, true); /* required */
+
+    SMBIOS_TABLE_SET_STR(0, vendor_str, type0.vendor);
+    SMBIOS_TABLE_SET_STR(0, bios_version_str, type0.version);
+
+    t->bios_starting_address_segment = 0xE800; /* hardcoded in SeaBIOS */
+
+    SMBIOS_TABLE_SET_STR(0, bios_release_date_str, type0.date);
+
+    t->bios_rom_size = 0; /* hardcoded in SeaBIOS with FIXME comment */
+
+    /* BIOS characteristics not supported */
+    memset(t->bios_characteristics, 0, 8);
+    t->bios_characteristics[0] = 0x08;
+
+    /* Enable targeted content distribution (needed for SVVP, per SeaBIOS) */
+    t->bios_characteristics_extension_bytes[0] = 0;
+    t->bios_characteristics_extension_bytes[1] = 4;
+
+    if (type0.have_major_minor) {
+        t->system_bios_major_release = type0.major;
+        t->system_bios_minor_release = type0.minor;
+    } else {
+        t->system_bios_major_release = 0;
+        t->system_bios_minor_release = 0;
+    }
+
+    /* hardcoded in SeaBIOS */
+    t->embedded_controller_major_release = 0xFF;
+    t->embedded_controller_minor_release = 0xFF;
+
+    SMBIOS_BUILD_TABLE_POST;
+}
+
+static void smbios_build_type_1_table(void)
+{
+    SMBIOS_BUILD_TABLE_PRE(1, 0x100, true); /* required */
+
+    SMBIOS_TABLE_SET_STR(1, manufacturer_str, type1.manufacturer);
+    SMBIOS_TABLE_SET_STR(1, product_name_str, type1.product);
+    SMBIOS_TABLE_SET_STR(1, version_str, type1.version);
+    SMBIOS_TABLE_SET_STR(1, serial_number_str, type1.serial);
+    if (qemu_uuid_set) {
+        memcpy(t->uuid, qemu_uuid, 16);
+    } else {
+        memset(t->uuid, 0, 16);
+    }
+    t->wake_up_type = 0x06; /* power switch */
+    SMBIOS_TABLE_SET_STR(1, sku_number_str, type1.sku);
+    SMBIOS_TABLE_SET_STR(1, family_str, type1.family);
+
+    SMBIOS_BUILD_TABLE_POST;
+}
+
 static void smbios_build_type_2_table(void)
 {
     SMBIOS_BUILD_TABLE_PRE(2, 0x200, false); /* optional */
@@ -379,6 +435,9 @@ void smbios_set_defaults(const char *manufacturer,
                          const char *product, const char *version)
 {
     smbios_build_tables = true;
+    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);
@@ -390,9 +449,13 @@ void smbios_set_defaults(const char *manufacturer,
 uint8_t *smbios_get_table(size_t *length)
 {
     if (!smbios_immutable) {
+        smbios_build_type_0_table();
+        smbios_build_type_1_table();
         smbios_build_type_2_table();
+if (false) { /* shut up gcc until we remove deprecated code */
         smbios_build_type_0_fields();
         smbios_build_type_1_fields();
+}
         smbios_validate_table();
         smbios_immutable = true;
     }
-- 
1.8.1.4

[Qemu-devel] [PATCH 7/7] SMBIOS: Remove unused code for passing individual fields to bios

[Gabriel L. Somlo]

This patch removes smbios_add_field() and the old code to insert
individual fields for types 0 and 1 into fw_cfg.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/i386/smbios.c | 80 --------------------------------------------------------
 1 file changed, 80 deletions(-)

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index e7df372..54ad3e7 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[];
@@ -283,75 +276,6 @@ static bool smbios_skip_table(uint8_t type, bool required_table)
         *(uint16_t *)smbios_entries += 1;                                 \
     } while (0)
 
-static void smbios_add_field(int type, int offset, const void *data, size_t len)
-{
-    struct smbios_field *field;
-
-    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);
-}
-
-static void smbios_maybe_add_str(int type, int offset, const char *data)
-{
-    if (data) {
-        smbios_add_field(type, offset, data, strlen(data) + 1);
-    }
-}
-
-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);
-    }
-}
-
-static void smbios_build_type_1_fields(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);
-    if (qemu_uuid_set) {
-        smbios_add_field(1, offsetof(struct smbios_type_1, uuid),
-                         qemu_uuid, 16);
-    }
-}
-
 static void smbios_build_type_0_table(void)
 {
     SMBIOS_BUILD_TABLE_PRE(0, 0x000, true); /* required */
@@ -452,10 +376,6 @@ uint8_t *smbios_get_table(size_t *length)
         smbios_build_type_0_table();
         smbios_build_type_1_table();
         smbios_build_type_2_table();
-if (false) { /* shut up gcc until we remove deprecated code */
-        smbios_build_type_0_fields();
-        smbios_build_type_1_fields();
-}
         smbios_validate_table();
         smbios_immutable = true;
     }
-- 
1.8.1.4