Re: [PATCH v6 1/3] ioreq: switch ioreq page allocation to vmap

On 20.04.2026 11:38, Julian Vetter wrote:
> Switch the Xen-side ioreq page mapping from prepare_ring_for_helper() /
> map_domain_page_global() to explicit vmap(), to ensure vmap_to_page()
> can recover the struct page_info * uniformly during teardown.

What's after the comma isn't really the main reason for this patch, is it?
Describing this aspect ...

> This is a prerequisite for multi-page ioreq support: the non-buf ioreq
> region will need to span multiple pages for domains with more vCPUs than
> fit in a single page, and vmap() is the natural interface for contiguous
> multi-page Xen VA mappings.
> 
> In non-debug builds map_domain_page_global() uses the directmap for low
> MFNs rather than vmap(), so this change has a small overhead in the
> common case. Debug builds already used vmap() indirectly.
> 
> With both paths using vmap(), vmap_to_page() can recover the struct
> page_info * uniformly, so drop the 'page' field from struct ioreq_page
> and update all callers accordingly.

... here (at the bottom) is fully sufficient.

> Signed-off-by: Julian Vetter <[email protected]>
> ---
> Changes in v6:
> - Updated commit message to clearly specify why these changes are made
> - Added comment to say that this is {prepare,destroy}_ring_for_helper()
>   just using vmap_to_page() + v{map,unmap}()
> - Kept proper ordering in ioreq_server_free_mfn(), first clearing the va
>   pointer before unmapping

Yet then you didn't extend the same consideration ...

> @@ -128,8 +129,13 @@ static void hvm_unmap_ioreq_gfn(struct ioreq_server *s, bool buf)
>      if ( gfn_eq(iorp->gfn, INVALID_GFN) )
>          return;
>  
> -    destroy_ring_for_helper(&iorp->va, iorp->page);
> -    iorp->page = NULL;
> +    /* Equivalent to destroy_ring_for_helper(), using vmap_to_page(). */
> +    if ( iorp->va )
> +    {
> +        put_page_and_type(vmap_to_page(iorp->va));
> +        vunmap(iorp->va);
> +        iorp->va = NULL;
> +    }

... to here. (Really we should perhaps introduce VUNMAP(), much like we
have XFREE(), XVFREE(), etc.)

Further the ordering doesn't match destroy_ring_for_helper(), which unmaps
first and only then drops the page refs.

> @@ -162,12 +171,40 @@ static int hvm_map_ioreq_gfn(struct ioreq_server *s, bool buf)
>      if ( gfn_eq(iorp->gfn, INVALID_GFN) )
>          return -ENOMEM;
>  
> -    rc = prepare_ring_for_helper(d, gfn_x(iorp->gfn), &iorp->page,
> -                                 &iorp->va);
> -
> +    /*
> +     * Equivalent to prepare_ring_for_helper() using vmap(). Using vmap()
> +     * rather than map_domain_page_global() ensures vmap_to_page() can
> +     * recover the struct page_info * uniformly at teardown, which is
> +     * needed to support multi-page ioreq mappings (see nr_ioreq_pages()).
> +     */

"is needed" is too strong, I think - surely there would be a way to handle
that without vmap_to_page(), by tracking all struct page_info * separately.

> @@ -309,15 +310,16 @@ static int ioreq_server_alloc_mfn(struct ioreq_server *s, bool buf)
>  static void ioreq_server_free_mfn(struct ioreq_server *s, bool buf)
>  {
>      struct ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
> -    struct page_info *page = iorp->page;
> +    struct page_info *page;
> +    void *va;
>  
> -    if ( !page )
> +    if ( !iorp->va )
>          return;
>  
> -    iorp->page = NULL;
> -
> -    unmap_domain_page_global(iorp->va);
> +    va = iorp->va;

Please can this be the initializer of the variable, for the if() above to
then use that local var?

> @@ -333,7 +335,8 @@ bool is_ioreq_server_page(struct domain *d, const struct page_info *page)
>  
>      FOR_EACH_IOREQ_SERVER(d, id, s)
>      {
> -        if ( (s->ioreq.page == page) || (s->bufioreq.page == page) )
> +        if ( (s->ioreq.va && vmap_to_page(s->ioreq.va) == page) ||
> +             (s->bufioreq.va && vmap_to_page(s->bufioreq.va) == page) )
>          {
>              found = true;
>              break;

You mention in the description that some extra overhead is introduced. The
(generally) two page walks done here are particularly concerning. Since we
have a valid struct page_info * available here, I wonder if we shouldn't
aid this lookup by recording the VA in one of struct page_info's fields.
Afaics vmap() doesn't use any of the fields, so it should be relatively
easy to determine a field to use for this purpose. The more involved part
would then be to make sure the field (in other struct page_info instances)
is also properly different from any VA vmap() may return.

Jan