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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.03.18 at 14:43, <julien.grall@xxxxxxx> wrote:
> On 02/03/18 14:55, Jan Beulich wrote:
>>>>> On 22.02.18 at 17:55, <julien.grall@xxxxxxx> 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.

I didn't say it's unsafe. It's clumsy and misleading.

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

I'd prefer if you didn't, but well, it'll be one of us to clean it up
then.

Jan


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