Re: [PATCH v2 05/10] xen/x86: Use ASSERT instead of VIRTUAL_BUG_ON for phys_to_nid

[Wei Chen <Wei.Chen@xxxxxxx>]

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