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

[Qemu-devel] [PATCH 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>
---
 hw/mem/pc-dimm.c         | 2 +-
 include/hw/mem/pc-dimm.h | 2 +-
 include/sysemu/sysemu.h  | 6 ++++++
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index ec8b1a3..4b26611 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -197,7 +197,7 @@ out:
 
 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;
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index d8539fd..c6b9bae 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -139,6 +139,12 @@ extern int mem_prealloc;
 
 #define MAX_NODES 128
 
+/* Initialize PCDIMMDevice->node to -1 so that even if user doesn't specify
+ * any numa option, PCDIMMDevice->node won't be 0, which indicates node0.
+ * In this case, SRAT won't be created, and guest kernel will fake a node.
+ */
+#define NO_NODE_ID -1
+
 /* The following shall be true for all CPUs:
  *   cpu->cpu_index < max_cpus <= MAX_CPUMASK_BITS
  *
-- 
1.8.4.2

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

[Michael S. Tsirkin]

On Mon, Aug 18, 2014 at 08:12:14PM +0800, 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>
> ---
>  hw/mem/pc-dimm.c         | 2 +-
>  include/hw/mem/pc-dimm.h | 2 +-
>  include/sysemu/sysemu.h  | 6 ++++++
>  3 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index ec8b1a3..4b26611 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -197,7 +197,7 @@ out:
>  
>  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;
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index d8539fd..c6b9bae 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -139,6 +139,12 @@ extern int mem_prealloc;
>  
>  #define MAX_NODES 128
>  
> +/* Initialize PCDIMMDevice->node to -1 so that even if user doesn't specify
> + * any numa option, PCDIMMDevice->node won't be 0, which indicates node0.
> + * In this case, SRAT won't be created, and guest kernel will fake a node.
> + */

I think you simply mean
/* default value for PCDIMMDevice->node (unless specified by user) */


> +#define NO_NODE_ID -1
> +

So why do we want this macro here? Put the value near PCDIMMDevice where
it's used.

>  /* The following shall be true for all CPUs:
>   *   cpu->cpu_index < max_cpus <= MAX_CPUMASK_BITS
>   *
> -- 
> 1.8.4.2

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

[Paolo Bonzini]

Il 18/08/2014 15:56, Michael S. Tsirkin ha scritto:
> > +/* Initialize PCDIMMDevice->node to -1 so that even if user doesn't specify
> > + * any numa option, PCDIMMDevice->node won't be 0, which indicates node0.
> > + * In this case, SRAT won't be created, and guest kernel will fake a node.
> > + */
> 
> I think you simply mean
> /* default value for PCDIMMDevice->node (unless specified by user) */
>
> > +#define NO_NODE_ID -1

I think a comment about the SRAT would be useful.

Paolo

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

[Michael S. Tsirkin]

On Mon, Aug 18, 2014 at 03:58:33PM +0200, Paolo Bonzini wrote:
> Il 18/08/2014 15:56, Michael S. Tsirkin ha scritto:
> > > +/* Initialize PCDIMMDevice->node to -1 so that even if user doesn't specify
> > > + * any numa option, PCDIMMDevice->node won't be 0, which indicates node0.
> > > + * In this case, SRAT won't be created, and guest kernel will fake a node.
> > > + */
> > 
> > I think you simply mean
> > /* default value for PCDIMMDevice->node (unless specified by user) */
> >
> > > +#define NO_NODE_ID -1
> 
> I think a comment about the SRAT would be useful.
> 
> Paolo

OK so add:

/* In this case, SRAT won't be created. */

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

[tangchen]

Hi Michael, Paolo

Thanks for the advices. Will send a v2 patch soon.

Thanks.

On 08/19/2014 04:04 AM, Michael S. Tsirkin wrote:
> On Mon, Aug 18, 2014 at 03:58:33PM +0200, Paolo Bonzini wrote:
>> Il 18/08/2014 15:56, Michael S. Tsirkin ha scritto:
>>>> +/* Initialize PCDIMMDevice->node to -1 so that even if user doesn't specify
>>>> + * any numa option, PCDIMMDevice->node won't be 0, which indicates node0.
>>>> + * In this case, SRAT won't be created, and guest kernel will fake a node.
>>>> + */
>>> I think you simply mean
>>> /* default value for PCDIMMDevice->node (unless specified by user) */
>>>
>>>> +#define NO_NODE_ID -1
>> I think a comment about the SRAT would be useful.
>>
>> Paolo
> OK so add:
>
> /* In this case, SRAT won't be created. */
> .
>