Re: [PATCH v4 08/16] xen/mm: Drop the parameter mfn from populate_pt_range

From: Julien Grall <julien.grall@arm.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Tim Deegan <tim@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	xen-devel@lists.xen.org
Subject: Re: [PATCH v4 08/16] xen/mm: Drop the parameter mfn from populate_pt_range
Date: Mon, 5 Mar 2018 13:43:12 +0000	[thread overview]
Message-ID: <ca1b2cfa-67b5-f6d7-038b-d95305189176@arm.com> (raw)
In-Reply-To: <5A9973D402000078001ADDA8@prv-mh.provo.novell.com>

Hi Jan,

On 02/03/18 14:55, Jan Beulich wrote:
>>>> On 22.02.18 at 17:55, <julien.grall@arm.com> wrote:
>> On 22/02/18 16:51, Wei Liu wrote:
>>> On Thu, Feb 22, 2018 at 04:40:04PM +0000, Julien Grall wrote:
>>>> On 22/02/18 16:35, Wei Liu wrote:
>>>>> On Wed, Feb 21, 2018 at 02:02:51PM +0000, Julien Grall wrote:
>>>>>> The function populate_pt_range is used to populate in advance the
>>>>>> page-table but it will not do the actual mapping. So passing the MFN in
>>>>>> parameter is pointless. Note that the only caller pass 0...
>>>>>>
>>>>>> At the same time replace 0 by INVALID_MFN to make clear the MFN is
>>>>>> invalid.
>>>>>>
>>>>>
>>>>> The mfn parameter is the first mfn of a consecutive nr MFNs passed to
>>>>> map_pages_to_xen. Putting INVALID_MFN isn't helping -- the value written
>>>>> to page table(s) will wrap around to 0.
>>>>>
>>>>> And I think starting from 0 to avoid overflow is probably a better
>>>>> behaviour. If you really want to make sure all entries are filled with
>>>>> INVALID_MFN you should call map_pages_to_xen for nr times with each
>>>>> page.
>>>>
>>>> I am not sure to understand this. From its name, populate_pt_range should
>>>> only create the intermediate tables. The leaf entry will stay invalid. So
>>>> how the value of mfn matters? Is it because the code is written in a such
>>>> way that passing INVALID_MFN will result to undefined behavior?
>>>
>>> Right, that's what I meant. It doesn't matter whether you use 0 or
>>> INVALID_MFN.
>>>
>>> Unsigned integer overflow is not UB in C, so passing INVALID_MFN is
>>> safe.
>>>
>>> But your intention seemed to be filling all entries with INVALID_MFN to
>>> aid debugging, so the function doesn't do what I think you wanted it to
>>> do. It could be I misunderstood your intention.
>>
>> That was not my intention. I replaced 0 by INVALID_MFN because from the
>> name you know the MFN is invalid. 0 could potentially be valid (at least
>> on Arm) and make the code confusing to understand.
>>
>> I can make it clearer in the commit message.
> 
> I don't think that'll be much better; I agree with Wei that you
> don't want the wrapping behavior here. What you want to do
> is skip the increments in x86's map_pages_to_xen() when
> mfn is INVALID_MFN. Granted this should have been done
> before (so that there wouldn't have been incrementing from
> zero), but as you say MFN 0 isn't fundamentally invalid (albeit
> on x86 we almost make it invalid).
> 
> As to your earlier argument - please don't forget that on x86
> the function still fills all leaf entries in the range, just that they
> all will be non-present ones.

I still don't understand why it matters. The entry is not present so the 
address is going to ignore. 0 or MFN_INVALID are just dummy value that 
are going to be replaced on the entry is made present.

Furthermore, as Wei pointed out unsigned integer overflow is not UB in 
C, so passing INVALID_MFN is safe.

Anyway, I don't have much knowledge on the x86 to make the modification 
that you suggested. So I am going to revert to _mfn(0) for x86.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel