[v3,1/3] x86: Data structure changes to support MSR based features

Message ID 1533909989-56115-2-git-send-email-robert.hu@linux.intel.com
State New
Headers show
Series
  • x86: QEMU side support on MSR based features
Related show

Commit Message

Robert Hoo Aug. 10, 2018, 2:06 p.m.
Define FeatureWordType.
Expand FeatureWordInfo to support both CPUID type feature word as well as
MSR type's.
Change feature_word_info[] accordingly.

Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
---
 target/i386/cpu.c | 133 ++++++++++++++++++++++++++++++++++++++----------------
 target/i386/cpu.h |   5 ++
 2 files changed, 99 insertions(+), 39 deletions(-)

Comments

Eduardo Habkost Aug. 17, 2018, 3:10 a.m. | #1
Hi,

Thanks for the patch and sorry for taking so long to review.
Comments below:

On Fri, Aug 10, 2018 at 10:06:27PM +0800, Robert Hoo wrote:
> Define FeatureWordType.
> Expand FeatureWordInfo to support both CPUID type feature word as well as
> MSR type's.
> Change feature_word_info[] accordingly.
> 
> Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> ---
>  target/i386/cpu.c | 133 ++++++++++++++++++++++++++++++++++++++----------------
>  target/i386/cpu.h |   5 ++
>  2 files changed, 99 insertions(+), 39 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index ba7abe5..f7c70d9 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -770,17 +770,36 @@ static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
>            /* missing:
>            CPUID_XSAVE_XSAVEC, CPUID_XSAVE_XSAVES */
>  
> +typedef enum FeatureWordType {
> +   CPUID_FEATURE_WORD,
> +   MSR_FEATURE_WORD,
> +} FeatureWordType;
> +
>  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.


