Re: [Xen-devel] [PATCH v2 04/25] arm/altp2m: Move hostp2m init/teardown to individual functions.

[Julien Grall <julien.grall@xxxxxxx>]

On 06/08/2016 09:43, Sergej Proskurin wrote:
> Hi Julien,
Hello Sergej,

> On 08/05/2016 11:16 AM, Julien Grall wrote:
> > On 05/08/16 08:26, Sergej Proskurin wrote:
> > > On 08/03/2016 07:40 PM, Julien Grall wrote:
[...]

> > > > > +    p2m->vttbr.vttbr = INVALID_VTTBR;
> > [...]
> >
> > > > >      if ( p2m->root )
> > > > >          free_domheap_pages(p2m->root, P2M_ROOT_ORDER);
> > > > >
> > > > >      p2m->root = NULL;
> > > > >
> > > > > -    p2m_free_vmid(d);
> > > > > -
> > here. So please don't move code unless there is a good reason. This
> > series is already quite difficult to read.
> This move did not have any particular reason except that for me, it
> appeared to be more logical and cleaner to read this way. Apart from
> that: This patch creates two functions out of one (in the case of the
> former p2m_teardown). Because of this, I did not even think of
> preserving a certain function call order as the former function does not
> exit in a way it used to anymore.
In this specific case, the call p2m_free_vmid is at the end of function 
to match the reverse order of p2m_init.
Regardless that I cannot see why moving p2m_free_vmid ealier will be 
more logical.
Anyway, you don't move code within a function unless there is a reason. 
And this should really be outside of a patch doing bigger.
A series like altp2m takes me about a full working day to review. So 
please don't make it more difficult to review.
Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel