[Qemu-devel] [PATCH v4 0/3] SMBIOS cleanup round

[Qemu-devel] [PATCH v4 0/3] SMBIOS cleanup round

[Gabriel L. Somlo]

Changelog:

new in v4:
  - slightly improved commit blurbs

v3:
  - endian-ness fix (1/3) now correctly handles cpuid version and features
  - included documentation (qemu-options.hx) for type 0 uefi flag (2/3)
  - fixed type 17 field sizes ("word" means 16 bits, not 32 :) :)


v2:
  - 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.


Gabriel L. Somlo (3):
  SMBIOS: Fix endian-ness when populating multi-byte fields
  SMBIOS: Update Type 0 struct generator for machines >= 2.1
  SMBIOS: Fix type 17 field sizes

 hw/i386/smbios.c         | 110 +++++++++++++++++++++++++----------------------
 include/hw/i386/smbios.h |  10 ++---
 qemu-options.hx          |   4 +-
 3 files changed, 65 insertions(+), 59 deletions(-)

-- 
1.9.0

[Qemu-devel] [PATCH v4 1/3] SMBIOS: Fix endian-ness when populating multi-byte fields

[Gabriel L. Somlo]

When i386 guests are emulated on big endian hosts, make sure
multi-byte fields are populated safely via cpu_to_le*().

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/smbios.c | 92 +++++++++++++++++++++++++++++---------------------------
 1 file changed, 47 insertions(+), 45 deletions(-)

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 7660718..aeb94bc 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] = cpu_to_le32(smbios_cpuid_version);
+    t->processor_id[1] = cpu_to_le32(smbios_cpuid_features);
     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,16 @@ 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 +796,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

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

[Gabriel L. Somlo]

Update how type 0 (bios info) structures are generated, 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 structure more useful when
used with edk2/ovmf. Only pc machines >= 2.1 are affected, and only
when a type 0 structure is explicitly specified on the command line.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/smbios.c         | 18 +++++++++++-------
 include/hw/i386/smbios.h |  2 +-
 qemu-options.hx          |  4 ++--
 3 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index aeb94bc..1793821 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;
@@ -979,6 +982,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;
diff --git a/qemu-options.hx b/qemu-options.hx
index 781af14..15779e7 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1335,7 +1335,7 @@ ETEXI
 DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
     "-smbios file=binary\n"
     "                load SMBIOS entry from binary file\n"
-    "-smbios type=0[,vendor=str][,version=str][,date=str][,release=%d.%d]\n"
+    "-smbios type=0[,vendor=str][,version=str][,date=str][,release=%d.%d][,uefi=on|off]\n"
     "                specify SMBIOS type 0 fields\n"
     "-smbios type=1[,manufacturer=str][,product=str][,version=str][,serial=str]\n"
     "              [,uuid=uuid][,sku=str][,family=str]\n"
@@ -1345,7 +1345,7 @@ STEXI
 @findex -smbios
 Load SMBIOS entry from binary file.
 
-@item -smbios type=0[,vendor=@var{str}][,version=@var{str}][,date=@var{str}][,release=@var{%d.%d}]
+@item -smbios type=0[,vendor=@var{str}][,version=@var{str}][,date=@var{str}][,release=@var{%d.%d}][,uefi=on|off]
 Specify SMBIOS type 0 fields
 
 @item -smbios type=1[,manufacturer=@var{str}][,product=@var{str}] [,version=@var{str}][,serial=@var{str}][,uuid=@var{uuid}][,sku=@var{str}] [,family=@var{str}]
-- 
1.9.0

[Qemu-devel] [PATCH v4 3/3] SMBIOS: Fix type 17 field sizes

[Gabriel L. Somlo]

Fields for configured_clock_speed and various voltage values
introduced in spec v2.7+ should be "word", i.e. 16 bits.

Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/smbios.c         | 8 ++++----
 include/hw/i386/smbios.h | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 1793821..b3bedde 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -683,10 +683,10 @@ static void smbios_build_type_17_table(unsigned instance, ram_addr_t size)
     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 = 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 */
+    t->configured_clock_speed = cpu_to_le16(0); /* Unknown */
+    t->minimum_voltage = cpu_to_le16(0); /* Unknown */
+    t->maximum_voltage = cpu_to_le16(0); /* Unknown */
+    t->configured_voltage = cpu_to_le16(0); /* Unknown */
 
     SMBIOS_BUILD_TABLE_POST;
 }
diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
index 5583f60..a3f4d88 100644
--- a/include/hw/i386/smbios.h
+++ b/include/hw/i386/smbios.h
@@ -182,10 +182,10 @@ struct smbios_type_17 {
     uint8_t part_number_str;
     uint8_t attributes;
     uint32_t extended_size;
-    uint32_t configured_clock_speed;
-    uint32_t minimum_voltage;
-    uint32_t maximum_voltage;
-    uint32_t configured_voltage;
+    uint16_t configured_clock_speed;
+    uint16_t minimum_voltage;
+    uint16_t maximum_voltage;
+    uint16_t configured_voltage;
 } QEMU_PACKED;
 
 /* SMBIOS type 19 - Memory Array Mapped Address (v2.7) */
-- 
1.9.0

Re: [Qemu-devel] [PATCH v4 0/3] SMBIOS cleanup round

[Michael S. Tsirkin]

On Mon, May 19, 2014 at 10:09:52AM -0400, Gabriel L. Somlo wrote:
> Changelog:

OK I queued this, but not yet pushed (to collect acks).
Gerd if it should be your tree instead pls let me know.

> new in v4:
>   - slightly improved commit blurbs
> 
> v3:
>   - endian-ness fix (1/3) now correctly handles cpuid version and features
>   - included documentation (qemu-options.hx) for type 0 uefi flag (2/3)
>   - fixed type 17 field sizes ("word" means 16 bits, not 32 :) :)
> 
> 
> v2:
>   - 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.
> 
> 
> Gabriel L. Somlo (3):
>   SMBIOS: Fix endian-ness when populating multi-byte fields
>   SMBIOS: Update Type 0 struct generator for machines >= 2.1
>   SMBIOS: Fix type 17 field sizes
> 
>  hw/i386/smbios.c         | 110 +++++++++++++++++++++++++----------------------
>  include/hw/i386/smbios.h |  10 ++---
>  qemu-options.hx          |   4 +-
>  3 files changed, 65 insertions(+), 59 deletions(-)
> 
> -- 
> 1.9.0

Re: [Qemu-devel] [PATCH v4 0/3] SMBIOS cleanup round

[Gerd Hoffmann]

On Mo, 2014-05-19 at 19:09 +0300, Michael S. Tsirkin wrote:
> On Mon, May 19, 2014 at 10:09:52AM -0400, Gabriel L. Somlo wrote:
> > Changelog:
> 
> OK I queued this, but not yet pushed (to collect acks).
> Gerd if it should be your tree instead pls let me know.

I was planning to do a pull for this later this week.  But if you merge
it though the pc queue that is fine with me too.

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v4 0/3] SMBIOS cleanup round

[Michael S. Tsirkin]

On Mon, May 19, 2014 at 10:09:52AM -0400, Gabriel L. Somlo wrote:
> Changelog:
> 
> new in v4:
>   - slightly improved commit blurbs
> 
> v3:
>   - endian-ness fix (1/3) now correctly handles cpuid version and features
>   - included documentation (qemu-options.hx) for type 0 uefi flag (2/3)
>   - fixed type 17 field sizes ("word" means 16 bits, not 32 :) :)
> 
> 
> v2:
>   - 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!
One question: we don't seem to have unit-test for this
interface in qemu, do we?
I would like to see at least a basic test along the lines of
tests/acpi-test.c: run bios to load tables from QEMU,
then do some basic checks on the tables that bios loaded.

> Gabriel L. Somlo (3):
>   SMBIOS: Fix endian-ness when populating multi-byte fields
>   SMBIOS: Update Type 0 struct generator for machines >= 2.1
>   SMBIOS: Fix type 17 field sizes
> 
>  hw/i386/smbios.c         | 110 +++++++++++++++++++++++++----------------------
>  include/hw/i386/smbios.h |  10 ++---
>  qemu-options.hx          |   4 +-
>  3 files changed, 65 insertions(+), 59 deletions(-)
> 
> -- 
> 1.9.0

Re: [Qemu-devel] [PATCH v4 0/3] SMBIOS cleanup round

[Gabriel L. Somlo]

On Mon, May 19, 2014 at 10:44:48PM +0300, Michael S. Tsirkin wrote:
> Thanks!
> One question: we don't seem to have unit-test for this
> interface in qemu, do we?
> I would like to see at least a basic test along the lines of
> tests/acpi-test.c: run bios to load tables from QEMU,
> then do some basic checks on the tables that bios loaded.

Heh, this is starting to feel a bit like "work" ;)

So, to be a good citizen, I took a look at acpi-test.c,
and it feels like if I just attempted to "cargo-cult" it,
creating a working unit test for the new smbios interface
is going to take me a while...

Is there anything I can RTFM that I could use to (more or
less gently) teach myself how to write a sane QEMU unit test?

Thanks much,
--Gabriel

