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 after we moved the phys_to_nid from
x86 ARCH header file to common header file.
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. Although NUMA allows one node
can only have CPUs but without any memory. And node with 0 bytes
of memory might have an entry in memnodemap[] theoretically. But
that doesn't mean phys_to_nid can find any valid address from a
node with 0 bytes memory.
Signed-off-by: Wei Chen <wei.chen@arm.com>
Tested-by: Jiamei Xie <jiamei.xie@arm.com>
---
v1 -> v2:
1. Move from part#1 to part#2. (Comment from NUMA part1 series)
2. Refine the justification of using
!node_data[nid].node_spanned_pages. (From NUMA part1 series)
3. Use ASSERT to replace VIRTUAL_BUG_ON in phys_to_nid.
4. Adjust the conditional express for ASSERT.
5. Move MAX_NUMNODES from xen/numa.h to asm/numa.h for x86.
6. Use conditional macro to gate MAX_NUMNODES for other architectures.
---
xen/include/xen/numa.h | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
index db0e524a0e..695ad51df0 100644
--- a/xen/include/xen/numa.h
+++ b/xen/include/xen/numa.h
@@ -36,8 +36,6 @@ struct node {
extern int compute_hash_shift(struct node *nodes, int numnodes,
nodeid_t *nodeids);
-#define VIRTUAL_BUG_ON(x)
-
/* Enumerations for NUMA status. */
enum numa_mode {
numa_on = 0,
@@ -77,9 +75,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;
}
--
2.25.1On 08.07.2022 16:54, 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 after we moved the phys_to_nid from
> x86 ARCH header file to common header file.
> 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. Although NUMA allows one node
> can only have CPUs but without any memory. And node with 0 bytes
> of memory might have an entry in memnodemap[] theoretically. But
> that doesn't mean phys_to_nid can find any valid address from a
> node with 0 bytes memory.
>
> Signed-off-by: Wei Chen <wei.chen@arm.com>
> Tested-by: Jiamei Xie <jiamei.xie@arm.com>
> ---
> v1 -> v2:
> 1. Move from part#1 to part#2. (Comment from NUMA part1 series)
> 2. Refine the justification of using
> !node_data[nid].node_spanned_pages. (From NUMA part1 series)
> 3. Use ASSERT to replace VIRTUAL_BUG_ON in phys_to_nid.
> 4. Adjust the conditional express for ASSERT.
> 5. Move MAX_NUMNODES from xen/numa.h to asm/numa.h for x86.
> 6. Use conditional macro to gate MAX_NUMNODES for other architectures.
The change looks okay, but can you please clarify what these last two
points describe? They don't seem to match any change ...
> --- a/xen/include/xen/numa.h
> +++ b/xen/include/xen/numa.h
> @@ -36,8 +36,6 @@ struct node {
> extern int compute_hash_shift(struct node *nodes, int numnodes,
> nodeid_t *nodeids);
>
> -#define VIRTUAL_BUG_ON(x)
> -
> /* Enumerations for NUMA status. */
> enum numa_mode {
> numa_on = 0,
> @@ -77,9 +75,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;
> }
>
... in the entire patch.
Jan
Hi Jan,
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 2022年7月11日 14:32
> To: Wei Chen <Wei.Chen@arm.com>
> Cc: nd <nd@arm.com>; Andrew Cooper <andrew.cooper3@citrix.com>; George
> Dunlap <george.dunlap@citrix.com>; Julien Grall <julien@xen.org>; Stefano
> Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Jiamei Xie
> <Jiamei.Xie@arm.com>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v2 4/9] xen/x86: Use ASSERT instead of VIRTUAL_BUG_ON
> for phys_to_nid
>
> On 08.07.2022 16:54, 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 after we moved the phys_to_nid from
> > x86 ARCH header file to common header file.
> > 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. Although NUMA allows one node
> > can only have CPUs but without any memory. And node with 0 bytes
> > of memory might have an entry in memnodemap[] theoretically. But
> > that doesn't mean phys_to_nid can find any valid address from a
> > node with 0 bytes memory.
> >
> > Signed-off-by: Wei Chen <wei.chen@arm.com>
> > Tested-by: Jiamei Xie <jiamei.xie@arm.com>
> > ---
> > v1 -> v2:
> > 1. Move from part#1 to part#2. (Comment from NUMA part1 series)
> > 2. Refine the justification of using
> > !node_data[nid].node_spanned_pages. (From NUMA part1 series)
> > 3. Use ASSERT to replace VIRTUAL_BUG_ON in phys_to_nid.
> > 4. Adjust the conditional express for ASSERT.
> > 5. Move MAX_NUMNODES from xen/numa.h to asm/numa.h for x86.
> > 6. Use conditional macro to gate MAX_NUMNODES for other architectures.
>
> The change looks okay, but can you please clarify what these last two
> points describe? They don't seem to match any change ...
>
I moved this patch form Part#1 to Part#2, but forgot to remove these
two change logs. In Part#1, we do those changes, but after we moved
this patch to Part#2, those changes are unnecessary. I will remove
these two lines of change logs. Sorry for the confusion!
Cheers,
Wei Chen
> > --- a/xen/include/xen/numa.h
> > +++ b/xen/include/xen/numa.h
> > @@ -36,8 +36,6 @@ struct node {
> > extern int compute_hash_shift(struct node *nodes, int numnodes,
> > nodeid_t *nodeids);
> >
> > -#define VIRTUAL_BUG_ON(x)
> > -
> > /* Enumerations for NUMA status. */
> > enum numa_mode {
> > numa_on = 0,
> > @@ -77,9 +75,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;
> > }
> >
>
> ... in the entire patch.
>
> Jan
On 12.07.2022 09:20, Wei Chen wrote: > Hi Jan, > >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 2022年7月11日 14:32 >> To: Wei Chen <Wei.Chen@arm.com> >> Cc: nd <nd@arm.com>; Andrew Cooper <andrew.cooper3@citrix.com>; George >> Dunlap <george.dunlap@citrix.com>; Julien Grall <julien@xen.org>; Stefano >> Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Jiamei Xie >> <Jiamei.Xie@arm.com>; xen-devel@lists.xenproject.org >> Subject: Re: [PATCH v2 4/9] xen/x86: Use ASSERT instead of VIRTUAL_BUG_ON >> for phys_to_nid >> >> On 08.07.2022 16:54, 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 after we moved the phys_to_nid from >>> x86 ARCH header file to common header file. >>> 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. Although NUMA allows one node >>> can only have CPUs but without any memory. And node with 0 bytes >>> of memory might have an entry in memnodemap[] theoretically. But >>> that doesn't mean phys_to_nid can find any valid address from a >>> node with 0 bytes memory. >>> >>> Signed-off-by: Wei Chen <wei.chen@arm.com> >>> Tested-by: Jiamei Xie <jiamei.xie@arm.com> >>> --- >>> v1 -> v2: >>> 1. Move from part#1 to part#2. (Comment from NUMA part1 series) >>> 2. Refine the justification of using >>> !node_data[nid].node_spanned_pages. (From NUMA part1 series) >>> 3. Use ASSERT to replace VIRTUAL_BUG_ON in phys_to_nid. >>> 4. Adjust the conditional express for ASSERT. >>> 5. Move MAX_NUMNODES from xen/numa.h to asm/numa.h for x86. >>> 6. Use conditional macro to gate MAX_NUMNODES for other architectures. >> >> The change looks okay, but can you please clarify what these last two >> points describe? They don't seem to match any change ... >> > > I moved this patch form Part#1 to Part#2, but forgot to remove these > two change logs. In Part#1, we do those changes, but after we moved > this patch to Part#2, those changes are unnecessary. I will remove > these two lines of change logs. Sorry for the confusion! With this clarified Acked-by: Jan Beulich <jbeulich@suse.com> Jan
© 2016 - 2026 Red Hat, Inc.