Re: [Qemu-devel] [PATCH qom-cpu-next 2/2] target-i386: Replace cpuid_*features fields with a feature word array

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 11 Apr 2013 17:07:24 -0300
Eduardo Habkost <ehabkost@redhat.com> 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 <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
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 <ehabkost@redhat.com>
> ---
[...]
> 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;