Re: [XEN PATCH v3 3/3] xen/mm: add declaration for first_valid_mfn

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.12.2023 20:10, Julien Grall wrote:
> On 14/12/2023 14:15, George Dunlap wrote:
>> But I do think that it's fair to ask Julien to think about a suitable
>> wording, since the comment is in a sense to remind him (or other ARM
>> maintainers) what's needed, and since the eventual solution will be
>> something to do with the ARM code and architecture anyway.
> 
> The comment is for anyone using !NUMA (i.e. all architectures but x86) 
> :). What about the following (this is Nicola's patch with the comments 
> reworked):

This clearly is better, yet then ...

> --- a/xen/arch/arm/include/asm/numa.h
> +++ b/xen/arch/arm/include/asm/numa.h
> @@ -2,8 +2,9 @@
>   #define __ARCH_ARM_NUMA_H
> 
>   #include <xen/mm.h>
> +#include <xen/types.h>
> 
> -typedef u8 nodeid_t;
> +typedef uint8_t nodeid_t;
> 
>   #ifndef CONFIG_NUMA
> 
> @@ -11,12 +12,6 @@ typedef u8 nodeid_t;
>   #define cpu_to_node(cpu) 0
>   #define node_to_cpumask(node)   (cpu_online_map)
> 
> -/*
> - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
> - * is required because the dummy helpers are using it.
> - */
> -extern mfn_t first_valid_mfn;
> -
>   /* XXX: implement NUMA support */
>   #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
>   #define node_start_pfn(nid) (mfn_x(first_valid_mfn))
> diff --git a/xen/arch/ppc/include/asm/numa.h 
> b/xen/arch/ppc/include/asm/numa.h
> index 7fdf66c3da74..888de2dbd1eb 100644
> --- a/xen/arch/ppc/include/asm/numa.h
> +++ b/xen/arch/ppc/include/asm/numa.h
> @@ -1,8 +1,8 @@
>   #ifndef __ASM_PPC_NUMA_H__
>   #define __ASM_PPC_NUMA_H__
> 
> -#include <xen/types.h>
>   #include <xen/mm.h>
> +#include <xen/types.h>
> 
>   typedef uint8_t nodeid_t;
> 
> @@ -10,12 +10,6 @@ typedef uint8_t nodeid_t;
>   #define cpu_to_node(cpu) 0
>   #define node_to_cpumask(node)   (cpu_online_map)
> 
> -/*
> - * TODO: make first_valid_mfn static when NUMA is supported on PPC, this
> - * is required because the dummy helpers are using it.
> - */
> -extern mfn_t first_valid_mfn;
> -
>   /* XXX: implement NUMA support */
>   #define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
>   #define node_start_pfn(nid) (mfn_x(first_valid_mfn))
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 9b5df74fddab..d874525916ea 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -255,8 +255,10 @@ static PAGE_LIST_HEAD(page_broken_list);
>    */
> 
>   /*
> - * first_valid_mfn is exported because it is use in ARM specific NUMA
> - * helpers. See comment in arch/arm/include/asm/numa.h.
> + * first_valid_mfn is exported because it is used when !CONFIG_NUMA.
> + *
> + * TODO: Consider if we can conditionally export first_valid_mfn based
> + * on whether NUMA is selected.
>    */
>   mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER;
> 
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 3d9b2d05a5c8..a13a9a46ced7 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -118,6 +118,8 @@ int destroy_xen_mappings(unsigned long s, unsigned 
> long e);
>   /* Retrieve the MFN mapped by VA in Xen virtual address space. */
>   mfn_t xen_map_to_mfn(unsigned long va);
> 
> +extern mfn_t first_valid_mfn;
> +
>   /*
>    * Create only non-leaf page table entries for the
>    * page range in Xen virtual address space.

... I still disagree with the placement here (should be xen/numa.h imo),
and I still don't see why we can't carry out the TODO right away, if we
have to touch all of this anyway. If it's really too much to ask from
the original contributor, I can certainly see about making a patch myself
(and I've now added this to my short-term TODO list).

Jan