Re: [Qemu-devel] [RESEND PATCH v3] i386: keep cpu_model field in MachineState uptodate

[Marcel Apfelbaum <marcel.apfelbaum@gmail.com>]

On 10/15/2015 06:12 AM, Zhu Guihua wrote:
> Update cpu_model in MachineState for i386, so that the field can be used
> for cpu hotplug, instead of using a static variable.
>
> This patch is rebased on the latest master.
>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> v3:
>   -use PCMachineState in pc_cpus_init() instead MachineState
>
> v2:
>   -transfer MachineState from all pc_cpus_init() callers
> ---
>   hw/i386/pc.c         | 17 ++++++++---------
>   hw/i386/pc_piix.c    |  2 +-
>   hw/i386/pc_q35.c     |  2 +-
>   include/hw/i386/pc.h |  2 +-
>   4 files changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 682867a..208f553 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1077,11 +1077,10 @@ out:
>       return cpu;
>   }
>
> -static const char *current_cpu_model;
> -
>   void pc_hot_add_cpu(const int64_t id, Error **errp)
>   {
>       X86CPU *cpu;
> +    MachineState *machine = MACHINE(qdev_get_machine());
>       int64_t apic_id = x86_cpu_apic_id_from_index(id);
>       Error *local_err = NULL;
>
> @@ -1109,7 +1108,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
>           return;
>       }
>
> -    cpu = pc_new_cpu(current_cpu_model, apic_id, &local_err);
> +    cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err);t

Hi,

I am not going to "stop" this patch and I do agree with what is trying to do.
What I still don't get is if we are "allowed" to directly access QOM object's private
fields outside the implementation C file.

This is why we have some wrappers in include/hw/boards.h when we access machine's fields.

Just wanted to raise the question, other than that (for what is worth):
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel



>       if (local_err) {
>           error_t whapropagate(errp, local_err);
>           return;
> @@ -1117,22 +1116,22 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
>       object_unref(OBJECT(cpu));
>   }
>
> -void pc_cpus_init(const char *cpu_model)
> +void pc_cpus_init(PCMachineState *pcms)
>   {
>       int i;
>       X86CPU *cpu = NULL;
> +    MachineState *machine = MACHINE(pcms);
>       Error *error = NULL;
>       unsigned long apic_id_limit;
>
>       /* init CPUs */
> -    if (cpu_model == NULL) {
> +    if (machine->cpu_model == NULL) {
>   #ifdef TARGET_X86_64
> -        cpu_model = "qemu64";
> +        machine->cpu_model = "qemu64";
>   #else
> -        cpu_model = "qemu32";
> +        machine->cpu_model = "qemu32";
>   #endif
>       }
> -    current_cpu_model = cpu_model;
>
>       apic_id_limit = pc_apic_id_limit(max_cpus);
>       if (apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT) {
> @@ -1142,7 +1141,7 @@ void pc_cpus_init(const char *cpu_model)
>       }
>
>       for (i = 0; i < smp_cpus; i++) {
> -        cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i),
> +        cpu = pc_new_cpu(machine->cpu_model, x86_cpu_apic_id_from_index(i),
>                            &error);
>           if (error) {
>               error_report_err(error);
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index ae7bbeb..0eacde1 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -139,7 +139,7 @@ static void pc_init1(MachineState *machine,
>           exit(1);
>       }
>
> -    pc_cpus_init(machine->cpu_model);
> +    pc_cpus_init(pcms);
>
>       if (kvm_enabled() && kvmclock_enabled) {
>           kvmclock_create();
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 19e6670..3744abd 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -128,7 +128,7 @@ static void pc_q35_init(MachineState *machine)
>           exit(1);
>       }
>
> -    pc_cpus_init(machine->cpu_model);
> +    pc_cpus_init(pcms);
>       pc_acpi_init("q35-acpi-dsdt.aml");
>
>       kvmclock_create();
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 0503485..c5961d7 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -168,7 +168,7 @@ bool pc_machine_is_smm_enabled(PCMachineState *pcms);
>   void pc_register_ferr_irq(qemu_irq irq);
>   void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
>
> -void pc_cpus_init(const char *cpu_model);
> +void pc_cpus_init(PCMachineState *pcms);
>   void pc_hot_add_cpu(const int64_t id, Error **errp);
>   void pc_acpi_init(const char *default_dsdt);
>
>