[Qemu-devel] [PULL v2 00/28] x86 queue, 2016-07-20

[Qemu-devel] [PULL v2 00/28] x86 queue, 2016-07-20

[Eduardo Habkost]

Changes v2:
* Fixed 32-bit build error by using %d and idx at
  pc_cpu_pre_plug()

The following changes since commit 338404d061144956b76f9893ca3434d057dff2d4:

  Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20160719' into staging (2016-07-20 12:48:18 +0100)

are available in the git repository at:

  git://github.com/ehabkost/qemu.git tags/x86-pull-request

for you to fetch changes up to 8fe6374e8e0c8dacb85e9e97897291541dd61be6:

  pc: Make device_del CPU work for x86 CPUs (2016-07-20 12:02:20 -0300)

----------------------------------------------------------------
x86 queue, 2016-07-20

----------------------------------------------------------------

Dr. David Alan Gilbert (5):
  target-i386: Provide TCG_PHYS_ADDR_BITS
  target-i386: Allow physical address bits to be set
  target-i386: Mask mtrr mask based on CPU physical address limits
  target-i386: Fill high bits of mtrr mask
  target-i386: Set physical address bits based on host

Igor Mammedov (22):
  target-i386: 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: Replace custom apic-id setter/getter with static property
  target-i386: Add socket/core/thread properties to X86CPU
  target-i386: cpu: Do not ignore error and fix apic parent
  target-i386: Fix apic object leak when CPU is deleted
  pc: Set APIC ID based on socket/core/thread ids if it's not been set
    yet
  pc: Delay setting number of boot CPUs to machine_done time
  pc: Register created initial and hotpluged CPUs in one place
    pc_cpu_plug()
  pc: Forbid BSP removal
  pc: Enforce adding CPUs contiguously and removing them in opposite
    order
  pc: cpu: Allow device_add to be used with x86 cpu
  pc: Implement query-hotpluggable-cpus callback
  apic: move MAX_APICS check to 'apic' class
  apic: Drop APICCommonState.idx and use APIC ID as index in
    local_apics[]
  apic: kvm-apic: Fix crash due to access to freed memory region
  (kvm)apic: Add unrealize callbacks
  apic: Use apic_id as apic's migration instance_id
  target-i386: Add x86_cpu_unrealizefn()
  pc: Make device_del CPU work for x86 CPUs

Paolo Bonzini (1):
  target-i386: Add support for UMIP and RDPID CPUID bits

 hw/i386/kvm/apic.c              |   9 +-
 hw/i386/pc.c                    | 279 +++++++++++++++++++++++++++++++++-------
 hw/intc/apic.c                  |  26 +++-
 hw/intc/apic_common.c           |  33 +++--
 include/hw/i386/apic_internal.h |   5 +-
 include/hw/i386/pc.h            |  10 ++
 include/hw/i386/topology.h      |  15 +++
 qmp-commands.hx                 |  15 +++
 target-i386/cpu.c               | 198 ++++++++++++++++++----------
 target-i386/cpu.h               |  28 +++-
 target-i386/kvm.c               |  39 +++++-
 11 files changed, 523 insertions(+), 134 deletions(-)

-- 
2.5.5

[Qemu-devel] [PULL 01/28] target-i386: Provide TCG_PHYS_ADDR_BITS

[Eduardo Habkost]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Provide a constant for the number of address bits supported under TCG.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 5b14a72..8b6bd0b 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1419,11 +1419,13 @@ uint64_t cpu_get_tsc(CPUX86State *env);
 /* XXX: This value should match the one returned by CPUID
  * and in exec.c */
 # if defined(TARGET_X86_64)
-# define PHYS_ADDR_MASK 0xffffffffffLL
+# define TCG_PHYS_ADDR_BITS 40
 # else
-# define PHYS_ADDR_MASK 0xfffffffffLL
+# define TCG_PHYS_ADDR_BITS 36
 # endif
 
+#define PHYS_ADDR_MASK MAKE_64BIT_MASK(0, TCG_PHYS_ADDR_BITS)
+
 #define cpu_init(cpu_model) CPU(cpu_x86_init(cpu_model))
 
 #define cpu_signal_handler cpu_x86_signal_handler
-- 
2.5.5

[Qemu-devel] [PULL 02/28] target-i386: Allow physical address bits to be set

[Eduardo Habkost]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Currently QEMU sets the x86 number of physical address bits to the
magic number 40.  This is only correct on some small AMD systems;
Intel systems tend to have 36, 39, 46 bits, and large AMD systems
tend to have 48.

Having the value different from your actual hardware is detectable
by the guest and in principal can cause problems;
The current limit of 40 stops TB VMs being created by those lucky
enough to have that much.

This patch lets you set the physical bits by a cpu property but
defaults to the same 40bits which matches TCGs setup.

I've removed the ancient warning about the 42 bit limit in exec.c;
I can't find that limit in there and no one else seems to know where
it is.

We use a magic value of 0 as the property default so that we can
later distinguish between the default and a user set value.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 52 +++++++++++++++++++++++++++++++++++++++++++---------
 target-i386/cpu.h |  3 +++
 2 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 6e49e4c..669180e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2641,17 +2641,13 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         break;
     case 0x80000008:
         /* virtual & phys address size in low 2 bytes. */
-/* XXX: This value must match the one used in the MMU code. */
         if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) {
-            /* 64 bit processor */
-/* XXX: The physical address space is limited to 42 bits in exec.c. */
-            *eax = 0x00003028; /* 48 bits virtual, 40 bits physical */
+            /* 64 bit processor, 48 bits virtual, configurable
+             * physical bits.
+             */
+            *eax = 0x00003000 + cpu->phys_bits;
         } else {
-            if (env->features[FEAT_1_EDX] & CPUID_PSE36) {
-                *eax = 0x00000024; /* 36 bits physical */
-            } else {
-                *eax = 0x00000020; /* 32 bits physical */
-            }
+            *eax = cpu->phys_bits;
         }
         *ebx = 0;
         *ecx = 0;
@@ -2993,7 +2989,44 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
            & CPUID_EXT2_AMD_ALIASES);
     }
 
+    if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) {
+        /* 0 means it was not explicitly set by the user (or by machine
+         * compat_props). In this case, the default is the value used by
+         * TCG (40).
+         */
+        if (cpu->phys_bits == 0) {
+            cpu->phys_bits = TCG_PHYS_ADDR_BITS;
+        }
+        if (kvm_enabled()) {
+            if (cpu->phys_bits > TARGET_PHYS_ADDR_SPACE_BITS ||
+                cpu->phys_bits < 32) {
+                error_setg(errp, "phys-bits should be between 32 and %u "
+                                 " (but is %u)",
+                                 TARGET_PHYS_ADDR_SPACE_BITS, cpu->phys_bits);
+                return;
+            }
+        } else {
+            if (cpu->phys_bits != TCG_PHYS_ADDR_BITS) {
+                error_setg(errp, "TCG only supports phys-bits=%u",
+                                  TCG_PHYS_ADDR_BITS);
+                return;
+            }
+        }
+    } else {
+        /* For 32 bit systems don't use the user set value, but keep
+         * phys_bits consistent with what we tell the guest.
+         */
+        if (cpu->phys_bits != 0) {
+            error_setg(errp, "phys-bits is not user-configurable in 32 bit");
+            return;
+        }
 
+        if (env->features[FEAT_1_EDX] & CPUID_PSE36) {
+            cpu->phys_bits = 36;
+        } else {
+            cpu->phys_bits = 32;
+        }
+    }
     cpu_exec_init(cs, &error_abort);
 
     if (tcg_enabled()) {
@@ -3294,6 +3327,7 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, true),
     DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
     DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
+    DEFINE_PROP_UINT32("phys-bits", X86CPU, phys_bits, 0),
     DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
     DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
     DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 8b6bd0b..b2ddcb8 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1198,6 +1198,9 @@ struct X86CPU {
     /* Compatibility bits for old machine types: */
     bool enable_cpuid_0xb;
 
+    /* Number of physical address bits supported */
+    uint32_t phys_bits;
+
     /* in order to simplify APIC support, we leave this pointer to the
        user */
     struct DeviceState *apic_state;
-- 
2.5.5

[Qemu-devel] [PULL 03/28] target-i386: Mask mtrr mask based on CPU physical address limits

[Eduardo Habkost]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

The CPU GPs if we try and set a bit in a variable MTRR mask above
the limit of physical address bits on the host.  We hit this
when loading a migration from a host with a larger physical
address limit than our destination (e.g. a Xeon->i7 of same
generation) but previously used to get away with it
until 48e1a45 started checking that msr writes actually worked.

It seems in our case the GP probably comes from KVM emulating
that GP.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/kvm.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 9327523..2f1cc62 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1716,6 +1716,8 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
             }
         }
         if (has_msr_mtrr) {
+            uint64_t phys_mask = MAKE_64BIT_MASK(0, cpu->phys_bits);
+
             kvm_msr_entry_add(cpu, MSR_MTRRdefType, env->mtrr_deftype);
             kvm_msr_entry_add(cpu, MSR_MTRRfix64K_00000, env->mtrr_fixed[0]);
             kvm_msr_entry_add(cpu, MSR_MTRRfix16K_80000, env->mtrr_fixed[1]);
@@ -1729,10 +1731,15 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
             kvm_msr_entry_add(cpu, MSR_MTRRfix4K_F0000, env->mtrr_fixed[9]);
             kvm_msr_entry_add(cpu, MSR_MTRRfix4K_F8000, env->mtrr_fixed[10]);
             for (i = 0; i < MSR_MTRRcap_VCNT; i++) {
+                /* The CPU GPs if we write to a bit above the physical limit of
+                 * the host CPU (and KVM emulates that)
+                 */
+                uint64_t mask = env->mtrr_var[i].mask;
+                mask &= phys_mask;
+
                 kvm_msr_entry_add(cpu, MSR_MTRRphysBase(i),
                                   env->mtrr_var[i].base);
-                kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i),
-                                  env->mtrr_var[i].mask);
+                kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
             }
         }
 
-- 
2.5.5

[Qemu-devel] [PULL 04/28] target-i386: Fill high bits of mtrr mask

[Eduardo Habkost]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Fill the bits between 51..number-of-physical-address-bits in the
MTRR_PHYSMASKn variable range mtrr masks so that they're consistent
in the migration stream irrespective of the physical address space
of the source VM in a migration.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 include/hw/i386/pc.h |  5 +++++
 target-i386/cpu.c    |  1 +
 target-i386/cpu.h    |  3 +++
 target-i386/kvm.c    | 28 +++++++++++++++++++++++++++-
 4 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index e38c95a..47411cb 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -377,6 +377,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
         .driver   = "vmxnet3",\
         .property = "romfile",\
         .value    = "",\
+    },\
+    {\
+        .driver = TYPE_X86_CPU,\
+        .property = "fill-mtrr-mask",\
+        .value = "off",\
     },
 
 #define PC_COMPAT_2_5 \
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 669180e..5ac7e97 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3328,6 +3328,7 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
     DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
     DEFINE_PROP_UINT32("phys-bits", X86CPU, phys_bits, 0),
+    DEFINE_PROP_BOOL("fill-mtrr-mask", X86CPU, fill_mtrr_mask, true),
     DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
     DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
     DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index b2ddcb8..b2ab8bf 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1198,6 +1198,9 @@ struct X86CPU {
     /* Compatibility bits for old machine types: */
     bool enable_cpuid_0xb;
 
+    /* if true fill the top bits of the MTRR_PHYSMASKn variable range */
+    bool fill_mtrr_mask;
+
     /* Number of physical address bits supported */
     uint32_t phys_bits;
 
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 2f1cc62..df28dd2 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1977,6 +1977,7 @@ static int kvm_get_msrs(X86CPU *cpu)
     CPUX86State *env = &cpu->env;
     struct kvm_msr_entry *msrs = cpu->kvm_msr_buf->entries;
     int ret, i;
+    uint64_t mtrr_top_bits;
 
     kvm_msr_buf_reset(cpu);
 
@@ -2129,6 +2130,30 @@ static int kvm_get_msrs(X86CPU *cpu)
     }
 
     assert(ret == cpu->kvm_msr_buf->nmsrs);