> 
> > Gabriel L. Somlo (3):
> >   SMBIOS: Fix endian-ness when populating multi-byte fields
> >   SMBIOS: Update Type 0 struct generator for machines >= 2.1
> >   SMBIOS: Fix type 17 field sizes
> > 
> >  hw/i386/smbios.c         | 110 +++++++++++++++++++++++++----------------------
> >  include/hw/i386/smbios.h |  10 ++---
> >  qemu-options.hx          |   4 +-
> >  3 files changed, 65 insertions(+), 59 deletions(-)
> > 
> > -- 
> > 1.9.0

Re: [Qemu-devel] [PATCH v4 0/3] SMBIOS cleanup round

[Gabriel L. Somlo]

Michael,

On Mon, May 19, 2014 at 10:44:48PM +0300, Michael S. Tsirkin wrote:
> One question: we don't seem to have unit-test for this
> interface in qemu, do we?
> I would like to see at least a basic test along the lines of
> tests/acpi-test.c: run bios to load tables from QEMU,
> then do some basic checks on the tables that bios loaded.

So I took a closer look at tests/acpi-test.c today, and things slowly
started to shift into focus :) So now I have two questions:

1. There's a fairly complex setup (create a boot disk, start the
guest, loop around waiting for the bios to finish booting, watch
when your disk-based boot loader runs, etc.) before starting to
examine the guest memory for the presence and correctness of the acpi
tables.

Would it make sense to  rename this file to something like e.g.
tests/biostables-test.c, and add checks for smbios to the already
started and booted guest ?

If not, I'd have to replicate most of your test-harness code,
which is almost half of acpi-test.c. That shouldn't be hard (you
already did the heavy lifting on that one), but I intuitively dislike
multiple cut'n'paste clones of significant code fragments :)

2. Assuming we can run smbios tests alongside acpi, what do you think
of the patch below ? I've currently stopped after finding and checking
the integrity of the smbios entry point structure. Not sure if I
really need to walk the tables themselves, or what I'd be testing for
if I did :)

Feedback/comments/flames welcome ! :)

Thanks much,
--Gabriel


diff --git a/tests/acpi-test.c b/tests/acpi-test.c
index 76fbccf..66cde26 100644
--- a/tests/acpi-test.c
+++ b/tests/acpi-test.c
@@ -18,6 +18,7 @@
 #include "libqtest.h"
 #include "qemu/compiler.h"
 #include "hw/i386/acpi-defs.h"
+#include "hw/i386/smbios.h"
 
 #define MACHINE_PC "pc"
 #define MACHINE_Q35 "q35"
@@ -46,6 +47,8 @@ typedef struct {
     uint32_t *rsdt_tables_addr;
     int rsdt_tables_nr;
     GArray *tables;
+    uint32_t smbios_ep_addr;
+    struct smbios_entry_point smbios_ep_table;
 } test_data;
 
 #define LOW(x) ((x) & 0xff)
@@ -220,6 +223,28 @@ static void test_acpi_rsdp_address(test_data *data)
     data->rsdp_addr = off;
 }
 
+static void test_smbios_ep_address(test_data *data)
+{
+    uint32_t off;
+
+    /* find smbios entry point structure */
+    for (off = 0xf0000; off < 0x100000; off += 0x10) {
+        uint8_t sig[] = "_SM_";
+        int i;
+
+        for (i = 0; i < sizeof sig - 1; ++i) {
+            sig[i] = readb(off + i);
+        }
+
+        if (!memcmp(sig, "_SM_", sizeof sig)) {
+            break;
+        }
+    }
+
+    g_assert_cmphex(off, <, 0x100000);
+    data->smbios_ep_addr = off;
+}
+
 static void test_acpi_rsdp_table(test_data *data)
 {
     AcpiRsdpDescriptor *rsdp_table = &data->rsdp_table;
@@ -238,6 +263,34 @@ static void test_acpi_rsdp_table(test_data *data)
     g_assert(!acpi_checksum((uint8_t *)rsdp_table, 20));
 }
 
