Hi Jan,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: 2022年7月11日 14:32
> To: Wei Chen <wei.c...@arm.com>
> Cc: nd <n...@arm.com>; Andrew Cooper <andrew.coop...@citrix.com>; George
> Dunlap <george.dun...@citrix.com>; Julien Grall <jul...@xen.org>; Stefano
> Stabellini <sstabell...@kernel.org>; Wei Liu <w...@xen.org>; Jiamei Xie
> <jiamei....@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.c...@arm.com>
> > Tested-by: Jiamei Xie <jiamei....@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
