[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Hi Jan,

On 05/03/18 14:00, Jan Beulich wrote:
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.

Well, that's just the way the page-table code has been written on x86. On Arm, the code is much clearer with INVALID_MFN than _mfn(0).


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.
I can keep as INVALID_MFN. But then either you or Andrew (or anyone x86 folks) would have to provide the patch to skip incrementing invalid MFN (if I understood correctly your request).

Cheers,

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.