Re: [Qemu-devel] [PATCH] cpu: don't allow negative core id

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 2 Aug 2017 18:29:33 -0300
Eduardo Habkost <ehabkost@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 <lvivier@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 <ehabkost@redhat.com>
> 
> I'm queueing it on machine-next.
>