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

From: Wei Liu <wei.liu2@citrix.com>
To: Julien Grall <julien.grall@arm.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	xen-devel@lists.xen.org, Jan Beulich <jbeulich@suse.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH v4 08/16] xen/mm: Drop the parameter mfn from populate_pt_range
Date: Thu, 22 Feb 2018 17:10:37 +0000	[thread overview]
Message-ID: <20180222171037.7c5brisd6qbef6sg@citrix.com> (raw)
In-Reply-To: <f3384413-021b-e12e-b602-c3e5fd619282@arm.com>

On Thu, Feb 22, 2018 at 04:55:10PM +0000, Julien Grall wrote:
> Hi Wei,
> 
> On 22/02/18 16:51, Wei Liu wrote:
> > On Thu, Feb 22, 2018 at 04:40:04PM +0000, Julien Grall wrote:
> > > Hi Wei,
> > > 
> > > 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.

Alright. That would be good.

Wei.

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