RE: [XEN RFC PATCH 21/40] xen/arm: introduce device_tree_numa as a switch for device tree NUMA

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: 2021年8月20日 1:45
> To: Wei Chen <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx; jbeulich@xxxxxxxx
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
> Subject: Re: [XEN RFC PATCH 21/40] xen/arm: introduce device_tree_numa as
> a switch for device tree NUMA
> 
> Hi Wei,
> 
> On 11/08/2021 11:24, Wei Chen wrote:
> > Like acpi_numa in x86 as a switch for ACPI based NUMA, we introduce
> > device_tree_numa as a switch for Arm device tree based NUMA. When
> > NUMA information in device tree is invalid, this switch will be set
> > to -1, then NUMA support for Arm will be disabled, even if user set
> > numa_off=0.
> 
> The hypervisor will never use both ACPI and DT at runtime. In fact...
> 

Yes.

> >
> > Keep using bad_srat and srat_disabled functions name, because we will
> > reuse node_covers_memory and acpi_scan_nodes code for Arm.
> 
> ... given that both functions will be called from the common code, it
> will be a lot more difficult to add ACPI afterwards.
> 
> So I think we should either rename acpi_numa to something more generic
> (maybe fw_numa) or convert numa_off to a tri-state.
> 
> This will allow to have the code mostly common.
> 

I will try to address them in next version.

> > These
> > functions are using these two API names. And, as device tree can be
> > treated as one kind of static resource table. So we keep these two
> > function names.
> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > ---
> >   xen/arch/arm/Makefile           |  1 +
> >   xen/arch/arm/numa_device_tree.c | 35 +++++++++++++++++++++++++++++++++
> >   xen/include/asm-arm/numa.h      |  2 ++
> >   3 files changed, 38 insertions(+)
> >   create mode 100644 xen/arch/arm/numa_device_tree.c
> >
> > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> > index 6e3fb8033e..13e1549be0 100644
> > --- a/xen/arch/arm/Makefile
> > +++ b/xen/arch/arm/Makefile
> > @@ -36,6 +36,7 @@ obj-y += mem_access.o
> >   obj-y += mm.o
> >   obj-y += monitor.o
> >   obj-$(CONFIG_NUMA) += numa.o
> > +obj-$(CONFIG_DEVICE_TREE_NUMA) += numa_device_tree.o
> >   obj-y += p2m.o
> >   obj-y += percpu.o
> >   obj-y += platform.o
> > diff --git a/xen/arch/arm/numa_device_tree.c
> b/xen/arch/arm/numa_device_tree.c
> > new file mode 100644
> > index 0000000000..1c74ad135d
> > --- /dev/null
> > +++ b/xen/arch/arm/numa_device_tree.c
> > @@ -0,0 +1,35 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Arm Architecture support layer for NUMA.
> > + *
> > + * Copyright (C) 2021 Arm Ltd
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> > + *
> > + */
> > +#include <xen/init.h>
> > +#include <xen/nodemask.h>
> > +#include <xen/numa.h>
> > +
> > +s8 device_tree_numa = 0;
> > +
> > +int srat_disabled(void)
> 
> You export this one and ...
> 
> > +void __init bad_srat(void)
> 
> ... this one without providing in a prototype.
> 
> Looking at the rest of the series... they will be turned static in the
> next patch (#21) but then re-exported in patch #33.
> 
> In general, we should refrain to modify code that was added in the same
> patch unless it is not possible for split reason (e.g code clean-up and
> then code movement).
> 
> In this case, the helpers should be exported from now.
> 

Ok.

> > +{
> > +    printk(KERN_ERR "DT: NUMA information is not used.\n");
> > +    device_tree_numa = -1;
> > +}
> > diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h
> > index 559b028a01..756ad82d07 100644
> > --- a/xen/include/asm-arm/numa.h
> > +++ b/xen/include/asm-arm/numa.h
> > @@ -23,6 +23,8 @@ typedef u8 nodeid_t;
> >   #define NUMA_LOCAL_DISTANCE     10
> >   #define NUMA_REMOTE_DISTANCE    20
> >
> > +extern s8 device_tree_numa;
> > +
> >   extern void numa_init(bool acpi_off);
> >   extern void numa_set_distance(nodeid_t from, nodeid_t to, uint32_t
> distance);
> >
> >
> 
> Cheers,
> 
> --
> Julien Grall