Re: [Qemu-devel] [PATCH v3 1/3] x86: Data structure changes to support MSR based features

[Robert Hoo]

On Sat, 2018-08-18 at 11:10 +0800, Robert Hoo wrote:
> On Fri, 2018-08-17 at 00:10 -0300, Eduardo Habkost wrote:
> [trim...]
> > > +
> > >  typedef struct FeatureWordInfo {
> > > -    /* feature flags names are taken from "Intel Processor 
> > > Identification and
> > > +    FeatureWordType type;
> > > +   /* feature flags names are taken from "Intel Processor Identification 
> > > and
> > >       * the CPUID Instruction" and AMD's "CPUID Specification".
> > >       * In cases of disagreement between feature naming conventions,
> > >       * aliases may be added.
> > >       */
> > >      const char *feat_names[32];
> > > -    uint32_t cpuid_eax;   /* Input EAX for CPUID */
> > > -    bool cpuid_needs_ecx; /* CPUID instruction uses ECX as input */
> > > -    uint32_t cpuid_ecx;   /* Input ECX value for CPUID */
> > > -    int cpuid_reg;        /* output register (R_* constant) */
> > > +    union {
> > > +        /* If type==CPUID_FEATURE_WORD */
> > > +        struct {
> > > +            uint32_t eax;   /* Input EAX for CPUID */
> > > +            bool needs_ecx; /* CPUID instruction uses ECX as input */
> > > +            uint32_t ecx;   /* Input ECX value for CPUID */
> > > +            int reg;        /* output register (R_* constant) */
> > > +        } cpuid;
> > > +        /* If type==MSR_FEATURE_WORD */
> > > +        struct {
> > > +            uint32_t index;
> > > +            struct {   /*CPUID that enumerate this MSR*/
> > > +                FeatureWord cpuid_class;
> > > +                uint32_t    cpuid_flag;
> > > +            } cpuid_dep;
> > > +        } msr;
> > > +    };
> > >      uint32_t tcg_features; /* Feature flags supported by TCG */
> > >      uint32_t unmigratable_flags; /* Feature flags known to be 
> > > unmigratable */
> > >      uint32_t migratable_flags; /* Feature flags known to be migratable */
> > 
> > The data structure change looks good, but you are breaking the
> > build if you touch them without updating the existing code.  If
> > you break the build in your series, you break git-bisect.
> > 
> I had tested in my environment before send out, build has no even a
> warning. Is it because this patch is based on master + previous icelake
> cpu model set? I see you are pulling in previous icelake cpu model patch
> set. I will rebase this patch to then master. Will previous icelake cpu
> model patch set appear in master soon?

or you mean each single patch in a series should be individually built
pass? then it will a huge one here: data structure changes + reference
functions.
> > 
> > [...]
> > > +    /*Below are MSR exposed features*/
> > > +    [FEATURE_WORDS_ARCH_CAPABILITIES] = {
> > > +        .type = MSR_FEATURE_WORD,
> > > +        .feat_names = {
> > > +            "rdctl-no", "ibrs-all", "rsba", NULL,
> > > +            "ssb-no", NULL, NULL, NULL,
> > > +            NULL, NULL, NULL, NULL,
> > > +            NULL, NULL, NULL, NULL,
> > > +            NULL, NULL, NULL, NULL,
> > > +            NULL, NULL, NULL, NULL,
> > > +            NULL, NULL, NULL, NULL,
> > > +            NULL, NULL, NULL, NULL,
> > > +        },
> > > +        .msr = { .index = MSR_IA32_ARCH_CAPABILITIES,
> > > +                .cpuid_dep = { FEAT_7_0_EDX,
> > > +                    CPUID_7_0_EDX_ARCH_CAPABILITIES }
> > > +                },
> > > +    },
> > 
> > I suggest moving this hunk to a separate patch: first we refactor
> > the existing code without changing behavior or adding new
> > features, then we add the new features.
> > 
> Sure.
> 
> > >  };
> > >  
> > >  typedef struct X86RegisterInfo32 {
> > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > > index cddf9d9..9e8879e 100644
> > > --- a/target/i386/cpu.h
> > > +++ b/target/i386/cpu.h
> > > @@ -502,9 +502,14 @@ typedef enum FeatureWord {
> > >      FEAT_6_EAX,         /* CPUID[6].EAX */
> > >      FEAT_XSAVE_COMP_LO, /* CPUID[EAX=0xd,ECX=0].EAX */
> > >      FEAT_XSAVE_COMP_HI, /* CPUID[EAX=0xd,ECX=0].EDX */
> > > +    FEATURE_WORDS_NUM_CPUID,
> > > +    FEATURE_WORDS_FIRST_MSR = FEATURE_WORDS_NUM_CPUID,
> > > +    FEATURE_WORDS_ARCH_CAPABILITIES = FEATURE_WORDS_FIRST_MSR,
> > >      FEATURE_WORDS,
> > >  } FeatureWord;
> > >  
> > > +#define FEATURE_WORDS_NUM_MSRS (FEATURE_WORDS - FEATURE_WORDS_FIRST_MSR)
> > > +
> > >  typedef uint32_t FeatureWordArray[FEATURE_WORDS];
> > >  
> > >  /* cpuid_features bits */
> > > -- 
> > > 1.8.3.1
> > > 
> > > 
> > 
> 
>