|
[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). 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).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. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |