Re: [Qemu-devel] [PATCH v2 1/1] pc-dimm: Change PCDIMMDevice->node from UINT32 to INT32, and initialize it as -1.

[Andrey Korolyov]

On Tue, Aug 26, 2014 at 1:32 PM, tangchen <address@hidden> wrote:
> Hi ,
>
> Would anybody help to review this patch ?
>
> Thanks. :)
>
>
> On 08/19/2014 09:55 AM, Tang Chen wrote:
>>
>> If user doesn't specify numa options, nb_numa_nodes will be 0. But
>> PCDIMMDevice->node
>> is also initialized to 0. As a result, the following check will fail:
>>
>> pc_dimm_realize()
>> {
>>         ......
>>         if (dimm->node >= nb_numa_nodes) {
>>              error_setg(errp, "'DIMM property " PC_DIMM_NODE_PROP " has
>> value %"
>>                         PRIu32 "' which exceeds the number of numa nodes:
>> %d",
>>                         dimm->node, nb_numa_nodes);
>>              return;
>>         }
>>         ......
>> }
>>
>> But this is not an error.
>>
>> PCDIMMDevice->node should be initialized to -1. This is for users who do
>> not use
>> NUMA.
>>
>> Signed-off-by: Tang Chen <address@hidden>
>> ---
>>
>> Change log v1 -> v2:
>> 1. Simplify the comment.
>> 2. Move the definition of NO_NODE_ID near where it is used.
>>
>>   hw/mem/pc-dimm.c         | 7 ++++++-
>>   include/hw/mem/pc-dimm.h | 2 +-
>>   2 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
>> index ec8b1a3..34109a2 100644
>> --- a/hw/mem/pc-dimm.c
>> +++ b/hw/mem/pc-dimm.c
>> @@ -195,9 +195,14 @@ out:
>>       return ret;
>>   }
>>   +/* Default value for PCDIMMDevice->node (unless specified by user).
>> + * In this case, SRAT won't be created.
>> + */
>> +#define NO_NODE_ID -1
>> +
>>   static Property pc_dimm_properties[] = {
>>       DEFINE_PROP_UINT64(PC_DIMM_ADDR_PROP, PCDIMMDevice, addr, 0),
>> -    DEFINE_PROP_UINT32(PC_DIMM_NODE_PROP, PCDIMMDevice, node, 0),
>> +    DEFINE_PROP_INT32(PC_DIMM_NODE_PROP, PCDIMMDevice, node, NO_NODE_ID),
>>       DEFINE_PROP_INT32(PC_DIMM_SLOT_PROP, PCDIMMDevice, slot,
>>                         PC_DIMM_UNASSIGNED_SLOT),
>>       DEFINE_PROP_END_OF_LIST(),
>> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
>> index 761eeef..82abb2f 100644
>> --- a/include/hw/mem/pc-dimm.h
>> +++ b/include/hw/mem/pc-dimm.h
>> @@ -53,7 +53,7 @@ typedef struct PCDIMMDevice {
>>         /* public */
>>       uint64_t addr;
>> -    uint32_t node;
>> +    int32_t node;
>>       int32_t slot;
>>       HostMemoryBackend *hostmem;
>>   } PCDIMMDevice;
>
>
>

Just to remind - Windows will not add pc dimms without populated SRAT,
so imho forcing NUMA topology to be set (and whose support is required
anyway from guest linux kernel in order to support ACPI hotplug) is
better than approach proposed by this patch.