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

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

[Igor Mammedov]

Changelog:
  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.

Series depends on following not yet commited patchsets:
 1: [PATCH v2 0/6] cpus: make "-cpu cpux, features" global properties
      https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg02579.html
    2: [PATCH v2 00/10] globals: Clean up validation and error checking
         https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg05662.html

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

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

Igor Mammedov (19):
  target-i386: cpu: use uint32_t for X86CPU.apic_id
  pc: add x86_topo_ids_from_apicid()
  pc: extract CPU lookup into a separate function
  pc: cpu: consolidate apic-id validity checks in pc_cpu_pre_plug()
  target-i386: cpu: replace custom apic-id setter/getter with static
    property
  target-i386: add socket/core/thread properties to X86CPU
  pc: set APIC ID based on socket/core/thread ids if it's not been set
    yet
  pc: implement query-hotpluggable-cpus callback
  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: cpu: allow device_add to be used with x86 cpu
  apic: move MAX_APICS check to 'apic' class
  apic: drop APICCommonState.idx and use APIC ID as index in
    local_apics[]
  (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

 hw/i386/kvm/apic.c              |   5 +
 hw/i386/pc.c                    | 233 ++++++++++++++++++++++++++++++++--------
 hw/intc/apic.c                  |  26 ++++-
 hw/intc/apic_common.c           |  33 ++++--
 include/hw/i386/apic_internal.h |   5 +-
 include/hw/i386/pc.h            |   5 +
 include/hw/i386/topology.h      |  15 +++
 qmp-commands.hx                 |  15 +++
 target-i386/cpu.c               |  88 ++++++---------
 target-i386/cpu.h               |  11 +-
 10 files changed, 320 insertions(+), 116 deletions(-)

-- 
2.7.0

[Qemu-devel] [PATCH v3 01/19] target-i386: cpu: use uint32_t for X86CPU.apic_id

[Igor Mammedov]

Redo 9886e834 (target-i386: Require APIC ID to be explicitly set before
CPU realize) in another way that doesn't use int64_t to detect
if apic-id property has been set.

Use the fact that 0xFFFFFFFF is the broadcast
value that a CPU can't have and set default
uint32_t apic_id to it instead of using int64_t.

Later uint32_t apic_id will be used to drop custom
property setter/getter in favor of static property.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c | 4 ++--
 target-i386/cpu.h | 7 ++++++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 5c69c43..e7319e3 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2883,7 +2883,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         goto out;
     }
 
-    if (cpu->apic_id < 0) {
+    if (cpu->apic_id == UNASSIGNED_APIC_ID) {
         error_setg(errp, "apic-id property was not initialized properly");
         return;
     }
@@ -3154,7 +3154,7 @@ static void x86_cpu_initfn(Object *obj)
 
 #ifndef CONFIG_USER_ONLY
     /* Any code creating new X86CPU objects have to set apic-id explicitly */
-    cpu->apic_id = -1;
+    cpu->apic_id = UNASSIGNED_APIC_ID;
 #endif
 
     for (w = 0; w < FEATURE_WORDS; w++) {
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 738958e..00de199 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -835,6 +835,11 @@ typedef struct {
 
 #define NB_OPMASK_REGS 8
 
+/* CPU can't have 0xFFFFFFFF APIC ID, use that value to distinguish
+ * that APIC ID hasn't been set yet
+ */
+#define UNASSIGNED_APIC_ID 0xFFFFFFFF
+
 typedef union X86LegacyXSaveArea {
     struct {
         uint16_t fcw;
@@ -1163,7 +1168,7 @@ struct X86CPU {
     bool expose_kvm;
     bool migratable;
     bool host_features;
-    int64_t apic_id;
+    uint32_t apic_id;
 
     /* if true the CPUID code directly forward host cache leaves to the guest */
     bool cache_info_passthrough;
-- 
2.7.0

Re: [Qemu-devel] [PATCH v3 01/19] target-i386: cpu: use uint32_t for X86CPU.apic_id

[Eduardo Habkost]

On Wed, Jul 06, 2016 at 08:20:37AM +0200, Igor Mammedov wrote:
> Redo 9886e834 (target-i386: Require APIC ID to be explicitly set before
> CPU realize) in another way that doesn't use int64_t to detect
> if apic-id property has been set.
> 
> Use the fact that 0xFFFFFFFF is the broadcast
> value that a CPU can't have and set default
> uint32_t apic_id to it instead of using int64_t.
> 
> Later uint32_t apic_id will be used to drop custom
> property setter/getter in favor of static property.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

Applied to x86-next. Thanks.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 01/19] target-i386: cpu: use uint32_t for X86CPU.apic_id

[Bandan Das]

I know some of these have already been pulled. I just have some minor
questions/comments that shouldn't conflict.

Igor Mammedov <imammedo@redhat.com> writes:

> Redo 9886e834 (target-i386: Require APIC ID to be explicitly set before
> CPU realize) in another way that doesn't use int64_t to detect
> if apic-id property has been set.
>
> Use the fact that 0xFFFFFFFF is the broadcast

I noticed this was UINT32_MAX in v2. Why is UINT32_MAX
not appropriate ?

> value that a CPU can't have and set default
> uint32_t apic_id to it instead of using int64_t.
>
> Later uint32_t apic_id will be used to drop custom
> property setter/getter in favor of static property.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c | 4 ++--
>  target-i386/cpu.h | 7 ++++++-
>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 5c69c43..e7319e3 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2883,7 +2883,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>          goto out;
>      }
>  
> -    if (cpu->apic_id < 0) {
> +    if (cpu->apic_id == UNASSIGNED_APIC_ID) {
>          error_setg(errp, "apic-id property was not initialized properly");
>          return;
>      }
> @@ -3154,7 +3154,7 @@ static void x86_cpu_initfn(Object *obj)
>  
>  #ifndef CONFIG_USER_ONLY
>      /* Any code creating new X86CPU objects have to set apic-id explicitly */
> -    cpu->apic_id = -1;
> +    cpu->apic_id = UNASSIGNED_APIC_ID;
>  #endif
>  
>      for (w = 0; w < FEATURE_WORDS; w++) {
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 738958e..00de199 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -835,6 +835,11 @@ typedef struct {
>  
>  #define NB_OPMASK_REGS 8
>  
> +/* CPU can't have 0xFFFFFFFF APIC ID, use that value to distinguish
> + * that APIC ID hasn't been set yet
> + */
> +#define UNASSIGNED_APIC_ID 0xFFFFFFFF
> +
>  typedef union X86LegacyXSaveArea {
>      struct {
>          uint16_t fcw;
> @@ -1163,7 +1168,7 @@ struct X86CPU {
>      bool expose_kvm;
>      bool migratable;
>      bool host_features;
> -    int64_t apic_id;
> +    uint32_t apic_id;
>  
>      /* if true the CPUID code directly forward host cache leaves to the guest */
>      bool cache_info_passthrough;

Re: [Qemu-devel] [PATCH v3 01/19] target-i386: cpu: use uint32_t for X86CPU.apic_id

[Igor Mammedov]

On Wed, 13 Jul 2016 18:13:59 -0400
Bandan Das <bsd@redhat.com> wrote:

> I know some of these have already been pulled. I just have some minor
> questions/comments that shouldn't conflict.
> 
> Igor Mammedov <imammedo@redhat.com> writes:
> 
> > Redo 9886e834 (target-i386: Require APIC ID to be explicitly set before
> > CPU realize) in another way that doesn't use int64_t to detect
> > if apic-id property has been set.
> >
> > Use the fact that 0xFFFFFFFF is the broadcast  
> 
> I noticed this was UINT32_MAX in v2. Why is UINT32_MAX
> not appropriate ?
mst requested it to be explicit 0xFFFFFFFF

> 
> > value that a CPU can't have and set default
> > uint32_t apic_id to it instead of using int64_t.
> >
> > Later uint32_t apic_id will be used to drop custom
> > property setter/getter in favor of static property.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  target-i386/cpu.c | 4 ++--
> >  target-i386/cpu.h | 7 ++++++-
> >  2 files changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 5c69c43..e7319e3 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2883,7 +2883,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> >          goto out;
> >      }
> >  
> > -    if (cpu->apic_id < 0) {
> > +    if (cpu->apic_id == UNASSIGNED_APIC_ID) {
> >          error_setg(errp, "apic-id property was not initialized properly");
> >          return;
> >      }
> > @@ -3154,7 +3154,7 @@ static void x86_cpu_initfn(Object *obj)
> >  
> >  #ifndef CONFIG_USER_ONLY
> >      /* Any code creating new X86CPU objects have to set apic-id explicitly */
> > -    cpu->apic_id = -1;
> > +    cpu->apic_id = UNASSIGNED_APIC_ID;
> >  #endif
> >  
> >      for (w = 0; w < FEATURE_WORDS; w++) {
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 738958e..00de199 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -835,6 +835,11 @@ typedef struct {
> >  
> >  #define NB_OPMASK_REGS 8
> >  
> > +/* CPU can't have 0xFFFFFFFF APIC ID, use that value to distinguish
> > + * that APIC ID hasn't been set yet
> > + */
> > +#define UNASSIGNED_APIC_ID 0xFFFFFFFF
> > +
> >  typedef union X86LegacyXSaveArea {
> >      struct {
> >          uint16_t fcw;
> > @@ -1163,7 +1168,7 @@ struct X86CPU {
> >      bool expose_kvm;
> >      bool migratable;
> >      bool host_features;
> > -    int64_t apic_id;
> > +    uint32_t apic_id;
> >  
> >      /* if true the CPUID code directly forward host cache leaves to the guest */
> >      bool cache_info_passthrough;  

[Qemu-devel] [PATCH v3 02/19] pc: add x86_topo_ids_from_apicid()

[Igor Mammedov]

it's reverse of apicid_from_topo_ids() and will be used in follow up
patches to fill in data structures for query-hotpluggable-cpus and
for user friendly error reporting

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/i386/topology.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
index fc95572..1ebaee0 100644
--- a/include/hw/i386/topology.h
+++ b/include/hw/i386/topology.h
@@ -117,6 +117,21 @@ static inline void x86_topo_ids_from_idx(unsigned nr_cores,
     topo->pkg_id = core_index / nr_cores;
 }
 
+/* Calculate thread/core/package IDs for a specific topology,
+ * based on APIC ID
+ */
+static inline void x86_topo_ids_from_apicid(apic_id_t apicid,
+                                            unsigned nr_cores,
+                                            unsigned nr_threads,
+                                            X86CPUTopoInfo *topo)
+{
+    topo->smt_id = apicid &
+                   ~(0xFFFFFFFFUL << apicid_smt_width(nr_cores, nr_threads));
+    topo->core_id = (apicid >> apicid_core_offset(nr_cores, nr_threads)) &
+                   ~(0xFFFFFFFFUL << apicid_core_width(nr_cores, nr_threads));
+    topo->pkg_id = apicid >> apicid_pkg_offset(nr_cores, nr_threads);
+}
+
 /* Make APIC ID for the CPU 'cpu_index'
  *
  * 'cpu_index' is a sequential, contiguous ID for the CPU.
-- 
2.7.0

Re: [Qemu-devel] [PATCH v3 02/19] pc: add x86_topo_ids_from_apicid()

[Eduardo Habkost]

On Wed, Jul 06, 2016 at 08:20:38AM +0200, Igor Mammedov wrote:
> it's reverse of apicid_from_topo_ids() and will be used in follow up
> patches to fill in data structures for query-hotpluggable-cpus and
> for user friendly error reporting
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

Applied to x86-next, thanks.

-- 
Eduardo

[Qemu-devel] [PATCH v3 03/19] pc: extract CPU lookup into a separate function

[Igor Mammedov]

it will be reused in the next patch at pre_plug time

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  - rename pc_find_cpu() into pc_find_cpu_slot() and add comment to it
     Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7ab06c2..3c42d84 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1675,11 +1675,30 @@ static int pc_apic_cmp(const void *a, const void *b)
    return apic_a->arch_id - apic_b->arch_id;
 }
 
+/* returns pointer to CPUArchId descriptor that matches CPU's apic_id
+ * in pcms->possible_cpus->cpus, if pcms->possible_cpus->cpus has no
+ * entry correponding to CPU's apic_id returns NULL.
+ */
+static CPUArchId *pc_find_cpu_slot(PCMachineState *pcms, CPUState *cpu,
+                                   int *idx)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+    CPUArchId apic_id, *found_cpu;
+
+    apic_id.arch_id = cc->get_arch_id(CPU(cpu));
+    found_cpu = bsearch(&apic_id, pcms->possible_cpus->cpus,
+        pcms->possible_cpus->len, sizeof(*pcms->possible_cpus->cpus),
+        pc_apic_cmp);
+    if (found_cpu && idx) {
+        *idx = found_cpu - pcms->possible_cpus->cpus;
+    }
+    return found_cpu;
+}
+
 static void pc_cpu_plug(HotplugHandler *hotplug_dev,
                         DeviceState *dev, Error **errp)
 {
-    CPUClass *cc = CPU_GET_CLASS(dev);
-    CPUArchId apic_id, *found_cpu;
+    CPUArchId *found_cpu;
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
@@ -1703,11 +1722,7 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
     /* increment the number of CPUs */
     rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1);
 
-    apic_id.arch_id = cc->get_arch_id(CPU(dev));
-    found_cpu = bsearch(&apic_id, pcms->possible_cpus->cpus,
-        pcms->possible_cpus->len, sizeof(*pcms->possible_cpus->cpus),
-        pc_apic_cmp);
-    assert(found_cpu);
+    found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL);
     found_cpu->cpu = CPU(dev);
 out:
     error_propagate(errp, local_err);
-- 
2.7.0

Re: [Qemu-devel] [PATCH v3 03/19] pc: extract CPU lookup into a separate function

[Eduardo Habkost]

On Wed, Jul 06, 2016 at 08:20:39AM +0200, Igor Mammedov wrote:
> it will be reused in the next patch at pre_plug time
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
>   - rename pc_find_cpu() into pc_find_cpu_slot() and add comment to it
>      Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/i386/pc.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 7ab06c2..3c42d84 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1675,11 +1675,30 @@ static int pc_apic_cmp(const void *a, const void *b)
>     return apic_a->arch_id - apic_b->arch_id;
>  }
>  
> +/* returns pointer to CPUArchId descriptor that matches CPU's apic_id
> + * in pcms->possible_cpus->cpus, if pcms->possible_cpus->cpus has no
> + * entry correponding to CPU's apic_id returns NULL.
> + */
> +static CPUArchId *pc_find_cpu_slot(PCMachineState *pcms, CPUState *cpu,
> +                                   int *idx)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +    CPUArchId apic_id, *found_cpu;
> +
> +    apic_id.arch_id = cc->get_arch_id(CPU(cpu));
> +    found_cpu = bsearch(&apic_id, pcms->possible_cpus->cpus,
> +        pcms->possible_cpus->len, sizeof(*pcms->possible_cpus->cpus),
> +        pc_apic_cmp);
> +    if (found_cpu && idx) {
> +        *idx = found_cpu - pcms->possible_cpus->cpus;

Do you have a use case for the idx parameter? I don't see it
being used for anything in this series.

> +    }
> +    return found_cpu;
> +}
> +
>  static void pc_cpu_plug(HotplugHandler *hotplug_dev,
>                          DeviceState *dev, Error **errp)
>  {
> -    CPUClass *cc = CPU_GET_CLASS(dev);
> -    CPUArchId apic_id, *found_cpu;
> +    CPUArchId *found_cpu;
>      HotplugHandlerClass *hhc;
>      Error *local_err = NULL;
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> @@ -1703,11 +1722,7 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
>      /* increment the number of CPUs */
>      rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1);
>  
> -    apic_id.arch_id = cc->get_arch_id(CPU(dev));
> -    found_cpu = bsearch(&apic_id, pcms->possible_cpus->cpus,
> -        pcms->possible_cpus->len, sizeof(*pcms->possible_cpus->cpus),
> -        pc_apic_cmp);
> -    assert(found_cpu);
> +    found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL);
>      found_cpu->cpu = CPU(dev);
>  out:
>      error_propagate(errp, local_err);
> -- 
> 2.7.0
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 03/19] pc: extract CPU lookup into a separate function

[Igor Mammedov]

On Mon, 11 Jul 2016 23:28:19 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Jul 06, 2016 at 08:20:39AM +0200, Igor Mammedov wrote:
> > it will be reused in the next patch at pre_plug time
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v2:
> >   - rename pc_find_cpu() into pc_find_cpu_slot() and add comment to it
> >      Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  hw/i386/pc.c | 29 ++++++++++++++++++++++-------
> >  1 file changed, 22 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 7ab06c2..3c42d84 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1675,11 +1675,30 @@ static int pc_apic_cmp(const void *a, const void *b)
> >     return apic_a->arch_id - apic_b->arch_id;
> >  }
> >  
> > +/* returns pointer to CPUArchId descriptor that matches CPU's apic_id
> > + * in pcms->possible_cpus->cpus, if pcms->possible_cpus->cpus has no
> > + * entry correponding to CPU's apic_id returns NULL.
> > + */
> > +static CPUArchId *pc_find_cpu_slot(PCMachineState *pcms, CPUState *cpu,
> > +                                   int *idx)
> > +{
> > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > +    CPUArchId apic_id, *found_cpu;
> > +
> > +    apic_id.arch_id = cc->get_arch_id(CPU(cpu));
> > +    found_cpu = bsearch(&apic_id, pcms->possible_cpus->cpus,
> > +        pcms->possible_cpus->len, sizeof(*pcms->possible_cpus->cpus),
> > +        pc_apic_cmp);
> > +    if (found_cpu && idx) {
> > +        *idx = found_cpu - pcms->possible_cpus->cpus;  
> 
> Do you have a use case for the idx parameter? I don't see it
> being used for anything in this series.
I've used to use idx but forgot about it on respin.

it was intended to avoid adhoc index calculations at call sites if needed.

And well, I've used "cpu_slot - pcms->possible_cpus->cpus" instead of idx.
So lets leave this patch as is and I'll fix the next patch to use idx.

> 
> > +    }
> > +    return found_cpu;
> > +}
> > +
> >  static void pc_cpu_plug(HotplugHandler *hotplug_dev,
> >                          DeviceState *dev, Error **errp)
> >  {
> > -    CPUClass *cc = CPU_GET_CLASS(dev);
> > -    CPUArchId apic_id, *found_cpu;
> > +    CPUArchId *found_cpu;
> >      HotplugHandlerClass *hhc;
> >      Error *local_err = NULL;
> >      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> > @@ -1703,11 +1722,7 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
> >      /* increment the number of CPUs */
> >      rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1);
> >  
> > -    apic_id.arch_id = cc->get_arch_id(CPU(dev));
> > -    found_cpu = bsearch(&apic_id, pcms->possible_cpus->cpus,
> > -        pcms->possible_cpus->len, sizeof(*pcms->possible_cpus->cpus),
> > -        pc_apic_cmp);
> > -    assert(found_cpu);
> > +    found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL);
> >      found_cpu->cpu = CPU(dev);
> >  out:
> >      error_propagate(errp, local_err);
> > -- 
> > 2.7.0
> >   
> 

Re: [Qemu-devel] [PATCH v3 03/19] pc: extract CPU lookup into a separate function

[Eduardo Habkost]

On Wed, Jul 06, 2016 at 08:20:39AM +0200, Igor Mammedov wrote:
> it will be reused in the next patch at pre_plug time
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

-- 
Eduardo

[Qemu-devel] [PATCH v3 04/19] pc: cpu: consolidate apic-id validity checks in pc_cpu_pre_plug()

[Igor Mammedov]

Machine code knows about all possible APIC IDs so use that
instead of hack which does O(n^2) complexity duplicate
checks, interating over global CPUs list.
As result duplicate check is done only once with O(log n) complexity.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c      | 44 ++++++++++++++++++++++++++++++++------------
 target-i386/cpu.c | 13 -------------
 2 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3c42d84..99dfbe0 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1071,18 +1071,6 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
         return;
     }
 
-    if (cpu_exists(apic_id)) {
-        error_setg(errp, "Unable to add CPU: %" PRIi64
-                   ", it already exists", id);
-        return;
-    }
-
-    if (id >= max_cpus) {
-        error_setg(errp, "Unable to add CPU: %" PRIi64
-                   ", max allowed: %d", id, max_cpus - 1);
-        return;
-    }
-
     if (apic_id >= ACPI_CPU_HOTPLUG_ID_LIMIT) {
         error_setg(errp, "Unable to add CPU: %" PRIi64
                    ", resulting APIC ID (%" PRIi64 ") is too large",
@@ -1771,6 +1759,37 @@ static void pc_cpu_unplug_cb(HotplugHandler *hotplug_dev,
     error_propagate(errp, local_err);
 }
 
+static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
+                            DeviceState *dev, Error **errp)
+{
+    int idx;
+    X86CPU *cpu = X86_CPU(dev);
+    PCMachineState *pcms = PC_MACHINE(hotplug_dev);
+    CPUArchId *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,
+                   pcms->possible_cpus->len - 1);
+        return;
+    }
+
+    if (cpu_slot->cpu) {
+        error_setg(errp, "CPU[%ld] with APIC ID %" PRIu32 " exists",
+                   cpu_slot - pcms->possible_cpus->cpus,
+                   cpu->apic_id);
+        return;
+    }
+}
+
+static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
+                                          DeviceState *dev, Error **errp)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        pc_cpu_pre_plug(hotplug_dev, dev, errp);
+    }
+}
+
 static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
@@ -2068,6 +2087,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     mc->hot_add_cpu = pc_hot_add_cpu;
     mc->max_cpus = 255;
     mc->reset = pc_machine_reset;
+    hc->pre_plug = pc_machine_device_pre_plug_cb;
     hc->plug = pc_machine_device_plug_cb;
     hc->unplug_request = pc_machine_device_unplug_request_cb;
     hc->unplug = pc_machine_device_unplug_cb;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e7319e3..9511474 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1838,8 +1838,6 @@ static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, const char *name,
 {
     X86CPU *cpu = X86_CPU(obj);
     DeviceState *dev = DEVICE(obj);
-    const int64_t min = 0;
-    const int64_t max = UINT32_MAX;
     Error *error = NULL;
     int64_t value;
 
@@ -1854,17 +1852,6 @@ static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, const char *name,
         error_propagate(errp, error);
         return;
     }
-    if (value < min || value > max) {
-        error_setg(errp, "Property %s.%s doesn't take value %" PRId64
-                   " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
-                   object_get_typename(obj), name, value, min, max);
-        return;
-    }
-
-    if ((value != cpu->apic_id) && cpu_exists(value)) {
-        error_setg(errp, "CPU with APIC ID %" PRIi64 " exists", value);
-        return;
-    }
     cpu->apic_id = value;
 }
 
-- 
2.7.0

Re: [Qemu-devel] [PATCH v3 04/19] pc: cpu: consolidate apic-id validity checks in pc_cpu_pre_plug()

[Eduardo Habkost]

On Wed, Jul 06, 2016 at 08:20:40AM +0200, Igor Mammedov wrote:
[...]
> +static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> +                            DeviceState *dev, Error **errp)
> +{
> +    int idx;
> +    X86CPU *cpu = X86_CPU(dev);
> +    PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> +    CPUArchId *cpu_slot = pc_find_cpu_slot(pcms, CPU(dev), &idx);

Why are you passing &idx to pc_find_cpu_slot() here, if you don't
use it for anything?

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 04/19] pc: cpu: consolidate apic-id validity checks in pc_cpu_pre_plug()

[Igor Mammedov]

On Mon, 11 Jul 2016 23:28:51 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Jul 06, 2016 at 08:20:40AM +0200, Igor Mammedov wrote:
> [...]
> > +static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> > +                            DeviceState *dev, Error **errp)
> > +{
> > +    int idx;
> > +    X86CPU *cpu = X86_CPU(dev);
> > +    PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> > +    CPUArchId *cpu_slot = pc_find_cpu_slot(pcms, CPU(dev), &idx);  
> 
> Why are you passing &idx to pc_find_cpu_slot() here, if you don't
> use it for anything?
> 

+static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
+                            DeviceState *dev, Error **errp)
+{
[...]
+    if (cpu_slot->cpu) {
+        error_setg(errp, "CPU[%ld] with APIC ID %" PRIu32 " exists",
+                   cpu_slot - pcms->possible_cpus->cpus,
it should have been used here, so I'll fix this place to use idx
and along the way print APIC ID in hex

+                   cpu->apic_id);
+        return;
+    }
+}

Re: [Qemu-devel] [PATCH v3 04/19] pc: cpu: consolidate apic-id validity checks in pc_cpu_pre_plug()

[Eduardo Habkost]

On Tue, Jul 12, 2016 at 02:01:13PM +0200, Igor Mammedov wrote:
> On Mon, 11 Jul 2016 23:28:51 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Jul 06, 2016 at 08:20:40AM +0200, Igor Mammedov wrote:
> > [...]
> > > +static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> > > +                            DeviceState *dev, Error **errp)
> > > +{
> > > +    int idx;
> > > +    X86CPU *cpu = X86_CPU(dev);
> > > +    PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> > > +    CPUArchId *cpu_slot = pc_find_cpu_slot(pcms, CPU(dev), &idx);  
> > 
> > Why are you passing &idx to pc_find_cpu_slot() here, if you don't
> > use it for anything?
> > 
> 
> +static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> +                            DeviceState *dev, Error **errp)
> +{
> [...]
> +    if (cpu_slot->cpu) {
> +        error_setg(errp, "CPU[%ld] with APIC ID %" PRIu32 " exists",
> +                   cpu_slot - pcms->possible_cpus->cpus,
> it should have been used here, so I'll fix this place to use idx
> and along the way print APIC ID in hex

With this change:

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

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 04/19] pc: cpu: consolidate apic-id validity checks in pc_cpu_pre_plug()

[Bandan Das]

Igor Mammedov <imammedo@redhat.com> writes:

> Machine code knows about all possible APIC IDs so use that
> instead of hack which does O(n^2) complexity duplicate
> checks, interating over global CPUs list.
> As result duplicate check is done only once with O(log n) complexity.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/i386/pc.c      | 44 ++++++++++++++++++++++++++++++++------------
>  target-i386/cpu.c | 13 -------------
>  2 files changed, 32 insertions(+), 25 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 3c42d84..99dfbe0 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1071,18 +1071,6 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
>          return;
>      }
>  
> -    if (cpu_exists(apic_id)) {
> -        error_setg(errp, "Unable to add CPU: %" PRIi64
> -                   ", it already exists", id);
> -        return;
> -    }
> -
> -    if (id >= max_cpus) {
> -        error_setg(errp, "Unable to add CPU: %" PRIi64
> -                   ", max allowed: %d", id, max_cpus - 1);
> -        return;
> -    }
> -
>      if (apic_id >= ACPI_CPU_HOTPLUG_ID_LIMIT) {
>          error_setg(errp, "Unable to add CPU: %" PRIi64
>                     ", resulting APIC ID (%" PRIi64 ") is too large",
> @@ -1771,6 +1759,37 @@ static void pc_cpu_unplug_cb(HotplugHandler *hotplug_dev,
>      error_propagate(errp, local_err);
>  }
>  
> +static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> +                            DeviceState *dev, Error **errp)
> +{
> +    int idx;
> +    X86CPU *cpu = X86_CPU(dev);
> +    PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> +    CPUArchId *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,
> +                   pcms->possible_cpus->len - 1);
> +        return;
> +    }
> +
> +    if (cpu_slot->cpu) {
> +        error_setg(errp, "CPU[%ld] with APIC ID %" PRIu32 " exists",
> +                   cpu_slot - pcms->possible_cpus->cpus,
> +                   cpu->apic_id);
> +        return;
> +    }
> +}
> +
> +static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> +                                          DeviceState *dev, Error **errp)
> +{
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> +        pc_cpu_pre_plug(hotplug_dev, dev, errp);
> +    }
> +}
>

Will this path be invoked for non TYPE_CPU callbacks too ? I was just wondering
if there should be an "else" error message.

Bandan

>  static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>                                        DeviceState *dev, Error **errp)
>  {
> @@ -2068,6 +2087,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      mc->hot_add_cpu = pc_hot_add_cpu;
>      mc->max_cpus = 255;
>      mc->reset = pc_machine_reset;
> +    hc->pre_plug = pc_machine_device_pre_plug_cb;
>      hc->plug = pc_machine_device_plug_cb;
>      hc->unplug_request = pc_machine_device_unplug_request_cb;
>      hc->unplug = pc_machine_device_unplug_cb;
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index e7319e3..9511474 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1838,8 +1838,6 @@ static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, const char *name,
>  {
>      X86CPU *cpu = X86_CPU(obj);
>      DeviceState *dev = DEVICE(obj);
> -    const int64_t min = 0;
> -    const int64_t max = UINT32_MAX;
>      Error *error = NULL;
>      int64_t value;
>  
> @@ -1854,17 +1852,6 @@ static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, const char *name,
>          error_propagate(errp, error);
>          return;
>      }
> -    if (value < min || value > max) {
> -        error_setg(errp, "Property %s.%s doesn't take value %" PRId64
> -                   " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
> -                   object_get_typename(obj), name, value, min, max);
> -        return;
> -    }
> -
> -    if ((value != cpu->apic_id) && cpu_exists(value)) {
> -        error_setg(errp, "CPU with APIC ID %" PRIi64 " exists", value);
> -        return;
> -    }
>      cpu->apic_id = value;
>  }

Re: [Qemu-devel] [PATCH v3 04/19] pc: cpu: consolidate apic-id validity checks in pc_cpu_pre_plug()

[Igor Mammedov]

On Wed, 13 Jul 2016 18:16:59 -0400
Bandan Das <bsd@redhat.com> wrote:

