Hi Julien, > -----Original Message----- > From: Julien Grall <jul...@xen.org> > Subject: Re: [PATCH v3] xen/arm: p2m: Populate pages for GICv2 mapping in > arch_domain_create() > > Hi Henry, > > On 17/10/2022 08:46, Henry Wang wrote: > > 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; > > > p2m_set_allocation() wants to be called after 'p->domain' is set. So > p2m_teardown_final() will not return early and leak memory (as Andrew > pointed out).
Yes this is a really silly mistake that I made, and I am really sorry for that. > > For simplicity I would move the code right at the end of the function. Definitely. > But if you want to keep it here then... > > > + > > p2m->vmid = INVALID_VMID; > > ... this needs to be done first as well. I agree moving that part to the end would be a better solution. > > > > > rc = p2m_alloc_vmid(d); > > Note that if you move the code at the end. Then you will need to add: > > if ( rc ) > return rc; > > after the call to p2m_alloc_table(); Yes, thanks very much for the reminder. > > Other than that the code looks good to me. As Andrew stated in his email that he will draft a patch, TBH I am not sure if my patch would be superseded by his, but I will try to see if it is possible for me to send a v4 today. Kind regards, Henry > > Cheers, > > -- > Julien Grall
