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 <ehabk...@redhat.com> 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