+static void test_smbios_ep_table(test_data *data)
+{
+    struct smbios_entry_point *ep_table = &data->smbios_ep_table;
+    uint32_t addr = data->smbios_ep_addr;
+
+    ACPI_READ_ARRAY(ep_table->anchor_string, addr);
+    g_assert(!memcmp(ep_table->anchor_string, "_SM_", 4));
+    ACPI_READ_FIELD(ep_table->checksum, addr);
+    ACPI_READ_FIELD(ep_table->length, addr);
+    ACPI_READ_FIELD(ep_table->smbios_major_version, addr);
+    ACPI_READ_FIELD(ep_table->smbios_minor_version, addr);
+    ACPI_READ_FIELD(ep_table->max_structure_size, addr);
+    ACPI_READ_FIELD(ep_table->entry_point_revision, addr);
+    ACPI_READ_ARRAY(ep_table->formatted_area, addr);
+    ACPI_READ_ARRAY(ep_table->intermediate_anchor_string, addr);
+    g_assert(!memcmp(ep_table->intermediate_anchor_string, "_DMI_", 5));
+    ACPI_READ_FIELD(ep_table->intermediate_checksum, addr);
+    ACPI_READ_FIELD(ep_table->structure_table_length, addr);
+    g_assert_cmpuint(ep_table->structure_table_length, >, 0);
+    ACPI_READ_FIELD(ep_table->structure_table_address, addr);
+    ACPI_READ_FIELD(ep_table->number_of_structures, addr);
+    g_assert_cmpuint(ep_table->number_of_structures, >, 0);
+    ACPI_READ_FIELD(ep_table->smbios_bcd_revision, addr);
+    g_assert(!acpi_checksum((uint8_t *)ep_table, sizeof *ep_table));
+    g_assert(!acpi_checksum((uint8_t *)ep_table + 0x10,
+                            sizeof *ep_table - 0x10));
+}
+
 static void test_acpi_rsdt_table(test_data *data)
 {
     AcpiRsdtDescriptorRev1 *rsdt_table = &data->rsdt_table;
@@ -633,6 +686,9 @@ static void test_acpi_one(const char *params, test_data *data)
         }
     }
 
+    test_smbios_ep_address(data);
+    test_smbios_ep_table(data);
+
     qtest_quit(global_qtest);
     g_free(args);
 }

Re: [Qemu-devel] [PATCH v4 0/3] SMBIOS cleanup round

[Michael S. Tsirkin]

On Thu, May 22, 2014 at 09:27:11PM -0400, Gabriel L. Somlo wrote:
> Michael,
> 
> On Mon, May 19, 2014 at 10:44:48PM +0300, Michael S. Tsirkin wrote:
> > One question: we don't seem to have unit-test for this
> > interface in qemu, do we?
> > I would like to see at least a basic test along the lines of
> > tests/acpi-test.c: run bios to load tables from QEMU,
> > then do some basic checks on the tables that bios loaded.
> 
> So I took a closer look at tests/acpi-test.c today, and things slowly
> started to shift into focus :) So now I have two questions:
> 
> 1. There's a fairly complex setup (create a boot disk, start the
> guest, loop around waiting for the bios to finish booting, watch
> when your disk-based boot loader runs, etc.) before starting to
> examine the guest memory for the presence and correctness of the acpi
> tables.
> 
> Would it make sense to  rename this file to something like e.g.
> tests/biostables-test.c, and add checks for smbios to the already
> started and booted guest ?
> 
> If not, I'd have to replicate most of your test-harness code,
> which is almost half of acpi-test.c. That shouldn't be hard (you
> already did the heavy lifting on that one), but I intuitively dislike
> multiple cut'n'paste clones of significant code fragments :)

Sure, fine.

> 2. Assuming we can run smbios tests alongside acpi, what do you think
> of the patch below ? I've currently stopped after finding and checking
> the integrity of the smbios entry point structure. Not sure if I
> really need to walk the tables themselves, or what I'd be testing for
> if I did :)
> 
> Feedback/comments/flames welcome ! :)
> 
> Thanks much,
> --Gabriel

Looks good to me.