> Igor Mammedov <imammedo@redhat.com> writes:
> 
> > Machine code knows about all possible APIC IDs so use that
> > instead of hack which does O(n^2) complexity duplicate
> > checks, interating over global CPUs list.
> > As result duplicate check is done only once with O(log n) complexity.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/i386/pc.c      | 44 ++++++++++++++++++++++++++++++++------------
> >  target-i386/cpu.c | 13 -------------
> >  2 files changed, 32 insertions(+), 25 deletions(-)
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 3c42d84..99dfbe0 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1071,18 +1071,6 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
> >          return;
> >      }
> >  
> > -    if (cpu_exists(apic_id)) {
> > -        error_setg(errp, "Unable to add CPU: %" PRIi64
> > -                   ", it already exists", id);
> > -        return;
> > -    }
> > -
> > -    if (id >= max_cpus) {
> > -        error_setg(errp, "Unable to add CPU: %" PRIi64
> > -                   ", max allowed: %d", id, max_cpus - 1);
> > -        return;
> > -    }
> > -
> >      if (apic_id >= ACPI_CPU_HOTPLUG_ID_LIMIT) {
> >          error_setg(errp, "Unable to add CPU: %" PRIi64
> >                     ", resulting APIC ID (%" PRIi64 ") is too large",
> > @@ -1771,6 +1759,37 @@ static void pc_cpu_unplug_cb(HotplugHandler *hotplug_dev,
> >      error_propagate(errp, local_err);
> >  }
> >  
> > +static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> > +                            DeviceState *dev, Error **errp)
> > +{
> > +    int idx;
> > +    X86CPU *cpu = X86_CPU(dev);
> > +    PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> > +    CPUArchId *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,
> > +                   pcms->possible_cpus->len - 1);
> > +        return;
> > +    }
> > +
> > +    if (cpu_slot->cpu) {
> > +        error_setg(errp, "CPU[%ld] with APIC ID %" PRIu32 " exists",
> > +                   cpu_slot - pcms->possible_cpus->cpus,
> > +                   cpu->apic_id);
> > +        return;
> > +    }
> > +}
> > +
> > +static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> > +                                          DeviceState *dev, Error **errp)
> > +{
> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> > +        pc_cpu_pre_plug(hotplug_dev, dev, errp);
> > +    }
> > +}
> >  
> 
> Will this path be invoked for non TYPE_CPU callbacks too ? I was just wondering
> if there should be an "else" error message.
This patch is called for every device,
so no there shouldn't be else error path here.

> 
> Bandan
> 
> >  static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
> >                                        DeviceState *dev, Error **errp)
> >  {
> > @@ -2068,6 +2087,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
> >      mc->hot_add_cpu = pc_hot_add_cpu;
> >      mc->max_cpus = 255;
> >      mc->reset = pc_machine_reset;
> > +    hc->pre_plug = pc_machine_device_pre_plug_cb;
> >      hc->plug = pc_machine_device_plug_cb;
> >      hc->unplug_request = pc_machine_device_unplug_request_cb;
> >      hc->unplug = pc_machine_device_unplug_cb;
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index e7319e3..9511474 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1838,8 +1838,6 @@ static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, const char *name,
> >  {
> >      X86CPU *cpu = X86_CPU(obj);
> >      DeviceState *dev = DEVICE(obj);
> > -    const int64_t min = 0;
> > -    const int64_t max = UINT32_MAX;
> >      Error *error = NULL;
> >      int64_t value;
> >  
> > @@ -1854,17 +1852,6 @@ static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, const char *name,
> >          error_propagate(errp, error);
> >          return;
> >      }
> > -    if (value < min || value > max) {
> > -        error_setg(errp, "Property %s.%s doesn't take value %" PRId64
> > -                   " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
> > -                   object_get_typename(obj), name, value, min, max);
> > -        return;
> > -    }
> > -
> > -    if ((value != cpu->apic_id) && cpu_exists(value)) {
> > -        error_setg(errp, "CPU with APIC ID %" PRIi64 " exists", value);
> > -        return;
> > -    }
> >      cpu->apic_id = value;
> >  }  
> 

Re: [Qemu-devel] [PATCH v3 04/19] pc: cpu: consolidate apic-id validity checks in pc_cpu_pre_plug()

[Eduardo Habkost]

On Wed, Jul 06, 2016 at 08:20:40AM +0200, Igor Mammedov wrote:
> +        error_setg(errp, "CPU[%ld] with APIC ID %" PRIu32 " exists",
> +                   cpu_slot - pcms->possible_cpus->cpus,
> +                   cpu->apic_id);

Build error on 32-bit reported by Peter Maydell:

/home/petmay01/qemu/hw/i386/pc.c: In function 'pc_cpu_pre_plug':
/home/petmay01/qemu/hw/i386/pc.c:1926:9: error: format '%ld' expects
argument of type 'long int', but argument 6 has type 'int'
[-Werror=format=]
         error_setg(errp, "CPU[%ld] with APIC ID %" PRIu32 " exists",
         ^
cc1: all warnings being treated as errors


I have fixed this on git, with:

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index cf518ac..ac7a4d5 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1923,9 +1923,8 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
     }
 
     if (cpu_slot->cpu) {
-        error_setg(errp, "CPU[%ld] with APIC ID %" PRIu32 " exists",
-                   cpu_slot - pcms->possible_cpus->cpus,
-                   cpu->apic_id);
+        error_setg(errp, "CPU[%d] with APIC ID %" PRIu32 " exists",
+                   idx, cpu->apic_id);
         return;
     }
 
-- 
Eduardo

[Qemu-devel] [PATCH v3 05/19] target-i386: cpu: replace custom apic-id setter/getter with static property

[Igor Mammedov]

custom apic-id setter/getter doesn't do any property specific
checks anymore, so clean it up and use more compact static
property DEFINE_PROP_UINT32 instead.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 9511474..9294b3d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1824,37 +1824,6 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name,
     cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000;
 }
 
-static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, const char *name,
-                                  void *opaque, Error **errp)
-{
-    X86CPU *cpu = X86_CPU(obj);
-    int64_t value = cpu->apic_id;
-
-    visit_type_int(v, name, &value, errp);
-}
-
-static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, const char *name,
-                                  void *opaque, Error **errp)
-{
-    X86CPU *cpu = X86_CPU(obj);
-    DeviceState *dev = DEVICE(obj);
-    Error *error = NULL;
-    int64_t value;
-
-    if (dev->realized) {
-        error_setg(errp, "Attempt to set property '%s' on '%s' after "
-                   "it was realized", name, object_get_typename(obj));
-        return;
-    }
-
-    visit_type_int(v, name, &value, &error);
-    if (error) {
-        error_propagate(errp, error);
-        return;
-    }
-    cpu->apic_id = value;
-}
-
 /* Generic getter for "feature-words" and "filtered-features" properties */
 static void x86_cpu_get_feature_words(Object *obj, Visitor *v,
                                       const char *name, void *opaque,
@@ -3127,9 +3096,6 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add(obj, "tsc-frequency", "int",
                         x86_cpuid_get_tsc_freq,
                         x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
-    object_property_add(obj, "apic-id", "int",
-                        x86_cpuid_get_apic_id,
-                        x86_cpuid_set_apic_id, NULL, NULL, NULL);
     object_property_add(obj, "feature-words", "X86CPUFeatureWordInfo",
                         x86_cpu_get_feature_words,
                         NULL, NULL, (void *)env->features, NULL);
@@ -3139,11 +3105,6 @@ static void x86_cpu_initfn(Object *obj)
 
     cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY;
 
-#ifndef CONFIG_USER_ONLY
-    /* Any code creating new X86CPU objects have to set apic-id explicitly */
-    cpu->apic_id = UNASSIGNED_APIC_ID;
-#endif
-
     for (w = 0; w < FEATURE_WORDS; w++) {
         int bitnr;
 
@@ -3200,6 +3161,12 @@ static bool x86_cpu_has_work(CPUState *cs)
 }
 
 static Property x86_cpu_properties[] = {
+#ifdef CONFIG_USER_ONLY
+    /* apic_id = 0 by default for *-user, see commit 9886e834 */
+    DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, 0),
+#else
+    DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, UNASSIGNED_APIC_ID),
+#endif
     DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false),
     { .name  = "hv-spinlocks", .info  = &qdev_prop_spinlocks },
     DEFINE_PROP_BOOL("hv-relaxed", X86CPU, hyperv_relaxed_timing, false),
-- 
2.7.0

[Qemu-devel] [PATCH v3 06/19] target-i386: add socket/core/thread properties to X86CPU

[Igor Mammedov]

these properties will be used by as address where to plug
CPU with help -device/device_add commands.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
  - use %u for printing topo ids
  - add to error message topo ids from set apic_id
v2:
  - rename socket/core/thread properties to socket-id/core-id/thread-id
  - add mismatch checks for apic_id and socket-id/core-id/thread-id
    in case both are set
---
 hw/i386/pc.c      | 29 +++++++++++++++++++++++++++++
 target-i386/cpu.c |  6 ++++++
 target-i386/cpu.h |  4 ++++
 3 files changed, 39 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 99dfbe0..24231ca 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1763,6 +1763,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
                             DeviceState *dev, Error **errp)
 {
     int idx;
+    X86CPUTopoInfo topo;
     X86CPU *cpu = X86_CPU(dev);
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
     CPUArchId *cpu_slot = pc_find_cpu_slot(pcms, CPU(dev), &idx);
@@ -1780,6 +1781,34 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
                    cpu->apic_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
+     */
+    /* TODO: move socket_id/core_id/thread_id checks into x86_cpu_realizefn()
+     * once -smp refactoring is complete and there will be CPU private
+     * CPUState::nr_cores and CPUState::nr_threads fields instead of globals */
+    x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads, &topo);
+    if (cpu->socket_id != -1 && cpu->socket_id != topo.pkg_id) {
+        error_setg(errp, "property socket-id: %u doesn't match set apic-id:"
+            " 0x%x (socket-id: %u)", cpu->socket_id, cpu->apic_id, topo.pkg_id);
+        return;
+    }
+    cpu->socket_id = topo.pkg_id;
+
+    if (cpu->core_id != -1 && cpu->core_id != topo.core_id) {
+        error_setg(errp, "property core-id: %u doesn't match set apic-id:"
+            " 0x%x (core-id: %u)", cpu->core_id, cpu->apic_id, topo.core_id);
+        return;
+    }
+    cpu->core_id = topo.core_id;
+
+    if (cpu->thread_id != -1 && cpu->thread_id != topo.smt_id) {
+        error_setg(errp, "property thread-id: %u doesn't match set apic-id:"
+            " 0x%x (thread-id: %u)", cpu->thread_id, cpu->apic_id, topo.smt_id);
+        return;
+    }
+    cpu->thread_id = topo.smt_id;
 }
 
 static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 9294b3d..1ec40a0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3164,8 +3164,14 @@ static Property x86_cpu_properties[] = {
 #ifdef CONFIG_USER_ONLY
     /* apic_id = 0 by default for *-user, see commit 9886e834 */
     DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, 0),
+    DEFINE_PROP_INT32("thread-id", X86CPU, thread_id, 0),
+    DEFINE_PROP_INT32("core-id", X86CPU, core_id, 0),
+    DEFINE_PROP_INT32("socket-id", X86CPU, socket_id, 0),
 #else
     DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, UNASSIGNED_APIC_ID),
+    DEFINE_PROP_INT32("thread-id", X86CPU, thread_id, -1),
+    DEFINE_PROP_INT32("core-id", X86CPU, core_id, -1),
+    DEFINE_PROP_INT32("socket-id", X86CPU, socket_id, -1),
 #endif
     DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false),
     { .name  = "hv-spinlocks", .info  = &qdev_prop_spinlocks },
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 00de199..0612181 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1193,6 +1193,10 @@ struct X86CPU {
     Notifier machine_done;
 
     struct kvm_msrs *kvm_msr_buf;
+
+    int32_t socket_id;
+    int32_t core_id;
+    int32_t thread_id;
 };
 
 static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
-- 
2.7.0

Re: [Qemu-devel] [PATCH v3 06/19] target-i386: add socket/core/thread properties to X86CPU

[Eduardo Habkost]

On Wed, Jul 06, 2016 at 08:20:42AM +0200, Igor Mammedov wrote:
> these properties will be used by as address where to plug
> CPU with help -device/device_add commands.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 06/19] target-i386: add socket/core/thread properties to X86CPU

[Bandan Das]

Igor Mammedov <imammedo@redhat.com> writes:

> these properties will be used by as address where to plug
> CPU with help -device/device_add commands.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v3:
>   - use %u for printing topo ids
>   - add to error message topo ids from set apic_id
> v2:
>   - rename socket/core/thread properties to socket-id/core-id/thread-id
>   - add mismatch checks for apic_id and socket-id/core-id/thread-id
>     in case both are set
> ---
>  hw/i386/pc.c      | 29 +++++++++++++++++++++++++++++
>  target-i386/cpu.c |  6 ++++++
>  target-i386/cpu.h |  4 ++++
>  3 files changed, 39 insertions(+)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 99dfbe0..24231ca 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1763,6 +1763,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>                              DeviceState *dev, Error **errp)
>  {
>      int idx;
> +    X86CPUTopoInfo topo;
>      X86CPU *cpu = X86_CPU(dev);
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>      CPUArchId *cpu_slot = pc_find_cpu_slot(pcms, CPU(dev), &idx);
> @@ -1780,6 +1781,34 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>                     cpu->apic_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
> +     */
> +    /* TODO: move socket_id/core_id/thread_id checks into x86_cpu_realizefn()
> +     * once -smp refactoring is complete and there will be CPU private
> +     * CPUState::nr_cores and CPUState::nr_threads fields instead of globals */
> +    x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads, &topo);
> +    if (cpu->socket_id != -1 && cpu->socket_id != topo.pkg_id) {
> +        error_setg(errp, "property socket-id: %u doesn't match set apic-id:"
> +            " 0x%x (socket-id: %u)", cpu->socket_id, cpu->apic_id, topo.pkg_id);
> +        return;
> +    }
> +    cpu->socket_id = topo.pkg_id;
> +
> +    if (cpu->core_id != -1 && cpu->core_id != topo.core_id) {
> +        error_setg(errp, "property core-id: %u doesn't match set apic-id:"
> +            " 0x%x (core-id: %u)", cpu->core_id, cpu->apic_id, topo.core_id);
> +        return;
> +    }
> +    cpu->core_id = topo.core_id;
> +
> +    if (cpu->thread_id != -1 && cpu->thread_id != topo.smt_id) {
> +        error_setg(errp, "property thread-id: %u doesn't match set apic-id:"
> +            " 0x%x (thread-id: %u)", cpu->thread_id, cpu->apic_id, topo.smt_id);
> +        return;
> +    }
> +    cpu->thread_id = topo.smt_id;
>  }

What is the case where these fields are already populated ? When the cpus are online
at system boot followed by an unplug and then a hotplug ?

