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 04:14:32PM +0100, Andreas Färber wrote:
> Am 12.12.2012 23:22, schrieb Eduardo Habkost:
> > 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).
> > 
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > ---
> > This patch was created solely using a sed script and no manual changes,
> > to try to avoid mistakes while converting the code, and make it easier
> > to rebase if necessary. The sed script can be seen at:
> >   https://gist.github.com/4271991
> > ---
> >  hw/kvm/clock.c            |   2 +-
> >  linux-user/elfload.c      |   2 +-
> >  linux-user/main.c         |   4 +-
> >  target-i386/cpu.c         | 578 
> > +++++++++++++++++++++++-----------------------
> >  target-i386/cpu.h         |  30 +--
> >  target-i386/helper.c      |   4 +-
> >  target-i386/kvm.c         |   5 +-
> >  target-i386/misc_helper.c |  14 +-
> >  target-i386/translate.c   |  10 +-
> >  9 files changed, 331 insertions(+), 318 deletions(-)
> 
> I wonder, if we're touching all these lines anyway, can't we place the
> new feature array directly into X86CPU? As far as I see the features are
> never changed at runtime, so the only reason to have them in the
> instance is the command-line-supplied overrides.

You mean directly into X86CPUClass? No, we can't: the features are
changed at the CPU instance at runtime, when based in the
+feature,-feature feature string. We also do some extra filtering based
on KVM capabilities at filter_features_for_kvm().

I believe we're moving towards another direction: making the feature
bits live only in the X86CPU object, and they will be initialized based
on the default property values of each CPU class. Probably we won't do
that in the first version of CPU subclasses/properties, but we are
trying to reach that point.

> 
> The clock code using first_cpu looks solvable; what about CR4 and MSR
> helpers, how performance-sensitive are they? (if they're not yet using
> X86CPU for something else)

I guess any CPU-state code inside QEMU is not performance-sensitive, as
it woud already require switching between KVM kernelspace and QEMU
userspace.

> 
> With the proposed variable change env -> cpu it would not be fully
> sed'able, but as a maintainer I need to review the whole patch anyway.

On the other hand, this cleanup will allow us to easily convert some
code to deal with the feature array only (not requiring the full X86CPU
or x86_def_t struct), making it easier to have only one feature array,
in only one place, in the future.

> 
> Either way since this change affects not just the core CPU that I have
> started to maintain I feel I should give other target-i386 stakeholders
> (Blue, Aurélien, malc, ...) sufficient time to object, so not before
> Christmas realistically.

OK.

-- 
Eduardo