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

[Julien Grall <julien.grall@xxxxxxx>]

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

> > 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.
Regards,

--
Julien Grall

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