| Message ID | 20190612084104.34984-8-like.xu@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce cpu die topology and enable CPUID.1F for i386 | expand |
On Wed, Jun 12, 2019 at 04:41:02PM +0800, Like Xu wrote: > In guest CPUID generation process, the cpuid_min_level would be adjusted to > the maximum passed value for basic CPUID configuration and it should not be > restricted by the limited value returned from cpu_x86_cpuid(). After the basic > cpu_x86_cpuid() loop is finished, the cpuid_0_entry.eax needs to be configured > again by the last adjusted cpuid_min_level value. > > If a user wants to expose CPUID.1F by passing dies > 1 for any reason without > host support, a per-cpu smp topology warning will appear but it's not blocked. > > Signed-off-by: Like Xu <like.xu@linux.intel.com> This code doesn't look at host CPUID at all, as far as I can see. Isn't it simpler to just make cpuid_x86_cpuid() return the correct data? > --- > target/i386/kvm.c | 24 ++++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > index 9b4da9b265..8bf1604d2b 100644 > --- a/target/i386/kvm.c > +++ b/target/i386/kvm.c > @@ -931,12 +931,12 @@ int kvm_arch_init_vcpu(CPUState *cs) > struct kvm_cpuid_entry2 *c; > uint32_t signature[3]; > int kvm_base = KVM_CPUID_SIGNATURE; > - int r; > + int r, cpuid_0_entry, cpuid_min_level; > Error *local_err = NULL; > > memset(&cpuid_data, 0, sizeof(cpuid_data)); > > - cpuid_i = 0; > + cpuid_i = cpuid_0_entry = cpuid_min_level = 0; > > r = kvm_arch_set_tsc_khz(cs); > if (r < 0) { > @@ -1050,6 +1050,12 @@ int kvm_arch_init_vcpu(CPUState *cs) > > cpu_x86_cpuid(env, 0, 0, &limit, &unused, &unused, &unused); > > + /* Allow 0x1f setting regardless of kvm support if nr_dies > 1 */ > + if (limit < 0x1f && env->nr_dies > 1 && cpu->enable_cpuid_0x1f) { > + limit = env->cpuid_level = env->cpuid_min_level = 0x1f; > + warn_report("CPU topology: the CPUID.1F isn't supported on the host."); > + } > + > for (i = 0; i <= limit; i++) { > if (cpuid_i == KVM_MAX_CPUID_ENTRIES) { > fprintf(stderr, "unsupported level value: 0x%x\n", limit); > @@ -1151,8 +1157,22 @@ int kvm_arch_init_vcpu(CPUState *cs) > cpu_x86_cpuid(env, i, 0, &c->eax, &c->ebx, &c->ecx, &c->edx); > break; > } > + > + /* Remember the index of cpuid.0 leaf for reconfiguration. */ > + cpuid_0_entry = (i == 0) ? (cpuid_i - 1) : cpuid_0_entry; > + > + /* Adjust cpuid_min_level to the maximum index of valid basic cpuids. */ > + cpuid_min_level = > + ((c->eax | c->ebx | c->ecx | c->edx | c->flags | c->index) && > + (i > cpuid_min_level)) ? i : cpuid_min_level; > } > > + env->cpuid_level = env->cpuid_min_level = cpuid_min_level; > + > + /* Reconfigure cpuid_0_eax value to follow CPUID.0 instruction spec.*/ > + c = &cpuid_data.entries[cpuid_0_entry]; > + cpu_x86_cpuid(env, 0, 0, &c->eax, &c->ebx, &c->ecx, &c->edx); > + > if (limit >= 0x0a) { > uint32_t eax, edx; > > -- > 2.21.0 >
On Wed, Jun 19, 2019 at 04:15:46PM -0300, Eduardo Habkost wrote: > On Wed, Jun 12, 2019 at 04:41:02PM +0800, Like Xu wrote: > > In guest CPUID generation process, the cpuid_min_level would be adjusted to > > the maximum passed value for basic CPUID configuration and it should not be > > restricted by the limited value returned from cpu_x86_cpuid(). After the basic > > cpu_x86_cpuid() loop is finished, the cpuid_0_entry.eax needs to be configured > > again by the last adjusted cpuid_min_level value. > > > > If a user wants to expose CPUID.1F by passing dies > 1 for any reason without > > host support, a per-cpu smp topology warning will appear but it's not blocked. > > > > Signed-off-by: Like Xu <like.xu@linux.intel.com> > > This code doesn't look at host CPUID at all, as far as I can see. > Isn't it simpler to just make cpuid_x86_cpuid() return the > correct data? I suggest the following change instead. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- target/i386/cpu.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 6db38e145b..d05a224092 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -5152,6 +5152,10 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error **errp) x86_cpu_adjust_level(cpu, &cpu->env.cpuid_min_level, 0x14); } + if (env->nr_dies > 1) { + x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F); + } + /* SVM requires CPUID[0x8000000A] */ if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_SVM) { x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000000A);
On 2019/6/20 7:36, Eduardo Habkost wrote: > On Wed, Jun 19, 2019 at 04:15:46PM -0300, Eduardo Habkost wrote: >> On Wed, Jun 12, 2019 at 04:41:02PM +0800, Like Xu wrote: >>> In guest CPUID generation process, the cpuid_min_level would be adjusted to >>> the maximum passed value for basic CPUID configuration and it should not be >>> restricted by the limited value returned from cpu_x86_cpuid(). After the basic >>> cpu_x86_cpuid() loop is finished, the cpuid_0_entry.eax needs to be configured >>> again by the last adjusted cpuid_min_level value. >>> >>> If a user wants to expose CPUID.1F by passing dies > 1 for any reason without >>> host support, a per-cpu smp topology warning will appear but it's not blocked. >>> >>> Signed-off-by: Like Xu <like.xu@linux.intel.com> >> >> This code doesn't look at host CPUID at all, as far as I can see. >> Isn't it simpler to just make cpuid_x86_cpuid() return the >> correct data? > > I suggest the following change instead. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> Hi Eduardo, Your code is more reasonable and concise than mine on this so let's not break cpuid_x86_cpuid(). I'll remove the use of enable_cpuid_0x1f in next version, and should I resend the patch series "Refactor cpu topo into machine properties" because rebase-fix may distract you ? > --- > target/i386/cpu.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 6db38e145b..d05a224092 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -5152,6 +5152,10 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error **errp) > x86_cpu_adjust_level(cpu, &cpu->env.cpuid_min_level, 0x14); > } > > + if (env->nr_dies > 1) { > + x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F); > + } > + > /* SVM requires CPUID[0x8000000A] */ > if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_SVM) { > x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000000A); >
On Thu, Jun 20, 2019 at 10:03:07AM +0800, Like Xu wrote: > On 2019/6/20 7:36, Eduardo Habkost wrote: > > On Wed, Jun 19, 2019 at 04:15:46PM -0300, Eduardo Habkost wrote: > > > On Wed, Jun 12, 2019 at 04:41:02PM +0800, Like Xu wrote: > > > > In guest CPUID generation process, the cpuid_min_level would be adjusted to > > > > the maximum passed value for basic CPUID configuration and it should not be > > > > restricted by the limited value returned from cpu_x86_cpuid(). After the basic > > > > cpu_x86_cpuid() loop is finished, the cpuid_0_entry.eax needs to be configured > > > > again by the last adjusted cpuid_min_level value. > > > > > > > > If a user wants to expose CPUID.1F by passing dies > 1 for any reason without > > > > host support, a per-cpu smp topology warning will appear but it's not blocked. > > > > > > > > Signed-off-by: Like Xu <like.xu@linux.intel.com> > > > > > > This code doesn't look at host CPUID at all, as far as I can see. > > > Isn't it simpler to just make cpuid_x86_cpuid() return the > > > correct data? > > > > I suggest the following change instead. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > Hi Eduardo, > > Your code is more reasonable and concise than mine on this > so let's not break cpuid_x86_cpuid(). > > I'll remove the use of enable_cpuid_0x1f in next version, and should I > resend the patch series "Refactor cpu topo into machine properties" because > rebase-fix may distract you ? "Refactor cpu topo" and patches 1-4 of this series are already queued on my machine-next branch. You can send the next version of the series using that branch as base: https://github.com/ehabkost/qemu.git machine-next
diff --git a/target/i386/kvm.c b/target/i386/kvm.c index 9b4da9b265..8bf1604d2b 100644 --- a/target/i386/kvm.c +++ b/target/i386/kvm.c @@ -931,12 +931,12 @@ int kvm_arch_init_vcpu(CPUState *cs) struct kvm_cpuid_entry2 *c; uint32_t signature[3]; int kvm_base = KVM_CPUID_SIGNATURE; - int r; + int r, cpuid_0_entry, cpuid_min_level; Error *local_err = NULL; memset(&cpuid_data, 0, sizeof(cpuid_data)); - cpuid_i = 0; + cpuid_i = cpuid_0_entry = cpuid_min_level = 0; r = kvm_arch_set_tsc_khz(cs); if (r < 0) { @@ -1050,6 +1050,12 @@ int kvm_arch_init_vcpu(CPUState *cs) cpu_x86_cpuid(env, 0, 0, &limit, &unused, &unused, &unused); + /* Allow 0x1f setting regardless of kvm support if nr_dies > 1 */ + if (limit < 0x1f && env->nr_dies > 1 && cpu->enable_cpuid_0x1f) { + limit = env->cpuid_level = env->cpuid_min_level = 0x1f; + warn_report("CPU topology: the CPUID.1F isn't supported on the host."); + } + for (i = 0; i <= limit; i++) { if (cpuid_i == KVM_MAX_CPUID_ENTRIES) { fprintf(stderr, "unsupported level value: 0x%x\n", limit); @@ -1151,8 +1157,22 @@ int kvm_arch_init_vcpu(CPUState *cs) cpu_x86_cpuid(env, i, 0, &c->eax, &c->ebx, &c->ecx, &c->edx); break; } + + /* Remember the index of cpuid.0 leaf for reconfiguration. */ + cpuid_0_entry = (i == 0) ? (cpuid_i - 1) : cpuid_0_entry; + + /* Adjust cpuid_min_level to the maximum index of valid basic cpuids. */ + cpuid_min_level = + ((c->eax | c->ebx | c->ecx | c->edx | c->flags | c->index) && + (i > cpuid_min_level)) ? i : cpuid_min_level; } + env->cpuid_level = env->cpuid_min_level = cpuid_min_level; + + /* Reconfigure cpuid_0_eax value to follow CPUID.0 instruction spec.*/ + c = &cpuid_data.entries[cpuid_0_entry]; + cpu_x86_cpuid(env, 0, 0, &c->eax, &c->ebx, &c->ecx, &c->edx); + if (limit >= 0x0a) { uint32_t eax, edx;
In guest CPUID generation process, the cpuid_min_level would be adjusted to the maximum passed value for basic CPUID configuration and it should not be restricted by the limited value returned from cpu_x86_cpuid(). After the basic cpu_x86_cpuid() loop is finished, the cpuid_0_entry.eax needs to be configured again by the last adjusted cpuid_min_level value. If a user wants to expose CPUID.1F by passing dies > 1 for any reason without host support, a per-cpu smp topology warning will appear but it's not blocked. Signed-off-by: Like Xu <like.xu@linux.intel.com> --- target/i386/kvm.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)