[Qemu-devel] [PATCH v4 00/16] pc: add CPU hot-add/hot-remove with device_add/device_del

[Qemu-devel] [PATCH v4 00/16] pc: add CPU hot-add/hot-remove with device_add/device_del

[Igor Mammedov]

Changelog:
  since v3:
    * rebase on top of x86-next tree as it's already applied 1-6/19 from v3
      (so they are could be omitted from v4)
    * set apic-state to NULL after destroying it in unrealize()
    * fix places where I forgot to print X86CPUTopoInfo fields as %u
    * move "pc: implement query-hotpluggable-cpus callback" after
      patch that enables device_add cpu
    * extract counting of present cpus in possible_cpus into a separate helper
    * newly added patches:
        * pc: forbid BSP removal
        * pc: enforce adding CPUs contiguously and removing them in opposit order
        * apic: kvm-apic: fix crash due to access to freed memory region
    * update Reviewed-bys
  since v2:
    * use 0xFFFFFFFF for UNASSIGNED_APIC_ID instead of UINT32_MAX
    * add comment why 0xFFFFFFFF could be used for UNASSIGNED_APIC_ID
    * print topo ids is unsigned
    * print APIC ID as hex
    * print topo ids calculated from APIC ID beside it
    * add extra patch to fix migration failure due to APIC's instance_id mismatch
  since v1:
    * s/pc_find_cpu/pc_find_cpu_slot/ + add comment to it
    * add more sanity checks for socket-id/core-id/thread-id and 'apic'
      properties
    * include device_del cpu patches and related fixes to x86 CPU/apic

Series enabling usage of -device/device_add for adding CPUs as devices
and device_del for removing them. Using -device/device_add in combination
with query-hotpluggable-cpus QMP command allows to hotplug CPUs at any
not used possition and then safely migrate QEMU instance by specifying
hotadded CPUs on target with help of -device CLI option like with any
other device.
Having been able to replicate exact topology on taggert with -device CPUs
also opens poosibility to hot-remove CPUs, which this series does by
enabling to use device_del with x86 CPUs.


git tree for testing:
  https://github.com/imammedo/qemu.git dev_del_cpu_v4
for viewing:
  https://github.com/imammedo/qemu/commits/dev_del_cpu_v4

Tested with RHEL7.2 guest including ping/pong migration with adding/removing
CPUs in between.

CC: pkrempa@redhat.com
CC: ehabkost@redhat.com
CC: mst@redhat.com
CC: eduardo.otubo@profitbricks.com
CC: Bandan Das <bdas@redhat.com>

Igor Mammedov (16):
  pc: set APIC ID based on socket/core/thread ids if it's not been set
    yet
  pc: delay setting number of boot CPUs to machine_done time
  pc: register created initial and hotpluged CPUs in one place
    pc_cpu_plug()
  pc: forbid BSP removal
  pc: enforce adding CPUs contiguously and removing them in opposit
    order
  pc: cpu: allow device_add to be used with x86 cpu
  pc: implement query-hotpluggable-cpus callback
  apic: move MAX_APICS check to 'apic' class
  apic: drop APICCommonState.idx and use APIC ID as index in
    local_apics[]
  apic: kvm-apic: fix crash due to access to freed memory region
  (kvm)apic: add unrealize callbacks
  apic: use apic_id as apic's migration instance_id
  target-i386: cpu: do not ignore error and fix apic parent
  target-i386: fix apic object leak when CPU is deleted
  target-i386: add x86_cpu_unrealizefn()
  pc: make device_del CPU work for x86 CPUs

 include/hw/i386/apic_internal.h |   5 +-
 include/hw/i386/pc.h            |   5 ++
 hw/i386/kvm/apic.c              |   9 +-
 hw/i386/pc.c                    | 183 ++++++++++++++++++++++++++++++++++------
 hw/intc/apic.c                  |  26 +++++-
 hw/intc/apic_common.c           |  33 ++++++--
 qmp-commands.hx                 |  15 ++++
 target-i386/cpu.c               |  23 ++++-
 8 files changed, 251 insertions(+), 48 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v4 01/16] pc: set APIC ID based on socket/core/thread ids if it's not been set yet

[Igor Mammedov]

CPU added with device_add help won't have APIC ID set,
so set it according to socket/core/thread ids provided
with device_add command.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
v4:
 - use %u for printing topo ids in if (!cpu_slot) branch
v3:
 - use %u for printing topo ids
v2:
 - add validity checks for socket-id/core-id/thread-id values
---
 hw/i386/pc.c | 44 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f07d38a..31b502c 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1792,14 +1792,52 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
                             DeviceState *dev, Error **errp)
 {
     int idx;
+    CPUArchId *cpu_slot;
     X86CPUTopoInfo topo;
     X86CPU *cpu = X86_CPU(dev);
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
-    CPUArchId *cpu_slot = pc_find_cpu_slot(pcms, CPU(dev), &idx);
 
+    /* if APIC ID is not set, set it based on socket/core/thread properties */
+    if (cpu->apic_id == UNASSIGNED_APIC_ID) {
+        int max_socket = (max_cpus - 1) / smp_threads / smp_cores;
+
+        if (cpu->socket_id < 0) {
+            error_setg(errp, "CPU socket-id is not set");
+            return;
+        } else if (cpu->socket_id > max_socket) {
+            error_setg(errp, "Invalid CPU socket-id: %u must be in range 0:%u",
+                       cpu->socket_id, max_socket);
+            return;
+        }
+        if (cpu->core_id < 0) {
+            error_setg(errp, "CPU core-id is not set");
+            return;
+        } else if (cpu->core_id > (smp_cores - 1)) {
+            error_setg(errp, "Invalid CPU core-id: %u must be in range 0:%u",
+                       cpu->core_id, smp_cores - 1);
+            return;
+        }
+        if (cpu->thread_id < 0) {
+            error_setg(errp, "CPU thread-id is not set");
+            return;
+        } else if (cpu->thread_id > (smp_threads - 1)) {
+            error_setg(errp, "Invalid CPU thread-id: %u must be in range 0:%u",
+                       cpu->thread_id, smp_threads - 1);
+            return;
+        }
+
+        topo.pkg_id = cpu->socket_id;
+        topo.core_id = cpu->core_id;
+        topo.smt_id = cpu->thread_id;
+        cpu->apic_id = apicid_from_topo_ids(smp_cores, smp_threads, &topo);
+    }
+
+    cpu_slot = pc_find_cpu_slot(pcms, CPU(dev), &idx);
     if (!cpu_slot) {
-        error_setg(errp, "Invalid CPU index with APIC ID (%" PRIu32
-                   "), valid range 0:%d", cpu->apic_id,
+        x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads, &topo);
+        error_setg(errp, "Invalid CPU [socket: %u, core: %u, thread: %u] with"
+                  " APIC ID %" PRIu32 ", valid index range 0:%d",
+                   topo.pkg_id, topo.core_id, topo.smt_id, cpu->apic_id,
                    pcms->possible_cpus->len - 1);
         return;
     }
-- 
2.7.4

[Qemu-devel] [PATCH v4 02/16] pc: delay setting number of boot CPUs to machine_done time

[Igor Mammedov]

currently present CPUs counter in CMOS only contains
smp_cpus (i.e. initial CPUs specified with -smp X) and
doesn't account for CPUs created with -device.
If VM is started with additional CPUs added with
 -device, it will hang in BIOS waiting for condition
   smp_cpus == counted_cpus
forever as counted_cpus will include -device CPUs as well
and be more than smp_cpus.

make present CPUs counter in CMOS to count all CPUs
(initial and coldplugged with -device) by delaying
it to machine done time when it possible to count
CPUs added with -device.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v4:
 - extract present cpus count into a separate function
     pc_present_cpus_count()
---
 hw/i386/pc.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 31b502c..d83fde4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -471,9 +471,6 @@ void pc_cmos_init(PCMachineState *pcms,
     rtc_set_memory(s, 0x5c, val >> 8);
     rtc_set_memory(s, 0x5d, val >> 16);
 
-    /* set the number of CPU */
-    rtc_set_memory(s, 0x5f, smp_cpus - 1);
-
     object_property_add_link(OBJECT(pcms), "rtc_state",
                              TYPE_ISA_DEVICE,
                              (Object **)&pcms->rtc,
@@ -1039,6 +1036,17 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
+static int pc_present_cpus_count(PCMachineState *pcms)
+{
+    int i, boot_cpus = 0;
+    for (i = 0; i < pcms->possible_cpus->len; i++) {
+        if (pcms->possible_cpus->cpus[i].cpu) {
+            boot_cpus++;
+        }
+    }
+    return boot_cpus;
+}
+
 static X86CPU *pc_new_cpu(const char *typename, int64_t apic_id,
                           Error **errp)
 {
@@ -1189,6 +1197,9 @@ void pc_machine_done(Notifier *notifier, void *data)
                                         PCMachineState, machine_done);
     PCIBus *bus = pcms->bus;
 
+    /* set the number of CPUs */
+    rtc_set_memory(pcms->rtc, 0x5f, pc_present_cpus_count(pcms) - 1);
+
     if (bus) {
         int extra_hosts = 0;
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 02/16] pc: delay setting number of boot CPUs to machine_done time

[Eduardo Habkost]

On Thu, Jul 14, 2016 at 06:54:31PM +0200, Igor Mammedov wrote:
> currently present CPUs counter in CMOS only contains
> smp_cpus (i.e. initial CPUs specified with -smp X) and
> doesn't account for CPUs created with -device.
> If VM is started with additional CPUs added with
>  -device, it will hang in BIOS waiting for condition
>    smp_cpus == counted_cpus
> forever as counted_cpus will include -device CPUs as well
> and be more than smp_cpus.
> 
> make present CPUs counter in CMOS to count all CPUs
> (initial and coldplugged with -device) by delaying
> it to machine done time when it possible to count
> CPUs added with -device.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

[Qemu-devel] [PATCH v4 03/16] pc: register created initial and hotpluged CPUs in one place pc_cpu_plug()

[Igor Mammedov]

consolidate possible_cpus array management in pc_cpu_plug()
for smp_cpus, coldplugged with -device and hotplugged with
device_add.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index d83fde4..5a67f15 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1153,7 +1153,6 @@ void pc_cpus_init(PCMachineState *pcms)
         if (i < smp_cpus) {
             cpu = pc_new_cpu(typename, x86_cpu_apic_id_from_index(i),
                              &error_fatal);
-            pcms->possible_cpus->cpus[i].cpu = CPU(cpu);
             object_unref(OBJECT(cpu));
         }
     }
@@ -1731,25 +1730,19 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
     Error *local_err = NULL;
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
 
-    if (!dev->hotplugged) {
-        goto out;
-    }
-
-    if (!pcms->acpi_dev) {
-        error_setg(&local_err,
-                   "cpu hotplug is not enabled: missing acpi device");
-        goto out;
+    if (pcms->acpi_dev) {
+        hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
+        hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
+        if (local_err) {
+            goto out;
+        }
     }
 
-    hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
-    hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
-    if (local_err) {
-        goto out;
+    if (dev->hotplugged) {
+        /* increment the number of CPUs */
+        rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1);
     }
 
-    /* increment the number of CPUs */
-    rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1);
-
     found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL);
     found_cpu->cpu = CPU(dev);
 out:
-- 
2.7.4

[Qemu-devel] [PATCH v4 04/16] pc: forbid BSP removal

[Igor Mammedov]

BSP is assumed to always present in QEMU code, so
untile that assumptions are gone, deny removal request.
In another words QEMU won't support BSP hot-unplug.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 5a67f15..33c5f97 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1751,10 +1751,17 @@ out:
 static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
                                      DeviceState *dev, Error **errp)
 {
+    int idx;
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
 
+    pc_find_cpu_slot(pcms, CPU(dev), &idx);
+    if (idx == 0) {
+        error_setg(&local_err, "1st CPU (BSP) is unpluggable");
+        goto out;
+    }
+
     hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
     hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 04/16] pc: forbid BSP removal

