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

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 16 Apr 2013 13:12:04 -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)
> 
> The following field replacements were made on X86CPU and x86_def_t:
> 
>   (cpuid_)features         -> features[FEAT_1_EDX]
>   (cpuid_)ext_features     -> features[FEAT_1_ECX]
>   (cpuid_)ext2_features    -> features[FEAT_8000_0001_EDX]
>   (cpuid_)ext3_features    -> features[FEAT_8000_0001_ECX]
>   (cpuid_)ext4_features    -> features[FEAT_C000_0001_EDX]
>   (cpuid_)kvm_features     -> features[FEAT_KVM]
>   (cpuid_)svm_features     -> features[FEAT_SVM]
>   (cpuid_)7_0_ebx_features -> features[FEAT_7_0_EBX]
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> This should help us 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)
> 
> 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
> The script is a lot simpler now, it is basically 8 s/.../.../ commands
> and a few insert/delete commands to change the cpu.h and cpu.c structs.
> 
> Changes v9:
>  * commit message changes: move some text below "---", add table
>    showing how each field was replaced
> 
> Changes v8:
>  * Move line-breaking and code-formatting changes to separate patches
>  * Rebase on top of qom-cpu commit b4d31f73
>    (qdev: Set device's parent before calling realize() down inheritance chain)
> 
> Changes v7:
>  * Rebase on top qom-cpu-next
>    (commit 3755f0a9d48da07258f4a0ef5e883272799e47b9)
> 
> Changes v7:
>  * 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>
> 
> Conflicts:
> 	target-i386/kvm.c
> ---
>  bsd-user/elfload.c        |   2 +-
>  bsd-user/main.c           |   4 +-
>  hw/i386/kvm/clock.c       |   2 +-
>  linux-user/elfload.c      |   2 +-
>  linux-user/main.c         |   4 +-
>  target-i386/cpu.c         | 329 +++++++++++++++++++++++-----------------------
>  target-i386/cpu.h         |  11 +-
>  target-i386/helper.c      |   4 +-
>  target-i386/kvm.c         |   4 +-
>  target-i386/misc_helper.c |  14 +-
>  target-i386/translate.c   |  10 +-
>  11 files changed, 186 insertions(+), 200 deletions(-)
> 
> diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
> index a6cd3ab..44e1568 100644
> --- a/bsd-user/elfload.c
> +++ b/bsd-user/elfload.c
> @@ -110,7 +110,7 @@ static const char *get_elf_platform(void)
>  
>  static uint32_t get_elf_hwcap(void)
>  {
> -  return thread_env->cpuid_features;
> +  return thread_env->features[FEAT_1_EDX];
wrong ident, since line is touched it should be fixed.


>  }
>  
[...]


-- 
Regards,
  Igor