[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Philippe Mathieu-Daudé
Subject: Re: [Qemu-ppc] [Qemu-devel] [PATCH] cpu: don't allow negative core id
Date: Thu, 3 Aug 2017 10:21:18 -0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1

On 08/02/2017 06:29 PM, Eduardo Habkost 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 <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' }}


{ '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);

   $ ./ppc64-softmmu/qemu-system-ppc64 -device 
   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.

Laurent with a /* TODO ... */  you can add:
Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

Reviewed-by: Eduardo Habkost <address@hidden>

I'm queueing it on machine-next.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]