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 <so...@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
