target/i386/cpu.c | 29 ++++++++++++++++++++++++++++- target/i386/cpu.h | 3 +++ target/i386/kvm.c | 3 ++- 3 files changed, 33 insertions(+), 2 deletions(-)
Intel CooperLake cpu adds AVX512_BF16 instruction, defining as
CPUID.(EAX=7,ECX=1):EAX[bit 05].
The release spec link as follows,
https://software.intel.com/sites/default/files/managed/c5/15/\
architecture-instruction-set-extensions-programming-reference.pdf
Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>
---
target/i386/cpu.c | 29 ++++++++++++++++++++++++++++-
target/i386/cpu.h | 3 +++
target/i386/kvm.c | 3 ++-
3 files changed, 33 insertions(+), 2 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index c1ab86d..de3daf5 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -767,6 +767,7 @@ static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
/* CPUID_7_0_ECX_OSPKE is dynamic */ \
CPUID_7_0_ECX_LA57)
#define TCG_7_0_EDX_FEATURES 0
+#define TCG_7_1_EAX_FEATURES 0
#define TCG_APM_FEATURES 0
#define TCG_6_EAX_FEATURES CPUID_6_EAX_ARAT
#define TCG_XSAVE_FEATURES (CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XGETBV1)
@@ -1092,6 +1093,25 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
},
.tcg_features = TCG_7_0_EDX_FEATURES,
},
+ [FEAT_7_1_EAX] = {
+ .type = CPUID_FEATURE_WORD,
+ .feat_names = {
+ NULL, NULL, NULL, NULL,
+ NULL, "avx512-bf16", 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,
+ },
+ .cpuid = {
+ .eax = 7,
+ .needs_ecx = true, .ecx = 1,
+ .reg = R_EAX,
+ },
+ .tcg_features = TCG_7_1_EAX_FEATURES,
+ },
[FEAT_8000_0007_EDX] = {
.type = CPUID_FEATURE_WORD,
.feat_names = {
@@ -4344,13 +4364,20 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
case 7:
/* Structured Extended Feature Flags Enumeration Leaf */
if (count == 0) {
- *eax = 0; /* Maximum ECX value for sub-leaves */
+ /* Maximum ECX value for sub-leaves */
+ *eax = kvm_arch_get_supported_cpuid(cs->kvm_state, 0x7,
+ count, R_EAX);
*ebx = env->features[FEAT_7_0_EBX]; /* Feature flags */
*ecx = env->features[FEAT_7_0_ECX]; /* Feature flags */
if ((*ecx & CPUID_7_0_ECX_PKU) && env->cr[4] & CR4_PKE_MASK) {
*ecx |= CPUID_7_0_ECX_OSPKE;
}
*edx = env->features[FEAT_7_0_EDX]; /* Feature flags */
+ } else if (count == 1) {
+ *eax = env->features[FEAT_7_1_EAX];
+ *ebx = 0;
+ *ecx = 0;
+ *edx = 0;
} else {
*eax = 0;
*ebx = 0;
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index bd06523..40594a1 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -488,6 +488,7 @@ typedef enum FeatureWord {
FEAT_7_0_EBX, /* CPUID[EAX=7,ECX=0].EBX */
FEAT_7_0_ECX, /* CPUID[EAX=7,ECX=0].ECX */
FEAT_7_0_EDX, /* CPUID[EAX=7,ECX=0].EDX */
+ FEAT_7_1_EAX, /* CPUID[EAX=7,ECX=1].EAX */
FEAT_8000_0001_EDX, /* CPUID[8000_0001].EDX */
FEAT_8000_0001_ECX, /* CPUID[8000_0001].ECX */
FEAT_8000_0007_EDX, /* CPUID[8000_0007].EDX */
@@ -699,6 +700,8 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
#define CPUID_7_0_EDX_ARCH_CAPABILITIES (1U << 29) /*Arch Capabilities*/
#define CPUID_7_0_EDX_SPEC_CTRL_SSBD (1U << 31) /* Speculative Store Bypass Disable */
+#define CPUID_7_1_EAX_AVX512_BF16 (1U << 5) /* AVX512 BFloat16 Instruction */
+
#define CPUID_8000_0008_EBX_WBNOINVD (1U << 9) /* Write back and
do not invalidate cache */
#define CPUID_8000_0008_EBX_IBPB (1U << 12) /* Indirect Branch Prediction Barrier */
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 3b29ce5..977aaa5 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -1110,6 +1110,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
c = &cpuid_data.entries[cpuid_i++];
}
break;
+ case 0x7:
case 0x14: {
uint32_t times;
@@ -1122,7 +1123,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
for (j = 1; j <= times; ++j) {
if (cpuid_i == KVM_MAX_CPUID_ENTRIES) {
fprintf(stderr, "cpuid_data is full, no space for "
- "cpuid(eax:0x14,ecx:0x%x)\n", j);
+ "cpuid(eax:0x%x,ecx:0x%x)\n", i, j);
abort();
}
c = &cpuid_data.entries[cpuid_i++];
--
1.8.3.1
On 7/11/2019 1:38 PM, Jing Liu wrote:
> Intel CooperLake cpu adds AVX512_BF16 instruction, defining as
> CPUID.(EAX=7,ECX=1):EAX[bit 05].
>
> The release spec link as follows,
> https://software.intel.com/sites/default/files/managed/c5/15/\
> architecture-instruction-set-extensions-programming-reference.pdf
>
> Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>
> ---
> target/i386/cpu.c | 29 ++++++++++++++++++++++++++++-
> target/i386/cpu.h | 3 +++
> target/i386/kvm.c | 3 ++-
> 3 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index c1ab86d..de3daf5 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -767,6 +767,7 @@ static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
> /* CPUID_7_0_ECX_OSPKE is dynamic */ \
> CPUID_7_0_ECX_LA57)
> #define TCG_7_0_EDX_FEATURES 0
> +#define TCG_7_1_EAX_FEATURES 0
> #define TCG_APM_FEATURES 0
> #define TCG_6_EAX_FEATURES CPUID_6_EAX_ARAT
> #define TCG_XSAVE_FEATURES (CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XGETBV1)
> @@ -1092,6 +1093,25 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
> },
> .tcg_features = TCG_7_0_EDX_FEATURES,
> },
> + [FEAT_7_1_EAX] = {
> + .type = CPUID_FEATURE_WORD,
> + .feat_names = {
> + NULL, NULL, NULL, NULL,
> + NULL, "avx512-bf16", 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,
> + },
> + .cpuid = {
> + .eax = 7,
> + .needs_ecx = true, .ecx = 1,
> + .reg = R_EAX,
> + },
> + .tcg_features = TCG_7_1_EAX_FEATURES,
> + },
> [FEAT_8000_0007_EDX] = {
> .type = CPUID_FEATURE_WORD,
> .feat_names = {
> @@ -4344,13 +4364,20 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> case 7:
> /* Structured Extended Feature Flags Enumeration Leaf */
> if (count == 0) {
> - *eax = 0; /* Maximum ECX value for sub-leaves */
> + /* Maximum ECX value for sub-leaves */
> + *eax = kvm_arch_get_supported_cpuid(cs->kvm_state, 0x7,
> + count, R_EAX);
This needs to be firstly checked as follows, otherwise some
architectures would fail to compile.
What about hvf and tcg CPUID 07 EAX value?
+ /* Maximum ECX value for sub-leaves */
+ if (kvm_enabled()) {
+ *eax = kvm_arch_get_supported_cpuid(cs->kvm_state, 0x7,
+ count, R_EAX);
+ } else if (hvf_enabled()) {
+ *eax = hvf_get_supported_cpuid(0x7, count, R_EAX);
+ } else {
+ *eax = 0;
+ }
Thanks,
Jing
> *ebx = env->features[FEAT_7_0_EBX]; /* Feature flags */
> *ecx = env->features[FEAT_7_0_ECX]; /* Feature flags */
> if ((*ecx & CPUID_7_0_ECX_PKU) && env->cr[4] & CR4_PKE_MASK) {
> *ecx |= CPUID_7_0_ECX_OSPKE;
> }
> *edx = env->features[FEAT_7_0_EDX]; /* Feature flags */
> + } else if (count == 1) {
> + *eax = env->features[FEAT_7_1_EAX];
> + *ebx = 0;
> + *ecx = 0;
> + *edx = 0;
> } else {
> *eax = 0;
> *ebx = 0;
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index bd06523..40594a1 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -488,6 +488,7 @@ typedef enum FeatureWord {
> FEAT_7_0_EBX, /* CPUID[EAX=7,ECX=0].EBX */
> FEAT_7_0_ECX, /* CPUID[EAX=7,ECX=0].ECX */
> FEAT_7_0_EDX, /* CPUID[EAX=7,ECX=0].EDX */
> + FEAT_7_1_EAX, /* CPUID[EAX=7,ECX=1].EAX */
> FEAT_8000_0001_EDX, /* CPUID[8000_0001].EDX */
> FEAT_8000_0001_ECX, /* CPUID[8000_0001].ECX */
> FEAT_8000_0007_EDX, /* CPUID[8000_0007].EDX */
> @@ -699,6 +700,8 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
> #define CPUID_7_0_EDX_ARCH_CAPABILITIES (1U << 29) /*Arch Capabilities*/
> #define CPUID_7_0_EDX_SPEC_CTRL_SSBD (1U << 31) /* Speculative Store Bypass Disable */
>
> +#define CPUID_7_1_EAX_AVX512_BF16 (1U << 5) /* AVX512 BFloat16 Instruction */
> +
> #define CPUID_8000_0008_EBX_WBNOINVD (1U << 9) /* Write back and
> do not invalidate cache */
> #define CPUID_8000_0008_EBX_IBPB (1U << 12) /* Indirect Branch Prediction Barrier */
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 3b29ce5..977aaa5 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -1110,6 +1110,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
> c = &cpuid_data.entries[cpuid_i++];
> }
> break;
> + case 0x7:
> case 0x14: {
> uint32_t times;
>
> @@ -1122,7 +1123,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
> for (j = 1; j <= times; ++j) {
> if (cpuid_i == KVM_MAX_CPUID_ENTRIES) {
> fprintf(stderr, "cpuid_data is full, no space for "
> - "cpuid(eax:0x14,ecx:0x%x)\n", j);
> + "cpuid(eax:0x%x,ecx:0x%x)\n", i, j);
> abort();
> }
> c = &cpuid_data.entries[cpuid_i++];
>
On 18/07/19 06:55, Jing Liu wrote:
>>
>> + *eax = kvm_arch_get_supported_cpuid(cs->kvm_state, 0x7,
>> + count, R_EAX);
> This needs to be firstly checked as follows, otherwise some
> architectures would fail to compile.
>
> What about hvf and tcg CPUID 07 EAX value?
>
> + /* Maximum ECX value for sub-leaves */
> + if (kvm_enabled()) {
> + *eax = kvm_arch_get_supported_cpuid(cs->kvm_state, 0x7,
> + count, R_EAX);
> + } else if (hvf_enabled()) {
> + *eax = hvf_get_supported_cpuid(0x7, count, R_EAX);
> + } else {
> + *eax = 0;
> + }
>
Good question. You need to add a new property, for example
cpuid_level_func7, whose code would be modeled around cpuid_level (and a
field cpuid_min_level_func7 whose code would be modeled around
cpuid_min_level).
Then CPUID[7,0].EAX is set automatically to 0 or 1 depending on whether
BF16 is enabled or not.
Paolo
On 7/18/2019 4:15 PM, Paolo Bonzini wrote:
> On 18/07/19 06:55, Jing Liu wrote:
>>>
>>> + *eax = kvm_arch_get_supported_cpuid(cs->kvm_state, 0x7,
>>> + count, R_EAX);
>> This needs to be firstly checked as follows, otherwise some
>> architectures would fail to compile.
>>
>> What about hvf and tcg CPUID 07 EAX value?
>>
>> + /* Maximum ECX value for sub-leaves */
>> + if (kvm_enabled()) {
>> + *eax = kvm_arch_get_supported_cpuid(cs->kvm_state, 0x7,
>> + count, R_EAX);
>> + } else if (hvf_enabled()) {
>> + *eax = hvf_get_supported_cpuid(0x7, count, R_EAX);
>> + } else {
>> + *eax = 0;
>> + }
>>
>
> Good question. You need to add a new property, for example
> cpuid_level_func7, whose code would be modeled around cpuid_level (and a
> field cpuid_min_level_func7 whose code would be modeled around
> cpuid_min_level).
>
> Then CPUID[7,0].EAX is set automatically to 0 or 1 depending on whether
> BF16 is enabled or not.
Could I ask why don't we directly check BF16 enabling when
cpu_x86_cpuid(env, 7, 0, ...) during kvm_arch_init_vcpu ?
What is the use of the two new properties? Are they used for users
setting parameters when boot up guest, and why we need users setting
func7 level?
I tried to implement the code as follows.
@@ -4293,13 +4313,19 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t
index, uint32_t count,
case 7:
/* Structured Extended Feature Flags Enumeration Leaf */
if (count == 0) {
- *eax = 0; /* Maximum ECX value for sub-leaves */
+ /* Maximum ECX value for sub-leaves */
+ *eax = env->cpuid_level_func7;
[...]
+ } else if (count == 1) {
+ *eax = env->features[FEAT_7_1_EAX];
+ *ebx = 0;
+ *ecx = 0;
+ *edx = 0;
[...]
@@ -5075,6 +5101,10 @@ static void x86_cpu_expand_features(X86CPU *cpu,
Error **errp)
x86_cpu_adjust_feat_level(cpu, FEAT_SVM);
x86_cpu_adjust_feat_level(cpu, FEAT_XSAVE);
+ if ((env->features[FEAT_7_1_EAX] & CPUID_7_1_EAX_AVX512_BF16) &&
+ kvm_enabled()) {
+ x86_cpu_adjust_level(cpu, &env->cpuid_min_level_func7, 1);
+ }
/* Intel Processor Trace requires CPUID[0x14] */
if ((env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) &&
kvm_enabled() && cpu->intel_pt_auto_level) {
@@ -5098,6 +5128,9 @@ static void x86_cpu_expand_features(X86CPU *cpu,
Error **errp)
}
/* Set cpuid_*level* based on cpuid_min_*level, if not explicitly
set */
+ if (env->cpuid_level_func7 == UINT32_MAX) {
+ env->cpuid_level_func7 = env->cpuid_min_level_func7;
+ }
if (env->cpuid_level == UINT32_MAX) {
env->cpuid_level = env->cpuid_min_level;
}
@@ -5869,9 +5902,11 @@ static Property x86_cpu_properties[] = {
DEFINE_PROP_BOOL("host-phys-bits", X86CPU, host_phys_bits, false),
DEFINE_PROP_UINT8("host-phys-bits-limit", X86CPU,
host_phys_bits_limit, 0),
DEFINE_PROP_BOOL("fill-mtrr-mask", X86CPU, fill_mtrr_mask, true),
+ DEFINE_PROP_UINT32("level-func7", X86CPU, env.cpuid_level_func7,
UINT32_MAX),
DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, UINT32_MAX),
DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, UINT32_MAX),
DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, UINT32_MAX),
+ DEFINE_PROP_UINT32("min-level-func7", X86CPU,
env.cpuid_min_level_func7, 0),
[...]
Thanks,
Jing
>
> Paolo
>
On 19/07/19 09:20, Jing Liu wrote:
>> Then CPUID[7,0].EAX is set automatically to 0 or 1 depending on whether
>> BF16 is enabled or not.
>
> Could I ask why don't we directly check BF16 enabling when
> cpu_x86_cpuid(env, 7, 0, ...) during kvm_arch_init_vcpu ?
Because the code for setting CPUID is common for all accelerators (there
are five supported: KVM, HAX, HVF, TCG, WHPX).
> What is the use of the two new properties? Are they used for users
> setting parameters when boot up guest, and why we need users setting
> func7 level?
For example to test guests with CPUID[7,0].EAX==1, even if the host does
not have BF16.
> I tried to implement the code as follows.
>
> @@ -4293,13 +4313,19 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t
> index, uint32_t count,
> case 7:
> /* Structured Extended Feature Flags Enumeration Leaf */
> if (count == 0) {
> - *eax = 0; /* Maximum ECX value for sub-leaves */
> + /* Maximum ECX value for sub-leaves */
> + *eax = env->cpuid_level_func7;
> [...]
> + } else if (count == 1) {
> + *eax = env->features[FEAT_7_1_EAX];
> + *ebx = 0;
> + *ecx = 0;
> + *edx = 0;
Looks good.
> @@ -5075,6 +5101,10 @@ static void x86_cpu_expand_features(X86CPU *cpu,
> Error **errp)
> x86_cpu_adjust_feat_level(cpu, FEAT_SVM);
> x86_cpu_adjust_feat_level(cpu, FEAT_XSAVE);
>
> + if ((env->features[FEAT_7_1_EAX] & CPUID_7_1_EAX_AVX512_BF16) &&
> + kvm_enabled()) {
No need to check KVM. You could also do just
x86_cpu_adjust_feat_level(cpu, FEAT_7_1_EAX) and set
cpu->min_level_func7 in x86_cpu_adjust_feat_level with something like
if (eax == 7) {
x86_cpu_adjust_level(cpu, &env->cpu_min_level_func7,
fi->cpuid.ecx);
}
> + x86_cpu_adjust_level(cpu, &env->cpuid_min_level_func7, 1);
> + }
> /* Intel Processor Trace requires CPUID[0x14] */
> if ((env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) &&
> kvm_enabled() && cpu->intel_pt_auto_level) {
> @@ -5098,6 +5128,9 @@ static void x86_cpu_expand_features(X86CPU *cpu,
> Error **errp)
> }
>
> /* Set cpuid_*level* based on cpuid_min_*level, if not explicitly
> set */
> + if (env->cpuid_level_func7 == UINT32_MAX) {
> + env->cpuid_level_func7 = env->cpuid_min_level_func7;
> + }
Looks good.
> if (env->cpuid_level == UINT32_MAX) {
> env->cpuid_level = env->cpuid_min_level;
> }
> @@ -5869,9 +5902,11 @@ static Property x86_cpu_properties[] = {
> DEFINE_PROP_BOOL("host-phys-bits", X86CPU, host_phys_bits, false),
> DEFINE_PROP_UINT8("host-phys-bits-limit", X86CPU,
> host_phys_bits_limit, 0),
> DEFINE_PROP_BOOL("fill-mtrr-mask", X86CPU, fill_mtrr_mask, true),
> + DEFINE_PROP_UINT32("level-func7", X86CPU, env.cpuid_level_func7,
> UINT32_MAX),
Looks good.
> DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, UINT32_MAX),
> DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, UINT32_MAX),
> DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, UINT32_MAX),
> + DEFINE_PROP_UINT32("min-level-func7", X86CPU,
> env.cpuid_min_level_func7, 0),
No need for this property, just like there is no min-level property.
Thanks,
Paolo
On 7/19/2019 4:10 PM, Paolo Bonzini wrote:
> On 19/07/19 09:20, Jing Liu wrote:
>>> Then CPUID[7,0].EAX is set automatically to 0 or 1 depending on whether
>>> BF16 is enabled or not.
>>
>> Could I ask why don't we directly check BF16 enabling when
>> cpu_x86_cpuid(env, 7, 0, ...) during kvm_arch_init_vcpu ?
>
> Because the code for setting CPUID is common for all accelerators (there
> are five supported: KVM, HAX, HVF, TCG, WHPX).
>
>> What is the use of the two new properties? Are they used for users
>> setting parameters when boot up guest, and why we need users setting
>> func7 level?
>
> For example to test guests with CPUID[7,0].EAX==1, even if the host does
> not have BF16.
Thanks. :)
>
>
>> @@ -5075,6 +5101,10 @@ static void x86_cpu_expand_features(X86CPU *cpu,
>> Error **errp)
>> x86_cpu_adjust_feat_level(cpu, FEAT_SVM);
>> x86_cpu_adjust_feat_level(cpu, FEAT_XSAVE);
>>
>> + if ((env->features[FEAT_7_1_EAX] & CPUID_7_1_EAX_AVX512_BF16) &&
>> + kvm_enabled()) {
>
> No need to check KVM. You could also do just
> x86_cpu_adjust_feat_level(cpu, FEAT_7_1_EAX) and set
> cpu->min_level_func7 in x86_cpu_adjust_feat_level with something like
>
> if (eax == 7) {
> x86_cpu_adjust_level(cpu, &env->cpu_min_level_func7,
> fi->cpuid.ecx);
> }
>
Got it. One question I'm wondering is, is it possible for users setting
an invalid property like level-func7=2? Do we need some protection?
>> @@ -5098,6 +5128,9 @@ static void x86_cpu_expand_features(X86CPU *cpu,
>> Error **errp)
>> }
>>
>> /* Set cpuid_*level* based on cpuid_min_*level, if not explicitly
>> set */
>> + if (env->cpuid_level_func7 == UINT32_MAX) {
>> + env->cpuid_level_func7 = env->cpuid_min_level_func7;
>> + }
>
> Looks good.
>
>> if (env->cpuid_level == UINT32_MAX) {
>> env->cpuid_level = env->cpuid_min_level;
>> }
>> @@ -5869,9 +5902,11 @@ static Property x86_cpu_properties[] = {
>> DEFINE_PROP_BOOL("host-phys-bits", X86CPU, host_phys_bits, false),
>> DEFINE_PROP_UINT8("host-phys-bits-limit", X86CPU,
>> host_phys_bits_limit, 0),
>> DEFINE_PROP_BOOL("fill-mtrr-mask", X86CPU, fill_mtrr_mask, true),
>> + DEFINE_PROP_UINT32("level-func7", X86CPU, env.cpuid_level_func7,
>> UINT32_MAX),
>
> Looks good.
>
>> DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, UINT32_MAX),
>> DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, UINT32_MAX),
>> DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, UINT32_MAX),
>> + DEFINE_PROP_UINT32("min-level-func7", X86CPU,
>> env.cpuid_min_level_func7, 0),
>
> No need for this property, just like there is no min-level property.
>
Would remove it.
Thanks,
Jing
On 22/07/19 04:59, Jing Liu wrote:
>
>
> On 7/19/2019 4:10 PM, Paolo Bonzini wrote:
>> On 19/07/19 09:20, Jing Liu wrote:
>>>> Then CPUID[7,0].EAX is set automatically to 0 or 1 depending on whether
>>>> BF16 is enabled or not.
>>>
>>> Could I ask why don't we directly check BF16 enabling when
>>> cpu_x86_cpuid(env, 7, 0, ...) during kvm_arch_init_vcpu ?
>>
>> Because the code for setting CPUID is common for all accelerators (there
>> are five supported: KVM, HAX, HVF, TCG, WHPX).
>>
>>> What is the use of the two new properties? Are they used for users
>>> setting parameters when boot up guest, and why we need users setting
>>> func7 level?
>>
>> For example to test guests with CPUID[7,0].EAX==1, even if the host does
>> not have BF16.
>
> Thanks. :)
>>
>>
>>> @@ -5075,6 +5101,10 @@ static void x86_cpu_expand_features(X86CPU *cpu,
>>> Error **errp)
>>> x86_cpu_adjust_feat_level(cpu, FEAT_SVM);
>>> x86_cpu_adjust_feat_level(cpu, FEAT_XSAVE);
>>>
>>> + if ((env->features[FEAT_7_1_EAX] & CPUID_7_1_EAX_AVX512_BF16) &&
>>> + kvm_enabled()) {
>>
>> No need to check KVM. You could also do just
>> x86_cpu_adjust_feat_level(cpu, FEAT_7_1_EAX) and set
>> cpu->min_level_func7 in x86_cpu_adjust_feat_level with something like
>>
>> if (eax == 7) {
>> x86_cpu_adjust_level(cpu, &env->cpu_min_level_func7,
>> fi->cpuid.ecx);
>> }
>>
>
> Got it. One question I'm wondering is, is it possible for users setting
> an invalid property like level-func7=2? Do we need some protection?
No, it's still not found in Intel silicon, but in principle you could
have higher indices than 1. So it's okay, if something breaks it's the
fault of whoever set the option!
Paolo
On 7/22/2019 7:50 PM, Paolo Bonzini wrote:
> On 22/07/19 04:59, Jing Liu wrote:
>>
>>
>> On 7/19/2019 4:10 PM, Paolo Bonzini wrote:
>>> On 19/07/19 09:20, Jing Liu wrote:
>>>>> Then CPUID[7,0].EAX is set automatically to 0 or 1 depending on whether
>>>>> BF16 is enabled or not.
>>>>
>>>> Could I ask why don't we directly check BF16 enabling when
>>>> cpu_x86_cpuid(env, 7, 0, ...) during kvm_arch_init_vcpu ?
>>>
>>> Because the code for setting CPUID is common for all accelerators (there
>>> are five supported: KVM, HAX, HVF, TCG, WHPX).
>>>
>>>> What is the use of the two new properties? Are they used for users
>>>> setting parameters when boot up guest, and why we need users setting
>>>> func7 level?
>>>
>>> For example to test guests with CPUID[7,0].EAX==1, even if the host does
>>> not have BF16.
>>
>> Thanks. :)
>>>
>>>
>>>> @@ -5075,6 +5101,10 @@ static void x86_cpu_expand_features(X86CPU *cpu,
>>>> Error **errp)
>>>> x86_cpu_adjust_feat_level(cpu, FEAT_SVM);
>>>> x86_cpu_adjust_feat_level(cpu, FEAT_XSAVE);
>>>>
>>>> + if ((env->features[FEAT_7_1_EAX] & CPUID_7_1_EAX_AVX512_BF16) &&
>>>> + kvm_enabled()) {
>>>
>>> No need to check KVM. You could also do just
>>> x86_cpu_adjust_feat_level(cpu, FEAT_7_1_EAX) and set
>>> cpu->min_level_func7 in x86_cpu_adjust_feat_level with something like
>>>
>>> if (eax == 7) {
>>> x86_cpu_adjust_level(cpu, &env->cpu_min_level_func7,
>>> fi->cpuid.ecx);
>>> }
>>>
>>
>> Got it. One question I'm wondering is, is it possible for users setting
>> an invalid property like level-func7=2? Do we need some protection?
>
> No, it's still not found in Intel silicon, but in principle you could
> have higher indices than 1. So it's okay, if something breaks it's the
> fault of whoever set the option!
>
Thanks very much. So would you like me to update the patch with v2 now?
Jing
On 24/07/19 14:05, Jing Liu wrote: > > Thanks very much. So would you like me to update the patch with v2 now? Yes, please. Paolo
Patchew URL: https://patchew.org/QEMU/1562823509-13072-1-git-send-email-jing2.liu@linux.intel.com/ Hi, This series failed build test on s390x host. Please find the details below. === TEST SCRIPT BEGIN === #!/bin/bash # Testing script will be invoked under the git checkout with # HEAD pointing to a commit that has the patches applied on top of "base" # branch set -e echo echo "=== ENV ===" env echo echo "=== PACKAGES ===" rpm -qa echo echo "=== UNAME ===" uname -a CC=$HOME/bin/cc INSTALL=$PWD/install BUILD=$PWD/build mkdir -p $BUILD $INSTALL SRC=$PWD cd $BUILD $SRC/configure --cc=$CC --prefix=$INSTALL make -j4 # XXX: we need reliable clean up # make check -j4 V=1 make install === TEST SCRIPT END === CC aarch64-softmmu/hw/arm/virt-acpi-build.o /usr/bin/ld: target/i386/cpu.o: in function `cpu_x86_cpuid': /var/tmp/patchew-tester-tmp-f9kile96/src/target/i386/cpu.c:4317: undefined reference to `kvm_arch_get_supported_cpuid' collect2: error: ld returned 1 exit status make[1]: *** [Makefile:208: qemu-system-i386] Error 1 make: *** [Makefile:472: i386-softmmu/all] Error 2 make: *** Waiting for unfinished jobs.... The full log is available at http://patchew.org/logs/1562823509-13072-1-git-send-email-jing2.liu@linux.intel.com/testing.s390x/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
© 2016 - 2024 Red Hat, Inc.