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

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

[Tang Chen]

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 <tangchen@cn.fujitsu.com>
---

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;
-- 
1.8.4.2

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

[tangchen]

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 <tangchen@cn.fujitsu.com>
> ---
>
> 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;

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 <tangchen@cn.fujitsu.com> 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 <tangchen@cn.fujitsu.com>
>> ---
>>
>> 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.

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

[tangchen]

On 08/26/2014 10:24 PM, Andrey Korolyov wrote:
> ......
> 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.
> .
Hi Andrey,

Sorry, I don't quite understand. If Windows won't add pc-dimm without SRAT,
why is this approach not enough ?  If there is no SRAT, there is no NUMA 
topology.
Why do we need to set a NUMA topology ?

Thanks.

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

[tangchen]

On 08/26/2014 10:24 PM, Andrey Korolyov wrote:
> ......
> 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.
> .
Hi Andrey,

Sorry, I don't quite understand. If Windows won't add pc-dimm without SRAT,
why is this approach not enough ?  If there is no SRAT, there is no NUMA 
topology.
Why do we need to set a NUMA topology ?

Thanks.

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

[Andrey Korolyov]

On Wed, Aug 27, 2014 at 12:39 PM, tangchen <tangchen@cn.fujitsu.com> wrote:
>
> On 08/26/2014 10:24 PM, Andrey Korolyov wrote:
>>
>> ......
>>
>> 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.
>> .
>
> Hi Andrey,
>
> Sorry, I don't quite understand. If Windows won't add pc-dimm without SRAT,
> why is this approach not enough ?  If there is no SRAT, there is no NUMA
> topology.
> Why do we need to set a NUMA topology ?
>
> Thanks.

Sorry, probably it was a bit unclear. I thnk that there should be no
cases when dimm is plugged (and check from patch is fired up) without
actually populated NUMA, because not every OS will workaround this by
faking the node.