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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxx>

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
>