[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/3] target-i386: replace cpuid_*features fields
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH 3/3] target-i386: replace cpuid_*features fields with a feature word array |
Date: |
Fri, 14 Dec 2012 15:36:22 -0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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
- [Qemu-devel] [PATCH 3/3] target-i386: replace cpuid_*features fields with a feature word array, (continued)
Re: [Qemu-devel] [PATCH 3/3] target-i386: replace cpuid_*features fields with a feature word array, Andreas Färber, 2012/12/14