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 06:20:41PM +0100, Andreas Färber wrote:
> Am 14.12.2012 17:52, schrieb 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, I literally meant X86CPU rather than CPUX86State (i.e., the place
> within the instance).

OK. That makes sense. I believe that moving fields to X86CPU would be
much easier if we change the code that will actually use those fields to
use the QOM CPU class.

> 
> >> 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.
> 
> I mean target-i386/[misc_]helper.c and thus TCG, IIUC. :)

Oh, right. I wonder how much performance impact it would have, if people
are already using TCG.

Anyway, would this really have any impact at all? I mean:
ENV_GET_CPU(env) is basically subtracing an constant offset from 'env'.
So I expect similar code to be generated, just using a different offset
from 'env' to get the cpuid_features field.


> 
> > 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.
> 
> The alternative line of thought is whether to group KVM stuff together.
> Tying it into an array makes that harder. But personally I'm not opposed
> to this array proposal.

I don't think we would gain much from grouping KVM stuff together. It
would just force us to add KVM special-cases to code that deal with
feature bits. KVM feature bits are CPUID bits that work like all others.

-- 
Eduardo