Re: [PATCH v6 3/3] x86/ioreq: Extend ioreq server to support multiple ioreq pages

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.04.2026 11:38, Julian Vetter wrote:
> As the number of vCPUs grows, a single ioreq page of 128 slots may not
> be sufficient. Add support for allocating and mapping multiple ioreq
> pages so that the ioreq region can scale with d->max_vcpus.
> 
> Introduce nr_ioreq_pages() to compute the number of pages required for
> a given domain, and IOREQ_NR_PAGES_MAX as a compile-time upper bound
> (based on HVM_MAX_VCPUS).
> 
> ioreq_server_alloc_mfn() is updated to allocate nr_ioreq_pages() pages
> and map them contiguously via vmap().
> 
> is_ioreq_server_page() iterates over all ioreq pages when checking
> page ownership. ioreq_server_get_frame() allows callers to retrieve any
> ioreq page by index via the XENMEM_acquire_resource interface.
> 
> On x86, the legacy GFN mapping path (hvm_map_ioreq_gfn) is limited to
> a single ioreq page; device models requiring more ioreq slots must use
> the resource mapping interface (XENMEM_acquire_resource).
> 
> Signed-off-by: Julian Vetter <julian.vetter@xxxxxxxxxx>
> ---
> Changes in v6:
> - Adapted the comment to not mention the guest, but the device model
> - Replaced the dynamic allocation for the mfns array by a static array
> - Fixed error handling in ioreq_server_alloc_mfn, using an extra
>   nr_alloc variable to track the already allocated pages
> - Dropped unnecessary void casts
> ---
>  xen/arch/x86/hvm/ioreq.c |  8 ++++
>  xen/common/ioreq.c       | 93 ++++++++++++++++++++++++++++------------
>  xen/include/xen/ioreq.h  | 12 ++++++
>  3 files changed, 86 insertions(+), 27 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> index 3cabec141c..ee679bdf5a 100644
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -166,6 +166,14 @@ static int hvm_map_ioreq_gfn(struct ioreq_server *s, 
> bool buf)
>      if ( d->is_dying )
>          return -EINVAL;
>  
> +    /*
> +     * The legacy GFN path supports only a single ioreq page. Device models
> +     * requiring more ioreq slots must use the resource mapping interface
> +     * (XENMEM_acquire_resource).
> +     */
> +    if ( !buf && nr_ioreq_pages(d) > 1 )
> +        return -EOPNOTSUPP;
> +
>      iorp->gfn = hvm_alloc_ioreq_gfn(s);
>  
>      if ( gfn_eq(iorp->gfn, INVALID_GFN) )
> diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
> index bae9b99c99..3a08e77597 100644
> --- a/xen/common/ioreq.c
> +++ b/xen/common/ioreq.c
> @@ -261,8 +261,11 @@ bool vcpu_ioreq_handle_completion(struct vcpu *v)
>  static int ioreq_server_alloc_mfn(struct ioreq_server *s, bool buf)
>  {
>      struct ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
> -    struct page_info *page;
> -    mfn_t mfn;
> +    unsigned int i, nr_alloc = 0, nr_pages = buf ? 1 : 
> nr_ioreq_pages(s->target);
> +    mfn_t mfns[IOREQ_NR_PAGES_MAX] = {};

This is okay to have with the present HVM_MAX_VCPUS, but we need to put in
place something to bound stack usage here. Once the upper bound on the
number of vCPU-s has grown enough, some other mechanism will need to be put
in place, preferably still without runtime allocation. (And no, this isn't
a static array, i.e. the revlog entry isn't quite correct.)

For the moment I'd suggest BUILD_BUG_ON(ARRAY_SIZE(mfns) > 32), with a
clarifying comment.

> +    int rc;
> +
> +    ASSERT(nr_pages <= IOREQ_NR_PAGES_MAX);

Why would this be relevant to check (only) here? Imo this either wants
dropping, or moving into nr_ioreq_pages().

> @@ -277,11 +280,16 @@ static int ioreq_server_alloc_mfn(struct ioreq_server 
> *s, bool buf)
>          return 0;
>      }
>  
> +    for ( i = 0; i < nr_pages; i++ )
>      {
> -        page = alloc_domheap_page(s->target, MEMF_no_refcount);
> +        struct page_info *page = alloc_domheap_page(s->target,
> +                                                    MEMF_no_refcount);

This movement of the decl would better also be part of patch 2.

> @@ -290,41 +298,59 @@ static int ioreq_server_alloc_mfn(struct ioreq_server 
> *s, bool buf)
>               * here is a clear indication of something fishy going on.
>               */
>              domain_crash(s->emulator);
> -            return -ENODATA;
> +            rc = -ENODATA;
> +            goto fail;
>          }
>  
> -        mfn = page_to_mfn(page);
> +        mfns[nr_alloc++] = page_to_mfn(page);
>      }
> -    iorp->va = vmap(&mfn, 1);
> +
> +    iorp->va = vmap(mfns, nr_pages);
>      if ( !iorp->va )
> +    {
> +        rc = -ENOMEM;
>          goto fail;
> +    }
>  
> -    clear_page(iorp->va);
> +    memset(iorp->va, 0, nr_pages * PAGE_SIZE);

