| Message ID | 20230116015820.1269387-4-Henry.Wang@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | P2M improvements for Arm | expand |
Hi Henry, On 16/01/2023 02:58, 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> > --- > 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? > --- > 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 > * 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 7de7d822e9..d41a316d18 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -1744,13 +1744,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)); Because you remove this, > ASSERT(page_list_empty(&p2m->pages)); you no longer need this assert, right? Apart from that: Reviewed-by: Michal Orzel <michal.orzel@amd.com> ~Michal
Hi, On 20/01/2023 10:23, Michal Orzel wrote: > Hi Henry, > > On 16/01/2023 02:58, 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> >> --- >> 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? >> --- >> 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 >> * 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 7de7d822e9..d41a316d18 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -1744,13 +1744,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)); > Because you remove this, >> ASSERT(page_list_empty(&p2m->pages)); > you no longer need this assert, right? I think the ASSERT() is still useful as it at least show that the pages should have been freed before the call to p2m_final_teardown(). Cheers,
Hi Michal, > -----Original Message----- > >> > >> - BUG_ON(p2m_teardown(d, false)); > > Because you remove this, > >> ASSERT(page_list_empty(&p2m->pages)); > > you no longer need this assert, right? > I think the ASSERT() is still useful as it at least show that the pages > should have been freed before the call to p2m_final_teardown(). I think I also prefer to have this ASSERT(), because of the exactly same reason as Julien's answer. I think having this ASSERT() will help us to avoid potential mistakes in the future. May I please ask if you are happy with keeping this ASSERT() and I can carry your reviewed-by tag? Thanks! Kind regards, Henry > > Cheers, > > -- > Julien Grall
Hi Henry, On 27/01/2023 12:15, Henry Wang wrote: > > > Hi Michal, > >> -----Original Message----- >>>> >>>> - BUG_ON(p2m_teardown(d, false)); >>> Because you remove this, >>>> ASSERT(page_list_empty(&p2m->pages)); >>> you no longer need this assert, right? >> I think the ASSERT() is still useful as it at least show that the pages >> should have been freed before the call to p2m_final_teardown(). > > I think I also prefer to have this ASSERT(), because of the exactly same > reason as Julien's answer. I think having this ASSERT() will help us to > avoid potential mistakes in the future. > > May I please ask if you are happy with keeping this ASSERT() and I can > carry your reviewed-by tag? Thanks! Yes, you can :) ~Michal
Hi Michal, > -----Original Message----- > From: Michal Orzel <michal.orzel@amd.com> > Subject: Re: [PATCH 3/3] xen/arm: Clean-up in p2m_init() and > p2m_final_teardown() > > Hi Henry, > > On 27/01/2023 12:15, Henry Wang wrote: > > > > > > Hi Michal, > > > >> -----Original Message----- > >>>> > >>>> - BUG_ON(p2m_teardown(d, false)); > >>> Because you remove this, > >>>> ASSERT(page_list_empty(&p2m->pages)); > >>> you no longer need this assert, right? > >> I think the ASSERT() is still useful as it at least show that the pages > >> should have been freed before the call to p2m_final_teardown(). > > > > I think I also prefer to have this ASSERT(), because of the exactly same > > reason as Julien's answer. I think having this ASSERT() will help us to > > avoid potential mistakes in the future. > > > > May I please ask if you are happy with keeping this ASSERT() and I can > > carry your reviewed-by tag? Thanks! > Yes, you can :) Thank you very much! And also thank you for your detailed review :) Kind regards, Henry > > ~Michal
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 7de7d822e9..d41a316d18 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1744,13 +1744,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 ) @@ -1821,20 +1817,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; }
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> --- 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? --- xen/arch/arm/include/asm/p2m.h | 4 ---- xen/arch/arm/p2m.c | 20 +------------------- 2 files changed, 1 insertion(+), 23 deletions(-)