qemu-devel
[Top][All Lists]
Advanced

[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 16:40:42 +0200

On Mon, 15 Apr 2013 10:40:36 -0300
Eduardo Habkost <address@hidden> wrote:

> On Mon, Apr 15, 2013 at 10:50:20AM +0200, Igor Mammedov wrote:
> > 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.
> 
> I'll redo and resend. Thanks.
> 
> > > 
> > > 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.
> 
> I'll do it next time.
> 
> 
> > 
> > > 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.
> 
> Before applying this patch, xlevel was close to cpuid_ext[23]_features
> and xlevel2 was close to cpuid_ext4_features. Leaving them at completely
> random places in the struct after removing the cpuid_*_features field
> looks confusing to me.
then do it ask separate clean up patch.
and it would be nicer with one field per line, instead of ^^^.

> 
> At the same time, moving them without removing/moving the
> cpuid_*_features at the same time wouldn't make sense to me either. So I
> decided to move them in the same patch.
> 
> > 
> > > +    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.
> 
> struct DisasContext doesn't have all feature words (SVM, KVM, and
> C000_0001_EDX are not present), so it doesn't need the full feature
> array. I also don't know if making the struct larger would impact TCG
> performance, so I prefer to not touch that part of the TCG code myself.
agreed

> 
> > 
> > >  #ifdef TARGET_X86_64
> > >      dc->lma = (flags >> HF_LMA_SHIFT) & 1;
> > >      dc->code64 = (flags >> HF_CS64_SHIFT) & 1;
> > 
> 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]