* [Qemu-devel] [RFC] Enabling x2apic on most (all?) x86 CPU models @ 2013-09-18 20:39 Eduardo Habkost 2013-09-19 11:50 ` Andreas Färber 0 siblings, 1 reply; 4+ messages in thread From: Eduardo Habkost @ 2013-09-18 20:39 UTC (permalink / raw) To: qemu-devel; +Cc: Igor Mammedov, Bandan Das, Andreas Färber, Gleb Natapov Hi, I would like to get your opinion on this: Currently we have x2apic enabled only on SandyBridge and Haswell CPU models because we try to keep the CPU models closer to real CPUs. However, x2apic improves performance by reducing the overhead of APIC accesses, and KVM can emulate it independently of host CPU support for x2apic. This feature is present on KVM for 4 years, already (since v2.6.32). There's no reason for people to not have x2apic enabled when running KVM. So, my question is: should we break the "try to be close to real CPUs" rule and enable x2apic by default on most (or all) CPU models? I believe it is a reasonable thing to do. Also: if we do it, should we do it for all CPU models on target-i386/cpu.c, or just a subset of them? (maybe the more recent ones?) (The patch below touches only Conroe, Penryn, Nehalem, and Westmere, and it lacks machine-type compatibility code. But I am planning to submit a patch that changes all CPU models to include x2apic by default.) --- diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 9abb73f..f76c34b 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -791,7 +791,7 @@ static x86_def_t builtin_x86_defs[] = { CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE | CPUID_DE | CPUID_FP87, .features[FEAT_1_ECX] = - CPUID_EXT_SSSE3 | CPUID_EXT_SSE3, + CPUID_EXT_SSSE3 | CPUID_EXT_SSE3 | CPUID_EXT_X2APIC, .features[FEAT_8000_0001_EDX] = CPUID_EXT2_LM | CPUID_EXT2_NX | CPUID_EXT2_SYSCALL, .features[FEAT_8000_0001_ECX] = @@ -814,7 +814,7 @@ static x86_def_t builtin_x86_defs[] = { CPUID_DE | CPUID_FP87, .features[FEAT_1_ECX] = CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | - CPUID_EXT_SSE3, + CPUID_EXT_SSE3 | CPUID_EXT_X2APIC, .features[FEAT_8000_0001_EDX] = CPUID_EXT2_LM | CPUID_EXT2_NX | CPUID_EXT2_SYSCALL, .features[FEAT_8000_0001_ECX] = @@ -837,7 +837,8 @@ static x86_def_t builtin_x86_defs[] = { CPUID_DE | CPUID_FP87, .features[FEAT_1_ECX] = CPUID_EXT_POPCNT | CPUID_EXT_SSE42 | CPUID_EXT_SSE41 | - CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | CPUID_EXT_SSE3, + CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | CPUID_EXT_SSE3 | + CPUID_EXT_X2APIC, .features[FEAT_8000_0001_EDX] = CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX, .features[FEAT_8000_0001_ECX] = @@ -861,7 +862,7 @@ static x86_def_t builtin_x86_defs[] = { .features[FEAT_1_ECX] = CPUID_EXT_AES | CPUID_EXT_POPCNT | CPUID_EXT_SSE42 | CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | - CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3, + CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3 | CPUID_EXT_X2APIC, .features[FEAT_8000_0001_EDX] = CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX, .features[FEAT_8000_0001_ECX] = ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [RFC] Enabling x2apic on most (all?) x86 CPU models 2013-09-18 20:39 [Qemu-devel] [RFC] Enabling x2apic on most (all?) x86 CPU models Eduardo Habkost @ 2013-09-19 11:50 ` Andreas Färber 2013-09-19 16:58 ` Eduardo Habkost 0 siblings, 1 reply; 4+ messages in thread From: Andreas Färber @ 2013-09-19 11:50 UTC (permalink / raw) To: Eduardo Habkost; +Cc: Igor Mammedov, Bandan Das, qemu-devel, Gleb Natapov Hi, Am 18.09.2013 22:39, schrieb Eduardo Habkost: > Hi, > > I would like to get your opinion on this: > > Currently we have x2apic enabled only on SandyBridge and Haswell CPU > models because we try to keep the CPU models closer to real CPUs. > However, x2apic improves performance by reducing the overhead of APIC > accesses, and KVM can emulate it independently of host CPU support for > x2apic. This feature is present on KVM for 4 years, already (since > v2.6.32). There's no reason for people to not have x2apic enabled when > running KVM. > > So, my question is: should we break the "try to be close to real CPUs" > rule and enable x2apic by default on most (or all) CPU models? I believe > it is a reasonable thing to do. I disagree, since this would also affect TCG. I would prefer to add x2apic only to models that really have it and would be open to generally enabling it for kvm_enabled() in instance_init/registration (so that users can disable it via ,-x2apic or soon QMP). As always, software might make weird assumptions about effects of a present CPUID bit, but I trust you'll do some more testing before submitting a non-RFC patch. :) Regards, Andreas > > Also: if we do it, should we do it for all CPU models on > target-i386/cpu.c, or just a subset of them? (maybe the more recent > ones?) > > (The patch below touches only Conroe, Penryn, Nehalem, and Westmere, and > it lacks machine-type compatibility code. But I am planning to submit a > patch that changes all CPU models to include x2apic by default.) > > --- > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 9abb73f..f76c34b 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -791,7 +791,7 @@ static x86_def_t builtin_x86_defs[] = { > CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE | > CPUID_DE | CPUID_FP87, > .features[FEAT_1_ECX] = > - CPUID_EXT_SSSE3 | CPUID_EXT_SSE3, > + CPUID_EXT_SSSE3 | CPUID_EXT_SSE3 | CPUID_EXT_X2APIC, > .features[FEAT_8000_0001_EDX] = > CPUID_EXT2_LM | CPUID_EXT2_NX | CPUID_EXT2_SYSCALL, > .features[FEAT_8000_0001_ECX] = > @@ -814,7 +814,7 @@ static x86_def_t builtin_x86_defs[] = { > CPUID_DE | CPUID_FP87, > .features[FEAT_1_ECX] = > CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | > - CPUID_EXT_SSE3, > + CPUID_EXT_SSE3 | CPUID_EXT_X2APIC, > .features[FEAT_8000_0001_EDX] = > CPUID_EXT2_LM | CPUID_EXT2_NX | CPUID_EXT2_SYSCALL, > .features[FEAT_8000_0001_ECX] = > @@ -837,7 +837,8 @@ static x86_def_t builtin_x86_defs[] = { > CPUID_DE | CPUID_FP87, > .features[FEAT_1_ECX] = > CPUID_EXT_POPCNT | CPUID_EXT_SSE42 | CPUID_EXT_SSE41 | > - CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | CPUID_EXT_SSE3, > + CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | CPUID_EXT_SSE3 | > + CPUID_EXT_X2APIC, > .features[FEAT_8000_0001_EDX] = > CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX, > .features[FEAT_8000_0001_ECX] = > @@ -861,7 +862,7 @@ static x86_def_t builtin_x86_defs[] = { > .features[FEAT_1_ECX] = > CPUID_EXT_AES | CPUID_EXT_POPCNT | CPUID_EXT_SSE42 | > CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | > - CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3, > + CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3 | CPUID_EXT_X2APIC, > .features[FEAT_8000_0001_EDX] = > CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX, > .features[FEAT_8000_0001_ECX] = > -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [RFC] Enabling x2apic on most (all?) x86 CPU models 2013-09-19 11:50 ` Andreas Färber @ 2013-09-19 16:58 ` Eduardo Habkost 2013-09-19 17:19 ` Paolo Bonzini 0 siblings, 1 reply; 4+ messages in thread From: Eduardo Habkost @ 2013-09-19 16:58 UTC (permalink / raw) To: Andreas Färber Cc: Igor Mammedov, Bandan Das, qemu-devel, Gleb Natapov, Paolo Bonzini (CCing Paolo, as he was involved in the previous discussion about TCG vs KVM CPU models) On Thu, Sep 19, 2013 at 01:50:58PM +0200, Andreas Färber wrote: > Hi, > > Am 18.09.2013 22:39, schrieb Eduardo Habkost: > > Hi, > > > > I would like to get your opinion on this: > > > > Currently we have x2apic enabled only on SandyBridge and Haswell CPU > > models because we try to keep the CPU models closer to real CPUs. > > However, x2apic improves performance by reducing the overhead of APIC > > accesses, and KVM can emulate it independently of host CPU support for > > x2apic. This feature is present on KVM for 4 years, already (since > > v2.6.32). There's no reason for people to not have x2apic enabled when > > running KVM. > > > > So, my question is: should we break the "try to be close to real CPUs" > > rule and enable x2apic by default on most (or all) CPU models? I believe > > it is a reasonable thing to do. > > I disagree, since this would also affect TCG. I would prefer to add > x2apic only to models that really have it and would be open to generally > enabling it for kvm_enabled() in instance_init/registration (so that > users can disable it via ,-x2apic or soon QMP). This won't affect TCG because features not supported by TCG are removed automatically, and "enforce" doesn't even work on TCG mode (yet). I believe we agreed that we don't want to make the semantics of CPU model names change depending if KVM or TCG are enabled[1], so I am trying to avoid making the default depend on kvm_enabled(). [1] http://marc.info/?l=qemu-devel&m=136983789405010&w=2 > > As always, software might make weird assumptions about effects of a > present CPUID bit, but I trust you'll do some more testing before > submitting a non-RFC patch. :) FWIW, on RHEL-6 x2apic is already set on Conroe/Penryn/Nehalem/Westmere/Opteron_G[1234], since RHEL-6.0. So at least on those CPU models, the presence of the x2apic flag already got a reasonable amount of testing. > > Regards, > Andreas > > > > > Also: if we do it, should we do it for all CPU models on > > target-i386/cpu.c, or just a subset of them? (maybe the more recent > > ones?) > > > > (The patch below touches only Conroe, Penryn, Nehalem, and Westmere, and > > it lacks machine-type compatibility code. But I am planning to submit a > > patch that changes all CPU models to include x2apic by default.) > > > > --- > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 9abb73f..f76c34b 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -791,7 +791,7 @@ static x86_def_t builtin_x86_defs[] = { > > CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE | > > CPUID_DE | CPUID_FP87, > > .features[FEAT_1_ECX] = > > - CPUID_EXT_SSSE3 | CPUID_EXT_SSE3, > > + CPUID_EXT_SSSE3 | CPUID_EXT_SSE3 | CPUID_EXT_X2APIC, > > .features[FEAT_8000_0001_EDX] = > > CPUID_EXT2_LM | CPUID_EXT2_NX | CPUID_EXT2_SYSCALL, > > .features[FEAT_8000_0001_ECX] = > > @@ -814,7 +814,7 @@ static x86_def_t builtin_x86_defs[] = { > > CPUID_DE | CPUID_FP87, > > .features[FEAT_1_ECX] = > > CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | > > - CPUID_EXT_SSE3, > > + CPUID_EXT_SSE3 | CPUID_EXT_X2APIC, > > .features[FEAT_8000_0001_EDX] = > > CPUID_EXT2_LM | CPUID_EXT2_NX | CPUID_EXT2_SYSCALL, > > .features[FEAT_8000_0001_ECX] = > > @@ -837,7 +837,8 @@ static x86_def_t builtin_x86_defs[] = { > > CPUID_DE | CPUID_FP87, > > .features[FEAT_1_ECX] = > > CPUID_EXT_POPCNT | CPUID_EXT_SSE42 | CPUID_EXT_SSE41 | > > - CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | CPUID_EXT_SSE3, > > + CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | CPUID_EXT_SSE3 | > > + CPUID_EXT_X2APIC, > > .features[FEAT_8000_0001_EDX] = > > CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX, > > .features[FEAT_8000_0001_ECX] = > > @@ -861,7 +862,7 @@ static x86_def_t builtin_x86_defs[] = { > > .features[FEAT_1_ECX] = > > CPUID_EXT_AES | CPUID_EXT_POPCNT | CPUID_EXT_SSE42 | > > CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | > > - CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3, > > + CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3 | CPUID_EXT_X2APIC, > > .features[FEAT_8000_0001_EDX] = > > CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX, > > .features[FEAT_8000_0001_ECX] = > > > > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg -- Eduardo ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [RFC] Enabling x2apic on most (all?) x86 CPU models 2013-09-19 16:58 ` Eduardo Habkost @ 2013-09-19 17:19 ` Paolo Bonzini 0 siblings, 0 replies; 4+ messages in thread From: Paolo Bonzini @ 2013-09-19 17:19 UTC (permalink / raw) To: Eduardo Habkost Cc: Igor Mammedov, Bandan Das, Andreas Färber, Gleb Natapov, qemu-devel Il 19/09/2013 18:58, Eduardo Habkost ha scritto: >> > >> > I disagree, since this would also affect TCG. I would prefer to add >> > x2apic only to models that really have it and would be open to generally >> > enabling it for kvm_enabled() in instance_init/registration (so that >> > users can disable it via ,-x2apic or soon QMP). > This won't affect TCG because features not supported by TCG are removed > automatically, and "enforce" doesn't even work on TCG mode (yet). > > I believe we agreed that we don't want to make the semantics of CPU > model names change depending if KVM or TCG are enabled[1], so I am > trying to avoid making the default depend on kvm_enabled(). I agree. I don't think it would matter much if one day TCG starts supporting x2apic (it's probably only a few hours work) and old CPUs started showing x2apic. Paolo ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-09-19 17:19 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-09-18 20:39 [Qemu-devel] [RFC] Enabling x2apic on most (all?) x86 CPU models Eduardo Habkost 2013-09-19 11:50 ` Andreas Färber 2013-09-19 16:58 ` Eduardo Habkost 2013-09-19 17:19 ` Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).