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

[Sergej Proskurin <proskurin@sec.in.tum.de>]

Hi Julien,


On 08/03/2016 07:40 PM, Julien Grall wrote:
> Hello Sergej,
>
> Title: s/altp2m/p2m/ and please drop the full stop.

Ok.

>
> On 01/08/16 18:10, Sergej Proskurin wrote:
>> This commit pulls out generic init/teardown functionality out of
>> p2m_init and p2m_teardown into p2m_init_one, p2m_free_one, and
>> p2m_flush_table functions.  This allows our future implementation to
>> reuse existing code for the initialization/teardown of altp2m views.
>
> Please avoid to mix-up code movement and new additions. This makes the
> code more difficult to review.

I will remove unnecessary code movements.

>
> Also, you don't mention the new changes in the commit message.

Since this patch is new to the patch series (the patch that got split is
#07, where I have commented the changes), I did not add any but rather
described the patch without being specific to the patch version.

>
> After reading the patch, it should really be divided and explain why
> you split like that.
>

Ok.

>>
>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
>> ---
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Julien Grall <julien.grall@arm.com>
>> ---
>> v2: Added the function p2m_flush_table to the previous version.
>> ---
>>  xen/arch/arm/p2m.c        | 74
>> +++++++++++++++++++++++++++++++++++++----------
>>  xen/include/asm-arm/p2m.h | 11 +++++++
>>  2 files changed, 70 insertions(+), 15 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index cbc64a1..17f3299 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1360,50 +1360,94 @@ static void p2m_free_vmid(struct domain *d)
>>      spin_unlock(&vmid_alloc_lock);
>>  }
>>
>> -void p2m_teardown(struct domain *d)
>> +/* Reset this p2m table to be empty */
>> +void p2m_flush_table(struct p2m_domain *p2m)
>
> Any function exported should have its prototype in an header within
> the same patch.
>

I will change that.

>>  {
>> -    struct p2m_domain *p2m = &d->arch.p2m;
>> -    struct page_info *pg;
>> +    struct page_info *page, *pg;
>> +    unsigned int i;
>> +
>> +    page = p2m->root;
>
>
> This function can be called with p2m->root equal to NULL. (see the
> check in p2m_free_one.
>

I will add the check, thank you.

>> +
>> +    /* Clear all concatenated first level pages */
>> +    for ( i = 0; i < P2M_ROOT_PAGES; i++ )
>> +        clear_and_clean_page(page + i);
>>
>> +    /* Free the rest of the trie pages back to the paging pool */
>>      while ( (pg = page_list_remove_head(&p2m->pages)) )
>>          free_domheap_page(pg);
>> +}
>> +
>> +static inline void p2m_free_one(struct p2m_domain *p2m)
>
> Why inline here? Also, it seems that you export the function later.
> Why don't you do it here?
>

I will do that. Thank you.

> Finally, I think this function should be rename p2m_teardown_one to
> match the callers' name.
>

Ok.

>> +{
>> +    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.

>> +    p2m->vttbr.vttbr = INVALID_VTTBR;
>
> Why do you reset vttbr, the p2m will never be used afterwards.
>

Fair. I did that just for the sake of completeness.

>>
>>      if ( p2m->root )
>>          free_domheap_pages(p2m->root, P2M_ROOT_ORDER);
>>
>>      p2m->root = NULL;
>>
>> -    p2m_free_vmid(d);
>> -
>>      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.

>>      rc = p2m_alloc_vmid(d);
>>      if ( rc != 0 )
>>          return rc;
>>
>> -    p2m->max_mapped_gfn = _gfn(0);
>> -    p2m->lowest_mapped_gfn = _gfn(ULONG_MAX);
>> -
>> -    p2m->default_access = p2m_access_rwx;
>> +    p2m->domain = d;
>> +    p2m->access_required = false;
>>      p2m->mem_access_enabled = false;
>> +    p2m->default_access = p2m_access_rwx;
>> +    p2m->root = NULL;
>> +    p2m->max_mapped_gfn = _gfn(0);
>> +    p2m->lowest_mapped_gfn = INVALID_GFN;
>
> Please don't move code when it is not necessary. This make the code
> review more difficult to read.
>

Ok.

>> +    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?

> Why altp2m_destroy_by_id don't free the p2m entirely?
> This would simply a lot this series and avoid to spread p2m
> initialization everywhere.
>

Same reason. I will change that accordingly, thank you.

>>      return rc;
>>  }
>>
>> +static void p2m_teardown_hostp2m(struct domain *d)
>> +{
>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> +
>> +    p2m_free_one(p2m);
>> +}
>> +
>> +void p2m_teardown(struct domain *d)
>> +{
>> +    p2m_teardown_hostp2m(d);
>> +}
>> +
>> +static int p2m_init_hostp2m(struct domain *d)
>> +{
>> +    int rc;
>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> +
>> +    p2m->p2m_class = p2m_host;
>> +
>> +    rc = p2m_init_one(d, p2m);
>> +    if ( rc )
>> +        return rc;
>> +
>> +    return p2m_alloc_table(d);
>> +}
>> +
>> +int p2m_init(struct domain *d)
>> +{
>> +    return p2m_init_hostp2m(d);
>> +}
>> +
>>  int relinquish_p2m_mapping(struct domain *d)
>>  {
>>      struct p2m_domain *p2m = &d->arch.p2m;
>> 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.

>>  /* Per-p2m-table state */
>>  struct p2m_domain {
>>      /* Lock that protects updates to the p2m */
>> @@ -78,6 +83,12 @@ struct p2m_domain {
>>       * enough available bits to store this information.
>>       */
>>      struct radix_tree_root mem_access_settings;
>> +
>> +    /* Choose between: host/alternate */
>> +    p2m_class_t p2m_class;
>> +
>> +    /* Back pointer to domain */
>> +    struct domain *domain;
>
> Same here. With justification why we want it.
>

Ok.

>>  };
>>
>>  /*
>>

Thank you very much.

Best regards,
~Sergej


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