diff mbox series

[v2,4/4] xen/arm: Clean-up in p2m_init() and p2m_final_teardown()

Message ID 20230130040614.188296-5-Henry.Wang@arm.com (mailing list archive)
State Superseded
Headers show
Series P2M improvements for Arm | expand

Commit Message

Henry Wang Jan. 30, 2023, 4:06 a.m. UTC 
With the change in previous patch, the initial 16 pages in the P2M
pool is not necessary anymore. Drop them for code simplification.

Also the call to p2m_teardown() from arch_domain_destroy() is not
necessary anymore since the movement of the P2M allocation out of
arch_domain_create(). Drop the code and the above in-code comment
mentioning it.

Signed-off-by: Henry Wang <Henry.Wang@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
---
I am not entirely sure if I should also drop the "TODO" on top of
the p2m_set_entry(). Because although we are sure there is no
p2m pages populated in domain_create() stage now, but we are not
sure if anyone will add more in the future...Any comments?
v1 -> v2:
1. Add Reviewed-by tag from Michal.
---
 xen/arch/arm/include/asm/p2m.h |  4 ----
 xen/arch/arm/p2m.c             | 20 +-------------------
 2 files changed, 1 insertion(+), 23 deletions(-)

Comments

Julien Grall March 21, 2023, 3:50 p.m. UTC | #1 
Hi Henry,

On 30/01/2023 04:06, Henry Wang wrote:
> With the change in previous patch, the initial 16 pages in the P2M
> pool is not necessary anymore. Drop them for code simplification.
> 
> Also the call to p2m_teardown() from arch_domain_destroy() is not
> necessary anymore since the movement of the P2M allocation out of
> arch_domain_create(). Drop the code and the above in-code comment
> mentioning it.
> 
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> ---
> I am not entirely sure if I should also drop the "TODO" on top of
> the p2m_set_entry(). Because although we are sure there is no
> p2m pages populated in domain_create() stage now, but we are not
> sure if anyone will add more in the future...Any comments?

I would keep it.

> v1 -> v2:
> 1. Add Reviewed-by tag from Michal.
> ---
>   xen/arch/arm/include/asm/p2m.h |  4 ----
>   xen/arch/arm/p2m.c             | 20 +-------------------
>   2 files changed, 1 insertion(+), 23 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
> index bf5183e53a..cf06d3cc21 100644
> --- a/xen/arch/arm/include/asm/p2m.h
> +++ b/xen/arch/arm/include/asm/p2m.h
> @@ -200,10 +200,6 @@ int p2m_init(struct domain *d);
>    *  - p2m_final_teardown() will be called when domain struct is been
>    *    freed. This *cannot* be preempted and therefore one small

NIT: As you are touching the comment, would you mind to fix the typo 
s/one/only/.

>    *    resources should be freed here.
> - *  Note that p2m_final_teardown() will also call p2m_teardown(), to properly
> - *  free the P2M when failures happen in the domain creation with P2M pages
> - *  already in use. In this case p2m_teardown() is called non-preemptively and
> - *  p2m_teardown() will always return 0.
>    */
>   int p2m_teardown(struct domain *d, bool allow_preemption);
>   void p2m_final_teardown(struct domain *d);
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index f1ccd7efbd..46406a9eb1 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1750,13 +1750,9 @@ void p2m_final_teardown(struct domain *d)
>       /*
>        * No need to call relinquish_p2m_mapping() here because
>        * p2m_final_teardown() is called either after domain_relinquish_resources()
> -     * where relinquish_p2m_mapping() has been called, or from failure path of
> -     * domain_create()/arch_domain_create() where mappings that require
> -     * p2m_put_l3_page() should never be created. For the latter case, also see
> -     * comment on top of the p2m_set_entry() for more info.
> +     * where relinquish_p2m_mapping() has been called.
>        */
>   
> -    BUG_ON(p2m_teardown(d, false));

With this change, I think we can also drop the second parameter of 
p2m_teardown(). Preferably with this change in the patch:

Acked-by: Julien Grall <jgrall@amazon.com>

Preferably, I would like this to happen in this patch.

Cheers,
Henry Wang March 22, 2023, 2:21 a.m. UTC | #2 
Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Subject: Re: [PATCH v2 4/4] xen/arm: Clean-up in p2m_init() and
> p2m_final_teardown()
> 
> Hi Henry,
> 
> > ---
> > I am not entirely sure if I should also drop the "TODO" on top of
> > the p2m_set_entry(). Because although we are sure there is no
> > p2m pages populated in domain_create() stage now, but we are not
> > sure if anyone will add more in the future...Any comments?
> 
> I would keep it.

Sure.

> 
> > @@ -200,10 +200,6 @@ int p2m_init(struct domain *d);
> >    *  - p2m_final_teardown() will be called when domain struct is been
> >    *    freed. This *cannot* be preempted and therefore one small
> 
> NIT: As you are touching the comment, would you mind to fix the typo
> s/one/only/.

I would be more than happy to fix it. Thanks for noticing this :)

> 
> > -    BUG_ON(p2m_teardown(d, false));
> 
> With this change, I think we can also drop the second parameter of
> p2m_teardown(). Preferably with this change in the patch:

Yes indeed, I was also thinking of this when writing this patch but in
the end decided to do minimal change..

> 
> Acked-by: Julien Grall <jgrall@amazon.com>

Thanks! I am not 100% comfortable to take this tag because I made
some extra code change, below is the diff to drop the second param
of p2m_teardown():

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
@@ -1030,7 +1030,7 @@ int domain_relinquish_resources(struct domain *d)
         p2m_clear_root_pages(&d->arch.p2m);

     PROGRESS(p2m):
-        ret = p2m_teardown(d, true);
+        ret = p2m_teardown(d);
         if ( ret )
             return ret;

diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
@@ -201,7 +201,7 @@ int p2m_init(struct domain *d);
  *    freed. This *cannot* be preempted and therefore only small
  *    resources should be freed here.
  */
