Re: [PATCH v11 6/6] xen: retrieve reserved pages on populate_physmap

[Julien Grall <julien@xxxxxxx>]

Hi Penny,

On 31/08/2022 03:40, Penny Zheng wrote:
> +/*
> + * Acquire a page from reserved page list(resv_page_list), when populating
> + * memory for static domain on runtime.
> + */
> +mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
> +{
> +    struct page_info *page;
> +
> +    ASSERT_ALLOC_CONTEXT();
> +
> +    /* Acquire a page from reserved page list(resv_page_list). */
> +    spin_lock(&d->page_alloc_lock);
> +    page = page_list_remove_head(&d->resv_page_list);
> +    spin_unlock(&d->page_alloc_lock);
> +    if ( unlikely(!page) )
> +        return INVALID_MFN;
> +
> +    if ( !prepare_staticmem_pages(page, 1, memflags) )
> +        goto fail;
> +
> +    if ( assign_domstatic_pages(d, page, 1, memflags) )
> +        goto fail_assign;
> +
> +    return page_to_mfn(page);
> +
> + fail_assign:
> +    unprepare_staticmem_pages(page, 1, false);
Looking at assign_domstatic_pages(). It will already call 
unprepare_staticmem_pages() in one of the error path. It doesn't look 
like the latter can be called twice on a page.
To be honest, I find a bit odd that assign_domstatic_pages() is calling 
unprepare_staticmem_pages() because the former doesn't call the 
"prepare" function.
AFAICT, this is an issue introduced in this patch. So I would remove the 
call from assign_domstatic_pages() and then let the caller calls 
unprepare_staticmem_pages() (this would need to be added in 
acquire_domstatic_pages()).
Also, I think it would be good to explain why we don't need to scrub. 
Something like:
"The page was never accessible by the domain. So scrubbing can be skipped".

Cheers,

--
Julien Grall