From 709787ee997f0a0ccab78e0edaf10d48929151ee Mon Sep 17 00:00:00 2001 From: "Dr. David Alan Gilbert" Date: Fri, 8 Jul 2016 16:01:35 +0100 Subject: [PATCH 01/28] target-i386: Provide TCG_PHYS_ADDR_BITS Provide a constant for the number of address bits supported under TCG. Signed-off-by: Dr. David Alan Gilbert Suggested-by: Eduardo Habkost Reviewed-by: Eduardo Habkost Signed-off-by: Eduardo Habkost --- 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 5b14a72baa..8b6bd0be2a 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 From af45907a132857cfd47acc998bf5f7c26cd13071 Mon Sep 17 00:00:00 2001 From: "Dr. David Alan Gilbert" Date: Fri, 8 Jul 2016 16:01:36 +0100 Subject: [PATCH 02/28] target-i386: Allow physical address bits to be set 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 Reviewed-by: Eduardo Habkost Signed-off-by: Eduardo Habkost --- 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 6e49e4ca82..669180e3fe 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 8b6bd0be2a..b2ddcb8085 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; From 112dad69d723a68205f255dd46d78871b5c5a8ca Mon Sep 17 00:00:00 2001 From: "Dr. David Alan Gilbert" Date: Fri, 8 Jul 2016 16:01:37 +0100 Subject: [PATCH 03/28] target-i386: Mask mtrr mask based on CPU physical address limits 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 Reviewed-by: Eduardo Habkost Signed-off-by: Eduardo Habkost --- 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 93275231ec..2f1cc62795 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); } } From fcc35e7ccaed771790940524f3b0eef7aebfc9b1 Mon Sep 17 00:00:00 2001 From: "Dr. David Alan Gilbert" Date: Fri, 8 Jul 2016 16:01:38 +0100 Subject: [PATCH 04/28] target-i386: Fill high bits of mtrr mask 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 Suggested-by: Paolo Bonzini Reviewed-by: Eduardo Habkost Signed-off-by: Eduardo Habkost --- 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 e38c95a4da..47411cbde4 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 669180e3fe..5ac7e975ad 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 b2ddcb8085..b2ab8bf4be 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 2f1cc62795..df28dd254a 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; } From d9c84f196970f78d4b55ab87e03cbcad7c65f86f Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Wed, 6 Jul 2016 08:20:37 +0200 Subject: [PATCH 05/28] target-i386: Use uint32_t for X86CPU.apic_id 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 Reviewed-by: Eduardo Habkost Signed-off-by: Eduardo Habkost --- 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 5ac7e975ad..5fc01c61dc 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 b2ab8bf4be..10d562d4ea 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; From 9f3aab58539b4cc716e42e772be8116dc2e7d159 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Wed, 6 Jul 2016 08:20:38 +0200 Subject: [PATCH 06/28] pc: Add x86_topo_ids_from_apicid() 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 Reviewed-by: Eduardo Habkost Signed-off-by: Eduardo Habkost --- 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 fc95572394..1ebaee0f76 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. From 11f6fee576680a2d482123535da920f8ceb33eb5 Mon Sep 17 00:00:00 2001 From: "Dr. David Alan Gilbert" Date: Mon, 11 Jul 2016 20:28:46 +0100 Subject: [PATCH 07/28] target-i386: Set physical address bits based on host 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 Reviewed-by: Eduardo Habkost Signed-off-by: Eduardo Habkost --- 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 5fc01c61dc..89ca3268d4 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 10d562d4ea..0ff88e155a 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; From 7baef5cfea3d2271442fee602eb0b0c872b54b8e Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Wed, 6 Jul 2016 08:20:39 +0200 Subject: [PATCH 08/28] pc: Extract CPU lookup into a separate function It will be reused in the next patch at pre_plug time Signed-off-by: Igor Mammedov Reviewed-by: Eduardo Habkost Signed-off-by: Eduardo Habkost --- 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 719884ff88..c9999a8da2 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); From 4ec60c76d5ab513e375f17b043d2b9cb849adf6c Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Wed, 6 Jul 2016 08:20:40 +0200 Subject: [PATCH 09/28] pc: cpu: Consolidate apic-id validity checks in pc_cpu_pre_plug() 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 Reviewed-by: Eduardo Habkost Signed-off-by: Eduardo Habkost --- 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 c9999a8da2..81206f4855 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 89ca3268d4..5d77c09863 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; } From 2da00e3176abac34ca7a6aab1f5bbb94a0d03fc5 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Wed, 6 Jul 2016 08:20:41 +0200 Subject: [PATCH 10/28] target-i386: Replace custom apic-id setter/getter with static property 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 Reviewed-by: Eduardo Habkost Signed-off-by: Eduardo Habkost --- 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 5d77c09863..af2d694e51 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), From d89c2b8b98e097b9cad5104b0f178bde1cfa011b Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Wed, 6 Jul 2016 08:20:42 +0200 Subject: [PATCH 11/28] target-i386: Add socket/core/thread properties to X86CPU These properties will be used by as address where to plug CPU with help -device/device_add commands. Signed-off-by: Igor Mammedov Reviewed-by: Eduardo Habkost Signed-off-by: Eduardo Habkost --- 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 81206f4855..394abfef37 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 af2d694e51..de0295a51f 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 0ff88e155a..c883cd5631 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) From c2f193b538032accb9db504998bf2ea7c0ef65af Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 12 Jul 2016 11:15:44 +0200 Subject: [PATCH 12/28] target-i386: Add support for UMIP and RDPID CPUID bits 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 Signed-off-by: Eduardo Habkost --- 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 de0295a51f..c36441d30c 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 c883cd5631..65615c0f3b 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) From 6816b1b3811e839540df22855d975b6d76ae438b Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Wed, 6 Jul 2016 08:20:52 +0200 Subject: [PATCH 13/28] target-i386: cpu: Do not ignore error and fix apic parent 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 Reviewed-by: Eduardo Habkost Signed-off-by: Eduardo Habkost --- 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 c36441d30c..6c36b137b8 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); From 67e55caa6dcb91c80428cee6fe463f8dd8a755ab Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Wed, 6 Jul 2016 08:20:53 +0200 Subject: [PATCH 14/28] target-i386: Fix apic object leak when CPU is deleted Signed-off-by: Igor Mammedov Reviewed-by: Eduardo Habkost Signed-off-by: Eduardo Habkost --- target-i386/cpu.c | 1 + 1 file changed, 1 insertion(+) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 6c36b137b8..5d0e085c88 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<> */ From e8f7b83e886361e4b16d46936f72d46bcf8fcb5b Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 14 Jul 2016 18:54:30 +0200 Subject: [PATCH 15/28] pc: Set APIC ID based on socket/core/thread ids if it's not been set yet 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 Reviewed-by: Eduardo Habkost Signed-off-by: Eduardo Habkost --- 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 394abfef37..72454fb8f7 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; } From ba157b696c4a82234ef1b43e98a7c74455538263 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 14 Jul 2016 18:54:31 +0200 Subject: [PATCH 16/28] pc: Delay setting number of boot CPUs to machine_done time 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 --- 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 72454fb8f7..669382f8e7 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; From a44a49dbf2eb8b9b430711b152663678c15f689c Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 14 Jul 2016 18:54:32 +0200 Subject: [PATCH 17/28] pc: Register created initial and hotpluged CPUs in one place pc_cpu_plug() 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 Reviewed-by: Eduardo Habkost Signed-off-by: Eduardo Habkost --- 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 669382f8e7..f4c6a27508 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) { + hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev); + hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err); + if (local_err) { + goto out; + } } - if (!pcms->acpi_dev) { - error_setg(&local_err, - "cpu hotplug is not enabled: missing acpi device"); - goto out; + if (dev->hotplugged) { + /* increment the number of CPUs */ + rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1); } - hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev); - hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err); - if (local_err) { - goto out; - } - - /* 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: From 73360e27850b213327011f7e22e03865b8c0dd5b Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Mon, 18 Jul 2016 10:31:22 +0200 Subject: [PATCH 18/28] pc: Forbid BSP removal 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 Reviewed-by: Michael S. Tsirkin Signed-off-by: Eduardo Habkost --- hw/i386/pc.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/hw/i386/pc.c b/hw/i386/pc.c index f4c6a27508..dd5661a72d 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); From 4da7faaeb0c7dd3f7f233165d336c878f78fd1eb Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Mon, 18 Jul 2016 10:32:36 +0200 Subject: [PATCH 19/28] pc: Enforce adding CPUs contiguously and removing them in opposite order It will still allow us to use cpu_index as migration instance_id since when CPUs are added contiguously (from the first to the last) and removed in opposite order, cpu_index stays stable and it's 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 Reviewed-by: Michael S. Tsirkin Signed-off-by: Eduardo Habkost --- hw/i386/pc.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/hw/i386/pc.c b/hw/i386/pc.c index dd5661a72d..68d8cad827 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 */ From edd1211194cd71afd78daf148c46801937ec11f5 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Fri, 27 May 2016 13:50:48 +0200 Subject: [PATCH 20/28] pc: cpu: Allow device_add to be used with x86 cpu Signed-off-by: Igor Mammedov Reviewed-by: Michael S. Tsirkin Signed-off-by: Eduardo Habkost --- target-i386/cpu.c | 1 + 1 file changed, 1 insertion(+) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 5d0e085c88..3df5f5f195 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(). From 4d952914a03548b863c3c0af191d7e2af482f09e Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Wed, 22 Jun 2016 11:11:42 +0200 Subject: [PATCH 21/28] pc: Implement query-hotpluggable-cpus callback 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 Reviewed-by: Eduardo Habkost Reviewed-by: Michael S. Tsirkin Signed-off-by: Eduardo Habkost --- 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 68d8cad827..13843da635 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 496d73c275..c8d360ad36 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} + } + ]} From 889211b18b8d0acc814fbbe01b986f07b229a8c9 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 5 May 2016 17:14:37 +0200 Subject: [PATCH 22/28] apic: move MAX_APICS check to 'apic' class MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Radim Krčmář Reviewed-by: Michael S. Tsirkin Signed-off-by: Eduardo Habkost --- 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 e1ab9354c6..b0d237b945 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 e6eb694de0..fd425d1d3c 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 73ce71674f..67348e9571 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; From 1dfe3282cf851dce186ab15b07225e5d8588b63f Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Fri, 24 Jun 2016 16:27:00 +0200 Subject: [PATCH 23/28] apic: Drop APICCommonState.idx and use APIC ID as index in local_apics[] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Radim Krčmář Reviewed-by: Michael S. Tsirkin Signed-off-by: Eduardo Habkost --- 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 b0d237b945..f473572fb4 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 67348e9571..8330592638 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; From 365aa1131fa61815eb1d672df6ba451bfe7f2cea Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 14 Jul 2016 16:58:02 +0200 Subject: [PATCH 24/28] apic: kvm-apic: Fix crash due to access to freed memory region 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 Reviewed-by: Michael S. Tsirkin Signed-off-by: Eduardo Habkost --- 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 c5983c79be..1f87e73d4d 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; From 9c156f9de52b75510d3951dfede2cf96773b6626 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Thu, 23 Jun 2016 17:30:14 +0200 Subject: [PATCH 25/28] (kvm)apic: Add unrealize callbacks Callbacks will do necessary cleanups before APIC device is deleted Signed-off-by: Chen Fan Signed-off-by: Gu Zheng Signed-off-by: Zhu Guihua Signed-off-by: Igor Mammedov Reviewed-by: Michael S. Tsirkin Signed-off-by: Eduardo Habkost --- 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 1f87e73d4d..2bd0de82b4 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 f473572fb4..45887d99c0 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 fd425d1d3c..0bb9ad51fa 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 8330592638..e549a62e7f 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); From f6e984443f1634eee8d6876a7cce039b976c95e0 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Fri, 1 Jul 2016 17:53:56 +0200 Subject: [PATCH 26/28] apic: Use apic_id as apic's migration instance_id 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 Signed-off-by: Igor Mammedov Reviewed-by: Dr. David Alan Gilbert Reviewed-by: Michael S. Tsirkin Signed-off-by: Eduardo Habkost --- 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 0bb9ad51fa..14ac43c186 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 e549a62e7f..06c4e9f6f9 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 47411cbde4..bc937b989e 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 \ From c884776e9dc947105827bd6c22192863f97267d2 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Fri, 24 Jun 2016 16:01:02 +0200 Subject: [PATCH 27/28] target-i386: Add x86_cpu_unrealizefn() First remove VCPU from exec loop and only then remove lapic. Signed-off-by: Chen Fan Signed-off-by: Gu Zheng Signed-off-by: Zhu Guihua Signed-off-by: Igor Mammedov Reviewed-by: Eduardo Habkost Signed-off-by: Eduardo Habkost --- target-i386/cpu.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 3df5f5f195..6a1afab595 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; From 8fe6374e8e0c8dacb85e9e97897291541dd61be6 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Fri, 24 Jun 2016 15:36:11 +0200 Subject: [PATCH 28/28] pc: Make device_del CPU work for x86 CPUs 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 Reviewed-by: Michael S. Tsirkin Signed-off-by: Eduardo Habkost --- 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 13843da635..ac7a4d5a47 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); }