[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH qom-cpu-next 2/2] target-i386: Replace cpuid_*fe
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH qom-cpu-next 2/2] target-i386: Replace cpuid_*features fields with a feature word array |
Date: |
Mon, 15 Apr 2013 10:50:20 +0200 |
On Thu, 11 Apr 2013 17:07:24 -0300
Eduardo Habkost <address@hidden> wrote:
> 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(), CPU feature-bit
> property lookup/registration, and the proposed "feature-words" property)
>
> This should also help avoid bugs like the ones introduced when we added
> cpuid_7_0_ebx_features. Today, adding a new feature word to the code
> requires chaning 5 or 6 different places in the code, and it's very easy
> to miss a problem when we forget to update one of those parts. See, for
> example:
>
> * The bug solved by commit ffa8c11f0bbf47e1b7a3a62f97bc1da591c6734a;
> (CPUID 7 leaf was not being filtered based on host capabilities)
> * The bug solved by commit 07ca59450c9a0c5df65665ce46aa8487af59a1dd
> (check/enforce flags were not checking all feature flags)
>
> 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
It doesn't apply anymore.
>
> Changes v7:
> - Rebase on top qom-cpu-next
> (commit 3755f0a9d48da07258f4a0ef5e883272799e47b9)
commit IDs are often useless on staging tree, since they are changing,
pls use commit's subj.
> Changes v6:
> - Rebase on top of Andreas' qom-cpu tree (commit
> 9260944307077b93a66bf861a467107af986fe47)
> - Break lines on kvm_check_features_against_host()
> - Break the lines on builtin_x86_defs just after the "=".
> This way the feature lists stay on separate lines, this patch gets
> easier to review, and future patches that touches the code around
> builtin_x86_defs will be even easier to review (as they won't need
> to touch the lines containing the fature lists again)
>
> Signed-off-by: Eduardo Habkost <address@hidden>
> ---
[...]
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index c2e02fe..e506d12 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -348,21 +348,14 @@ static void add_flagname_to_bitmaps(const char
> *flagname,
> typedef struct x86_def_t {
> const char *name;
> - uint32_t level;
> + uint32_t level, xlevel, xlevel2;
it's not cpuid_*features that patch re-factors, pls move to separate patch
if this movement necessary at all. I don't see any gain in touch it.
> + FeatureWordArray features;
> /* vendor is zero-terminated, 12 character ASCII string */
> char vendor[CPUID_VENDOR_SZ + 1];
[...]
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 2b4e319..299a793 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -834,23 +834,14 @@ typedef struct CPUX86State {
> uint64_t pat;
>
> /* processor features (e.g. for CPUID insn) */
> - uint32_t cpuid_level;
> + uint32_t cpuid_level, cpuid_xlevel, cpuid_xlevel2;ditto
ditto
> + FeatureWordArray features;
> uint32_t cpuid_vendor1;
> uint32_t cpuid_vendor2;
> uint32_t cpuid_vendor3;
> uint32_t cpuid_version;
> - uint32_t cpuid_features;
> - uint32_t cpuid_ext_features;
> - uint32_t cpuid_xlevel;
> uint32_t cpuid_model[12];
> - uint32_t cpuid_ext2_features;
> - uint32_t cpuid_ext3_features;
> uint32_t cpuid_apic_id;
> - /* Store the results of Centaur's CPUID instructions */
> - uint32_t cpuid_xlevel2;
> - uint32_t cpuid_ext4_features;
> - /* Flags from CPUID[EAX=7,ECX=0].EBX */
> - uint32_t cpuid_7_0_ebx_features;
>
> /* MTRRs */
> uint64_t mtrr_fixed[11];
> @@ -864,8 +855,6 @@ typedef struct CPUX86State {
> uint8_t soft_interrupt;
> uint8_t has_error_code;
> uint32_t sipi_vector;
> - uint32_t cpuid_kvm_features;
> - uint32_t cpuid_svm_features;
> bool tsc_valid;
> int tsc_khz;
> void *kvm_xsave_buf;
[...]
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index 7596a90..d340aab 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -8279,11 +8279,11 @@ static inline void
> gen_intermediate_code_internal(CPUX86State *env, if (flags &
> HF_SOFTMMU_MASK) { dc->mem_index = (cpu_mmu_index(env) + 1) << 2;
> }
> - dc->cpuid_features = env->cpuid_features;
> - dc->cpuid_ext_features = env->cpuid_ext_features;
> - dc->cpuid_ext2_features = env->cpuid_ext2_features;
> - dc->cpuid_ext3_features = env->cpuid_ext3_features;
> - dc->cpuid_7_0_ebx_features = env->cpuid_7_0_ebx_features;
> + dc->cpuid_features = env->features[FEAT_1_EDX];
> + dc->cpuid_ext_features = env->features[FEAT_1_ECX];
> + dc->cpuid_ext2_features = env->features[FEAT_8000_0001_EDX];
> + dc->cpuid_ext3_features = env->features[FEAT_8000_0001_ECX];
> + dc->cpuid_7_0_ebx_features = env->features[FEAT_7_0_EBX];
why leaving cpuid_*features here, it's not much extra code on the first
glance, so a consistent re-factoring would be justified.
> #ifdef TARGET_X86_64
> dc->lma = (flags >> HF_LMA_SHIFT) & 1;
> dc->code64 = (flags >> HF_CS64_SHIFT) & 1;