> 
> diff --git a/tests/acpi-test.c b/tests/acpi-test.c
> index 76fbccf..66cde26 100644
> --- a/tests/acpi-test.c
> +++ b/tests/acpi-test.c
> @@ -18,6 +18,7 @@
>  #include "libqtest.h"
>  #include "qemu/compiler.h"
>  #include "hw/i386/acpi-defs.h"
> +#include "hw/i386/smbios.h"
>  
>  #define MACHINE_PC "pc"
>  #define MACHINE_Q35 "q35"
> @@ -46,6 +47,8 @@ typedef struct {
>      uint32_t *rsdt_tables_addr;
>      int rsdt_tables_nr;
>      GArray *tables;
> +    uint32_t smbios_ep_addr;
> +    struct smbios_entry_point smbios_ep_table;
>  } test_data;
>  
>  #define LOW(x) ((x) & 0xff)
> @@ -220,6 +223,28 @@ static void test_acpi_rsdp_address(test_data *data)
>      data->rsdp_addr = off;
>  }
>  
> +static void test_smbios_ep_address(test_data *data)
> +{
> +    uint32_t off;
> +
> +    /* find smbios entry point structure */
> +    for (off = 0xf0000; off < 0x100000; off += 0x10) {
> +        uint8_t sig[] = "_SM_";
> +        int i;
> +
> +        for (i = 0; i < sizeof sig - 1; ++i) {
> +            sig[i] = readb(off + i);
> +        }
> +
> +        if (!memcmp(sig, "_SM_", sizeof sig)) {
> +            break;
> +        }
> +    }
> +
> +    g_assert_cmphex(off, <, 0x100000);
> +    data->smbios_ep_addr = off;
> +}
> +
>  static void test_acpi_rsdp_table(test_data *data)
>  {
>      AcpiRsdpDescriptor *rsdp_table = &data->rsdp_table;
> @@ -238,6 +263,34 @@ static void test_acpi_rsdp_table(test_data *data)
>      g_assert(!acpi_checksum((uint8_t *)rsdp_table, 20));
>  }
>  
> +static void test_smbios_ep_table(test_data *data)
> +{
> +    struct smbios_entry_point *ep_table = &data->smbios_ep_table;
> +    uint32_t addr = data->smbios_ep_addr;
> +
> +    ACPI_READ_ARRAY(ep_table->anchor_string, addr);
> +    g_assert(!memcmp(ep_table->anchor_string, "_SM_", 4));
> +    ACPI_READ_FIELD(ep_table->checksum, addr);
> +    ACPI_READ_FIELD(ep_table->length, addr);
> +    ACPI_READ_FIELD(ep_table->smbios_major_version, addr);
> +    ACPI_READ_FIELD(ep_table->smbios_minor_version, addr);
> +    ACPI_READ_FIELD(ep_table->max_structure_size, addr);
> +    ACPI_READ_FIELD(ep_table->entry_point_revision, addr);
> +    ACPI_READ_ARRAY(ep_table->formatted_area, addr);
> +    ACPI_READ_ARRAY(ep_table->intermediate_anchor_string, addr);
> +    g_assert(!memcmp(ep_table->intermediate_anchor_string, "_DMI_", 5));
> +    ACPI_READ_FIELD(ep_table->intermediate_checksum, addr);
> +    ACPI_READ_FIELD(ep_table->structure_table_length, addr);
> +    g_assert_cmpuint(ep_table->structure_table_length, >, 0);
> +    ACPI_READ_FIELD(ep_table->structure_table_address, addr);
> +    ACPI_READ_FIELD(ep_table->number_of_structures, addr);
> +    g_assert_cmpuint(ep_table->number_of_structures, >, 0);
> +    ACPI_READ_FIELD(ep_table->smbios_bcd_revision, addr);
> +    g_assert(!acpi_checksum((uint8_t *)ep_table, sizeof *ep_table));
> +    g_assert(!acpi_checksum((uint8_t *)ep_table + 0x10,
> +                            sizeof *ep_table - 0x10));
> +}
> +
>  static void test_acpi_rsdt_table(test_data *data)
>  {
>      AcpiRsdtDescriptorRev1 *rsdt_table = &data->rsdt_table;
> @@ -633,6 +686,9 @@ static void test_acpi_one(const char *params, test_data *data)
>          }
>      }
>  
> +    test_smbios_ep_address(data);
> +    test_smbios_ep_table(data);
> +
>      qtest_quit(global_qtest);
>      g_free(args);
>  }

Re: [Qemu-devel] [PATCH v4 0/3] SMBIOS cleanup round

[Gabriel L. Somlo]

On Fri, May 23, 2014 at 12:00:12PM +0300, Michael S. Tsirkin wrote:
> > 1. There's a fairly complex setup (create a boot disk, start the
> > guest, loop around waiting for the bios to finish booting, watch
> > when your disk-based boot loader runs, etc.) before starting to
> > examine the guest memory for the presence and correctness of the acpi
> > tables.
> > 
> > Would it make sense to  rename this file to something like e.g.
> > tests/biostables-test.c, and add checks for smbios to the already
> > started and booted guest ?
> > 
> > If not, I'd have to replicate most of your test-harness code,
> > which is almost half of acpi-test.c. That shouldn't be hard (you
> > already did the heavy lifting on that one), but I intuitively dislike
> > multiple cut'n'paste clones of significant code fragments :)
> 
> Sure, fine.

So I was about to send a patch with acpi-test.c renamed to
bios-tables-test.c, but the patch is basically removing all of
acpi-test.c, and creating a new file bios-tables-test.c.

Do you have a better way to rename the file first, and then I can
send a patch against it ? Or should we give up on renaming it
altogether ? Or should I just bite the bullet and cut'n'paste your
test harness into a new file specific to smbios ?

It's not particularly important to me which way we go -- I want to do
the right thing, whatever you decide that is :)

Thanks,
--Gabriel

> > 2. Assuming we can run smbios tests alongside acpi, what do you think
> > of the patch below ? I've currently stopped after finding and checking
> > the integrity of the smbios entry point structure. Not sure if I
> > really need to walk the tables themselves, or what I'd be testing for
> > if I did :)
> > 
> > Feedback/comments/flames welcome ! :)
> > 
> > Thanks much,
> > --Gabriel
> 
> Looks good to me.
> 
> > 
> > diff --git a/tests/acpi-test.c b/tests/acpi-test.c
> > index 76fbccf..66cde26 100644
> > --- a/tests/acpi-test.c
> > +++ b/tests/acpi-test.c
> > @@ -18,6 +18,7 @@
> >  #include "libqtest.h"
> >  #include "qemu/compiler.h"
> >  #include "hw/i386/acpi-defs.h"
> > +#include "hw/i386/smbios.h"
> >  
> >  #define MACHINE_PC "pc"
> >  #define MACHINE_Q35 "q35"
> > @@ -46,6 +47,8 @@ typedef struct {
> >      uint32_t *rsdt_tables_addr;
> >      int rsdt_tables_nr;
> >      GArray *tables;
> > +    uint32_t smbios_ep_addr;
> > +    struct smbios_entry_point smbios_ep_table;
> >  } test_data;
> >  
> >  #define LOW(x) ((x) & 0xff)
> > @@ -220,6 +223,28 @@ static void test_acpi_rsdp_address(test_data *data)
> >      data->rsdp_addr = off;
> >  }
> >  
> > +static void test_smbios_ep_address(test_data *data)
> > +{
> > +    uint32_t off;
> > +
> > +    /* find smbios entry point structure */
> > +    for (off = 0xf0000; off < 0x100000; off += 0x10) {
> > +        uint8_t sig[] = "_SM_";
> > +        int i;
> > +
> > +        for (i = 0; i < sizeof sig - 1; ++i) {
> > +            sig[i] = readb(off + i);
> > +        }
> > +
> > +        if (!memcmp(sig, "_SM_", sizeof sig)) {
> > +            break;
> > +        }
> > +    }
> > +
> > +    g_assert_cmphex(off, <, 0x100000);
> > +    data->smbios_ep_addr = off;
> > +}
> > +
> >  static void test_acpi_rsdp_table(test_data *data)
> >  {
> >      AcpiRsdpDescriptor *rsdp_table = &data->rsdp_table;
> > @@ -238,6 +263,34 @@ static void test_acpi_rsdp_table(test_data *data)
> >      g_assert(!acpi_checksum((uint8_t *)rsdp_table, 20));
> >  }
> >  
> > +static void test_smbios_ep_table(test_data *data)
> > +{
> > +    struct smbios_entry_point *ep_table = &data->smbios_ep_table;
> > +    uint32_t addr = data->smbios_ep_addr;
> > +
> > +    ACPI_READ_ARRAY(ep_table->anchor_string, addr);
> > +    g_assert(!memcmp(ep_table->anchor_string, "_SM_", 4));
> > +    ACPI_READ_FIELD(ep_table->checksum, addr);
> > +    ACPI_READ_FIELD(ep_table->length, addr);
> > +    ACPI_READ_FIELD(ep_table->smbios_major_version, addr);
> > +    ACPI_READ_FIELD(ep_table->smbios_minor_version, addr);
> > +    ACPI_READ_FIELD(ep_table->max_structure_size, addr);
> > +    ACPI_READ_FIELD(ep_table->entry_point_revision, addr);
> > +    ACPI_READ_ARRAY(ep_table->formatted_area, addr);
> > +    ACPI_READ_ARRAY(ep_table->intermediate_anchor_string, addr);
> > +    g_assert(!memcmp(ep_table->intermediate_anchor_string, "_DMI_", 5));
> > +    ACPI_READ_FIELD(ep_table->intermediate_checksum, addr);
> > +    ACPI_READ_FIELD(ep_table->structure_table_length, addr);
> > +    g_assert_cmpuint(ep_table->structure_table_length, >, 0);
> > +    ACPI_READ_FIELD(ep_table->structure_table_address, addr);
> > +    ACPI_READ_FIELD(ep_table->number_of_structures, addr);
> > +    g_assert_cmpuint(ep_table->number_of_structures, >, 0);
> > +    ACPI_READ_FIELD(ep_table->smbios_bcd_revision, addr);
> > +    g_assert(!acpi_checksum((uint8_t *)ep_table, sizeof *ep_table));
> > +    g_assert(!acpi_checksum((uint8_t *)ep_table + 0x10,
> > +                            sizeof *ep_table - 0x10));
> > +}
> > +
> >  static void test_acpi_rsdt_table(test_data *data)
> >  {
> >      AcpiRsdtDescriptorRev1 *rsdt_table = &data->rsdt_table;
> > @@ -633,6 +686,9 @@ static void test_acpi_one(const char *params, test_data *data)
> >          }
> >      }
> >  
> > +    test_smbios_ep_address(data);
> > +    test_smbios_ep_table(data);
> > +
> >      qtest_quit(global_qtest);
> >      g_free(args);
> >  }