[...]
> +    /*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.

>  };
>  
>  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
> 
>
Paolo Bonzini Aug. 17, 2018, 3:50 p.m. | #2
On 10/08/2018 16:06, Robert Hoo wrote:
> +typedef enum FeatureWordType {
> +   CPUID_FEATURE_WORD,
> +   MSR_FEATURE_WORD,
> +} FeatureWordType;

This enum probably should be defined with QAPI, so that it can be reused
in the feature-words property:

# @X86CPUFeatureWordType:
#
# Kinds of X86 CPU feature words
#
# @cpuid: A CPUID leaf
#
# @msr: An MSR
##
{ 'enum': 'X86CPUFeatureWordType',
  'data': 'cpuid', 'msr' }

The names will be X86_CPU_FEATURE_WORD_TYPE_CPUID and
X86_CPU_FEATURE_WORD_TYPE_MSR.

Thanks,

Paolo
Eduardo Habkost Aug. 17, 2018, 3:55 p.m. | #3
On Fri, Aug 17, 2018 at 05:50:15PM +0200, Paolo Bonzini wrote:
> On 10/08/2018 16:06, Robert Hoo wrote:
> > +typedef enum FeatureWordType {
> > +   CPUID_FEATURE_WORD,
> > +   MSR_FEATURE_WORD,
> > +} FeatureWordType;
> 
> This enum probably should be defined with QAPI, so that it can be reused
> in the feature-words property:
> 
> # @X86CPUFeatureWordType:
> #
> # Kinds of X86 CPU feature words
> #
> # @cpuid: A CPUID leaf
> #
> # @msr: An MSR
> ##
> { 'enum': 'X86CPUFeatureWordType',
>   'data': 'cpuid', 'msr' }
> 
> The names will be X86_CPU_FEATURE_WORD_TYPE_CPUID and
> X86_CPU_FEATURE_WORD_TYPE_MSR.

I wouldn't like to make this an external API unless really
necessary.  I would rather deprecate the "feature-words" property
because nobody ended up using it.
Paolo Bonzini Aug. 17, 2018, 5:34 p.m. | #4
On 17/08/2018 17:55, Eduardo Habkost wrote:
>> The names will be X86_CPU_FEATURE_WORD_TYPE_CPUID and
>> X86_CPU_FEATURE_WORD_TYPE_MSR.
> I wouldn't like to make this an external API unless really
> necessary.  I would rather deprecate the "feature-words" property
> because nobody ended up using it.

I think we should either remove it directly, or extend it to support MSR
features.  Deprecating and only supporting CPUID features is the worst
of both worlds...

Paolo
Eduardo Habkost Aug. 17, 2018, 5:48 p.m. | #5
On Fri, Aug 17, 2018 at 07:34:13PM +0200, Paolo Bonzini wrote:
> On 17/08/2018 17:55, Eduardo Habkost wrote:
> >> The names will be X86_CPU_FEATURE_WORD_TYPE_CPUID and
> >> X86_CPU_FEATURE_WORD_TYPE_MSR.
> > I wouldn't like to make this an external API unless really
> > necessary.  I would rather deprecate the "feature-words" property
> > because nobody ended up using it.
> 
> I think we should either remove it directly, or extend it to support MSR
> features.  Deprecating and only supporting CPUID features is the worst
> of both worlds...

So let's do what's necessary to remove it.  But I don't think the
removal of "feature-words" should block the inclusion of this
series.

Now, should QOM properties follow our feature deprecation policy,
or they were never a supported external API and we can remove it
immediately?

CCing Jiri and libvir-list, because I just found that there's
code on libvirt that uses it, but I don't know exactly it does
with that info.
Paolo Bonzini Aug. 17, 2018, 5:59 p.m. | #6
On 17/08/2018 19:48, Eduardo Habkost wrote:
> So let's do what's necessary to remove it.  But I don't think the
> removal of "feature-words" should block the inclusion of this
> series.
> 
> Now, should QOM properties follow our feature deprecation policy,
> or they were never a supported external API and we can remove it
> immediately?
> 
> CCing Jiri and libvir-list, because I just found that there's
> code on libvirt that uses it, but I don't know exactly it does
> with that info.

It is used to check whether the host supports invtsc and pv-unhalt and
avoid changing the guest ABI when migrating: see
https://marc.info/?l=libvir-list&m=152445761414746&w=2 for a patch that
introduces one user.

I think we should extend it to MSRs, not remove it.

Paolo
Eduardo Habkost Aug. 17, 2018, 6:10 p.m. | #7
On Fri, Aug 17, 2018 at 07:59:40PM +0200, Paolo Bonzini wrote:
> On 17/08/2018 19:48, Eduardo Habkost wrote:
> > So let's do what's necessary to remove it.  But I don't think the
> > removal of "feature-words" should block the inclusion of this
> > series.
> > 
> > Now, should QOM properties follow our feature deprecation policy,
> > or they were never a supported external API and we can remove it
> > immediately?
> > 
> > CCing Jiri and libvir-list, because I just found that there's
> > code on libvirt that uses it, but I don't know exactly it does
> > with that info.
> 
> It is used to check whether the host supports invtsc and pv-unhalt and
> avoid changing the guest ABI when migrating: see
> https://marc.info/?l=libvir-list&m=152445761414746&w=2 for a patch that
> introduces one user.
> 
> I think we should extend it to MSRs, not remove it.

Well, I would prefer if libvirt simply did (e.g.)
"qom-get property=invtsc" instead of fetching raw feature-words
data.  But if we do have an existing user, I now agree with you:
let's keep it working and extend it to include MSR info too.

BTW, libvirt must stop using this hardcoded QOM path:

  #define QOM_CPU_PATH  "/machine/unattached/device[0]"

It should use the "qom_path" field of "query-cpus" instead.
Robert Hoo Aug. 18, 2018, 3:10 a.m. | #8
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?
> 
> [...]
> > +    /*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
> > 
> > 
>
Robert Hoo Aug. 18, 2018, 5:48 a.m. | #9
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
> > > 
> > > 
> > 
> 
>
Paolo Bonzini Aug. 18, 2018, 7:50 a.m. | #10
----- 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

Patch

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index ba7abe5..f7c70d9 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -770,17 +770,36 @@  static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
           /* missing:
           CPUID_XSAVE_XSAVEC, CPUID_XSAVE_XSAVES */
 
+typedef enum FeatureWordType {
+   CPUID_FEATURE_WORD,
+   MSR_FEATURE_WORD,
+} FeatureWordType;
+
 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 */
