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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 15 Dec 2023, Jan Beulich wrote:
> 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),

This is where I would call it "good enough" and close it for now. I
think we should commit such a patch (Julien's patch with the placement
in xen/numa.h).

Anything else...


> 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).

... can still be done later whenever you get to it.