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

From: Nicola Vetrini <nicola.vetrini@bugseng.com>
To: Julien Grall <julien@xen.org>, Jbeulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	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 10:59:02 +0100	[thread overview]
Message-ID: <9c981e710ef8c86f2a71ef332d991ef1@bugseng.com> (raw)
In-Reply-To: <2879189b-eea3-48c4-86e4-baed69e6a8f0@xen.org>

Hi Jan, Julien

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

Given the controversy that arose on this patch, maybe it's best to drop 
it now from this series. If then it's agreed that as a temporary 
workaround you want to leave it as is and deviate, as I proposed 
elsewhere in the thread, feel free to reach out. Meanwhile, if patch 1 
looks ok I can mark this series as done and focus on the v2 for R2.1, 
which is almost ready.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)