Re: [Xen-devel] [RFC PATCH v1 16/21] ARM: NUMA: Extract proximity from SRAT table

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Fri, Mar 3, 2017 at 8:22 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
>
>
> On 03/03/17 14:45, Vijay Kilari wrote:
>>
>> On Fri, Mar 3, 2017 at 7:22 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
>>>
>>>
>>>
>>> On 03/03/17 13:50, Vijay Kilari wrote:
>>>>
>>>>
>>>> On Fri, Mar 3, 2017 at 7:14 PM, Julien Grall <julien.grall@xxxxxxx>
>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>> This is quite disgusting. We should avoid any #ifdef CONFIG_{X86,ARM}
>>>>>>> in
>>>>>>> common header.
>>>>>>>
>>>>>>> Also, x2apic and gicc are respectively x86-specific and arm-specific.
>>>>>>> So
>>>>>>> I
>>>>>>> think we should move the parsing in a separate arch-depend function
>>>>>>> to
>>>>>>> avoid
>>>>>>> those ifdery.
>>>>>>>
>>>>>>> By that I mean having a function acpi_table_arch_parse_srat that will
>>>>>>> parse
>>>>>>> x2apci on x86 and gicc on ARM. Jan, what do you think?
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Linux also follows similar approach
>>>>>>
>>>>>>
>>>>>>
>>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/acpi.h?id=refs/tags/v4.10#n265
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> So? What are you trying to prove?
>>>>>
>>>>> The linux version is much readable than yours. Anyway, we should limit
>>>>> CONFIG_{X86,ARM} ifdefery in common code.
>>>>>
>>>>> Currently, I see no point to have those ifdefery when it is possible to
>>>>> add
>>>>> an arch-depend function.
>>>>
>>>>
>>>>
>>>> This is because in xen we have another level of config CONFIG_ACPI_NUMA.
>>>> I have plans to reuse cpu and memory part next revision.
>>>
>>>
>>>
>>> This does not explain why you want to do like Linux.
>>
>>
>> Basically want to reuse xen/drivers/acpi/numa.c which is common for
>> both x86 and ARM.
>> If not, then we have move some arch specific as you mentioned.
>
>
> I think you misunderstood what I suggested. I only asked to do something
> like:

Got it.
>
> int __init acpi_numa_init(void)
> {
>         if (!acpi_parse_table(....)) {
>                 acpi_table_parse_srat(TYPE_CPU_AFFINITY);

This is not defined for ARM. We have to make this also arch specific.
So all arch specific code from xen/drivers/acpi/numa.c should be moved
to arch specific to xen/arch/x86/srat.c

>                 acpi_table_parse_srat(TYPE_MEMORY_AFFINITY);
>                 acpi_table_arch_parse_srat();
>         }
> }
>
> And then for x86
>
> void acpi_table_arch_parse_start(void)
> {
>         acpi_table_parse_srat(TYPE_X2APIC_CPU_AFFINITY);
> }
>
> And for ARM
>
> void acpi_table_arch_parse_start(void)
> {
>         acpi_table_parse_srat(TYPE_GICC_AFFINITY);
> }
>
>
> The code is still as common but a function is called for arch specific
> setup. This does not require any ifdefery.
>
>>
>> I have another idea where in, if all the NUMA ACPI code is programmed
>> under
>> CONFIG_NUMA and only initialization is kept under CONFIG_ACPI_NUMA
>> similar to x86
>> then we don't need to pollute this header much and limit the changes.
>>
>> I will try to implement this and see how simple it can go and let you
>> know. OK?
>
>
> I don't want to see the common header polluted with #ifdef CONFIG_X86 and
> #ifdef CONFIG_ARM. This is just not right.
>
> Cheers,
>
> --
> Julien Grall

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