Re: [PATCH v4] hw/i386/cpu: remove default_cpu_version and simplify

[Zhao Liu <zhao1.liu@intel.com>]

Hi Ani,

[snip]

> --- a/include/hw/i386/x86.h
> +++ b/include/hw/i386/x86.h
> @@ -114,7 +114,10 @@ void init_topo_info(X86CPUTopoInfo *topo_info, const X86MachineState *x86ms);
>  uint32_t x86_cpu_apic_id_from_index(X86MachineState *x86ms,
>                                      unsigned int cpu_index);
>  
> -void x86_cpus_init(X86MachineState *pcms, int default_cpu_version);
> +void x86_cpus_init(X86MachineState *pcms);
> +#ifndef I386_CPU_H
> +void x86_cpu_set_legacy_version(void);
> +#endif
>  void x86_rtc_set_cpus_count(ISADevice *rtc, uint16_t cpus_count);
>  void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
>                        DeviceState *dev, Error **errp);

[snip]

> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h

...

> +#ifndef HW_I386_X86_H
> +void x86_cpu_set_legacy_version(void);
> +#endif
>  #ifndef CONFIG_USER_ONLY
>  
>  void do_cpu_sipi(X86CPU *cpu);

I see your problem: Due to the complex reference relationships, you have
to check the header files and make declarations twice.

What about the following solution?

 * Declare pc_init_cpus() in pc.h and define it in pc.c (including cpu.h
   in pc.c).
 * Only declare x86_cpu_set_legacy_version() in cpu.h.

An example would be like:

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index b46975c8a4db..9e8b5fa33596 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -61,6 +61,7 @@
 #include "hw/i386/kvm/xen_xenstore.h"
 #include "hw/mem/memory-device.h"
 #include "e820_memory_layout.h"
+#include "target/i386/cpu.h"
 #include "trace.h"
 #include "sev.h"
 #include CONFIG_DEVICES
@@ -615,6 +616,19 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }

+void pc_init_cpus(MachineState *ms)
+{
+    X86MachineState *x86ms = X86_MACHINE(ms);
+    PCMachineState *pcms = PC_MACHINE(ms);
+    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+
+    if (pcmc->no_versioned_cpu_model) {
+        /* use legacy cpu as it does not support versions */
+        x86_cpu_set_legacy_version();
+    }
+    x86_cpus_init(x86ms);
+}
+
 static
 void pc_machine_done(Notifier *notifier, void *data)
 {
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 34106bb52db8..dc0ca5bcb2a5 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -130,19 +130,6 @@ struct PCMachineClass {
 #define TYPE_PC_MACHINE "generic-pc-machine"
 OBJECT_DECLARE_TYPE(PCMachineState, PCMachineClass, PC_MACHINE)

-static inline void pc_init_cpus(MachineState *ms)
-{
-    X86MachineState *x86ms = X86_MACHINE(ms);
-    PCMachineState *pcms = PC_MACHINE(ms);
-    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
-
-    if (pcmc->no_versioned_cpu_model) {
-        /* use legacy cpu as it does not support versions */
-        x86_cpu_set_legacy_version();
-    }
-    x86_cpus_init(x86ms);
-}
-
 /* ioapic.c */

 GSIState *pc_gsi_create(qemu_irq **irqs, bool pci_enabled);
@@ -150,6 +137,7 @@ GSIState *pc_gsi_create(qemu_irq **irqs, bool pci_enabled);
 /* pc.c */

 void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
+void pc_init_cpus(MachineState *ms);

 #define PCI_HOST_PROP_RAM_MEM          "ram-mem"
 #define PCI_HOST_PROP_PCI_MEM          "pci-mem"
diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index 41236d2a8081..2d2b987fa135 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -115,9 +115,6 @@ uint32_t x86_cpu_apic_id_from_index(X86MachineState *x86ms,
                                     unsigned int cpu_index);

 void x86_cpus_init(X86MachineState *pcms);
-#ifndef I386_CPU_H
-void x86_cpu_set_legacy_version(void);
-#endif
 void x86_rtc_set_cpus_count(ISADevice *rtc, uint16_t cpus_count);
 void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
                       DeviceState *dev, Error **errp);
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 9c0ce2adafe7..bdbe54b26f60 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2686,10 +2686,8 @@ typedef int X86CPUVersion;
  * Currently, this is only used by microvm.
  */
 void x86_cpu_uses_lastest_version(void);
-
-#ifndef HW_I386_X86_H
 void x86_cpu_set_legacy_version(void);
-#endif
+
 #ifndef CONFIG_USER_ONLY

 void do_cpu_sipi(X86CPU *cpu);

---

The change can be applied on this patch I think. If you like this
approach, you can try if the pipeline is happy with it.

Regards,
Zhao