-- 
------------------------------------
Gabriel L. Somlo
Director of Computing Services
Information Networking Institute
Carnegie Mellon University
4616 Henry St., Pittsburgh, PA 15213
+1.412.268.9310      www.ini.cmu.edu

Re: [Qemu-devel] [PATCH v4 0/3] SMBIOS cleanup round

[Markus Armbruster]

"Gabriel L. Somlo" <gsomlo@gmail.com> writes:

> On Fri, May 23, 2014 at 12:00:12PM +0300, Michael S. Tsirkin wrote:
>> > 1. There's a fairly complex setup (create a boot disk, start the
>> > guest, loop around waiting for the bios to finish booting, watch
>> > when your disk-based boot loader runs, etc.) before starting to
>> > examine the guest memory for the presence and correctness of the acpi
>> > tables.
>> > 
>> > Would it make sense to  rename this file to something like e.g.
>> > tests/biostables-test.c, and add checks for smbios to the already
>> > started and booted guest ?
>> > 
>> > If not, I'd have to replicate most of your test-harness code,
>> > which is almost half of acpi-test.c. That shouldn't be hard (you
>> > already did the heavy lifting on that one), but I intuitively dislike
>> > multiple cut'n'paste clones of significant code fragments :)
>> 
>> Sure, fine.
>
> So I was about to send a patch with acpi-test.c renamed to
> bios-tables-test.c, but the patch is basically removing all of
> acpi-test.c, and creating a new file bios-tables-test.c.

Err, isn't that what a rename does?

> Do you have a better way to rename the file first, and then I can
> send a patch against it ? Or should we give up on renaming it
> altogether ? Or should I just bite the bullet and cut'n'paste your
> test harness into a new file specific to smbios ?
>
> It's not particularly important to me which way we go -- I want to do
> the right thing, whatever you decide that is :)

Did you rename with git-mv?  Did you diff with rename detection on?  See
diff.renames in git-config(1).

[...]

Re: [Qemu-devel] [PATCH v4 0/3, ping] SMBIOS cleanup round

[Gabriel L. Somlo]

Hi,

On Mon, May 19, 2014 at 10:44:48PM +0300, Michael S. Tsirkin wrote:
> On Mon, May 19, 2014 at 10:09:52AM -0400, Gabriel L. Somlo wrote:
> > Changelog:
> > 
> > new in v4:
> >   - slightly improved commit blurbs
> > 
> > v3:
> >   - endian-ness fix (1/3) now correctly handles cpuid version and features
> >   - included documentation (qemu-options.hx) for type 0 uefi flag (2/3)
> >   - fixed type 17 field sizes ("word" means 16 bits, not 32 :) :)
> > 
> > 
> > v2:
> >   - 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!

http://patchwork.ozlabs.org/patch/350248/
http://patchwork.ozlabs.org/patch/350253/
http://patchwork.ozlabs.org/patch/350252/

Not sure where we left this, was somebody going to pull these, or are
we waiting for the unit tests:

> One question: we don't seem to have unit-test for this
> interface in qemu, do we?
> I would like to see at least a basic test along the lines of
> tests/acpi-test.c: run bios to load tables from QEMU,
> then do some basic checks on the tables that bios loaded.

... in which case:

http://patchwork.ozlabs.org/patch/353072/
http://patchwork.ozlabs.org/patch/353073/

Wondering if I should send out another series containing all five, or
if the first three are already "accepted", in which case "ping" only
on the latter two :)

Thanks much,
--Gabriel

Re: [Qemu-devel] [PATCH v4 0/3, ping] SMBIOS cleanup round

[Michael S. Tsirkin]

On Mon, Jun 02, 2014 at 01:13:18PM -0400, Gabriel L. Somlo wrote:
> Hi,
> 
> On Mon, May 19, 2014 at 10:44:48PM +0300, Michael S. Tsirkin wrote:
> > On Mon, May 19, 2014 at 10:09:52AM -0400, Gabriel L. Somlo wrote:
> > > Changelog:
> > > 
> > > new in v4:
> > >   - slightly improved commit blurbs
> > > 
> > > v3:
> > >   - endian-ness fix (1/3) now correctly handles cpuid version and features
> > >   - included documentation (qemu-options.hx) for type 0 uefi flag (2/3)
> > >   - fixed type 17 field sizes ("word" means 16 bits, not 32 :) :)
> > > 
> > > 
> > > v2:
> > >   - 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!
> 
> http://patchwork.ozlabs.org/patch/350248/
> http://patchwork.ozlabs.org/patch/350253/
> http://patchwork.ozlabs.org/patch/350252/
> 
> Not sure where we left this, was somebody going to pull these, or are
> we waiting for the unit tests:
> 
> > One question: we don't seem to have unit-test for this
> > interface in qemu, do we?
> > I would like to see at least a basic test along the lines of
> > tests/acpi-test.c: run bios to load tables from QEMU,
> > then do some basic checks on the tables that bios loaded.
> 
> ... in which case:
> 
> http://patchwork.ozlabs.org/patch/353072/
> http://patchwork.ozlabs.org/patch/353073/
> 
> Wondering if I should send out another series containing all five, or
> if the first three are already "accepted", in which case "ping" only
> on the latter two :)
> 
> Thanks much,
> --Gabriel

