topoext feature cannot be supported in certain cases
with large number of cores or threads. Add the check.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 target/i386/cpu.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 86fb1a4..fc5c66d 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -509,6 +509,20 @@ static void encode_topo_cpuid8000001e(CPUState *cs, X86CPU *cpu,
 }
 
 /*
+ * Check if we can support this topology
+ * Fail if number of cores are beyond the supported config
+ * or nr_threads is more than 2
+ */
+static int topology_supports_topoext(int nr_cores, int nr_threads)
+{
+    if ((nr_cores > (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET)) ||
+        (nr_threads > 2)) {
+        return 0;
+    }
+    return 1;
+}
+
+/*
  * Definitions of the hardcoded cache entries we expose:
  * These are legacy cache values. If there is a need to change any
  * of these values please use builtin_x86_defs
@@ -4941,6 +4955,19 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 
     qemu_init_vcpu(cs);
 
+    /* On AMD systems, check if we can support topoext feature */
+    if (IS_AMD_CPU(env) &&
+        (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT)) {
+        if (!topology_supports_topoext(cs->nr_cores, cs->nr_threads)) {
+                /* Cannot support topoext */
+                error_setg(errp, "CPU model does not support topoext feature "
+                                 "with number of cores(%d) and threads(%d). "
+                                 "Please configure -smp options properly.",
+                                 cs->nr_cores, cs->nr_threads);
+                return;
+        }
+    }
+
     /* Only Intel CPUs support hyperthreading. Even though QEMU fixes this
      * issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
      * based on inputs (sockets,cores,threads), it is still better to gives
-- 
1.8.3.1

On Wed, Jun 06, 2018 at 10:36:44AM -0400, Babu Moger wrote:
> topoext feature cannot be supported in certain cases
> with large number of cores or threads. Add the check.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  target/i386/cpu.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 86fb1a4..fc5c66d 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -509,6 +509,20 @@ static void encode_topo_cpuid8000001e(CPUState *cs, X86CPU *cpu,
>  }
>  
>  /*
> + * Check if we can support this topology
> + * Fail if number of cores are beyond the supported config
> + * or nr_threads is more than 2
> + */
> +static int topology_supports_topoext(int nr_cores, int nr_threads)
> +{
> +    if ((nr_cores > (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET)) ||
> +        (nr_threads > 2)) {
> +        return 0;
> +    }
> +    return 1;
> +}
> +
> +/*
>   * Definitions of the hardcoded cache entries we expose:
>   * These are legacy cache values. If there is a need to change any
>   * of these values please use builtin_x86_defs
> @@ -4941,6 +4955,19 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>  
>      qemu_init_vcpu(cs);
>  
> +    /* On AMD systems, check if we can support topoext feature */
> +    if (IS_AMD_CPU(env) &&
> +        (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT)) {
> +        if (!topology_supports_topoext(cs->nr_cores, cs->nr_threads)) {
> +                /* Cannot support topoext */
> +                error_setg(errp, "CPU model does not support topoext feature "
> +                                 "with number of cores(%d) and threads(%d). "
> +                                 "Please configure -smp options properly.",
> +                                 cs->nr_cores, cs->nr_threads);

See error.h documentation:

 * Error reporting system loosely patterned after Glib's GError.
 *
 * Create an error:
 *     error_setg(&err, "situation normal, all fouled up");
 *
 * Create an error and add additional explanation:
 *     error_setg(&err, "invalid quark");
 *     error_append_hint(&err, "Valid quarks are up, down, strange, "
 *                       "charm, top, bottom.\n");
 *
 * Do *not* contract this to
 *     error_setg(&err, "invalid quark\n"
 *                "Valid quarks are up, down, strange, charm, top, bottom.");
 *

I suggest something like this:

static bool topology_supports_topoext(int nr_cores, int nr_threads, Error **errp)
{
    if (nr_cores > (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET))) {
        error_setg(errp, "TOPOEXT unsupported with %d cores per socket", nr_cores);
        error_append_hint(errp, "TOPOEXT supports only up to %d cores per socket",
                          (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET));
        return false;
    }
    if (nr_threads > 2) {
        error_setg(errp, "TOPOEXT unsupported with %d threads per core", nr_threads);
        error_append_hint(errp, "TOPOEXT supports only up to 2 threads per core");
                          (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET));
        return false;
    }
    return true;
}

