[Qemu-devel] [PATCH v2 0/2] SMBIOS cleanup round

[Qemu-devel] [PATCH v2 0/2] SMBIOS cleanup round

[Gabriel L. Somlo]

New since version 1:

  - fix endian-ness for *all* larger-than-8bit fields (patch 1/2)

  - resubmit type0 update (2/2) with appropriate endian-ness fix
    for 64-bit field.

Thanks,
  Gabriel

Gabriel L. Somlo (2):
  SMBIOS: Fix endian-ness when populating fields wider than 8-bit
  SMBIOS: Update Type 0 struct generator for machines >= 2.1

 hw/i386/smbios.c         | 109 +++++++++++++++++++++++++----------------------
 include/hw/i386/smbios.h |   2 +-
 2 files changed, 58 insertions(+), 53 deletions(-)

-- 
1.9.0

[Qemu-devel] [PATCH v2 1/2] SMBIOS: Fix endian-ness when populating fields wider than 8-bit

[Gabriel L. Somlo]

When i386 guests are emulated on big endian hosts, make sure
fields wider than 8 bits are populated safely via cpu_to_le*().

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

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 7660718..609e921 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -444,7 +444,7 @@ static bool smbios_skip_table(uint8_t type, bool required_table)
                                                                           \
         t->header.type = tbl_type;                                        \
         t->header.length = sizeof(*t);                                    \
-        t->header.handle = tbl_handle;                                    \
+        t->header.handle = cpu_to_le16(tbl_handle);                       \
     } while (0)
 
 #define SMBIOS_TABLE_SET_STR(tbl_type, field, value)                      \
@@ -491,7 +491,7 @@ static void smbios_build_type_0_table(void)
     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 */
+    t->bios_starting_address_segment = cpu_to_le16(0xE800); /* from SeaBIOS */
 
     SMBIOS_TABLE_SET_STR(0, bios_release_date_str, type0.date);
 
@@ -551,7 +551,7 @@ static void smbios_build_type_2_table(void)
     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->chassis_handle = cpu_to_le16(0x300); /* Type 3 (System enclosure) */
     t->board_type = 0x0A; /* Motherboard */
     t->contained_element_count = 0;
 
@@ -571,7 +571,7 @@ static void smbios_build_type_3_table(void)
     t->power_supply_state = 0x03; /* Safe */
     t->thermal_state = 0x03; /* Safe */
     t->security_status = 0x02; /* Unknown */
-    t->oem_defined = 0;
+    t->oem_defined = cpu_to_le32(0);
     t->height = 0;
     t->number_of_power_cords = 0;
     t->contained_element_count = 0;
@@ -589,26 +589,27 @@ static void smbios_build_type_4_table(unsigned instance)
     snprintf(sock_str, sizeof(sock_str), "%s%2x", type4.sock_pfx, instance);
     SMBIOS_TABLE_SET_STR(4, socket_designation_str, sock_str);
     t->processor_type = 0x03; /* CPU */
+    t->processor_family = 0x01; /* Other */
     SMBIOS_TABLE_SET_STR(4, processor_manufacturer_str, type4.manufacturer);
-    t->processor_id[0] = smbios_cpuid_version;
-    t->processor_id[1] = smbios_cpuid_features;
+    t->processor_id[0] = smbios_cpuid_version; /* opaque, no cpu_to_le16 */
+    t->processor_id[1] = smbios_cpuid_features; /* opaque, no cpu_to_le16 */
     SMBIOS_TABLE_SET_STR(4, processor_version_str, type4.version);
     t->voltage = 0;
-    t->external_clock = 0; /* Unknown */
-    t->max_speed = 0; /* Unknown */
-    t->current_speed = 0; /* Unknown */
+    t->external_clock = cpu_to_le16(0); /* Unknown */
+    t->max_speed = cpu_to_le16(0); /* Unknown */
+    t->current_speed = cpu_to_le16(0); /* Unknown */
     t->status = 0x41; /* Socket populated, CPU enabled */
     t->processor_upgrade = 0x01; /* Other */
-    t->l1_cache_handle = 0xFFFF; /* N/A */
-    t->l2_cache_handle = 0xFFFF; /* N/A */
-    t->l3_cache_handle = 0xFFFF; /* N/A */
+    t->l1_cache_handle = cpu_to_le16(0xFFFF); /* N/A */
+    t->l2_cache_handle = cpu_to_le16(0xFFFF); /* N/A */
+    t->l3_cache_handle = cpu_to_le16(0xFFFF); /* N/A */
     SMBIOS_TABLE_SET_STR(4, serial_number_str, type4.serial);
     SMBIOS_TABLE_SET_STR(4, asset_tag_number_str, type4.asset);
     SMBIOS_TABLE_SET_STR(4, part_number_str, type4.part);
     t->core_count = t->core_enabled = smp_cores;
     t->thread_count = smp_threads;
-    t->processor_characteristics = 0x02; /* Unknown */
-    t->processor_family = t->processor_family2 = 0x01; /* Other */
+    t->processor_characteristics = cpu_to_le16(0x02); /* Unknown */
+    t->processor_family2 = cpu_to_le16(0x01); /* Other */
 
     SMBIOS_BUILD_TABLE_POST;
     smbios_type4_count++;
@@ -631,14 +632,14 @@ static void smbios_build_type_16_table(unsigned dimm_cnt)
     t->error_correction = 0x06; /* Multi-bit ECC (for Microsoft, per SeaBIOS) */
     size_kb = QEMU_ALIGN_UP(ram_size, ONE_KB) / ONE_KB;
     if (size_kb < MAX_T16_STD_SZ) {
-        t->maximum_capacity = size_kb;
-        t->extended_maximum_capacity = 0;
+        t->maximum_capacity = cpu_to_le32(size_kb);
+        t->extended_maximum_capacity = cpu_to_le64(0);
     } else {
-        t->maximum_capacity = MAX_T16_STD_SZ;
-        t->extended_maximum_capacity = ram_size;
+        t->maximum_capacity = cpu_to_le32(MAX_T16_STD_SZ);
+        t->extended_maximum_capacity = cpu_to_le64(ram_size);
     }
-    t->memory_error_information_handle = 0xFFFE; /* Not provided */
-    t->number_of_memory_devices = dimm_cnt;
+    t->memory_error_information_handle = cpu_to_le16(0xFFFE); /* Not provided */
+    t->number_of_memory_devices = cpu_to_le16(dimm_cnt);
 
     SMBIOS_BUILD_TABLE_POST;
 }
@@ -653,18 +654,18 @@ static void smbios_build_type_17_table(unsigned instance, ram_addr_t size)
 
     SMBIOS_BUILD_TABLE_PRE(17, 0x1100 + instance, true); /* required */
 
-    t->physical_memory_array_handle = 0x1000; /* Type 16 (Phys. Mem. Array) */
-    t->memory_error_information_handle = 0xFFFE; /* Not provided */
-    t->total_width = 0xFFFF; /* Unknown */
-    t->data_width = 0xFFFF; /* Unknown */
+    t->physical_memory_array_handle = cpu_to_le16(0x1000); /* Type 16 above */
+    t->memory_error_information_handle = cpu_to_le16(0xFFFE); /* Not provided */
+    t->total_width = cpu_to_le16(0xFFFF); /* Unknown */
+    t->data_width = cpu_to_le16(0xFFFF); /* Unknown */
     size_mb = QEMU_ALIGN_UP(size, ONE_MB) / ONE_MB;
     if (size_mb < MAX_T17_STD_SZ) {
-        t->size = size_mb;
-        t->extended_size = 0;
+        t->size = cpu_to_le16(size_mb);
+        t->extended_size = cpu_to_le32(0);
     } else {
         assert(size_mb < MAX_T17_EXT_SZ);
-        t->size = MAX_T17_STD_SZ;
-        t->extended_size = size_mb;
+        t->size = cpu_to_le16(MAX_T17_STD_SZ);
+        t->extended_size = cpu_to_le32(size_mb);
     }
     t->form_factor = 0x09; /* DIMM */
     t->device_set = 0; /* Not in a set */
@@ -672,17 +673,17 @@ static void smbios_build_type_17_table(unsigned instance, ram_addr_t size)
     SMBIOS_TABLE_SET_STR(17, device_locator_str, loc_str);
     SMBIOS_TABLE_SET_STR(17, bank_locator_str, type17.bank);
     t->memory_type = 0x07; /* RAM */
-    t->type_detail = 0x02; /* Other */
-    t->speed = 0; /* Unknown */
+    t->type_detail = cpu_to_le16(0x02); /* Other */
+    t->speed = cpu_to_le16(0); /* Unknown */
     SMBIOS_TABLE_SET_STR(17, manufacturer_str, type17.manufacturer);
     SMBIOS_TABLE_SET_STR(17, serial_number_str, type17.serial);
     SMBIOS_TABLE_SET_STR(17, asset_tag_number_str, type17.asset);
     SMBIOS_TABLE_SET_STR(17, part_number_str, type17.part);
     t->attributes = 0; /* Unknown */
-    t->configured_clock_speed = 0; /* Unknown */
-    t->minimum_voltage = 0; /* Unknown */
-    t->maximum_voltage = 0; /* Unknown */
-    t->configured_voltage = 0; /* Unknown */
+    t->configured_clock_speed = cpu_to_le32(0); /* Unknown */
+    t->minimum_voltage = cpu_to_le32(0); /* Unknown */
+    t->maximum_voltage = cpu_to_le32(0); /* Unknown */
+    t->configured_voltage = cpu_to_le32(0); /* Unknown */
 
     SMBIOS_BUILD_TABLE_POST;
 }
@@ -699,15 +700,15 @@ static void smbios_build_type_19_table(unsigned instance,
     start_kb = start / ONE_KB;
     end_kb = end / ONE_KB;
     if (start_kb < UINT32_MAX && end_kb < UINT32_MAX) {
-        t->starting_address = start_kb;
-        t->ending_address = end_kb;
-        t->extended_starting_address = t->extended_ending_address = 0;
+        t->starting_address = cpu_to_le32(start_kb);
+        t->ending_address = cpu_to_le32(end_kb);
+        t->extended_starting_address = t->extended_ending_address = cpu_to_le64(0);
     } else {
-        t->starting_address = t->ending_address = UINT32_MAX;
-        t->extended_starting_address = start;
-        t->extended_ending_address = end;
+        t->starting_address = t->ending_address = cpu_to_le32(UINT32_MAX);
+        t->extended_starting_address = cpu_to_le64(start);
+        t->extended_ending_address = cpu_to_le64(end);
     }
-    t->memory_array_handle = 0x1000; /* Type 16 (Phys. Mem. Array) */
+    t->memory_array_handle = cpu_to_le16(0x1000); /* Type 16 above */
     t->partition_width = 1; /* One device per row */
 
     SMBIOS_BUILD_TABLE_POST;
@@ -794,14 +795,14 @@ static void smbios_entry_point_setup(void)
     ep.smbios_bcd_revision = 0x28;
 
     /* set during table construction, but BIOS may override: */
-    ep.structure_table_length = smbios_tables_len;
-    ep.max_structure_size = smbios_table_max;
-    ep.number_of_structures = smbios_table_cnt;
+    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 = 0; /* where BIOS has copied smbios_tables */
+    ep.structure_table_address = cpu_to_le32(0);
 }
 
 void smbios_get_tables(uint8_t **tables, size_t *tables_len,
-- 
1.9.0

Re: [Qemu-devel] [PATCH v2 1/2] SMBIOS: Fix endian-ness when populating fields wider than 8-bit

[Laszlo Ersek]

comments below

On 05/13/14 20:17, Gabriel L. Somlo wrote:
> When i386 guests are emulated on big endian hosts, make sure
> fields wider than 8 bits are populated safely via cpu_to_le*().
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  hw/i386/smbios.c | 91 ++++++++++++++++++++++++++++----------------------------
>  1 file changed, 46 insertions(+), 45 deletions(-)
> 
> diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
> index 7660718..609e921 100644
> --- a/hw/i386/smbios.c
> +++ b/hw/i386/smbios.c
> @@ -444,7 +444,7 @@ static bool smbios_skip_table(uint8_t type, bool required_table)
>                                                                            \
>          t->header.type = tbl_type;                                        \
>          t->header.length = sizeof(*t);                                    \
> -        t->header.handle = tbl_handle;                                    \
> +        t->header.handle = cpu_to_le16(tbl_handle);                       \
>      } while (0)
>  
>  #define SMBIOS_TABLE_SET_STR(tbl_type, field, value)                      \
> @@ -491,7 +491,7 @@ static void smbios_build_type_0_table(void)
>      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 */
> +    t->bios_starting_address_segment = cpu_to_le16(0xE800); /* from SeaBIOS */
>  
>      SMBIOS_TABLE_SET_STR(0, bios_release_date_str, type0.date);
>  
> @@ -551,7 +551,7 @@ static void smbios_build_type_2_table(void)
>      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->chassis_handle = cpu_to_le16(0x300); /* Type 3 (System enclosure) */
>      t->board_type = 0x0A; /* Motherboard */
>      t->contained_element_count = 0;
>  
> @@ -571,7 +571,7 @@ static void smbios_build_type_3_table(void)
>      t->power_supply_state = 0x03; /* Safe */
>      t->thermal_state = 0x03; /* Safe */
>      t->security_status = 0x02; /* Unknown */
> -    t->oem_defined = 0;
> +    t->oem_defined = cpu_to_le32(0);
>      t->height = 0;
>      t->number_of_power_cords = 0;
>      t->contained_element_count = 0;
> @@ -589,26 +589,27 @@ static void smbios_build_type_4_table(unsigned instance)
>      snprintf(sock_str, sizeof(sock_str), "%s%2x", type4.sock_pfx, instance);
>      SMBIOS_TABLE_SET_STR(4, socket_designation_str, sock_str);
>      t->processor_type = 0x03; /* CPU */
> +    t->processor_family = 0x01; /* Other */

This does something else than a cpu_to_leXX() conversion. Intended?

>      SMBIOS_TABLE_SET_STR(4, processor_manufacturer_str, type4.manufacturer);
> -    t->processor_id[0] = smbios_cpuid_version;
> -    t->processor_id[1] = smbios_cpuid_features;
> +    t->processor_id[0] = smbios_cpuid_version; /* opaque, no cpu_to_le16 */
> +    t->processor_id[1] = smbios_cpuid_features; /* opaque, no cpu_to_le16 */

My 2.7.1 smbios spec says one QWORD here (spanning both array elements).
The array elements are uint32_t, so the comments should probably say "no
cpu_to_le32". Not important.

>      SMBIOS_TABLE_SET_STR(4, processor_version_str, type4.version);
>      t->voltage = 0;
> -    t->external_clock = 0; /* Unknown */
> -    t->max_speed = 0; /* Unknown */
> -    t->current_speed = 0; /* Unknown */
> +    t->external_clock = cpu_to_le16(0); /* Unknown */
> +    t->max_speed = cpu_to_le16(0); /* Unknown */
> +    t->current_speed = cpu_to_le16(0); /* Unknown */
>      t->status = 0x41; /* Socket populated, CPU enabled */
>      t->processor_upgrade = 0x01; /* Other */
> -    t->l1_cache_handle = 0xFFFF; /* N/A */
> -    t->l2_cache_handle = 0xFFFF; /* N/A */
> -    t->l3_cache_handle = 0xFFFF; /* N/A */
> +    t->l1_cache_handle = cpu_to_le16(0xFFFF); /* N/A */
> +    t->l2_cache_handle = cpu_to_le16(0xFFFF); /* N/A */
> +    t->l3_cache_handle = cpu_to_le16(0xFFFF); /* N/A */
>      SMBIOS_TABLE_SET_STR(4, serial_number_str, type4.serial);
>      SMBIOS_TABLE_SET_STR(4, asset_tag_number_str, type4.asset);
>      SMBIOS_TABLE_SET_STR(4, part_number_str, type4.part);
>      t->core_count = t->core_enabled = smp_cores;
>      t->thread_count = smp_threads;
> -    t->processor_characteristics = 0x02; /* Unknown */
> -    t->processor_family = t->processor_family2 = 0x01; /* Other */
> +    t->processor_characteristics = cpu_to_le16(0x02); /* Unknown */
> +    t->processor_family2 = cpu_to_le16(0x01); /* Other */

Ah I understand. "processor_family" is BYTE, that's why you split it off.

