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

From: Julien Grall <julien@xen.org>
To: Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Nicola Vetrini <nicola.vetrini@bugseng.com>,
	consulting@bugseng.com,
	Bertrand Marquis <bertrand.marquis@arm.com>,
	Michal Orzel <michal.orzel@amd.com>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>, Wei Liu <wl@xen.org>,
	Shawn Anastasio <sanastasio@raptorengineering.com>,
	xen-devel@lists.xenproject.org,
	George Dunlap <george.dunlap@cloud.com>
Subject: Re: [XEN PATCH v3 3/3] xen/mm: add declaration for first_valid_mfn
Date: Fri, 15 Dec 2023 09:43:08 +0000	[thread overview]
Message-ID: <2879189b-eea3-48c4-86e4-baed69e6a8f0@xen.org> (raw)
In-Reply-To: <3b2f1699-d58c-40d8-af36-caef70064601@suse.com>

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