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 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 )
> + 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(). 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.
> @@ -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?
Jan