[Bandan Das]

Igor Mammedov <imammedo@redhat.com> writes:

> BSP is assumed to always present in QEMU code, so
> untile that assumptions are gone, deny removal request.
> In another words QEMU won't support BSP hot-unplug.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/i386/pc.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 5a67f15..33c5f97 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1751,10 +1751,17 @@ out:
>  static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
>                                       DeviceState *dev, Error **errp)
>  {
> +    int idx;
>      HotplugHandlerClass *hhc;
>      Error *local_err = NULL;
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>  
> +    pc_find_cpu_slot(pcms, CPU(dev), &idx);
> +    if (idx == 0) {
> +        error_setg(&local_err, "1st CPU (BSP) is unpluggable");
> +        goto out;
> +    }

Nit: Boot CPU or simply Bootstrap Processor sounds better IMO.

>      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>      hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);

Re: [Qemu-devel] [PATCH v4 04/16] pc: forbid BSP removal

[Eduardo Habkost]

On Thu, Jul 14, 2016 at 06:54:33PM +0200, Igor Mammedov wrote:
> BSP is assumed to always present in QEMU code, so
> untile that assumptions are gone, deny removal request.
> In another words QEMU won't support BSP hot-unplug.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/i386/pc.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 5a67f15..33c5f97 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1751,10 +1751,17 @@ out:
>  static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
>                                       DeviceState *dev, Error **errp)
>  {
> +    int idx;
>      HotplugHandlerClass *hhc;
>      Error *local_err = NULL;
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>  
> +    pc_find_cpu_slot(pcms, CPU(dev), &idx);

Looks fragile: if one day we create any TYPE_CPU object that is
not in possible_cpus array, idx is undefined. I suggest
initializing idx to -1 above.

I can change it when committing, if that's OK for you.


> +    if (idx == 0) {
> +        error_setg(&local_err, "1st CPU (BSP) is unpluggable");
> +        goto out;
> +    }
> +
>      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>      hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
>  
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 04/16] pc: forbid BSP removal

[Bandan Das]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Jul 14, 2016 at 06:54:33PM +0200, Igor Mammedov wrote:
>> BSP is assumed to always present in QEMU code, so
>> untile that assumptions are gone, deny removal request.
>> In another words QEMU won't support BSP hot-unplug.
>> 
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>> ---
>>  hw/i386/pc.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>> 
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 5a67f15..33c5f97 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1751,10 +1751,17 @@ out:
>>  static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
>>                                       DeviceState *dev, Error **errp)
>>  {
>> +    int idx;
>>      HotplugHandlerClass *hhc;
>>      Error *local_err = NULL;
>>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>>  
>> +    pc_find_cpu_slot(pcms, CPU(dev), &idx);
>
> Looks fragile: if one day we create any TYPE_CPU object that is
> not in possible_cpus array, idx is undefined. I suggest
> initializing idx to -1 above.

Or just let pc_find_cpu_slot universally set it to -1 since
this series assumes that -1 means index isn't valid.

> I can change it when committing, if that's OK for you.
>
>
>> +    if (idx == 0) {
>> +        error_setg(&local_err, "1st CPU (BSP) is unpluggable");
>> +        goto out;
>> +    }
>> +
>>      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>>      hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
>>  
>> -- 
>> 2.7.4
>> 

Re: [Qemu-devel] [PATCH v4 04/16] pc: forbid BSP removal

[Eduardo Habkost]

On Thu, Jul 14, 2016 at 02:16:39PM -0400, Bandan Das wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Thu, Jul 14, 2016 at 06:54:33PM +0200, Igor Mammedov wrote:
> >> BSP is assumed to always present in QEMU code, so
> >> untile that assumptions are gone, deny removal request.
> >> In another words QEMU won't support BSP hot-unplug.
> >> 
> >> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >> ---
> >>  hw/i386/pc.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >> 
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index 5a67f15..33c5f97 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> >> @@ -1751,10 +1751,17 @@ out:
> >>  static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
> >>                                       DeviceState *dev, Error **errp)
> >>  {
> >> +    int idx;
> >>      HotplugHandlerClass *hhc;
> >>      Error *local_err = NULL;
> >>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> >>  
> >> +    pc_find_cpu_slot(pcms, CPU(dev), &idx);
> >
> > Looks fragile: if one day we create any TYPE_CPU object that is
> > not in possible_cpus array, idx is undefined. I suggest
> > initializing idx to -1 above.
> 
> Or just let pc_find_cpu_slot universally set it to -1 since
> this series assumes that -1 means index isn't valid.

I think it would be more intuitive if pc_find_cpu_slot() didn't
touch *idx if no slot is found. But both ways sound good to me.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 04/16] pc: forbid BSP removal

[Bandan Das]

Eduardo Habkost <ehabkost@redhat.com> writes:
...
>> >>                                       DeviceState *dev, Error **errp)
>> >>  {
>> >> +    int idx;
>> >>      HotplugHandlerClass *hhc;
>> >>      Error *local_err = NULL;
>> >>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>> >>  
>> >> +    pc_find_cpu_slot(pcms, CPU(dev), &idx);
>> >
>> > Looks fragile: if one day we create any TYPE_CPU object that is
>> > not in possible_cpus array, idx is undefined. I suggest
>> > initializing idx to -1 above.
>> 
>> Or just let pc_find_cpu_slot universally set it to -1 since
>> this series assumes that -1 means index isn't valid.
>
> I think it would be more intuitive if pc_find_cpu_slot() didn't
> touch *idx if no slot is found. But both ways sound good to me.

The caller then has to always take care of making sure there
is no bogus value in idx. Maybe, always calling if (pc_find_cpu_slot())
is better.

Re: [Qemu-devel] [PATCH v4 04/16] pc: forbid BSP removal

[Igor Mammedov]

On Thu, 14 Jul 2016 14:54:53 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Jul 14, 2016 at 06:54:33PM +0200, Igor Mammedov wrote:
> > BSP is assumed to always present in QEMU code, so
> > untile that assumptions are gone, deny removal request.
> > In another words QEMU won't support BSP hot-unplug.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/i386/pc.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 5a67f15..33c5f97 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1751,10 +1751,17 @@ out:
> >  static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
> >                                       DeviceState *dev, Error **errp)
> >  {
> > +    int idx;
> >      HotplugHandlerClass *hhc;
> >      Error *local_err = NULL;
> >      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> >  
> > +    pc_find_cpu_slot(pcms, CPU(dev), &idx);  
> 
> Looks fragile: if one day we create any TYPE_CPU object that is
> not in possible_cpus array, idx is undefined. I suggest
> initializing idx to -1 above.
not that we expect that ever happen, but it won't hurt to
initialize it to -1.

> 
> I can change it when committing, if that's OK for you.
sounds good to me, thanks.

> 
> 
> > +    if (idx == 0) {
> > +        error_setg(&local_err, "1st CPU (BSP) is unpluggable");
> > +        goto out;
> > +    }
> > +
> >      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
> >      hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
> >  
> > -- 
> > 2.7.4
> >   
> 

[Qemu-devel] [PATCH v5 04/16] pc: forbid BSP removal

[Igor Mammedov]

Boot CPU is assumed to always present in QEMU code, so
untile that assumptions are gone, deny removal request,
In another words QEMU won't support BSP hot-unplug.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v5:
  - s/1st CPU (BSP)/Boot CPU/
     Eduardo Habkost <ehabkost@redhat.com>
  - intialize idx to -1 and assert on it,
     Eduardo Habkost <ehabkost@redhat.com>
    (note: that should nexer happen in current code as we don't
     have stray CPUs and most likely never will, but it doesn't
     hurt to cautios)
---
 hw/i386/pc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 110f1bf..e15fcc1 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1757,10 +1757,18 @@ out:
 static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
                                      DeviceState *dev, Error **errp)
 {
+    int idx = -1;
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
 
+    pc_find_cpu_slot(pcms, CPU(dev), &idx);
+    assert(idx != -1);
+    if (idx == 0) {
+        error_setg(&local_err, "Boot CPU is unpluggable");
+        goto out;
+    }
+
     hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
     hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v5 04/16] pc: forbid BSP removal

[Eduardo Habkost]

On Mon, Jul 18, 2016 at 10:31:22AM +0200, Igor Mammedov wrote:
> Boot CPU is assumed to always present in QEMU code, so
> untile that assumptions are gone, deny removal request,
> In another words QEMU won't support BSP hot-unplug.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

> ---
> v5:
>   - s/1st CPU (BSP)/Boot CPU/
>      Eduardo Habkost <ehabkost@redhat.com>
>   - intialize idx to -1 and assert on it,
>      Eduardo Habkost <ehabkost@redhat.com>
>     (note: that should nexer happen in current code as we don't
>      have stray CPUs and most likely never will, but it doesn't
>      hurt to cautios)
> ---
>  hw/i386/pc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 110f1bf..e15fcc1 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1757,10 +1757,18 @@ out:
>  static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
>                                       DeviceState *dev, Error **errp)
>  {
> +    int idx = -1;
>      HotplugHandlerClass *hhc;
>      Error *local_err = NULL;
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>  
> +    pc_find_cpu_slot(pcms, CPU(dev), &idx);
> +    assert(idx != -1);
> +    if (idx == 0) {
> +        error_setg(&local_err, "Boot CPU is unpluggable");
> +        goto out;
> +    }
> +
>      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>      hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
>  
> -- 
> 2.7.4
> 

-- 
Eduardo

[Qemu-devel] [PATCH v4 05/16] pc: enforce adding CPUs contiguously and removing them in opposit order

[Igor Mammedov]

it will still allow us to use cpu_index as migration instance_id
since when CPUs are added contiguously (from the first to the last)
and removed in opposite order, cpu_index stays stable and it's
reproducable on destination side.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
While there is work in progress to support migration when there are holes
in cpu_index range resulting from out-of-order plug or unplug, this patch
is intended as a last resort if no easy, risk-free and elegant solution
emerges before 2.7 dev cycle ends.
---
 hw/i386/pc.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 33c5f97..75a92d0 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1762,6 +1762,23 @@ static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
         goto out;
     }
 
+    if (idx < pcms->possible_cpus->len - 1 &&
+        pcms->possible_cpus->cpus[idx + 1].cpu != NULL) {
+        X86CPU *cpu;
+
+        for (idx = pcms->possible_cpus->len - 1;
+             pcms->possible_cpus->cpus[idx].cpu == NULL; idx--) {
+            ;;
+        }
+
+        cpu = X86_CPU(pcms->possible_cpus->cpus[idx].cpu);
+        error_setg(&local_err, "CPU [socket-id: %u, core-id: %u,"
+                   " thread-id: %u] should be removed first",
+                   cpu->socket_id, cpu->core_id, cpu->thread_id);
+        goto out;
+
+    }
+
     hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
     hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
 
