diff mbox

[RFC,15/16] smbios: don't use smp_cores, smp_threads

Message ID 1465580427-13596-16-git-send-email-drjones@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andrew Jones June 10, 2016, 5:40 p.m. UTC
SMBIOS needs cpu topology for Type4 tables, so we need to pass
it in. There are several parameters so we use a structure. There
are two callers (of non-legacy, which generates Type4 tables),
x86 and arm, so we also update both to pass the topology
parameters from their MachineState properties (directly in the
case of x86, indirectly through VirtGuestInfo in the case of arm).

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 hw/arm/virt.c              |  9 ++++++++-
 hw/i386/pc.c               | 13 ++++++++++---
 hw/smbios/smbios.c         | 20 +++++++++++---------
 include/hw/smbios/smbios.h | 10 ++++++++++
 4 files changed, 39 insertions(+), 13 deletions(-)

Comments

Eduardo Habkost July 14, 2016, 8:51 p.m. UTC | #1
On Fri, Jun 10, 2016 at 07:40:26PM +0200, Andrew Jones wrote:
> SMBIOS needs cpu topology for Type4 tables, so we need to pass
> it in. There are several parameters so we use a structure. There
> are two callers (of non-legacy, which generates Type4 tables),
> x86 and arm, so we also update both to pass the topology
> parameters from their MachineState properties (directly in the
> case of x86, indirectly through VirtGuestInfo in the case of arm).
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
[...]
> @@ -701,12 +701,19 @@ static uint32_t x86_cpu_apic_id_from_index(MachineState *ms,
>      }
>  }
>  
> -static void pc_build_smbios(FWCfgState *fw_cfg)
> +static void pc_build_smbios(MachineState *ms, FWCfgState *fw_cfg)
>  {
>      uint8_t *smbios_tables, *smbios_anchor;
>      size_t smbios_tables_len, smbios_anchor_len;
>      struct smbios_phys_mem_area *mem_array;
>      unsigned i, array_count;
> +    struct smbios_cpu_topology topo = {
> +        .sockets = ms->sockets,
> +        .cores   = ms->cores,
> +        .threads = ms->threads,
> +        .maxcpus = ms->maxcpus,
> +        .cpus    = ms->cpus,
> +    };
>  

Do we really need to manually copy data around this way when
smbios.c could just use qdev_get_machine() directly?
Andrew Jones July 15, 2016, 6:45 a.m. UTC | #2
On Thu, Jul 14, 2016 at 05:51:19PM -0300, Eduardo Habkost wrote:
> On Fri, Jun 10, 2016 at 07:40:26PM +0200, Andrew Jones wrote:
> > SMBIOS needs cpu topology for Type4 tables, so we need to pass
> > it in. There are several parameters so we use a structure. There
> > are two callers (of non-legacy, which generates Type4 tables),
> > x86 and arm, so we also update both to pass the topology
> > parameters from their MachineState properties (directly in the
> > case of x86, indirectly through VirtGuestInfo in the case of arm).
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> [...]
> > @@ -701,12 +701,19 @@ static uint32_t x86_cpu_apic_id_from_index(MachineState *ms,
> >      }
> >  }
> >  
> > -static void pc_build_smbios(FWCfgState *fw_cfg)
> > +static void pc_build_smbios(MachineState *ms, FWCfgState *fw_cfg)
> >  {
> >      uint8_t *smbios_tables, *smbios_anchor;
> >      size_t smbios_tables_len, smbios_anchor_len;
> >      struct smbios_phys_mem_area *mem_array;
> >      unsigned i, array_count;
> > +    struct smbios_cpu_topology topo = {
> > +        .sockets = ms->sockets,
> > +        .cores   = ms->cores,
> > +        .threads = ms->threads,
> > +        .maxcpus = ms->maxcpus,
> > +        .cpus    = ms->cpus,
> > +    };
> >  
> 
> Do we really need to manually copy data around this way when
> smbios.c could just use qdev_get_machine() directly?

You tell me. The art of writing object-oriented-ish code in C with
qom and qdev and a pile of qemu-best-practices is still something
I'm trying to sort out :-)

Using qdev_get_machine() sounds good to me though.

drew

> 
> -- 
> Eduardo
>
diff mbox

Patch

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 769a49aa5be77..4482fab91c139 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1067,6 +1067,13 @@  static void virt_build_smbios(VirtGuestInfo *guest_info)
     uint8_t *smbios_tables, *smbios_anchor;
     size_t smbios_tables_len, smbios_anchor_len;
     const char *product = "QEMU Virtual Machine";
+    struct smbios_cpu_topology topo = {
+        .sockets = guest_info->sockets,
+        .cores   = guest_info->cores,
+        .threads = guest_info->threads,
+        .maxcpus = guest_info->maxcpus,
+        .cpus    = guest_info->cpus,
+    };
 
     if (!fw_cfg) {
         return;
@@ -1079,7 +1086,7 @@  static void virt_build_smbios(VirtGuestInfo *guest_info)
     smbios_set_defaults("QEMU", product,
                         "1.0", false, true, SMBIOS_ENTRY_POINT_30);
 
-    smbios_get_tables(NULL, 0, &smbios_tables, &smbios_tables_len,
+    smbios_get_tables(NULL, 0, &topo, &smbios_tables, &smbios_tables_len,
                       &smbios_anchor, &smbios_anchor_len);
 
     if (smbios_anchor) {
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 4fa86d6387ce9..afea1a535a653 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -701,12 +701,19 @@  static uint32_t x86_cpu_apic_id_from_index(MachineState *ms,
     }
 }
 
-static void pc_build_smbios(FWCfgState *fw_cfg)
+static void pc_build_smbios(MachineState *ms, FWCfgState *fw_cfg)
 {
     uint8_t *smbios_tables, *smbios_anchor;
     size_t smbios_tables_len, smbios_anchor_len;
     struct smbios_phys_mem_area *mem_array;
     unsigned i, array_count;
+    struct smbios_cpu_topology topo = {
+        .sockets = ms->sockets,
+        .cores   = ms->cores,
+        .threads = ms->threads,
+        .maxcpus = ms->maxcpus,
+        .cpus    = ms->cpus,
+    };
 
     smbios_tables = smbios_get_table_legacy(&smbios_tables_len);
     if (smbios_tables) {
@@ -725,7 +732,7 @@  static void pc_build_smbios(FWCfgState *fw_cfg)
             array_count++;
         }
     }
-    smbios_get_tables(mem_array, array_count,
+    smbios_get_tables(mem_array, array_count, &topo,
                       &smbios_tables, &smbios_tables_len,
                       &smbios_anchor, &smbios_anchor_len);
     g_free(mem_array);
@@ -1176,7 +1183,7 @@  void pc_machine_done(Notifier *notifier, void *data)
 
     acpi_setup();
     if (pcms->fw_cfg) {
-        pc_build_smbios(pcms->fw_cfg);
+        pc_build_smbios(MACHINE(pcms), pcms->fw_cfg);
     }
 }
 
diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index cf18ecfd8599c..99b5f984b945a 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -20,7 +20,6 @@ 
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
-#include "sysemu/cpus.h"
 #include "hw/smbios/smbios.h"
 #include "hw/loader.h"
 #include "exec/cpu-common.h"
@@ -64,7 +63,9 @@  static SmbiosEntryPoint ep;
 static int smbios_type4_count = 0;
 static bool smbios_immutable;
 static bool smbios_have_defaults;
-static uint32_t smbios_cpuid_version, smbios_cpuid_features, smbios_smp_sockets;
+static uint32_t smbios_cpuid_version, smbios_cpuid_features;
+
+static struct smbios_cpu_topology smbios_cpu_topology;
 
 static DECLARE_BITMAP(have_binfile_bitmap, SMBIOS_MAX_TYPE+1);
 static DECLARE_BITMAP(have_fields_bitmap, SMBIOS_MAX_TYPE+1);
@@ -325,7 +326,8 @@  opts_init(smbios_register_config);
 
 static void smbios_validate_table(void)
 {
-    uint32_t expect_t4_count = smbios_legacy ? smp_cpus : smbios_smp_sockets;
+    uint32_t expect_t4_count = smbios_legacy ? smp_cpus
+                                             : smbios_cpu_topology.sockets;
 
     if (smbios_type4_count && smbios_type4_count != expect_t4_count) {
         error_report("Expected %d SMBIOS Type 4 tables, got %d instead",
@@ -637,8 +639,8 @@  static void smbios_build_type_4_table(unsigned instance)
     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->core_count = t->core_enabled = smbios_cpu_topology.cores;
+    t->thread_count = smbios_cpu_topology.threads;
     t->processor_characteristics = cpu_to_le16(0x02); /* Unknown */
     t->processor_family2 = cpu_to_le16(0x01); /* Other */
 
@@ -864,6 +866,7 @@  static void smbios_entry_point_setup(void)
 
 void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
                        const unsigned int mem_array_size,
+                       const struct smbios_cpu_topology *topo,
                        uint8_t **tables, size_t *tables_len,
                        uint8_t **anchor, size_t *anchor_len)
 {
@@ -875,16 +878,15 @@  void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
         return;
     }
 
+    smbios_cpu_topology = *topo;
+
     if (!smbios_immutable) {
         smbios_build_type_0_table();
         smbios_build_type_1_table();
         smbios_build_type_2_table();
         smbios_build_type_3_table();
 
-        smbios_smp_sockets = DIV_ROUND_UP(max_cpus, smp_cores * smp_threads);
-        assert(smbios_smp_sockets >= 1);
-
-        for (i = 0; i < smbios_smp_sockets; i++) {
+        for (i = 0; i < smbios_cpu_topology.sockets; i++) {
             smbios_build_type_4_table(i);
         }
 
diff --git a/include/hw/smbios/smbios.h b/include/hw/smbios/smbios.h
index ba3674609edf4..64f41ee9e2605 100644
--- a/include/hw/smbios/smbios.h
+++ b/include/hw/smbios/smbios.h
@@ -23,6 +23,15 @@  struct smbios_phys_mem_area {
     uint64_t length;
 };
 
+/* cpu topology, used by type 4 table */
+struct smbios_cpu_topology {
+    int sockets;
+    int cores;
+    int threads;
+    int maxcpus;
+    int cpus;
+};
+
 /*
  * SMBIOS spec defined tables
  */
@@ -264,6 +273,7 @@  void smbios_set_defaults(const char *manufacturer, const char *product,
 uint8_t *smbios_get_table_legacy(size_t *length);
 void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
                        const unsigned int mem_array_size,
+                       const struct smbios_cpu_topology *topo,
                        uint8_t **tables, size_t *tables_len,
                        uint8_t **anchor, size_t *anchor_len);
 #endif /*QEMU_SMBIOS_H */