Re: [Qemu-devel] [RFC] Enabling x2apic on most (all?) x86 CPU models

[Eduardo Habkost]

(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