Re: [Xen-devel] [RFC PATCH v3 13/24] ARM: NUMA: DT: Parse memory NUMA information

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

Hi Julien,

On Thu, Jul 20, 2017 at 4:56 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
>
>
> On 19/07/17 19:39, Julien Grall wrote:
>>>
>>>      cell = (const __be32 *)prop->data;
>>>      banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
>>>
>>> -    for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS;
>>> i++ )
>>> +    for ( i = 0; i < banks; i++ )
>>>      {
>>>          device_tree_get_reg(&cell, address_cells, size_cells, &start,
>>> &size);
>>>          if ( !size )
>>>              continue;
>>> -        bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
>>> -        bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
>>> -        bootinfo.mem.nr_banks++;
>>> +        if ( !efi_enabled(EFI_BOOT) && bootinfo.mem.nr_banks <
>>> NR_MEM_BANKS )
>>> +        {
>>> +            bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
>>> +            bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
>>> +            bootinfo.mem.nr_banks++;
>>> +        }
>>
>>
>> This change should be split.
>
>
> I thought a bit more about this code during the week. I think it would be
> nicer to write:
>
> #ifdef CONFIG_NUMA
> dt_numa_process_memory_node(nid, start, size);
> #endif
>
> if ( !efi_enabled(EFI_BOOT) )
>   continue;

Should be if ( efi_enabled(EFI_BOOT) ) ?
>
> if ( bootinfo.mem.nr_banks < NR_MEM_BANKS )

Should be if ( bootinfo.mem.nr_banks >= NR_MEM_BANKS ) ?

>   break;
>
> bootinfo.mem.bank[....];
> ....
>
> Also, you may want to add a stub for dt_numa_process_memory_node rather than
> #ifdef in the code.
>
> Cheers,
>
> --
> Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel