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

[Julien Grall <julien.grall@xxxxxxx>]

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:
int __init acpi_numa_init(void)
{
        if (!acpi_parse_table(....)) {
                acpi_table_parse_srat(TYPE_CPU_AFFINITY);
                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