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 7:14 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hello Vijay,
>
> On 03/03/17 12:39, Vijay Kilari wrote:
>>
>> On Thu, Mar 2, 2017 at 10:51 PM, Julien Grall <julien.grall@xxxxxxx>
>> wrote:
>>>>
>>>> diff --git a/xen/drivers/acpi/numa.c b/xen/drivers/acpi/numa.c
>>>> index 50bf9f8..ce22e88 100644
>>>> --- a/xen/drivers/acpi/numa.c
>>>> +++ b/xen/drivers/acpi/numa.c
>>>> @@ -25,9 +25,11 @@
>>>>  #include <xen/init.h>
>>>>  #include <xen/types.h>
>>>>  #include <xen/errno.h>
>>>> +#include <xen/mm.h>
>>>
>>>
>>>
>>> Why do you need to inlucde xen/mm.h and ...
>>>
>>>>  #include <xen/acpi.h>
>>>>  #include <xen/numa.h>
>>>>  #include <acpi/acmacros.h>
>>>> +#include <asm/mm.h>
>>>
>>>
>>>
>>> asm/mm.h?
>>
>>
>> I remember when CONFIG_ACPI +CONFIG_NUMA is enabled
>> there is compilation error.
>
>
> Regarding asm/mm.h, it is already included by xen/mm.h. So no point to add
> it.
>
> Now regarding xen/mm.h, just saying there is a compilation error is not
> helpful. Can you expand why it is needed?

I remember just adding xen/mm.h has not solved the problem. Anyway I
will check this
when I work for next revision.

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

Regards
Vijay

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