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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> 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.

Jan