+    /*
+     * MTRR masks: Each mask consists of 5 parts
+     * a  10..0: must be zero
+     * b  11   : valid bit
+     * c n-1.12: actual mask bits
+     * d  51..n: reserved must be zero
+     * e  63.52: reserved must be zero
+     *
+     * 'n' is the number of physical bits supported by the CPU and is
+     * apparently always <= 52.   We know our 'n' but don't know what
+     * the destinations 'n' is; it might be smaller, in which case
+     * it masks (c) on loading. It might be larger, in which case
+     * we fill 'd' so that d..c is consistent irrespetive of the 'n'
+     * we're migrating to.
+     */
+
+    if (cpu->fill_mtrr_mask) {
+        QEMU_BUILD_BUG_ON(TARGET_PHYS_ADDR_SPACE_BITS > 52);
+        assert(cpu->phys_bits <= TARGET_PHYS_ADDR_SPACE_BITS);
+        mtrr_top_bits = MAKE_64BIT_MASK(cpu->phys_bits, 52 - cpu->phys_bits);
+    } else {
+        mtrr_top_bits = 0;
+    }
+
     for (i = 0; i < ret; i++) {
         uint32_t index = msrs[i].index;
         switch (index) {
@@ -2327,7 +2352,8 @@ static int kvm_get_msrs(X86CPU *cpu)
             break;
         case MSR_MTRRphysBase(0) ... MSR_MTRRphysMask(MSR_MTRRcap_VCNT - 1):
             if (index & 1) {
-                env->mtrr_var[MSR_MTRRphysIndex(index)].mask = msrs[i].data;
+                env->mtrr_var[MSR_MTRRphysIndex(index)].mask = msrs[i].data |
+                                                               mtrr_top_bits;
             } else {
                 env->mtrr_var[MSR_MTRRphysIndex(index)].base = msrs[i].data;
             }
-- 
2.5.5

[Qemu-devel] [PULL 05/28] target-i386: Use uint32_t for X86CPU.apic_id

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

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>
Signed-off-by: Eduardo Habkost <ehabkost@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 5ac7e97..5fc01c6 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2946,7 +2946,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;
     }
@@ -3254,7 +3254,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 b2ab8bf..10d562d 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -845,6 +845,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;
@@ -1174,7 +1179,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.5.5

[Qemu-devel] [PULL 06/28] pc: Add x86_topo_ids_from_apicid()

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

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>
Signed-off-by: Eduardo Habkost <ehabkost@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.5.5

[Qemu-devel] [PULL 07/28] target-i386: Set physical address bits based on host

[Eduardo Habkost]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Add the host-phys-bits boolean property, if true, take phys-bits
from the hosts physical bits value, overriding either the default
or the user specified value.

We can also use the value we read from the host to check the users
explicitly set value and warn them if it doesn't match.

Note:
   a) We only read the hosts value in KVM mode (because on non-x86
      we get an abort if we try)
   b) We don't warn about trying to use host-phys-bits in TCG mode,
      we just fall back to the TCG default.  This allows the machine
      type to set the host-phys-bits flag if it wants and then to
      work in both TCG and KVM.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++--------
 target-i386/cpu.h |  3 +++
 2 files changed, 65 insertions(+), 10 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 5fc01c6..89ca326 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2922,6 +2922,31 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
 }
 #endif
 
+/* Note: Only safe for use on x86(-64) hosts */
+static uint32_t x86_host_phys_bits(void)
+{
+    uint32_t eax;
+    uint32_t host_phys_bits;
+
+    host_cpuid(0x80000000, 0, &eax, NULL, NULL, NULL);
+    if (eax >= 0x80000008) {
+        host_cpuid(0x80000008, 0, &eax, NULL, NULL, NULL);
+        /* Note: According to AMD doc 25481 rev 2.34 they have a field
+         * at 23:16 that can specify a maximum physical address bits for
+         * the guest that can override this value; but I've not seen
+         * anything with that set.
+         */
+        host_phys_bits = eax & 0xff;
+    } else {
+        /* It's an odd 64 bit machine that doesn't have the leaf for
+         * physical address bits; fall back to 36 that's most older
+         * Intel.
+         */
+        host_phys_bits = 36;
+    }
+
+    return host_phys_bits;
+}
 
 #define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && \
                            (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 && \
@@ -2989,29 +3014,55 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
            & CPUID_EXT2_AMD_ALIASES);
     }
 
+    /* For 64bit systems think about the number of physical bits to present.
+     * ideally this should be the same as the host; anything other than matching
+     * the host can cause incorrect guest behaviour.
+     * QEMU used to pick the magic value of 40 bits that corresponds to
+     * consumer AMD devices but nothing else.
+     */
     if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) {
-        /* 0 means it was not explicitly set by the user (or by machine
-         * compat_props). In this case, the default is the value used by
-         * TCG (40).
-         */
-        if (cpu->phys_bits == 0) {
-            cpu->phys_bits = TCG_PHYS_ADDR_BITS;
-        }
         if (kvm_enabled()) {
-            if (cpu->phys_bits > TARGET_PHYS_ADDR_SPACE_BITS ||
-                cpu->phys_bits < 32) {
+            uint32_t host_phys_bits = x86_host_phys_bits();
+            static bool warned;
+
+            if (cpu->host_phys_bits) {
+                /* The user asked for us to use the host physical bits */
+                cpu->phys_bits = host_phys_bits;
+            }
+
+            /* Print a warning if the user set it to a value that's not the
+             * host value.
+             */
+            if (cpu->phys_bits != host_phys_bits && cpu->phys_bits != 0 &&
+                !warned) {
+                error_report("Warning: Host physical bits (%u)"
+                                 " does not match phys-bits property (%u)",
+                                 host_phys_bits, cpu->phys_bits);
+                warned = true;
+            }
+
+            if (cpu->phys_bits &&
+                (cpu->phys_bits > TARGET_PHYS_ADDR_SPACE_BITS ||
+                cpu->phys_bits < 32)) {
                 error_setg(errp, "phys-bits should be between 32 and %u "
                                  " (but is %u)",
                                  TARGET_PHYS_ADDR_SPACE_BITS, cpu->phys_bits);
                 return;
             }
         } else {
-            if (cpu->phys_bits != TCG_PHYS_ADDR_BITS) {
+            if (cpu->phys_bits && cpu->phys_bits != TCG_PHYS_ADDR_BITS) {
                 error_setg(errp, "TCG only supports phys-bits=%u",
                                   TCG_PHYS_ADDR_BITS);
                 return;
             }
         }
+        /* 0 means it was not explicitly set by the user (or by machine
+         * compat_props or by the host code above). In this case, the default
+         * is the value used by TCG (40).
+         */
+        if (cpu->phys_bits == 0) {
+            cpu->phys_bits = TCG_PHYS_ADDR_BITS;
+        }
     } else {
         /* For 32 bit systems don't use the user set value, but keep
          * phys_bits consistent with what we tell the guest.
@@ -3328,6 +3379,7 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
     DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
     DEFINE_PROP_UINT32("phys-bits", X86CPU, phys_bits, 0),
+    DEFINE_PROP_BOOL("host-phys-bits", X86CPU, host_phys_bits, false),
     DEFINE_PROP_BOOL("fill-mtrr-mask", X86CPU, fill_mtrr_mask, true),
     DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
     DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 10d562d..0ff88e1 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1206,6 +1206,9 @@ struct X86CPU {
     /* if true fill the top bits of the MTRR_PHYSMASKn variable range */
     bool fill_mtrr_mask;
 
+    /* if true override the phys_bits value with a value read from the host */
+    bool host_phys_bits;
+
     /* Number of physical address bits supported */
     uint32_t phys_bits;
 
-- 
2.5.5

[Qemu-devel] [PULL 08/28] pc: Extract CPU lookup into a separate function

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

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>
Signed-off-by: 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 719884f..c9999a8 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1756,11 +1756,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);
@@ -1784,11 +1803,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.5.5

[Qemu-devel] [PULL 09/28] pc: cpu: Consolidate apic-id validity checks in pc_cpu_pre_plug()

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

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>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc.c      | 43 +++++++++++++++++++++++++++++++------------
 target-i386/cpu.c | 13 -------------
 2 files changed, 31 insertions(+), 25 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c9999a8..81206f4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1122,18 +1122,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",
@@ -1852,6 +1840,36 @@ 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[%d] with APIC ID %" PRIu32 " exists",
+                   idx, 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)
 {
@@ -2149,6 +2167,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 89ca326..5d77c09 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1907,8 +1907,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;
 
@@ -1923,17 +1921,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.5.5

[Qemu-devel] [PULL 10/28] target-i386: Replace custom apic-id setter/getter with static property

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

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>
Signed-off-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 5d77c09..af2d694 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1893,37 +1893,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,
@@ -3278,9 +3247,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);
@@ -3290,11 +3256,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;
 
@@ -3351,6 +3312,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.5.5

[Qemu-devel] [PULL 11/28] target-i386: Add socket/core/thread properties to X86CPU

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

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>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 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 81206f4..394abfe 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1844,6 +1844,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);
@@ -1860,6 +1861,34 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
                    idx, 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 af2d694..de0295a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3315,8 +3315,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 0ff88e1..c883cd5 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1219,6 +1219,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.5.5

