Re: [PATCH v10 8/9] xen: retrieve reserved pages on populate_physmap

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.08.2022 04:36, Penny Zheng wrote:
> @@ -2867,6 +2854,61 @@ int __init acquire_domstatic_pages(struct domain *d, 
> mfn_t smfn,
>  
>      return 0;
>  }
> +
> +/*
> + * Acquire nr_mfns contiguous pages, starting at #smfn, of static memory,
> + * then assign them to one specific domain #d.
> + */
> +int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
> +                                   unsigned int nr_mfns, unsigned int 
> memflags)
> +{
> +    struct page_info *pg;
> +
> +    ASSERT_ALLOC_CONTEXT();
> +
> +    pg = acquire_staticmem_pages(smfn, nr_mfns, memflags);
> +    if ( !pg )
> +        return -ENOENT;
> +
> +    if ( assign_domstatic_pages(d, pg, nr_mfns, memflags) )
> +        return -EINVAL;
> +
> +    return 0;
> +}
> +
> +/*
> + * 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:
> +    free_staticmem_pages(page, 1, memflags & MEMF_no_scrub);

Doesn't this need to be !(memflags & MEMF_no_scrub)? And then -
with assignment having failed and with it being just a single page
we're talking about, the page was not exposed to the guest at any
point afaict. So I don't see the need for scrubbing in the first
place.

Also I think the rename of the function would better be done first,
since then you wouldn't need to touch this line again right in the
next patch, and the prepare/unprepare pairing would also be visible
right here. This would then also better fit with the introduction
of prepare_*() in the previous patch (which, afaic, the name
change could also be merged into; FTAOD I don't mind it to be
separate).

Jan