On Wed, Jun 13, 2018 at 09:18:26PM -0400, Babu Moger wrote:
> Disable the TOPOEXT feature if it cannot be supported.
> We cannot support this feature with more than 2 nr_threads
> or more than 32 cores in a socket.
>
> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
> ---
> target/i386/cpu.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 2eb26da..637d8eb 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -4765,7 +4765,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
> CPUX86State *env = &cpu->env;
> Error *local_err = NULL;
> - static bool ht_warned;
> + static bool ht_warned, topo_warned;
>
> if (xcc->host_cpuid_required && !accel_uses_host_cpuid()) {
> char *name = x86_cpu_class_get_model_name(xcc);
> @@ -4779,6 +4779,21 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> return;
> }
>
> + /* Disable TOPOEXT if topology cannot be supported */
> + if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) {
> + if (!topology_supports_topoext(MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET,
> + 2)) {
I understand you stopped using cpu->nr_cores/cpu->nr_threads
because it was not filled yet.
But why exactly do you need to do this before calling
x86_cpu_expand_features()?
If you really need nr_cores and nr_threads to be available
earlier, we could simply move their initialization to
cpu_exec_initfn() instead of the solution you implemented in
patch 4/6.
> + env->features[FEAT_8000_0001_ECX] &= !CPUID_EXT3_TOPOEXT;
!CPUID_EXT3_TOPOEXT is 0, this will clear all bits in
env->features[FEAT_8000_0001_ECX]. Did you mean
~CPUID_EXT3_TOPOEXT?
> + if (!topo_warned) {
> + error_report("TOPOEXT feature cannot be supported with more"
> + " than %d cores or more than 2 threads per socket."
> + " Disabling the feature.",
> + (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET));
> + topo_warned = true;
This will print a warning for "-cpu EPYC -smp 64,cores=64".
We shouldn't.
I'm starting to believe we shouldn't add TOPOEXT to EPYC unless
we're ready to make the TOPOEXT CPUID leaves work for all valid
-smp configurations. If the feature will work only on a few
specific cases, the feature should be enabled explicitly using
"-cpu ...,+topoext".
Is it really impossible to make CPUID return reasonable topology
data for larger nr_cores and nr_threads values? It would make
everything much simpler.
--
Eduardo
