[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2] xen/arm: p2m: Populate pages for GICv2 mapping in arch_domain_create()
On 14/10/2022 10:28, Henry Wang wrote: I understand both of Jan and your concern. I don't really have a strong opinion either way.Hi Jan, Thanks for the review.-----Original Message----- From: Jan Beulich <jbeulich@xxxxxxxx> Subject: Re: [PATCH v2] xen/arm: p2m: Populate pages for GICv2 mapping in arch_domain_create() On 14.10.2022 10:09, Henry Wang wrote:--- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -740,6 +740,20 @@ int arch_domain_create(struct domain *d, BUG(); } + /* + * Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2area+ * when the domain is created. Considering the worst case for page + * tables and keep a buffer, populate 16 pages to the P2M pages poolhere.+ * For GICv3, the above-mentioned P2M mapping is not necessary, butsince+ * 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 ) + goto fail;Putting this level of knowledge here feels like a layering violation to me. My first suggestion would be to move this call somewhere under p2m_init().That is definitely possible. If Julien or other Arm maintainers are not against that I am happy to move this to p2m_init() in v3. You are the author of the patch, so I will let you chose. The reason why the above block is placed here is just I thought to use d->arch.vgic.version to only populate the 16 pages for GICv2 in the beginning, and d->arch.vgic.version is first assigned later after p2m_init(), but later we decided to populated the pages unconditionally so actually now we can move the part to p2m_init().If that's not possible for some reason, I'd like to suggest passing 1 here as the count and then adding a min-acceptable check to p2m_set_allocation() along the lines of x86'es shadow_set_allocation(). That way you'd also guarantee the minimum number of pages in case a subsequent tiny allocation request came in via domctl. I really dislike this. If the user ask for 1 pages and we only allow 16. Then this should be rejected (not bumped to 16). However, the code in p2m_set_allocation() will only look at the free pages (like x86 does). So what you suggest would not do what you want. @@ -1736,6 +1736,17 @@ void p2m_final_teardown(struct domain *d) if ( !p2m->domain ) return; + if ( !page_list_empty(&p2m->pages) ) + p2m_teardown(d, false); + + if ( d->arch.paging.p2m_total_pages != 0 ) + { + spin_lock(&d->arch.paging.lock); + p2m_set_allocation(d, 0, NULL); + spin_unlock(&d->arch.paging.lock); + ASSERT(d->arch.paging.p2m_total_pages == 0); + }Is it intentional to largely open-code p2m_teardown_allocation() here?Yes, AFAICT p2m_teardown_allocation() is preemptible and we don't want any preemption here. Like Jan, I would prefer if we can avoid the duplication. The loop suggested by Jan should work. Cheers, -- Julien Grall
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |