* [Qemu-devel] [PATCH v2 1/1] pc-dimm: Change PCDIMMDevice->node from UINT32 to INT32, and initialize it as -1.
@ 2014-08-19 1:55 Tang Chen
2014-08-26 9:32 ` tangchen
0 siblings, 1 reply; 6+ messages in thread
From: Tang Chen @ 2014-08-19 1:55 UTC (permalink / raw)
To: mst, imammedo, hutao, peter.crosthwaite, eblake, pbonzini,
ehabkost, qemu-devel
Cc: isimatu.yasuaki, tangchen
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/1] pc-dimm: Change PCDIMMDevice->node from UINT32 to INT32, and initialize it as -1.
2014-08-19 1:55 [Qemu-devel] [PATCH v2 1/1] pc-dimm: Change PCDIMMDevice->node from UINT32 to INT32, and initialize it as -1 Tang Chen
@ 2014-08-26 9:32 ` tangchen
2014-08-26 14:24 ` Andrey Korolyov
0 siblings, 1 reply; 6+ messages in thread
From: tangchen @ 2014-08-26 9:32 UTC (permalink / raw)
To: mst, imammedo, hutao, peter.crosthwaite, eblake, pbonzini,
ehabkost, qemu-devel
Cc: isimatu.yasuaki, 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;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/1] pc-dimm: Change PCDIMMDevice->node from UINT32 to INT32, and initialize it as -1.
2014-08-26 9:32 ` tangchen
@ 2014-08-26 14:24 ` Andrey Korolyov
2014-08-27 8:39 ` tangchen
2014-08-27 8:39 ` tangchen
0 siblings, 2 replies; 6+ messages in thread
From: Andrey Korolyov @ 2014-08-26 14:24 UTC (permalink / raw)
To: tangchen
Cc: peter.crosthwaite, ehabkost, Michael S. Tsirkin, hutao,
qemu-devel@nongnu.org, isimatu.yasuaki, Paolo Bonzini,
Igor Mammedov
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/1] pc-dimm: Change PCDIMMDevice->node from UINT32 to INT32, and initialize it as -1.
2014-08-26 14:24 ` Andrey Korolyov
@ 2014-08-27 8:39 ` tangchen
2014-08-27 8:39 ` tangchen
1 sibling, 0 replies; 6+ messages in thread
From: tangchen @ 2014-08-27 8:39 UTC (permalink / raw)
To: Andrey Korolyov
Cc: peter.crosthwaite, ehabkost, Michael S. Tsirkin, hutao,
qemu-devel@nongnu.org, isimatu.yasuaki, Paolo Bonzini,
Igor Mammedov
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/1] pc-dimm: Change PCDIMMDevice->node from UINT32 to INT32, and initialize it as -1.
2014-08-26 14:24 ` Andrey Korolyov
2014-08-27 8:39 ` tangchen
@ 2014-08-27 8:39 ` tangchen
2014-08-27 8:51 ` Andrey Korolyov
1 sibling, 1 reply; 6+ messages in thread
From: tangchen @ 2014-08-27 8:39 UTC (permalink / raw)
To: Andrey Korolyov
Cc: peter.crosthwaite, ehabkost, Michael S. Tsirkin, hutao,
qemu-devel@nongnu.org, tangchen, isimatu.yasuaki, Paolo Bonzini,
Igor Mammedov
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/1] pc-dimm: Change PCDIMMDevice->node from UINT32 to INT32, and initialize it as -1.
2014-08-27 8:39 ` tangchen
@ 2014-08-27 8:51 ` Andrey Korolyov
0 siblings, 0 replies; 6+ messages in thread
From: Andrey Korolyov @ 2014-08-27 8:51 UTC (permalink / raw)
To: tangchen
Cc: peter.crosthwaite, ehabkost, Michael S. Tsirkin, hutao,
qemu-devel@nongnu.org, isimatu.yasuaki, Paolo Bonzini,
Igor Mammedov
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-08-27 8:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-08-19 1:55 [Qemu-devel] [PATCH v2 1/1] pc-dimm: Change PCDIMMDevice->node from UINT32 to INT32, and initialize it as -1 Tang Chen
2014-08-26 9:32 ` tangchen
2014-08-26 14:24 ` Andrey Korolyov
2014-08-27 8:39 ` tangchen
2014-08-27 8:39 ` tangchen
2014-08-27 8:51 ` Andrey Korolyov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).