| Message ID | 1408413327-31868-1-git-send-email-tangchen@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
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;
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.
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.
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.
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.
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;
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(-)