| Message ID | 20220418090735.3940393-6-wei.chen@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Device tree based NUMA support for Arm - Part#1 | expand |
On Mon, 18 Apr 2022, Wei Chen wrote: > VIRTUAL_BUG_ON is an empty macro used in phys_to_nid. This > results in two lines of error-checking code in phys_to_nid > that is not actually working and causing two compilation > errors: > 1. error: "MAX_NUMNODES" undeclared (first use in this function). > This is because in the common header file, "MAX_NUMNODES" is > defined after the common header file includes the ARCH header > file, where phys_to_nid has attempted to use "MAX_NUMNODES". > This error was resolved when we moved the definition of > "MAX_NUMNODES" to x86 ARCH header file. And we reserve the > "MAX_NUMNODES" definition in common header file through a > conditional compilation for some architectures that don't > need to define "MAX_NUMNODES" in their ARCH header files. > 2. error: wrong type argument to unary exclamation mark. > This is because, the error-checking code contains !node_data[nid]. > But node_data is a data structure variable, it's not a pointer. > > So, in this patch, we use ASSERT instead of VIRTUAL_BUG_ON to > enable the two lines of error-checking code. And fix the left > compilation errors by replacing !node_data[nid] to > !node_data[nid].node_spanned_pages. > > Because when node_spanned_pages is 0, this node has no memory, > numa_scan_node will print warning message for such kind of nodes: > "Firmware Bug or mis-configured hardware?". > > Signed-off-by: Wei Chen <wei.chen@arm.com> This patch looks OK to me but the x86 changes would benefit from a review from one of the x86 maintainers > --- > v1 -> v2: > 1. Use ASSERT to replace VIRTUAL_BUG_ON in phys_to_nid. > 2. Adjust the conditional express for ASSERT. > 3. Move MAX_NUMNODES from xen/numa.h to asm/numa.h for x86. > 4. Use conditional macro to gate MAX_NUMNODES for other architectures. > --- > xen/arch/x86/include/asm/numa.h | 6 +++--- > xen/include/xen/numa.h | 2 ++ > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/x86/include/asm/numa.h b/xen/arch/x86/include/asm/numa.h > index bada2c0bb9..1f268ce77d 100644 > --- a/xen/arch/x86/include/asm/numa.h > +++ b/xen/arch/x86/include/asm/numa.h > @@ -4,6 +4,7 @@ > #include <xen/cpumask.h> > > #define NODES_SHIFT 6 > +#define MAX_NUMNODES (1 << NODES_SHIFT) > > typedef u8 nodeid_t; > > @@ -26,7 +27,6 @@ extern int compute_hash_shift(struct node *nodes, int numnodes, > extern nodeid_t pxm_to_node(unsigned int pxm); > > #define ZONE_ALIGN (1UL << (MAX_ORDER+PAGE_SHIFT)) > -#define VIRTUAL_BUG_ON(x) > > extern void numa_add_cpu(int cpu); > extern void numa_init_array(void); > @@ -62,9 +62,9 @@ extern struct node_data node_data[]; > static inline __attribute__((pure)) nodeid_t phys_to_nid(paddr_t addr) > { > nodeid_t nid; > - VIRTUAL_BUG_ON((paddr_to_pdx(addr) >> memnode_shift) >= memnodemapsize); > + ASSERT((paddr_to_pdx(addr) >> memnode_shift) < memnodemapsize); > nid = memnodemap[paddr_to_pdx(addr) >> memnode_shift]; > - VIRTUAL_BUG_ON(nid >= MAX_NUMNODES || !node_data[nid]); > + ASSERT(nid < MAX_NUMNODES && node_data[nid].node_spanned_pages); > return nid; > } > > diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h > index 7aef1a88dc..91b25c5617 100644 > --- a/xen/include/xen/numa.h > +++ b/xen/include/xen/numa.h > @@ -10,7 +10,9 @@ > #define NUMA_NO_NODE 0xFF > #define NUMA_NO_DISTANCE 0xFF > > +#ifndef MAX_NUMNODES > #define MAX_NUMNODES (1 << NODES_SHIFT) > +#endif > > #define vcpu_to_node(v) (cpu_to_node((v)->processor)) > > -- > 2.25.1 >
On 18.04.2022 11:07, Wei Chen wrote: > VIRTUAL_BUG_ON is an empty macro used in phys_to_nid. This > results in two lines of error-checking code in phys_to_nid > that is not actually working and causing two compilation > errors: > 1. error: "MAX_NUMNODES" undeclared (first use in this function). > This is because in the common header file, "MAX_NUMNODES" is > defined after the common header file includes the ARCH header > file, where phys_to_nid has attempted to use "MAX_NUMNODES". > This error was resolved when we moved the definition of > "MAX_NUMNODES" to x86 ARCH header file. And we reserve the > "MAX_NUMNODES" definition in common header file through a > conditional compilation for some architectures that don't > need to define "MAX_NUMNODES" in their ARCH header files. No, that's setting up a trap for someone else to fall into, especially with the #ifdef around the original definition. Afaict all you need to do is to move that #define ahead of the #include in xen/numa.h. Unlike functions, #define-s can reference not-yet-defined identifiers. > 2. error: wrong type argument to unary exclamation mark. > This is because, the error-checking code contains !node_data[nid]. > But node_data is a data structure variable, it's not a pointer. > > So, in this patch, we use ASSERT instead of VIRTUAL_BUG_ON to > enable the two lines of error-checking code. And fix the left > compilation errors by replacing !node_data[nid] to > !node_data[nid].node_spanned_pages. > > Because when node_spanned_pages is 0, this node has no memory, > numa_scan_node will print warning message for such kind of nodes: > "Firmware Bug or mis-configured hardware?". This warning is bogus - nodes can have only processors. Therefore I'd like to ask that you don't use it for justification. And indeed you don't need to: phys_to_nid() is about translating an address. The input address can't be valid if it maps to a node with no memory. Jan
Hi Jan, On 2022/4/26 17:02, Jan Beulich wrote: > On 18.04.2022 11:07, Wei Chen wrote: >> VIRTUAL_BUG_ON is an empty macro used in phys_to_nid. This >> results in two lines of error-checking code in phys_to_nid >> that is not actually working and causing two compilation >> errors: >> 1. error: "MAX_NUMNODES" undeclared (first use in this function). >> This is because in the common header file, "MAX_NUMNODES" is >> defined after the common header file includes the ARCH header >> file, where phys_to_nid has attempted to use "MAX_NUMNODES". >> This error was resolved when we moved the definition of >> "MAX_NUMNODES" to x86 ARCH header file. And we reserve the >> "MAX_NUMNODES" definition in common header file through a >> conditional compilation for some architectures that don't >> need to define "MAX_NUMNODES" in their ARCH header files. > > No, that's setting up a trap for someone else to fall into, especially > with the #ifdef around the original definition. Afaict all you need to > do is to move that #define ahead of the #include in xen/numa.h. Unlike > functions, #define-s can reference not-yet-defined identifiers. > I had tried it before. MAX_NUMNODES depends on NODE_SHIFT. But NODE_SHIFT depends on the definition status in asm/numa.h. If I move MAX_NUMNODES to before asm/numa.h, then I have to move NODES_SHIFT as well. But this will break the original design. NODES_SHIFT in xen/numa.h will always be defined before asm/numa.h. This will be a duplicated definition error. How about I move MAX_NUMNODES to arm and x86 asm/numa.h in this patch at the same time? Because in one of following patches, MAX_NUMNODES and phys_to_nid will be moved to xen/numa.h at the same time? >> 2. error: wrong type argument to unary exclamation mark. >> This is because, the error-checking code contains !node_data[nid]. >> But node_data is a data structure variable, it's not a pointer. >> >> So, in this patch, we use ASSERT instead of VIRTUAL_BUG_ON to >> enable the two lines of error-checking code. And fix the left >> compilation errors by replacing !node_data[nid] to >> !node_data[nid].node_spanned_pages. >> >> Because when node_spanned_pages is 0, this node has no memory, >> numa_scan_node will print warning message for such kind of nodes: >> "Firmware Bug or mis-configured hardware?". > > This warning is bogus - nodes can have only processors. Therefore I'd > like to ask that you don't use it for justification. And indeed you Yes, you're right, node can only has CPUs! I will remove it. > don't need to: phys_to_nid() is about translating an address. The > input address can't be valid if it maps to a node with no memory. > Can I understand your comment: Any input address is invalid, when node_spanned_pages is zero, because this node has no memory? Thanks, Wei Chen > Jan >
On 26.04.2022 12:59, Wei Chen wrote: > On 2022/4/26 17:02, Jan Beulich wrote: >> On 18.04.2022 11:07, Wei Chen wrote: >>> VIRTUAL_BUG_ON is an empty macro used in phys_to_nid. This >>> results in two lines of error-checking code in phys_to_nid >>> that is not actually working and causing two compilation >>> errors: >>> 1. error: "MAX_NUMNODES" undeclared (first use in this function). >>> This is because in the common header file, "MAX_NUMNODES" is >>> defined after the common header file includes the ARCH header >>> file, where phys_to_nid has attempted to use "MAX_NUMNODES". >>> This error was resolved when we moved the definition of >>> "MAX_NUMNODES" to x86 ARCH header file. And we reserve the >>> "MAX_NUMNODES" definition in common header file through a >>> conditional compilation for some architectures that don't >>> need to define "MAX_NUMNODES" in their ARCH header files. >> >> No, that's setting up a trap for someone else to fall into, especially >> with the #ifdef around the original definition. Afaict all you need to >> do is to move that #define ahead of the #include in xen/numa.h. Unlike >> functions, #define-s can reference not-yet-defined identifiers. >> > > I had tried it before. MAX_NUMNODES depends on NODE_SHIFT. But > NODE_SHIFT depends on the definition status in asm/numa.h. If I move > MAX_NUMNODES to before asm/numa.h, then I have to move NODES_SHIFT as > well. But this will break the original design. NODES_SHIFT in xen/numa.h > will always be defined before asm/numa.h. This will be a duplicated > definition error. I'm afraid I don't follow. MAX_NUMNODES depends on NODES_SHIFT only as soon as some code actually uses MAX_NUMNODES. It does not require NODES_SHIFT to be defined up front. Of course with the current layout (phys_to_nid() living in an inline function in asm/numa.h) things won't build. But wasn't the plan to move phys_to_nid() to xen/numa.h as well? Otherwise I'd recommend to introduce a new header, say numa-defs.h, holding (for now) just NODES_SHIFT. Then you'd include asm/numa-defs.h first and asm/numa.h only after having defined MAX_NUMNODES. But splitting the header should only be a last resort if things can't be made work another way. > How about I move MAX_NUMNODES to arm and x86 asm/numa.h in this patch > at the same time? Because in one of following patches, MAX_NUMNODES and > phys_to_nid will be moved to xen/numa.h at the same time? > >>> 2. error: wrong type argument to unary exclamation mark. >>> This is because, the error-checking code contains !node_data[nid]. >>> But node_data is a data structure variable, it's not a pointer. >>> >>> So, in this patch, we use ASSERT instead of VIRTUAL_BUG_ON to >>> enable the two lines of error-checking code. And fix the left >>> compilation errors by replacing !node_data[nid] to >>> !node_data[nid].node_spanned_pages. >>> >>> Because when node_spanned_pages is 0, this node has no memory, >>> numa_scan_node will print warning message for such kind of nodes: >>> "Firmware Bug or mis-configured hardware?". >> >> This warning is bogus - nodes can have only processors. Therefore I'd >> like to ask that you don't use it for justification. And indeed you > > Yes, you're right, node can only has CPUs! I will remove it. > >> don't need to: phys_to_nid() is about translating an address. The >> input address can't be valid if it maps to a node with no memory. >> > > Can I understand your comment: > Any input address is invalid, when node_spanned_pages is zero, because > this node has no memory? It's getting close, but it's not exactly equivalent I think. A node with 0 bytes of memory might (at least in theory) have an entry in memnodemap[]. But finding a node ID for that address would still not mean that at least one byte of memory at that address is present on the given node, because the node covers 0 bytes. Jan
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 2022年4月26日 22:42 > To: Wei Chen <Wei.Chen@arm.com> > Cc: nd <nd@arm.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Roger Pau > Monné <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; George Dunlap > <george.dunlap@citrix.com>; Julien Grall <julien@xen.org>; Stefano > Stabellini <sstabellini@kernel.org>; xen-devel@lists.xenproject.org > Subject: Re: [PATCH v2 05/10] xen/x86: Use ASSERT instead of > VIRTUAL_BUG_ON for phys_to_nid > > On 26.04.2022 12:59, Wei Chen wrote: > > On 2022/4/26 17:02, Jan Beulich wrote: > >> On 18.04.2022 11:07, Wei Chen wrote: > >>> VIRTUAL_BUG_ON is an empty macro used in phys_to_nid. This > >>> results in two lines of error-checking code in phys_to_nid > >>> that is not actually working and causing two compilation > >>> errors: > >>> 1. error: "MAX_NUMNODES" undeclared (first use in this function). > >>> This is because in the common header file, "MAX_NUMNODES" is > >>> defined after the common header file includes the ARCH header > >>> file, where phys_to_nid has attempted to use "MAX_NUMNODES". > >>> This error was resolved when we moved the definition of > >>> "MAX_NUMNODES" to x86 ARCH header file. And we reserve the > >>> "MAX_NUMNODES" definition in common header file through a > >>> conditional compilation for some architectures that don't > >>> need to define "MAX_NUMNODES" in their ARCH header files. > >> > >> No, that's setting up a trap for someone else to fall into, especially > >> with the #ifdef around the original definition. Afaict all you need to > >> do is to move that #define ahead of the #include in xen/numa.h. Unlike > >> functions, #define-s can reference not-yet-defined identifiers. > >> > > > > I had tried it before. MAX_NUMNODES depends on NODE_SHIFT. But > > NODE_SHIFT depends on the definition status in asm/numa.h. If I move > > MAX_NUMNODES to before asm/numa.h, then I have to move NODES_SHIFT as > > well. But this will break the original design. NODES_SHIFT in xen/numa.h > > will always be defined before asm/numa.h. This will be a duplicated > > definition error. > > I'm afraid I don't follow. MAX_NUMNODES depends on NODES_SHIFT only as > soon as some code actually uses MAX_NUMNODES. It does not require > NODES_SHIFT to be defined up front. Of course with the current layout > (phys_to_nid() living in an inline function in asm/numa.h) things won't > build. But wasn't the plan to move phys_to_nid() to xen/numa.h as well? > Yes, I will drop this patch from part#1, and move it to part#2. This patch will follow when we move phys_to_nid() to xen/numa.h. Thanks, Wei Chen > Otherwise I'd recommend to introduce a new header, say numa-defs.h, > holding (for now) just NODES_SHIFT. Then you'd include asm/numa-defs.h > first and asm/numa.h only after having defined MAX_NUMNODES. But > splitting the header should only be a last resort if things can't be > made work another way. > > > How about I move MAX_NUMNODES to arm and x86 asm/numa.h in this patch > > at the same time? Because in one of following patches, MAX_NUMNODES and > > phys_to_nid will be moved to xen/numa.h at the same time? > > > >>> 2. error: wrong type argument to unary exclamation mark. > >>> This is because, the error-checking code contains !node_data[nid]. > >>> But node_data is a data structure variable, it's not a pointer. > >>> > >>> So, in this patch, we use ASSERT instead of VIRTUAL_BUG_ON to > >>> enable the two lines of error-checking code. And fix the left > >>> compilation errors by replacing !node_data[nid] to > >>> !node_data[nid].node_spanned_pages. > >>> > >>> Because when node_spanned_pages is 0, this node has no memory, > >>> numa_scan_node will print warning message for such kind of nodes: > >>> "Firmware Bug or mis-configured hardware?". > >> > >> This warning is bogus - nodes can have only processors. Therefore I'd > >> like to ask that you don't use it for justification. And indeed you > > > > Yes, you're right, node can only has CPUs! I will remove it. > > > >> don't need to: phys_to_nid() is about translating an address. The > >> input address can't be valid if it maps to a node with no memory. > >> > > > > Can I understand your comment: > > Any input address is invalid, when node_spanned_pages is zero, because > > this node has no memory? > > It's getting close, but it's not exactly equivalent I think. A node > with 0 bytes of memory might (at least in theory) have an entry in > memnodemap[]. But finding a node ID for that address would still I have done a quick check in populate_memnodemap: 74 spdx = paddr_to_pdx(nodes[i].start); 75 epdx = paddr_to_pdx(nodes[i].end - 1) + 1; 76 if ( spdx >= epdx ) 77 continue; It seems that if node has no memory, start == end, then this function will not populate memnodemap entry for this node. > not mean that at least one byte of memory at that address is present > on the given node, because the node covers 0 bytes. > And back to this patch, can I just drop the unnecessary justification from the commit message? And for the bogus warning message, can I update it to an INFO level message in part#2 series, and just keep: printk(KERN_INFO "SRAT: Node %u has no memory!\n", i); but remove "BIOS Bug or mis-configured hardware?\n", i); ? Thanks, Wei Chen > Jan
On 27.04.2022 05:52, Wei Chen wrote: >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 2022年4月26日 22:42 >> >> On 26.04.2022 12:59, Wei Chen wrote: >>> On 2022/4/26 17:02, Jan Beulich wrote: >>>> On 18.04.2022 11:07, Wei Chen wrote: >>>>> 2. error: wrong type argument to unary exclamation mark. >>>>> This is because, the error-checking code contains !node_data[nid]. >>>>> But node_data is a data structure variable, it's not a pointer. >>>>> >>>>> So, in this patch, we use ASSERT instead of VIRTUAL_BUG_ON to >>>>> enable the two lines of error-checking code. And fix the left >>>>> compilation errors by replacing !node_data[nid] to >>>>> !node_data[nid].node_spanned_pages. >>>>> >>>>> Because when node_spanned_pages is 0, this node has no memory, >>>>> numa_scan_node will print warning message for such kind of nodes: >>>>> "Firmware Bug or mis-configured hardware?". >>>> >>>> This warning is bogus - nodes can have only processors. Therefore I'd >>>> like to ask that you don't use it for justification. And indeed you >>> >>> Yes, you're right, node can only has CPUs! I will remove it. >>> >>>> don't need to: phys_to_nid() is about translating an address. The >>>> input address can't be valid if it maps to a node with no memory. >>>> >>> >>> Can I understand your comment: >>> Any input address is invalid, when node_spanned_pages is zero, because >>> this node has no memory? >> >> It's getting close, but it's not exactly equivalent I think. A node >> with 0 bytes of memory might (at least in theory) have an entry in >> memnodemap[]. But finding a node ID for that address would still > > I have done a quick check in populate_memnodemap: > 74 spdx = paddr_to_pdx(nodes[i].start); > 75 epdx = paddr_to_pdx(nodes[i].end - 1) + 1; > 76 if ( spdx >= epdx ) > 77 continue; > > It seems that if node has no memory, start == end, then this function > will not populate memnodemap entry for this node. > >> not mean that at least one byte of memory at that address is present >> on the given node, because the node covers 0 bytes. >> > > And back to this patch, can I just drop the unnecessary justification > from the commit message? Well, you'll want to have _some_ justification for that particular aspect of the patch. > And for the bogus warning message, can I update it to an INFO level > message in part#2 series, and just keep: > printk(KERN_INFO "SRAT: Node %u has no memory!\n", i); > but remove "BIOS Bug or mis-configured hardware?\n", i); ? This sounds at least plausible to do. Jan
Hi Jan, On 2022/4/27 13:56, Jan Beulich wrote: > On 27.04.2022 05:52, Wei Chen wrote: >>> From: Jan Beulich <jbeulich@suse.com> >>> Sent: 2022年4月26日 22:42 >>> >>> On 26.04.2022 12:59, Wei Chen wrote: >>>> On 2022/4/26 17:02, Jan Beulich wrote: >>>>> On 18.04.2022 11:07, Wei Chen wrote: >>>>>> 2. error: wrong type argument to unary exclamation mark. >>>>>> This is because, the error-checking code contains !node_data[nid]. >>>>>> But node_data is a data structure variable, it's not a pointer. >>>>>> >>>>>> So, in this patch, we use ASSERT instead of VIRTUAL_BUG_ON to >>>>>> enable the two lines of error-checking code. And fix the left >>>>>> compilation errors by replacing !node_data[nid] to >>>>>> !node_data[nid].node_spanned_pages. >>>>>> >>>>>> Because when node_spanned_pages is 0, this node has no memory, >>>>>> numa_scan_node will print warning message for such kind of nodes: >>>>>> "Firmware Bug or mis-configured hardware?". >>>>> >>>>> This warning is bogus - nodes can have only processors. Therefore I'd >>>>> like to ask that you don't use it for justification. And indeed you >>>> >>>> Yes, you're right, node can only has CPUs! I will remove it. >>>> >>>>> don't need to: phys_to_nid() is about translating an address. The >>>>> input address can't be valid if it maps to a node with no memory. >>>>> >>>> >>>> Can I understand your comment: >>>> Any input address is invalid, when node_spanned_pages is zero, because >>>> this node has no memory? >>> >>> It's getting close, but it's not exactly equivalent I think. A node >>> with 0 bytes of memory might (at least in theory) have an entry in >>> memnodemap[]. But finding a node ID for that address would still >> >> I have done a quick check in populate_memnodemap: >> 74 spdx = paddr_to_pdx(nodes[i].start); >> 75 epdx = paddr_to_pdx(nodes[i].end - 1) + 1; >> 76 if ( spdx >= epdx ) >> 77 continue; >> >> It seems that if node has no memory, start == end, then this function >> will not populate memnodemap entry for this node. >> >>> not mean that at least one byte of memory at that address is present >>> on the given node, because the node covers 0 bytes. >>> >> >> And back to this patch, can I just drop the unnecessary justification >> from the commit message? > > Well, you'll want to have _some_ justification for that particular > aspect of the patch. > Ok. >> And for the bogus warning message, can I update it to an INFO level >> message in part#2 series, and just keep: >> printk(KERN_INFO "SRAT: Node %u has no memory!\n", i); >> but remove "BIOS Bug or mis-configured hardware?\n", i); ? > > This sounds at least plausible to do. > Ok. > Jan >
diff --git a/xen/arch/x86/include/asm/numa.h b/xen/arch/x86/include/asm/numa.h index bada2c0bb9..1f268ce77d 100644 --- a/xen/arch/x86/include/asm/numa.h +++ b/xen/arch/x86/include/asm/numa.h @@ -4,6 +4,7 @@ #include <xen/cpumask.h> #define NODES_SHIFT 6 +#define MAX_NUMNODES (1 << NODES_SHIFT) typedef u8 nodeid_t; @@ -26,7 +27,6 @@ extern int compute_hash_shift(struct node *nodes, int numnodes, extern nodeid_t pxm_to_node(unsigned int pxm); #define ZONE_ALIGN (1UL << (MAX_ORDER+PAGE_SHIFT)) -#define VIRTUAL_BUG_ON(x) extern void numa_add_cpu(int cpu); extern void numa_init_array(void); @@ -62,9 +62,9 @@ extern struct node_data node_data[]; static inline __attribute__((pure)) nodeid_t phys_to_nid(paddr_t addr) { nodeid_t nid; - VIRTUAL_BUG_ON((paddr_to_pdx(addr) >> memnode_shift) >= memnodemapsize); + ASSERT((paddr_to_pdx(addr) >> memnode_shift) < memnodemapsize); nid = memnodemap[paddr_to_pdx(addr) >> memnode_shift]; - VIRTUAL_BUG_ON(nid >= MAX_NUMNODES || !node_data[nid]); + ASSERT(nid < MAX_NUMNODES && node_data[nid].node_spanned_pages); return nid; } diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h index 7aef1a88dc..91b25c5617 100644 --- a/xen/include/xen/numa.h +++ b/xen/include/xen/numa.h @@ -10,7 +10,9 @@ #define NUMA_NO_NODE 0xFF #define NUMA_NO_DISTANCE 0xFF +#ifndef MAX_NUMNODES #define MAX_NUMNODES (1 << NODES_SHIFT) +#endif #define vcpu_to_node(v) (cpu_to_node((v)->processor))
VIRTUAL_BUG_ON is an empty macro used in phys_to_nid. This results in two lines of error-checking code in phys_to_nid that is not actually working and causing two compilation errors: 1. error: "MAX_NUMNODES" undeclared (first use in this function). This is because in the common header file, "MAX_NUMNODES" is defined after the common header file includes the ARCH header file, where phys_to_nid has attempted to use "MAX_NUMNODES". This error was resolved when we moved the definition of "MAX_NUMNODES" to x86 ARCH header file. And we reserve the "MAX_NUMNODES" definition in common header file through a conditional compilation for some architectures that don't need to define "MAX_NUMNODES" in their ARCH header files. 2. error: wrong type argument to unary exclamation mark. This is because, the error-checking code contains !node_data[nid]. But node_data is a data structure variable, it's not a pointer. So, in this patch, we use ASSERT instead of VIRTUAL_BUG_ON to enable the two lines of error-checking code. And fix the left compilation errors by replacing !node_data[nid] to !node_data[nid].node_spanned_pages. Because when node_spanned_pages is 0, this node has no memory, numa_scan_node will print warning message for such kind of nodes: "Firmware Bug or mis-configured hardware?". Signed-off-by: Wei Chen <wei.chen@arm.com> --- v1 -> v2: 1. Use ASSERT to replace VIRTUAL_BUG_ON in phys_to_nid. 2. Adjust the conditional express for ASSERT. 3. Move MAX_NUMNODES from xen/numa.h to asm/numa.h for x86. 4. Use conditional macro to gate MAX_NUMNODES for other architectures. --- xen/arch/x86/include/asm/numa.h | 6 +++--- xen/include/xen/numa.h | 2 ++ 2 files changed, 5 insertions(+), 3 deletions(-)