>  
>      SMBIOS_BUILD_TABLE_POST;
>      smbios_type4_count++;
> @@ -631,14 +632,14 @@ static void smbios_build_type_16_table(unsigned dimm_cnt)
>      t->error_correction = 0x06; /* Multi-bit ECC (for Microsoft, per SeaBIOS) */
>      size_kb = QEMU_ALIGN_UP(ram_size, ONE_KB) / ONE_KB;
>      if (size_kb < MAX_T16_STD_SZ) {
> -        t->maximum_capacity = size_kb;
> -        t->extended_maximum_capacity = 0;
> +        t->maximum_capacity = cpu_to_le32(size_kb);
> +        t->extended_maximum_capacity = cpu_to_le64(0);
>      } else {
> -        t->maximum_capacity = MAX_T16_STD_SZ;
> -        t->extended_maximum_capacity = ram_size;
> +        t->maximum_capacity = cpu_to_le32(MAX_T16_STD_SZ);
> +        t->extended_maximum_capacity = cpu_to_le64(ram_size);
>      }
> -    t->memory_error_information_handle = 0xFFFE; /* Not provided */
> -    t->number_of_memory_devices = dimm_cnt;
> +    t->memory_error_information_handle = cpu_to_le16(0xFFFE); /* Not provided */
> +    t->number_of_memory_devices = cpu_to_le16(dimm_cnt);
>  
>      SMBIOS_BUILD_TABLE_POST;
>  }
> @@ -653,18 +654,18 @@ static void smbios_build_type_17_table(unsigned instance, ram_addr_t size)
>  
>      SMBIOS_BUILD_TABLE_PRE(17, 0x1100 + instance, true); /* required */
>  
> -    t->physical_memory_array_handle = 0x1000; /* Type 16 (Phys. Mem. Array) */
> -    t->memory_error_information_handle = 0xFFFE; /* Not provided */
> -    t->total_width = 0xFFFF; /* Unknown */
> -    t->data_width = 0xFFFF; /* Unknown */
> +    t->physical_memory_array_handle = cpu_to_le16(0x1000); /* Type 16 above */
> +    t->memory_error_information_handle = cpu_to_le16(0xFFFE); /* Not provided */
> +    t->total_width = cpu_to_le16(0xFFFF); /* Unknown */
> +    t->data_width = cpu_to_le16(0xFFFF); /* Unknown */
>      size_mb = QEMU_ALIGN_UP(size, ONE_MB) / ONE_MB;
>      if (size_mb < MAX_T17_STD_SZ) {
> -        t->size = size_mb;
> -        t->extended_size = 0;
> +        t->size = cpu_to_le16(size_mb);
> +        t->extended_size = cpu_to_le32(0);
>      } else {
>          assert(size_mb < MAX_T17_EXT_SZ);
> -        t->size = MAX_T17_STD_SZ;
> -        t->extended_size = size_mb;
> +        t->size = cpu_to_le16(MAX_T17_STD_SZ);
> +        t->extended_size = cpu_to_le32(size_mb);
>      }
>      t->form_factor = 0x09; /* DIMM */
>      t->device_set = 0; /* Not in a set */
> @@ -672,17 +673,17 @@ static void smbios_build_type_17_table(unsigned instance, ram_addr_t size)
>      SMBIOS_TABLE_SET_STR(17, device_locator_str, loc_str);
>      SMBIOS_TABLE_SET_STR(17, bank_locator_str, type17.bank);
>      t->memory_type = 0x07; /* RAM */
> -    t->type_detail = 0x02; /* Other */
> -    t->speed = 0; /* Unknown */
> +    t->type_detail = cpu_to_le16(0x02); /* Other */
> +    t->speed = cpu_to_le16(0); /* Unknown */
>      SMBIOS_TABLE_SET_STR(17, manufacturer_str, type17.manufacturer);
>      SMBIOS_TABLE_SET_STR(17, serial_number_str, type17.serial);
>      SMBIOS_TABLE_SET_STR(17, asset_tag_number_str, type17.asset);
>      SMBIOS_TABLE_SET_STR(17, part_number_str, type17.part);
>      t->attributes = 0; /* Unknown */
> -    t->configured_clock_speed = 0; /* Unknown */
> -    t->minimum_voltage = 0; /* Unknown */
> -    t->maximum_voltage = 0; /* Unknown */
> -    t->configured_voltage = 0; /* Unknown */
> +    t->configured_clock_speed = cpu_to_le32(0); /* Unknown */

"Configured Memory Clock Speed" at offset 20h in Type17 is WORD in my
2.7.1 spec. Was this expanded to DWORD in 2.8? (Along with the addition
of the voltage fields?)

> +    t->minimum_voltage = cpu_to_le32(0); /* Unknown */
> +    t->maximum_voltage = cpu_to_le32(0); /* Unknown */
> +    t->configured_voltage = cpu_to_le32(0); /* Unknown */
>  
>      SMBIOS_BUILD_TABLE_POST;
>  }
> @@ -699,15 +700,15 @@ static void smbios_build_type_19_table(unsigned instance,
>      start_kb = start / ONE_KB;
>      end_kb = end / ONE_KB;
>      if (start_kb < UINT32_MAX && end_kb < UINT32_MAX) {
> -        t->starting_address = start_kb;
> -        t->ending_address = end_kb;
> -        t->extended_starting_address = t->extended_ending_address = 0;
> +        t->starting_address = cpu_to_le32(start_kb);
> +        t->ending_address = cpu_to_le32(end_kb);
> +        t->extended_starting_address = t->extended_ending_address = cpu_to_le64(0);
>      } else {
> -        t->starting_address = t->ending_address = UINT32_MAX;
> -        t->extended_starting_address = start;
> -        t->extended_ending_address = end;
> +        t->starting_address = t->ending_address = cpu_to_le32(UINT32_MAX);
> +        t->extended_starting_address = cpu_to_le64(start);
> +        t->extended_ending_address = cpu_to_le64(end);
>      }
> -    t->memory_array_handle = 0x1000; /* Type 16 (Phys. Mem. Array) */
> +    t->memory_array_handle = cpu_to_le16(0x1000); /* Type 16 above */
>      t->partition_width = 1; /* One device per row */
>  
>      SMBIOS_BUILD_TABLE_POST;
> @@ -794,14 +795,14 @@ static void smbios_entry_point_setup(void)
>      ep.smbios_bcd_revision = 0x28;
>  
>      /* set during table construction, but BIOS may override: */
> -    ep.structure_table_length = smbios_tables_len;
> -    ep.max_structure_size = smbios_table_max;
> -    ep.number_of_structures = smbios_table_cnt;
> +    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 = 0; /* where BIOS has copied smbios_tables */
> +    ep.structure_table_address = cpu_to_le32(0);
>  }
>  
>  void smbios_get_tables(uint8_t **tables, size_t *tables_len,
> 

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v2 1/2] SMBIOS: Fix endian-ness when populating fields wider than 8-bit

