Re: [Qemu-devel] [PATCH 3/3] target-i386: replace cpuid_*features fields with a feature word array

[Eduardo Habkost]

On Fri, Dec 14, 2012 at 03:53:58PM +0100, Andreas Färber wrote:
> Am 14.12.2012 13:27, schrieb Eduardo Habkost:
> > On Fri, Dec 14, 2012 at 10:38:50AM +0100, Igor Mammedov wrote:
> >> On Wed, 12 Dec 2012 20:22:26 -0200
> >> Eduardo Habkost <address@hidden> wrote:
> >>
> >>> This replaces the feature-bit fields on both X86CPU and x86_def_t
> >>> structs with an array.
> >>>
> >>> With this, we will be able to simplify code that simply does the same
> >>> operation on all feature words (e.g. kvm_check_features_against_host(),
> >>> filter_features_for_kvm(), add_flagname_to_bitmaps(), and CPU
> >>> feature-bit property lookup/registration).
> >>>
> >>
> >> do you have a patch that simplifies kvm_check_features_against_host() using
> >> this?
> > 
> > I have a very old one, based on an older (and more complex) version of
> > this series:
> > https://github.com/ehabkost/qemu-hacks/commit/eb01d374baecf6df26fd6f0d0bb23f2e1547f499
> > 
> > It's in the work/cpuid-refactor-v0.22-2012-08-31 branch in my git
> > repository.
> > 
> > That branch also has some patches to merge kvm_check_features_against_host()
> > and filter_features_for_kvm() (because the purpose of
> > kvm_check_features_against_host() is simply to check if anything is
> > going to be filtered out by filter_features_for_kvm()).
> > 
> > If people are happy with the approach in this series, I plan to write
> > and submit cleanups for kvm_cpu_fill_host(),
> > kvm_check_features_against_host(), filter_features_for_kvm(),
> > add_flagname_to_bitmaps(), and the cpudef -> CPU feature copying code.
> > 
> > There's so much code that could be cleaned up using the array, that I am
> > afraid that it would cause too much conflicts in the CPU properties
> > work. So I can wait until the CPU properties series are submitted before
> > making the cleanups, if necessary.
> 
> As stated elsewhere, for me a proper way to define new CPU models has
> higher priority than feature properties, especially now that we're
> headed for a class_init based approach where properties cannot be used
> for original initialization.

We can go that way. But then I still strongly suggest we add the feature
array before doing that, because it will help a lot to simplify the
"host" CPU subclass code, and kvm_check_features_against_host() and
kvm_cpu_fill_host().

(The main change I see is that kvm_check_features_against_host() won't
need to fill a full x86_def_t struct from the host, just a local
in-stack feature array. Also, kvm_check_features_against_host() and
filter_features_for_kvm() could be unified as well)

(Maybe I can work around the lack of feature array by keeping an
embedded x86_def_t struct inside CPUClass (so kvm_cpu_fill_host() don't
need a full CPUClass struct like in your RFC). I will try and see what's
possible)

> 
> As suggested with my RFC I would like to take a quick, simplistic route
> to subclasses where no major refactoring of data fields happens. If we
> prepend a couple coding style and function movement patches, that's fine
> with me. But if we do large functional refactorings > 10 patches that
> change history will be clobbered by the touch-all conversion and we need
> to do intensive functional testing, for which I don't see sufficient
> time before the Soft Freeze, given the holidays.

I thought we would be in the "we can't introduce major changes" mode
_after_ the soft freeze, not before the soft freeze.  ;-)

So, are we already in soft freeze in practice?


> 
> When the feature bits are stored in the classes, setting them via
> properties in instance_init seems like overkill (visitor overhead);

If performance is an issue (I doubt it would be), we could still keep
the array inside the model/class, and copy it on instance_init. But we
still need a per-instance array as well, in addition to the per-model
data (see below).


> we
> no longer dump them in -cpu ?foo; only +feature/-feature would benefit.
> Thus I assume them to mainly conflict where function signatures change
> and trivially where def changes to env, making them rather orthogonal to
> each other.

We can't just change "env" to "def", because "def" is static and "env"
contains the result of the "<model>,+feature,-feature" parsing.

We could try to go through a route where there's no def->env copy in the
code, but then I believe this would be the opposite of the quick
simplistic route you're trying to take.

-- 
Eduardo