On Wed, 27 Mar 2013 12:19:01 +0100
Paolo Bonzini <pbonz...@redhat.com> wrote:
> Il 21/03/2013 15:28, Igor Mammedov ha scritto:
> > ... via do_cpu_hot_add() hook called by cpu_set QMP command,
> > for x86 target.
> >
> > * add extra check that APIC ID is in allowed range
> > * return error if CPU with requested APIC ID exists before creating
> > a new instance. (CPU removal is broken now, will be fixed with CPU unplug)
> > * call CPU add notifier as the last step of x86_cpu_realizefn() to
> > update rtc_cmos and trigger CPU hot-plug ACPI GPE to notify guest.
> > Doing it from x86_cpu_realizefn() will allow to do the same when
> > it would be possible to add CPU using device_add.
> >
> > Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> > ---
> > hw/i386/pc.c | 22 ++++++++++++++++++++++
> > target-i386/cpu.c | 1 +
> > 2 files changed, 23 insertions(+), 0 deletions(-)
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 7481f73..e3ba9ee 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -867,6 +867,19 @@ static void pc_new_cpu(const char *cpu_model, int64_t
> > apic_id, Error **errp)
> > {
> > X86CPU *cpu;
> >
> > + if (apic_id >= pc_apic_id_limit(max_cpus)) {
> > + error_setg(errp, "Unable to add CPU with ID: 0x%" PRIx64
> > + ", max allowed ID: 0x%x", apic_id,
> > + pc_apic_id_limit(max_cpus) - 1);
> > + return;
> > + }
>
> This seems the wrong place to do this check. It should be done in
> do_cpu_hot_add, simply comparing against max_cpus. Here, instead, you
> should _assert_ that the APIC ID is in range.
I'll move it there, but I'd leave pc_apic_id_limit(max_cpus) since id it
compares with is APIC ID.
>
> > + if (x86_cpu_is_cpu_exist(qdev_get_machine(), &apic_id)) {
>
> Similarly, can this be done in qmp_cpu_set? And should it really be an
I've tried to abstract qmp part from target/implementation details.
We could introduce global func like, and then use it if from QMP subsystem:
bool is_cpu_exist(int64_t id) {
... iterate over CPUs ...
if (cpu->get_firmware_id() == id)
return true;
...
}
Andreas, Is it acceptable if I add it to qom/cpu.h, qom/cpu.c ?
> error? Onlining an already-online CPU is fine.
CPU is not just onlined though, it's created and it couldn't be right to
create the same CPU. Hence a error here, so user would know that it tries to
add the same device.
>
> Again, here you could assert that the CPU is not a duplicate, instead.
sure
>
> > + error_setg(errp, "Unable to add CPU with ID: 0x%" PRIx64
> > + ", it's already exists", apic_id);
> > + return;
> > + }
> > cpu = cpu_x86_create(cpu_model, errp);
> > if (!cpu) {
> > return;
> > @@ -882,6 +895,8 @@ static void pc_new_cpu(const char *cpu_model, int64_t
> > apic_id, Error **errp)
> > }
> > }
> >
> > +static const char *saved_cpu_model;
> > +
> > void pc_cpus_init(const char *cpu_model)
>
> Instead of using this global, see the approach in the previous patch.
>
> > {
> > int i;
> > @@ -895,6 +910,8 @@ void pc_cpus_init(const char *cpu_model)
> > cpu_model = "qemu32";
> > #endif
> > }
> > + saved_cpu_model = cpu_model;
> > +
>
> > for (i = 0; i < smp_cpus; i++) {
> > pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), &error);
> > @@ -906,6 +923,11 @@ void pc_cpus_init(const char *cpu_model)
> > }
> > }
> >
> > +void do_cpu_hot_add(const int64_t id, Error **errp)
> > +{
> > + pc_new_cpu(saved_cpu_model, id, errp);
> > +}
> > +
>
> Missing x86_cpu_apic_id_from_index(id)?
There was(is?) opposition to using cpu_index to identify x86 CPU.
So, it is expected from management to provide APIC ID instead of cpu_index.
It could be useful to make hotplug to a specific NUMA node/cpu to work in
future.
Though interface of possible APIC IDs discovery is not part of this series.
>
> > void pc_acpi_init(const char *default_dsdt)
> > {
> > char *filename = NULL, *arg = NULL;
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index ae46f81..d127141 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2271,6 +2271,7 @@ out:
> > if (dev->hotplugged) {
> > cpu_synchronize_post_init(env);
> > resume_vcpu(CPU(cpu));
> > + qemu_system_cpu_hotplug_request(env->cpuid_apic_id);
>
> As mentioned earlier, this notifier should be invoked at the CPU level,
> not X86CPU.
done
> Paolo
>
> > }
> > }
> >
> >
>
>
--
Regards,
Igor
