On 30.11.2021 16:24, Roger Pau Monné wrote:
> On Fri, Sep 24, 2021 at 11:45:57AM +0200, Jan Beulich wrote:
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -260,12 +260,38 @@ void iommu_domain_destroy(struct domain
>> arch_iommu_domain_destroy(d);
>> }
>>
>> -int iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
>> +static unsigned int mapping_order(const struct domain_iommu *hd,
>> + dfn_t dfn, mfn_t mfn, unsigned long nr)
>> +{
>> + unsigned long res = dfn_x(dfn) | mfn_x(mfn);
>> + unsigned long sizes = hd->platform_ops->page_sizes;
>> + unsigned int bit = find_first_set_bit(sizes), order = 0;
>> +
>> + ASSERT(bit == PAGE_SHIFT);
>> +
>> + while ( (sizes = (sizes >> bit) & ~1) )
>> + {
>> + unsigned long mask;
>> +
>> + bit = find_first_set_bit(sizes);
>> + mask = (1UL << bit) - 1;
>> + if ( nr <= mask || (res & mask) )
>> + break;
>> + order += bit;
>> + nr >>= bit;
>> + res >>= bit;
>> + }
>> +
>> + return order;
>> +}
>
> This looks like it could be used in other places, I would at least
> consider using it in pvh_populate_memory_range where we also need to
> figure out the maximum order given an address and a number of pages.
>
> Do you think you could place it in a more generic file and also use
> more generic parameters (ie: unsigned long gfn and mfn)?
The function as is surely isn't reusable, for its use of IOMMU
specific data. If and when a 2nd user appears, it'll be far clearer
whether and if so how much of it is worth generalizing. (Among other
things I'd like to retain the typesafe parameter types here.)
>> @@ -284,14 +316,18 @@ int iommu_map(struct domain *d, dfn_t df
>> if ( !d->is_shutting_down && printk_ratelimit() )
>> printk(XENLOG_ERR
>> "d%d: IOMMU mapping dfn %"PRI_dfn" to mfn %"PRI_mfn"
>> failed: %d\n",
>> - d->domain_id, dfn_x(dfn_add(dfn, i)),
>> - mfn_x(mfn_add(mfn, i)), rc);
>> + d->domain_id, dfn_x(dfn), mfn_x(mfn), rc);
>> +
>> + for ( j = 0; j < i; j += 1UL << order )
>> + {
>> + dfn = dfn_add(dfn0, j);
>> + order = mapping_order(hd, dfn, _mfn(0), i - j);
>>
>> - while ( i-- )
>> /* if statement to satisfy __must_check */
>> - if ( iommu_call(hd->platform_ops, unmap_page, d, dfn_add(dfn,
>> i),
>> - 0, flush_flags) )
>> + if ( iommu_call(hd->platform_ops, unmap_page, d, dfn, order,
>> + flush_flags) )
>> continue;
>> + }
>
> Why you need this unmap loop here, can't you just use iommu_unmap?
Good question - I merely converted the loop that was already there.
Looks like that could have been changed to a simple call already
before. I'll change it here, on the assumption that splitting this
out isn't going to be a worthwhile exercise.
Jan