[Qemu-devel] [PULL 12/28] target-i386: Add support for UMIP and RDPID CPUID bits

[Eduardo Habkost]

From: Paolo Bonzini <pbonzini@redhat.com>

These are both stored in CPUID[EAX=7,EBX=0].ECX.  KVM is going to
be able to emulate both (albeit with a performance loss in the case
of RDPID, which therefore will be in KVM_GET_EMULATED_CPUID rather
than KVM_GET_SUPPORTED_CPUID).

It's also possible to implement both in TCG, but this is for 2.8.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 4 ++--
 target-i386/cpu.h | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index de0295a..c36441d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -305,12 +305,12 @@ static const char *cpuid_7_0_ebx_feature_name[] = {
 };
 
 static const char *cpuid_7_0_ecx_feature_name[] = {
-    NULL, NULL, NULL, "pku",
+    NULL, NULL, "umip", "pku",
     "ospke", NULL, NULL, NULL,
     NULL, NULL, NULL, NULL,
     NULL, NULL, NULL, NULL,
     NULL, NULL, NULL, NULL,
-    NULL, NULL, NULL, NULL,
+    NULL, NULL, "rdpid", NULL,
     NULL, NULL, NULL, NULL,
     NULL, NULL, NULL, NULL,
 };
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index c883cd5..65615c0 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -616,8 +616,10 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
 #define CPUID_7_0_EBX_AVX512ER (1U << 27) /* AVX-512 Exponential and Reciprocal */
 #define CPUID_7_0_EBX_AVX512CD (1U << 28) /* AVX-512 Conflict Detection */
 
+#define CPUID_7_0_ECX_UMIP     (1U << 2)
 #define CPUID_7_0_ECX_PKU      (1U << 3)
 #define CPUID_7_0_ECX_OSPKE    (1U << 4)
+#define CPUID_7_0_ECX_RDPID    (1U << 22)
 
 #define CPUID_XSAVE_XSAVEOPT   (1U << 0)
 #define CPUID_XSAVE_XSAVEC     (1U << 1)
-- 
2.5.5

[Qemu-devel] [PULL 13/28] target-i386: cpu: Do not ignore error and fix apic parent

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c36441d..6c36b13 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2826,8 +2826,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.5.5

[Qemu-devel] [PULL 14/28] target-i386: Fix apic object leak when CPU is deleted

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 6c36b13..5d0e085 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2828,6 +2828,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.5.5

[Qemu-devel] [PULL 15/28] pc: Set APIC ID based on socket/core/thread ids if it's not been set yet

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

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

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 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 394abfe..72454fb 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1844,14 +1844,52 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
                             DeviceState *dev, Error **errp)
 {
     int idx;
+    CPUArchId *cpu_slot;
     X86CPUTopoInfo topo;
     X86CPU *cpu = X86_CPU(dev);
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
-    CPUArchId *cpu_slot = pc_find_cpu_slot(pcms, CPU(dev), &idx);
 
+    /* if APIC ID is not set, set it based on socket/core/thread properties */
+    if (cpu->apic_id == UNASSIGNED_APIC_ID) {
+        int max_socket = (max_cpus - 1) / smp_threads / smp_cores;
+
+        if (cpu->socket_id < 0) {
+            error_setg(errp, "CPU socket-id is not set");
+            return;
+        } else if (cpu->socket_id > max_socket) {
+            error_setg(errp, "Invalid CPU socket-id: %u must be in range 0:%u",
+                       cpu->socket_id, max_socket);
+            return;
+        }
+        if (cpu->core_id < 0) {
+            error_setg(errp, "CPU core-id is not set");
+            return;
+        } else if (cpu->core_id > (smp_cores - 1)) {
+            error_setg(errp, "Invalid CPU core-id: %u must be in range 0:%u",
+                       cpu->core_id, smp_cores - 1);
+            return;
+        }
+        if (cpu->thread_id < 0) {
+            error_setg(errp, "CPU thread-id is not set");
+            return;
+        } else if (cpu->thread_id > (smp_threads - 1)) {
+            error_setg(errp, "Invalid CPU thread-id: %u must be in range 0:%u",
+                       cpu->thread_id, smp_threads - 1);
+            return;
+        }
+
+        topo.pkg_id = cpu->socket_id;
+        topo.core_id = cpu->core_id;
+        topo.smt_id = cpu->thread_id;
+        cpu->apic_id = apicid_from_topo_ids(smp_cores, smp_threads, &topo);
+    }
+
+    cpu_slot = pc_find_cpu_slot(pcms, CPU(dev), &idx);
     if (!cpu_slot) {
-        error_setg(errp, "Invalid CPU index with APIC ID (%" PRIu32
-                   "), valid range 0:%d", cpu->apic_id,
+        x86_topo_ids_from_apicid(cpu->apic_id, smp_cores, smp_threads, &topo);
+        error_setg(errp, "Invalid CPU [socket: %u, core: %u, thread: %u] with"
+                  " APIC ID %" PRIu32 ", valid index range 0:%d",
+                   topo.pkg_id, topo.core_id, topo.smt_id, cpu->apic_id,
                    pcms->possible_cpus->len - 1);
         return;
     }
-- 
2.5.5

[Qemu-devel] [PULL 16/28] pc: Delay setting number of boot CPUs to machine_done time

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

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 | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 72454fb..669382f 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,
@@ -1090,6 +1087,17 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
+static int pc_present_cpus_count(PCMachineState *pcms)
+{
+    int i, boot_cpus = 0;
+    for (i = 0; i < pcms->possible_cpus->len; i++) {
+        if (pcms->possible_cpus->cpus[i].cpu) {
+            boot_cpus++;
+        }
+    }
+    return boot_cpus;
+}
+
 static X86CPU *pc_new_cpu(const char *typename, int64_t apic_id,
                           Error **errp)
 {
@@ -1240,6 +1248,9 @@ void pc_machine_done(Notifier *notifier, void *data)
                                         PCMachineState, machine_done);
     PCIBus *bus = pcms->bus;
 
+    /* set the number of CPUs */
+    rtc_set_memory(pcms->rtc, 0x5f, pc_present_cpus_count(pcms) - 1);
+
     if (bus) {
         int extra_hosts = 0;
 
-- 
2.5.5

[Qemu-devel] [PULL 17/28] pc: Register created initial and hotpluged CPUs in one place pc_cpu_plug()

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

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

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 669382f..f4c6a27 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1204,7 +1204,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));
         }
     }