@@ -1860,6 +1877,23 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
         return;
     }
 
+    if (idx != 0 && pcms->possible_cpus->cpus[idx - 1].cpu == NULL) {
+        PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+
+        for (idx = 1; pcms->possible_cpus->cpus[idx].cpu != NULL; idx++) {
+            ;;
+        }
+
+        x86_topo_ids_from_apicid(pcms->possible_cpus->cpus[idx].arch_id,
+                                 smp_cores, smp_threads, &topo);
+
+        if (!pcmc->legacy_cpu_hotplug) {
+            error_setg(errp, "CPU [socket: %u, core: %u, thread: %u] should be"
+                       " added first", topo.pkg_id, topo.core_id, topo.smt_id);
+            return;
+        }
+    }
+
     /* if 'address' properties socket-id/core-id/thread-id are not set, set them
      * so that query_hotpluggable_cpus would show correct values
      */
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 05/16] pc: enforce adding CPUs contiguously and removing them in opposit order

[Bandan Das]

Igor Mammedov <imammedo@redhat.com> writes:

> it will still allow us to use cpu_index as migration instance_id
> since when CPUs are added contiguously (from the first to the last)
> and removed in opposite order, cpu_index stays stable and it's
> reproducable on destination side.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> While there is work in progress to support migration when there are holes
> in cpu_index range resulting from out-of-order plug or unplug, this patch
> is intended as a last resort if no easy, risk-free and elegant solution
> emerges before 2.7 dev cycle ends.

I think this (or a modified version) is appropriate comment
material to accompany the changes. Ok if you are sure this code
is short-lived, but if it stays longer, a comment is definitely
helpful. Maybe a bit of reasoning added to the error message is
fine too.

> ---
>  hw/i386/pc.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 33c5f97..75a92d0 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1762,6 +1762,23 @@ static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
>          goto out;
>      }
>  
> +    if (idx < pcms->possible_cpus->len - 1 &&
> +        pcms->possible_cpus->cpus[idx + 1].cpu != NULL) {
> +        X86CPU *cpu;
> +
> +        for (idx = pcms->possible_cpus->len - 1;
> +             pcms->possible_cpus->cpus[idx].cpu == NULL; idx--) {
> +            ;;
> +        }
> +
> +        cpu = X86_CPU(pcms->possible_cpus->cpus[idx].cpu);
> +        error_setg(&local_err, "CPU [socket-id: %u, core-id: %u,"
> +                   " thread-id: %u] should be removed first",
> +                   cpu->socket_id, cpu->core_id, cpu->thread_id);
> +        goto out;
> +
> +    }
> +
>      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>      hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
>  
> @@ -1860,6 +1877,23 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>          return;
>      }
>  
> +    if (idx != 0 && pcms->possible_cpus->cpus[idx - 1].cpu == NULL) {
> +        PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> +
> +        for (idx = 1; pcms->possible_cpus->cpus[idx].cpu != NULL; idx++) {
> +            ;;
> +        }
> +
> +        x86_topo_ids_from_apicid(pcms->possible_cpus->cpus[idx].arch_id,
> +                                 smp_cores, smp_threads, &topo);
> +
> +        if (!pcmc->legacy_cpu_hotplug) {
> +            error_setg(errp, "CPU [socket: %u, core: %u, thread: %u] should be"
> +                       " added first", topo.pkg_id, topo.core_id, topo.smt_id);
> +            return;
> +        }
> +    }
> +
>      /* if 'address' properties socket-id/core-id/thread-id are not set, set them
>       * so that query_hotpluggable_cpus would show correct values
>       */

Re: [Qemu-devel] [PATCH v4 05/16] pc: enforce adding CPUs contiguously and removing them in opposit order

[Igor Mammedov]

On Thu, 14 Jul 2016 14:10:24 -0400
Bandan Das <bsd@redhat.com> wrote:

> Igor Mammedov <imammedo@redhat.com> writes:
> 
> > it will still allow us to use cpu_index as migration instance_id
> > since when CPUs are added contiguously (from the first to the last)
> > and removed in opposite order, cpu_index stays stable and it's
> > reproducable on destination side.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > While there is work in progress to support migration when there are holes
> > in cpu_index range resulting from out-of-order plug or unplug, this patch
> > is intended as a last resort if no easy, risk-free and elegant solution
> > emerges before 2.7 dev cycle ends.  
> 
> I think this (or a modified version) is appropriate comment
> material to accompany the changes. Ok if you are sure this code
> is short-lived, but if it stays longer, a comment is definitely
> helpful. Maybe a bit of reasoning added to the error message is
> fine too.
dwg is looking at cpu_index refactoring but that's not 2.7 material,
this patch is doing what the similar spapr patch did
(which David applied to his ppc queue).

Perhaps moving comment under --- to commit message itself would
be better as to leave trace of future refactoring plans.

> > ---
> >  hw/i386/pc.c | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 33c5f97..75a92d0 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1762,6 +1762,23 @@ static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
> >          goto out;
> >      }
> >  
> > +    if (idx < pcms->possible_cpus->len - 1 &&
> > +        pcms->possible_cpus->cpus[idx + 1].cpu != NULL) {
> > +        X86CPU *cpu;
> > +
> > +        for (idx = pcms->possible_cpus->len - 1;
> > +             pcms->possible_cpus->cpus[idx].cpu == NULL; idx--) {
> > +            ;;
> > +        }
> > +
> > +        cpu = X86_CPU(pcms->possible_cpus->cpus[idx].cpu);
> > +        error_setg(&local_err, "CPU [socket-id: %u, core-id: %u,"
> > +                   " thread-id: %u] should be removed first",
> > +                   cpu->socket_id, cpu->core_id, cpu->thread_id);
> > +        goto out;
> > +
> > +    }
> > +
> >      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
> >      hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
> >  
> > @@ -1860,6 +1877,23 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> >          return;
> >      }
> >  
> > +    if (idx != 0 && pcms->possible_cpus->cpus[idx - 1].cpu == NULL) {
> > +        PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> > +
> > +        for (idx = 1; pcms->possible_cpus->cpus[idx].cpu != NULL; idx++) {
> > +            ;;
> > +        }
> > +
> > +        x86_topo_ids_from_apicid(pcms->possible_cpus->cpus[idx].arch_id,
> > +                                 smp_cores, smp_threads, &topo);
> > +
> > +        if (!pcmc->legacy_cpu_hotplug) {
> > +            error_setg(errp, "CPU [socket: %u, core: %u, thread: %u] should be"
> > +                       " added first", topo.pkg_id, topo.core_id, topo.smt_id);
> > +            return;
> > +        }
> > +    }
> > +
> >      /* if 'address' properties socket-id/core-id/thread-id are not set, set them
> >       * so that query_hotpluggable_cpus would show correct values
> >       */  

Re: [Qemu-devel] [PATCH v4 05/16] pc: enforce adding CPUs contiguously and removing them in opposit order

[Bandan Das]

Igor Mammedov <imammedo@redhat.com> writes:

> On Thu, 14 Jul 2016 14:10:24 -0400
> Bandan Das <bsd@redhat.com> wrote:
>
>> Igor Mammedov <imammedo@redhat.com> writes:
>> 
>> > it will still allow us to use cpu_index as migration instance_id
>> > since when CPUs are added contiguously (from the first to the last)
>> > and removed in opposite order, cpu_index stays stable and it's
>> > reproducable on destination side.
>> >
>> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>> > ---
>> > While there is work in progress to support migration when there are holes
>> > in cpu_index range resulting from out-of-order plug or unplug, this patch
>> > is intended as a last resort if no easy, risk-free and elegant solution
>> > emerges before 2.7 dev cycle ends.  
>> 
>> I think this (or a modified version) is appropriate comment
>> material to accompany the changes. Ok if you are sure this code
>> is short-lived, but if it stays longer, a comment is definitely
>> helpful. Maybe a bit of reasoning added to the error message is
>> fine too.
> dwg is looking at cpu_index refactoring but that's not 2.7 material,
> this patch is doing what the similar spapr patch did
> (which David applied to his ppc queue).
>
> Perhaps moving comment under --- to commit message itself would
> be better as to leave trace of future refactoring plans.

Right, sounds good.

>> > ---
>> >  hw/i386/pc.c | 34 ++++++++++++++++++++++++++++++++++
>> >  1 file changed, 34 insertions(+)
>> >
>> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> > index 33c5f97..75a92d0 100644
>> > --- a/hw/i386/pc.c
>> > +++ b/hw/i386/pc.c
>> > @@ -1762,6 +1762,23 @@ static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
>> >          goto out;
>> >      }
>> >  
>> > +    if (idx < pcms->possible_cpus->len - 1 &&
>> > +        pcms->possible_cpus->cpus[idx + 1].cpu != NULL) {
>> > +        X86CPU *cpu;
>> > +
>> > +        for (idx = pcms->possible_cpus->len - 1;
>> > +             pcms->possible_cpus->cpus[idx].cpu == NULL; idx--) {
>> > +            ;;
>> > +        }
>> > +
>> > +        cpu = X86_CPU(pcms->possible_cpus->cpus[idx].cpu);
>> > +        error_setg(&local_err, "CPU [socket-id: %u, core-id: %u,"
>> > +                   " thread-id: %u] should be removed first",
>> > +                   cpu->socket_id, cpu->core_id, cpu->thread_id);
>> > +        goto out;
>> > +
>> > +    }
>> > +
>> >      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>> >      hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
>> >  
>> > @@ -1860,6 +1877,23 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>> >          return;
>> >      }
>> >  
>> > +    if (idx != 0 && pcms->possible_cpus->cpus[idx - 1].cpu == NULL) {
>> > +        PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>> > +
>> > +        for (idx = 1; pcms->possible_cpus->cpus[idx].cpu != NULL; idx++) {
>> > +            ;;
>> > +        }
>> > +
>> > +        x86_topo_ids_from_apicid(pcms->possible_cpus->cpus[idx].arch_id,
>> > +                                 smp_cores, smp_threads, &topo);
>> > +
>> > +        if (!pcmc->legacy_cpu_hotplug) {
>> > +            error_setg(errp, "CPU [socket: %u, core: %u, thread: %u] should be"
>> > +                       " added first", topo.pkg_id, topo.core_id, topo.smt_id);
>> > +            return;
>> > +        }
>> > +    }
>> > +
>> >      /* if 'address' properties socket-id/core-id/thread-id are not set, set them
>> >       * so that query_hotpluggable_cpus would show correct values
>> >       */  

[Qemu-devel] [PATCH v5 05/16] pc: enforce adding CPUs contiguously and removing them in opposit order

[Igor Mammedov]

it will still allow us to use cpu_index as migration instance_id
since when CPUs are added contiguously (from the first to the last)
and removed in opposite order, cpu_index stays stable and it's
reproducable on destination side.

While there is work in progress to support migration when there
are holes in cpu_index range resulting from out-of-order plug or
unplug, this patch is intended as an interim solution until
cpu_index usage is cleaned up.

