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

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi Julien,


On 08/05/2016 11:16 AM, Julien Grall wrote:
>
>
> On 05/08/16 08:26, Sergej Proskurin wrote:
>> Hi Julien,
>
> Hello Sergej,
>
>>
>> On 08/03/2016 07:40 PM, Julien Grall wrote:
>
> [...]
>
>>>> +{
>>>> +    p2m_flush_table(p2m);
>>>> +
>>>> +    /* Free VMID and reset VTTBR */
>>>> +    p2m_free_vmid(p2m->domain);
>>>
>>> Why do you move the call to p2m_free_vmid?
>>>
>>
>> When flushing a table, I did not want to free the associated VMID, as it
>> would need to be allocated right afterwards (see altp2m_propagate_change
>> and altp2m_reset). Since this would need to be done also in functions
>> like p2m_altp2m_reset, I moved this call to p2m_free_one. I believe
>> there is no need to free the VMIDs if the associated p2m is not freed as
>> well.
>
> That does not answer my question. You moved p2m_free_vmid within
> p2m_free_one. It used to be at the end of the function and now it is
> at the beginning. See...
>
>>>> +    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.

>>>>      radix_tree_destroy(&p2m->mem_access_settings, NULL);
>>>>  }
>>>>
>>>> -int p2m_init(struct domain *d)
>>>> +int p2m_init_one(struct domain *d, struct p2m_domain *p2m)
>>>
>>> Any function exported should have its prototype in an header within
>>> the same patch.
>>>
>>
>> I will change that, thank you.
>>
>>>>  {
>>>> -    struct p2m_domain *p2m = &d->arch.p2m;
>>>>      int rc = 0;
>>>>
>>>>      rwlock_init(&p2m->lock);
>>>>      INIT_PAGE_LIST_HEAD(&p2m->pages);
>>>>
>>>> -    p2m->vmid = INVALID_VMID;
>>>> -
>>>
>>> Why this is dropped?
>>>
>>
>> This will be shown in patch #07. We reuse altp2m views and check whether
>> a p2m was flushed by checking for a valid VMID.
>
> Which is wrong. A flushed altp2m should not need to be reinitialized
> (i.e reseting the lock...).
>
> A flushed altp2m only need to have max_mapped_gfn and
> lowest_mapped_gfn reset.
>
> [...]
>

I will try to adjust this function considering your suggestion in the
patch #07.

>>>> +    p2m->vttbr.vttbr = INVALID_VTTBR;
>>>>      radix_tree_init(&p2m->mem_access_settings);
>>>>
>>>> -    rc = p2m_alloc_table(d);
>>>> -
>>>
>>> The function p2m_init_one should fully initialize the p2m (i.e
>>> allocate the table).
>>>
>>
>> The function p2m_init_one is currently called also for initializing the
>> hostp2m, which is not dynamically allocated. Since we are sharing code
>> between the hostp2m and altp2m initialization, I solved it that way. We
>> could always allocate the hostp2m dynamically, that would solve it quite
>> easily without additional checks. The other solution can simply check,
>> whether the p2m is NULL and perform additional p2m allocation. What do
>> you think?
>
> I don't understand your point here. It does not matter whether the
> hostp2m is allocated dynamically or not.
>
> p2m_init_one should fully initialize a p2m and this does not depends
> on dynamic allocation.
>
> [...]
>

Yes, you are correct. I will move try to prevent spreading of the p2m
allocation across multiple functions. Thank you.

>>>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>>>> index 5c7cd1a..1f9c370 100644
>>>> --- a/xen/include/asm-arm/p2m.h
>>>> +++ b/xen/include/asm-arm/p2m.h
>>>> @@ -18,6 +18,11 @@ struct domain;
>>>>
>>>>  extern void memory_type_changed(struct domain *);
>>>>
>>>> +typedef enum {
>>>> +    p2m_host,
>>>> +    p2m_alternate,
>>>> +} p2m_class_t;
>>>> +
>>>
>>> This addition should really be in a separate patch.
>>>
>>
>> The function p2m_init_hostp2m uses p2m_host for initialization. I can
>> introduce a patch before this one, however to make it easier for the
>> reviewer.
>
> You did not get my point here. A patch should do one thing: moving
> code or adding new code. Not both at the same.
>
> Adding p2m_class_t and setting p2m_host should really be done in a
> separate patch. This will ease the review this patch series.

Fair enough. Thank you.

Best regards,
~Sergej


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