* [Qemu-devel] [PATCH v1] x86: Intel AVX512_BF16 feature enabling
@ 2019-07-11 5:38 Jing Liu
2019-07-11 5:47 ` no-reply
2019-07-18 4:55 ` Jing Liu
0 siblings, 2 replies; 10+ messages in thread
From: Jing Liu @ 2019-07-11 5:38 UTC (permalink / raw)
To: qemu-devel, pbonzini; +Cc: Jing Liu
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH v1] x86: Intel AVX512_BF16 feature enabling
2019-07-11 5:38 [Qemu-devel] [PATCH v1] x86: Intel AVX512_BF16 feature enabling Jing Liu
@ 2019-07-11 5:47 ` no-reply
2019-07-18 4:55 ` Jing Liu
1 sibling, 0 replies; 10+ messages in thread
From: no-reply @ 2019-07-11 5:47 UTC (permalink / raw)
To: jing2.liu; +Cc: pbonzini, jing2.liu, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v1] x86: Intel AVX512_BF16 feature enabling
2019-07-11 5:38 [Qemu-devel] [PATCH v1] x86: Intel AVX512_BF16 feature enabling Jing Liu
2019-07-11 5:47 ` no-reply
@ 2019-07-18 4:55 ` Jing Liu
2019-07-18 8:15 ` Paolo Bonzini
1 sibling, 1 reply; 10+ messages in thread
From: Jing Liu @ 2019-07-18 4:55 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini
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++];
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH v1] x86: Intel AVX512_BF16 feature enabling
2019-07-18 4:55 ` Jing Liu
@ 2019-07-18 8:15 ` Paolo Bonzini
2019-07-19 7:20 ` Jing Liu
0 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2019-07-18 8:15 UTC (permalink / raw)
To: Jing Liu, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH v1] x86: Intel AVX512_BF16 feature enabling
2019-07-18 8:15 ` Paolo Bonzini
@ 2019-07-19 7:20 ` Jing Liu
2019-07-19 8:10 ` Paolo Bonzini
0 siblings, 1 reply; 10+ messages in thread
From: Jing Liu @ 2019-07-19 7:20 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH v1] x86: Intel AVX512_BF16 feature enabling
2019-07-19 7:20 ` Jing Liu
@ 2019-07-19 8:10 ` Paolo Bonzini
2019-07-22 2:59 ` Jing Liu
0 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2019-07-19 8:10 UTC (permalink / raw)
To: Jing Liu, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH v1] x86: Intel AVX512_BF16 feature enabling
2019-07-19 8:10 ` Paolo Bonzini
@ 2019-07-22 2:59 ` Jing Liu
2019-07-22 11:50 ` Paolo Bonzini
0 siblings, 1 reply; 10+ messages in thread
From: Jing Liu @ 2019-07-22 2:59 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH v1] x86: Intel AVX512_BF16 feature enabling
2019-07-22 2:59 ` Jing Liu
@ 2019-07-22 11:50 ` Paolo Bonzini
2019-07-24 12:05 ` Jing Liu
0 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2019-07-22 11:50 UTC (permalink / raw)
To: Jing Liu, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH v1] x86: Intel AVX512_BF16 feature enabling
2019-07-22 11:50 ` Paolo Bonzini
@ 2019-07-24 12:05 ` Jing Liu
2019-07-24 12:12 ` Paolo Bonzini
0 siblings, 1 reply; 10+ messages in thread
From: Jing Liu @ 2019-07-24 12:05 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-07-24 12:12 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-07-11 5:38 [Qemu-devel] [PATCH v1] x86: Intel AVX512_BF16 feature enabling Jing Liu
2019-07-11 5:47 ` no-reply
2019-07-18 4:55 ` Jing Liu
2019-07-18 8:15 ` Paolo Bonzini
2019-07-19 7:20 ` Jing Liu
2019-07-19 8:10 ` Paolo Bonzini
2019-07-22 2:59 ` Jing Liu
2019-07-22 11:50 ` Paolo Bonzini
2019-07-24 12:05 ` Jing Liu
2019-07-24 12:12 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).