Re: [PATCH v12 2/4] i386: Verify if topoext feature can be supported

[Eduardo Habkost <ehabkost@redhat.com>]

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