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 02:52:50PM +0100, Igor Mammedov wrote:
> On Fri, 14 Dec 2012 10:27:34 -0200
> Eduardo Habkost <address@hidden> wrote:
> 
> > 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.
> with cpu properties and subclasses in place,
> kvm_check_features_against_host() could be expressed as simple subtraction of
> host subclass features list from guest cpu features list. Which won't require
> access to feature words at all. Just iterating over properties of both to
> build lists and then do subtraction and print difference if any, it could be
> eventially generalized to other kvm supported targets since it won't use any
> x86 specific fields.

Once we have the CPU properties code, maybe we could do that. I find the
idea a bit confusing (I would like to avoid coupling the "check" code to
the "host" CPU class directly), but may be doeable.

We also have one problem: the host class class_init method may be called
before KVM is initialized, so I don't know if we can really use the
class instrospection system to find out which flags are supported by the
host by looking at the "host" class.

Considering the complexity of what you suggest, I think there's value in
cleaning up that code first (and use the array to allow us to clean up
other code as well), and try to use the solution you suggest later,
after we actually have working CPU properties and subclasses.

-- 
Eduardo