Xen project Mailing List

Hi, Jan.

There are the questions I would like to clarify.

On Thu, Mar 23, 2017 at 2:47 PM, Oleksandr Tyshchenko <olekstysh@xxxxxxxxx> wrote:
> On Thu, Mar 23, 2017 at 11:07 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>> On 22.03.17 at 19:01, <olekstysh@xxxxxxxxx> wrote:
>>> Hi Jan
>>>
>>> On Wed, Mar 22, 2017 at 5:44 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>>> On 15.03.17 at 21:05, <olekstysh@xxxxxxxxx> wrote:
>>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>>>>>
>>>>> Extend the IOMMU code with new APIs and platform callbacks.
>>>>> These new map_pages/unmap_pages API do almost the same thing
>>>>> as existing map_page/unmap_page ones except the formers can
>>>>> handle the number of pages. So do new platform callbacks.
>>>>>
>>>>> Currently, this patch requires to modify neither
>>>>> existing IOMMU drivers nor P2M code.
>>>>> But, the patch might be rewritten to replace existing
>>>>> single-page stuff with the multi-page one followed by modifications
>>>>> of all related parts.
>>>>
>>>> Yes please. However, unless you strictly need a page count, please
>>>> instead use an "order" parameter. Doing that has been on my todo
>>>> list for quite a while.
>>>
>>> The patch introduces new iommu_map_pages/iommu_unmap_pages APIs as well
>>> as new map_pages/unmap_pages platform callbacks.
>>>
>>> So, we just need to replace single-page APIs with multi-page ones, but
>>> leave both "new" (map_pages/unmap_pages) and "old"
>>> (map_page/unmap_page) platform callbacks.
>>> This way doesn't require to modify existing IOMMU drivers right now,
>>> just P2M code. Or we need to replace platform callbacks too?
>>
>> That was the "yes please" part of my answer: I don't see why we
>> would want two API / callback flavors, with one being a strict
>> superset of the other.
>
> Agree. I'll be back if I have questions that need to be clarified.

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?

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

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?

P.S. As for using "order" parameter instead of page_count.

There are *few* places where "order" doesn't fit. I can introduce something like this:

#define __iommu_map_pages(d,gfn,mfn,order,flags) (iommu_map_pages(d,gfn,mfn,1U << (order),flags))

>
>>
>> Jan
>>
>
>
> --
> Regards,
>
> Oleksandr Tyshchenko

--
Regards,

Oleksandr Tyshchenko

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