Re: [Qemu-devel] [PATCH 3/3] target-i386: replace cpuid_*features fields with a feature word array

["Andreas Färber" <afaerber@suse.de>]

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 <ehabkost@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).

>> 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. :)

> 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.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg