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

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

[Zhu Guihua]

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);
     if (local_err) {
         error_propagate(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);
 
-- 
1.9.3

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

[Eduardo Habkost]

On Thu, Oct 15, 2015 at 11:12:12AM +0800, 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>

Michael, if you prefer to get this through the x86 tree, please let me
know.

-- 
Eduardo

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

[Marcel Apfelbaum]

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

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


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

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

[Andreas Färber]

Am 18.10.2015 um 14:20 schrieb 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 <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
>>
> 
> 
> Andreas, could you ack/nack this patch pls?

I won't nack it, as putting it into the QOM state now is a good idea.
But I would rather put this into PCMachineState, as current_cpu_model
was PC-only and I'd prefer not to encourage more uses of the old API.

Marcel, since this is machine/PC and not core QOM/device, I don't see
accessing the field as a problem. An inline wrapper function would make
sense when many callers need to access it, which I do not envision
beyond this patch.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

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

[Eduardo Habkost]

On Sun, Oct 18, 2015 at 06:22:40PM +0200, Andreas Färber wrote:
> Am 18.10.2015 um 14:20 schrieb 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:
[...]
> >>> -    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>
> > 
> > Andreas, could you ack/nack this patch pls?
> 
> I won't nack it, as putting it into the QOM state now is a good idea.
> But I would rather put this into PCMachineState, as current_cpu_model
> was PC-only and I'd prefer not to encourage more uses of the old API.

I don't undersand what you suggest. The patch doesn't add any new state,
it is just updating the existing MachineState::cpu_model field (just
like it is already done by multiple arm, mips, ppc, and tricore
machines).

-- 
Eduardo

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

[Andreas Färber]

Am 19.10.2015 um 17:47 schrieb Eduardo Habkost:
> On Sun, Oct 18, 2015 at 06:22:40PM +0200, Andreas Färber wrote:
>> Am 18.10.2015 um 14:20 schrieb 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:
> [...]
>>>>> -    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>
>>>
>>> Andreas, could you ack/nack this patch pls?
>>
>> I won't nack it, as putting it into the QOM state now is a good idea.
>> But I would rather put this into PCMachineState, as current_cpu_model
>> was PC-only and I'd prefer not to encourage more uses of the old API.
> 
> I don't undersand what you suggest. The patch doesn't add any new state,
> it is just updating the existing MachineState::cpu_model field (just
> like it is already done by multiple arm, mips, ppc, and tricore
> machines).

Looking at the full patch now, you are right. It is not adding a new
field but merely setting it.

Acked-by: Andreas Färber <afaerber@suse.de>

Sorry for the confusion,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)