Re: [Xen-devel] [RFC PATCH 2/9] iommu: Add ability to map/unmap the number of pages

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.04.17 at 18:56, <olekstysh@xxxxxxxxx> wrote:
> Now I am trying to replace single-page stuff with the multi-page one.
> Currently, with the single-page API, if map fails we always try to unmap
> already mapped pages.
> 
> This is an example of generic code I am speaking about:
> ...
> for ( i = 0; i < (1 << order); i++ )
> {
>     rc = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags);
>     if ( unlikely(rc) )
>     {
>         while ( i-- )
>             /* If statement to satisfy __must_check. */
>             if ( iommu_unmap_page(p2m->domain, gfn + i) )
>                 continue;
> 
>         break;
>     }
> }
> ...
> 
> After modification the generic code will look like:
> ...
> rc = iommu_map_pages(d, gfn, mfn_x(mfn), 1 << order, iommu_flags);
> ...
> In case of an error we won't know how many pages have been already mapped
> and
> as the result we won't be able to unmap them in order to restore the
> initial state.
> Therefore I will move the rollback logic to the IOMMU drivers code. So, the
> IOMMU drivers code
> will take care of rollback mapping if something goes wrong. Am I right?

Yes, it should be iommu_map_pages() (or its descendants) to clean
up after itself upon error.

> If all described above make sense, then there are several places I am
> trying to optimize, but I am not familiar with)
> 
> 1. xen/arch/x86/x86_64/mm.c:1443:
> ...
> if ( iommu_enabled && !iommu_passthrough && !need_iommu(hardware_domain) )
> {
>     for ( i = spfn; i < epfn; i++ )
>         if ( iommu_map_page(hardware_domain, i, i,
> IOMMUF_readable|IOMMUF_writable)
> )
>             break;
>     if ( i != epfn )
>     {
>         while (i-- > old_max) // <--- [1]
>             /* If statement to satisfy __must_check. */
>             if ( iommu_unmap_page(hardware_domain, i) )
>                 continue;
> 
>         goto destroy_m2p;
>     }
> }
> ...
> 
> [1] Shouldn't we unmap already mapped pages only?  I mean to use "while
> (i-- > spfn)" instead.

Both should have the same effect, considering what old_max
represents, at least as long as there's no MMIO in between. But
yes, generally it would be more logical to unmap using spfn.

> And if the use of "old_max" here has a special meaning, I think, that this
> place of code won't be optimized
> by passing the whole memory block (epfn - spfn) to the IOMMU. Just keep it
> as is (handle one page at time).

Right, that would have been my general advice: If in doubt, keep
the code as is rather than trying to optimize it.

> 2. xen/drivers/passthrough/vtd/x86/vtd.c:143:
> ...
> tmp = 1 << (PAGE_SHIFT - PAGE_SHIFT_4K);
> for ( j = 0; j < tmp; j++ )
> {
>     int ret = iommu_map_page(d, pfn * tmp + j, pfn * tmp + j,
>                              IOMMUF_readable|IOMMUF_writable);
> 
>     if ( !rc )
>        rc = ret;
> }
> ...
> 
> Here we don't try to rollback mapping at all. Was the idea to do so? Or was
> this place simply missed?

Did you consider both the context this is in (establishing hwdom
mappings) and the code's history? Both would tell you that yes,
this is a best effort mapping attempt deliberately continuing
despite errors (but reporting the first one). This behavior will
need to be retained. Plus I'm sure you've noticed that this
effectively is a single page mapping only anyway (due to
PAGE_SHIFT == PAGE_SHIFT_4K); I have no idea why this
"clever" code was used.

And as a side note - the way you quote code (by line number and
without naming the function it's in) makes it somewhat more
complicated to answer your questions. In both cases, had I known
the function the code is in, I wouldn't have had a need at all to go
look up the context.

> P.S. As for using "order" parameter instead of page_count.
> There are *few* places where "order" doesn't fit.

Well, I'd prefer the "few places" to then break up their requests to
fit the "order" parameter. Especially for the purpose of possibly
using large pages, an order input is quite a bit more sensible.

> I can introduce something like this:
> 
> #define __iommu_map_pages(d,gfn,mfn,order,flags)
> (iommu_map_pages(d,gfn,mfn,1U << (order),flags))

I'd prefer if you didn't. In no case should this be an identifier
starting with an underscore.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel