Re: [Qemu-devel] [PATCH] target-i386: enable x2apic by default on more recent CPU models

[Andreas Färber]

Am 17.02.2014 14:58, schrieb Michael S. Tsirkin:
> On Tue, Feb 04, 2014 at 03:12:43PM +0100, Andreas Färber wrote:
>> Am 03.02.2014 20:01, schrieb Eduardo Habkost:
>>> On Tue, Jan 21, 2014 at 05:13:50PM +0100, Paolo Bonzini wrote:
>>>> Il 21/01/2014 16:51, Andreas Färber ha scritto:
>>>>>>> We already do that for other bits (e.g. XSAVE/OSXSAVE),
>>>>> Please point me to the commit, a search for xsave did not come up with a
>>>>> commit changing such a thing - either it did not go through my queue or
>>>>> it slipped me through: Bugs are no excuse to produce more bugs.
>>>>
>>>> I meant that "-cpu SandyBridge" with TCG produces a CPU that doesn't
>>>> have XSAVE.
>>>>
>>>>>>> and in fact it
>>>>>>> is the same that we do for KVM: the KVM_GET_SUPPORTED_CPUID result is
>>>>>>> used to trim the generic feature bits.
>>>>> Our model definitions are the place to put stuff that real CPUs have.
>>>>> Either the CPU has it or it doesn't. If it does, then this patch is
>>>>> fully correct and it's TCG's job to mask things out. If we're adding
>>>>> artificial flags to the generic model definitions just to make KVM
>>>>> faster, then it is wrong - we have a choice of post_initialize and
>>>>> realize hooks for that.
>>>>
>>>> It would make TCG faster as well, and there would be no reason
>>>> really to avoid the "artificial" x2apic on TCG, if TCG implemented
>>>> x2apic at all.
>>>
>>> So, the discussion seem to have stalled.
>>>
>>> Andreas, are you still against the patch, after the arguments from Paolo
>>> and me?
>>
>> Yes, I am. I had proposed to discuss solutions at FOSDEM but Paolo was
>> not there, so no solution yet.
> 
> We have the weekly call tomorrow.
> Let's discuss there?
> 
>> My main concern still is that if a CPU does not have a certain feature
>> we should not list it as one of its features but add it to its features
>> where sensible. Just because TCG filters it out today is not keeping
>> anyone from implementing it tomorrow, in which case the emulated CPUs
>> would suddenly gain the feature.
> 
> Why is this a problem?
> We will just have to make sure features stay consistent
> for old -M machine types.

I thought I made my point pretty clear: I will not accept a patch that

a) adds a feature flag that has no relation to the model it is named after

Instead of adding the flag to 9 models, define a rule of when to add it.
=> Model semantics remain clear.
=> Avoids someone forgetting the flag for new qualifying models.
=> 3 lines of code, including braces.

b) adds the flag to "more recent models"

Please define this in technical terms, so that we can implement a rule.
Patch is waiting in qom-cpu-next for that answer.
=> Avoids forgetting to add to some new "more recent" model.
Suppose I want to add a model for, e.g., the Intel Quark X1000 (new
product, but stripped of features), should it get the flag or not?!

c) relies on TCG filtering out a feature being added

Someone implementing it for TCG in 2.x needs to know for which models to
filter it out via x86_cpu_compat_set_features() or future compat_props.
Previously I was under the assumption they would need to be filtered out
based on whether the hardware actually has it, but in fact it would need
to be filtered out for all affected models. How?
=> kvm_enabled() for speed optimizations, tcg_enabled() for emulation.
If desired for TCG, it can be added via -cpu ...,+x2apic syntax. Why
would a user instead need to know she needs to use Westmere,-x2apic in
order to get a Westmere CPU. That's counter-intuitive.

Libvirt launching an x86 TCG guest will be rather rare BTW for
performance reasons waaay beyond x2apic, and it would be for the libvirt
folks to answer whether their TCG machines are supposed to get
performance-optimized or not (I doubt so, but anyway libvirt != QEMU; a
consequence would otherwise be enabling SIMD such as SSE42 for old x86
models, too, and NEON and VFPv4 for old ARM models, etc. so that we
would in practice only need one "any" model as for linux-user).

For accel=kvm we already do all sorts of tweaks, including vendor
override; no disagreement on the overall goal there. I just don't want
KVM people to ignorantly trample our emulation command line (ehabkost:
"academic"; I say consistency, usability and maintainability) for a
misguided convenience, when my suggestion probably even requires less
lines of code than the original patch, not to mention all the discussion
that followed and is taking away time for other patches.

Answers or v2 patches could've easily been provided via list already.
Without input, I see nothing to discuss really, Eduardo doesn't seem to
understand or care. I would be available tomorrow, but I will rather
step down as x86 CPU maintainer than sign off a hostile takeover by KVM
or libvirt! CC'ing PMM and rth to counter the KVM/libvirt bias in CC.

A compromise might be some global command line flag to enforce
hardware-close emulation or to tweak for performance. But that still
requires a) and b) to be addressed. And it would likely be redundant
with accel=.

Regards,
Andreas

>> So my question still is, what rule can
>> we apply for enabling x2apic? (something like greater or equal this
>> family, etc. - then we can put it in your post_initialize hook so that
>> users can still override it)
>>
>> Regards,
>> Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg