Re: [PATCH v6 02/17] xen/arm: implement helpers to get and update NUMA status

[Julien Grall <julien@xxxxxxx>]

Hi Henry,

On 20/11/2023 02:54, Henry Wang wrote:
> From: Wei Chen <wei.chen@xxxxxxx>
>
> NUMA has one global and one implementation specific switches. For
> ACPI NUMA implementation, Xen has acpi_numa, so we introduce
> device_tree_numa for device tree NUMA implementation. And use
> enumerations to indicate init, off and on status.
>
> arch_numa_disabled will get device_tree_numa status, but for
> arch_numa_setup we have not provided boot arguments to setup
> device_tree_numa. So we just return -EINVAL in this patch.
>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>
> ---
> v3 -> v6:
> - Rebase on top of staging without code changes.
> v2 -> v3:
> - Rename the first entry of enum dt_numa_status as DT_NUMA_DEFAULT.
> - Make enum dt_numa_status device_tree_numa as __ro_after_init and
> - assign it explicitly to DT_NUMA_DEFAULT.
> - Update the year in copyright to 2023.
> - Don't move the x86 numa_disabled() and make Arm's numa_disabled()
>    a static inline function for !CONFIG_NUMA.
> v1 -> v2:
> - Use arch_numa_disabled to replace numa_enable_with_firmware.
> - Introduce enumerations for device tree numa status.
> - Use common numa_disabled, drop Arm version numa_disabled.
> - Introduce arch_numa_setup for Arm.
> - Rename bad_srat to numa_bad.
> - Add numa_enable_with_firmware helper.
> - Add numa_disabled helper.
> - Refine commit message.
> ---
>   xen/arch/arm/include/asm/numa.h | 17 +++++++++++
>   xen/arch/arm/numa.c             | 50 +++++++++++++++++++++++++++++++++
>   2 files changed, 67 insertions(+)
>   create mode 100644 xen/arch/arm/numa.c
>
> diff --git a/xen/arch/arm/include/asm/numa.h b/xen/arch/arm/include/asm/numa.h
> index 7d6ae36a19..83f60ad05b 100644
> --- a/xen/arch/arm/include/asm/numa.h
> +++ b/xen/arch/arm/include/asm/numa.h
> @@ -22,6 +22,8 @@ typedef u8 nodeid_t;
>    */
>   #define NR_NODE_MEMBLKS NR_MEM_BANKS
>   
> +extern bool numa_disabled(void);
> +
>   #else
>   
>   /* Fake one node for now. See also node_online_map. */
> @@ -39,6 +41,21 @@ extern mfn_t first_valid_mfn;
>   #define node_start_pfn(nid) (mfn_x(first_valid_mfn))
>   #define __node_distance(a, b) (20)
>   
> +static inline bool numa_disabled(void)
> +{
> +    return true;
> +}
> +
> +static inline bool arch_numa_unavailable(void)
> +{
> +    return true;
> +}
> +
> +static inline bool arch_numa_broken(void)
> +{
> +    return true;
> +}
> +
>   #endif
>   
>   #define arch_want_default_dmazone() (false)
> diff --git a/xen/arch/arm/numa.c b/xen/arch/arm/numa.c
> new file mode 100644
> index 0000000000..eb5d0632cb
> --- /dev/null
> +++ b/xen/arch/arm/numa.c
> @@ -0,0 +1,50 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
Please use GPL-2.0-only for the SPDX tag.

> +/*
> + * Arm Architecture support layer for NUMA.
> + *
> + * Copyright (C) 2023 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/>.
> + *
You provided a SPDX tag, so the full license txt should not be added.

> + */
> +#include <xen/init.h>
> +#include <xen/numa.h>
> +
> +enum dt_numa_status {
> +    DT_NUMA_DEFAULT,
> +    DT_NUMA_ON,
> +    DT_NUMA_OFF,
> +};
NIT: I am guessing at some point we will want to support ACPI. So I 
would consider to remove dt/DT from the name. So we avoid some churn in 
the future.
> +
> +static enum dt_numa_status __ro_after_init device_tree_numa = DT_NUMA_DEFAULT;
> +
> +void __init numa_fw_bad(void)
> +{
> +    printk(KERN_ERR "NUMA: device tree numa info table not used.\n");
KERN_ERR is used for compatibility with Linux. Given this is a Xen file, 
then please use XENLOG_ERR.
Acked-by: Julien Grall <julien@xxxxxxx>

Cheers,

--
Julien Grall