@@ -1783,25 +1782,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.5.5

[Qemu-devel] [PULL 18/28] pc: Forbid BSP removal

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

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

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

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

[Qemu-devel] [PULL 19/28] pc: Enforce adding CPUs contiguously and removing them in opposite order

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

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

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

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

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

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

[Qemu-devel] [PULL 20/28] pc: cpu: Allow device_add to be used with x86 cpu

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 5d0e085..3df5f5f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3396,6 +3396,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.5.5

[Qemu-devel] [PULL 21/28] pc: Implement query-hotpluggable-cpus callback

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

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>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 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 68d8cad..13843da 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2236,6 +2236,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 */
@@ -2276,6 +2320,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 496d73c..c8d360a 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -5026,3 +5026,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.5.5

[Qemu-devel] [PULL 22/28] apic: move MAX_APICS check to 'apic' class

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

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>
Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@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 73ce716..67348e9 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -121,8 +121,6 @@
 #define VAPIC_ENABLE_BIT                0
 #define VAPIC_ENABLE_MASK               (1 << VAPIC_ENABLE_BIT)
 
-#define MAX_APICS 255
-
 typedef struct APICCommonState APICCommonState;
 
 #define TYPE_APIC_COMMON "apic-common"
@@ -176,7 +174,7 @@ struct APICCommonState {
     uint32_t initial_count;
     int64_t initial_count_load_time;
     int64_t next_time;
-    int idx;
+    int idx; /* not actually common, used only by 'apic' derived class */
     QEMUTimer *timer;
     int64_t timer_expiry;
     int sipi_vector;
-- 
2.5.5

[Qemu-devel] [PULL 23/28] apic: Drop APICCommonState.idx and use APIC ID as index in local_apics[]

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

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>
Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@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 67348e9..8330592 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -174,7 +174,6 @@ struct APICCommonState {
     uint32_t initial_count;
     int64_t initial_count_load_time;
     int64_t next_time;
-    int idx; /* not actually common, used only by 'apic' derived class */
     QEMUTimer *timer;
     int64_t timer_expiry;
     int sipi_vector;
-- 
2.5.5

[Qemu-devel] [PULL 24/28] apic: kvm-apic: Fix crash due to access to freed memory region

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

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

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

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/kvm/apic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

[Qemu-devel] [PULL 25/28] (kvm)apic: Add unrealize callbacks

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

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

[Qemu-devel] [PULL 26/28] apic: Use apic_id as apic's migration instance_id

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

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

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

Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@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 e549a62..06c4e9f 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -183,6 +183,7 @@ struct APICCommonState {
     uint32_t vapic_control;
     DeviceState *vapic;
     hwaddr vapic_paddr; /* note: persistence via kvmvapic */
+    bool legacy_instance_id;
 };
 
 typedef struct VAPICState {
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 47411cb..bc937b9 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -382,6 +382,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
         .driver = TYPE_X86_CPU,\
         .property = "fill-mtrr-mask",\
         .value = "off",\
+    },\
+    {\
+        .driver   = "apic",\
+        .property = "legacy-instance-id",\
+        .value    = "on",\
     },
 
 #define PC_COMPAT_2_5 \
-- 
2.5.5

[Qemu-devel] [PULL 27/28] target-i386: Add x86_cpu_unrealizefn()

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

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

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3df5f5f..6a1afab 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3114,6 +3114,21 @@ out:
     }
 }
 
+static void x86_cpu_unrealizefn(DeviceState *dev, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(dev);
+
+#ifndef CONFIG_USER_ONLY
+    cpu_remove_sync(CPU(dev));
+    qemu_unregister_reset(x86_cpu_machine_reset_cb, dev);
+#endif
+
+    if (cpu->apic_state) {
+        object_unparent(OBJECT(cpu->apic_state));
+        cpu->apic_state = NULL;
+    }
+}
+
 typedef struct BitProperty {
     uint32_t *ptr;
     uint32_t mask;
@@ -3360,6 +3375,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.5.5

[Qemu-devel] [PULL 28/28] pc: Make device_del CPU work for x86 CPUs

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

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>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@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 13843da..ac7a4d5 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1847,6 +1847,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);
@@ -1858,13 +1859,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.5.5

Re: [Qemu-devel] [PULL v2 00/28] x86 queue, 2016-07-20

[Peter Maydell]