These are all in my tree.
I plan to send pull request Thursday.

-- 
MST

Re: [Qemu-devel] [PATCH v4 0/3, ping] SMBIOS cleanup round

[Michael S. Tsirkin]

On Mon, Jun 02, 2014 at 01:13:18PM -0400, Gabriel L. Somlo wrote:
> Hi,
> 
> On Mon, May 19, 2014 at 10:44:48PM +0300, Michael S. Tsirkin wrote:
> > On Mon, May 19, 2014 at 10:09:52AM -0400, Gabriel L. Somlo wrote:
> > > Changelog:
> > > 
> > > new in v4:
> > >   - slightly improved commit blurbs
> > > 
> > > v3:
> > >   - endian-ness fix (1/3) now correctly handles cpuid version and features
> > >   - included documentation (qemu-options.hx) for type 0 uefi flag (2/3)
> > >   - fixed type 17 field sizes ("word" means 16 bits, not 32 :) :)
> > > 
> > > 
> > > v2:
> > >   - 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!
> 
> http://patchwork.ozlabs.org/patch/350248/
> http://patchwork.ozlabs.org/patch/350253/
> http://patchwork.ozlabs.org/patch/350252/
> 
> Not sure where we left this, was somebody going to pull these, or are
> we waiting for the unit tests:
> 
> > One question: we don't seem to have unit-test for this
> > interface in qemu, do we?
> > I would like to see at least a basic test along the lines of
> > tests/acpi-test.c: run bios to load tables from QEMU,
> > then do some basic checks on the tables that bios loaded.
> 
> ... in which case:
> 
> http://patchwork.ozlabs.org/patch/353072/
> http://patchwork.ozlabs.org/patch/353073/
> 
> Wondering if I should send out another series containing all five, or
> if the first three are already "accepted", in which case "ping" only
> on the latter two :)
> 
> Thanks much,
> --Gabriel


Also pls confirm everything necessary is there in my tree:
git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream

Re: [Qemu-devel] [PATCH v4 0/3, ping] SMBIOS cleanup round

[Gabriel L. Somlo]

On Wed, Jun 04, 2014 at 10:47:21PM +0300, Michael S. Tsirkin wrote:
> On Mon, Jun 02, 2014 at 01:13:18PM -0400, Gabriel L. Somlo wrote:
> > Hi,
> > 
> > On Mon, May 19, 2014 at 10:44:48PM +0300, Michael S. Tsirkin wrote:
> > > On Mon, May 19, 2014 at 10:09:52AM -0400, Gabriel L. Somlo wrote:
> > > > Changelog:
> > > > 
> > > > new in v4:
> > > >   - slightly improved commit blurbs
> > > > 
> > > > v3:
> > > >   - endian-ness fix (1/3) now correctly handles cpuid version and features
> > > >   - included documentation (qemu-options.hx) for type 0 uefi flag (2/3)
> > > >   - fixed type 17 field sizes ("word" means 16 bits, not 32 :) :)
> > > > 
> > > > 
> > > > v2:
> > > >   - 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!
> > 
> > http://patchwork.ozlabs.org/patch/350248/
> > http://patchwork.ozlabs.org/patch/350253/
> > http://patchwork.ozlabs.org/patch/350252/
> > 
> > Not sure where we left this, was somebody going to pull these, or are
> > we waiting for the unit tests:
> > 
> > > One question: we don't seem to have unit-test for this
> > > interface in qemu, do we?
> > > I would like to see at least a basic test along the lines of
> > > tests/acpi-test.c: run bios to load tables from QEMU,
> > > then do some basic checks on the tables that bios loaded.
> > 
> > ... in which case:
> > 
> > http://patchwork.ozlabs.org/patch/353072/
> > http://patchwork.ozlabs.org/patch/353073/
> > 
> > Wondering if I should send out another series containing all five, or
> > if the first three are already "accepted", in which case "ping" only
> > on the latter two :)
> > 
> > Thanks much,
> > --Gabriel
> 
> 
> Also pls confirm everything necessary is there in my tree:
> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream

Looks good !

Thanks,
--Gabriel