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

From: Julien Grall <julien@xen.org>
To: Nicola Vetrini <nicola.vetrini@bugseng.com>,
	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:06:22 +0000	[thread overview]
Message-ID: <05133f2d-c322-459b-b0b2-e34a07fbb3b6@xen.org> (raw)
In-Reply-To: <9c981e710ef8c86f2a71ef332d991ef1@bugseng.com>

Hi,

On 15/12/2023 09:59, Nicola Vetrini wrote:
>>>> +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.

I am still not in favor of deviation. We would need to create a new 
SAF-* (because none of the existing one are not correct) and it is not 
clear how you could argue this is a good deviation.

Just to be clear, this is not a Nack for a deviation. I find just silly 
we are trying to use deviation here just because there is a small roadblock.

To me the goal is to try to improve Xen, not trying to deviate 
everything because this is easier.

Cheers,

-- 
Julien Grall