Hi Henry, On 22/03/2023 03:21, Henry Wang wrote: > > > Hi Julien, > >> -----Original Message----- >> From: Julien Grall <[email protected]> >> 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 <[email protected]> > > 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