clear_page() is a bit more efficient, so I wonder whether - especially for
small nr_pages - we aren't needlessly losing performance here. Question is
whether there's a reasonable to establish boundary at which memset()
(largely) catches up.

> @@ -337,12 +363,25 @@ bool is_ioreq_server_page(struct domain *d, const 
> struct page_info *page)
>  
>      FOR_EACH_IOREQ_SERVER(d, id, s)
>      {
> -        if ( (s->ioreq.va && vmap_to_page(s->ioreq.va) == page) ||
> -             (s->bufioreq.va && vmap_to_page(s->bufioreq.va) == page) )
> +        unsigned int i;
> +
> +        if ( s->bufioreq.va && vmap_to_page(s->bufioreq.va) == page )
>          {
>              found = true;
>              break;
>          }
> +
> +        for ( i = 0; i < nr_ioreq_pages(d) && s->ioreq.va; i++ )

s->ioreq.va is loop invariant, without the compiler being in the position
to know. The condition therefore wants moving out, preferably as

        if ( !s->ioreq.va )
            continue;

to avoid indentation growing too much.

> +        {
> +            if ( vmap_to_page(s->ioreq.va + i * PAGE_SIZE) == page )
> +            {
> +                found = true;
> +                break;
> +            }
> +        }

It may help readability here if for()'s curly braces were dropped.

> +        if ( found )
> +            break;
>      }
>  
>      rspin_unlock(&d->ioreq_server.lock);
> @@ -816,26 +855,26 @@ int ioreq_server_get_frame(struct domain *d, ioservid_t 
> id,
>      if ( rc )
>          goto out;
>  
> -    switch ( idx )
> +    if ( idx == XENMEM_resource_ioreq_server_frame_bufioreq )
>      {
> -    case XENMEM_resource_ioreq_server_frame_bufioreq:
>          rc = -ENOENT;
>          if ( !HANDLE_BUFIOREQ(s) )
>              goto out;
>  
>          *mfn = vmap_to_mfn(s->bufioreq.va);
>          rc = 0;
> -        break;
> +    }
> +    else if ( idx >= XENMEM_resource_ioreq_server_frame_ioreq(0) &&
> +              idx < 
> XENMEM_resource_ioreq_server_frame_ioreq(nr_ioreq_pages(d)) )
> +    {
> +        unsigned int page_idx = idx - 
> XENMEM_resource_ioreq_server_frame_ioreq(0);
>  
> -    case XENMEM_resource_ioreq_server_frame_ioreq(0):
> -        *mfn = vmap_to_mfn(s->ioreq.va);
> +        ASSERT(page_idx < nr_ioreq_pages(d));

Isn't this redundant with the range check on idx?

> --- a/xen/include/xen/ioreq.h
> +++ b/xen/include/xen/ioreq.h
> @@ -35,6 +35,18 @@ struct ioreq_vcpu {
>      bool             pending;
>  };
>  
> +/*
> + * Maximum number of ioreq pages, based on the maximum number
> + * of vCPUs and the number of ioreq slots per page.
> + */
> +#define IOREQ_NR_PAGES_MAX \
> +    DIV_ROUND_UP(HVM_MAX_VCPUS, PAGE_SIZE / sizeof(ioreq_t))
> +
> +static inline unsigned int nr_ioreq_pages(const struct domain *d)
> +{
> +    return DIV_ROUND_UP(d->max_vcpus, PAGE_SIZE / sizeof(ioreq_t));
> +}

To reduce redundancy, how about

static inline unsigned int nr_ioreq_pages(const struct domain *d)
{
    return DIV_ROUND_UP(d ? d->max_vcpus : HVM_MAX_VCPUS,
                        PAGE_SIZE / sizeof(ioreq_t));
}

#define IOREQ_NR_PAGES_MAX nr_ioreq_pages(NULL)

?

Jan