>  static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 9294b3d..1ec40a0 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -3164,8 +3164,14 @@ static Property x86_cpu_properties[] = {
>  #ifdef CONFIG_USER_ONLY
>      /* apic_id = 0 by default for *-user, see commit 9886e834 */
>      DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, 0),
> +    DEFINE_PROP_INT32("thread-id", X86CPU, thread_id, 0),
> +    DEFINE_PROP_INT32("core-id", X86CPU, core_id, 0),
> +    DEFINE_PROP_INT32("socket-id", X86CPU, socket_id, 0),
>  #else
>      DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, UNASSIGNED_APIC_ID),
> +    DEFINE_PROP_INT32("thread-id", X86CPU, thread_id, -1),
> +    DEFINE_PROP_INT32("core-id", X86CPU, core_id, -1),
> +    DEFINE_PROP_INT32("socket-id", X86CPU, socket_id, -1),

Are these values (including the UNASSIGNED_APIC_ID) mandated by spec
or just convenient values ?

Bandan

>  #endif
>      DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false),
>      { .name  = "hv-spinlocks", .info  = &qdev_prop_spinlocks },
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 00de199..0612181 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -1193,6 +1193,10 @@ struct X86CPU {
>      Notifier machine_done;
>  
>      struct kvm_msrs *kvm_msr_buf;
> +
> +    int32_t socket_id;
> +    int32_t core_id;
> +    int32_t thread_id;
>  };
>  
>  static inline X86CPU *x86_env_get_cpu(CPUX86State *env)

Re: [Qemu-devel] [PATCH v3 06/19] target-i386: add socket/core/thread properties to X86CPU

[Igor Mammedov]

On Wed, 13 Jul 2016 18:22:18 -0400
Bandan Das <bsd@redhat.com> wrote:

> Igor Mammedov <imammedo@redhat.com> writes:
> 
> > these properties will be used by as address where to plug
> > CPU with help -device/device_add commands.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v3:
> >   - use %u for printing topo ids
> >   - add to error message topo ids from set apic_id
> > v2:
> >   - rename socket/core/thread properties to socket-id/core-id/thread-id
> >   - add mismatch checks for apic_id and socket-id/core-id/thread-id
> >     in case both are set
> > ---
> >  hw/i386/pc.c      | 29 +++++++++++++++++++++++++++++
> >  target-i386/cpu.c |  6 ++++++
> >  target-i386/cpu.h |  4 ++++
> >  3 files changed, 39 insertions(+)
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 99dfbe0..24231ca 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1763,6 +1763,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> >                              DeviceState *dev, Error **errp)
> >  {
> >      int idx;
> > +    X86CPUTopoInfo topo;
> >      X86CPU *cpu = X86_CPU(dev);
> >      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> >      CPUArchId *cpu_slot = pc_find_cpu_slot(pcms, CPU(dev), &idx);
> > @@ -1780,6 +1781,34 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> >                     cpu->apic_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
> > +     */
> > +    /* TODO: move socket_id/core_id/thread_id checks into x86_cpu_realizefn()
> > +     * once -smp refactoring is complete and there will be CPU private
> > +     * CPUState::nr_cores and CPUState::nr_threads fields instead of globals */
> > +    x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads, &topo);
> > +    if (cpu->socket_id != -1 && cpu->socket_id != topo.pkg_id) {
> > +        error_setg(errp, "property socket-id: %u doesn't match set apic-id:"
> > +            " 0x%x (socket-id: %u)", cpu->socket_id, cpu->apic_id, topo.pkg_id);
> > +        return;
> > +    }
> > +    cpu->socket_id = topo.pkg_id;
> > +
> > +    if (cpu->core_id != -1 && cpu->core_id != topo.core_id) {
> > +        error_setg(errp, "property core-id: %u doesn't match set apic-id:"
> > +            " 0x%x (core-id: %u)", cpu->core_id, cpu->apic_id, topo.core_id);
> > +        return;
> > +    }
> > +    cpu->core_id = topo.core_id;
> > +
> > +    if (cpu->thread_id != -1 && cpu->thread_id != topo.smt_id) {
> > +        error_setg(errp, "property thread-id: %u doesn't match set apic-id:"
> > +            " 0x%x (thread-id: %u)", cpu->thread_id, cpu->apic_id, topo.smt_id);
> > +        return;
> > +    }
> > +    cpu->thread_id = topo.smt_id;
> >  }  
> 
> What is the case where these fields are already populated ? When the cpus are online
> at system boot followed by an unplug and then a hotplug ?
there fields are supposed to be populated by -device/device_add callers
who specify them using socket-id/core-id/thread-id properties

> 
> >  static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 9294b3d..1ec40a0 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -3164,8 +3164,14 @@ static Property x86_cpu_properties[] = {
> >  #ifdef CONFIG_USER_ONLY
> >      /* apic_id = 0 by default for *-user, see commit 9886e834 */
> >      DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, 0),
> > +    DEFINE_PROP_INT32("thread-id", X86CPU, thread_id, 0),
> > +    DEFINE_PROP_INT32("core-id", X86CPU, core_id, 0),
> > +    DEFINE_PROP_INT32("socket-id", X86CPU, socket_id, 0),
> >  #else
> >      DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, UNASSIGNED_APIC_ID),
> > +    DEFINE_PROP_INT32("thread-id", X86CPU, thread_id, -1),
> > +    DEFINE_PROP_INT32("core-id", X86CPU, core_id, -1),
> > +    DEFINE_PROP_INT32("socket-id", X86CPU, socket_id, -1),  
> 
> Are these values (including the UNASSIGNED_APIC_ID) mandated by spec
> or just convenient values ?
it's convenience values 

> 
> Bandan
> 
> >  #endif
> >      DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false),
> >      { .name  = "hv-spinlocks", .info  = &qdev_prop_spinlocks },
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 00de199..0612181 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -1193,6 +1193,10 @@ struct X86CPU {
> >      Notifier machine_done;
> >  
> >      struct kvm_msrs *kvm_msr_buf;
> > +
> > +    int32_t socket_id;
> > +    int32_t core_id;
> > +    int32_t thread_id;
> >  };
> >  
> >  static inline X86CPU *x86_env_get_cpu(CPUX86State *env)  

[Qemu-devel] [PATCH v3 07/19] 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>
---
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 24231ca..29da2d4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1763,14 +1763,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: %d, core: %d, thread: %d] 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.0

Re: [Qemu-devel] [PATCH v3 07/19] pc: set APIC ID based on socket/core/thread ids if it's not been set yet

[Eduardo Habkost]

On Wed, Jul 06, 2016 at 08:20:43AM +0200, Igor Mammedov wrote:
> 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>
> ---
> v3:
>  - use %u for printing topo ids

You didn't change the "Invalid CPU" error message.

> 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 24231ca..29da2d4 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1763,14 +1763,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: %d, core: %d, thread: %d] with"

You are still printing the X86CPUTopoInfo fields as signed, here.

With this changed to use %u too:

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


> +                  " APIC ID (%" PRIu32 "), valid index range 0:%d",

I thought you were going to change it to print APIC ID as hex.

(You can keep my Reviewed-by on either case)

> +                   topo.pkg_id, topo.core_id, topo.smt_id, cpu->apic_id,
>                     pcms->possible_cpus->len - 1);
>          return;
>      }
> -- 
> 2.7.0
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 07/19] pc: set APIC ID based on socket/core/thread ids if it's not been set yet

[Igor Mammedov]

On Mon, 11 Jul 2016 23:48:27 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Jul 06, 2016 at 08:20:43AM +0200, Igor Mammedov wrote:
> > 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>
> > ---
> > v3:
> >  - use %u for printing topo ids  
> 
> You didn't change the "Invalid CPU" error message.
> 
> > 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 24231ca..29da2d4 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1763,14 +1763,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: %d, core: %d, thread: %d] with"  
> 
> You are still printing the X86CPUTopoInfo fields as signed, here.
> 
> With this changed to use %u too:
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
thanks, fixed in v4

> 
> 
> > +                  " APIC ID (%" PRIu32 "), valid index range 0:%d",  
> 
> I thought you were going to change it to print APIC ID as hex.
done, I'm sorry for missing it here.

> 
> (You can keep my Reviewed-by on either case)
> 
> > +                   topo.pkg_id, topo.core_id, topo.smt_id, cpu->apic_id,
> >                     pcms->possible_cpus->len - 1);
> >          return;
> >      }
> > -- 
> > 2.7.0
> >   
> 

Re: [Qemu-devel] [PATCH v3 07/19] pc: set APIC ID based on socket/core/thread ids if it's not been set yet

[Igor Mammedov]

On Mon, 11 Jul 2016 23:48:27 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Jul 06, 2016 at 08:20:43AM +0200, Igor Mammedov wrote:
> > 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>
[...]
> 
> 
> > +                  " APIC ID (%" PRIu32 "), valid index range 0:%d",  
> 
> I thought you were going to change it to print APIC ID as hex.
Since unsigned variant for APIC ID is already in x86-next,
lets continue with that,
and I'll do a patch on top of series that changes printed APIC ID
to hex consistently all over the three.

Re: [Qemu-devel] [PATCH v3 07/19] pc: set APIC ID based on socket/core/thread ids if it's not been set yet

[Bandan Das]

Igor Mammedov <imammedo@redhat.com> writes:

> 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>
> ---
> 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 24231ca..29da2d4 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1763,14 +1763,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;
> +        }

Just curoious, when any of these values < 0, the only other values that they
can take is -1, right ? I am wondering why decided to do the check differently
for in the preceeding patch.

Bandan
> +        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: %d, core: %d, thread: %d] 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;
>      }

Re: [Qemu-devel] [PATCH v3 07/19] pc: set APIC ID based on socket/core/thread ids if it's not been set yet

[Eduardo Habkost]

On Wed, Jul 13, 2016 at 06:24:17PM -0400, Bandan Das wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
> 
> > 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>
> > ---
> > 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 24231ca..29da2d4 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1763,14 +1763,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;
> > +        }
> 
> Just curoious, when any of these values < 0, the only other values that they
> can take is -1, right ?

No, the user might have set thread=-2 explicitly, and this is
where we validate the user-provided values.

> I am wondering why decided to do the check differently
> for in the preceeding patch.

Because on both cases we want thread_id <= -2 to generate an
error message. On patch 06/19, thread_id == -1 is one case where
the error message will be skipped (but thread_id == -2 will
generate an error). In this patch, all negative values should
generate an error.

> 
> Bandan
> > +        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: %d, core: %d, thread: %d] 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;
> >      }

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 07/19] pc: set APIC ID based on socket/core/thread ids if it's not been set yet

[Bandan Das]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Wed, Jul 13, 2016 at 06:24:17PM -0400, Bandan Das wrote:
>> Igor Mammedov <imammedo@redhat.com> writes:
>> 
>> > 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>
>> > ---
>> > 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 24231ca..29da2d4 100644
>> > --- a/hw/i386/pc.c
>> > +++ b/hw/i386/pc.c
>> > @@ -1763,14 +1763,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;
>> > +        }
>> 
>> Just curoious, when any of these values < 0, the only other values that they
>> can take is -1, right ?
>
> No, the user might have set thread=-2 explicitly, and this is
> where we validate the user-provided values.
>
>> I am wondering why decided to do the check differently
>> for in the preceeding patch.
>
> Because on both cases we want thread_id <= -2 to generate an
> error message. On patch 06/19, thread_id == -1 is one case where
> the error message will be skipped (but thread_id == -2 will
> generate an error). In this patch, all negative values should
> generate an error.

Understood, this makes sense. Thanks.

>> 
>> Bandan
>> > +        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: %d, core: %d, thread: %d] 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;
>> >      }

[Qemu-devel] [PATCH v3 08/19] 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>
---
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 29da2d4..6691825 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2110,6 +2110,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 */
@@ -2150,6 +2194,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.0

Re: [Qemu-devel] [PATCH v3 08/19] pc: implement query-hotpluggable-cpus callback

[Eduardo Habkost]

On Wed, Jul 06, 2016 at 08:20:44AM +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>

But shouldn't this be at the end of the series, or at least after
the code that makes device_add work with X86CPU?

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 08/19] pc: implement query-hotpluggable-cpus callback

[Igor Mammedov]

On Mon, 11 Jul 2016 23:54:42 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Jul 06, 2016 at 08:20:44AM +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>
> 
> But shouldn't this be at the end of the series, or at least after
> the code that makes device_add work with X86CPU?
sure, I'll move it there

Re: [Qemu-devel] [PATCH v3 08/19] pc: implement query-hotpluggable-cpus callback

[Eric Blake]

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

On 07/06/2016 12:20 AM, 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}
>      }
>    ]}
> 

Matches the schema.

> Signed-off-by: Igor Mammedov <imammedo@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(+)
> 

> +++ 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}
> +         }
> +       ]}

I didn't review the full patch, but the interface change looks okay.

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

[Qemu-devel] [PATCH v3 09/19] 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>
---
 hw/i386/pc.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 6691825..3206572 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,
@@ -1157,10 +1154,19 @@ void pc_cpus_init(PCMachineState *pcms)
 static
 void pc_machine_done(Notifier *notifier, void *data)
 {
+    int i, boot_cpus = 0;
     PCMachineState *pcms = container_of(notifier,
                                         PCMachineState, machine_done);
     PCIBus *bus = pcms->bus;
 
+    for (i = 0; i < pcms->possible_cpus->len; i++) {
+        if (pcms->possible_cpus->cpus[i].cpu) {
+            boot_cpus++;
+        }
+    }
+    /* set the number of CPUs */
+    rtc_set_memory(pcms->rtc, 0x5f, boot_cpus - 1);
+
     if (bus) {
         int extra_hosts = 0;
 
-- 
2.7.0

Re: [Qemu-devel] [PATCH v3 09/19] pc: delay setting number of boot CPUs to machine_done time

[Eduardo Habkost]

On Wed, Jul 06, 2016 at 08:20:45AM +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.

Do you plan to fix the remaining code using smp_cpus? e.g.:

1) x86_cpu_realizefn():
    if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC || smp_cpus > 1) {
            x86_cpu_apic_create(cpu, &local_err);
    [...]
  (Maybe we should simply make realizefn fail if we try to create a second CPU
  using -device or device_add without CPUID_APIC)

2) the smp_cpus checks at hw/i386/kvmvapic.c.

> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/i386/pc.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 6691825..3206572 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,
> @@ -1157,10 +1154,19 @@ void pc_cpus_init(PCMachineState *pcms)
>  static
>  void pc_machine_done(Notifier *notifier, void *data)
>  {
> +    int i, boot_cpus = 0;
>      PCMachineState *pcms = container_of(notifier,
>                                          PCMachineState, machine_done);
>      PCIBus *bus = pcms->bus;
>  
> +    for (i = 0; i < pcms->possible_cpus->len; i++) {
> +        if (pcms->possible_cpus->cpus[i].cpu) {
> +            boot_cpus++;
> +        }
> +    }

Any specific reason you chose to check possible_cpus instead of the
arch-independent CPU list from exec.c? I believe other architectures will be
interested in a generic way to count online CPUs instead of using smp_cpus.

> +    /* set the number of CPUs */
> +    rtc_set_memory(pcms->rtc, 0x5f, boot_cpus - 1);
> +
>      if (bus) {
>          int extra_hosts = 0;
>  
> -- 
> 2.7.0
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 09/19] pc: delay setting number of boot CPUs to machine_done time

[Igor Mammedov]

On Tue, 12 Jul 2016 00:29:08 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Jul 06, 2016 at 08:20:45AM +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.  
> 
> Do you plan to fix the remaining code using smp_cpus? e.g.:
perhaps after Drew's -smp refactoring or maybe during it,
it looks like good candidate for that series,
I'll work with Drew on that.

> 
> 1) x86_cpu_realizefn():
>     if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC || smp_cpus > 1) {
>             x86_cpu_apic_create(cpu, &local_err);
>     [...]
>   (Maybe we should simply make realizefn fail if we try to create a second CPU
>   using -device or device_add without CPUID_APIC)
wouldn't that break some setups that doing it but still able
to boot?

> 
> 2) the smp_cpus checks at hw/i386/kvmvapic.c.
> 
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/i386/pc.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 6691825..3206572 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,
> > @@ -1157,10 +1154,19 @@ void pc_cpus_init(PCMachineState *pcms)
> >  static
> >  void pc_machine_done(Notifier *notifier, void *data)
> >  {
> > +    int i, boot_cpus = 0;
> >      PCMachineState *pcms = container_of(notifier,
> >                                          PCMachineState, machine_done);
> >      PCIBus *bus = pcms->bus;
> >  
> > +    for (i = 0; i < pcms->possible_cpus->len; i++) {
> > +        if (pcms->possible_cpus->cpus[i].cpu) {
> > +            boot_cpus++;
> > +        }
> > +    }  
> 
> Any specific reason you chose to check possible_cpus instead of the
> arch-independent CPU list from exec.c? I believe other architectures will be
> interested in a generic way to count online CPUs instead of using smp_cpus.
When others would need to do it we can switch to arch-independent CPU list but
for now I'd prefer to keep using possible_cpus throughout pc.c for consistency
reasons (so it would be the single point of bookkeeping CPUs on PC),
and I most likely will do the same for ARM probably reusing some of PC code.

 
> > +    /* set the number of CPUs */
> > +    rtc_set_memory(pcms->rtc, 0x5f, boot_cpus - 1);
> > +
> >      if (bus) {
> >          int extra_hosts = 0;
> >  
> > -- 
> > 2.7.0
> >   
> 

Re: [Qemu-devel] [PATCH v3 09/19] pc: delay setting number of boot CPUs to machine_done time

[Igor Mammedov]

On Tue, 12 Jul 2016 14:48:43 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Tue, 12 Jul 2016 00:29:08 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
[...] 
> > 1) x86_cpu_realizefn():
> >     if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC || smp_cpus > 1) {
> >             x86_cpu_apic_create(cpu, &local_err);
Usage of smp_cpus here looks incorrect to me, it should be max_cpus.
But we can't make it max_cpus without compat glue as it will break
backwards migration (not that upstream cares about it)

Re: [Qemu-devel] [PATCH v3 09/19] pc: delay setting number of boot CPUs to machine_done time

[Eduardo Habkost]

On Tue, Jul 12, 2016 at 03:42:58PM +0200, Igor Mammedov wrote:
> On Tue, 12 Jul 2016 14:48:43 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Tue, 12 Jul 2016 00:29:08 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> [...] 
> > > 1) x86_cpu_realizefn():
> > >     if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC || smp_cpus > 1) {
> > >             x86_cpu_apic_create(cpu, &local_err);
> Usage of smp_cpus here looks incorrect to me, it should be max_cpus.
> But we can't make it max_cpus without compat glue as it will break
> backwards migration (not that upstream cares about it)

We can at least print a warning, by now: "your CPU doesn't have
APIC, CPU hotplug will never work".

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 09/19] pc: delay setting number of boot CPUs to machine_done time

[Igor Mammedov]

On Tue, 12 Jul 2016 14:19:25 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Jul 12, 2016 at 03:42:58PM +0200, Igor Mammedov wrote:
> > On Tue, 12 Jul 2016 14:48:43 +0200
> > Igor Mammedov <imammedo@redhat.com> wrote:
> >   
> > > On Tue, 12 Jul 2016 00:29:08 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:  
> > [...]   
> > > > 1) x86_cpu_realizefn():
> > > >     if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC || smp_cpus > 1) {
> > > >             x86_cpu_apic_create(cpu, &local_err);  
> > Usage of smp_cpus here looks incorrect to me, it should be max_cpus.
> > But we can't make it max_cpus without compat glue as it will break
> > backwards migration (not that upstream cares about it)  
> 
> We can at least print a warning, by now: "your CPU doesn't have
> APIC, CPU hotplug will never work".
sure, lets do it at 2.8 time

Re: [Qemu-devel] [PATCH v3 09/19] pc: delay setting number of boot CPUs to machine_done time

[Eduardo Habkost]

On Tue, Jul 12, 2016 at 02:48:43PM +0200, Igor Mammedov wrote:
> On Tue, 12 Jul 2016 00:29:08 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Jul 06, 2016 at 08:20:45AM +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.  
> > 
> > Do you plan to fix the remaining code using smp_cpus? e.g.:
> perhaps after Drew's -smp refactoring or maybe during it,
> it looks like good candidate for that series,
> I'll work with Drew on that.
> 
> > 
> > 1) x86_cpu_realizefn():
> >     if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC || smp_cpus > 1) {
> >             x86_cpu_apic_create(cpu, &local_err);
> >     [...]
> >   (Maybe we should simply make realizefn fail if we try to create a second CPU
> >   using -device or device_add without CPUID_APIC)
> wouldn't that break some setups that doing it but still able
> to boot?

It would, that's why we need to do it only in the case of -device
or device_add. What about something like:

  if (!(cpu->env.features[FEAT_1_EDX] & CPUID_APIC) && smp_cpus < 2 &&total_cpus_already_created() > 0) {
      error_setg("we can't create a new VCPU without an APIC");
      return;
  }

I believe this logic should be moved to PC code, eventually. Or
at least the process should be controlled by PC code (by setting
a force-apic-creation property in X86CPU, for example).

> 
> > 
> > 2) the smp_cpus checks at hw/i386/kvmvapic.c.
> > 
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  hw/i386/pc.c | 12 +++++++++---
> > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index 6691825..3206572 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,
> > > @@ -1157,10 +1154,19 @@ void pc_cpus_init(PCMachineState *pcms)
> > >  static
> > >  void pc_machine_done(Notifier *notifier, void *data)
> > >  {
> > > +    int i, boot_cpus = 0;
> > >      PCMachineState *pcms = container_of(notifier,
> > >                                          PCMachineState, machine_done);
> > >      PCIBus *bus = pcms->bus;
> > >  
> > > +    for (i = 0; i < pcms->possible_cpus->len; i++) {
> > > +        if (pcms->possible_cpus->cpus[i].cpu) {
> > > +            boot_cpus++;
> > > +        }
> > > +    }  
> > 
> > Any specific reason you chose to check possible_cpus instead of the
> > arch-independent CPU list from exec.c? I believe other architectures will be
> > interested in a generic way to count online CPUs instead of using smp_cpus.
> When others would need to do it we can switch to arch-independent CPU list but
> for now I'd prefer to keep using possible_cpus throughout pc.c for consistency
> reasons (so it would be the single point of bookkeeping CPUs on PC),
> and I most likely will do the same for ARM probably reusing some of PC code.

Common can be done as a follow-up, no problem. In that case,
could you move the calculation to a reusable function in pc.c?
The APIC check above would also need to check how many CPUs
already exist, for example.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 09/19] pc: delay setting number of boot CPUs to machine_done time

[Igor Mammedov]

On Tue, 12 Jul 2016 14:18:22 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Jul 12, 2016 at 02:48:43PM +0200, Igor Mammedov wrote:
> > On Tue, 12 Jul 2016 00:29:08 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Wed, Jul 06, 2016 at 08:20:45AM +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.    
> > > 
> > > Do you plan to fix the remaining code using smp_cpus? e.g.:  
> > perhaps after Drew's -smp refactoring or maybe during it,
> > it looks like good candidate for that series,
> > I'll work with Drew on that.
> >   
> > > 
> > > 1) x86_cpu_realizefn():
> > >     if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC || smp_cpus > 1) {
> > >             x86_cpu_apic_create(cpu, &local_err);
> > >     [...]
> > >   (Maybe we should simply make realizefn fail if we try to create a second CPU
> > >   using -device or device_add without CPUID_APIC)  
> > wouldn't that break some setups that doing it but still able
> > to boot?  
> 
> It would, that's why we need to do it only in the case of -device
> or device_add. What about something like: 
>   if (!(cpu->env.features[FEAT_1_EDX] & CPUID_APIC) && smp_cpus < 2 &&total_cpus_already_created() > 0) {
>       error_setg("we can't create a new VCPU without an APIC");
>       return;
-device/device_add is orthogonal here, cpu-add is also affected.

I'd woul make it a warning instead of hard error:
"broken configuration, only 1st CPU will work when creating multiple CPUs with APIC disabled,
to fix it remove -apic/apic=off from -cpu option"

>   }
> 
> I believe this logic should be moved to PC code, eventually. Or
> at least the process should be controlled by PC code (by setting
> a force-apic-creation property in X86CPU, for example).
smp_cpus check should definitely be part of PC code.

PS:
You do not consider above as part of this series, do you?

> 
> >   
> > > 
> > > 2) the smp_cpus checks at hw/i386/kvmvapic.c.
> > >   
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > >  hw/i386/pc.c | 12 +++++++++---
> > > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > > index 6691825..3206572 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,
> > > > @@ -1157,10 +1154,19 @@ void pc_cpus_init(PCMachineState *pcms)
> > > >  static
> > > >  void pc_machine_done(Notifier *notifier, void *data)
> > > >  {
> > > > +    int i, boot_cpus = 0;
> > > >      PCMachineState *pcms = container_of(notifier,
> > > >                                          PCMachineState, machine_done);
> > > >      PCIBus *bus = pcms->bus;
> > > >  
> > > > +    for (i = 0; i < pcms->possible_cpus->len; i++) {
> > > > +        if (pcms->possible_cpus->cpus[i].cpu) {
> > > > +            boot_cpus++;
> > > > +        }
> > > > +    }    
> > > 
> > > Any specific reason you chose to check possible_cpus instead of the
> > > arch-independent CPU list from exec.c? I believe other architectures will be
> > > interested in a generic way to count online CPUs instead of using smp_cpus.  
> > When others would need to do it we can switch to arch-independent CPU list but
> > for now I'd prefer to keep using possible_cpus throughout pc.c for consistency
> > reasons (so it would be the single point of bookkeeping CPUs on PC),
> > and I most likely will do the same for ARM probably reusing some of PC code.  
> 
> Common can be done as a follow-up, no problem. In that case,
> could you move the calculation to a reusable function in pc.c?
> The APIC check above would also need to check how many CPUs
> already exist, for example.
sure, I'll do it.

Re: [Qemu-devel] [PATCH v3 09/19] pc: delay setting number of boot CPUs to machine_done time

[Eduardo Habkost]

On Wed, Jul 13, 2016 at 09:56:25AM +0200, Igor Mammedov wrote:
> On Tue, 12 Jul 2016 14:18:22 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Tue, Jul 12, 2016 at 02:48:43PM +0200, Igor Mammedov wrote:
> > > On Tue, 12 Jul 2016 00:29:08 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >   
> > > > On Wed, Jul 06, 2016 at 08:20:45AM +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.    
> > > > 
> > > > Do you plan to fix the remaining code using smp_cpus? e.g.:  
> > > perhaps after Drew's -smp refactoring or maybe during it,
> > > it looks like good candidate for that series,
> > > I'll work with Drew on that.
> > >   
> > > > 
> > > > 1) x86_cpu_realizefn():
> > > >     if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC || smp_cpus > 1) {
> > > >             x86_cpu_apic_create(cpu, &local_err);
> > > >     [...]
> > > >   (Maybe we should simply make realizefn fail if we try to create a second CPU
> > > >   using -device or device_add without CPUID_APIC)  
> > > wouldn't that break some setups that doing it but still able
> > > to boot?  
> > 
> > It would, that's why we need to do it only in the case of -device
> > or device_add. What about something like: 
> >   if (!(cpu->env.features[FEAT_1_EDX] & CPUID_APIC) && smp_cpus < 2 &&total_cpus_already_created() > 0) {
> >       error_setg("we can't create a new VCPU without an APIC");
> >       return;
> -device/device_add is orthogonal here, cpu-add is also affected.
> 
> I'd woul make it a warning instead of hard error:
> "broken configuration, only 1st CPU will work when creating multiple CPUs with APIC disabled,
> to fix it remove -apic/apic=off from -cpu option"

I forgot about cpu-add. You're right.

> 
> >   }
> > 
> > I believe this logic should be moved to PC code, eventually. Or
> > at least the process should be controlled by PC code (by setting
> > a force-apic-creation property in X86CPU, for example).
> smp_cpus check should definitely be part of PC code.
> 
> PS:
> You do not consider above as part of this series, do you?

No, it's just something to be done later.

-- 
Eduardo

[Qemu-devel] [PATCH v3 10/19] 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>
---
 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 3206572..0f85b56 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1142,7 +1142,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));
         }
     }
@@ -1697,25 +1696,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.0

Re: [Qemu-devel] [PATCH v3 10/19] pc: register created initial and hotpluged CPUs in one place pc_cpu_plug()

[Bandan Das]

Igor Mammedov <imammedo@redhat.com> writes:

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

So, this takes care of the hotplug case and 09/19 took care of the
coldplug case, right ? If yes, we should probably modify this commit
message a little.

Bandan
> Signed-off-by: Igor Mammedov <imammedo@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 3206572..0f85b56 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1142,7 +1142,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));
>          }
>      }
> @@ -1697,25 +1696,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:

Re: [Qemu-devel] [PATCH v3 10/19] pc: register created initial and hotpluged CPUs in one place pc_cpu_plug()

[Eduardo Habkost]

On Wed, Jul 13, 2016 at 06:32:27PM -0400, Bandan Das wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
> 
> > consolidate possible_cpus array management in pc_cpu_plug()
> > for smp_cpus, coldplugged with -device and hotplugged with
> > device_add.
> 
> So, this takes care of the hotplug case and 09/19 took care of the
> coldplug case, right ? If yes, we should probably modify this commit
> message a little.

This makes pc_cpu_plug() take care of both: now it will handle
the coldplug case also (in addition to the hotplug case, that it
already handled). I don't understand what you mean.

Are you talking about the rtc_set_memory() call, only? This seems
to be the only hotplug-specific code that still remains, in this
patch.

