diff mbox

[ARM,SMBIOS,V1,5/6] smbios: add smbios 3.0 support

Message ID 55BBAC71.5010206@redhat.com
State New
Headers show

Commit Message

Laszlo Ersek July 31, 2015, 5:12 p.m. UTC 
Hello Wei,

there's a bug in one of the SMBIOS 3.0 entry point struct fields. It
relates to the use of "smbios_table_max":

On 07/28/15 08:00, Wei Huang wrote:
> This patch adds support for SMBIOS 3.0 entry point. When caller invokes
> smbios_set_defaults(), it can specify entry point as 2.1 or 3.0. Then
> smbios_get_tables() will return the entry point table in right format.
> 
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  hw/i386/pc_piix.c          |  3 +-
>  hw/i386/pc_q35.c           |  3 +-
>  hw/smbios/smbios.c         | 72 +++++++++++++++++++++++++++++++---------------
>  include/hw/smbios/smbios.h | 30 +++++++++++++++++--
>  4 files changed, 81 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 653c710..04636b1 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -173,7 +173,8 @@ static void pc_init1(MachineState *machine)
>          MachineClass *mc = MACHINE_GET_CLASS(machine);
>          /* These values are guest ABI, do not change */
>          smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)",
> -                            mc->name, smbios_legacy_mode, smbios_uuid_encoded);
> +                            mc->name, smbios_legacy_mode, smbios_uuid_encoded,
> +                            SMBIOS_ENTRY_POINT_21);
>      }
>  
>      /* allocate ram and load rom/bios */
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index d83df14..061507d 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -165,7 +165,8 @@ static void pc_q35_init(MachineState *machine)
>      if (smbios_defaults) {
>          /* These values are guest ABI, do not change */
>          smbios_set_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)",
> -                            mc->name, smbios_legacy_mode, smbios_uuid_encoded);
> +                            mc->name, smbios_legacy_mode, smbios_uuid_encoded,
> +                            SMBIOS_ENTRY_POINT_21);
>      }
>  
>      /* allocate ram and load rom/bios */
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 08ba62a..70ecd87 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -48,6 +48,7 @@ static uint8_t *smbios_entries;
>  static size_t smbios_entries_len;
>  static bool smbios_legacy = true;
>  static bool smbios_uuid_encoded = true;
> +static SmbiosEntryPointType smbios_ep_type = SMBIOS_ENTRY_POINT_21;
>  /* end: legacy structures & constants for <= 2.0 machines */
>  
>  
> @@ -55,7 +56,8 @@ static uint8_t *smbios_tables;
>  static size_t smbios_tables_len;
>  static unsigned smbios_table_max;
>  static unsigned smbios_table_cnt;
> -static struct smbios_entry_point ep;
> +static smbios_entry_point ep;
> +static size_t ep_length;
>  
>  static int smbios_type4_count = 0;
>  static bool smbios_immutable;
> @@ -771,11 +773,12 @@ void smbios_set_cpuid(uint32_t version, uint32_t features)
>  
>  void smbios_set_defaults(const char *manufacturer, const char *product,
>                           const char *version, bool legacy_mode,
> -                         bool uuid_encoded)
> +                         bool uuid_encoded, SmbiosEntryPointType ep_type)
>  {
>      smbios_have_defaults = true;
>      smbios_legacy = legacy_mode;
>      smbios_uuid_encoded = uuid_encoded;
> +    smbios_ep_type = ep_type;
>  
>      /* drop unwanted version of command-line file blob(s) */
>      if (smbios_legacy) {
> @@ -808,26 +811,49 @@ void smbios_set_defaults(const char *manufacturer, const char *product,
>  
>  static void smbios_entry_point_setup(void)
>  {
> -    memcpy(ep.anchor_string, "_SM_", 4);
> -    memcpy(ep.intermediate_anchor_string, "_DMI_", 5);
> -    ep.length = sizeof(struct smbios_entry_point);
> -    ep.entry_point_revision = 0; /* formatted_area reserved, per spec v2.1+ */
> -    memset(ep.formatted_area, 0, 5);
> -
> -    /* compliant with smbios spec v2.8 */
> -    ep.smbios_major_version = 2;
> -    ep.smbios_minor_version = 8;
> -    ep.smbios_bcd_revision = 0x28;
> -
> -    /* set during table construction, but BIOS may override: */
> -    ep.structure_table_length = cpu_to_le16(smbios_tables_len);
> -    ep.max_structure_size = cpu_to_le16(smbios_table_max);
> -    ep.number_of_structures = cpu_to_le16(smbios_table_cnt);
> -
> -    /* BIOS must recalculate: */
> -    ep.checksum = 0;
> -    ep.intermediate_checksum = 0;
> -    ep.structure_table_address = cpu_to_le32(0);
> +    if (smbios_ep_type == SMBIOS_ENTRY_POINT_21) {
> +        memcpy(ep.ep21.anchor_string, "_SM_", 4);
> +        memcpy(ep.ep21.intermediate_anchor_string, "_DMI_", 5);
> +        ep.ep21.length = sizeof(struct smbios_21_entry_point);
> +        ep.ep21.entry_point_revision = 0; /* formatted_area reserved */
> +        memset(ep.ep21.formatted_area, 0, 5);
> +
> +        /* compliant with smbios spec v2.8 */
> +        ep.ep21.smbios_major_version = 2;
> +        ep.ep21.smbios_minor_version = 8;
> +        ep.ep21.smbios_bcd_revision = 0x28;
> +
> +        /* set during table construction, but BIOS may override: */
> +        ep.ep21.structure_table_length = cpu_to_le16(smbios_tables_len);
> +        ep.ep21.max_structure_size = cpu_to_le16(smbios_table_max);

This is correct. In SMBIOS 2.x, the "max_structure_size" field reports:

  Size of the largest SMBIOS structure, in bytes, and encompasses the
  structure’s formatted area and text strings

And the "smbios_table_max" variable actually tracks that, see the
SMBIOS_BUILD_TABLE_POST macro.

        /* update smbios max. element size */

However,

> +        ep.ep21.number_of_structures = cpu_to_le16(smbios_table_cnt);
> +
> +        /* BIOS must recalculate: */
> +        ep.ep21.checksum = 0;
> +        ep.ep21.intermediate_checksum = 0;
> +        ep.ep21.structure_table_address = cpu_to_le32(0);
> +
> +        /* setup the anchor point length */
> +        ep_length = sizeof (struct smbios_21_entry_point);
> +    } else if (smbios_ep_type == SMBIOS_ENTRY_POINT_30) {
> +        memcpy(ep.ep30.anchor_string, "_SM3_", 5);
> +        ep.ep30.length = sizeof(struct smbios_30_entry_point);
> +        ep.ep30.entry_point_revision = 1;
> +
> +        /* compliant with smbios spec 3.0 */
> +        ep.ep30.smbios_major_version = 3;
> +        ep.ep30.smbios_minor_version = 0;
> +        ep.ep30.smbios_doc_rev = 0;
> +
> +        /* set during table construct, but BIOS might override */
> +        ep.ep30.structure_table_max_size = cpu_to_le32(smbios_table_max);

This is incorrect. In SMBIOS 3.0, the "structure_table_max_size" field
does not track the largest individual table (there is no such field any
longer). Instead, this field reports

  Maximum size of SMBIOS Structure Table, pointed to by the Structure
  Table Address, in bytes. The actual size is guaranteed to be less or
  equal to the maximum size.

In other words, you should fill this field in from "smbios_tables_len"
(similarly to "ep.ep21.structure_table_length").

Now you might ask what sense it makes for the 3.0 spec to require an
"upper limit" on the full blob length, and not an *exact* length (as it
was with 2.x). I don't know. I asked Jon Masters on IRC (CC'd), and if I
understood his reply correctly, this is simply an error in the 3.0 spec,
to be fixed in 3.1.

So, please just squash in the attached patch, which should fix this bug
and satisfy the requirements of both the 3.0 and the (expected) 3.1 specs.

With that patch applied, the firmware code I'm about to post (round 2)
seems to accept and install the stuff in the guest. I haven't verified
the tables in a Linux guest (with the dmidecode utility) yet, because I
don't have ready access to a Mustang right now, and with TCG I won't
boot Linux :)

But, I ran the SMBIOSVIEW command in the UEFI shell of AAVMF, and things
look okay there. I'm attaching an excerpt of the output. (Note that the
Type=0 table comes from the firmware itself, as a fallback, same as on
x86 / OVMF, if QEMU doesn't provide one.)

Thanks!
Laszlo

> +
> +        /* BIOS must recalculate */
> +        ep.ep30.checksum = 0;
> +        ep.ep30.structure_table_address = cpu_to_le64(0);
> +
> +        ep_length = sizeof (struct smbios_30_entry_point);
> +    }
>  }
>  
>  void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
> @@ -886,7 +912,7 @@ void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
>      *tables = smbios_tables;
>      *tables_len = smbios_tables_len;
>      *anchor = (uint8_t *)&ep;
> -    *anchor_len = sizeof(struct smbios_entry_point);
> +    *anchor_len = ep_length;
>  }
>  
>  static void save_opt(const char **dest, QemuOpts *opts, const char *name)
> diff --git a/include/hw/smbios/smbios.h b/include/hw/smbios/smbios.h
> index e727233..3dd8a41 100644
> --- a/include/hw/smbios/smbios.h
> +++ b/include/hw/smbios/smbios.h
> @@ -18,6 +18,12 @@
>  
>  #define SMBIOS_MAX_TYPE 127
>  
> +typedef enum SmbiosEntryPointType {
> +    SMBIOS_ENTRY_POINT_21,
> +    SMBIOS_ENTRY_POINT_30,
> +} SmbiosEntryPointType;
> +
> +
>  /* memory area description, used by type 19 table */
>  struct smbios_phys_mem_area {
>      uint64_t address;
> @@ -28,7 +34,7 @@ void smbios_entry_add(QemuOpts *opts);
>  void smbios_set_cpuid(uint32_t version, uint32_t features);
>  void smbios_set_defaults(const char *manufacturer, const char *product,
>                           const char *version, bool legacy_mode,
> -                         bool uuid_encoded);
> +                         bool uuid_encoded, SmbiosEntryPointType ep_type);
>  uint8_t *smbios_get_table_legacy(size_t *length);
>  void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
>                         const unsigned int mem_array_size,
> @@ -43,7 +49,8 @@ void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
>  /* SMBIOS entry point (anchor).
>   * BIOS must place this at a 16-bit-aligned address between 0xf0000 and 0xfffff.
>   */
> -struct smbios_entry_point {
> +/* SMBIOS 2.1 entry point */
> +struct smbios_21_entry_point {
>      uint8_t anchor_string[4];
>      uint8_t checksum;
>      uint8_t length;
> @@ -60,6 +67,25 @@ struct smbios_entry_point {
>      uint8_t smbios_bcd_revision;
>  } QEMU_PACKED;
>  
> +/* SMBIOS 3.0 entry point */
> +struct smbios_30_entry_point {
> +    uint8_t anchor_string[5];
> +    uint8_t checksum;
> +    uint8_t length;
> +    uint8_t smbios_major_version;
> +    uint8_t smbios_minor_version;
> +    uint8_t smbios_doc_rev;
> +    uint8_t entry_point_revision;
> +    uint8_t _reserved;
> +    uint32_t structure_table_max_size;
> +    uint64_t structure_table_address;
> +} QEMU_PACKED;
> +
> +typedef union QEMU_PACKED {
> +    struct smbios_21_entry_point ep21;
> +    struct smbios_30_entry_point ep30;
> +} smbios_entry_point;
> +
>  /* This goes at the beginning of every SMBIOS structure. */
>  struct smbios_structure_header {
>      uint8_t type;
>
SMBIOS 3.0 (64-bit) Entry Point Structure:
Anchor String:        _SM3_
EPS Checksum:         0x71
Entry Point Len:      24
Version:              3.0
SMBIOS Docrev:        0x0
Table Max Size:       316
Table Address:        0xB6EF0000
Entry Point revision: 0x1

=========================================================
Query Structure, conditions are:
QueryType   = Random
QueryHandle = Random
ShowType    = SHOW_DETAIL


=========================================================
Type=1, Handle=0x100
Dump Structure as:
Index=0,Length=0x3A,Addr=0xB6EF0000
00000000: 01 1B 00 01 01 02 03 00-00 00 00 00 00 00 00 00  *................*
00000010: 00 00 00 00 00 00 00 00-06 00 00 51 45 4D 55 00  *...........QEMU.*
00000020: 51 45 4D 55 20 56 69 72-74 75 61 6C 20 4D 61 63  *QEMU Virtual Mac*
00000030: 68 69 6E 65 00 31 2E 30-00 00                    *hine.1.0..*
Structure Type: System Information
Format part Len : 27
Structure Handle: 256
Manufacturer: QEMU
ProductName: QEMU Virtual Machine
Version: 1.0
SerialNumber:
Dump Uuid
size=16:
00000000: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00  *................*
System Wakeup Type: Power Switch
SKUNumber:
Family:


=========================================================
Type=3, Handle=0x300
Dump Structure as:
Index=1,Length=0x1F,Addr=0xB6EF003A
00000000: 03 15 00 03 01 01 02 00-00 03 03 03 02 00 00 00  *................*
00000010: 00 00 00 00 00 51 45 4D-55 00 31 2E 30 00 00     *.....QEMU.1.0..*
Structure Type: System Enclosure
Format part Len : 21
Structure Handle: 768
Manufacturer: QEMU
Type: 1
System Enclosure or Chassis Types:  Other
Version: 1.0
SerialNumber:
AssetTag:
Bootup state System Enclosure or Chassis Status:  Safe
Power Supply State System Enclosure or Chassis Status:  Safe
Thermal state System Enclosure or Chassis Status:  Safe
Security Status System Enclosure or Chassis Security Status:  Unknown
Dump OemDefined
size=4:
00000000: 00 00 00 00                                      *....*


=========================================================
Type=4, Handle=0x400
Dump Structure as:
Index=2,Length=0x3A,Addr=0xB6EF0059
00000000: 04 2A 00 04 01 03 01 02-00 00 00 00 00 00 00 00  *.*..............*
00000010: 03 00 00 00 D0 07 D0 07-41 01 FF FF FF FF FF FF  *........A.......*
00000020: 00 00 00 01 01 01 02 00-01 00 43 50 55 20 30 00  *..........CPU 0.*
00000030: 51 45 4D 55 00 31 2E 30-00 00                    *QEMU.1.0..*
Structure Type: Processor Information
Format part Len : 42
Structure Handle: 1024
Socket: CPU 0
Processor Type:  Central Processor
Processor Family: Other
ProcessorManufacture: QEMU
Dump ProcessorId
size=8:
00000000: 00 00 00 00 00 00 00 00-                         *........*
ProcessorVersion: 1.0
Processor Information -  Voltage:
ExternalClock: 0
MaxSpeed: 2000
CurrentSpeed: 2000
Processor Status:
CPU Socket Populated
CPU Enabled
Processor Upgrade: Other
L1CacheHandle: 0xFFFF
L2CacheHandle: 0xFFFF
L3CacheHandle: 0xFFFF
SerialNumber:
AssetTag:
PartNumber:
CoreCount: 1
EnabledCoreCount: 1
ThreadCount: 1
Processor Characteristics:  Unknown |


=========================================================
Type=16, Handle=0x1000
Dump Structure as:
Index=3,Length=0x19,Addr=0xB6EF0093
00000000: 10 17 00 10 01 03 06 00-00 20 00 FE FF 01 00 00  *......... ......*
00000010: 00 00 00 00 00 00 00 00-00                       *.........*
Structure Type: Physical Memory Array
Format part Len : 23
Structure Handle: 4096
Physical Memory Array Location:   Other
Physical Memory Array Use:  System memory
Physical Memory Array Error Correction Types:   Multi-bit ECC
MaximumCapacity: 0x200000
MemoryErrorInformationHandle: 0xFFFE
NumberOfMemoryDevices: 0x1
ExtendedMaximumCapacity: 0x0


=========================================================
Type=17, Handle=0x1100
Dump Structure as:
Index=4,Length=0x35,Addr=0xB6EF00AC
00000000: 11 28 00 11 00 10 FE FF-FF FF FF FF 00 08 09 00  *.(..............*
00000010: 01 00 07 02 00 00 00 02-00 00 00 00 00 00 00 00  *................*
00000020: 00 00 00 00 00 00 00 00-44 49 4D 4D 20 30 00 51  *........DIMM 0.Q*
00000030: 45 4D 55 00 00                                   *EMU..*
Structure Type: Memory Device
Format part Len : 40
Structure Handle: 4352
MemoryArrayHandle: 0x1000
MemoryErrorInformationHandle: 0xFFFE
TotalWidth: 65535
DataWidth: 65535
Size: 2048
Memory Device - Form Factor:   DIMM
DeviceSet: 0x0
DeviceLocator: DIMM 0
BankLocator:
Memory Device - Type:   RAM
Memory Device - Type Detail:  Other |
Speed: 0x0
Manufacturer: QEMU
SerialNumber:
AssetTag:
PartNumber:
Attributes: 0x0
ExtendedSize: 0
ConfiguredMemoryClockSpeed: 0


=========================================================
Type=32, Handle=0x2000
Dump Structure as:
Index=5,Length=0xD,Addr=0xB6EF00E1
00000000: 20 0B 00 20 00 00 00 00-00 00 00 00 00           * .. .........*
Structure Type: System Boot Information
Format part Len : 11
Structure Handle: 8192
Dump Reserved
size=6:
00000000: 00 00 00 00 00 00                                *......*
System Boot Status:  No errors detected

=========================================================
Type=0, Handle=0x0
Dump Structure as:
Index=6,Length=0x48,Addr=0xB6EF00EE
00000000: 00 18 00 00 01 02 00 E8-03 00 08 00 00 00 00 00  *................*
00000010: 00 00 00 1C 00 00 FF FF-45 46 49 20 44 65 76 65  *........EFI Deve*
00000020: 6C 6F 70 6D 65 6E 74 20-4B 69 74 20 49 49 20 2F  *lopment Kit II /*
00000030: 20 4F 56 4D 46 00 30 2E-30 2E 30 00 30 32 2F 30  * OVMF.0.0.0.02/0*
00000040: 36 2F 32 30 31 35 00 00-                         *6/2015..*
Structure Type: BIOS Information
Format part Len : 24
Structure Handle: 0
Vendor: EFI Development Kit II / OVMF
BiosVersion: 0.0.0
BiosSegment: 59392
BiosReleaseDate: 02/06/2015
BiosSize:  64 KB
BIOS Characteristics:
BIOS Characteristics Not Supported
 Bits 32:47 are reserved for BIOS Vendor
 Bits 48:64 are reserved for System Vendor
BIOS Characteristics Extension Byte1:
BIOS Characteristics Extension Byte2:
Enable Targeted Content Distribution
UEFI Specification is supported
The SMBIOS table describes a virtual machine
 Bits 5:7 are reserved for future assignment
SystemBiosMajorRelease: 0
SystemBiosMinorRelease: 0
EmbeddedControllerFirmwareMajorRelease: 255
EmbeddedControllerFirmwareMinorRelease: 255


=========================================================
Type=127, Handle=0xFEFF
Dump Structure as:
Index=7,Length=0x6,Addr=0xB6EF0136
00000000: 7F 04 FF FE 00 00                                *......*
Structure Type: End-of-Table
Format part Len : 4
Structure Handle: 65279
This structure indicates the End-of-table!


=========================================================
diff mbox

Patch

From 4bac0aa296bd2b3b7dc63895c3d7e19e39c5b13c Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Fri, 31 Jul 2015 18:54:21 +0200
Subject: [PATCH] hw/smbios: fix "Structure table maximum size" field in the
 3.0 entry point

This field carries an inclusive upper bound on the size of the blob that
collects the tables, and not the size of the largest individual table.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/smbios/smbios.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 70ecd87..6f58709 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -846,7 +846,7 @@  static void smbios_entry_point_setup(void)
         ep.ep30.smbios_doc_rev = 0;
 
         /* set during table construct, but BIOS might override */
-        ep.ep30.structure_table_max_size = cpu_to_le32(smbios_table_max);
+        ep.ep30.structure_table_max_size = cpu_to_le32(smbios_tables_len);
 
         /* BIOS must recalculate */
         ep.ep30.checksum = 0;
-- 
1.8.3.1