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

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

On 08/06/2016 03:26 PM, Julien Grall wrote:
> 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.
>

I will try to avoid such code movements. Thank you.

Best regards,
~Sergej

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