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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 15/12/2023 08:03, 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),
I don't care too much about which header is used. So I am ok with numa.h 
if this is what you really want. However...
> 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
There are a few reasons I didn't update with that approach or want to 
ask Nicola for this:
  1. I assume this will be a macro and it is not clear to me whether 
Eclair would be able to detect the visibility change based on the config
  2. IMHO, this is obfuscating a bit more the code
  3. I don't see why we want to special case first_valid_mfn. I am sure 
we have other external variables in the same situation.
So if any solution needs to came up, then it needs to be generic, and I 
fear this is going to be yet another lengthy work which is not worth it 
for this case.
> (and I've now added this to my short-term TODO list).
Thanks. But as I wrote above, I am not sure I agree such proposal. Let 
see what the code looks like.
Cheers,


--
Julien Grall