[Paolo Bonzini]

Il 13/05/2014 20:17, Gabriel L. Somlo ha scritto:
> -    t->processor_id[0] = smbios_cpuid_version;
> -    t->processor_id[1] = smbios_cpuid_features;
> +    t->processor_id[0] = smbios_cpuid_version; /* opaque, no cpu_to_le16 */
> +    t->processor_id[1] = smbios_cpuid_features; /* opaque, no cpu_to_le16 */

Actually I think these should be subject to cpu_to_le32.  They 
correspond to CPUID values, so they are little-endian 32-bit.

Paolo

Re: [Qemu-devel] [PATCH v2 1/2] SMBIOS: Fix endian-ness when populating fields wider than 8-bit

[Gabriel L. Somlo]

On Tue, May 13, 2014 at 10:09:45PM +0200, Paolo Bonzini wrote:
> Il 13/05/2014 20:17, Gabriel L. Somlo ha scritto:
> >-    t->processor_id[0] = smbios_cpuid_version;
> >-    t->processor_id[1] = smbios_cpuid_features;
> >+    t->processor_id[0] = smbios_cpuid_version; /* opaque, no cpu_to_le16 */
> >+    t->processor_id[1] = smbios_cpuid_features; /* opaque, no cpu_to_le16 */
> 
> Actually I think these should be subject to cpu_to_le32.  They
> correspond to CPUID values, so they are little-endian 32-bit.

Right, my comment is wrong, should be "no need for cpu_to_le32()" instead
of what I said ("no need for cpu_to_le16()")... :)

Re. why I said "opaque, no need for endianness fix" is because they're
a straight-through assignment from cpu->env.cpuid_version and 
cpu->env.features[FEAT_1_EDX], respectively. So they're not really
"numbers" which need endianness adjustment.

Did I get that wrong ? (I mean, besides the 16 vs. 32 in the comment) ?

Thanks,
--Gabriel

Re: [Qemu-devel] [PATCH v2 1/2] SMBIOS: Fix endian-ness when populating fields wider than 8-bit

[Laszlo Ersek]

On 05/13/14 22:21, Gabriel L. Somlo wrote:
> On Tue, May 13, 2014 at 10:09:45PM +0200, Paolo Bonzini wrote:
>> Il 13/05/2014 20:17, Gabriel L. Somlo ha scritto:
>>> -    t->processor_id[0] = smbios_cpuid_version;
>>> -    t->processor_id[1] = smbios_cpuid_features;
>>> +    t->processor_id[0] = smbios_cpuid_version; /* opaque, no cpu_to_le16 */
>>> +    t->processor_id[1] = smbios_cpuid_features; /* opaque, no cpu_to_le16 */
>>
>> Actually I think these should be subject to cpu_to_le32.  They
>> correspond to CPUID values, so they are little-endian 32-bit.
> 
> Right, my comment is wrong, should be "no need for cpu_to_le32()" instead
> of what I said ("no need for cpu_to_le16()")... :)
> 
> Re. why I said "opaque, no need for endianness fix" is because they're
> a straight-through assignment from cpu->env.cpuid_version and 
> cpu->env.features[FEAT_1_EDX], respectively. So they're not really
> "numbers" which need endianness adjustment.
> 
> Did I get that wrong ? (I mean, besides the 16 vs. 32 in the comment) ?

See 7.5.3.1 "x86-Class CPUs" in the spec.

> For x86 class CPUs, the field’s format depends on the processor’s
> support of the CPUID instruction. If the instruction is supported,
> the Processor ID field contains two DWORD-formatted values. The first
> (offsets 08h-0Bh) is the EAX value returned by a CPUID instruction
> with input EAX set to 1; the second (offsets 0Ch-0Fh) is the EDX
> value returned by that instruction.

I'm inclined to think that Paolo's right.

Laszlo

Re: [Qemu-devel] [PATCH v2 1/2] SMBIOS: Fix endian-ness when populating fields wider than 8-bit

[Paolo Bonzini]

Il 13/05/2014 22:21, Gabriel L. Somlo ha scritto:
>>> > >-    t->processor_id[0] = smbios_cpuid_version;
>>> > >-    t->processor_id[1] = smbios_cpuid_features;
>>> > >+    t->processor_id[0] = smbios_cpuid_version; /* opaque, no cpu_to_le16 */
>>> > >+    t->processor_id[1] = smbios_cpuid_features; /* opaque, no cpu_to_le16 */
>> >
>> > Actually I think these should be subject to cpu_to_le32.  They
>> > correspond to CPUID values, so they are little-endian 32-bit.
> Right, my comment is wrong, should be "no need for cpu_to_le32()" instead
> of what I said ("no need for cpu_to_le16()")... :)
>
> Re. why I said "opaque, no need for endianness fix" is because they're
> a straight-through assignment from cpu->env.cpuid_version and
> cpu->env.features[FEAT_1_EDX], respectively. So they're not really
> "numbers" which need endianness adjustment.
>
> Did I get that wrong ? (I mean, besides the 16 vs. 32 in the comment) ?

