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.
> + 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
error? Onlining an already-online CPU is fine.
Again, here you could assert that the CPU is not a duplicate, instead.
> + 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)?
> 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.
Paolo
> }
> }
>
>