> 
> Bandan
> > Signed-off-by: Igor Mammedov <imammedo@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 3206572..0f85b56 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1142,7 +1142,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));
> >          }
> >      }
> > @@ -1697,25 +1696,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:

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 10/19] pc: register created initial and hotpluged CPUs in one place pc_cpu_plug()

[Bandan Das]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Wed, Jul 13, 2016 at 06:32:27PM -0400, Bandan Das wrote:
>> Igor Mammedov <imammedo@redhat.com> writes:
>> 
>> > consolidate possible_cpus array management in pc_cpu_plug()
>> > for smp_cpus, coldplugged with -device and hotplugged with
>> > device_add.
>> 
>> So, this takes care of the hotplug case and 09/19 took care of the
>> coldplug case, right ? If yes, we should probably modify this commit
>> message a little.
>
> This makes pc_cpu_plug() take care of both: now it will handle
> the coldplug case also (in addition to the hotplug case, that it
> already handled). I don't understand what you mean.
>
> Are you talking about the rtc_set_memory() call, only? This seems
> to be the only hotplug-specific code that still remains, in this

Right, I thought the rtc_set_memory() call in 09/19 takes care of the
case when the user boots with "-device" and this takes care of the case
when user invokes device_add, no ?

> patch.
>
>> 
>> Bandan
>> > Signed-off-by: Igor Mammedov <imammedo@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 3206572..0f85b56 100644
>> > --- a/hw/i386/pc.c
>> > +++ b/hw/i386/pc.c
>> > @@ -1142,7 +1142,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));
>> >          }
>> >      }
>> > @@ -1697,25 +1696,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:

Re: [Qemu-devel] [PATCH v3 10/19] pc: register created initial and hotpluged CPUs in one place pc_cpu_plug()

[Eduardo Habkost]

On Wed, Jul 13, 2016 at 06:59:21PM -0400, Bandan Das wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Wed, Jul 13, 2016 at 06:32:27PM -0400, Bandan Das wrote:
> >> Igor Mammedov <imammedo@redhat.com> writes:
> >> 
> >> > consolidate possible_cpus array management in pc_cpu_plug()
> >> > for smp_cpus, coldplugged with -device and hotplugged with
> >> > device_add.
> >> 
> >> So, this takes care of the hotplug case and 09/19 took care of the
> >> coldplug case, right ? If yes, we should probably modify this commit
> >> message a little.
> >
> > This makes pc_cpu_plug() take care of both: now it will handle
> > the coldplug case also (in addition to the hotplug case, that it
> > already handled). I don't understand what you mean.
> >
> > Are you talking about the rtc_set_memory() call, only? This seems
> > to be the only hotplug-specific code that still remains, in this
> 
> Right, I thought the rtc_set_memory() call in 09/19 takes care of the
> case when the user boots with "-device" and this takes care of the case
> when user invokes device_add, no ?

Yes for rtc_set_memory(). But the main purpose of this patch is
to reuse all the rest of pc_cpu_plug() (i.e. everything except
for the rtc_set_memory() call) for the coldplug CPUs too.

Note that I didn't review the patch completely, yet. The
replacement for the possible_cpus code looks OK, but I didn't
check if it is safe to call
HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev)->plug() for coldplugged
CPUs too.

> 
> > patch.
> >
> >> 
> >> Bandan
> >> > Signed-off-by: Igor Mammedov <imammedo@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 3206572..0f85b56 100644
> >> > --- a/hw/i386/pc.c
> >> > +++ b/hw/i386/pc.c
> >> > @@ -1142,7 +1142,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));
> >> >          }
> >> >      }
> >> > @@ -1697,25 +1696,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:

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 10/19] pc: register created initial and hotpluged CPUs in one place pc_cpu_plug()

[Bandan Das]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Wed, Jul 13, 2016 at 06:59:21PM -0400, Bandan Das wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> 
>> > On Wed, Jul 13, 2016 at 06:32:27PM -0400, Bandan Das wrote:
>> >> Igor Mammedov <imammedo@redhat.com> writes:
>> >> 
>> >> > consolidate possible_cpus array management in pc_cpu_plug()
>> >> > for smp_cpus, coldplugged with -device and hotplugged with
>> >> > device_add.
>> >> 
>> >> So, this takes care of the hotplug case and 09/19 took care of the
>> >> coldplug case, right ? If yes, we should probably modify this commit
>> >> message a little.
>> >
>> > This makes pc_cpu_plug() take care of both: now it will handle
>> > the coldplug case also (in addition to the hotplug case, that it
>> > already handled). I don't understand what you mean.
>> >
>> > Are you talking about the rtc_set_memory() call, only? This seems
>> > to be the only hotplug-specific code that still remains, in this
>> 
>> Right, I thought the rtc_set_memory() call in 09/19 takes care of the
>> case when the user boots with "-device" and this takes care of the case
>> when user invokes device_add, no ?
>
> Yes for rtc_set_memory(). But the main purpose of this patch is
> to reuse all the rest of pc_cpu_plug() (i.e. everything except
> for the rtc_set_memory() call) for the coldplug CPUs too.
>
> Note that I didn't review the patch completely, yet. The
> replacement for the possible_cpus code looks OK, but I didn't
> check if it is safe to call
> HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev)->plug() for coldplugged
> CPUs too.

Owing to "HOTPLUG_HANDLER_GET_CLASS" name, I think I incorrectly assumed that
pc_cpu_plug() is only invoked for the hotplug case. I understand the hotplug
cpu path which iiuc is via pc_hot_add_cpu()->realized->pc_cpu_plug but I am not
sure of "-device" case. I will take a closer look.

Anyway, if this patch takes care of both cases, the commit message makes sense.

Thanks,
Bandan


>> 
>> > patch.
>> >
>> >> 
>> >> Bandan
>> >> > Signed-off-by: Igor Mammedov <imammedo@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 3206572..0f85b56 100644
>> >> > --- a/hw/i386/pc.c
>> >> > +++ b/hw/i386/pc.c
>> >> > @@ -1142,7 +1142,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));
>> >> >          }
>> >> >      }
>> >> > @@ -1697,25 +1696,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:

Re: [Qemu-devel] [PATCH v3 10/19] pc: register created initial and hotpluged CPUs in one place pc_cpu_plug()

[Igor Mammedov]

On Wed, 13 Jul 2016 20:37:13 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Jul 13, 2016 at 06:59:21PM -0400, Bandan Das wrote:
> > Eduardo Habkost <ehabkost@redhat.com> writes:
> >   
> > > On Wed, Jul 13, 2016 at 06:32:27PM -0400, Bandan Das wrote:  
> > >> Igor Mammedov <imammedo@redhat.com> writes:
> > >>   
> > >> > consolidate possible_cpus array management in pc_cpu_plug()
> > >> > for smp_cpus, coldplugged with -device and hotplugged with
> > >> > device_add.  
> > >> 
> > >> So, this takes care of the hotplug case and 09/19 took care of the
> > >> coldplug case, right ? If yes, we should probably modify this commit
> > >> message a little.  
> > >
> > > This makes pc_cpu_plug() take care of both: now it will handle
> > > the coldplug case also (in addition to the hotplug case, that it
> > > already handled). I don't understand what you mean.
> > >
> > > Are you talking about the rtc_set_memory() call, only? This seems
> > > to be the only hotplug-specific code that still remains, in this  
> > 
> > Right, I thought the rtc_set_memory() call in 09/19 takes care of the
> > case when the user boots with "-device" and this takes care of the case
> > when user invokes device_add, no ?  
> 
> Yes for rtc_set_memory(). But the main purpose of this patch is
> to reuse all the rest of pc_cpu_plug() (i.e. everything except
> for the rtc_set_memory() call) for the coldplug CPUs too.
> 
> Note that I didn't review the patch completely, yet. The
> replacement for the possible_cpus code looks OK, but I didn't
> check if it is safe to call
> HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev)->plug() for coldplugged
> CPUs too.
for cpus created at  pc_cpus_init() time, that path is unreachable
since pcms->acpi_dev == NULL,

even if in the future that path becomes reachable
(when we could specify all CPUs including BSP with -device, it doesn't work for BSP yet)
 i.e. pcms->acpi_dev != NULL
then acpi_device will have be initialized see cpu_hotplug_hw_init()
and following plug handler will be called acpi_cpu_plug_cb() doing what
it was supposed to do.

> 
> >   
> > > patch.
> > >  
> > >> 
> > >> Bandan  
> > >> > Signed-off-by: Igor Mammedov <imammedo@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 3206572..0f85b56 100644
> > >> > --- a/hw/i386/pc.c
> > >> > +++ b/hw/i386/pc.c
> > >> > @@ -1142,7 +1142,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));
> > >> >          }
> > >> >      }
> > >> > @@ -1697,25 +1696,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:  
> 

Re: [Qemu-devel] [PATCH v3 10/19] pc: register created initial and hotpluged CPUs in one place pc_cpu_plug()

[Eduardo Habkost]

On Thu, Jul 14, 2016 at 11:18:36AM +0200, Igor Mammedov wrote:
> On Wed, 13 Jul 2016 20:37:13 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Jul 13, 2016 at 06:59:21PM -0400, Bandan Das wrote:
> > > Eduardo Habkost <ehabkost@redhat.com> writes:
> > >   
> > > > On Wed, Jul 13, 2016 at 06:32:27PM -0400, Bandan Das wrote:  
> > > >> Igor Mammedov <imammedo@redhat.com> writes:
> > > >>   
> > > >> > consolidate possible_cpus array management in pc_cpu_plug()
> > > >> > for smp_cpus, coldplugged with -device and hotplugged with
> > > >> > device_add.  
> > > >> 
> > > >> So, this takes care of the hotplug case and 09/19 took care of the
> > > >> coldplug case, right ? If yes, we should probably modify this commit
> > > >> message a little.  
> > > >
> > > > This makes pc_cpu_plug() take care of both: now it will handle
> > > > the coldplug case also (in addition to the hotplug case, that it
> > > > already handled). I don't understand what you mean.
> > > >
> > > > Are you talking about the rtc_set_memory() call, only? This seems
> > > > to be the only hotplug-specific code that still remains, in this  
> > > 
> > > Right, I thought the rtc_set_memory() call in 09/19 takes care of the
> > > case when the user boots with "-device" and this takes care of the case
> > > when user invokes device_add, no ?  
> > 
> > Yes for rtc_set_memory(). But the main purpose of this patch is
> > to reuse all the rest of pc_cpu_plug() (i.e. everything except
> > for the rtc_set_memory() call) for the coldplug CPUs too.
> > 
> > Note that I didn't review the patch completely, yet. The
> > replacement for the possible_cpus code looks OK, but I didn't
> > check if it is safe to call
> > HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev)->plug() for coldplugged
> > CPUs too.
> for cpus created at  pc_cpus_init() time, that path is unreachable
> since pcms->acpi_dev == NULL,

Sounds fragile, as it requires specific initialization ordering
in the pc_cpus_init() caller. But:

> 
> even if in the future that path becomes reachable
> (when we could specify all CPUs including BSP with -device, it doesn't work for BSP yet)
>  i.e. pcms->acpi_dev != NULL
> then acpi_device will have be initialized see cpu_hotplug_hw_init()
> and following plug handler will be called acpi_cpu_plug_cb() doing what
> it was supposed to do.

For reference for others, acpi_cpu_plug_cb() is:

void acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
                      CPUHotplugState *cpu_st, DeviceState *dev, Error **errp)
{
    AcpiCpuStatus *cdev;

    cdev = get_cpu_status(cpu_st, dev);
    if (!cdev) {
        return;
    }

    cdev->cpu = CPU(dev);
    if (dev->hotplugged) {
        cdev->is_inserting = true;
        acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS);
    }
}

get_cpu_status() should return non-NULL for all possible CPUs. So the only code
for coldplug case is cdev->cpu = CPU(dev), which is really something we want to
run.

So, that means it works properly on both cases: if pcms->acpi_dev==NULL and if
pcms->acpi_dev != NULL.

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

Related question: why does CPUHotplugState needs to duplicate the list of
(CPUState *cpu, uint64_t arch_id) pairs, if PCMachineState already maintains a
list with exactly the same information?

> 
> > 
> > >   
> > > > patch.
> > > >  
> > > >> 
> > > >> Bandan  
> > > >> > Signed-off-by: Igor Mammedov <imammedo@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 3206572..0f85b56 100644
> > > >> > --- a/hw/i386/pc.c
> > > >> > +++ b/hw/i386/pc.c
> > > >> > @@ -1142,7 +1142,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));
> > > >> >          }
> > > >> >      }
> > > >> > @@ -1697,25 +1696,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:  
> > 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 10/19] pc: register created initial and hotpluged CPUs in one place pc_cpu_plug()

[Igor Mammedov]

On Thu, 14 Jul 2016 12:03:19 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Jul 14, 2016 at 11:18:36AM +0200, Igor Mammedov wrote:
> > On Wed, 13 Jul 2016 20:37:13 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Wed, Jul 13, 2016 at 06:59:21PM -0400, Bandan Das wrote:  
> > > > Eduardo Habkost <ehabkost@redhat.com> writes:
> > > >     
> > > > > On Wed, Jul 13, 2016 at 06:32:27PM -0400, Bandan Das wrote:    
> > > > >> Igor Mammedov <imammedo@redhat.com> writes:
> > > > >>     
> > > > >> > consolidate possible_cpus array management in pc_cpu_plug()
> > > > >> > for smp_cpus, coldplugged with -device and hotplugged with
> > > > >> > device_add.    
> > > > >> 
> > > > >> So, this takes care of the hotplug case and 09/19 took care of the
> > > > >> coldplug case, right ? If yes, we should probably modify this commit
> > > > >> message a little.    
> > > > >
> > > > > This makes pc_cpu_plug() take care of both: now it will handle
> > > > > the coldplug case also (in addition to the hotplug case, that it
> > > > > already handled). I don't understand what you mean.
> > > > >
> > > > > Are you talking about the rtc_set_memory() call, only? This seems
> > > > > to be the only hotplug-specific code that still remains, in this    
> > > > 
> > > > Right, I thought the rtc_set_memory() call in 09/19 takes care of the
> > > > case when the user boots with "-device" and this takes care of the case
> > > > when user invokes device_add, no ?    
> > > 
> > > Yes for rtc_set_memory(). But the main purpose of this patch is
> > > to reuse all the rest of pc_cpu_plug() (i.e. everything except
> > > for the rtc_set_memory() call) for the coldplug CPUs too.
> > > 
> > > Note that I didn't review the patch completely, yet. The
> > > replacement for the possible_cpus code looks OK, but I didn't
> > > check if it is safe to call
> > > HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev)->plug() for coldplugged
> > > CPUs too.  
> > for cpus created at  pc_cpus_init() time, that path is unreachable
> > since pcms->acpi_dev == NULL,  
> 
> Sounds fragile, as it requires specific initialization ordering
> in the pc_cpus_init() caller. But:
> 
> > 
> > even if in the future that path becomes reachable
> > (when we could specify all CPUs including BSP with -device, it doesn't work for BSP yet)
> >  i.e. pcms->acpi_dev != NULL
> > then acpi_device will have be initialized see cpu_hotplug_hw_init()
> > and following plug handler will be called acpi_cpu_plug_cb() doing what
> > it was supposed to do.  
> 
> For reference for others, acpi_cpu_plug_cb() is:
> 
> void acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
>                       CPUHotplugState *cpu_st, DeviceState *dev, Error **errp)
> {
>     AcpiCpuStatus *cdev;
> 
>     cdev = get_cpu_status(cpu_st, dev);
>     if (!cdev) {
>         return;
>     }
> 
>     cdev->cpu = CPU(dev);
>     if (dev->hotplugged) {
>         cdev->is_inserting = true;
>         acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS);
>     }
> }
> 
> get_cpu_status() should return non-NULL for all possible CPUs. So the only code
> for coldplug case is cdev->cpu = CPU(dev), which is really something we want to
> run.
> 
> So, that means it works properly on both cases: if pcms->acpi_dev==NULL and if
> pcms->acpi_dev != NULL.
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> Related question: why does CPUHotplugState needs to duplicate the list of
> (CPUState *cpu, uint64_t arch_id) pairs, if PCMachineState already maintains a
> list with exactly the same information?
It maintains an array of AcpiCpuStatus, which is not only what
PCMachineState::possible_cpus is but an additional ACPI related
hot-plug state for each possible CPU.

Probably PCMachineState::possible_cpus could be used in hw/acpi/cpu.c
but I've not looked at it. It doesn't look like directly accessing
PCMachineState::possible_cpus would give anything beside leaving out
managing AcpiCpuStatus.cpu (which serves as present/missing flag beside
of providing direct access to CPU instance when needed).

And I obviously don't want to push ACPI runtime state into generic
possible_cpus.



> 
> >   
> > >   
> > > >     
> > > > > patch.
> > > > >    
> > > > >> 
> > > > >> Bandan    
> > > > >> > Signed-off-by: Igor Mammedov <imammedo@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 3206572..0f85b56 100644
> > > > >> > --- a/hw/i386/pc.c
> > > > >> > +++ b/hw/i386/pc.c
> > > > >> > @@ -1142,7 +1142,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));
> > > > >> >          }
> > > > >> >      }
> > > > >> > @@ -1697,25 +1696,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:    
> > >   
> >   
> 

Re: [Qemu-devel] [PATCH v3 10/19] pc: register created initial and hotpluged CPUs in one place pc_cpu_plug()

[Eduardo Habkost]

On Thu, Jul 14, 2016 at 05:40:47PM +0200, Igor Mammedov wrote:
> On Thu, 14 Jul 2016 12:03:19 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Jul 14, 2016 at 11:18:36AM +0200, Igor Mammedov wrote:
> > > On Wed, 13 Jul 2016 20:37:13 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >   
> > > > On Wed, Jul 13, 2016 at 06:59:21PM -0400, Bandan Das wrote:  
> > > > > Eduardo Habkost <ehabkost@redhat.com> writes:
> > > > >     
> > > > > > On Wed, Jul 13, 2016 at 06:32:27PM -0400, Bandan Das wrote:    
> > > > > >> Igor Mammedov <imammedo@redhat.com> writes:
> > > > > >>     
> > > > > >> > consolidate possible_cpus array management in pc_cpu_plug()
> > > > > >> > for smp_cpus, coldplugged with -device and hotplugged with
> > > > > >> > device_add.    
> > > > > >> 
> > > > > >> So, this takes care of the hotplug case and 09/19 took care of the
> > > > > >> coldplug case, right ? If yes, we should probably modify this commit
> > > > > >> message a little.    
> > > > > >
> > > > > > This makes pc_cpu_plug() take care of both: now it will handle
> > > > > > the coldplug case also (in addition to the hotplug case, that it
> > > > > > already handled). I don't understand what you mean.
> > > > > >
> > > > > > Are you talking about the rtc_set_memory() call, only? This seems
> > > > > > to be the only hotplug-specific code that still remains, in this    
> > > > > 
> > > > > Right, I thought the rtc_set_memory() call in 09/19 takes care of the
> > > > > case when the user boots with "-device" and this takes care of the case
> > > > > when user invokes device_add, no ?    
> > > > 
> > > > Yes for rtc_set_memory(). But the main purpose of this patch is
> > > > to reuse all the rest of pc_cpu_plug() (i.e. everything except
> > > > for the rtc_set_memory() call) for the coldplug CPUs too.
> > > > 
> > > > Note that I didn't review the patch completely, yet. The
> > > > replacement for the possible_cpus code looks OK, but I didn't
> > > > check if it is safe to call
> > > > HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev)->plug() for coldplugged
> > > > CPUs too.  
> > > for cpus created at  pc_cpus_init() time, that path is unreachable
> > > since pcms->acpi_dev == NULL,  
> > 
> > Sounds fragile, as it requires specific initialization ordering
> > in the pc_cpus_init() caller. But:
> > 
> > > 
> > > even if in the future that path becomes reachable
> > > (when we could specify all CPUs including BSP with -device, it doesn't work for BSP yet)
> > >  i.e. pcms->acpi_dev != NULL
> > > then acpi_device will have be initialized see cpu_hotplug_hw_init()
> > > and following plug handler will be called acpi_cpu_plug_cb() doing what
> > > it was supposed to do.  
> > 
> > For reference for others, acpi_cpu_plug_cb() is:
> > 
> > void acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
> >                       CPUHotplugState *cpu_st, DeviceState *dev, Error **errp)
> > {
> >     AcpiCpuStatus *cdev;
> > 
> >     cdev = get_cpu_status(cpu_st, dev);
> >     if (!cdev) {
> >         return;
> >     }
> > 
> >     cdev->cpu = CPU(dev);
> >     if (dev->hotplugged) {
> >         cdev->is_inserting = true;
> >         acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS);
> >     }
> > }
> > 
> > get_cpu_status() should return non-NULL for all possible CPUs. So the only code
> > for coldplug case is cdev->cpu = CPU(dev), which is really something we want to
> > run.
> > 
> > So, that means it works properly on both cases: if pcms->acpi_dev==NULL and if
> > pcms->acpi_dev != NULL.
> > 
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > 
> > Related question: why does CPUHotplugState needs to duplicate the list of
> > (CPUState *cpu, uint64_t arch_id) pairs, if PCMachineState already maintains a
> > list with exactly the same information?
> It maintains an array of AcpiCpuStatus, which is not only what
> PCMachineState::possible_cpus is but an additional ACPI related
> hot-plug state for each possible CPU.
> 
> Probably PCMachineState::possible_cpus could be used in hw/acpi/cpu.c
> but I've not looked at it. It doesn't look like directly accessing
> PCMachineState::possible_cpus would give anything beside leaving out
> managing AcpiCpuStatus.cpu (which serves as present/missing flag beside
> of providing direct access to CPU instance when needed).

Yeah, it looks like all we could get from it is the removal of
the 'cdev->cpu = CPU(dev)' line from acpi_cpu_plug_cb(). Maybe
later, if we have other reasons for a generic (and efficient)
arch_id->CPU lookup mechanism in MachineState.

> 
> And I obviously don't want to push ACPI runtime state into generic
> possible_cpus.

Right.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 10/19] pc: register created initial and hotpluged CPUs in one place pc_cpu_plug()

[Igor Mammedov]

On Thu, 14 Jul 2016 13:43:29 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Jul 14, 2016 at 05:40:47PM +0200, Igor Mammedov wrote:
> > On Thu, 14 Jul 2016 12:03:19 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Thu, Jul 14, 2016 at 11:18:36AM +0200, Igor Mammedov wrote:  
> > > > On Wed, 13 Jul 2016 20:37:13 -0300
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >     
> > > > > On Wed, Jul 13, 2016 at 06:59:21PM -0400, Bandan Das wrote:    
> > > > > > Eduardo Habkost <ehabkost@redhat.com> writes:
> > > > > >       
> > > > > > > On Wed, Jul 13, 2016 at 06:32:27PM -0400, Bandan Das wrote:      
> > > > > > >> Igor Mammedov <imammedo@redhat.com> writes:
> > > > > > >>       
> > > > > > >> > consolidate possible_cpus array management in pc_cpu_plug()
> > > > > > >> > for smp_cpus, coldplugged with -device and hotplugged with
> > > > > > >> > device_add.      
> > > > > > >> 
> > > > > > >> So, this takes care of the hotplug case and 09/19 took care of the
> > > > > > >> coldplug case, right ? If yes, we should probably modify this commit
> > > > > > >> message a little.      
> > > > > > >
> > > > > > > This makes pc_cpu_plug() take care of both: now it will handle
> > > > > > > the coldplug case also (in addition to the hotplug case, that it
> > > > > > > already handled). I don't understand what you mean.
> > > > > > >
> > > > > > > Are you talking about the rtc_set_memory() call, only? This seems
> > > > > > > to be the only hotplug-specific code that still remains, in this      
> > > > > > 
> > > > > > Right, I thought the rtc_set_memory() call in 09/19 takes care of the
> > > > > > case when the user boots with "-device" and this takes care of the case
> > > > > > when user invokes device_add, no ?      
> > > > > 
> > > > > Yes for rtc_set_memory(). But the main purpose of this patch is
> > > > > to reuse all the rest of pc_cpu_plug() (i.e. everything except
> > > > > for the rtc_set_memory() call) for the coldplug CPUs too.
> > > > > 
> > > > > Note that I didn't review the patch completely, yet. The
> > > > > replacement for the possible_cpus code looks OK, but I didn't
> > > > > check if it is safe to call
> > > > > HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev)->plug() for coldplugged
> > > > > CPUs too.    
> > > > for cpus created at  pc_cpus_init() time, that path is unreachable
> > > > since pcms->acpi_dev == NULL,    
> > > 
> > > Sounds fragile, as it requires specific initialization ordering
> > > in the pc_cpus_init() caller. But:
> > >   
> > > > 
> > > > even if in the future that path becomes reachable
> > > > (when we could specify all CPUs including BSP with -device, it doesn't work for BSP yet)
> > > >  i.e. pcms->acpi_dev != NULL
> > > > then acpi_device will have be initialized see cpu_hotplug_hw_init()
> > > > and following plug handler will be called acpi_cpu_plug_cb() doing what
> > > > it was supposed to do.    
> > > 
> > > For reference for others, acpi_cpu_plug_cb() is:
> > > 
> > > void acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
> > >                       CPUHotplugState *cpu_st, DeviceState *dev, Error **errp)
> > > {
> > >     AcpiCpuStatus *cdev;
> > > 
> > >     cdev = get_cpu_status(cpu_st, dev);
> > >     if (!cdev) {
> > >         return;
> > >     }
> > > 
> > >     cdev->cpu = CPU(dev);
> > >     if (dev->hotplugged) {
> > >         cdev->is_inserting = true;
> > >         acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS);
> > >     }
> > > }
> > > 
> > > get_cpu_status() should return non-NULL for all possible CPUs. So the only code
> > > for coldplug case is cdev->cpu = CPU(dev), which is really something we want to
> > > run.
> > > 
> > > So, that means it works properly on both cases: if pcms->acpi_dev==NULL and if
> > > pcms->acpi_dev != NULL.
> > > 
> > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > > 
> > > Related question: why does CPUHotplugState needs to duplicate the list of
> > > (CPUState *cpu, uint64_t arch_id) pairs, if PCMachineState already maintains a
> > > list with exactly the same information?  
> > It maintains an array of AcpiCpuStatus, which is not only what
> > PCMachineState::possible_cpus is but an additional ACPI related
> > hot-plug state for each possible CPU.
> > 
> > Probably PCMachineState::possible_cpus could be used in hw/acpi/cpu.c
> > but I've not looked at it. It doesn't look like directly accessing
> > PCMachineState::possible_cpus would give anything beside leaving out
> > managing AcpiCpuStatus.cpu (which serves as present/missing flag beside
> > of providing direct access to CPU instance when needed).  
> 
> Yeah, it looks like all we could get from it is the removal of
> the 'cdev->cpu = CPU(dev)' line from acpi_cpu_plug_cb(). Maybe
> later, if we have other reasons for a generic (and efficient)
> arch_id->CPU lookup mechanism in MachineState.
yep, I have an idea to replace lookups of apic_id by reverse conversion
of apic_id to index in possible_cpus, but yep it's material for 2.8

> 
> > 
> > And I obviously don't want to push ACPI runtime state into generic
> > possible_cpus.  
> 
> Right.
> 

[Qemu-devel] [PATCH v3 11/19] 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 1ec40a0..ebf4140 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3239,6 +3239,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.0

[Qemu-devel] [PATCH v3 12/19] 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>
---
 hw/intc/apic.c                  | 10 ++++++++++
 hw/intc/apic_common.c           |  8 --------
 include/hw/i386/apic_internal.h |  4 +---
 3 files changed, 11 insertions(+), 11 deletions(-)

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);
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 74fe935..5d3be9a 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -120,8 +120,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"
@@ -175,7 +173,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;
-- 
2.7.0

Re: [Qemu-devel] [PATCH v3 12/19] apic: move MAX_APICS check to 'apic' class

[Bandan Das]

Igor Mammedov <imammedo@redhat.com> writes:

> 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>
> ---
>  hw/intc/apic.c                  | 10 ++++++++++
>  hw/intc/apic_common.c           |  8 --------
>  include/hw/i386/apic_internal.h |  4 +---
>  3 files changed, 11 insertions(+), 11 deletions(-)
>
> 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;

Can this be a global ? I understand there are no other users but
maybe it fits alongside local_apics.

> +    if (apic_no >= MAX_APICS) {
> +        error_setg(errp, "%s initialization failed.",
> +                   object_get_typename(OBJECT(dev)));
> +        return;
> +    }
> +    s->idx = apic_no++;

Is there a possibility of a race here with the apic removal path ?

Bandan

>      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);
> diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
> index 74fe935..5d3be9a 100644
> --- a/include/hw/i386/apic_internal.h
> +++ b/include/hw/i386/apic_internal.h
> @@ -120,8 +120,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"
> @@ -175,7 +173,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;

Re: [Qemu-devel] [PATCH v3 12/19] apic: move MAX_APICS check to 'apic' class

[Eduardo Habkost]

On Wed, Jul 13, 2016 at 06:47:20PM -0400, Bandan Das wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
> 
> > 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>
> > ---
> >  hw/intc/apic.c                  | 10 ++++++++++
> >  hw/intc/apic_common.c           |  8 --------
> >  include/hw/i386/apic_internal.h |  4 +---
> >  3 files changed, 11 insertions(+), 11 deletions(-)
> >
> > 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;
> 
> Can this be a global ? I understand there are no other users but
> maybe it fits alongside local_apics.

The variable is removed by the next patch. :)

