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

[Eduardo Habkost]

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 <address@hidden> 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.

Reviewed-by: Eduardo Habkost <address@hidden>

I'm queueing it on machine-next.

-- 
Eduardo