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

[Paolo Bonzini <pbonzini@redhat.com>]

----- Original Message -----
> From: "Robert Hoo" <robert.hu@linux.intel.com>
> To: "Eduardo Habkost" <ehabkost@redhat.com>
> Cc: "robert hu" <robert.hu@intel.com>, "robert hu" <robert.hu@linux.intel.com>, pbonzini@redhat.com, rth@twiddle.net,
> "thomas lendacky" <thomas.lendacky@amd.com>, qemu-devel@nongnu.org, "jingqi liu" <jingqi.liu@intel.com>
> Sent: Saturday, August 18, 2018 7:48:43 AM
> Subject: Re: [Qemu-devel] [PATCH v3 1/3] x86: Data structure changes to support MSR based features
> 
> 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.

Yes, each patch should individually build and pass.  That's why you should
first introduce the changes to CPUID feature words (data structure and
functions) and then add support for MSR features.

Paolo