Re: [Xen-devel] [PATCH RFC 04/55] x86/mm: introduce l{1, 2}t local variables to map_pages_to_xen

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.04.19 at 14:59, <wei.liu2@xxxxxxxxxx> wrote:
> On Tue, Apr 09, 2019 at 06:49:34AM -0600, Jan Beulich wrote:
>> >>> On 09.04.19 at 14:22, <wei.liu2@xxxxxxxxxx> wrote:
>> > On Fri, Mar 15, 2019 at 09:36:37AM -0600, Jan Beulich wrote:
>> >> >>> On 07.02.19 at 17:44, <wei.liu2@xxxxxxxxxx> wrote:
>> >> > The pl2e and pl1e variables are heavily (ab)used in that function. It
>> >> > is fine at the moment because all page tables are always mapped so
>> >> > there is no need to track the life time of each variable.
>> >> > 
>> >> > We will soon have the requirement to map and unmap page tables. We
>> >> > need to track the life time of each variable to avoid leakage.
>> >> > 
>> >> > Introduce some l{1,2}t variables with limited scope so that we can
>> >> > track life time of pointers to xen page tables more easily.
>> >> 
>> >> But you retain some uses of the old variables, and to be honest it's
>> >> not really clear to me by what criteria (and having multiple instances
>> >> of a variable name in a single function isn't necessarily less confusing).
>> >> I think we either stick to what's there (doesn't look too bad to me),
>> >> or you switch to scope restricted page table pointers throughout the
>> >> function, such that the function scope symbols can go away
>> >> altogether).
>> > 
>> > I thought my commit message was clear enough: it helped tracking the
>> > lifetime of pointers more easily. There is nothing new in this trick:
>> > the less variables of the same name the better.
>> > 
>> > In this patch, places where it is clear that using local scope variables
>> > suffices are changed to use local variables.
>> > 
>> > In the end, pl*e are only used to point to entries which point to the
>> > page tables related to the linear address being mapped. They won't be
>> > used to point to any intermediate pointers which are used to manipulate
>> > page tables.
>> > 
>> > We can't eliminate function scope variables because they need to stay
>> > function scope -- see later patches where they are unmapped in the out
>> > path.
>> 
>> Except that I'm not really happy with that approach either (not
>> only but also because of the proliferation of goto-s).
> 
> The root cause is there are far too many exit paths in this function.
> Previously it was okay because we didn't need to clean up. It won't be
> okay once we have to.
> 
> I picked the option to unify them into one or two places.
> 
> One option is to duplicate cleanup code in each exit path. That's
> repetitive and error-prone IMHO.
> 
> One option is to break up map_pages_to_xen into smaller functions which
> don't require gotos but require more mapping / unmapping. I thought
> about that but it was too much faff without getting us to where we
> wanted to be.
> 
> Pick your poison. There isn't an easy solution to this...

Breaking up the function is quite likely going to become very desirable
for 5-level page tables anyway. But I'm not going to insist you do this
work as a prereq here.

Jan



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