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

[Jan Beulich <jbeulich@xxxxxxxx>]

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