On 17/10/2022 08:46, Henry Wang wrote: > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index f17500ddf3..8c9ddf58e1 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -1736,7 +1739,20 @@ void p2m_final_teardown(struct domain *d) > if ( !p2m->domain ) > return; >
Everything below here is "dead" code, because... > + /* > + * 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. > + */ > + > + BUG_ON(p2m_teardown(d, false)); > ASSERT(page_list_empty(&p2m->pages)); > + > + while ( p2m_teardown_allocation(d) == -ERESTART ) > + continue; /* No preemption support here */ > ASSERT(page_list_empty(&d->arch.paging.p2m_freelist)); > > if ( p2m->root ) > @@ -1762,6 +1778,20 @@ int p2m_init(struct domain *d) > INIT_PAGE_LIST_HEAD(&p2m->pages); > INIT_PAGE_LIST_HEAD(&d->arch.paging.p2m_freelist); > > + /* > + * 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 != 0 ) > + return rc; ... this early exit is ahead of p2m_init() settings p2m->domain = d. In particular, you introduce a bug whereby ... > + > p2m->vmid = INVALID_VMID; > > rc = p2m_alloc_vmid(d); ... this error path now leaks the 16 page p2m allocation. This change is overly complex. You add a set_allocation(16) path in p2m_init(), so should only be adding a single set_allocation(0) to compensate. The `while ( p2m_teardown_allocation(d) == -ERESTART ) continue;` is especially silly because you're specifically wasting time ignoring the preemption wrapper around the non-preemptible function that you actually want to use. Looking at between 4.13 and staging, you want to be calling set_allocation(0) in p2m_final_teardown() ahead of of the p->domain check. Except idempotency which is going to be irritating here. It will be a no-op from the normal domain destroy path (as relinquish resource will have emptied the whole pool), while in domain_create error path, it has a maximum of 16 pages to release. I'll draft a patch. ~Andrew