> 
> > +    if (apic_no >= MAX_APICS) {
> > +        error_setg(errp, "%s initialization failed.",
> > +                   object_get_typename(OBJECT(dev)));
> > +        return;
> > +    }
> > +    s->idx = apic_no++;
> 
> Is there a possibility of a race here with the apic removal path ?

If there's that possibility: 1) it was already in
apic_common_realize(); 2) it is removed by patch 13/19.

> 
> Bandan
> 
> >      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);
> > diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
> > index 74fe935..5d3be9a 100644
> > --- a/include/hw/i386/apic_internal.h
> > +++ b/include/hw/i386/apic_internal.h
> > @@ -120,8 +120,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"
> > @@ -175,7 +173,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;

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 12/19] apic: move MAX_APICS check to 'apic' class

[Bandan Das]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Wed, Jul 13, 2016 at 06:47:20PM -0400, Bandan Das wrote:
>> Igor Mammedov <imammedo@redhat.com> writes:
>> 
>> > 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>
>> > ---
>> >  hw/intc/apic.c                  | 10 ++++++++++
>> >  hw/intc/apic_common.c           |  8 --------
>> >  include/hw/i386/apic_internal.h |  4 +---
>> >  3 files changed, 11 insertions(+), 11 deletions(-)
>> >
>> > 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;
>> 
>> Can this be a global ? I understand there are no other users but
>> maybe it fits alongside local_apics.
>
> The variable is removed by the next patch. :)
>
>> 
>> > +    if (apic_no >= MAX_APICS) {
>> > +        error_setg(errp, "%s initialization failed.",
>> > +                   object_get_typename(OBJECT(dev)));
>> > +        return;
>> > +    }
>> > +    s->idx = apic_no++;
>> 
>> Is there a possibility of a race here with the apic removal path ?
>
> If there's that possibility: 1) it was already in
> apic_common_realize(); 2) it is removed by patch 13/19.

Ugh! Stupid me :) Yep, thanks for pointing that out.

>> 
>> Bandan
>> 
>> >      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);
>> > diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
>> > index 74fe935..5d3be9a 100644
>> > --- a/include/hw/i386/apic_internal.h
>> > +++ b/include/hw/i386/apic_internal.h
>> > @@ -120,8 +120,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"
>> > @@ -175,7 +173,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;

[Qemu-devel] [PATCH v3 13/19] 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>
---
 hw/intc/apic.c                  | 16 +++++++---------
 include/hw/i386/apic_internal.h |  1 -
 2 files changed, 7 insertions(+), 10 deletions(-)

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;
 }
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 5d3be9a..e5d1550 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -173,7 +173,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;
-- 
2.7.0

[Qemu-devel] [PATCH v3 14/19] (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>
---
 hw/i386/kvm/apic.c              |  5 +++++
 hw/intc/apic.c                  | 10 ++++++++++
 hw/intc/apic_common.c           | 13 +++++++++++++
 include/hw/i386/apic_internal.h |  1 +
 4 files changed, 29 insertions(+)

diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index c5983c7..6cde5f1 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()
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index e5d1550..52ca45d 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -135,6 +135,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);
-- 
2.7.0

[Qemu-devel] [PATCH v3 15/19] 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>
---
 hw/intc/apic_common.c           | 12 +++++++++++-
 include/hw/i386/apic_internal.h |  1 +
 include/hw/i386/pc.h            |  5 +++++
 3 files changed, 17 insertions(+), 1 deletion(-)

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;
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 52ca45d..a694322 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -182,6 +182,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 7e43b20..e2f0426 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -374,6 +374,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
         .driver   = TYPE_X86_CPU,\
         .property = "cpuid-0xb",\
         .value    = "off",\
+    },\
+    {\
+        .driver   = "apic",\
+        .property = "legacy-instance-id",\
+        .value    = "on",\
     },
 
 #define PC_COMPAT_2_5 \
-- 
2.7.0

Re: [Qemu-devel] [PATCH v3 15/19] apic: use apic_id as apic's migration instance_id

[Dr. David Alan Gilbert]

* Igor Mammedov (imammedo@redhat.com) wrote:
> 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>

OK, I think this works from the migration side of things.
The old version used the qdev registration but since you didn't
have a bus it was still the simple "apic" naming, so I don't think
that changes.
Since you're tieing it to machine type we never have to worry
about clashes between the old and new numbering.

So, from the migration side of things I think this is about
as simple a fix as we're going to see, although I'm
expecting you're running into similar problems in other devices.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Dave

> ---
>  hw/intc/apic_common.c           | 12 +++++++++++-
>  include/hw/i386/apic_internal.h |  1 +
>  include/hw/i386/pc.h            |  5 +++++
>  3 files changed, 17 insertions(+), 1 deletion(-)
> 
> 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;
> diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
> index 52ca45d..a694322 100644
> --- a/include/hw/i386/apic_internal.h
> +++ b/include/hw/i386/apic_internal.h
> @@ -182,6 +182,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 7e43b20..e2f0426 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -374,6 +374,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>          .driver   = TYPE_X86_CPU,\
>          .property = "cpuid-0xb",\
>          .value    = "off",\
> +    },\
> +    {\
> +        .driver   = "apic",\
> +        .property = "legacy-instance-id",\
> +        .value    = "on",\
>      },
>  
>  #define PC_COMPAT_2_5 \
> -- 
> 2.7.0
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH v3 16/19] 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>
---
 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 ebf4140..04c0b79 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2763,8 +2763,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.0

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

[Eduardo Habkost]

On Wed, Jul 06, 2016 at 08:20:52AM +0200, Igor Mammedov wrote:
> 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>

-- 
Eduardo

[Qemu-devel] [PATCH v3 17/19] target-i386: fix apic object leak when CPU is deleted

[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 04c0b79..2fa445d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2765,6 +2765,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.0

Re: [Qemu-devel] [PATCH v3 17/19] target-i386: fix apic object leak when CPU is deleted

[Eduardo Habkost]

On Wed, Jul 06, 2016 at 08:20:53AM +0200, Igor Mammedov wrote:
> 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 04c0b79..2fa445d 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2765,6 +2765,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));

What kind of event can trigger object_unparent() or
object_del_property() on "lapic"? Can we guarantee that the child
property will never be deleted by any other code, only by
x86_cpu_unrealizefn() and object_finalize(cpu)?

Because with this change, deleting the property will leave us
with with a dangling cpu->apic_state pointer.

>  
>      qdev_prop_set_uint8(cpu->apic_state, "id", cpu->apic_id);
>      /* TODO: convert to link<> */
> -- 
> 2.7.0
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 17/19] target-i386: fix apic object leak when CPU is deleted

[Igor Mammedov]

On Wed, 13 Jul 2016 12:04:44 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Jul 06, 2016 at 08:20:53AM +0200, Igor Mammedov wrote:
> > 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 04c0b79..2fa445d 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2765,6 +2765,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));  
> 
> What kind of event can trigger object_unparent() or
> object_del_property() on "lapic"? Can we guarantee that the child
> property will never be deleted by any other code, only by
> x86_cpu_unrealizefn() and object_finalize(cpu)?
code path that triggers unparent of lapic implicitly is
cpu instance removal when it deletes all children.

So unless someone adds explicit lapic removal somewhere in target-i386/cpu.c
I don't see how it could be deleted by other code path.

> Because with this change, deleting the property will leave us
> with with a dangling cpu->apic_state pointer.
since there aren't other place that deletes lapic property we won't get it
dangling pointer, see the next patch comment for call chain

> 
> >  
> >      qdev_prop_set_uint8(cpu->apic_state, "id", cpu->apic_id);
> >      /* TODO: convert to link<> */
> > -- 
> > 2.7.0
> >   
> 

Re: [Qemu-devel] [PATCH v3 17/19] target-i386: fix apic object leak when CPU is deleted

[Igor Mammedov]

On Wed, 13 Jul 2016 17:26:18 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Wed, 13 Jul 2016 12:04:44 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Jul 06, 2016 at 08:20:53AM +0200, Igor Mammedov wrote:  
> > > 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 04c0b79..2fa445d 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -2765,6 +2765,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));    
> > 
> > What kind of event can trigger object_unparent() or
> > object_del_property() on "lapic"? Can we guarantee that the child
> > property will never be deleted by any other code, only by
> > x86_cpu_unrealizefn() and object_finalize(cpu)?  
> code path that triggers unparent of lapic implicitly is
> cpu instance removal when it deletes all children.
> 
> So unless someone adds explicit lapic removal somewhere in target-i386/cpu.c
Well, I've wrote nonsense here as I do remove child explicitly
in x86_cpu_unrealizefn(), so it's fine to set cpu->apic_state to NULL
as you suggest in 18/19.

The other way around might be call only apic_state->unrealize() explicitly
from x86_cpu_unrealizefn() and let QOM do unparenting/finalizing
automatically for us. I'd even prefer this one over the former.
Which one would you prefer?

> I don't see how it could be deleted by other code path.
That point still stands.

> 
> > Because with this change, deleting the property will leave us
> > with with a dangling cpu->apic_state pointer.  
> since there aren't other place that deletes lapic property we won't get it
> dangling pointer, see the next patch comment for call chain
> 
> >   
> > >  
> > >      qdev_prop_set_uint8(cpu->apic_state, "id", cpu->apic_id);
> > >      /* TODO: convert to link<> */
> > > -- 
> > > 2.7.0
> > >     
> >   
> 
> 

Re: [Qemu-devel] [PATCH v3 17/19] target-i386: fix apic object leak when CPU is deleted

[Eduardo Habkost]

On Wed, Jul 13, 2016 at 05:46:25PM +0200, Igor Mammedov wrote:
> On Wed, 13 Jul 2016 17:26:18 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Wed, 13 Jul 2016 12:04:44 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Wed, Jul 06, 2016 at 08:20:53AM +0200, Igor Mammedov wrote:  
> > > > 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 04c0b79..2fa445d 100644
> > > > --- a/target-i386/cpu.c
> > > > +++ b/target-i386/cpu.c
> > > > @@ -2765,6 +2765,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));    
> > > 
> > > What kind of event can trigger object_unparent() or
> > > object_del_property() on "lapic"? Can we guarantee that the child
> > > property will never be deleted by any other code, only by
> > > x86_cpu_unrealizefn() and object_finalize(cpu)?  
> > code path that triggers unparent of lapic implicitly is
> > cpu instance removal when it deletes all children.
> > 
> > So unless someone adds explicit lapic removal somewhere in target-i386/cpu.c
> Well, I've wrote nonsense here as I do remove child explicitly
> in x86_cpu_unrealizefn(), so it's fine to set cpu->apic_state to NULL
> as you suggest in 18/19.
> 
> The other way around might be call only apic_state->unrealize() explicitly
> from x86_cpu_unrealizefn() and let QOM do unparenting/finalizing
> automatically for us. I'd even prefer this one over the former.
> Which one would you prefer?

I'd prefer to delete the object on unrealize, because it is
created on realize.

> 
> > I don't see how it could be deleted by other code path.
> That point still stands.

I don't see it, either, but I was not 100% sure it was not
possible to trigger property removal from other code paths (e.g.
QMP commands). If you are sure it can't be triggered:

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

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 17/19] target-i386: fix apic object leak when CPU is deleted

[Bandan Das]

Igor Mammedov <imammedo@redhat.com> writes:

> 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 04c0b79..2fa445d 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2765,6 +2765,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));

Just about to write down my concern but I noticed you guys have already sorted this
out :)

>      qdev_prop_set_uint8(cpu->apic_state, "id", cpu->apic_id);
>      /* TODO: convert to link<> */

[Qemu-devel] [PATCH v3 18/19] 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 | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 2fa445d..f86dae0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2963,6 +2963,20 @@ 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));
+    }
+}
+
 typedef struct BitProperty {
     uint32_t *ptr;
     uint32_t mask;
@@ -3205,6 +3219,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.0

Re: [Qemu-devel] [PATCH v3 18/19] target-i386: add x86_cpu_unrealizefn()

[Eduardo Habkost]

On Wed, Jul 06, 2016 at 08:20:54AM +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>
> ---
>  target-i386/cpu.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 2fa445d..f86dae0 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2963,6 +2963,20 @@ 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));

As patch 17/19 drops the reference corresponding to
cpu->apic_state (leaving only the child property reference), this
will leave cpu->apic_state pointing to a dead object. Please set
it to NULL.

> +    }
> +}
> +
>  typedef struct BitProperty {
>      uint32_t *ptr;
>      uint32_t mask;
> @@ -3205,6 +3219,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.0
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 18/19] target-i386: add x86_cpu_unrealizefn()

[Igor Mammedov]

On Wed, 13 Jul 2016 11:59:29 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Jul 06, 2016 at 08:20:54AM +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>
> > ---
> >  target-i386/cpu.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 2fa445d..f86dae0 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2963,6 +2963,20 @@ 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));  
> 
> As patch 17/19 drops the reference corresponding to
> cpu->apic_state (leaving only the child property reference), this
> will leave cpu->apic_state pointing to a dead object. Please set
> it to NULL.
true, if we go with this explicit unparenting then
cpu->apic_state should be set to NULL.

If we only unrealize here then we do not need set cpu->apic_state to NULL
here as apic will die together with CPU instance when it starts to destroy
its children.

> 
> > +    }
> > +}
> > +
> >  typedef struct BitProperty {
> >      uint32_t *ptr;
> >      uint32_t mask;
> > @@ -3205,6 +3219,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.0
> >   
> 

[Qemu-devel] [PATCH v3 19/19] 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 0f85b56..ca1953c 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1736,6 +1736,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);
@@ -1747,13 +1748,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.0

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

[Eduardo Habkost]

On Wed, Jul 06, 2016 at 08:20:36AM +0200, Igor Mammedov wrote:
> Igor Mammedov (19):
>   target-i386: cpu: use uint32_t for X86CPU.apic_id
>   pc: add x86_topo_ids_from_apicid()
>   pc: extract CPU lookup into a separate function
>   pc: cpu: consolidate apic-id validity checks in pc_cpu_pre_plug()
>   target-i386: cpu: replace custom apic-id setter/getter with static
>     property
>   target-i386: add socket/core/thread properties to X86CPU

Patches 1-6 were applied to x86-next. Thanks.

-- 
Eduardo

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

[Igor Mammedov]

On Wed, 13 Jul 2016 11:27:23 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Jul 06, 2016 at 08:20:36AM +0200, Igor Mammedov wrote:
> > Igor Mammedov (19):
> >   target-i386: cpu: use uint32_t for X86CPU.apic_id
> >   pc: add x86_topo_ids_from_apicid()
> >   pc: extract CPU lookup into a separate function
> >   pc: cpu: consolidate apic-id validity checks in pc_cpu_pre_plug()
> >   target-i386: cpu: replace custom apic-id setter/getter with static
> >     property
> >   target-i386: add socket/core/thread properties to X86CPU  
> 
> Patches 1-6 were applied to x86-next. Thanks.
> 
Thanks Eduardo,
I'll rebase and post v4 on top of your tree.

Could you also take 16-17/19 as it's fixes to x86 CPUs,
17th isn't actually triggered without hot-remove but its still a leak