I think they do need cpu_to_le32.  We assign them straight from other 
32-bit integers, but still bits 0..7 should be in byte 0, bits 8..15 
should be in byte 1, etc.  So we need to convert from host CPU 
endianness to target CPU endianness.

Paolo

Re: [Qemu-devel] [PATCH v2 1/2] SMBIOS: Fix endian-ness when populating fields wider than 8-bit

[Gabriel L. Somlo]

On Tue, May 13, 2014 at 10:33:35PM +0200, Paolo Bonzini wrote:
> Il 13/05/2014 22:21, Gabriel L. Somlo ha scritto:
> >>>> >-    t->processor_id[0] = smbios_cpuid_version;
> >>>> >-    t->processor_id[1] = smbios_cpuid_features;
> >>>> >+    t->processor_id[0] = smbios_cpuid_version; /* opaque, no cpu_to_le16 */
> >>>> >+    t->processor_id[1] = smbios_cpuid_features; /* opaque, no cpu_to_le16 */
> >>>
> >>> Actually I think these should be subject to cpu_to_le32.  They
> >>> correspond to CPUID values, so they are little-endian 32-bit.
> >Right, my comment is wrong, should be "no need for cpu_to_le32()" instead
> >of what I said ("no need for cpu_to_le16()")... :)
> >
> >Re. why I said "opaque, no need for endianness fix" is because they're
> >a straight-through assignment from cpu->env.cpuid_version and
> >cpu->env.features[FEAT_1_EDX], respectively. So they're not really
> >"numbers" which need endianness adjustment.
> >
> >Did I get that wrong ? (I mean, besides the 16 vs. 32 in the comment) ?
> 
> I think they do need cpu_to_le32.  We assign them straight from
> other 32-bit integers, but still bits 0..7 should be in byte 0, bits
> 8..15 should be in byte 1, etc.  So we need to convert from host CPU
> endianness to target CPU endianness.

OK, so the host creates cpu->env.cpuid_version and
cpu->env.features[FEAT_1_EDX] in *host* format, so on
a BE host the i386 cpu values would be in BE ordrer,
and so I'd have to convert them to LE on behalf of the
x86 guest ?

OK, I'll go ahead and do that, then :)

Thanks!

--Gabriel

[Qemu-devel] [PATCH v2 2/2] SMBIOS: Update Type 0 struct generator for machines >= 2.1

[Gabriel L. Somlo]

A type 0 (bios info) smbios structure is only generated if explicitly
requested on the command line. This patch updates the mechanism for
generating this type of structure as follows:

  - convert bios_characteristics field to uin64_t (instead of uint8_t[8])
    as described in the current smbios spec (v2.8)

  - enable "virtual machine" bit in bios_characteristics_extension_bits

  - add command line option to enable "uefi supported" bit in
    bios_characteristics_extension_bits

These updates should make this optional smbios structure more useful when
used with edk2/ovmf. Only pc machines 2.1 and newer are affected, and only
when the user explicitly requests that a type 0 struct be generated.

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

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 609e921..a002fa6 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -67,7 +67,7 @@ static DECLARE_BITMAP(have_fields_bitmap, SMBIOS_MAX_TYPE+1);
 
 static struct {
     const char *vendor, *version, *date;
-    bool have_major_minor;
+    bool have_major_minor, uefi;
     uint8_t major, minor;
 } type0;
 
@@ -134,6 +134,10 @@ static const QemuOptDesc qemu_smbios_type0_opts[] = {
         .name = "release",
         .type = QEMU_OPT_STRING,
         .help = "revision number",
+    },{
+        .name = "uefi",
+        .type = QEMU_OPT_BOOL,
+        .help = "uefi support",
     },
     { /* end of list */ }
 };
@@ -497,13 +501,12 @@ static void smbios_build_type_0_table(void)
 
     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 = cpu_to_le64(0x08); /* Not supported */
     t->bios_characteristics_extension_bytes[0] = 0;