On 20 July 2016 at 16:08, Eduardo Habkost <ehabkost@redhat.com> wrote:
> Changes v2:
> * Fixed 32-bit build error by using %d and idx at
>   pc_cpu_pre_plug()
>
> The following changes since commit 338404d061144956b76f9893ca3434d057dff2d4:
>
>   Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20160719' into staging (2016-07-20 12:48:18 +0100)
>
> are available in the git repository at:
>
>   git://github.com/ehabkost/qemu.git tags/x86-pull-request
>
> for you to fetch changes up to 8fe6374e8e0c8dacb85e9e97897291541dd61be6:
>
>   pc: Make device_del CPU work for x86 CPUs (2016-07-20 12:02:20 -0300)
>
> ----------------------------------------------------------------
> x86 queue, 2016-07-20
>
> ----------------------------------------------------------------

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PULL 26/28] apic: Use apic_id as apic's migration instance_id

[Amit Shah]

On (Wed) 20 Jul 2016 [12:08:32], Eduardo Habkost wrote:
> From: Igor Mammedov <imammedo@redhat.com>
> 
> instance_id is generated by last_used_id + 1 for a given device type
> so for QEMU with 3 CPUs instance_id for APICs is a seti of [0, 1, 2]
> When CPU in the middle is hot-removed and migration started
> APICs with instance_ids 0 and 2 are transferred in migration stream.
> However target starts with 2 CPUs and APICs' instance_ids are
> generated from scratch [0, 1] hence migration fails with error
>   Unknown savevm section or instance 'apic' 2
> 
> Fix issue by manually registering APIC's vmsd with apic_id as
> instance_id, in this case instance_id on target will always
> match instance_id on source as apic_id is the same for a given
> cpu instance.
> 
> Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

After these patches, the static checker complains about missing
sections:

Section "apic-common" does not exist in dest
Section "apic" does not exist in dest
Section "kvm-apic" does not exist in dest

This will break migration from older versions.

		Amit

Re: [Qemu-devel] [PULL 26/28] apic: Use apic_id as apic's migration instance_id

[Igor Mammedov]

On Tue, 26 Jul 2016 10:41:38 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> On (Wed) 20 Jul 2016 [12:08:32], Eduardo Habkost wrote:
> > From: Igor Mammedov <imammedo@redhat.com>
> > 
> > instance_id is generated by last_used_id + 1 for a given device type
> > so for QEMU with 3 CPUs instance_id for APICs is a seti of [0, 1, 2]
> > When CPU in the middle is hot-removed and migration started
> > APICs with instance_ids 0 and 2 are transferred in migration stream.
> > However target starts with 2 CPUs and APICs' instance_ids are
> > generated from scratch [0, 1] hence migration fails with error
> >   Unknown savevm section or instance 'apic' 2
> > 
> > Fix issue by manually registering APIC's vmsd with apic_id as
> > instance_id, in this case instance_id on target will always
> > match instance_id on source as apic_id is the same for a given
> > cpu instance.
> > 
> > Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>  
> 
> After these patches, the static checker complains about missing
> sections:
> 
> Section "apic-common" does not exist in dest
> Section "apic" does not exist in dest
> Section "kvm-apic" does not exist in dest
It works for me, could you post reproducing commands?

> 
> This will break migration from older versions.
> 
> 		Amit

Re: [Qemu-devel] [PULL 26/28] apic: Use apic_id as apic's migration instance_id

[Igor Mammedov]

On Tue, 26 Jul 2016 10:41:38 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> On (Wed) 20 Jul 2016 [12:08:32], Eduardo Habkost wrote:
> > From: Igor Mammedov <imammedo@redhat.com>
> > 
> > instance_id is generated by last_used_id + 1 for a given device type
> > so for QEMU with 3 CPUs instance_id for APICs is a seti of [0, 1, 2]
> > When CPU in the middle is hot-removed and migration started
> > APICs with instance_ids 0 and 2 are transferred in migration stream.
> > However target starts with 2 CPUs and APICs' instance_ids are
> > generated from scratch [0, 1] hence migration fails with error
> >   Unknown savevm section or instance 'apic' 2
> > 
> > Fix issue by manually registering APIC's vmsd with apic_id as
> > instance_id, in this case instance_id on target will always
> > match instance_id on source as apic_id is the same for a given
> > cpu instance.
> > 
> > Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>  
> 
> After these patches, the static checker complains about missing
> sections:
> 
> Section "apic-common" does not exist in dest
> Section "apic" does not exist in dest
> Section "kvm-apic" does not exist in dest
> 
> This will break migration from older versions.
Still can't reproduce:
here is my CLI on SRC:
  qemu-system-x86_64-v2.6.0  \
   -snapshot -enable-kvm -smp 6,maxcpus=6 -m 256M rhel72.img -monitor stdio -M pc-i440fx-2.6 -nodefaults

monitor# stop
monitor# migrate "exec:gzip -c > STATEFILE.gz"
^C

CLI on DST:
  qemu-system-x86_64-v2.7.0-rc0 \
   -snapshot -enable-kvm -smp 6,maxcpus=6 -m 256M rhel72.img -monitor stdio -M pc-i440fx-2.6 -nodefaults -incoming "exec: gzip -c -d STATEFILE.gz"

But I've found issue with I2C, which breaks migration for me with:

(qemu) qemu-system-x86_64: Missing section footer for i2c_bus
qemu-system-x86_64: load of migration failed: Invalid argument

Which is bisects to:

commit 2293c27faddf9547dd8b52423caa6e85844eec3a
Author: KONRAD Frederic <fred.konrad@greensocs.com>
Date:   Tue Jun 14 15:59:14 2016 +0100

    i2c: implement broadcast write

hacking migration hunks of it to old VMState fixes I2C issue,
and no apic related issues are noticed.

> 
> 		Amit
> 

Re: [Qemu-devel] [PULL 26/28] apic: Use apic_id as apic's migration instance_id

[Amit Shah]

On (Tue) 26 Jul 2016 [10:00:49], Igor Mammedov wrote:
> On Tue, 26 Jul 2016 10:41:38 +0530
> Amit Shah <amit.shah@redhat.com> wrote:
> 
> > On (Wed) 20 Jul 2016 [12:08:32], Eduardo Habkost wrote:
> > > From: Igor Mammedov <imammedo@redhat.com>
> > > 
> > > instance_id is generated by last_used_id + 1 for a given device type
> > > so for QEMU with 3 CPUs instance_id for APICs is a seti of [0, 1, 2]
> > > When CPU in the middle is hot-removed and migration started
> > > APICs with instance_ids 0 and 2 are transferred in migration stream.
> > > However target starts with 2 CPUs and APICs' instance_ids are
> > > generated from scratch [0, 1] hence migration fails with error
> > >   Unknown savevm section or instance 'apic' 2
> > > 
> > > Fix issue by manually registering APIC's vmsd with apic_id as
> > > instance_id, in this case instance_id on target will always
> > > match instance_id on source as apic_id is the same for a given
> > > cpu instance.
> > > 
> > > Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>  
> > 
> > After these patches, the static checker complains about missing
> > sections:
> > 
> > Section "apic-common" does not exist in dest
> > Section "apic" does not exist in dest
> > Section "kvm-apic" does not exist in dest
> It works for me, could you post reproducing commands?

