Re: [Qemu-devel] [PATCHv3 2/4] pseries: Move CPU compatibility property to machine

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4452 bytes --]

On Tue, May 02, 2017 at 04:24:55PM +0200, Greg Kurz wrote:
> On Thu, 27 Apr 2017 17:28:41 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
[snip]
> > @@ -45,18 +48,21 @@ static const CompatInfo compat_table[] = {
> >          .max_threads = 2,
> >      },
> >      { /* POWER7, ISA2.06 */
> > +        .name = "power7",
> >          .pvr = CPU_POWERPC_LOGICAL_2_06,
> >          .pcr = PCR_COMPAT_3_00 | PCR_COMPAT_2_07 | PCR_COMPAT_2_06 | PCR_TM_DIS,
> >          .pcr_level = PCR_COMPAT_2_06,
> >          .max_threads = 4,
> >      },
> >      {
> > +        .name = "power7+",
> >          .pvr = CPU_POWERPC_LOGICAL_2_06_PLUS,
> >          .pcr = PCR_COMPAT_3_00 | PCR_COMPAT_2_07 | PCR_COMPAT_2_06 | PCR_TM_DIS,
> >          .pcr_level = PCR_COMPAT_2_06,
> >          .max_threads = 4,
> >      },
> >      { /* POWER8, ISA2.07 */
> > +        .name = "power8",
> >          .pvr = CPU_POWERPC_LOGICAL_2_07,
> >          .pcr = PCR_COMPAT_3_00 | PCR_COMPAT_2_07,
> >          .pcr_level = PCR_COMPAT_2_07,
> 
> And now we also have POWER9 in the list, so:
> 
>          .max_threads = 8,
>      },
>      { /* POWER9, ISA3.00 */
> +        .name = "power9",
>          .pvr = CPU_POWERPC_LOGICAL_3_00,
>          .pcr = PCR_COMPAT_3_00,
>          .pcr_level = PCR_COMPAT_3_00,

Updated for the next spin.

> > @@ -189,3 +195,62 @@ int ppc_compat_max_threads(PowerPCCPU *cpu)
> >  
> >      return n_threads;
> >  }
> > +
> > +void ppc_compat_prop_get(Object *obj, Visitor *v, const char *name,
> > +                         void *opaque, Error **errp)
> > +{
> > +    uint32_t compat_pvr = *((uint32_t *)opaque);
> > +    const char *value;
> > +
> > +    if (!compat_pvr) {
> > +        value = "";
> > +    } else {
> > +        const CompatInfo *compat = compat_by_pvr(compat_pvr);
> > +
> > +        g_assert(compat);
> > +
> > +        value = compat->name;
> > +    }
> > +
> > +    visit_type_str(v, name, (char **)&value, errp);
> > +}
> > +
> > +void ppc_compat_prop_set(Object *obj, Visitor *v, const char *name,
> > +                         void *opaque, Error **errp)
> > +{
> > +    Error *error = NULL;
> > +    char *value;
> > +    uint32_t compat_pvr;
> > +
> > +    visit_type_str(v, name, &value, &error);
> > +    if (error) {
> > +        error_propagate(errp, error);
> > +        return;
> > +    }
> > +
> > +    if (strcmp(value, "") == 0) {
> > +        compat_pvr = 0;
> 
> The current implementation in powerpc_get_compat() considers "" to be an
> invalid compatibility mode. Is there a reason to behave differently with
> max-cpu-compat ?

Hrm.  Symmetry, really.  In ppc_compat_prop_get() we represent no
compatibility mode set as an empty string.  Setting the same value
back should have the corresponding effect.

[snip]
> > +static void getset_compat_deprecated(Object *obj, Visitor *v, const char *name,
> > +                                     void *opaque, Error **errp)
> >  {
> > -    Error *error = NULL;
> > -    char *value = NULL;
> > -    Property *prop = opaque;
> > -    uint32_t *max_compat = qdev_get_prop_ptr(DEVICE(obj), prop);
> > -
> > -    visit_type_str(v, name, &value, &error);
> > -    if (error) {
> > -        error_propagate(errp, error);
> > -        return;
> > -    }
> > -
> > -    if (strcmp(value, "power6") == 0) {
> > -        *max_compat = CPU_POWERPC_LOGICAL_2_05;
> > -    } else if (strcmp(value, "power7") == 0) {
> > -        *max_compat = CPU_POWERPC_LOGICAL_2_06;
> > -    } else if (strcmp(value, "power8") == 0) {
> > -        *max_compat = CPU_POWERPC_LOGICAL_2_07;
> > -    } else {
> > -        error_setg(errp, "Invalid compatibility mode \"%s\"", value);
> > -    }
> > -
> > -    g_free(value);
> > +    error_report("CPU 'compat' property is deprecated and has no effect; use max-cpu-compat machine property instead");
> > +    visit_type_null(v, name, errp);
> >  }
> >  
> 
> As suggested in another mail, maybe NULL should be passed instead of errp if
> we really want to implement the "has no effect". Otherwise, it has the effect
> of terminating QEMU when passing a compat prop that isn't "".

Good point.  I'll change that.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]