On Wed, 2 Aug 2017 18:29:33 -0300
Eduardo Habkost <ehabk...@redhat.com> wrote:
> On Wed, Aug 02, 2017 at 03:50:36PM +0200, Laurent Vivier wrote:
> > On 02/08/2017 15:42, Philippe Mathieu-Daudé wrote:
> > > Hi Laurent,
> > >
> > > On Wed, Aug 2, 2017 at 7:32 AM, Laurent Vivier <lviv...@redhat.com>
> > > wrote:
> > >> With pseries machine type a negative core-id is not managed properly:
> > >> -1 gives an inaccurate error message ("core -1 already populated"),
> > >> -2 crashes QEMU (core dump)
> > >>
> > >> As it seems a negative value is invalid for any architecture,
> > >> instead of checking this in spapr_core_pre_plug() I think it's better
> > >> to check this in the generic part, core_prop_set_core_id()
> > >
> > > Why is this property signed? If there is not reason to use it negative,
> > > is it possible to use object_property_add(.."uint"..)?
> >
> > You should be right:
> >
> > { 'struct': 'NumaNodeOptions',
> > 'data': {
> > '*nodeid': 'uint16',
> > '*cpus': ['uint16'],
> > '*mem': 'size',
> > '*memdev': 'str' }}
> >
> > but
> >
> > { 'struct': 'CpuInstanceProperties',
> > 'data': { '*node-id': 'int',
> > '*socket-id': 'int',
> > '*core-id': 'int',
> > '*thread-id': 'int'
> > }
> > }
> >
> > But I'm not sure it's a good idea to change the API now.
>
> Property parsing is not affected by the QAPI schema at all, so
> touching the schema wouldn't fix the bug.
>
> The same applies to the 'type' argument to object_property_add():
> it is ignored everywhere.
>
> However, the property setter can simply use a visitor for unsigned values, and
> it will reject negative values automatically, e.g.:
>
> diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> index 2bf960d..b5af2bf 100644
> --- a/hw/cpu/core.c
> +++ b/hw/cpu/core.c
> @@ -25,9 +25,9 @@ static void core_prop_set_core_id(Object *obj, Visitor
> *v, const char *name,
> {
> CPUCore *core = CPU_CORE(obj);
> Error *local_err = NULL;
> - int64_t value;
> + uint32_t value;
>
> - visit_type_int(v, name, &value, &local_err);
> + visit_type_uint32(v, name, &value, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> return;
>
>
> $ ./ppc64-softmmu/qemu-system-ppc64 -device
> POWER8_v2.0-spapr-cpu-core,core-id=-2
> qemu-system-ppc64: -device POWER8_v2.0-spapr-cpu-core,core-id=-2: Parameter
> 'core-id' expects uint32_t
>
>
> I would suggest changing the CPUCore struct fields to uint32_t or
> uint64_t, but it would be more intrusive and we're past hard
> freeze. Your patch looks good for 2.10.
there is one reason to use signed int here,
negative values might be used to mark not set property value,
I recall we do something like this in target-i386.
>
> Reviewed-by: Eduardo Habkost <ehabk...@redhat.com>
>
> I'm queueing it on machine-next.
>
