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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.02.2026 13:30, Julian Vetter wrote:
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -71,6 +71,38 @@ static gfn_t hvm_alloc_legacy_ioreq_gfn(struct 
> ioreq_server *s)
>      return INVALID_GFN;
>  }
>  
> +static gfn_t hvm_alloc_ioreq_gfns(struct ioreq_server *s,
> +                                  unsigned int nr_pages)
> +{
> +    struct domain *d = s->target;
> +    unsigned long mask = d->arch.hvm.ioreq_gfn.mask;
> +    unsigned int i, run;
> +
> +    /* Find nr_pages consecutive set bits */
> +    for ( i = 0, run = 0; i < BITS_PER_LONG; i++ )
> +    {
> +        if ( test_bit(i, &mask) )
> +        {
> +            if ( ++run == nr_pages )
> +            {
> +                /* Found a run - clear all bits and return base GFN */
> +                unsigned int start = i - nr_pages + 1;
> +                for ( unsigned int j = start; j <= i; j++ )
> +                    clear_bit(j, &d->arch.hvm.ioreq_gfn.mask);
> +                return _gfn(d->arch.hvm.ioreq_gfn.base + start);
> +            }

We may want to gain a bitmap library function for this. Sadly
bitmap_find_free_region() is too special purpose for the needs here.
Otherwise I think ...

> +        }
> +        else
> +            run = 0;

... the construct as a whole would benefit from re-working into
if/else-if, as that'll reduce indentation by one level for the main
block of code.

Also, nit: Blank line please between declaration(s) and statement(s).

> @@ -121,52 +153,95 @@ static void hvm_free_ioreq_gfn(struct ioreq_server *s, 
> gfn_t gfn)
>      }
>  }
>  
> +static void hvm_free_ioreq_gfns(struct ioreq_server *s, gfn_t gfn,
> +                                unsigned int nr_pages)
> +{
> +    unsigned int i;
> +
> +    for ( i = 0; i < nr_pages; i++ )
> +        hvm_free_ioreq_gfn(s, _gfn(gfn_x(gfn) + i));
> +}
> +
>  static void hvm_unmap_ioreq_gfn(struct ioreq_server *s, bool buf)
>  {
> -    struct ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
> +    unsigned int i, nr_pages = buf ? 1 : NR_IOREQ_PAGES;
>  
> -    if ( gfn_eq(iorp->gfn, INVALID_GFN) )
> -        return;
> +    for ( i = 0; i < nr_pages; i++ )
> +    {
> +        struct ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreqs.page[i];
> +
> +        if ( gfn_eq(iorp->gfn, INVALID_GFN) )
> +            continue;
>  
> -    destroy_ring_for_helper(&iorp->va, iorp->page);
> -    iorp->page = NULL;
> +        destroy_ring_for_helper(&iorp->va, iorp->page);
> +        iorp->page = NULL;
>  
> -    hvm_free_ioreq_gfn(s, iorp->gfn);
> -    iorp->gfn = INVALID_GFN;
> +        hvm_free_ioreq_gfn(s, iorp->gfn);
> +        iorp->gfn = INVALID_GFN;
> +    }
>  }
>  
>  static int hvm_map_ioreq_gfn(struct ioreq_server *s, bool buf)
>  {
>      struct domain *d = s->target;
> -    struct ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
> +    unsigned int i, nr_pages = buf ? 1 : NR_IOREQ_PAGES;
> +    gfn_t base_gfn;
>      int rc;
>  
> -    if ( iorp->page )
> +    /* Check if already mapped */
> +    for ( i = 0; i < nr_pages; i++ )
>      {
> -        /*
> -         * If a page has already been allocated (which will happen on
> -         * demand if ioreq_server_get_frame() is called), then
> -         * mapping a guest frame is not permitted.
> -         */
> -        if ( gfn_eq(iorp->gfn, INVALID_GFN) )
> -            return -EPERM;
> +        struct ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreqs.page[i];
>  
> -        return 0;
> +        if ( iorp->page )
> +        {
> +            /*
> +             * If a page has already been allocated (which will happen on
> +             * demand if ioreq_server_get_frame() is called), then
> +             * mapping a guest frame is not permitted.
> +             */
> +            if ( gfn_eq(iorp->gfn, INVALID_GFN) )
> +                return -EPERM;
> +
> +            return 0;
> +        }

How can you simply return here when you found one page already mapped?
(This will likely solve itself when the data structure is changed; see
remark at the very bottom.)

> @@ -174,32 +249,43 @@ static int hvm_map_ioreq_gfn(struct ioreq_server *s, 
> bool buf)
>  static void hvm_remove_ioreq_gfn(struct ioreq_server *s, bool buf)
>  {
>      struct domain *d = s->target;
> -    struct ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
> +    unsigned int i, nr_pages = buf ? 1 : NR_IOREQ_PAGES;
>  
> -    if ( gfn_eq(iorp->gfn, INVALID_GFN) )
> -        return;
> +    for ( i = 0; i < nr_pages; i++ )
> +    {
> +        struct ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreqs.page[i];
> +
> +        if ( gfn_eq(iorp->gfn, INVALID_GFN) )
> +            continue;
>  
> -    if ( p2m_remove_page(d, iorp->gfn, page_to_mfn(iorp->page), 0) )
> -        domain_crash(d);
> -    clear_page(iorp->va);
> +        if ( p2m_remove_page(d, iorp->gfn, page_to_mfn(iorp->page), 0) )
> +            domain_crash(d);
> +        clear_page(iorp->va);
> +    }
>  }
>  
>  static int hvm_add_ioreq_gfn(struct ioreq_server *s, bool buf)
>  {
>      struct domain *d = s->target;
> -    struct ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
> +    unsigned int i, nr_pages = buf ? 1 : NR_IOREQ_PAGES;
>      int rc;
>  
> -    if ( gfn_eq(iorp->gfn, INVALID_GFN) )
> -        return 0;
> +    for ( i = 0; i < nr_pages; i++ )
> +    {
> +        struct ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreqs.page[i];
>  
> -    clear_page(iorp->va);
> +        if ( gfn_eq(iorp->gfn, INVALID_GFN) )
> +            continue;
>  
> -    rc = p2m_add_page(d, iorp->gfn, page_to_mfn(iorp->page), 0, p2m_ram_rw);
> -    if ( rc == 0 )
> -        paging_mark_pfn_dirty(d, _pfn(gfn_x(iorp->gfn)));
> +        clear_page(iorp->va);
>  
> -    return rc;
> +        rc = p2m_add_page(d, iorp->gfn, page_to_mfn(iorp->page), 0, 
> p2m_ram_rw);
> +        if ( rc )
> +            return rc;

No rolling back of what was successfully done before will want explaining
in a comment.

> @@ -260,84 +263,120 @@ 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;
> +    unsigned int i, j, nr_pages = buf ? 1 : NR_IOREQ_PAGES;
>  
> -    if ( iorp->page )
> +    for ( i = 0; i < nr_pages; i++ )
>      {
> -        /*
> -         * If a guest frame has already been mapped (which may happen
> -         * on demand if ioreq_server_get_info() is called), then
> -         * allocating a page is not permitted.
> -         */
> -        if ( !gfn_eq(iorp->gfn, INVALID_GFN) )
> -            return -EPERM;
> +        struct ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreqs.page[i];
>  
> -        return 0;
> -    }
> +        if ( iorp->page )
> +        {
> +            /*
> +             * If a guest frame has already been mapped (which may happen
> +             * on demand if ioreq_server_get_info() is called), then
> +             * allocating a page is not permitted.
> +             */
> +            if ( !gfn_eq(iorp->gfn, INVALID_GFN) )
> +                return -EPERM;
> +            continue;  /* Already allocated */
> +        }
>  
> -    page = alloc_domheap_page(s->target, MEMF_no_refcount);
> +        page = alloc_domheap_page(s->target, MEMF_no_refcount);
> +        if ( !page )
> +            goto fail;
>  
> -    if ( !page )
> -        return -ENOMEM;
> +        if ( !get_page_and_type(page, s->target, PGT_writable_page) )
> +        {
> +            /*
> +             * The domain can't possibly know about this page yet, so failure
> +             * here is a clear indication of something fishy going on.
> +             */
> +            put_page_alloc_ref(page);
> +            domain_crash(s->emulator);
> +            return -ENODATA;
> +        }
>  
> -    if ( !get_page_and_type(page, s->target, PGT_writable_page) )
> -    {
> -        /*
> -         * The domain can't possibly know about this page yet, so failure
> -         * here is a clear indication of something fishy going on.
> -         */
> -        domain_crash(s->emulator);
> -        return -ENODATA;
> -    }
> +        /* Assign early so cleanup can find it */
> +        iorp->page = page;
>  
> -    iorp->va = __map_domain_page_global(page);
> -    if ( !iorp->va )
> -        goto fail;
> +        iorp->va = __map_domain_page_global(page);
> +        if ( !iorp->va )
> +            goto fail;
> +
> +        clear_page(iorp->va);
> +    }
>  
> -    iorp->page = page;
> -    clear_page(iorp->va);
>      return 0;
>  
> - fail:
> -    put_page_alloc_ref(page);
> -    put_page_and_type(page);
> +fail:
> +    /* Free all previously allocated pages */
> +    for ( j = 0; j <= i; j++ )
> +    {
> +        struct ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreqs.page[j];
> +        if ( iorp->page )
> +        {
> +            if ( iorp->va )
> +                  unmap_domain_page_global(iorp->va);

Nit: Indentation.

> +            iorp->va = NULL;

Perhaps best to introduce and use UNMAP_DOMAIN_PAGE_GLOBAL(), paralleling
UNMAP_DOMAIN_PAGE().

> +            put_page_alloc_ref(iorp->page);
> +            put_page_and_type(iorp->page);
> +            iorp->page = NULL;

Maybe also PUT_PAGE_AND_TYPE().

> @@ -29,6 +36,10 @@ struct ioreq_page {
>      void *va;
>  };
>  
> +struct ioreq_pages {
> +    struct ioreq_page page[NR_IOREQ_PAGES];
> +};
> +
>  struct ioreq_vcpu {
>      struct list_head list_entry;
>      struct vcpu      *vcpu;
> @@ -45,7 +56,7 @@ struct ioreq_server {
>      /* Lock to serialize toolstack modifications */
>      spinlock_t             lock;
>  
> -    struct ioreq_page      ioreq;
> +    struct ioreq_pages     ioreqs;
>      struct list_head       ioreq_vcpu_list;
>      struct ioreq_page      bufioreq;

You allocate contiguous GFNs and you will, as per what Andrew requested, also
allocate contiguous VA space. No need to inflate the structure like this then?

Having reached the bottom - how's qemu going to know that multiple pages are
in use?

Jan