-    t->bios_characteristics_extension_bytes[1] = 4;
+    t->bios_characteristics_extension_bytes[1] = 0x14; /* TCD/SVVP | VM */
+    if (type0.uefi) {
+        t->bios_characteristics_extension_bytes[1] |= 0x08; /* |= UEFI */
+    }
 
     if (type0.have_major_minor) {
         t->system_bios_major_release = type0.major;
@@ -978,6 +981,7 @@ void smbios_entry_add(QemuOpts *opts)
             save_opt(&type0.vendor, opts, "vendor");
             save_opt(&type0.version, opts, "version");
             save_opt(&type0.date, opts, "date");
+            type0.uefi = qemu_opt_get_bool(opts, "uefi", false);
 
             val = qemu_opt_get(opts, "release");
             if (val) {
diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
index 6d854b7..5583f60 100644
--- a/include/hw/i386/smbios.h
+++ b/include/hw/i386/smbios.h
@@ -64,7 +64,7 @@ struct smbios_type_0 {
     uint16_t bios_starting_address_segment;
     uint8_t bios_release_date_str;
     uint8_t bios_rom_size;
-    uint8_t bios_characteristics[8];
+    uint64_t bios_characteristics;
     uint8_t bios_characteristics_extension_bytes[2];
     uint8_t system_bios_major_release;
     uint8_t system_bios_minor_release;
-- 
1.9.0

Re: [Qemu-devel] [PATCH v2 2/2] SMBIOS: Update Type 0 struct generator for machines >= 2.1

[Laszlo Ersek]

On 05/13/14 20:17, Gabriel L. Somlo wrote:
> A type 0 (bios info) smbios structure is only generated if explicitly
> requested on the command line. This patch updates the mechanism for
> generating this type of structure as follows:
> 
>   - convert bios_characteristics field to uin64_t (instead of uint8_t[8])
>     as described in the current smbios spec (v2.8)
> 
>   - enable "virtual machine" bit in bios_characteristics_extension_bits
> 
>   - add command line option to enable "uefi supported" bit in
>     bios_characteristics_extension_bits
> 
> These updates should make this optional smbios structure more useful when
> used with edk2/ovmf. Only pc machines 2.1 and newer are affected, and only
> when the user explicitly requests that a type 0 struct be generated.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  hw/i386/smbios.c         | 18 +++++++++++-------
>  include/hw/i386/smbios.h |  2 +-
>  2 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
> index 609e921..a002fa6 100644
> --- a/hw/i386/smbios.c
> +++ b/hw/i386/smbios.c
> @@ -67,7 +67,7 @@ static DECLARE_BITMAP(have_fields_bitmap, SMBIOS_MAX_TYPE+1);
>  
>  static struct {
>      const char *vendor, *version, *date;
> -    bool have_major_minor;
> +    bool have_major_minor, uefi;
>      uint8_t major, minor;
>  } type0;
>  
> @@ -134,6 +134,10 @@ static const QemuOptDesc qemu_smbios_type0_opts[] = {
>          .name = "release",
>          .type = QEMU_OPT_STRING,
>          .help = "revision number",
> +    },{
> +        .name = "uefi",
> +        .type = QEMU_OPT_BOOL,
> +        .help = "uefi support",
>      },
>      { /* end of list */ }
>  };
> @@ -497,13 +501,12 @@ static void smbios_build_type_0_table(void)
>  
>      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 = cpu_to_le64(0x08); /* Not supported */
>      t->bios_characteristics_extension_bytes[0] = 0;
> -    t->bios_characteristics_extension_bytes[1] = 4;
> +    t->bios_characteristics_extension_bytes[1] = 0x14; /* TCD/SVVP | VM */
> +    if (type0.uefi) {
> +        t->bios_characteristics_extension_bytes[1] |= 0x08; /* |= UEFI */
> +    }
>  
>      if (type0.have_major_minor) {
>          t->system_bios_major_release = type0.major;
> @@ -978,6 +981,7 @@ void smbios_entry_add(QemuOpts *opts)
>              save_opt(&type0.vendor, opts, "vendor");
>              save_opt(&type0.version, opts, "version");
>              save_opt(&type0.date, opts, "date");
> +            type0.uefi = qemu_opt_get_bool(opts, "uefi", false);
>  
>              val = qemu_opt_get(opts, "release");
>              if (val) {
> diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
> index 6d854b7..5583f60 100644
> --- a/include/hw/i386/smbios.h
> +++ b/include/hw/i386/smbios.h
> @@ -64,7 +64,7 @@ struct smbios_type_0 {
>      uint16_t bios_starting_address_segment;
>      uint8_t bios_release_date_str;
>      uint8_t bios_rom_size;
> -    uint8_t bios_characteristics[8];
> +    uint64_t bios_characteristics;
>      uint8_t bios_characteristics_extension_bytes[2];
>      uint8_t system_bios_major_release;
>      uint8_t system_bios_minor_release;
> 

Looks good to me, but I forgot to ask you to extend the documentation
too, in qemu-options.hx (near '-smbios type=0', two instances of it). I
apologize.

Thanks
Laszlo