| Message ID | 1533909989-56115-2-git-send-email-robert.hu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: QEMU side support on MSR based features | expand |
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 > >
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
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.
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
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.
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
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.
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 > > > > >
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 > > > > > > > > > >
----- 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
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 */
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(-)