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

[Eduardo Habkost]

On Mon, Feb 17, 2014 at 05:17:51PM +0100, Andreas Färber wrote:
> 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.

I am not worried about code size, I worry about complexity added when
probing for CPU model information from the outside. Right now we have a
single namespace for CPU models, where the results all look the same on
TCG and KVM mode (except for the CPU vendor, and I wouldn't like to make
the list of exceptions grow).

If you propose changing it and having different results in KVM and TCG
mode, then let's discuss how to implement that. If we are going to do
that, I would like to have different CPU model names for KVM and TCG CPU
models, so management code won't get confused. Or maybe we could flag
some KVM-specific CPU models as KVM-only. Or we could look for other
solutions.

> 
> 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?!

What about "CPU models that can't run in TCG mode and are only useful
for KVM"? The Opteron_G* models can't run in TCG mode today, and were
added only because of KVM. I am not sure about Penryn, Conroe, Nehalem,
Westmere, but I won't be surprised if they can't run in TCG mode as
well.

All I want is to have CPU models that are useful. You, too, want CPU
models that are useful, but for different purposes. We have two
conflicting goals, so let's find solutions that will help us reach those
goals.


> 
> 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.

Having "-cpu Westmere" mean different things in KVM and TCG mode is,
too, counter-intuitive. I am trying to find a trade-off here.


> 
> 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).

The point was not optimizing TCG, but avoiding adding complexity by
making the CPU models look different in KVM and TCG mode.


> 
> 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.

We fortunately don't do "all sorts of tweaks", except for the vendor
override, which also adds the kind of complexity I am trying to avoid.


> 
> 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.

I was still waiting for an answer for my questions/assumptions at:
http://marc.info/?l=qemu-devel&m=139176948602576

I didn't send v2 because I still don't see my main point being answered:
I believed we wanted to avoid having separate CPU model namespaces for
TCG and KVM (in other words, having different results depending on
accel)[1]. Has this changed?

I am trying to find and evaluate alternatives, but it is hard to do that
when I don't get my questions answered. I don't understand why you call
this a "hostile takeover".


[1] http://marc.info/?l=qemu-devel&m=136983789405010&w=2

> 
> 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=.

If we change the resulting CPU depending on a new command-line option or
accel=, we are in practice having two CPU model namespaces, depending on
that option (see [1] above).

Let's discuss approaches to implement that tomorrow, then? Because I had
the feeling we had discarded that option in the past.

> 
> 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

-- 
Eduardo