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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> 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@xxxxxxxxxx>

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