[PATCH 0/3] hw/smbios: Add dedicated handle range for Type 8 tables

[PATCH 0/3] hw/smbios: Add dedicated handle range for Type 8 tables

[Alexander Gryanko]

This is a continuation of the work originally proposed by Felix Wu [1],
which changed the SMBIOS Type 8 handle base unconditionally. As Igor
Mammedov pointed out in review, an unconditional change would break
live migration between hosts running different QEMU versions.
This series addresses the review feedback by:
- Using machine-type versioning (pc-*-11.2) to opt in to the new
  behavior, preserving T0_BASE for older machine types
- Adding a PCMachineClass field to control the setting per machine type
- Adding tests for both new and compat machine versions
[1] https://lists.gnu.org/archive/html/qemu-devel/2024-01/msg03208.html

Alexander Gryanko (3):
  hw/i386/pc: Introduce 11.2 machine type for SMBIOS type 8 base
    migration
  hw/smbios: Add dedicated handle range for Type 8 tables
  tests/qtest: Add SMBIOS Type 8 handle base tests

 hw/core/machine.c              |  3 ++
 hw/i386/fw_cfg.c               |  1 +
 hw/i386/pc.c                   |  3 ++
 hw/i386/pc_piix.c              | 16 +++++++-
 hw/i386/pc_q35.c               | 16 +++++++-
 hw/smbios/smbios.c             | 10 ++++-
 include/hw/core/boards.h       |  3 ++
 include/hw/firmware/smbios.h   |  1 +
 include/hw/i386/pc.h           |  4 ++
 tests/qtest/bios-tables-test.c | 70 +++++++++++++++++++++++++++++++++-
 10 files changed, 121 insertions(+), 6 deletions(-)

-- 
2.39.5 (Apple Git-154)

[PATCH 1/3] hw/i386/pc: Introduce 11.2 machine type for SMBIOS type 8 base migration

[Alexander Gryanko]

SMBIOS Type 8 (Port Connector) tables have been using T0_BASE (0x0) for
handle allocation, which can collide with handles assigned to other
SMBIOS types. Types 2, 3, 4, 9, and 11 already use dedicated ranges
(T2_BASE=0x200, T3_BASE=0x300, etc.).
Introduce pc-q35-11.2 and pc-i440fx-11.2 with smbios_type8_handle_t8_base
enabled, so Type 8 handles start at 0x800. The 11.1 machine types retain
T0_BASE for migration compatibility.
Other platforms (ARM, LoongArch, RISC-V) are not changed by this patch.

Signed-off-by: Alexander Gryanko <xpahos@gmail.com>
---
 hw/core/machine.c        |  3 +++
 hw/i386/pc.c             |  3 +++
 hw/i386/pc_piix.c        | 16 ++++++++++++++--
 hw/i386/pc_q35.c         | 16 ++++++++++++++--
 include/hw/core/boards.h |  3 +++
 include/hw/i386/pc.h     |  4 ++++
 6 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 63baff859f..96de2d20f0 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -39,6 +39,9 @@
 #include "hw/acpi/generic_event_device.h"
 #include "qemu/audio.h"
 
+GlobalProperty hw_compat_11_1[] = {};
+const size_t hw_compat_11_1_len = G_N_ELEMENTS(hw_compat_11_1);
+
 GlobalProperty hw_compat_11_0[] = {
     { "chardev-vc", "encoding", "cp437" },
 };
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 2ecad3c503..17ad8b6489 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -73,6 +73,9 @@
 #include "hw/xen/xen-bus.h"
 #endif
 
+GlobalProperty pc_compat_11_1[] = {};
+const size_t pc_compat_11_1_len = G_N_ELEMENTS(pc_compat_11_1);
+
 GlobalProperty pc_compat_11_0[] = {};
 const size_t pc_compat_11_0_len = G_N_ELEMENTS(pc_compat_11_0);
 
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 82457bdb16..27e1d552f6 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -406,6 +406,7 @@ static void pc_i440fx_machine_options(MachineClass *m)
     pcmc->default_south_bridge = TYPE_PIIX3_DEVICE;
     pcmc->pci_root_uid = 0;
     pcmc->default_cpu_version = 1;
+    pcmc->smbios_type8_handle_t8_base = true;
 
     m->family = "pc_piix";
     m->desc = "Standard PC (i440FX + PIIX, 1996)";
@@ -428,12 +429,23 @@ static void pc_i440fx_machine_options(MachineClass *m)
                      pc_piix_compat_defaults, pc_piix_compat_defaults_len);
 }
 
-static void pc_i440fx_machine_11_1_options(MachineClass *m)
+static void pc_i440fx_machine_11_2_options(MachineClass *m)
 {
     pc_i440fx_machine_options(m);
 }
 
-DEFINE_I440FX_MACHINE_AS_LATEST(11, 1);
+DEFINE_I440FX_MACHINE_AS_LATEST(11, 2);
+
+static void pc_i440fx_machine_11_1_options(MachineClass *m)
+{
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
+    pc_i440fx_machine_11_2_options(m);
+    pcmc->smbios_type8_handle_t8_base = false;
+    compat_props_add(m->compat_props, hw_compat_11_1, hw_compat_11_1_len);
+    compat_props_add(m->compat_props, pc_compat_11_1, pc_compat_11_1_len);
+}
+
+DEFINE_I440FX_MACHINE(11, 1);
 
 static void pc_i440fx_machine_11_0_options(MachineClass *m)
 {
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index d8fed698c7..cf1ae9b78b 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -345,6 +345,7 @@ static void pc_q35_machine_options(MachineClass *m)
     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pcmc->pci_root_uid = 0;
     pcmc->default_cpu_version = 1;
+    pcmc->smbios_type8_handle_t8_base = true;
 
     m->family = "pc_q35";
     m->desc = "Standard PC (Q35 + ICH9, 2009)";
@@ -365,12 +366,23 @@ static void pc_q35_machine_options(MachineClass *m)
                      pc_q35_compat_defaults, pc_q35_compat_defaults_len);
 }
 
-static void pc_q35_machine_11_1_options(MachineClass *m)
+static void pc_q35_machine_11_2_options(MachineClass *m)
 {
     pc_q35_machine_options(m);
 }
 
-DEFINE_Q35_MACHINE_AS_LATEST(11, 1);
+DEFINE_Q35_MACHINE_AS_LATEST(11, 2);
+
+static void pc_q35_machine_11_1_options(MachineClass *m)
+{
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
+    pc_q35_machine_11_2_options(m);
+    pcmc->smbios_type8_handle_t8_base = false;
+    compat_props_add(m->compat_props, hw_compat_11_1, hw_compat_11_1_len);
+    compat_props_add(m->compat_props, pc_compat_11_1, pc_compat_11_1_len);
+}
+
+DEFINE_Q35_MACHINE(11, 1);
 
 static void pc_q35_machine_11_0_options(MachineClass *m)
 {
diff --git a/include/hw/core/boards.h b/include/hw/core/boards.h
index 29c68931d8..a436d48c8e 100644
--- a/include/hw/core/boards.h
+++ b/include/hw/core/boards.h
@@ -815,6 +815,9 @@ compat_props_add(GPtrArray *arr,
     }
 }
 
+extern GlobalProperty hw_compat_11_1[];
+extern const size_t hw_compat_11_1_len;
+
 extern GlobalProperty hw_compat_11_0[];
 extern const size_t hw_compat_11_0_len;
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 85a74363b5..b7d21fb96e 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -102,6 +102,7 @@ struct PCMachineClass {
     /* SMBIOS compat: */
     bool smbios_defaults;
     bool smbios_legacy_mode;
+    bool smbios_type8_handle_t8_base;
     SmbiosEntryPointType default_smbios_ep_type;
 
     /* RAM / address space compat: */
@@ -208,6 +209,9 @@ void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t flash_size);
 /* sgx.c */
 void pc_machine_init_sgx_epc(PCMachineState *pcms);
 
+extern GlobalProperty pc_compat_11_1[];
+extern const size_t pc_compat_11_1_len;
+
 extern GlobalProperty pc_compat_11_0[];
 extern const size_t pc_compat_11_0_len;
 
-- 
2.39.5 (Apple Git-154)

[PATCH 2/3] hw/smbios: Add dedicated handle range for Type 8 tables

[Alexander Gryanko]

SMBIOS Type 8 (Port Connector) tables currently use T0_BASE (0x0) for
handle allocation. This can lead to handle collisions with other table
types that also start at T0_BASE. Types 2, 3, 4, 9, and 11 already
have dedicated handle ranges (T2_BASE=0x200, T3_BASE=0x300, T4_BASE=0x400,
T9_BASE=0x900, T11_BASE=0xe00), but Type 8 was missed.
Add T8_BASE (0x800) to fill the gap in the handle range allocation.
Introduce a static flag and smbios_set_type8_handle_t8_base() setter
to allow machine types to opt in to the new range. When the flag is
false (the default), the old T0_BASE behavior is preserved.

Signed-off-by: Alexander Gryanko <xpahos@gmail.com>
---
 hw/i386/fw_cfg.c             |  1 +
 hw/smbios/smbios.c           | 10 +++++++++-
 include/hw/firmware/smbios.h |  1 +
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
index 2876490f06..0b3a14cdb1 100644
--- a/hw/i386/fw_cfg.c
+++ b/hw/i386/fw_cfg.c
@@ -77,6 +77,7 @@ void fw_cfg_build_smbios(PCMachineState *pcms, FWCfgState *fw_cfg,
         /* These values are guest ABI, do not change */
         smbios_set_defaults("QEMU", mc->desc, mc->name);
     }
+    smbios_set_type8_handle_t8_base(pcmc->smbios_type8_handle_t8_base);
 
     /* tell smbios about cpuid version and features */
     smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]);
diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 7d7141851b..0e0200600e 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -49,6 +49,7 @@ static SmbiosEntryPoint ep;
 static int smbios_type4_count = 0;
 static bool smbios_have_defaults;
 static uint32_t smbios_cpuid_version, smbios_cpuid_features;
+static bool smbios_type8_t8_base;
 
 DECLARE_BITMAP(smbios_have_binfile_bitmap, SMBIOS_MAX_TYPE + 1);
 DECLARE_BITMAP(smbios_have_fields_bitmap, SMBIOS_MAX_TYPE + 1);
@@ -553,6 +554,7 @@ bool smbios_skip_table(uint8_t type, bool required_table)
 #define T2_BASE 0x200
 #define T3_BASE 0x300
 #define T4_BASE 0x400
+#define T8_BASE 0x800
 #define T9_BASE 0x900
 #define T11_BASE 0xe00
 
@@ -742,13 +744,19 @@ static void smbios_build_type_4_table(MachineState *ms, unsigned instance,
     smbios_type4_count++;
 }
 
+void smbios_set_type8_handle_t8_base(bool t8_base)
+{
+    smbios_type8_t8_base = t8_base;
+}
+
 static void smbios_build_type_8_table(void)
 {
     unsigned instance = 0;
     struct type8_instance *t8;
+    uint16_t base = smbios_type8_t8_base ? T8_BASE : T0_BASE;
 
     QTAILQ_FOREACH(t8, &type8, next) {
-        SMBIOS_BUILD_TABLE_PRE(8, T0_BASE + instance, true);
+        SMBIOS_BUILD_TABLE_PRE(8, base + instance, true);
 
         SMBIOS_TABLE_SET_STR(8, internal_reference_str, t8->internal_reference);
         SMBIOS_TABLE_SET_STR(8, external_reference_str, t8->external_reference);
diff --git a/include/hw/firmware/smbios.h b/include/hw/firmware/smbios.h
index 3ea732f4e6..62e728b69c 100644
--- a/include/hw/firmware/smbios.h
+++ b/include/hw/firmware/smbios.h
@@ -333,6 +333,7 @@ void smbios_set_cpuid(uint32_t version, uint32_t features);
 void smbios_set_defaults(const char *manufacturer, const char *product,
                          const char *version);
 void smbios_set_default_processor_family(uint16_t processor_family);
+void smbios_set_type8_handle_t8_base(bool t8_base);
 uint8_t *smbios_get_table_legacy(size_t *length, Error **errp);
 void smbios_get_tables(MachineState *ms,
                        SmbiosEntryPointType ep_type,
-- 
2.39.5 (Apple Git-154)

[PATCH 3/3] tests/qtest: Add SMBIOS Type 8 handle base tests

[Alexander Gryanko]

Verify that SMBIOS Type 8 table handles use the correct base address
depending on the machine type version:
- pc-q35-11.2 and pc-i440fx-11.2 (latest): handles start at 0x800
- pc-q35-11.1 and pc-i440fx-11.1 (compat): handles start at 0x0
Each test creates two Type 8 entries to verify both the base address
and sequential handle assignment (base+0, base+1).

Signed-off-by: Alexander Gryanko <xpahos@gmail.com>
---
 tests/qtest/bios-tables-test.c | 70 +++++++++++++++++++++++++++++++++-
 1 file changed, 69 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 510751799e..66d6015667 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -99,6 +99,7 @@ typedef struct {
     uint16_t smbios_core_count2;
     uint8_t smbios_thread_count;
     uint16_t smbios_thread_count2;
+    bool smbios_type8_t8_base;
     uint8_t *required_struct_types;
     int required_struct_types_len;
     int type4_count;
@@ -720,12 +721,22 @@ static void smbios_type4_count_test(test_data *data, int type4_count)
     }
 }
 
+static void smbios_type8_test(test_data *data, uint32_t addr,
+                              int instance)
+{
+    uint16_t handle = qtest_readw(data->qts,
+        addr + offsetof(struct smbios_structure_header, handle));
+    uint16_t expected_base = data->smbios_type8_t8_base ? 0x800 : 0x0;
+
+    g_assert_cmpuint(handle, ==, expected_base + instance);
+}
+
 static void test_smbios_structs(test_data *data, SmbiosEntryPointType ep_type)
 {
     DECLARE_BITMAP(struct_bitmap, SMBIOS_MAX_TYPE+1) = { 0 };
 
     SmbiosEntryPoint *ep_table = &data->smbios_ep_table;
-    int i = 0, len, max_len = 0, type4_count = 0;
+    int i = 0, len, max_len = 0, type4_count = 0, type8_count = 0;
     uint8_t type, prv, crt;
     uint64_t addr;
 
@@ -754,6 +765,11 @@ static void test_smbios_structs(test_data *data, SmbiosEntryPointType ep_type)
             type4_count++;
         }
 
+        if (type == 8) {
+            smbios_type8_test(data, addr, type8_count);
+            type8_count++;
+        }
+
         /* seek to end of unformatted string area of this struct ("\0\0") */
         prv = crt = 1;
         while (prv || crt) {
@@ -2534,6 +2550,50 @@ static void test_acpi_isapc_smbios_legacy(void)
     free_test_data(&data);
 }
 
+static void test_smbios_type8_common(const char *machine,
+                                     const char *variant, bool t8_base)
+{
+    uint8_t req_type8[] = { 8 };
+    test_data data = {
+        .machine = machine,
+        .arch    = "x86",
+        .variant = variant,
+        .required_struct_types = req_type8,
+        .required_struct_types_len = ARRAY_SIZE(req_type8),
+        .smbios_type8_t8_base = t8_base,
+    };
+
+    test_smbios("-smbios type=8,connector_type=7,port_type=8,"
+                "external_reference=USB1 "
+                "-smbios type=8,connector_type=8,port_type=9,"
+                "external_reference=USB2", &data);
+    free_test_data(&data);
+}
+
+static void test_acpi_q35_smbios_type8(void)
+{
+    test_smbios_type8_common(MACHINE_Q35,
+        ".q35_smbios_type8", true);
+}
+
+static void test_acpi_q35_smbios_type8_compat(void)
+{
+    test_smbios_type8_common("pc-q35-11.1",
+        ".q35_smbios_type8_compat", false);
+}
+
+static void test_acpi_pc_smbios_type8(void)
+{
+    test_smbios_type8_common(MACHINE_PC,
+        ".pc_smbios_type8", true);
+}
+
+static void test_acpi_pc_smbios_type8_compat(void)
+{
+    test_smbios_type8_common("pc-i440fx-11.1",
+        ".pc_smbios_type8_compat", false);
+}
+
 static void test_oem_fields(test_data *data)
 {
     int i;
@@ -2760,6 +2820,10 @@ int main(int argc, char *argv[])
                            test_acpi_pc_smbios_options);
             qtest_add_func("acpi/piix4/smbios-blob",
                            test_acpi_pc_smbios_blob);
+            qtest_add_func("acpi/piix4/smbios-type8",
+                           test_acpi_pc_smbios_type8);
+            qtest_add_func("acpi/piix4/smbios-type8-compat",
+                           test_acpi_pc_smbios_type8_compat);
             qtest_add_func("acpi/piix4/smbios-legacy",
                            test_acpi_isapc_smbios_legacy);
         }
@@ -2828,6 +2892,10 @@ int main(int argc, char *argv[])
             qtest_add_func("acpi/q35/cxl", test_acpi_q35_cxl);
 #endif
             qtest_add_func("acpi/q35/slic", test_acpi_q35_slic);
+            qtest_add_func("acpi/q35/smbios-type8",
+                test_acpi_q35_smbios_type8);
+            qtest_add_func("acpi/q35/smbios-type8-compat",
+                test_acpi_q35_smbios_type8_compat);
         }
         if (qtest_has_machine("microvm")) {
             qtest_add_func("acpi/microvm", test_acpi_microvm_tcg);
-- 
2.39.5 (Apple Git-154)