static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
{
    /* ... */
    if (IS_AMD_CPU(env) &&
        (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
        !topology_supports_topoext(cs->nr_cores, cs->nr_threads, errp)) {
            return;
    }
    /* ... */
}

-- 
Eduardo

> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Wednesday, June 6, 2018 5:06 PM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com;
> rth@twiddle.net; mtosatti@redhat.com; qemu-devel@nongnu.org;
> kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com
> Subject: Re: [PATCH v12 2/4] i386: Verify if topoext feature can be supported
> 
> On Wed, Jun 06, 2018 at 10:36:44AM -0400, Babu Moger wrote:
> > topoext feature cannot be supported in certain cases
> > with large number of cores or threads. Add the check.
> >
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> > ---
> >  target/i386/cpu.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 86fb1a4..fc5c66d 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -509,6 +509,20 @@ static void encode_topo_cpuid8000001e(CPUState
> *cs, X86CPU *cpu,
> >  }
> >
> >  /*
> > + * Check if we can support this topology
> > + * Fail if number of cores are beyond the supported config
> > + * or nr_threads is more than 2
> > + */
> > +static int topology_supports_topoext(int nr_cores, int nr_threads)
> > +{
> > +    if ((nr_cores > (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET))
> ||
> > +        (nr_threads > 2)) {
> > +        return 0;
> > +    }
> > +    return 1;
> > +}
> > +
> > +/*
> >   * Definitions of the hardcoded cache entries we expose:
> >   * These are legacy cache values. If there is a need to change any
> >   * of these values please use builtin_x86_defs
> > @@ -4941,6 +4955,19 @@ static void x86_cpu_realizefn(DeviceState *dev,
> Error **errp)
> >
> >      qemu_init_vcpu(cs);
> >
> > +    /* On AMD systems, check if we can support topoext feature */
> > +    if (IS_AMD_CPU(env) &&
> > +        (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT)) {
> > +        if (!topology_supports_topoext(cs->nr_cores, cs->nr_threads)) {
> > +                /* Cannot support topoext */
> > +                error_setg(errp, "CPU model does not support topoext feature "
> > +                                 "with number of cores(%d) and threads(%d). "
> > +                                 "Please configure -smp options properly.",
> > +                                 cs->nr_cores, cs->nr_threads);
> 
> See error.h documentation:
> 
>  * Error reporting system loosely patterned after Glib's GError.
>  *
>  * Create an error:
>  *     error_setg(&err, "situation normal, all fouled up");
>  *
>  * Create an error and add additional explanation:
>  *     error_setg(&err, "invalid quark");
>  *     error_append_hint(&err, "Valid quarks are up, down, strange, "
>  *                       "charm, top, bottom.\n");
>  *
>  * Do *not* contract this to
>  *     error_setg(&err, "invalid quark\n"
>  *                "Valid quarks are up, down, strange, charm, top, bottom.");
>  *
> 
> I suggest something like this:

Sure. I will make these changes. Thanks

> 
> static bool topology_supports_topoext(int nr_cores, int nr_threads, Error
> **errp)
> {
>     if (nr_cores > (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET))) {
>         error_setg(errp, "TOPOEXT unsupported with %d cores per socket",
> nr_cores);
>         error_append_hint(errp, "TOPOEXT supports only up to %d cores per
> socket",
>                           (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET));
>         return false;
>     }
>     if (nr_threads > 2) {
>         error_setg(errp, "TOPOEXT unsupported with %d threads per core",
> nr_threads);
>         error_append_hint(errp, "TOPOEXT supports only up to 2 threads per
> core");
>                           (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET));
>         return false;
>     }
>     return true;
> }
> 
> static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> {
>     /* ... */
>     if (IS_AMD_CPU(env) &&
>         (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
>         !topology_supports_topoext(cs->nr_cores, cs->nr_threads, errp)) {
>             return;
>     }
>     /* ... */
> }
> 
> --
> Eduardo

On Thu, Jun 07, 2018 at 02:24:18PM +0000, Moger, Babu wrote:
[...]
> > > +    /* On AMD systems, check if we can support topoext feature */
> > > +    if (IS_AMD_CPU(env) &&
> > > +        (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT)) {
> > > +        if (!topology_supports_topoext(cs->nr_cores, cs->nr_threads)) {
> > > +                /* Cannot support topoext */
> > > +                error_setg(errp, "CPU model does not support topoext feature "
> > > +                                 "with number of cores(%d) and threads(%d). "
> > > +                                 "Please configure -smp options properly.",
> > > +                                 cs->nr_cores, cs->nr_threads);
> > 
> > See error.h documentation:
> > 
> >  * Error reporting system loosely patterned after Glib's GError.
> >  *
> >  * Create an error:
> >  *     error_setg(&err, "situation normal, all fouled up");
> >  *
> >  * Create an error and add additional explanation:
> >  *     error_setg(&err, "invalid quark");
> >  *     error_append_hint(&err, "Valid quarks are up, down, strange, "
> >  *                       "charm, top, bottom.\n");
> >  *
> >  * Do *not* contract this to
> >  *     error_setg(&err, "invalid quark\n"
> >  *                "Valid quarks are up, down, strange, charm, top, bottom.");
> >  *
> > 
> > I suggest something like this:
> 
> Sure. I will make these changes. Thanks

Thanks.  Note that I have made a mistake below, by not including
a newline in error_append_hint().

Also, I'm not sure if it's better to mention the current value of
nr_cores in error_setg() or just in the error hint.  Markus, do
you have any suggestion?

> 
> > 
> > static bool topology_supports_topoext(int nr_cores, int nr_threads, Error
> > **errp)
> > {
> >     if (nr_cores > (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET))) {
> >         error_setg(errp, "TOPOEXT unsupported with %d cores per socket",
> > nr_cores);
> >         error_append_hint(errp, "TOPOEXT supports only up to %d cores per
> > socket",
> >                           (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET));
> >         return false;
> >     }
> >     if (nr_threads > 2) {
> >         error_setg(errp, "TOPOEXT unsupported with %d threads per core",
> > nr_threads);
> >         error_append_hint(errp, "TOPOEXT supports only up to 2 threads per
> > core");
> >                           (MAX_CORES_IN_NODE * MAX_NODES_PER_SOCKET));
> >         return false;
> >     }
> >     return true;
> > }
> > 
> > static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
> > {
> >     /* ... */
> >     if (IS_AMD_CPU(env) &&
> >         (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
> >         !topology_supports_topoext(cs->nr_cores, cs->nr_threads, errp)) {
> >             return;
> >     }
> >     /* ... */
> > }
> > 
> > --
> > Eduardo

-- 
Eduardo