RE: [PATCH 19/37] xen/x86: promote VIRTUAL_BUG_ON to ASSERT in

2022-01-17 Thread Wei Chen 
Hi Jan,

> -Original Message-
> From: Jan Beulich 
> Sent: 2022年1月18日 0:22
> To: Wei Chen 
> Cc: Bertrand Marquis ; xen-
> de...@lists.xenproject.org; sstabell...@kernel.org; jul...@xen.org
> Subject: Re: [PATCH 19/37] xen/x86: promote VIRTUAL_BUG_ON to ASSERT in
> 
> On 23.09.2021 14:02, Wei Chen wrote:
> > VIRTUAL_BUG_ON that is using in phys_to_nid is an empty macro. This
> > results in two lines of error-checking code in phys_to_nid are not
> > actually working. It also covers up two compilation errors:
> > 1. error: ‘MAX_NUMNODES’ undeclared (first use in this function).
> >This is because MAX_NUMNODES is defined in xen/numa.h.
> >But asm/numa.h is a dependent file of xen/numa.h, we can't
> >include xen/numa.h in asm/numa.h. This error has been fixed
> >after we move phys_to_nid to xen/numa.h.
> 
> This could easily be taken care of by move MAX_NUMNODES up ahead of
> the asm/numa.h inclusion point. And then the change here would become
> independent of the rest of the series (and could hence go in early).
> 
> > 2. error: wrong type argument to unary exclamation mark.
> >This is becuase, 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 in VIRTUAL_BUG_ON to enable the two
> > lines of error-checking code.
> 
> May I suggest to drop VIRTUAL_BUG_ON() and instead use ASSERT()
> directly?
> 

Sure!

> Jan

Re: [PATCH 19/37] xen/x86: promote VIRTUAL_BUG_ON to ASSERT in

2022-01-17 Thread Jan Beulich 
On 23.09.2021 14:02, Wei Chen wrote:
> VIRTUAL_BUG_ON that is using in phys_to_nid is an empty macro. This
> results in two lines of error-checking code in phys_to_nid are not
> actually working. It also covers up two compilation errors:
> 1. error: ‘MAX_NUMNODES’ undeclared (first use in this function).
>This is because MAX_NUMNODES is defined in xen/numa.h.
>But asm/numa.h is a dependent file of xen/numa.h, we can't
>include xen/numa.h in asm/numa.h. This error has been fixed
>after we move phys_to_nid to xen/numa.h.

This could easily be taken care of by move MAX_NUMNODES up ahead of
the asm/numa.h inclusion point. And then the change here would become
independent of the rest of the series (and could hence go in early).

> 2. error: wrong type argument to unary exclamation mark.
>This is becuase, 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 in VIRTUAL_BUG_ON to enable the two
> lines of error-checking code.

May I suggest to drop VIRTUAL_BUG_ON() and instead use ASSERT()
directly?

Jan

[PATCH 19/37] xen/x86: promote VIRTUAL_BUG_ON to ASSERT in

2021-09-23 Thread Wei Chen 
VIRTUAL_BUG_ON that is using in phys_to_nid is an empty macro. This
results in two lines of error-checking code in phys_to_nid are not
actually working. It also covers up two compilation errors:
1. error: ‘MAX_NUMNODES’ undeclared (first use in this function).
   This is because MAX_NUMNODES is defined in xen/numa.h.
   But asm/numa.h is a dependent file of xen/numa.h, we can't
   include xen/numa.h in asm/numa.h. This error has been fixed
   after we move phys_to_nid to xen/numa.h.
2. error: wrong type argument to unary exclamation mark.
   This is becuase, 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 in VIRTUAL_BUG_ON to enable the two
lines of error-checking code. And fix the 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?". Even Xen allows to online
such kind of nodes. I still think it's impossible for phys_to_nid to
return a no memory node for a physical address.

Signed-off-by: Wei Chen 
---
 xen/include/xen/numa.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
index 51391a2440..1978e2be1b 100644
--- a/xen/include/xen/numa.h
+++ b/xen/include/xen/numa.h
@@ -38,7 +38,7 @@ struct node {
 extern int compute_hash_shift(struct node *nodes, int numnodes,
  nodeid_t *nodeids);
 
-#define VIRTUAL_BUG_ON(x)
+#define VIRTUAL_BUG_ON(x) ASSERT(!(x))
 
 extern void numa_add_cpu(int cpu);
 extern void numa_init_array(void);
@@ -75,7 +75,7 @@ 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);
nid = memnodemap[paddr_to_pdx(addr) >> memnode_shift];
-   VIRTUAL_BUG_ON(nid >= MAX_NUMNODES || !node_data[nid]);
+   VIRTUAL_BUG_ON(nid >= MAX_NUMNODES || 
!node_data[nid].node_spanned_pages);
return nid;
 }
 
-- 
2.25.1