As result of this patch it would be possible to plug/unplug CPUs,
but in limited order that doesn't break migration.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v5:
  - move note from under --- to commit message itself so it won't
    be discarded along the way try to improve cmmit message
    Bandan Das <bsd@redhat.com>
---
 hw/i386/pc.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e15fcc1..dda91da 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1769,6 +1769,23 @@ static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
         goto out;
     }
 
+    if (idx < pcms->possible_cpus->len - 1 &&
+        pcms->possible_cpus->cpus[idx + 1].cpu != NULL) {
+        X86CPU *cpu;
+
+        for (idx = pcms->possible_cpus->len - 1;
+             pcms->possible_cpus->cpus[idx].cpu == NULL; idx--) {
+            ;;
+        }
+
+        cpu = X86_CPU(pcms->possible_cpus->cpus[idx].cpu);
+        error_setg(&local_err, "CPU [socket-id: %u, core-id: %u,"
+                   " thread-id: %u] should be removed first",
+                   cpu->socket_id, cpu->core_id, cpu->thread_id);
+        goto out;
+
+    }
+
     hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
     hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
 
@@ -1867,6 +1884,23 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
         return;
     }
 
+    if (idx != 0 && pcms->possible_cpus->cpus[idx - 1].cpu == NULL) {
+        PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+
+        for (idx = 1; pcms->possible_cpus->cpus[idx].cpu != NULL; idx++) {
+            ;;
+        }
+
+        x86_topo_ids_from_apicid(pcms->possible_cpus->cpus[idx].arch_id,
+                                 smp_cores, smp_threads, &topo);
+
+        if (!pcmc->legacy_cpu_hotplug) {
+            error_setg(errp, "CPU [socket: %u, core: %u, thread: %u] should be"
+                       " added first", topo.pkg_id, topo.core_id, topo.smt_id);
+            return;
+        }
+    }
+
     /* if 'address' properties socket-id/core-id/thread-id are not set, set them
      * so that query_hotpluggable_cpus would show correct values
      */
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 05/16] pc: enforce adding CPUs contiguously and removing them in opposit order

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1500 bytes --]

On 07/14/2016 10:54 AM, Igor Mammedov wrote:

s/opposit/opposite/ in the subject line, but it's already long. I wonder
if you can go shorter, with:

pc: enforce CPU add/remove in contiguous order

> it will still allow us to use cpu_index as migration instance_id
> since when CPUs are added contiguously (from the first to the last)
> and removed in opposite order, cpu_index stays stable and it's
> reproducable on destination side.

s/reproducable/reproducible/

> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> While there is work in progress to support migration when there are holes
> in cpu_index range resulting from out-of-order plug or unplug, this patch
> is intended as a last resort if no easy, risk-free and elegant solution
> emerges before 2.7 dev cycle ends.

I'm not too worried about succeeding only on contiguous ids, as that has
been something libvirt has already had to deal with.  But I am a bit
worried about whether it is easy to introspect whether 2.8 adds a way to
hot-plug (or hot-remove) cpus in arbitrary order, with gaps rather than
contiguous ids, especially if the interface does not gain any obvious
parameters.

If we are going to enhance the interface in the future, do we have any
plans how to make it easily detectable which version of the interface we
are working with (contiguous-only, or full power)?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 05/16] pc: enforce adding CPUs contiguously and removing them in opposit order

[Igor Mammedov]

On Mon, 18 Jul 2016 15:05:37 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 07/14/2016 10:54 AM, Igor Mammedov wrote:
> 
> s/opposit/opposite/ in the subject line, but it's already long. I wonder
> if you can go shorter, with:
> 
> pc: enforce CPU add/remove in contiguous order
> 
> > it will still allow us to use cpu_index as migration instance_id
> > since when CPUs are added contiguously (from the first to the last)
> > and removed in opposite order, cpu_index stays stable and it's
> > reproducable on destination side.  
> 
> s/reproducable/reproducible/
Eduardo could you fix it up when applying or should I respin it?

> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > While there is work in progress to support migration when there are holes
> > in cpu_index range resulting from out-of-order plug or unplug, this patch
> > is intended as a last resort if no easy, risk-free and elegant solution
> > emerges before 2.7 dev cycle ends.  
> 
> I'm not too worried about succeeding only on contiguous ids, as that has
> been something libvirt has already had to deal with.  But I am a bit
> worried about whether it is easy to introspect whether 2.8 adds a way to
> hot-plug (or hot-remove) cpus in arbitrary order, with gaps rather than
> contiguous ids, especially if the interface does not gain any obvious
> parameters.
> 
> If we are going to enhance the interface in the future, do we have any
> plans how to make it easily detectable which version of the interface we
> are working with (contiguous-only, or full power)?
We are discussing on list if we should keep limitation for now and later
add means to detect full vs limited interface or go other route and
revert limitation patch exposing full interface. Lets wait for feedback
from David.

Anyway this patch should be applied to be symmetric with what spapr does
and if we decide to lift limitation then we should revert it for both
spapr and x86.

Re: [Qemu-devel] [PATCH v4 05/16] pc: enforce adding CPUs contiguously and removing them in opposit order

[Eduardo Habkost]

On Tue, Jul 19, 2016 at 02:25:19PM +0200, Igor Mammedov wrote:
> On Mon, 18 Jul 2016 15:05:37 -0600
> Eric Blake <eblake@redhat.com> wrote:
> 
> > On 07/14/2016 10:54 AM, Igor Mammedov wrote:
> > 
> > s/opposit/opposite/ in the subject line, but it's already long. I wonder
> > if you can go shorter, with:
> > 
> > pc: enforce CPU add/remove in contiguous order
> > 
> > > it will still allow us to use cpu_index as migration instance_id
> > > since when CPUs are added contiguously (from the first to the last)
> > > and removed in opposite order, cpu_index stays stable and it's
> > > reproducable on destination side.  
> > 
> > s/reproducable/reproducible/
> Eduardo could you fix it up when applying or should I respin it?

I can fix it.

-- 
Eduardo

[Qemu-devel] [PATCH v4 06/16] pc: cpu: allow device_add to be used with x86 cpu

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 91396e8..c6d8704 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3397,6 +3397,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
     cc->cpu_exec_enter = x86_cpu_exec_enter;
     cc->cpu_exec_exit = x86_cpu_exec_exit;
 
+    dc->cannot_instantiate_with_device_add_yet = false;
     /*
      * Reason: x86_cpu_initfn() calls cpu_exec_init(), which saves the
      * object in cpus -> dangling pointer after final object_unref().
-- 
2.7.4

[Qemu-devel] [PATCH v4 07/16] pc: implement query-hotpluggable-cpus callback

[Igor Mammedov]

it returns a list of present/possible to hotplug CPU
objects with a list of properties to use with
device_add.

in PC case returned list would looks like:
-> { "execute": "query-hotpluggable-cpus" }
<- {"return": [
     {
        "type": "qemu64-x86_64-cpu", "vcpus-count": 1,
        "props": {"core-id": 0, "socket-id": 1, "thread-id": 0}
     },
     {
        "qom-path": "/machine/unattached/device[0]",
        "type": "qemu64-x86_64-cpu", "vcpus-count": 1,
        "props": {"core-id": 0, "socket-id": 0, "thread-id": 0}
     }
   ]}

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
v2:
 - add -id suffix to socket/core/thread properties to match fixed schema
---
 hw/i386/pc.c    | 45 +++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx | 15 +++++++++++++++
 2 files changed, 60 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 75a92d0..feb71f9 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2184,6 +2184,50 @@ static CPUArchIdList *pc_possible_cpu_arch_ids(MachineState *machine)
     return list;
 }
 
+static HotpluggableCPUList *pc_query_hotpluggable_cpus(MachineState *machine)
+{
+    int i;
+    CPUState *cpu;
+    HotpluggableCPUList *head = NULL;
+    PCMachineState *pcms = PC_MACHINE(machine);
+    const char *cpu_type;
+
+    cpu = pcms->possible_cpus->cpus[0].cpu;
+    assert(cpu); /* BSP is always present */
+    cpu_type = object_class_get_name(OBJECT_CLASS(CPU_GET_CLASS(cpu)));
+
+    for (i = 0; i < pcms->possible_cpus->len; i++) {
+        X86CPUTopoInfo topo;
+        HotpluggableCPUList *list_item = g_new0(typeof(*list_item), 1);
+        HotpluggableCPU *cpu_item = g_new0(typeof(*cpu_item), 1);
+        CpuInstanceProperties *cpu_props = g_new0(typeof(*cpu_props), 1);
+        const uint32_t apic_id = pcms->possible_cpus->cpus[i].arch_id;
+
+        x86_topo_ids_from_apicid(apic_id, smp_cores, smp_threads, &topo);
+
+        cpu_item->type = g_strdup(cpu_type);
+        cpu_item->vcpus_count = 1;
+        cpu_props->has_socket_id = true;
+        cpu_props->socket_id = topo.pkg_id;
+        cpu_props->has_core_id = true;
+        cpu_props->core_id = topo.core_id;
+        cpu_props->has_thread_id = true;
+        cpu_props->thread_id = topo.smt_id;
+        cpu_item->props = cpu_props;
+
+        cpu = pcms->possible_cpus->cpus[i].cpu;
+        if (cpu) {
+            cpu_item->has_qom_path = true;
+            cpu_item->qom_path = object_get_canonical_path(OBJECT(cpu));
+        }
+
+        list_item->value = cpu_item;
+        list_item->next = head;
+        head = list_item;
+    }
+    return head;
+}
+
 static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
 {
     /* cpu index isn't used */
@@ -2224,6 +2268,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     mc->get_hotplug_handler = pc_get_hotpug_handler;
     mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id;
     mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids;
+    mc->query_hotpluggable_cpus = pc_query_hotpluggable_cpus;
     mc->default_boot_order = "cad";
     mc->hot_add_cpu = pc_hot_add_cpu;
     mc->max_cpus = 255;
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6937e83..3afe066 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4983,3 +4983,18 @@ Example for pseries machine type started with
      { "props": { "core-id": 0 }, "type": "POWER8-spapr-cpu-core",
        "vcpus-count": 1, "qom-path": "/machine/unattached/device[0]"}
    ]}'
+
+Example for pc machine type started with
+-smp 1,maxcpus=2:
+    -> { "execute": "query-hotpluggable-cpus" }
+    <- {"return": [
+         {
+            "type": "qemu64-x86_64-cpu", "vcpus-count": 1,
+            "props": {"core-id": 0, "socket-id": 1, "thread-id": 0}
+         },
+         {
+            "qom-path": "/machine/unattached/device[0]",
+            "type": "qemu64-x86_64-cpu", "vcpus-count": 1,
+            "props": {"core-id": 0, "socket-id": 0, "thread-id": 0}
+         }
+       ]}
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 07/16] pc: implement query-hotpluggable-cpus callback

[Michael S. Tsirkin]

On Thu, Jul 14, 2016 at 06:54:36PM +0200, Igor Mammedov wrote:
> it returns a list of present/possible to hotplug CPU
> objects with a list of properties to use with
> device_add.
> 
> in PC case returned list would looks like:
> -> { "execute": "query-hotpluggable-cpus" }
> <- {"return": [
>      {
>         "type": "qemu64-x86_64-cpu", "vcpus-count": 1,
>         "props": {"core-id": 0, "socket-id": 1, "thread-id": 0}
>      },
>      {
>         "qom-path": "/machine/unattached/device[0]",
>         "type": "qemu64-x86_64-cpu", "vcpus-count": 1,
>         "props": {"core-id": 0, "socket-id": 0, "thread-id": 0}
>      }
>    ]}
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Shouldn't QMP maintainer be Cc'd?

