Re: [PATCH v5 5/6] xen/x86: move NUMA scan nodes codes from x86 to common

From: Wei Chen <Wei.Chen@arm.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: nd@arm.com, "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Julien Grall" <julien@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v5 5/6] xen/x86: move NUMA scan nodes codes from x86 to common
Date: Thu, 29 Sep 2022 16:21:02 +0800	[thread overview]
Message-ID: <fc03f565-8c05-e310-fa0a-698ad7c569e3@arm.com> (raw)
In-Reply-To: <5ac85819-9a1d-b8bc-d49d-13cb34a3e7e9@suse.com>

Hi Jan,

On 2022/9/27 23:48, Jan Beulich wrote:
> On 20.09.2022 11:12, Wei Chen wrote:
>> +static bool __init nodes_cover_memory(void)
>> +{
>> +    unsigned int i;
>> +
>> +    for ( i = 0; ; i++ )
>> +    {
>> +        int err;
>> +        bool found;
>> +        unsigned int j;
>> +        paddr_t start, end;
>> +
>> +        /* Try to loop memory map from index 0 to end to get RAM ranges. */
>> +        err = arch_get_ram_range(i, &start, &end);
>> +
>> +        /* Reached the end of the memory map? */
>> +        if ( err == -ENOENT )
>> +            break;
>> +
>> +        /* Skip non-RAM entries. */
>> +        if ( err )
>> +            continue;
>> +
>> +        do {
>> +            found = false;
>> +            for_each_node_mask ( j, memory_nodes_parsed )
>> +                if ( start < nodes[j].end
>> +                    && end > nodes[j].start )
> 
> Nit: Style (placement of && and indentation). Does this actually need
> splitting across two lines?
> 

After covert from tab to spaces, yes, it doesn't need split into two 
lines. I will fix it.

>> --- a/xen/drivers/acpi/Kconfig
>> +++ b/xen/drivers/acpi/Kconfig
>> @@ -7,4 +7,5 @@ config ACPI_LEGACY_TABLES_LOOKUP
>>   
>>   config ACPI_NUMA
>>   	bool
>> +	select HAS_NUMA_NODE_FWID
>>   	select NUMA
> 
> While I might guess that you've chosen the insertion point to have
> things sorted alphabetically, I think here it would be more natural
> to select the wider option first and then also select the more
> narrow one.
> 

Ok, I will adjust the order.

> One further question though: How is this going to work for Arm64
> once it wants to support both the form of NUMA you're working to
> enable _and_ ACPI-based NUMA? There better wouldn't be a requirement
> to pick one of the two at build time - it would be nice for support
> of both forms to be able to co-exist in a single binary.

We are also working in this way. In part#3, we will check ACPI first,
only when ACPI is off, the DT NUMA will be used by Arm. If ACPI is on, 
we will skip DT NUMA.

Cheers,
Wei Chen

> 
> Jan