-int p2m_teardown(struct domain *d, bool allow_preemption);
+int p2m_teardown(struct domain *d);
 void p2m_final_teardown(struct domain *d);

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
-int p2m_teardown(struct domain *d, bool allow_preemption)
+int p2m_teardown(struct domain *d)
 {
[...]
         /* Arbitrarily preempt every 512 iterations */
-        if ( allow_preemption && !(count % 512) && hypercall_preempt_check() )
+        if ( !(count % 512) && hypercall_preempt_check() )

If you are happy, I will take this acked-by. Same question to Michal for his
Reviewed-by.

Kind regards,
Henry

> 
> Preferably, I would like this to happen in this patch.
> 
> Cheers,
> 
> --
> Julien Grall
Michal Orzel March 22, 2023, 8:02 a.m. UTC | #3 
Hi Henry,

On 22/03/2023 03:21, Henry Wang wrote:
> 
> 
> Hi Julien,
> 
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Subject: Re: [PATCH v2 4/4] xen/arm: Clean-up in p2m_init() and
>> p2m_final_teardown()
>>
>> Hi Henry,
>>
>>> ---
>>> I am not entirely sure if I should also drop the "TODO" on top of
>>> the p2m_set_entry(). Because although we are sure there is no
>>> p2m pages populated in domain_create() stage now, but we are not
>>> sure if anyone will add more in the future...Any comments?
>>
>> I would keep it.
> 
> Sure.
> 
>>
>>> @@ -200,10 +200,6 @@ int p2m_init(struct domain *d);
>>>    *  - p2m_final_teardown() will be called when domain struct is been
>>>    *    freed. This *cannot* be preempted and therefore one small
>>
>> NIT: As you are touching the comment, would you mind to fix the typo
>> s/one/only/.
> 
> I would be more than happy to fix it. Thanks for noticing this :)
> 
>>
>>> -    BUG_ON(p2m_teardown(d, false));
>>
>> With this change, I think we can also drop the second parameter of
>> p2m_teardown(). Preferably with this change in the patch:
> 
> Yes indeed, I was also thinking of this when writing this patch but in
> the end decided to do minimal change..
> 
>>
>> Acked-by: Julien Grall <jgrall@amazon.com>
> 
> Thanks! I am not 100% comfortable to take this tag because I made
> some extra code change, below is the diff to drop the second param
> of p2m_teardown():
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> @@ -1030,7 +1030,7 @@ int domain_relinquish_resources(struct domain *d)
>          p2m_clear_root_pages(&d->arch.p2m);
> 
>      PROGRESS(p2m):
> -        ret = p2m_teardown(d, true);
> +        ret = p2m_teardown(d);
>          if ( ret )
>              return ret;
> 
> diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
> @@ -201,7 +201,7 @@ int p2m_init(struct domain *d);
>   *    freed. This *cannot* be preempted and therefore only small
>   *    resources should be freed here.
>   */
> -int p2m_teardown(struct domain *d, bool allow_preemption);
> +int p2m_teardown(struct domain *d);
>  void p2m_final_teardown(struct domain *d);
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> -int p2m_teardown(struct domain *d, bool allow_preemption)
> +int p2m_teardown(struct domain *d)
>  {
> [...]
>          /* Arbitrarily preempt every 512 iterations */
> -        if ( allow_preemption && !(count % 512) && hypercall_preempt_check() )
> +        if ( !(count % 512) && hypercall_preempt_check() )
> 
> If you are happy, I will take this acked-by. Same question to Michal for his
> Reviewed-by.
The diff looks good to me and surely you can keep my tag.
However, we might want to modify the comment on top of p2m_teardown() prototype as
it assumes presence of preemptive/non-preemptive p2m_teardown() call (at least this
is how I understand this).

~Michal
Henry Wang March 22, 2023, 8:16 a.m. UTC | #4 
Hi Michal,

> -----Original Message-----
> From: Michal Orzel <michal.orzel@amd.com>
> Subject: Re: [PATCH v2 4/4] xen/arm: Clean-up in p2m_init() and
> p2m_final_teardown()
> > If you are happy, I will take this acked-by. Same question to Michal for his
> > Reviewed-by.
> The diff looks good to me and surely you can keep my tag.

Great, thanks!

> However, we might want to modify the comment on top of p2m_teardown()
> prototype as
> it assumes presence of preemptive/non-preemptive p2m_teardown() call (at
> least this
> is how I understand this).

I understand your point. I will revert it to its original form written by Julien:

"p2m_teardown() will be called when relinquish the resources. It
will free large resources (e.g. intermediate page-tables) that
requires preemption."

Kind regards,
Henry

> 
> ~Michal
diff mbox series

Patch

diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
index bf5183e53a..cf06d3cc21 100644
--- a/xen/arch/arm/include/asm/p2m.h
+++ b/xen/arch/arm/include/asm/p2m.h
@@ -200,10 +200,6 @@  int p2m_init(struct domain *d);
  *  - p2m_final_teardown() will be called when domain struct is been
  *    freed. This *cannot* be preempted and therefore one small
  *    resources should be freed here.
- *  Note that p2m_final_teardown() will also call p2m_teardown(), to properly
- *  free the P2M when failures happen in the domain creation with P2M pages
- *  already in use. In this case p2m_teardown() is called non-preemptively and
- *  p2m_teardown() will always return 0.
  */
 int p2m_teardown(struct domain *d, bool allow_preemption);
 void p2m_final_teardown(struct domain *d);
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index f1ccd7efbd..46406a9eb1 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1750,13 +1750,9 @@  void p2m_final_teardown(struct domain *d)
     /*
      * No need to call relinquish_p2m_mapping() here because
      * p2m_final_teardown() is called either after domain_relinquish_resources()
-     * where relinquish_p2m_mapping() has been called, or from failure path of
-     * domain_create()/arch_domain_create() where mappings that require
-     * p2m_put_l3_page() should never be created. For the latter case, also see
-     * comment on top of the p2m_set_entry() for more info.
+     * where relinquish_p2m_mapping() has been called.
      */
 
-    BUG_ON(p2m_teardown(d, false));
     ASSERT(page_list_empty(&p2m->pages));
 
     while ( p2m_teardown_allocation(d) == -ERESTART )
@@ -1827,20 +1823,6 @@  int p2m_init(struct domain *d)
     if ( rc )
         return rc;
 
-    /*
-     * Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area
-     * when the domain is created. Considering the worst case for page
-     * tables and keep a buffer, populate 16 pages to the P2M pages pool here.
-     * For GICv3, the above-mentioned P2M mapping is not necessary, but since
-     * the allocated 16 pages here would not be lost, hence populate these
-     * pages unconditionally.
-     */
-    spin_lock(&d->arch.paging.lock);
-    rc = p2m_set_allocation(d, 16, NULL);
-    spin_unlock(&d->arch.paging.lock);
-    if ( rc )
-        return rc;
-
     return 0;
 }