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

On 8/18/26 3:06 PM, Jan Beulich wrote:
> 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.

Hello Jan,

Thank you again for your feedback! I will wait then for Anthony's 
decision regarding whether the multi-page ioreq support and the ioreq_t 
growth should be combined into a single effort, before I proceed further 
with a v7.

I just wanted to clarify one thing regarding the overhead I mentioned in 
the first patch's commit message ("this change has a small overhead in 
the common case"). Here, I was referring to vmap()/vunmap() replacing 
map_domain_page_global(). Where map_domain_page_global() has a directmap 
fast path. I didn't mean the overhead of the added vmap_to_page(). I 
should maybe clarify this better in my next iteration's commit message.

On your suggestion to cache the VA in struct page_info to speed up the 
vmap_to_page() lookups in is_ioreq_server_page(): I looked through the 
tree, and that function currently has only one caller 
sh_remove_all_mappings() (in xen/arch/x86/mm/shadow/common.c) and is 
only reached in a failure case. Given that, and given how widely shared 
and size-critical struct page_info is, I'm wondering whether it's really 
worth touching it for the gain of not having to do the 2 lookups. What 
do you think?

Thanks,
Julian

>
> Jan



-- 
 | Vates 

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech