Re: [Qemu-devel] [PATCH v14 5/6] i386: Disable TOPOEXT feature if it cannot be supported

[Eduardo Habkost <ehabkost@redhat.com>]

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@amd.com>
> ---
>  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