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