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

[Michael S. Tsirkin]

On Sun, Oct 18, 2015 at 02:18:31PM +0300, Marcel Apfelbaum wrote:
> 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 <address@hidden>
> >Reviewed-by: Eduardo Habkost <address@hidden>
> >---
> >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 <address@hidden>
> 
> Thanks,
> Marcel
> 


Andreas, could you ack/nack this patch pls?


> 
> >      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);
> >
> >