@@ -790,6 +809,7 @@  typedef struct FeatureWordInfo {
 
 static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
     [FEAT_1_EDX] = {
+        .type = CPUID_FEATURE_WORD,
         .feat_names = {
             "fpu", "vme", "de", "pse",
             "tsc", "msr", "pae", "mce",
@@ -800,10 +820,11 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             "fxsr", "sse", "sse2", "ss",
             "ht" /* Intel htt */, "tm", "ia64", "pbe",
         },
-        .cpuid_eax = 1, .cpuid_reg = R_EDX,
+        .cpuid = {.eax = 1, .reg = R_EDX, },
         .tcg_features = TCG_FEATURES,
     },
     [FEAT_1_ECX] = {
+        .type = CPUID_FEATURE_WORD,
         .feat_names = {
             "pni" /* Intel,AMD sse3 */, "pclmulqdq", "dtes64", "monitor",
             "ds-cpl", "vmx", "smx", "est",
@@ -814,7 +835,7 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             "tsc-deadline", "aes", "xsave", NULL /* osxsave */,
             "avx", "f16c", "rdrand", "hypervisor",
         },
-        .cpuid_eax = 1, .cpuid_reg = R_ECX,
+        .cpuid = { .eax = 1, .reg = R_ECX, },
         .tcg_features = TCG_EXT_FEATURES,
     },
     /* Feature names that are already defined on feature_name[] but
@@ -823,6 +844,7 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
      * to features[FEAT_8000_0001_EDX] if and only if CPU vendor is AMD.
      */
     [FEAT_8000_0001_EDX] = {
+        .type = CPUID_FEATURE_WORD,
         .feat_names = {
             NULL /* fpu */, NULL /* vme */, NULL /* de */, NULL /* pse */,
             NULL /* tsc */, NULL /* msr */, NULL /* pae */, NULL /* mce */,
@@ -833,10 +855,11 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             NULL /* fxsr */, "fxsr-opt", "pdpe1gb", "rdtscp",
             NULL, "lm", "3dnowext", "3dnow",
         },
-        .cpuid_eax = 0x80000001, .cpuid_reg = R_EDX,
+        .cpuid = { .eax = 0x80000001, .reg = R_EDX, },
         .tcg_features = TCG_EXT2_FEATURES,
     },
     [FEAT_8000_0001_ECX] = {
+        .type = CPUID_FEATURE_WORD,
         .feat_names = {
             "lahf-lm", "cmp-legacy", "svm", "extapic",
             "cr8legacy", "abm", "sse4a", "misalignsse",
@@ -847,10 +870,11 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             "perfctr-nb", NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
         },
-        .cpuid_eax = 0x80000001, .cpuid_reg = R_ECX,
+        .cpuid = { .eax = 0x80000001, .reg = R_ECX, },
         .tcg_features = TCG_EXT3_FEATURES,
     },
     [FEAT_C000_0001_EDX] = {
+        .type = CPUID_FEATURE_WORD,
         .feat_names = {
             NULL, NULL, "xstore", "xstore-en",
             NULL, NULL, "xcrypt", "xcrypt-en",
@@ -861,10 +885,11 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
         },
-        .cpuid_eax = 0xC0000001, .cpuid_reg = R_EDX,
+        .cpuid = { .eax = 0xC0000001, .reg = R_EDX, },
         .tcg_features = TCG_EXT4_FEATURES,
     },
     [FEAT_KVM] = {
+        .type = CPUID_FEATURE_WORD,
         .feat_names = {
             "kvmclock", "kvm-nopiodelay", "kvm-mmu", "kvmclock",
             "kvm-asyncpf", "kvm-steal-time", "kvm-pv-eoi", "kvm-pv-unhalt",
@@ -875,10 +900,11 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             "kvmclock-stable-bit", NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
         },
-        .cpuid_eax = KVM_CPUID_FEATURES, .cpuid_reg = R_EAX,
+        .cpuid = { .eax = KVM_CPUID_FEATURES, .reg = R_EAX, },
         .tcg_features = TCG_KVM_FEATURES,
     },
     [FEAT_KVM_HINTS] = {
+        .type = CPUID_FEATURE_WORD,
         .feat_names = {
             "kvm-hint-dedicated", NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
@@ -889,7 +915,7 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
         },
-        .cpuid_eax = KVM_CPUID_FEATURES, .cpuid_reg = R_EDX,
+        .cpuid = { .eax = KVM_CPUID_FEATURES, .reg = R_EDX, },
         .tcg_features = TCG_KVM_FEATURES,
         /*
          * KVM hints aren't auto-enabled by -cpu host, they need to be
@@ -898,6 +924,7 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
         .no_autoenable_flags = ~0U,
     },
     [FEAT_HYPERV_EAX] = {
+        .type = CPUID_FEATURE_WORD,
         .feat_names = {
             NULL /* hv_msr_vp_runtime_access */, NULL /* hv_msr_time_refcount_access */,
             NULL /* hv_msr_synic_access */, NULL /* hv_msr_stimer_access */,
@@ -912,9 +939,10 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
         },
-        .cpuid_eax = 0x40000003, .cpuid_reg = R_EAX,
+        .cpuid = { .eax = 0x40000003, .reg = R_EAX, },
     },
     [FEAT_HYPERV_EBX] = {
+        .type = CPUID_FEATURE_WORD,
         .feat_names = {
             NULL /* hv_create_partitions */, NULL /* hv_access_partition_id */,
             NULL /* hv_access_memory_pool */, NULL /* hv_adjust_message_buffers */,
@@ -928,9 +956,10 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
         },
-        .cpuid_eax = 0x40000003, .cpuid_reg = R_EBX,
+        .cpuid = { .eax = 0x40000003, .reg = R_EBX, },
     },
     [FEAT_HYPERV_EDX] = {
+        .type = CPUID_FEATURE_WORD,
         .feat_names = {
             NULL /* hv_mwait */, NULL /* hv_guest_debugging */,
             NULL /* hv_perf_monitor */, NULL /* hv_cpu_dynamic_part */,
@@ -943,9 +972,10 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
         },
-        .cpuid_eax = 0x40000003, .cpuid_reg = R_EDX,
+        .cpuid = { .eax = 0x40000003, .reg = R_EDX, },
     },
     [FEAT_SVM] = {
+        .type = CPUID_FEATURE_WORD,
         .feat_names = {
             "npt", "lbrv", "svm-lock", "nrip-save",
             "tsc-scale", "vmcb-clean",  "flushbyasid", "decodeassists",
@@ -956,10 +986,11 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
         },
-        .cpuid_eax = 0x8000000A, .cpuid_reg = R_EDX,
+        .cpuid = { .eax = 0x8000000A, .reg = R_EDX, },
         .tcg_features = TCG_SVM_FEATURES,
     },
     [FEAT_7_0_EBX] = {
+        .type = CPUID_FEATURE_WORD,
         .feat_names = {
             "fsgsbase", "tsc-adjust", NULL, "bmi1",
             "hle", "avx2", NULL, "smep",
@@ -970,12 +1001,13 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             "clwb", "intel-pt", "avx512pf", "avx512er",
             "avx512cd", "sha-ni", "avx512bw", "avx512vl",
         },
-        .cpuid_eax = 7,
-        .cpuid_needs_ecx = true, .cpuid_ecx = 0,
-        .cpuid_reg = R_EBX,
+        .cpuid = { .eax = 7,
+            .needs_ecx = true, .ecx = 0,
+            .reg = R_EBX, },
         .tcg_features = TCG_7_0_EBX_FEATURES,
     },
     [FEAT_7_0_ECX] = {
+        .type = CPUID_FEATURE_WORD,
         .feat_names = {
             NULL, "avx512vbmi", "umip", "pku",
             NULL /* ospke */, NULL, "avx512vbmi2", NULL,
@@ -986,12 +1018,13 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             NULL, "cldemote", NULL, NULL,
             NULL, NULL, NULL, NULL,
         },
-        .cpuid_eax = 7,
-        .cpuid_needs_ecx = true, .cpuid_ecx = 0,
-        .cpuid_reg = R_ECX,
+        .cpuid = { .eax = 7,
+            .needs_ecx = true, .ecx = 0,
+            .reg = R_ECX, },
         .tcg_features = TCG_7_0_ECX_FEATURES,
     },
     [FEAT_7_0_EDX] = {
+        .type = CPUID_FEATURE_WORD,
         .feat_names = {
             NULL, NULL, "avx512-4vnniw", "avx512-4fmaps",
             NULL, NULL, NULL, NULL,
@@ -1002,13 +1035,14 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             NULL, NULL, "spec-ctrl", NULL,
             NULL, "arch-capabilities", NULL, "ssbd",
         },
-        .cpuid_eax = 7,
-        .cpuid_needs_ecx = true, .cpuid_ecx = 0,
-        .cpuid_reg = R_EDX,
+        .cpuid = { .eax = 7,
+            .needs_ecx = true, .ecx = 0,
+            .reg = R_EDX, },
         .tcg_features = TCG_7_0_EDX_FEATURES,
         .unmigratable_flags = CPUID_7_0_EDX_ARCH_CAPABILITIES,
     },
     [FEAT_8000_0007_EDX] = {
+        .type = CPUID_FEATURE_WORD,
         .feat_names = {
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
@@ -1019,12 +1053,12 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
         },
-        .cpuid_eax = 0x80000007,
-        .cpuid_reg = R_EDX,
+        .cpuid = { .eax = 0x80000007, .reg = R_EDX, },
         .tcg_features = TCG_APM_FEATURES,
         .unmigratable_flags = CPUID_APM_INVTSC,
     },
     [FEAT_8000_0008_EBX] = {
+        .type = CPUID_FEATURE_WORD,
         .feat_names = {
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
@@ -1035,12 +1069,12 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             "amd-ssbd", "virt-ssbd", "amd-no-ssb", NULL,
             NULL, NULL, NULL, NULL,
         },
-        .cpuid_eax = 0x80000008,
-        .cpuid_reg = R_EBX,
+        .cpuid = { .eax = 0x80000008, .reg = R_EBX, },
         .tcg_features = 0,
         .unmigratable_flags = 0,
     },
     [FEAT_XSAVE] = {
+        .type = CPUID_FEATURE_WORD,
         .feat_names = {
             "xsaveopt", "xsavec", "xgetbv1", "xsaves",
             NULL, NULL, NULL, NULL,
@@ -1051,12 +1085,13 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
         },
-        .cpuid_eax = 0xd,
-        .cpuid_needs_ecx = true, .cpuid_ecx = 1,
-        .cpuid_reg = R_EAX,
+        .cpuid = { .eax = 0xd,
+            .needs_ecx = true, .ecx = 1,
+            .reg = R_EAX, },
         .tcg_features = TCG_XSAVE_FEATURES,
     },
     [FEAT_6_EAX] = {
+        .type = CPUID_FEATURE_WORD,
         .feat_names = {
             NULL, NULL, "arat", NULL,
             NULL, NULL, NULL, NULL,
@@ -1067,13 +1102,14 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
         },
-        .cpuid_eax = 6, .cpuid_reg = R_EAX,
+        .cpuid = { .eax = 6, .reg = R_EAX, },
         .tcg_features = TCG_6_EAX_FEATURES,
     },
     [FEAT_XSAVE_COMP_LO] = {
-        .cpuid_eax = 0xD,
-        .cpuid_needs_ecx = true, .cpuid_ecx = 0,
-        .cpuid_reg = R_EAX,
+        .type = CPUID_FEATURE_WORD,
+        .cpuid = { .eax = 0xD,
+            .needs_ecx = true, .ecx = 0,
+            .reg = R_EAX, },
         .tcg_features = ~0U,
         .migratable_flags = XSTATE_FP_MASK | XSTATE_SSE_MASK |
             XSTATE_YMM_MASK | XSTATE_BNDREGS_MASK | XSTATE_BNDCSR_MASK |
@@ -1081,11 +1117,30 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             XSTATE_PKRU_MASK,
     },
     [FEAT_XSAVE_COMP_HI] = {
-        .cpuid_eax = 0xD,
-        .cpuid_needs_ecx = true, .cpuid_ecx = 0,
-        .cpuid_reg = R_EDX,
+        .type = CPUID_FEATURE_WORD,
+        .cpuid = { .eax = 0xD,
+            .needs_ecx = true, .ecx = 0,
+            .reg = R_EDX, },
         .tcg_features = ~0U,
     },
+    /*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 }
+                },
+    },
 };
 
 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 */