> ---
> v2:
>  - add -id suffix to socket/core/thread properties to match fixed schema
> ---
>  hw/i386/pc.c    | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx | 15 +++++++++++++++
>  2 files changed, 60 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 75a92d0..feb71f9 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2184,6 +2184,50 @@ static CPUArchIdList *pc_possible_cpu_arch_ids(MachineState *machine)
>      return list;
>  }
>  
> +static HotpluggableCPUList *pc_query_hotpluggable_cpus(MachineState *machine)
> +{
> +    int i;
> +    CPUState *cpu;
> +    HotpluggableCPUList *head = NULL;
> +    PCMachineState *pcms = PC_MACHINE(machine);
> +    const char *cpu_type;
> +
> +    cpu = pcms->possible_cpus->cpus[0].cpu;
> +    assert(cpu); /* BSP is always present */
> +    cpu_type = object_class_get_name(OBJECT_CLASS(CPU_GET_CLASS(cpu)));
> +
> +    for (i = 0; i < pcms->possible_cpus->len; i++) {
> +        X86CPUTopoInfo topo;
> +        HotpluggableCPUList *list_item = g_new0(typeof(*list_item), 1);
> +        HotpluggableCPU *cpu_item = g_new0(typeof(*cpu_item), 1);
> +        CpuInstanceProperties *cpu_props = g_new0(typeof(*cpu_props), 1);
> +        const uint32_t apic_id = pcms->possible_cpus->cpus[i].arch_id;
> +
> +        x86_topo_ids_from_apicid(apic_id, smp_cores, smp_threads, &topo);
> +
> +        cpu_item->type = g_strdup(cpu_type);
> +        cpu_item->vcpus_count = 1;
> +        cpu_props->has_socket_id = true;
> +        cpu_props->socket_id = topo.pkg_id;
> +        cpu_props->has_core_id = true;
> +        cpu_props->core_id = topo.core_id;
> +        cpu_props->has_thread_id = true;
> +        cpu_props->thread_id = topo.smt_id;
> +        cpu_item->props = cpu_props;
> +
> +        cpu = pcms->possible_cpus->cpus[i].cpu;
> +        if (cpu) {
> +            cpu_item->has_qom_path = true;
> +            cpu_item->qom_path = object_get_canonical_path(OBJECT(cpu));
> +        }
> +
> +        list_item->value = cpu_item;
> +        list_item->next = head;
> +        head = list_item;
> +    }
> +    return head;
> +}
> +
>  static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
>  {
>      /* cpu index isn't used */
> @@ -2224,6 +2268,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      mc->get_hotplug_handler = pc_get_hotpug_handler;
>      mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id;
>      mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids;
> +    mc->query_hotpluggable_cpus = pc_query_hotpluggable_cpus;
>      mc->default_boot_order = "cad";
>      mc->hot_add_cpu = pc_hot_add_cpu;
>      mc->max_cpus = 255;
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 6937e83..3afe066 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -4983,3 +4983,18 @@ Example for pseries machine type started with
>       { "props": { "core-id": 0 }, "type": "POWER8-spapr-cpu-core",
>         "vcpus-count": 1, "qom-path": "/machine/unattached/device[0]"}
>     ]}'
> +
> +Example for pc machine type started with
> +-smp 1,maxcpus=2:
> +    -> { "execute": "query-hotpluggable-cpus" }
> +    <- {"return": [
> +         {
> +            "type": "qemu64-x86_64-cpu", "vcpus-count": 1,
> +            "props": {"core-id": 0, "socket-id": 1, "thread-id": 0}
> +         },
> +         {
> +            "qom-path": "/machine/unattached/device[0]",
> +            "type": "qemu64-x86_64-cpu", "vcpus-count": 1,
> +            "props": {"core-id": 0, "socket-id": 0, "thread-id": 0}
> +         }
> +       ]}
> -- 
> 2.7.4

[Qemu-devel] [PATCH v4 08/16] apic: move MAX_APICS check to 'apic' class

[Igor Mammedov]

MAX_APICS is only used by child 'apic' class and not
by its parent TYPE_APIC_COMMON or any other derived
class.
Move check into end user 'apic' class so it won't
get in the way of other APIC implementations
if they support more then MAX_APICS.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: Radim Krčmář <rkrcmar@redhat.com>
CC: pbonzini@redhat.com
---
 include/hw/i386/apic_internal.h |  4 +---
 hw/intc/apic.c                  | 10 ++++++++++
 hw/intc/apic_common.c           |  8 --------
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 73ce716..67348e9 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -121,8 +121,6 @@
 #define VAPIC_ENABLE_BIT                0
 #define VAPIC_ENABLE_MASK               (1 << VAPIC_ENABLE_BIT)
 
-#define MAX_APICS 255
-
 typedef struct APICCommonState APICCommonState;
 
 #define TYPE_APIC_COMMON "apic-common"
