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/27 13:56, Jan Beulich wrote:
> On 27.04.2022 05:52, Wei Chen wrote:
> > > From: Jan Beulich <jbeulich@xxxxxxxx>
> > > Sent: 2022年4月26日 22:42
> > >
> > > On 26.04.2022 12:59, Wei Chen wrote:
> > > > On 2022/4/26 17:02, Jan Beulich wrote:
> > > > > On 18.04.2022 11:07, Wei Chen wrote:
> > > > > > 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?
> > > It's getting close, but it's not exactly equivalent I think. A node
> > > with 0 bytes of memory might (at least in theory) have an entry in
> > > memnodemap[]. But finding a node ID for that address would still
> > I have done a quick check in populate_memnodemap:
> > 74          spdx = paddr_to_pdx(nodes[i].start);
> > 75          epdx = paddr_to_pdx(nodes[i].end - 1) + 1;
> > 76          if ( spdx >= epdx )
> > 77              continue;
> >
> > It seems that if node has no memory, start == end, then this function
> > will not populate memnodemap entry for this node.
> >
> > > not mean that at least one byte of memory at that address is present
> > > on the given node, because the node covers 0 bytes.
> > And back to this patch, can I just drop the unnecessary justification
> > from the commit message?
> Well, you'll want to have _some_ justification for that particular
> aspect of the patch.
Ok.

> > And for the bogus warning message, can I update it to an INFO level
> > message in part#2 series, and just keep:
> > printk(KERN_INFO "SRAT: Node %u has no memory!\n", i);
> > but remove "BIOS Bug or mis-configured hardware?\n", i); ?
> This sounds at least plausible to do.
Ok.

> Jan