This was flagged by a nightly run of the static checker when this
series was pulled.  On a 'before' tree, ie one w/o the patches, do
this:

qemu -dump-vmstate before.json

and for after:

qemu -dump-vmstate after.json

then,

python ./scripts/vmstate-static-checker.py -s before.json -d after.json

and that shows the output from above.


		Amit

Re: [Qemu-devel] [PULL 26/28] apic: Use apic_id as apic's migration instance_id

[Amit Shah]

On (Tue) 26 Jul 2016 [11:41:33], Igor Mammedov wrote:
> On Tue, 26 Jul 2016 10:41:38 +0530
> Amit Shah <amit.shah@redhat.com> wrote:
> 
> > On (Wed) 20 Jul 2016 [12:08:32], Eduardo Habkost wrote:
> > > From: Igor Mammedov <imammedo@redhat.com>
> > > 
> > > instance_id is generated by last_used_id + 1 for a given device type
> > > so for QEMU with 3 CPUs instance_id for APICs is a seti of [0, 1, 2]
> > > When CPU in the middle is hot-removed and migration started
> > > APICs with instance_ids 0 and 2 are transferred in migration stream.
> > > However target starts with 2 CPUs and APICs' instance_ids are
> > > generated from scratch [0, 1] hence migration fails with error
> > >   Unknown savevm section or instance 'apic' 2
> > > 
> > > Fix issue by manually registering APIC's vmsd with apic_id as
> > > instance_id, in this case instance_id on target will always
> > > match instance_id on source as apic_id is the same for a given
> > > cpu instance.
> > > 
> > > Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>  
> > 
> > After these patches, the static checker complains about missing
> > sections:
> > 
> > Section "apic-common" does not exist in dest
> > Section "apic" does not exist in dest
> > Section "kvm-apic" does not exist in dest
> > 
> > This will break migration from older versions.
> Still can't reproduce:
> here is my CLI on SRC:
>   qemu-system-x86_64-v2.6.0  \
>    -snapshot -enable-kvm -smp 6,maxcpus=6 -m 256M rhel72.img -monitor stdio -M pc-i440fx-2.6 -nodefaults
> 
> monitor# stop
> monitor# migrate "exec:gzip -c > STATEFILE.gz"
> ^C
> 
> CLI on DST:
>   qemu-system-x86_64-v2.7.0-rc0 \
>    -snapshot -enable-kvm -smp 6,maxcpus=6 -m 256M rhel72.img -monitor stdio -M pc-i440fx-2.6 -nodefaults -incoming "exec: gzip -c -d STATEFILE.gz"

I'll check.

> But I've found issue with I2C, which breaks migration for me with:
> 
> (qemu) qemu-system-x86_64: Missing section footer for i2c_bus
> qemu-system-x86_64: load of migration failed: Invalid argument
> 
> Which is bisects to:
> 
> commit 2293c27faddf9547dd8b52423caa6e85844eec3a
> Author: KONRAD Frederic <fred.konrad@greensocs.com>
> Date:   Tue Jun 14 15:59:14 2016 +0100
> 
>     i2c: implement broadcast write
> 
> hacking migration hunks of it to old VMState fixes I2C issue,
> and no apic related issues are noticed.

Yea, the i2c change will also break migration: adding a field
('broadcast') without updating version info.

i2c doesn't appear at all in the json output, so the script didn't
catch it.  I'll check why.

		Amit

Re: [Qemu-devel] [PULL 26/28] apic: Use apic_id as apic's migration instance_id

[Igor Mammedov]

On Tue, 26 Jul 2016 17:17:47 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> On (Tue) 26 Jul 2016 [10:00:49], Igor Mammedov wrote:
> > On Tue, 26 Jul 2016 10:41:38 +0530
> > Amit Shah <amit.shah@redhat.com> wrote:
> >   
> > > On (Wed) 20 Jul 2016 [12:08:32], Eduardo Habkost wrote:  
> > > > From: Igor Mammedov <imammedo@redhat.com>
> > > > 
> > > > instance_id is generated by last_used_id + 1 for a given device type
> > > > so for QEMU with 3 CPUs instance_id for APICs is a seti of [0, 1, 2]
> > > > When CPU in the middle is hot-removed and migration started
> > > > APICs with instance_ids 0 and 2 are transferred in migration stream.
> > > > However target starts with 2 CPUs and APICs' instance_ids are
> > > > generated from scratch [0, 1] hence migration fails with error
> > > >   Unknown savevm section or instance 'apic' 2
> > > > 
> > > > Fix issue by manually registering APIC's vmsd with apic_id as
> > > > instance_id, in this case instance_id on target will always
> > > > match instance_id on source as apic_id is the same for a given
> > > > cpu instance.
> > > > 
> > > > Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>    
> > > 
> > > After these patches, the static checker complains about missing
> > > sections:
> > > 
> > > Section "apic-common" does not exist in dest
> > > Section "apic" does not exist in dest
> > > Section "kvm-apic" does not exist in dest  
> > It works for me, could you post reproducing commands?  
> 
> This was flagged by a nightly run of the static checker when this
> series was pulled.  On a 'before' tree, ie one w/o the patches, do
> this:
> 
> qemu -dump-vmstate before.json
> 
> and for after:
> 
> qemu -dump-vmstate after.json
> 
> then,
> 
> python ./scripts/vmstate-static-checker.py -s before.json -d after.json
I don't think it is valid comparison though, as it compares default PC machines.
In this case it's pc-i440fx-2.6 and pc-i440fx-2.7, you see the difference
which is expected due to instance_id change.

You shouldn't see it when comparing same machine types.
 
> and that shows the output from above.
>
> 
> 		Amit

Re: [Qemu-devel] [PULL 26/28] apic: Use apic_id as apic's migration instance_id

[Amit Shah]

On (Tue) 26 Jul 2016 [14:58:39], Igor Mammedov wrote:
> > This was flagged by a nightly run of the static checker when this
> > series was pulled.  On a 'before' tree, ie one w/o the patches, do
> > this:
> > 
> > qemu -dump-vmstate before.json
> > 
> > and for after:
> > 
> > qemu -dump-vmstate after.json
> > 
> > then,
> > 
> > python ./scripts/vmstate-static-checker.py -s before.json -d after.json
> I don't think it is valid comparison though, as it compares default PC machines.
> In this case it's pc-i440fx-2.6 and pc-i440fx-2.7, you see the difference
> which is expected due to instance_id change.
> 
> You shouldn't see it when comparing same machine types.

No, this is comparing the git tree just before and after the series is
applied.

		Amit

Re: [Qemu-devel] [PULL 26/28] apic: Use apic_id as apic's migration instance_id

[Igor Mammedov]

On Tue, 26 Jul 2016 18:41:22 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> On (Tue) 26 Jul 2016 [14:58:39], Igor Mammedov wrote:
> > > This was flagged by a nightly run of the static checker when this
> > > series was pulled.  On a 'before' tree, ie one w/o the patches, do
> > > this:
> > > 
> > > qemu -dump-vmstate before.json
> > > 
> > > and for after:
> > > 
> > > qemu -dump-vmstate after.json
> > > 
> > > then,
> > > 
> > > python ./scripts/vmstate-static-checker.py -s before.json -d after.json  
> > I don't think it is valid comparison though, as it compares default PC machines.
> > In this case it's pc-i440fx-2.6 and pc-i440fx-2.7, you see the difference
> > which is expected due to instance_id change.
> > 
> > You shouldn't see it when comparing same machine types.  
> 
> No, this is comparing the git tree just before and after the series is
> applied.
I'd say it's expected change introduced by this commit, it should be fine
as it doesn't affect other machine types and 2.7 will be released with it.

I really don't see an issue here, care to point it out?

> 
> 		Amit

Re: [Qemu-devel] [PULL 26/28] apic: Use apic_id as apic's migration instance_id

[Igor Mammedov]

On Tue, 26 Jul 2016 18:41:22 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> On (Tue) 26 Jul 2016 [14:58:39], Igor Mammedov wrote:
> > > This was flagged by a nightly run of the static checker when this
> > > series was pulled.  On a 'before' tree, ie one w/o the patches, do
> > > this:
> > > 
> > > qemu -dump-vmstate before.json
> > > 
> > > and for after:
> > > 
> > > qemu -dump-vmstate after.json
> > > 
> > > then,
> > > 
> > > python ./scripts/vmstate-static-checker.py -s before.json -d after.json  
> > I don't think it is valid comparison though, as it compares default PC machines.
> > In this case it's pc-i440fx-2.6 and pc-i440fx-2.7, you see the difference
> > which is expected due to instance_id change.
> > 
> > You shouldn't see it when comparing same machine types.  
> 
> No, this is comparing the git tree just before and after the series is
> applied.
I've checked dump_vmstate_json_to_file() implementation and it looks like
it dumps only dc->vmsd enabled devices.

In this patch vmstate registration has been moved to to apic_comon_realize()
that's why dump_vmstate_json_to_file() doesn't dump apics anymore and you see the change

> 
> 		Amit
> 

Re: [Qemu-devel] [PULL 26/28] apic: Use apic_id as apic's migration instance_id

[Eduardo Habkost]

On Tue, Jul 26, 2016 at 04:16:04PM +0200, Igor Mammedov wrote:
> On Tue, 26 Jul 2016 18:41:22 +0530
> Amit Shah <amit.shah@redhat.com> wrote:
> 
> > On (Tue) 26 Jul 2016 [14:58:39], Igor Mammedov wrote:
> > > > This was flagged by a nightly run of the static checker when this
> > > > series was pulled.  On a 'before' tree, ie one w/o the patches, do
> > > > this:
> > > > 
> > > > qemu -dump-vmstate before.json
> > > > 
> > > > and for after:
> > > > 
> > > > qemu -dump-vmstate after.json
> > > > 
> > > > then,
> > > > 
> > > > python ./scripts/vmstate-static-checker.py -s before.json -d after.json  
> > > I don't think it is valid comparison though, as it compares default PC machines.
> > > In this case it's pc-i440fx-2.6 and pc-i440fx-2.7, you see the difference
> > > which is expected due to instance_id change.
> > > 
> > > You shouldn't see it when comparing same machine types.  
> > 
> > No, this is comparing the git tree just before and after the series is
> > applied.
> I've checked dump_vmstate_json_to_file() implementation and it looks like
> it dumps only dc->vmsd enabled devices.
> 
> In this patch vmstate registration has been moved to to apic_comon_realize()
> that's why dump_vmstate_json_to_file() doesn't dump apics anymore and you see the change

So, can anything break because of this difference, or is this
just a limitation of -dump-vmstate?

-- 
Eduardo

Re: [Qemu-devel] [PULL 26/28] apic: Use apic_id as apic's migration instance_id

[Igor Mammedov]

On Tue, 26 Jul 2016 16:19:28 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Jul 26, 2016 at 04:16:04PM +0200, Igor Mammedov wrote:
> > On Tue, 26 Jul 2016 18:41:22 +0530
> > Amit Shah <amit.shah@redhat.com> wrote:
> >   
> > > On (Tue) 26 Jul 2016 [14:58:39], Igor Mammedov wrote:  
> > > > > This was flagged by a nightly run of the static checker when this
> > > > > series was pulled.  On a 'before' tree, ie one w/o the patches, do
> > > > > this:
> > > > > 
> > > > > qemu -dump-vmstate before.json
> > > > > 
> > > > > and for after:
> > > > > 
> > > > > qemu -dump-vmstate after.json
> > > > > 
> > > > > then,
> > > > > 
> > > > > python ./scripts/vmstate-static-checker.py -s before.json -d after.json    
> > > > I don't think it is valid comparison though, as it compares default PC machines.
> > > > In this case it's pc-i440fx-2.6 and pc-i440fx-2.7, you see the difference
> > > > which is expected due to instance_id change.
> > > > 
> > > > You shouldn't see it when comparing same machine types.    
> > > 
> > > No, this is comparing the git tree just before and after the series is
> > > applied.  
> > I've checked dump_vmstate_json_to_file() implementation and it looks like
> > it dumps only dc->vmsd enabled devices.
> > 
> > In this patch vmstate registration has been moved to to apic_comon_realize()
> > that's why dump_vmstate_json_to_file() doesn't dump apics anymore and you see the change  
> 
> So, can anything break because of this difference, or is this
> just a limitation of -dump-vmstate?
it's limitation of -dump-vmstate, there shouldn't be anything
breaking because of the change.