@@ -176,7 +174,7 @@ struct APICCommonState {
     uint32_t initial_count;
     int64_t initial_count_load_time;
     int64_t next_time;
-    int idx;
+    int idx; /* not actually common, used only by 'apic' derived class */
     QEMUTimer *timer;
     int64_t timer_expiry;
     int sipi_vector;
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index e1ab935..b0d237b 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -28,7 +28,9 @@
 #include "trace.h"
 #include "hw/i386/pc.h"
 #include "hw/i386/apic-msidef.h"
+#include "qapi/error.h"
 
+#define MAX_APICS 255
 #define MAX_APIC_WORDS 8
 
 #define SYNC_FROM_VAPIC                 0x1
@@ -869,6 +871,14 @@ static const MemoryRegionOps apic_io_ops = {
 static void apic_realize(DeviceState *dev, Error **errp)
 {
     APICCommonState *s = APIC_COMMON(dev);
+    static int apic_no;
+
+    if (apic_no >= MAX_APICS) {
+        error_setg(errp, "%s initialization failed.",
+                   object_get_typename(OBJECT(dev)));
+        return;
+    }
+    s->idx = apic_no++;
 
     memory_region_init_io(&s->io_memory, OBJECT(s), &apic_io_ops, s, "apic-msi",
                           APIC_SPACE_SIZE);
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index e6eb694..fd425d1 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -299,14 +299,6 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
     APICCommonState *s = APIC_COMMON(dev);
     APICCommonClass *info;
     static DeviceState *vapic;
-    static int apic_no;
-
-    if (apic_no >= MAX_APICS) {
-        error_setg(errp, "%s initialization failed.",
-                   object_get_typename(OBJECT(dev)));
-        return;
-    }
-    s->idx = apic_no++;
 
     info = APIC_COMMON_GET_CLASS(s);
     info->realize(dev, errp);
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 08/16] apic: move MAX_APICS check to 'apic' class

[Radim Krčmář]

2016-07-14 18:54+0200, Igor Mammedov:
> MAX_APICS is only used by child 'apic' class and not
> by its parent TYPE_APIC_COMMON or any other derived
> class.
> Move check into end user 'apic' class so it won't
> get in the way of other APIC implementations
> if they support more then MAX_APICS.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---

The other two APIC implementations, for KVM a Xen, don't seem to need
this extra check.

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

> CC: Radim Krčmář <rkrcmar@redhat.com>
> CC: pbonzini@redhat.com
> ---
>  include/hw/i386/apic_internal.h |  4 +---
>  hw/intc/apic.c                  | 10 ++++++++++
>  hw/intc/apic_common.c           |  8 --------
>  3 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
> index 73ce716..67348e9 100644
> --- a/include/hw/i386/apic_internal.h
> +++ b/include/hw/i386/apic_internal.h
> @@ -121,8 +121,6 @@
>  #define VAPIC_ENABLE_BIT                0
>  #define VAPIC_ENABLE_MASK               (1 << VAPIC_ENABLE_BIT)
>  
> -#define MAX_APICS 255
> -
>  typedef struct APICCommonState APICCommonState;
>  
>  #define TYPE_APIC_COMMON "apic-common"
> @@ -176,7 +174,7 @@ struct APICCommonState {
>      uint32_t initial_count;
>      int64_t initial_count_load_time;
>      int64_t next_time;
> -    int idx;
> +    int idx; /* not actually common, used only by 'apic' derived class */
>      QEMUTimer *timer;
>      int64_t timer_expiry;
>      int sipi_vector;
> diff --git a/hw/intc/apic.c b/hw/intc/apic.c
> index e1ab935..b0d237b 100644
> --- a/hw/intc/apic.c
> +++ b/hw/intc/apic.c
> @@ -28,7 +28,9 @@
>  #include "trace.h"
>  #include "hw/i386/pc.h"
>  #include "hw/i386/apic-msidef.h"
> +#include "qapi/error.h"
>  
> +#define MAX_APICS 255
>  #define MAX_APIC_WORDS 8
>  
>  #define SYNC_FROM_VAPIC                 0x1
> @@ -869,6 +871,14 @@ static const MemoryRegionOps apic_io_ops = {
>  static void apic_realize(DeviceState *dev, Error **errp)
>  {
>      APICCommonState *s = APIC_COMMON(dev);
> +    static int apic_no;
> +
> +    if (apic_no >= MAX_APICS) {
> +        error_setg(errp, "%s initialization failed.",
> +                   object_get_typename(OBJECT(dev)));
> +        return;
> +    }
> +    s->idx = apic_no++;
>  
>      memory_region_init_io(&s->io_memory, OBJECT(s), &apic_io_ops, s, "apic-msi",
>                            APIC_SPACE_SIZE);
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index e6eb694..fd425d1 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -299,14 +299,6 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
>      APICCommonState *s = APIC_COMMON(dev);
>      APICCommonClass *info;
>      static DeviceState *vapic;
> -    static int apic_no;
> -
> -    if (apic_no >= MAX_APICS) {
> -        error_setg(errp, "%s initialization failed.",
> -                   object_get_typename(OBJECT(dev)));
> -        return;
> -    }
> -    s->idx = apic_no++;
>  
>      info = APIC_COMMON_GET_CLASS(s);
>      info->realize(dev, errp);
> -- 
> 2.7.4
> 

[Qemu-devel] [PATCH v4 09/16] apic: drop APICCommonState.idx and use APIC ID as index in local_apics[]

[Igor Mammedov]

local_apics[] is sized to contain all APIC ID supported in xAPIC mode,
so use APIC ID as index in it instead of constantly increasing counter idx.

Fixes error "apic initialization failed" when a CPU hotplugged and
unplugged more times than there are free slots in local_apics[].

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: Radim Krčmář <rkrcmar@redhat.com>
CC: pbonzini@redhat.com
---
 include/hw/i386/apic_internal.h |  1 -
 hw/intc/apic.c                  | 16 +++++++---------
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 67348e9..8330592 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -174,7 +174,6 @@ struct APICCommonState {
     uint32_t initial_count;
     int64_t initial_count_load_time;
     int64_t next_time;
-    int idx; /* not actually common, used only by 'apic' derived class */
     QEMUTimer *timer;
     int64_t timer_expiry;
     int sipi_vector;
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index b0d237b..f473572 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -421,7 +421,7 @@ static int apic_find_dest(uint8_t dest)
     int i;
 
     if (apic && apic->id == dest)
-        return dest;  /* shortcut in case apic->id == apic->idx */
+        return dest;  /* shortcut in case apic->id == local_apics[dest]->id */
 
     for (i = 0; i < MAX_APICS; i++) {
         apic = local_apics[i];
@@ -504,14 +504,14 @@ static void apic_deliver(DeviceState *dev, uint8_t dest, uint8_t dest_mode,
         break;
     case 1:
         memset(deliver_bitmask, 0x00, sizeof(deliver_bitmask));
-        apic_set_bit(deliver_bitmask, s->idx);
+        apic_set_bit(deliver_bitmask, s->id);
         break;
     case 2:
         memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
         break;
     case 3:
         memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
-        apic_reset_bit(deliver_bitmask, s->idx);
+        apic_reset_bit(deliver_bitmask, s->id);
         break;
     }
 
@@ -871,20 +871,18 @@ static const MemoryRegionOps apic_io_ops = {
 static void apic_realize(DeviceState *dev, Error **errp)
 {
     APICCommonState *s = APIC_COMMON(dev);
-    static int apic_no;
 
-    if (apic_no >= MAX_APICS) {
-        error_setg(errp, "%s initialization failed.",
-                   object_get_typename(OBJECT(dev)));
+    if (s->id >= MAX_APICS) {
+        error_setg(errp, "%s initialization failed. APIC ID %d is invalid",
+                   object_get_typename(OBJECT(dev)), s->id);
         return;
     }
-    s->idx = apic_no++;
 
     memory_region_init_io(&s->io_memory, OBJECT(s), &apic_io_ops, s, "apic-msi",
                           APIC_SPACE_SIZE);
 
     s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, apic_timer, s);
-    local_apics[s->idx] = s;
+    local_apics[s->id] = s;
 
     msi_nonbroken = true;
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 09/16] apic: drop APICCommonState.idx and use APIC ID as index in local_apics[]

[Radim Krčmář]

2016-07-14 18:54+0200, Igor Mammedov:
> local_apics[] is sized to contain all APIC ID supported in xAPIC mode,
> so use APIC ID as index in it instead of constantly increasing counter idx.
> 
> Fixes error "apic initialization failed" when a CPU hotplugged and
> unplugged more times than there are free slots in local_apics[].
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---

The new method handles dynamic APIC ID changes made by the guest and id
is set to the initial apic_id when realize is called, so it's even
better than the previous one as we have higher chance of hitting the
correct apic in local_apics[].

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

> CC: Radim Krčmář <rkrcmar@redhat.com>
> CC: pbonzini@redhat.com
> ---
>  include/hw/i386/apic_internal.h |  1 -
>  hw/intc/apic.c                  | 16 +++++++---------
>  2 files changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
> index 67348e9..8330592 100644
> --- a/include/hw/i386/apic_internal.h
> +++ b/include/hw/i386/apic_internal.h
> @@ -174,7 +174,6 @@ struct APICCommonState {
>      uint32_t initial_count;
>      int64_t initial_count_load_time;
>      int64_t next_time;
> -    int idx; /* not actually common, used only by 'apic' derived class */
>      QEMUTimer *timer;
>      int64_t timer_expiry;
>      int sipi_vector;
> diff --git a/hw/intc/apic.c b/hw/intc/apic.c
> index b0d237b..f473572 100644
> --- a/hw/intc/apic.c
> +++ b/hw/intc/apic.c
> @@ -421,7 +421,7 @@ static int apic_find_dest(uint8_t dest)
>      int i;
>  
>      if (apic && apic->id == dest)
> -        return dest;  /* shortcut in case apic->id == apic->idx */
> +        return dest;  /* shortcut in case apic->id == local_apics[dest]->id */
>  
>      for (i = 0; i < MAX_APICS; i++) {
>          apic = local_apics[i];

(We could also update local_apics[] when APIC ID changes and drop the
 loop, but it's safer this way.)

> @@ -504,14 +504,14 @@ static void apic_deliver(DeviceState *dev, uint8_t dest, uint8_t dest_mode,
>          break;
>      case 1:
>          memset(deliver_bitmask, 0x00, sizeof(deliver_bitmask));
> -        apic_set_bit(deliver_bitmask, s->idx);
> +        apic_set_bit(deliver_bitmask, s->id);
>          break;
>      case 2:
>          memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
>          break;
>      case 3:
>          memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
> -        apic_reset_bit(deliver_bitmask, s->idx);
> +        apic_reset_bit(deliver_bitmask, s->id);
>          break;
>      }
>  
> @@ -871,20 +871,18 @@ static const MemoryRegionOps apic_io_ops = {
>  static void apic_realize(DeviceState *dev, Error **errp)
>  {
>      APICCommonState *s = APIC_COMMON(dev);
> -    static int apic_no;
>  
> -    if (apic_no >= MAX_APICS) {
> -        error_setg(errp, "%s initialization failed.",
> -                   object_get_typename(OBJECT(dev)));
> +    if (s->id >= MAX_APICS) {
> +        error_setg(errp, "%s initialization failed. APIC ID %d is invalid",
> +                   object_get_typename(OBJECT(dev)), s->id);
>          return;
>      }
> -    s->idx = apic_no++;
>  
>      memory_region_init_io(&s->io_memory, OBJECT(s), &apic_io_ops, s, "apic-msi",
>                            APIC_SPACE_SIZE);
>  
>      s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, apic_timer, s);
> -    local_apics[s->idx] = s;
> +    local_apics[s->id] = s;
>  
>      msi_nonbroken = true;
>  }
> -- 
> 2.7.4
> 

[Qemu-devel] [PATCH v4 10/16] apic: kvm-apic: fix crash due to access to freed memory region

[Igor Mammedov]

kvm-apic.io_memory memory region had its parent set to NULL at
memory_region_init_io() time, so it ended up as a child in
 /unattached contaner.
As result when kvm-apic instance was deleted, the child property
 /unattached/kvm-apic-msi[XXX] contained a reference to
kvm-apic.io_memory address which was freed as part of kvm-apic.

Do the same as 'apic' and make kvm-apic instance the owner
of the memory region so that it won't end up in /unattached
and gets cleanly released along with related kvm-apic instance.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: Radim Krčmář <rkrcmar@redhat.com>
CC: pbonzini@redhat.com
---
 hw/i386/kvm/apic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index c5983c7..1f87e73 100644
--- a/hw/i386/kvm/apic.c
+++ b/hw/i386/kvm/apic.c
@@ -184,8 +184,8 @@ static void kvm_apic_realize(DeviceState *dev, Error **errp)
 {
     APICCommonState *s = APIC_COMMON(dev);
 
-    memory_region_init_io(&s->io_memory, NULL, &kvm_apic_io_ops, s, "kvm-apic-msi",
-                          APIC_SPACE_SIZE);
+    memory_region_init_io(&s->io_memory, OBJECT(s), &kvm_apic_io_ops, s,
+                          "kvm-apic-msi", APIC_SPACE_SIZE);
 
     if (kvm_has_gsi_routing()) {
         msi_nonbroken = true;
-- 
2.7.4

[Qemu-devel] [PATCH v4 11/16] (kvm)apic: add unrealize callbacks

[Igor Mammedov]

callbacks will do necessary cleanups before APIC device is deleted

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: pbonzini@redhat.com
---
 include/hw/i386/apic_internal.h |  1 +
 hw/i386/kvm/apic.c              |  5 +++++
 hw/intc/apic.c                  | 10 ++++++++++
 hw/intc/apic_common.c           | 13 +++++++++++++
 4 files changed, 29 insertions(+)

diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 8330592..e549a62 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -136,6 +136,7 @@ typedef struct APICCommonClass
     DeviceClass parent_class;
 
     DeviceRealize realize;
+    DeviceUnrealize unrealize;
     void (*set_base)(APICCommonState *s, uint64_t val);
     void (*set_tpr)(APICCommonState *s, uint8_t val);
     uint8_t (*get_tpr)(APICCommonState *s);
diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index 1f87e73..2bd0de8 100644
--- a/hw/i386/kvm/apic.c
+++ b/hw/i386/kvm/apic.c
@@ -192,11 +192,16 @@ static void kvm_apic_realize(DeviceState *dev, Error **errp)
     }
 }
 
+static void kvm_apic_unrealize(DeviceState *dev, Error **errp)
+{
+}
+
 static void kvm_apic_class_init(ObjectClass *klass, void *data)
 {
     APICCommonClass *k = APIC_COMMON_CLASS(klass);
 
     k->realize = kvm_apic_realize;
+    k->unrealize = kvm_apic_unrealize;
     k->reset = kvm_apic_reset;
     k->set_base = kvm_apic_set_base;
     k->set_tpr = kvm_apic_set_tpr;
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index f473572..45887d9 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -887,11 +887,21 @@ static void apic_realize(DeviceState *dev, Error **errp)
     msi_nonbroken = true;
 }
 
+static void apic_unrealize(DeviceState *dev, Error **errp)
+{
+    APICCommonState *s = APIC_COMMON(dev);
+
+    timer_del(s->timer);
+    timer_free(s->timer);
+    local_apics[s->id] = NULL;
+}
+
 static void apic_class_init(ObjectClass *klass, void *data)
 {
     APICCommonClass *k = APIC_COMMON_CLASS(klass);
 
     k->realize = apic_realize;
+    k->unrealize = apic_unrealize;
     k->set_base = apic_set_base;
     k->set_tpr = apic_set_tpr;
     k->get_tpr = apic_get_tpr;
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index fd425d1..0bb9ad5 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -315,6 +315,18 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
 
 }
 
+static void apic_common_unrealize(DeviceState *dev, Error **errp)
+{
+    APICCommonState *s = APIC_COMMON(dev);
+    APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
+
+    info->unrealize(dev, errp);
+
+    if (apic_report_tpr_access && info->enable_tpr_reporting) {
+        info->enable_tpr_reporting(s, false);
+    }
+}
+
 static int apic_pre_load(void *opaque)
 {
     APICCommonState *s = APIC_COMMON(opaque);
@@ -421,6 +433,7 @@ static void apic_common_class_init(ObjectClass *klass, void *data)
     dc->reset = apic_reset_common;
     dc->props = apic_properties_common;
     dc->realize = apic_common_realize;
+    dc->unrealize = apic_common_unrealize;
     /*
      * Reason: APIC and CPU need to be wired up by
      * x86_cpu_apic_create()
-- 
2.7.4

[Qemu-devel] [PATCH v4 12/16] apic: use apic_id as apic's migration instance_id

[Igor Mammedov]

instance_id is generated by last_used_id + 1 for a given device type
so for QEMU with 3 CPUs instance_id for APICs is a seti of [0, 1, 2]
When CPU in the middle is hot-removed and migration started
APICs with instance_ids 0 and 2 are transferred in migration stream.
However target starts with 2 CPUs and APICs' instance_ids are
generated from scratch [0, 1] hence migration fails with error
  Unknown savevm section or instance 'apic' 2

Fix issue by manually registering APIC's vmsd with apic_id as
instance_id, in this case instance_id on target will always
match instance_id on source as apic_id is the same for a given
cpu instance.

Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
CC: pbonzini@redhat.com
---
 include/hw/i386/apic_internal.h |  1 +
 include/hw/i386/pc.h            |  5 +++++
 hw/intc/apic_common.c           | 12 +++++++++++-
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index e549a62..06c4e9f 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -183,6 +183,7 @@ struct APICCommonState {
     uint32_t vapic_control;
     DeviceState *vapic;
     hwaddr vapic_paddr; /* note: persistence via kvmvapic */
+    bool legacy_instance_id;
 };
 
 typedef struct VAPICState {
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 7d9d03c..dc82e29 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -378,6 +378,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
         .driver = TYPE_X86_CPU,\
         .property = "fill-mtrr-mask",\
         .value = "off",\
+    },\
+    {\
+        .driver   = "apic",\
+        .property = "legacy-instance-id",\
+        .value    = "on",\
     },
 
 #define PC_COMPAT_2_5 \
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 0bb9ad5..14ac43c 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -294,11 +294,14 @@ static int apic_load_old(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
+static const VMStateDescription vmstate_apic_common;
+
 static void apic_common_realize(DeviceState *dev, Error **errp)
 {
     APICCommonState *s = APIC_COMMON(dev);
     APICCommonClass *info;
     static DeviceState *vapic;
+    int instance_id = s->id;
 
     info = APIC_COMMON_GET_CLASS(s);
     info->realize(dev, errp);
@@ -313,6 +316,11 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
         info->enable_tpr_reporting(s, true);
     }
 
+    if (s->legacy_instance_id) {
+        instance_id = -1;
+    }
+    vmstate_register_with_alias_id(NULL, instance_id, &vmstate_apic_common,
+                                   s, -1, 0);
 }
 
 static void apic_common_unrealize(DeviceState *dev, Error **errp)
@@ -320,6 +328,7 @@ static void apic_common_unrealize(DeviceState *dev, Error **errp)
     APICCommonState *s = APIC_COMMON(dev);
     APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
 
+    vmstate_unregister(NULL, &vmstate_apic_common, s);
     info->unrealize(dev, errp);
 
     if (apic_report_tpr_access && info->enable_tpr_reporting) {
@@ -422,6 +431,8 @@ static Property apic_properties_common[] = {
     DEFINE_PROP_UINT8("version", APICCommonState, version, 0x14),
     DEFINE_PROP_BIT("vapic", APICCommonState, vapic_control, VAPIC_ENABLE_BIT,
                     true),
+    DEFINE_PROP_BOOL("legacy-instance-id", APICCommonState, legacy_instance_id,
+                     false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -429,7 +440,6 @@ static void apic_common_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
-    dc->vmsd = &vmstate_apic_common;
     dc->reset = apic_reset_common;
     dc->props = apic_properties_common;
     dc->realize = apic_common_realize;
-- 
2.7.4

[Qemu-devel] [PATCH v4 13/16] target-i386: cpu: do not ignore error and fix apic parent

[Igor Mammedov]

object_property_add_child() silently fails with error that it can't
create duplicate propery 'apic' as we already have 'apic' property
registered for 'apic' feature. As result generic device_realize puts
apic into unattached container.

As it's programming error, abort if name collision happens in future
and fix property name for apic_state to 'lapic', this way apic is
a child of cpu instance.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c6d8704..fb528de 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2829,8 +2829,9 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
 
     cpu->apic_state = DEVICE(object_new(apic_type));
 
-    object_property_add_child(OBJECT(cpu), "apic",
-                              OBJECT(cpu->apic_state), NULL);
+    object_property_add_child(OBJECT(cpu), "lapic",
+                              OBJECT(cpu->apic_state), &error_abort);
+
     qdev_prop_set_uint8(cpu->apic_state, "id", cpu->apic_id);
     /* TODO: convert to link<> */
     apic = APIC_COMMON(cpu->apic_state);
-- 
2.7.4

[Qemu-devel] [PATCH v4 14/16] target-i386: fix apic object leak when CPU is deleted

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index fb528de..73ed447 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2831,6 +2831,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
 
     object_property_add_child(OBJECT(cpu), "lapic",
                               OBJECT(cpu->apic_state), &error_abort);
+    object_unref(OBJECT(cpu->apic_state));
 
     qdev_prop_set_uint8(cpu->apic_state, "id", cpu->apic_id);
     /* TODO: convert to link<> */
-- 
2.7.4

[Qemu-devel] [PATCH v4 15/16] target-i386: add x86_cpu_unrealizefn()

[Igor Mammedov]

first remove VCPU from exec loop and only then remove lapic.

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 73ed447..f904671 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3117,6 +3117,21 @@ out:
     }
 }
 
+static void x86_cpu_unrealizefn(DeviceState *dev, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(dev);
+
+#ifndef CONFIG_USER_ONLY
+    cpu_remove_sync(CPU(dev));
+    qemu_unregister_reset(x86_cpu_machine_reset_cb, dev);
+#endif
+
+    if (cpu->apic_state) {
+        object_unparent(OBJECT(cpu->apic_state));
+        cpu->apic_state = NULL;
+    }
+}
+
 typedef struct BitProperty {
     uint32_t *ptr;
     uint32_t mask;
@@ -3363,6 +3378,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
 
     xcc->parent_realize = dc->realize;
     dc->realize = x86_cpu_realizefn;
+    dc->unrealize = x86_cpu_unrealizefn;
     dc->props = x86_cpu_properties;
 
     xcc->parent_reset = cc->reset;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 15/16] target-i386: add x86_cpu_unrealizefn()

[Eduardo Habkost]

On Thu, Jul 14, 2016 at 06:54:44PM +0200, Igor Mammedov wrote:
> first remove VCPU from exec loop and only then remove lapic.
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

> ---
>  target-i386/cpu.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 73ed447..f904671 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -3117,6 +3117,21 @@ out:
>      }
>  }
>  
> +static void x86_cpu_unrealizefn(DeviceState *dev, Error **errp)
> +{
> +    X86CPU *cpu = X86_CPU(dev);
> +
> +#ifndef CONFIG_USER_ONLY
> +    cpu_remove_sync(CPU(dev));
> +    qemu_unregister_reset(x86_cpu_machine_reset_cb, dev);
> +#endif
> +
> +    if (cpu->apic_state) {
> +        object_unparent(OBJECT(cpu->apic_state));
> +        cpu->apic_state = NULL;
> +    }
> +}
> +
>  typedef struct BitProperty {
>      uint32_t *ptr;
>      uint32_t mask;
> @@ -3363,6 +3378,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>  
>      xcc->parent_realize = dc->realize;
>      dc->realize = x86_cpu_realizefn;
> +    dc->unrealize = x86_cpu_unrealizefn;
>      dc->props = x86_cpu_properties;
>  
>      xcc->parent_reset = cc->reset;
> -- 
> 2.7.4
> 
> 

-- 
Eduardo

[Qemu-devel] [PATCH v4 16/16] pc: make device_del CPU work for x86 CPUs

[Igor Mammedov]

ACPI subsystem already has all logic in place the only
thing left to eject CPU is destroy it and ammend
present CPUs counter in CMOS, do so.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index feb71f9..dec74c9 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1794,6 +1794,7 @@ static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
 static void pc_cpu_unplug_cb(HotplugHandler *hotplug_dev,
                              DeviceState *dev, Error **errp)
 {
+    CPUArchId *found_cpu;
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
@@ -1805,13 +1806,11 @@ static void pc_cpu_unplug_cb(HotplugHandler *hotplug_dev,
         goto out;
     }
 
-    /*
-     * TODO: enable unplug once generic CPU remove bits land
-     * for now guest will be able to eject CPU ACPI wise but
-     * it will come back again on machine reset.
-     */
-    /*  object_unparent(OBJECT(dev)); */
+    found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL);
+    found_cpu->cpu = NULL;
+    object_unparent(OBJECT(dev));
 
+    rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) - 1);
  out:
     error_propagate(errp, local_err);
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 00/16] pc: add CPU hot-add/hot-remove with device_add/device_del

[Michael S. Tsirkin]

On Thu, Jul 14, 2016 at 06:54:29PM +0200, Igor Mammedov wrote:
> Changelog:

So for pc and apic bits:

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Since Eduardo picked up first part of this, feel free
to merge the rest too.



>   since v3:
>     * rebase on top of x86-next tree as it's already applied 1-6/19 from v3
>       (so they are could be omitted from v4)
>     * set apic-state to NULL after destroying it in unrealize()
>     * fix places where I forgot to print X86CPUTopoInfo fields as %u
>     * move "pc: implement query-hotpluggable-cpus callback" after
>       patch that enables device_add cpu
>     * extract counting of present cpus in possible_cpus into a separate helper
>     * newly added patches:
>         * pc: forbid BSP removal
>         * pc: enforce adding CPUs contiguously and removing them in opposit order
>         * apic: kvm-apic: fix crash due to access to freed memory region
>     * update Reviewed-bys
>   since v2:
>     * use 0xFFFFFFFF for UNASSIGNED_APIC_ID instead of UINT32_MAX
>     * add comment why 0xFFFFFFFF could be used for UNASSIGNED_APIC_ID
>     * print topo ids is unsigned
>     * print APIC ID as hex
>     * print topo ids calculated from APIC ID beside it
>     * add extra patch to fix migration failure due to APIC's instance_id mismatch
>   since v1:
>     * s/pc_find_cpu/pc_find_cpu_slot/ + add comment to it
>     * add more sanity checks for socket-id/core-id/thread-id and 'apic'
>       properties
>     * include device_del cpu patches and related fixes to x86 CPU/apic
> 
> Series enabling usage of -device/device_add for adding CPUs as devices
> and device_del for removing them. Using -device/device_add in combination
> with query-hotpluggable-cpus QMP command allows to hotplug CPUs at any
> not used possition and then safely migrate QEMU instance by specifying
> hotadded CPUs on target with help of -device CLI option like with any
> other device.
> Having been able to replicate exact topology on taggert with -device CPUs
> also opens poosibility to hot-remove CPUs, which this series does by
> enabling to use device_del with x86 CPUs.
> 
> 
> git tree for testing:
>   https://github.com/imammedo/qemu.git dev_del_cpu_v4
> for viewing:
>   https://github.com/imammedo/qemu/commits/dev_del_cpu_v4
> 
> Tested with RHEL7.2 guest including ping/pong migration with adding/removing
> CPUs in between.
> 
> CC: pkrempa@redhat.com
> CC: ehabkost@redhat.com
> CC: mst@redhat.com
> CC: eduardo.otubo@profitbricks.com
> CC: Bandan Das <bdas@redhat.com>
> 
> Igor Mammedov (16):
>   pc: set APIC ID based on socket/core/thread ids if it's not been set
>     yet
>   pc: delay setting number of boot CPUs to machine_done time
>   pc: register created initial and hotpluged CPUs in one place
>     pc_cpu_plug()
>   pc: forbid BSP removal
>   pc: enforce adding CPUs contiguously and removing them in opposit
>     order
>   pc: cpu: allow device_add to be used with x86 cpu
>   pc: implement query-hotpluggable-cpus callback
>   apic: move MAX_APICS check to 'apic' class
>   apic: drop APICCommonState.idx and use APIC ID as index in
>     local_apics[]
>   apic: kvm-apic: fix crash due to access to freed memory region
>   (kvm)apic: add unrealize callbacks
>   apic: use apic_id as apic's migration instance_id
>   target-i386: cpu: do not ignore error and fix apic parent
>   target-i386: fix apic object leak when CPU is deleted
>   target-i386: add x86_cpu_unrealizefn()
>   pc: make device_del CPU work for x86 CPUs
> 
>  include/hw/i386/apic_internal.h |   5 +-
>  include/hw/i386/pc.h            |   5 ++
>  hw/i386/kvm/apic.c              |   9 +-
>  hw/i386/pc.c                    | 183 ++++++++++++++++++++++++++++++++++------
>  hw/intc/apic.c                  |  26 +++++-
>  hw/intc/apic_common.c           |  33 ++++++--
>  qmp-commands.hx                 |  15 ++++
>  target-i386/cpu.c               |  23 ++++-
>  8 files changed, 251 insertions(+), 48 deletions(-)
> 
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH v4 00/16] pc: add CPU hot-add/hot-remove with device_add/device_del

[Eduardo Otubo]

[-- Attachment #1: Type: text/plain, Size: 4509 bytes --]

On Tue, Jul 19, 2016 at 12=59=36AM +0300, Michael S. Tsirkin wrote:
> On Thu, Jul 14, 2016 at 06:54:29PM +0200, Igor Mammedov wrote:
> > Changelog:
> 
> So for pc and apic bits:
> 
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Since Eduardo picked up first part of this, feel free
> to merge the rest too.

Hi Igor,

I was a little curios about the state of this feature. Is this going to
make it to 2.7?

Best regards,

> 
> 
> 
> >   since v3:
> >     * rebase on top of x86-next tree as it's already applied 1-6/19 from v3
> >       (so they are could be omitted from v4)
> >     * set apic-state to NULL after destroying it in unrealize()
> >     * fix places where I forgot to print X86CPUTopoInfo fields as %u
> >     * move "pc: implement query-hotpluggable-cpus callback" after
> >       patch that enables device_add cpu
> >     * extract counting of present cpus in possible_cpus into a separate helper
> >     * newly added patches:
> >         * pc: forbid BSP removal
> >         * pc: enforce adding CPUs contiguously and removing them in opposit order
> >         * apic: kvm-apic: fix crash due to access to freed memory region
> >     * update Reviewed-bys
> >   since v2:
> >     * use 0xFFFFFFFF for UNASSIGNED_APIC_ID instead of UINT32_MAX
> >     * add comment why 0xFFFFFFFF could be used for UNASSIGNED_APIC_ID
> >     * print topo ids is unsigned
> >     * print APIC ID as hex
> >     * print topo ids calculated from APIC ID beside it
> >     * add extra patch to fix migration failure due to APIC's instance_id mismatch
> >   since v1:
> >     * s/pc_find_cpu/pc_find_cpu_slot/ + add comment to it
> >     * add more sanity checks for socket-id/core-id/thread-id and 'apic'
> >       properties
> >     * include device_del cpu patches and related fixes to x86 CPU/apic
> > 
> > Series enabling usage of -device/device_add for adding CPUs as devices
> > and device_del for removing them. Using -device/device_add in combination
> > with query-hotpluggable-cpus QMP command allows to hotplug CPUs at any
> > not used possition and then safely migrate QEMU instance by specifying
> > hotadded CPUs on target with help of -device CLI option like with any
> > other device.
> > Having been able to replicate exact topology on taggert with -device CPUs
> > also opens poosibility to hot-remove CPUs, which this series does by
> > enabling to use device_del with x86 CPUs.
> > 
> > 
> > git tree for testing:
> >   https://github.com/imammedo/qemu.git dev_del_cpu_v4
> > for viewing:
> >   https://github.com/imammedo/qemu/commits/dev_del_cpu_v4
> > 
> > Tested with RHEL7.2 guest including ping/pong migration with adding/removing
> > CPUs in between.
> > 
> > CC: pkrempa@redhat.com
> > CC: ehabkost@redhat.com
> > CC: mst@redhat.com
> > CC: eduardo.otubo@profitbricks.com
> > CC: Bandan Das <bdas@redhat.com>
> > 
> > Igor Mammedov (16):
> >   pc: set APIC ID based on socket/core/thread ids if it's not been set
> >     yet
> >   pc: delay setting number of boot CPUs to machine_done time
> >   pc: register created initial and hotpluged CPUs in one place
> >     pc_cpu_plug()
> >   pc: forbid BSP removal
> >   pc: enforce adding CPUs contiguously and removing them in opposit
> >     order
> >   pc: cpu: allow device_add to be used with x86 cpu
> >   pc: implement query-hotpluggable-cpus callback
> >   apic: move MAX_APICS check to 'apic' class
> >   apic: drop APICCommonState.idx and use APIC ID as index in
> >     local_apics[]
> >   apic: kvm-apic: fix crash due to access to freed memory region
> >   (kvm)apic: add unrealize callbacks
> >   apic: use apic_id as apic's migration instance_id
> >   target-i386: cpu: do not ignore error and fix apic parent
> >   target-i386: fix apic object leak when CPU is deleted
> >   target-i386: add x86_cpu_unrealizefn()
> >   pc: make device_del CPU work for x86 CPUs
> > 
> >  include/hw/i386/apic_internal.h |   5 +-
> >  include/hw/i386/pc.h            |   5 ++
> >  hw/i386/kvm/apic.c              |   9 +-
> >  hw/i386/pc.c                    | 183 ++++++++++++++++++++++++++++++++++------
> >  hw/intc/apic.c                  |  26 +++++-
> >  hw/intc/apic_common.c           |  33 ++++++--
> >  qmp-commands.hx                 |  15 ++++
> >  target-i386/cpu.c               |  23 ++++-
> >  8 files changed, 251 insertions(+), 48 deletions(-)
> > 
> > -- 
> > 2.7.4

-- 
Eduardo Otubo
ProfitBricks GmbH

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v4 00/16] pc: add CPU hot-add/hot-remove with device_add/device_del

[Igor Mammedov]

On Wed, 10 Aug 2016 15:56:24 +0200
Eduardo Otubo <eduardo.otubo@profitbricks.com> wrote:

> On Tue, Jul 19, 2016 at 12=59=36AM +0300, Michael S. Tsirkin wrote:
> > On Thu, Jul 14, 2016 at 06:54:29PM +0200, Igor Mammedov wrote:  
> > > Changelog:  
> > 
> > So for pc and apic bits:
> > 
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > Since Eduardo picked up first part of this, feel free
> > to merge the rest too.  
> 
> Hi Igor,
> 
> I was a little curios about the state of this feature. Is this going to
> make it to 2.7?
it's merged

> 
> Best regards,
> 
> > 
> > 
> >   
> > >   since v3:
> > >     * rebase on top of x86-next tree as it's already applied 1-6/19 from v3
> > >       (so they are could be omitted from v4)
> > >     * set apic-state to NULL after destroying it in unrealize()
> > >     * fix places where I forgot to print X86CPUTopoInfo fields as %u
> > >     * move "pc: implement query-hotpluggable-cpus callback" after
> > >       patch that enables device_add cpu
> > >     * extract counting of present cpus in possible_cpus into a separate helper
> > >     * newly added patches:
> > >         * pc: forbid BSP removal
> > >         * pc: enforce adding CPUs contiguously and removing them in opposit order
> > >         * apic: kvm-apic: fix crash due to access to freed memory region
> > >     * update Reviewed-bys
> > >   since v2:
> > >     * use 0xFFFFFFFF for UNASSIGNED_APIC_ID instead of UINT32_MAX
> > >     * add comment why 0xFFFFFFFF could be used for UNASSIGNED_APIC_ID
> > >     * print topo ids is unsigned
> > >     * print APIC ID as hex
> > >     * print topo ids calculated from APIC ID beside it
> > >     * add extra patch to fix migration failure due to APIC's instance_id mismatch
> > >   since v1:
> > >     * s/pc_find_cpu/pc_find_cpu_slot/ + add comment to it
> > >     * add more sanity checks for socket-id/core-id/thread-id and 'apic'
> > >       properties
> > >     * include device_del cpu patches and related fixes to x86 CPU/apic
> > > 
> > > Series enabling usage of -device/device_add for adding CPUs as devices
> > > and device_del for removing them. Using -device/device_add in combination
> > > with query-hotpluggable-cpus QMP command allows to hotplug CPUs at any
> > > not used possition and then safely migrate QEMU instance by specifying
> > > hotadded CPUs on target with help of -device CLI option like with any
> > > other device.
> > > Having been able to replicate exact topology on taggert with -device CPUs
> > > also opens poosibility to hot-remove CPUs, which this series does by
> > > enabling to use device_del with x86 CPUs.
> > > 
> > > 
> > > git tree for testing:
> > >   https://github.com/imammedo/qemu.git dev_del_cpu_v4
> > > for viewing:
> > >   https://github.com/imammedo/qemu/commits/dev_del_cpu_v4
> > > 
> > > Tested with RHEL7.2 guest including ping/pong migration with adding/removing
> > > CPUs in between.
> > > 
> > > CC: pkrempa@redhat.com
> > > CC: ehabkost@redhat.com
> > > CC: mst@redhat.com
> > > CC: eduardo.otubo@profitbricks.com
> > > CC: Bandan Das <bdas@redhat.com>
> > > 
> > > Igor Mammedov (16):
> > >   pc: set APIC ID based on socket/core/thread ids if it's not been set
> > >     yet
> > >   pc: delay setting number of boot CPUs to machine_done time
> > >   pc: register created initial and hotpluged CPUs in one place
> > >     pc_cpu_plug()
> > >   pc: forbid BSP removal
> > >   pc: enforce adding CPUs contiguously and removing them in opposit
> > >     order
> > >   pc: cpu: allow device_add to be used with x86 cpu
> > >   pc: implement query-hotpluggable-cpus callback
> > >   apic: move MAX_APICS check to 'apic' class
> > >   apic: drop APICCommonState.idx and use APIC ID as index in
> > >     local_apics[]
> > >   apic: kvm-apic: fix crash due to access to freed memory region
> > >   (kvm)apic: add unrealize callbacks
> > >   apic: use apic_id as apic's migration instance_id
> > >   target-i386: cpu: do not ignore error and fix apic parent
> > >   target-i386: fix apic object leak when CPU is deleted
> > >   target-i386: add x86_cpu_unrealizefn()
> > >   pc: make device_del CPU work for x86 CPUs
> > > 
> > >  include/hw/i386/apic_internal.h |   5 +-
> > >  include/hw/i386/pc.h            |   5 ++
> > >  hw/i386/kvm/apic.c              |   9 +-
> > >  hw/i386/pc.c                    | 183 ++++++++++++++++++++++++++++++++++------
> > >  hw/intc/apic.c                  |  26 +++++-
> > >  hw/intc/apic_common.c           |  33 ++++++--
> > >  qmp-commands.hx                 |  15 ++++
> > >  target-i386/cpu.c               |  23 ++++-
> > >  8 files changed, 251 insertions(+), 48 deletions(-)
> > > 
> > > -- 
> > > 2.7.4  
>