| Message ID | 1444878732-12626-1-git-send-email-zhugh.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
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.
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); > >
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); > > > >
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
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).
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
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);