| Message ID | 20210811102423.28908-23-wei.chen@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add device tree based NUMA support to Arm64 | expand |
Hi Wei, On 11/08/2021 11:24, Wei Chen wrote: > Processor NUMA ID information is stored in device tree's processor > node as "numa-node-id". We need a new helper to parse this ID from > processor node. If we get this ID from processor node, this ID's > validity still need to be checked. Once we got a invalid NUMA ID > from any processor node, the device tree will be marked as NUMA > information invalid. > > Signed-off-by: Wei Chen <wei.chen@arm.com> > --- > xen/arch/arm/numa_device_tree.c | 41 +++++++++++++++++++++++++++++++-- > 1 file changed, 39 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/numa_device_tree.c b/xen/arch/arm/numa_device_tree.c > index 1c74ad135d..37cc56acf3 100644 > --- a/xen/arch/arm/numa_device_tree.c > +++ b/xen/arch/arm/numa_device_tree.c > @@ -20,16 +20,53 @@ > #include <xen/init.h> > #include <xen/nodemask.h> > #include <xen/numa.h> > +#include <xen/device_tree.h> > +#include <asm/setup.h> > > s8 device_tree_numa = 0; > +static nodemask_t processor_nodes_parsed __initdata; > > -int srat_disabled(void) > +static int srat_disabled(void) > { > return numa_off || device_tree_numa < 0; > } > > -void __init bad_srat(void) > +static __init void bad_srat(void) > { > printk(KERN_ERR "DT: NUMA information is not used.\n"); > device_tree_numa = -1; > } > + > +/* Callback for device tree processor affinity */ > +static int __init dtb_numa_processor_affinity_init(nodeid_t node) > +{ > + if ( srat_disabled() ) > + return -EINVAL; > + else if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) { > + bad_srat(); > + return -EINVAL; You seem to have a mix of soft and hard tab in this file. Is there a lot of the code that was directly copied from Linux? If not, then the file should be using Xen coding style. > + } > + > + node_set(node, processor_nodes_parsed); > + > + device_tree_numa = 1; > + printk(KERN_INFO "DT: NUMA node %u processor parsed\n", node); > + > + return 0; > +} > + > +/* Parse CPU NUMA node info */ > +int __init device_tree_parse_numa_cpu_node(const void *fdt, int node) AFAICT, you are going to turn this helper static in a follow-up patch. This is a bad practice. Instead, the function should be static from the beginning. If it is not possible, then you should re-order the code. In this case, I think you can add the boilerplate to parse the NUMA information (patch #25) here and then extend it in each patch. > +{ > + uint32_t nid; > + > + nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES); > + printk(XENLOG_WARNING "CPU on NUMA node:%u\n", nid); > + if ( nid >= MAX_NUMNODES ) > + { > + printk(XENLOG_WARNING "Node id %u exceeds maximum value\n", nid); > + return -EINVAL; > + } > + > + return dtb_numa_processor_affinity_init(nid); > +} > Cheers,
On 11/08/2021 11:24, Wei Chen wrote: > Processor NUMA ID information is stored in device tree's processor > node as "numa-node-id". We need a new helper to parse this ID from > processor node. If we get this ID from processor node, this ID's > validity still need to be checked. Once we got a invalid NUMA ID > from any processor node, the device tree will be marked as NUMA > information invalid. > > Signed-off-by: Wei Chen <wei.chen@arm.com> > --- > xen/arch/arm/numa_device_tree.c | 41 +++++++++++++++++++++++++++++++-- > 1 file changed, 39 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/numa_device_tree.c b/xen/arch/arm/numa_device_tree.c > index 1c74ad135d..37cc56acf3 100644 > --- a/xen/arch/arm/numa_device_tree.c > +++ b/xen/arch/arm/numa_device_tree.c > @@ -20,16 +20,53 @@ > #include <xen/init.h> > #include <xen/nodemask.h> > #include <xen/numa.h> > +#include <xen/device_tree.h> > +#include <asm/setup.h> > > s8 device_tree_numa = 0; > +static nodemask_t processor_nodes_parsed __initdata; > > -int srat_disabled(void) > +static int srat_disabled(void) > { > return numa_off || device_tree_numa < 0; > } > > -void __init bad_srat(void) > +static __init void bad_srat(void) > { > printk(KERN_ERR "DT: NUMA information is not used.\n"); > device_tree_numa = -1; > } > + > +/* Callback for device tree processor affinity */ > +static int __init dtb_numa_processor_affinity_init(nodeid_t node) I forgot to answer. It seems odd that some of the function names start with dtb_* while other starts device_tree_*. Any particular reason for that difference of naming? > +{ > + if ( srat_disabled() ) > + return -EINVAL; > + else if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) { > + bad_srat(); > + return -EINVAL; > + } > + > + node_set(node, processor_nodes_parsed); > + > + device_tree_numa = 1; > + printk(KERN_INFO "DT: NUMA node %u processor parsed\n", node); > + > + return 0; > +} > + > +/* Parse CPU NUMA node info */ > +int __init device_tree_parse_numa_cpu_node(const void *fdt, int node) > +{ > + uint32_t nid; > + > + nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES); > + printk(XENLOG_WARNING "CPU on NUMA node:%u\n", nid); > + if ( nid >= MAX_NUMNODES ) > + { > + printk(XENLOG_WARNING "Node id %u exceeds maximum value\n", nid); > + return -EINVAL; > + } > + > + return dtb_numa_processor_affinity_init(nid); > +} > Cheers,
Hi Wei, On 11/08/2021 11:24, Wei Chen wrote: > Processor NUMA ID information is stored in device tree's processor > node as "numa-node-id". We need a new helper to parse this ID from > processor node. If we get this ID from processor node, this ID's > validity still need to be checked. Once we got a invalid NUMA ID > from any processor node, the device tree will be marked as NUMA > information invalid. > > Signed-off-by: Wei Chen <wei.chen@arm.com> > --- > xen/arch/arm/numa_device_tree.c | 41 +++++++++++++++++++++++++++++++-- > 1 file changed, 39 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/numa_device_tree.c b/xen/arch/arm/numa_device_tree.c > index 1c74ad135d..37cc56acf3 100644 > --- a/xen/arch/arm/numa_device_tree.c > +++ b/xen/arch/arm/numa_device_tree.c > @@ -20,16 +20,53 @@ > #include <xen/init.h> > #include <xen/nodemask.h> > #include <xen/numa.h> > +#include <xen/device_tree.h> Nothing in this file seems to depend on xen/device_tree.h. So why do you need to include it? > +#include <asm/setup.h> > > s8 device_tree_numa = 0; > +static nodemask_t processor_nodes_parsed __initdata; > > -int srat_disabled(void) > +static int srat_disabled(void) > { > return numa_off || device_tree_numa < 0; > } > > -void __init bad_srat(void) > +static __init void bad_srat(void) > { > printk(KERN_ERR "DT: NUMA information is not used.\n"); > device_tree_numa = -1; > } > + > +/* Callback for device tree processor affinity */ > +static int __init dtb_numa_processor_affinity_init(nodeid_t node) > +{ > + if ( srat_disabled() ) > + return -EINVAL; > + else if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) { > + bad_srat(); > + return -EINVAL; > + } > + > + node_set(node, processor_nodes_parsed); > + > + device_tree_numa = 1; > + printk(KERN_INFO "DT: NUMA node %u processor parsed\n", node); > + > + return 0; > +} > + > +/* Parse CPU NUMA node info */ > +int __init device_tree_parse_numa_cpu_node(const void *fdt, int node) > +{ > + uint32_t nid; > + > + nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES); > + printk(XENLOG_WARNING "CPU on NUMA node:%u\n", nid); > + if ( nid >= MAX_NUMNODES ) > + { > + printk(XENLOG_WARNING "Node id %u exceeds maximum value\n", nid); > + return -EINVAL; > + } > + > + return dtb_numa_processor_affinity_init(nid); > +} >
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: 2021年8月20日 2:13 > To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org; jbeulich@suse.com > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com> > Subject: Re: [XEN RFC PATCH 22/40] xen/arm: introduce a helper to parse > device tree processor node > > Hi Wei, > > On 11/08/2021 11:24, Wei Chen wrote: > > Processor NUMA ID information is stored in device tree's processor > > node as "numa-node-id". We need a new helper to parse this ID from > > processor node. If we get this ID from processor node, this ID's > > validity still need to be checked. Once we got a invalid NUMA ID > > from any processor node, the device tree will be marked as NUMA > > information invalid. > > > > Signed-off-by: Wei Chen <wei.chen@arm.com> > > --- > > xen/arch/arm/numa_device_tree.c | 41 +++++++++++++++++++++++++++++++-- > > 1 file changed, 39 insertions(+), 2 deletions(-) > > > > diff --git a/xen/arch/arm/numa_device_tree.c > b/xen/arch/arm/numa_device_tree.c > > index 1c74ad135d..37cc56acf3 100644 > > --- a/xen/arch/arm/numa_device_tree.c > > +++ b/xen/arch/arm/numa_device_tree.c > > @@ -20,16 +20,53 @@ > > #include <xen/init.h> > > #include <xen/nodemask.h> > > #include <xen/numa.h> > > +#include <xen/device_tree.h> > > Nothing in this file seems to depend on xen/device_tree.h. So why do you > need to include it? > I remember that without this header file, device_tree_get_u32 in this patch will cause compiling failed. > > +#include <asm/setup.h> > > > > s8 device_tree_numa = 0; > > +static nodemask_t processor_nodes_parsed __initdata; > > > > -int srat_disabled(void) > > +static int srat_disabled(void) > > { > > return numa_off || device_tree_numa < 0; > > } > > > > -void __init bad_srat(void) > > +static __init void bad_srat(void) > > { > > printk(KERN_ERR "DT: NUMA information is not used.\n"); > > device_tree_numa = -1; > > } > > + > > +/* Callback for device tree processor affinity */ > > +static int __init dtb_numa_processor_affinity_init(nodeid_t node) > > +{ > > + if ( srat_disabled() ) > > + return -EINVAL; > > + else if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) { > > + bad_srat(); > > + return -EINVAL; > > + } > > + > > + node_set(node, processor_nodes_parsed); > > + > > + device_tree_numa = 1; > > + printk(KERN_INFO "DT: NUMA node %u processor parsed\n", node); > > + > > + return 0; > > +} > > + > > +/* Parse CPU NUMA node info */ > > +int __init device_tree_parse_numa_cpu_node(const void *fdt, int node) > > +{ > > + uint32_t nid; > > + > > + nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES); > > + printk(XENLOG_WARNING "CPU on NUMA node:%u\n", nid); > > + if ( nid >= MAX_NUMNODES ) > > + { > > + printk(XENLOG_WARNING "Node id %u exceeds maximum value\n", > nid); > > + return -EINVAL; > > + } > > + > > + return dtb_numa_processor_affinity_init(nid); > > +} > > > > -- > Julien Grall
On 20/08/2021 03:23, Wei Chen wrote: > Hi Julien, Hi Wei, >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Sent: 2021年8月20日 2:13 >> To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; >> sstabellini@kernel.org; jbeulich@suse.com >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com> >> Subject: Re: [XEN RFC PATCH 22/40] xen/arm: introduce a helper to parse >> device tree processor node >> >> Hi Wei, >> >> On 11/08/2021 11:24, Wei Chen wrote: >>> Processor NUMA ID information is stored in device tree's processor >>> node as "numa-node-id". We need a new helper to parse this ID from >>> processor node. If we get this ID from processor node, this ID's >>> validity still need to be checked. Once we got a invalid NUMA ID >>> from any processor node, the device tree will be marked as NUMA >>> information invalid. >>> >>> Signed-off-by: Wei Chen <wei.chen@arm.com> >>> --- >>> xen/arch/arm/numa_device_tree.c | 41 +++++++++++++++++++++++++++++++-- >>> 1 file changed, 39 insertions(+), 2 deletions(-) >>> >>> diff --git a/xen/arch/arm/numa_device_tree.c >> b/xen/arch/arm/numa_device_tree.c >>> index 1c74ad135d..37cc56acf3 100644 >>> --- a/xen/arch/arm/numa_device_tree.c >>> +++ b/xen/arch/arm/numa_device_tree.c >>> @@ -20,16 +20,53 @@ >>> #include <xen/init.h> >>> #include <xen/nodemask.h> >>> #include <xen/numa.h> >>> +#include <xen/device_tree.h> >> >> Nothing in this file seems to depend on xen/device_tree.h. So why do you >> need to include it? >> > > I remember that without this header file, device_tree_get_u32 in this patch > will cause compiling failed. I looked at the prototype of device_tree_get_u32() and I can't find how it depends on bits from device_tree.h. Can you paste the compilation error? > >>> +#include <asm/setup.h> >>> >>> s8 device_tree_numa = 0; >>> +static nodemask_t processor_nodes_parsed __initdata; >>> >>> -int srat_disabled(void) >>> +static int srat_disabled(void) >>> { >>> return numa_off || device_tree_numa < 0; >>> } >>> >>> -void __init bad_srat(void) >>> +static __init void bad_srat(void) >>> { >>> printk(KERN_ERR "DT: NUMA information is not used.\n"); >>> device_tree_numa = -1; >>> } >>> + >>> +/* Callback for device tree processor affinity */ >>> +static int __init dtb_numa_processor_affinity_init(nodeid_t node) >>> +{ >>> + if ( srat_disabled() ) >>> + return -EINVAL; >>> + else if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) { >>> + bad_srat(); >>> + return -EINVAL; >>> + } >>> + >>> + node_set(node, processor_nodes_parsed); >>> + >>> + device_tree_numa = 1; >>> + printk(KERN_INFO "DT: NUMA node %u processor parsed\n", node); >>> + >>> + return 0; >>> +} >>> + >>> +/* Parse CPU NUMA node info */ >>> +int __init device_tree_parse_numa_cpu_node(const void *fdt, int node) >>> +{ >>> + uint32_t nid; >>> + >>> + nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES); >>> + printk(XENLOG_WARNING "CPU on NUMA node:%u\n", nid); >>> + if ( nid >= MAX_NUMNODES ) >>> + { >>> + printk(XENLOG_WARNING "Node id %u exceeds maximum value\n", >> nid); >>> + return -EINVAL; >>> + } >>> + >>> + return dtb_numa_processor_affinity_init(nid); >>> +} >>> >> >> -- >> Julien Grall Cheers,
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: 2021年8月20日 16:44 > To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org; jbeulich@suse.com > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com> > Subject: Re: [XEN RFC PATCH 22/40] xen/arm: introduce a helper to parse > device tree processor node > > > > On 20/08/2021 03:23, Wei Chen wrote: > > Hi Julien, > > Hi Wei, > > >> -----Original Message----- > >> From: Julien Grall <julien@xen.org> > >> Sent: 2021年8月20日 2:13 > >> To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; > >> sstabellini@kernel.org; jbeulich@suse.com > >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com> > >> Subject: Re: [XEN RFC PATCH 22/40] xen/arm: introduce a helper to parse > >> device tree processor node > >> > >> Hi Wei, > >> > >> On 11/08/2021 11:24, Wei Chen wrote: > >>> Processor NUMA ID information is stored in device tree's processor > >>> node as "numa-node-id". We need a new helper to parse this ID from > >>> processor node. If we get this ID from processor node, this ID's > >>> validity still need to be checked. Once we got a invalid NUMA ID > >>> from any processor node, the device tree will be marked as NUMA > >>> information invalid. > >>> > >>> Signed-off-by: Wei Chen <wei.chen@arm.com> > >>> --- > >>> xen/arch/arm/numa_device_tree.c | 41 > +++++++++++++++++++++++++++++++-- > >>> 1 file changed, 39 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/xen/arch/arm/numa_device_tree.c > >> b/xen/arch/arm/numa_device_tree.c > >>> index 1c74ad135d..37cc56acf3 100644 > >>> --- a/xen/arch/arm/numa_device_tree.c > >>> +++ b/xen/arch/arm/numa_device_tree.c > >>> @@ -20,16 +20,53 @@ > >>> #include <xen/init.h> > >>> #include <xen/nodemask.h> > >>> #include <xen/numa.h> > >>> +#include <xen/device_tree.h> > >> > >> Nothing in this file seems to depend on xen/device_tree.h. So why do > you > >> need to include it? > >> > > > > I remember that without this header file, device_tree_get_u32 in this > patch > > will cause compiling failed. > > I looked at the prototype of device_tree_get_u32() and I can't find how > it depends on bits from device_tree.h. Can you paste the compilation error? > I tested it again, this header file should be introduced in following patches: numa_device_tree.c: In function ‘device_tree_parse_numa_distance_map_v1’: numa_device_tree.c:243:16: error: implicit declaration of function ‘dt_read_number’ [-Werror=implicit-function-declaration] 243 | from = dt_read_number(matrix, 1); | ^~~~~~~~~~~~~~ numa_device_tree.c:243:16: error: nested extern declaration of ‘dt_read_number’ [-Werror=nested-externs] cc1: all warnings being treated as errors I will move it to another patch. > > > >>> +#include <asm/setup.h> > >>> > >>> s8 device_tree_numa = 0; > >>> +static nodemask_t processor_nodes_parsed __initdata; > >>> > >>> -int srat_disabled(void) > >>> +static int srat_disabled(void) > >>> { > >>> return numa_off || device_tree_numa < 0; > >>> } > >>> > >>> -void __init bad_srat(void) > >>> +static __init void bad_srat(void) > >>> { > >>> printk(KERN_ERR "DT: NUMA information is not used.\n"); > >>> device_tree_numa = -1; > >>> } > >>> + > >>> +/* Callback for device tree processor affinity */ > >>> +static int __init dtb_numa_processor_affinity_init(nodeid_t node) > >>> +{ > >>> + if ( srat_disabled() ) > >>> + return -EINVAL; > >>> + else if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) { > >>> + bad_srat(); > >>> + return -EINVAL; > >>> + } > >>> + > >>> + node_set(node, processor_nodes_parsed); > >>> + > >>> + device_tree_numa = 1; > >>> + printk(KERN_INFO "DT: NUMA node %u processor parsed\n", node); > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +/* Parse CPU NUMA node info */ > >>> +int __init device_tree_parse_numa_cpu_node(const void *fdt, int node) > >>> +{ > >>> + uint32_t nid; > >>> + > >>> + nid = device_tree_get_u32(fdt, node, "numa-node-id", > MAX_NUMNODES); > >>> + printk(XENLOG_WARNING "CPU on NUMA node:%u\n", nid); > >>> + if ( nid >= MAX_NUMNODES ) > >>> + { > >>> + printk(XENLOG_WARNING "Node id %u exceeds maximum value\n", > >> nid); > >>> + return -EINVAL; > >>> + } > >>> + > >>> + return dtb_numa_processor_affinity_init(nid); > >>> +} > >>> > >> > >> -- > >> Julien Grall > > Cheers, > > -- > Julien Grall
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: 2021年8月20日 2:10 > To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org; jbeulich@suse.com > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com> > Subject: Re: [XEN RFC PATCH 22/40] xen/arm: introduce a helper to parse > device tree processor node > > Hi Wei, > > On 11/08/2021 11:24, Wei Chen wrote: > > Processor NUMA ID information is stored in device tree's processor > > node as "numa-node-id". We need a new helper to parse this ID from > > processor node. If we get this ID from processor node, this ID's > > validity still need to be checked. Once we got a invalid NUMA ID > > from any processor node, the device tree will be marked as NUMA > > information invalid. > > > > Signed-off-by: Wei Chen <wei.chen@arm.com> > > --- > > xen/arch/arm/numa_device_tree.c | 41 +++++++++++++++++++++++++++++++-- > > 1 file changed, 39 insertions(+), 2 deletions(-) > > > > diff --git a/xen/arch/arm/numa_device_tree.c > b/xen/arch/arm/numa_device_tree.c > > index 1c74ad135d..37cc56acf3 100644 > > --- a/xen/arch/arm/numa_device_tree.c > > +++ b/xen/arch/arm/numa_device_tree.c > > @@ -20,16 +20,53 @@ > > #include <xen/init.h> > > #include <xen/nodemask.h> > > #include <xen/numa.h> > > +#include <xen/device_tree.h> > > +#include <asm/setup.h> > > > > s8 device_tree_numa = 0; > > +static nodemask_t processor_nodes_parsed __initdata; > > > > -int srat_disabled(void) > > +static int srat_disabled(void) > > { > > return numa_off || device_tree_numa < 0; > > } > > > > -void __init bad_srat(void) > > +static __init void bad_srat(void) > > { > > printk(KERN_ERR "DT: NUMA information is not used.\n"); > > device_tree_numa = -1; > > } > > + > > +/* Callback for device tree processor affinity */ > > +static int __init dtb_numa_processor_affinity_init(nodeid_t node) > > +{ > > + if ( srat_disabled() ) > > + return -EINVAL; > > + else if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) { > > + bad_srat(); > > + return -EINVAL; > > You seem to have a mix of soft and hard tab in this file. Is there a lot > of the code that was directly copied from Linux? If not, then the file > should be using Xen coding style. > I copied some code from x86, and x86 is Linux style. So, yes, I should adjust them it Xen coding style. I will do it in next version. > > + } > > + > > + node_set(node, processor_nodes_parsed); > > + > > + device_tree_numa = 1; > > + printk(KERN_INFO "DT: NUMA node %u processor parsed\n", node); > > + > > + return 0; > > +} > > + > > +/* Parse CPU NUMA node info */ > > +int __init device_tree_parse_numa_cpu_node(const void *fdt, int node) > > AFAICT, you are going to turn this helper static in a follow-up patch. > This is a bad practice. Instead, the function should be static from the > beginning. If it is not possible, then you should re-order the code. > > In this case, I think you can add the boilerplate to parse the NUMA > information (patch #25) here and then extend it in each patch. > > That's make sense, I will try to address it in next version. > > +{ > > + uint32_t nid; > > + > > + nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES); > > + printk(XENLOG_WARNING "CPU on NUMA node:%u\n", nid); > > + if ( nid >= MAX_NUMNODES ) > > + { > > + printk(XENLOG_WARNING "Node id %u exceeds maximum value\n", > nid); > > + return -EINVAL; > > + } > > + > > + return dtb_numa_processor_affinity_init(nid); > > +} > > > > Cheers, > > -- > Julien Grall
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: 2021年8月20日 2:11 > To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org; jbeulich@suse.com > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com> > Subject: Re: [XEN RFC PATCH 22/40] xen/arm: introduce a helper to parse > device tree processor node > > On 11/08/2021 11:24, Wei Chen wrote: > > Processor NUMA ID information is stored in device tree's processor > > node as "numa-node-id". We need a new helper to parse this ID from > > processor node. If we get this ID from processor node, this ID's > > validity still need to be checked. Once we got a invalid NUMA ID > > from any processor node, the device tree will be marked as NUMA > > information invalid. > > > > Signed-off-by: Wei Chen <wei.chen@arm.com> > > --- > > xen/arch/arm/numa_device_tree.c | 41 +++++++++++++++++++++++++++++++-- > > 1 file changed, 39 insertions(+), 2 deletions(-) > > > > diff --git a/xen/arch/arm/numa_device_tree.c > b/xen/arch/arm/numa_device_tree.c > > index 1c74ad135d..37cc56acf3 100644 > > --- a/xen/arch/arm/numa_device_tree.c > > +++ b/xen/arch/arm/numa_device_tree.c > > @@ -20,16 +20,53 @@ > > #include <xen/init.h> > > #include <xen/nodemask.h> > > #include <xen/numa.h> > > +#include <xen/device_tree.h> > > +#include <asm/setup.h> > > > > s8 device_tree_numa = 0; > > +static nodemask_t processor_nodes_parsed __initdata; > > > > -int srat_disabled(void) > > +static int srat_disabled(void) > > { > > return numa_off || device_tree_numa < 0; > > } > > > > -void __init bad_srat(void) > > +static __init void bad_srat(void) > > { > > printk(KERN_ERR "DT: NUMA information is not used.\n"); > > device_tree_numa = -1; > > } > > + > > +/* Callback for device tree processor affinity */ > > +static int __init dtb_numa_processor_affinity_init(nodeid_t node) > > I forgot to answer. It seems odd that some of the function names start > with dtb_* while other starts device_tree_*. Any particular reason for > that difference of naming? > yes, in the very beginning, I want to keep device_tree_ prefix for functions that will handle dtb file. And use dtb_ prefix to replace acpi, to indicate, this function is device tree version numa implementation. If that's not the right reason, I will unify all prefix to device_tree_ in next version. How do you think about it? > > +{ > > + if ( srat_disabled() ) > > + return -EINVAL; > > + else if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) { > > + bad_srat(); > > + return -EINVAL; > > + } > > + > > + node_set(node, processor_nodes_parsed); > > + > > + device_tree_numa = 1; > > + printk(KERN_INFO "DT: NUMA node %u processor parsed\n", node); > > + > > + return 0; > > +} > > + > > +/* Parse CPU NUMA node info */ > > +int __init device_tree_parse_numa_cpu_node(const void *fdt, int node) > > +{ > > + uint32_t nid; > > + > > + nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES); > > + printk(XENLOG_WARNING "CPU on NUMA node:%u\n", nid); > > + if ( nid >= MAX_NUMNODES ) > > + { > > + printk(XENLOG_WARNING "Node id %u exceeds maximum value\n", > nid); > > + return -EINVAL; > > + } > > + > > + return dtb_numa_processor_affinity_init(nid); > > +} > > > > Cheers, > > -- > Julien Grall
On 23/08/2021 09:47, Wei Chen wrote: > Hi Julien, Hi Wei, >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Sent: 2021年8月20日 2:11 >> To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; >> sstabellini@kernel.org; jbeulich@suse.com >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com> >> Subject: Re: [XEN RFC PATCH 22/40] xen/arm: introduce a helper to parse >> device tree processor node >> >> On 11/08/2021 11:24, Wei Chen wrote: >>> Processor NUMA ID information is stored in device tree's processor >>> node as "numa-node-id". We need a new helper to parse this ID from >>> processor node. If we get this ID from processor node, this ID's >>> validity still need to be checked. Once we got a invalid NUMA ID >>> from any processor node, the device tree will be marked as NUMA >>> information invalid. >>> >>> Signed-off-by: Wei Chen <wei.chen@arm.com> >>> --- >>> xen/arch/arm/numa_device_tree.c | 41 +++++++++++++++++++++++++++++++-- >>> 1 file changed, 39 insertions(+), 2 deletions(-) >>> >>> diff --git a/xen/arch/arm/numa_device_tree.c >> b/xen/arch/arm/numa_device_tree.c >>> index 1c74ad135d..37cc56acf3 100644 >>> --- a/xen/arch/arm/numa_device_tree.c >>> +++ b/xen/arch/arm/numa_device_tree.c >>> @@ -20,16 +20,53 @@ >>> #include <xen/init.h> >>> #include <xen/nodemask.h> >>> #include <xen/numa.h> >>> +#include <xen/device_tree.h> >>> +#include <asm/setup.h> >>> >>> s8 device_tree_numa = 0; >>> +static nodemask_t processor_nodes_parsed __initdata; >>> >>> -int srat_disabled(void) >>> +static int srat_disabled(void) >>> { >>> return numa_off || device_tree_numa < 0; >>> } >>> >>> -void __init bad_srat(void) >>> +static __init void bad_srat(void) >>> { >>> printk(KERN_ERR "DT: NUMA information is not used.\n"); >>> device_tree_numa = -1; >>> } >>> + >>> +/* Callback for device tree processor affinity */ >>> +static int __init dtb_numa_processor_affinity_init(nodeid_t node) >> >> I forgot to answer. It seems odd that some of the function names start >> with dtb_* while other starts device_tree_*. Any particular reason for >> that difference of naming? >> > > yes, in the very beginning, I want to keep device_tree_ prefix for > functions that will handle dtb file. And use dtb_ prefix to replace > acpi, to indicate, this function is device tree version numa implementation. Thanks for the clarification. The difference between "dtb" and "device_tree" is quite subttle: the former refers to the binary while the latter refers to the format. Most of the readers are likely to infer they mean the same. So I think this will bring more confusion. > > If that's not the right reason, I will unify all prefix to device_tree_ > in next version. How do you think about it? AFAICT, your parsing functions will always start with "device_tree_parse_". I would prefer if the set replacing the ACPI helpers start with "device_tree_". If you are concern with the length of the function name, then I would suggest to prefix all the functions with "fdt" (We are dealing with the flattened DT after all) or "dt". Cheers,
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: 2021年8月23日 18:59 > To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com> > Subject: Re: [XEN RFC PATCH 22/40] xen/arm: introduce a helper to parse > device tree processor node > > > > On 23/08/2021 09:47, Wei Chen wrote: > > Hi Julien, > > Hi Wei, > > >> -----Original Message----- > >> From: Julien Grall <julien@xen.org> > >> Sent: 2021年8月20日 2:11 > >> To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; > >> sstabellini@kernel.org; jbeulich@suse.com > >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com> > >> Subject: Re: [XEN RFC PATCH 22/40] xen/arm: introduce a helper to parse > >> device tree processor node > >> > >> On 11/08/2021 11:24, Wei Chen wrote: > >>> Processor NUMA ID information is stored in device tree's processor > >>> node as "numa-node-id". We need a new helper to parse this ID from > >>> processor node. If we get this ID from processor node, this ID's > >>> validity still need to be checked. Once we got a invalid NUMA ID > >>> from any processor node, the device tree will be marked as NUMA > >>> information invalid. > >>> > >>> Signed-off-by: Wei Chen <wei.chen@arm.com> > >>> --- > >>> xen/arch/arm/numa_device_tree.c | 41 > +++++++++++++++++++++++++++++++-- > >>> 1 file changed, 39 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/xen/arch/arm/numa_device_tree.c > >> b/xen/arch/arm/numa_device_tree.c > >>> index 1c74ad135d..37cc56acf3 100644 > >>> --- a/xen/arch/arm/numa_device_tree.c > >>> +++ b/xen/arch/arm/numa_device_tree.c > >>> @@ -20,16 +20,53 @@ > >>> #include <xen/init.h> > >>> #include <xen/nodemask.h> > >>> #include <xen/numa.h> > >>> +#include <xen/device_tree.h> > >>> +#include <asm/setup.h> > >>> > >>> s8 device_tree_numa = 0; > >>> +static nodemask_t processor_nodes_parsed __initdata; > >>> > >>> -int srat_disabled(void) > >>> +static int srat_disabled(void) > >>> { > >>> return numa_off || device_tree_numa < 0; > >>> } > >>> > >>> -void __init bad_srat(void) > >>> +static __init void bad_srat(void) > >>> { > >>> printk(KERN_ERR "DT: NUMA information is not used.\n"); > >>> device_tree_numa = -1; > >>> } > >>> + > >>> +/* Callback for device tree processor affinity */ > >>> +static int __init dtb_numa_processor_affinity_init(nodeid_t node) > >> > >> I forgot to answer. It seems odd that some of the function names start > >> with dtb_* while other starts device_tree_*. Any particular reason for > >> that difference of naming? > >> > > > > yes, in the very beginning, I want to keep device_tree_ prefix for > > functions that will handle dtb file. And use dtb_ prefix to replace > > acpi, to indicate, this function is device tree version numa > implementation. > > Thanks for the clarification. The difference between "dtb" and > "device_tree" is quite subttle: the former refers to the binary while > the latter refers to the format. Most of the readers are likely to infer > they mean the same. So I think this will bring more confusion. > Thanks for the clarification. > > > > If that's not the right reason, I will unify all prefix to device_tree_ > > in next version. How do you think about it? > > AFAICT, your parsing functions will always start with > "device_tree_parse_". I would prefer if the set replacing the ACPI > helpers start with "device_tree_". > > If you are concern with the length of the function name, then I would > suggest to prefix all the functions with "fdt" (We are dealing with the > flattened DT after all) or "dt". That makes sense, I will do it. > > Cheers, > > -- > Julien Grall
On Wed, 11 Aug 2021, Wei Chen wrote: > Processor NUMA ID information is stored in device tree's processor > node as "numa-node-id". We need a new helper to parse this ID from > processor node. If we get this ID from processor node, this ID's > validity still need to be checked. Once we got a invalid NUMA ID > from any processor node, the device tree will be marked as NUMA > information invalid. > > Signed-off-by: Wei Chen <wei.chen@arm.com> > --- > xen/arch/arm/numa_device_tree.c | 41 +++++++++++++++++++++++++++++++-- > 1 file changed, 39 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/numa_device_tree.c b/xen/arch/arm/numa_device_tree.c > index 1c74ad135d..37cc56acf3 100644 > --- a/xen/arch/arm/numa_device_tree.c > +++ b/xen/arch/arm/numa_device_tree.c > @@ -20,16 +20,53 @@ > #include <xen/init.h> > #include <xen/nodemask.h> > #include <xen/numa.h> > +#include <xen/device_tree.h> > +#include <asm/setup.h> > > s8 device_tree_numa = 0; > +static nodemask_t processor_nodes_parsed __initdata; > > -int srat_disabled(void) > +static int srat_disabled(void) > { > return numa_off || device_tree_numa < 0; > } > > -void __init bad_srat(void) > +static __init void bad_srat(void) > { > printk(KERN_ERR "DT: NUMA information is not used.\n"); > device_tree_numa = -1; > } > + > +/* Callback for device tree processor affinity */ > +static int __init dtb_numa_processor_affinity_init(nodeid_t node) > +{ > + if ( srat_disabled() ) > + return -EINVAL; > + else if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) { > + bad_srat(); > + return -EINVAL; > + } > + > + node_set(node, processor_nodes_parsed); > + > + device_tree_numa = 1; > + printk(KERN_INFO "DT: NUMA node %u processor parsed\n", node); > + > + return 0; > +} > + > +/* Parse CPU NUMA node info */ > +int __init device_tree_parse_numa_cpu_node(const void *fdt, int node) > +{ > + uint32_t nid; > + > + nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES); > + printk(XENLOG_WARNING "CPU on NUMA node:%u\n", nid); Given that this is not actually a warning (is it?) then I would move it to XENLOG_INFO > + if ( nid >= MAX_NUMNODES ) > + { > + printk(XENLOG_WARNING "Node id %u exceeds maximum value\n", nid); This could be XENLOG_ERR > + return -EINVAL; > + } > + > + return dtb_numa_processor_affinity_init(nid); > +}
Hi Stefano, > -----Original Message----- > From: Stefano Stabellini <sstabellini@kernel.org> > Sent: 2021年8月27日 8:06 > To: Wei Chen <Wei.Chen@arm.com> > Cc: xen-devel@lists.xenproject.org; sstabellini@kernel.org; julien@xen.org; > jbeulich@suse.com; Bertrand Marquis <Bertrand.Marquis@arm.com> > Subject: Re: [XEN RFC PATCH 22/40] xen/arm: introduce a helper to parse > device tree processor node > > On Wed, 11 Aug 2021, Wei Chen wrote: > > Processor NUMA ID information is stored in device tree's processor > > node as "numa-node-id". We need a new helper to parse this ID from > > processor node. If we get this ID from processor node, this ID's > > validity still need to be checked. Once we got a invalid NUMA ID > > from any processor node, the device tree will be marked as NUMA > > information invalid. > > > > Signed-off-by: Wei Chen <wei.chen@arm.com> > > --- > > xen/arch/arm/numa_device_tree.c | 41 +++++++++++++++++++++++++++++++-- > > 1 file changed, 39 insertions(+), 2 deletions(-) > > > > diff --git a/xen/arch/arm/numa_device_tree.c > b/xen/arch/arm/numa_device_tree.c > > index 1c74ad135d..37cc56acf3 100644 > > --- a/xen/arch/arm/numa_device_tree.c > > +++ b/xen/arch/arm/numa_device_tree.c > > @@ -20,16 +20,53 @@ > > #include <xen/init.h> > > #include <xen/nodemask.h> > > #include <xen/numa.h> > > +#include <xen/device_tree.h> > > +#include <asm/setup.h> > > > > s8 device_tree_numa = 0; > > +static nodemask_t processor_nodes_parsed __initdata; > > > > -int srat_disabled(void) > > +static int srat_disabled(void) > > { > > return numa_off || device_tree_numa < 0; > > } > > > > -void __init bad_srat(void) > > +static __init void bad_srat(void) > > { > > printk(KERN_ERR "DT: NUMA information is not used.\n"); > > device_tree_numa = -1; > > } > > + > > +/* Callback for device tree processor affinity */ > > +static int __init dtb_numa_processor_affinity_init(nodeid_t node) > > +{ > > + if ( srat_disabled() ) > > + return -EINVAL; > > + else if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) { > > + bad_srat(); > > + return -EINVAL; > > + } > > + > > + node_set(node, processor_nodes_parsed); > > + > > + device_tree_numa = 1; > > + printk(KERN_INFO "DT: NUMA node %u processor parsed\n", node); > > + > > + return 0; > > +} > > + > > +/* Parse CPU NUMA node info */ > > +int __init device_tree_parse_numa_cpu_node(const void *fdt, int node) > > +{ > > + uint32_t nid; > > + > > + nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES); > > + printk(XENLOG_WARNING "CPU on NUMA node:%u\n", nid); > > Given that this is not actually a warning (is it?) then I would move it > to XENLOG_INFO > > OK > > + if ( nid >= MAX_NUMNODES ) > > + { > > + printk(XENLOG_WARNING "Node id %u exceeds maximum value\n", > nid); > > This could be XENLOG_ERR > OK > > > + return -EINVAL; > > + } > > + > > + return dtb_numa_processor_affinity_init(nid); > > +}
diff --git a/xen/arch/arm/numa_device_tree.c b/xen/arch/arm/numa_device_tree.c index 1c74ad135d..37cc56acf3 100644 --- a/xen/arch/arm/numa_device_tree.c +++ b/xen/arch/arm/numa_device_tree.c @@ -20,16 +20,53 @@ #include <xen/init.h> #include <xen/nodemask.h> #include <xen/numa.h> +#include <xen/device_tree.h> +#include <asm/setup.h> s8 device_tree_numa = 0; +static nodemask_t processor_nodes_parsed __initdata; -int srat_disabled(void) +static int srat_disabled(void) { return numa_off || device_tree_numa < 0; } -void __init bad_srat(void) +static __init void bad_srat(void) { printk(KERN_ERR "DT: NUMA information is not used.\n"); device_tree_numa = -1; } + +/* Callback for device tree processor affinity */ +static int __init dtb_numa_processor_affinity_init(nodeid_t node) +{ + if ( srat_disabled() ) + return -EINVAL; + else if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) { + bad_srat(); + return -EINVAL; + } + + node_set(node, processor_nodes_parsed); + + device_tree_numa = 1; + printk(KERN_INFO "DT: NUMA node %u processor parsed\n", node); + + return 0; +} + +/* Parse CPU NUMA node info */ +int __init device_tree_parse_numa_cpu_node(const void *fdt, int node) +{ + uint32_t nid; + + nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES); + printk(XENLOG_WARNING "CPU on NUMA node:%u\n", nid); + if ( nid >= MAX_NUMNODES ) + { + printk(XENLOG_WARNING "Node id %u exceeds maximum value\n", nid); + return -EINVAL; + } + + return dtb_numa_processor_affinity_init(nid); +}
Processor NUMA ID information is stored in device tree's processor node as "numa-node-id". We need a new helper to parse this ID from processor node. If we get this ID from processor node, this ID's validity still need to be checked. Once we got a invalid NUMA ID from any processor node, the device tree will be marked as NUMA information invalid. Signed-off-by: Wei Chen <wei.chen@arm.com> --- xen/arch/arm/numa_device_tree.c | 41 +++++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-)