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

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

Hi, Jan.

Thank you for your reply.

On Fri, Apr 28, 2017 at 9:23 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>
> >>> 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.
OK

>
>
> > 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.
So, if I understand what your meant I don't even need to try to
optimize this code.
Despite the fact that this code would become much more simple:
...
rc = iommu_map_pages(d, pfn, pfn, 1,
                  IOMMUF_readable|IOMMUF_writable);
...
Right?

>
> 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.
Sorry for that. Next time I will provide more detailed snippet.

>
> > 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.
OK

>
> > 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.
I got it. I proposed because I had seen analogy with
__map_domain_page/map_domain_page.

>
> Jan

-- 
Regards,

Oleksandr Tyshchenko

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