On Thu, Jan 23, 2025 at 1:56 PM Zhao Liu <[email protected]> wrote:
>
> 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.
yeah nothing that I did try was working except when I was checking
explicitly if a header has been included in that path already.
>
> 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.
yeah this is something I wondered but did not try in the end.
>
> 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.
yes it works. Thanks for the suggestion. Pipeline passes.
>
> Regards,
> Zhao
>
>
