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

[Oleksandr Tyshchenko]

On Fri, Apr 28, 2017 at 1:29 PM, Jan Beulich  wrote:
 On 28.04.17 at 12:16,  wrote:
>> On Fri, Apr 28, 2017 at 9:23 AM, Jan Beulich  wrote:
>>> >>> On 27.04.17 at 18:56,  wrote:
>>> > 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?
>
> Well, the actual simplification is entirely independent of you patch;
> what you'd add is just the extra order argument (which ought to
> be zero, btw). I am, however, of the opinion that the simplification
> would be good to do, but independent of your patch, and only
> unless the VT-d maintainers actually think there is a reason for this
> "cleverness".
Clear enough.

>
>>> > 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.
>
> Well, there are quite a few things in long standing code which
> we wouldn't allow in new instances of anymore, one of which
> being the non-standard-conforming use of leading underscores.
> Eventually old code would be nice to be cleaned up too, but
> that's tedious and time consuming, and most if not all of us
> have better uses for their time. So commonly we do such
> cleanup only when respective code needs touching anyway.
Clear enough.

>
> Jan
>

-- 
Regards,

Oleksandr Tyshchenko

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

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

[Jan Beulich]

>>> On 28.04.17 at 12:16,  wrote:
> On Fri, Apr 28, 2017 at 9:23 AM, Jan Beulich  wrote:
>> >>> On 27.04.17 at 18:56,  wrote:
>> > 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?

Well, the actual simplification is entirely independent of you patch;
what you'd add is just the extra order argument (which ought to
be zero, btw). I am, however, of the opinion that the simplification
would be good to do, but independent of your patch, and only
unless the VT-d maintainers actually think there is a reason for this
"cleverness".

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

Well, there are quite a few things in long standing code which
we wouldn't allow in new instances of anymore, one of which
being the non-standard-conforming use of leading underscores.
Eventually old code would be nice to be cleaned up too, but
that's tedious and time consuming, and most if not all of us
have better uses for their time. So commonly we do such
cleanup only when respective code needs touching anyway.

Jan


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

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

[Oleksandr Tyshchenko]

Hi, Jan.

Thank you for your reply.

On Fri, Apr 28, 2017 at 9:23 AM, Jan Beulich  wrote:
>
> >>> On 27.04.17 at 18:56,  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,

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

[Jan Beulich]

>>> On 27.04.17 at 18:56,  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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Oleksandr Tyshchenko]

Hi, Jan.

There are the questions I would like to clarify.

On Thu, Mar 23, 2017 at 2:47 PM, Oleksandr Tyshchenko 
wrote:
> On Thu, Mar 23, 2017 at 11:07 AM, Jan Beulich  wrote:
> On 22.03.17 at 19:01,  wrote:
>>> Hi Jan
>>>
>>> On Wed, Mar 22, 2017 at 5:44 PM, Jan Beulich  wrote:
>>> On 15.03.17 at 21:05,  wrote:
> From: Oleksandr Tyshchenko 
>
> 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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Oleksandr Tyshchenko]

On Wed, Apr 19, 2017 at 8:31 PM, Julien Grall  wrote:
> Hi Oleksandr,
Hi, Julien

>
>
> On 15/03/17 20:05, Oleksandr Tyshchenko wrote:
>>
>> From: Oleksandr Tyshchenko 
>>
>> 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.
>>
>> Signed-off-by: Oleksandr Tyshchenko 
>> ---
>>  xen/drivers/passthrough/iommu.c | 50
>> -
>>  xen/include/xen/iommu.h | 16 ++---
>>  2 files changed, 52 insertions(+), 14 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/iommu.c
>> b/xen/drivers/passthrough/iommu.c
>> index 5e81813..115698f 100644
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -249,22 +249,37 @@ void iommu_domain_destroy(struct domain *d)
>>  arch_iommu_domain_destroy(d);
>>  }
>>
>> -int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long
>> mfn,
>> -   unsigned int flags)
>> +int iommu_map_pages(struct domain *d, unsigned long gfn, unsigned long
>> mfn,
>> +unsigned long page_count, unsigned int flags)
>
>
> It would be nice if you can use mfn_t and gfn_t instead of unsigned long for
> any new functions. They are typesafe and avoid confusion between gfn and
> mfn.
ok

>
>
>>  {
>>  const struct domain_iommu *hd = dom_iommu(d);
>> -int rc;
>> +int rc = 0;
>> +unsigned long i;
>>
>>  if ( !iommu_enabled || !hd->platform_ops )
>>  return 0;
>>
>> -rc = hd->platform_ops->map_page(d, gfn, mfn, flags);
>> +if ( hd->platform_ops->map_pages )
>> +rc = hd->platform_ops->map_pages(d, gfn, mfn, page_count, flags);
>> +else
>> +{
>> +for ( i = 0; i < page_count; i++ )
>> +{
>> +rc = hd->platform_ops->map_page(d, gfn + i, mfn + i, flags);
>> +if ( unlikely(rc) )
>> +{
>> +/* TODO Do we need to unmap if map failed? */
>> +break;
>> +}
>> +}
>> +}
>> +
>>  if ( unlikely(rc) )
>>  {
>>  if ( !d->is_shutting_down && printk_ratelimit() )
>>  printk(XENLOG_ERR
>> -   "d%d: IOMMU mapping gfn %#lx to mfn %#lx failed:
>> %d\n",
>> -   d->domain_id, gfn, mfn, rc);
>> +   "d%d: IOMMU mapping gfn %#lx to mfn %#lx page count
>> %lu failed: %d\n",
>> +   d->domain_id, gfn, mfn, page_count, rc);
>>
>>  if ( !is_hardware_domain(d) )
>>  domain_crash(d);
>> @@ -273,21 +288,34 @@ int iommu_map_page(struct domain *d, unsigned long
>> gfn, unsigned long mfn,
>>  return rc;
>>  }
>>
>> -int iommu_unmap_page(struct domain *d, unsigned long gfn)
>> +int iommu_unmap_pages(struct domain *d, unsigned long gfn,
>> +  unsigned long page_count)
>>  {
>>  const struct domain_iommu *hd = dom_iommu(d);
>> -int rc;
>> +int ret, rc = 0;
>> +unsigned long i;
>>
>>  if ( !iommu_enabled || !hd->platform_ops )
>>  return 0;
>>
>> -rc = hd->platform_ops->unmap_page(d, gfn);
>> +if ( hd->platform_ops->unmap_pages )
>> +rc = hd->platform_ops->unmap_pages(d, gfn, page_count);
>> +else
>> +{
>> +for ( i = 0; i < page_count; i++ )
>> +{
>> +ret = hd->platform_ops->unmap_page(d, gfn + i);
>> +if ( likely(!rc) )
>> +rc = ret;
>> +}
>> +}
>> +
>>  if ( unlikely(rc) )
>>  {
>>  if ( !d->is_shutting_down && printk_ratelimit() )
>>  printk(XENLOG_ERR
>> -   "d%d: IOMMU unmapping gfn %#lx failed: %d\n",
>> -   d->domain_id, gfn, rc);
>> +   "d%d: IOMMU unmapping gfn %#lx page count %lu failed:
>> %d\n",
>> +   d->domain_id, gfn, page_count, rc);
>>
>>  if ( !is_hardware_domain(d) )
>>  domain_crash(d);
>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>> index 5803e3f..0446ed3 100644
>> --- a/xen/include/xen/iommu.h
>> +++ b/xen/include/xen/iommu.h
>> @@ -76,9 +76,14 @@ void iommu_teardown(struct domain *d);
>>  #define IOMMUF_readable  (1u<<_IOMMUF_readable)
>>  #define _IOMMUF_writable 1
>>  #define IOMMUF_writable  (1u<<_IOMMUF_writable)
>> -int __must_check iommu_map_page(struct domain *d, unsigned long gfn,
>> -unsigned long mfn, unsigned int flags);
>> -int __must_check iommu_unmap_page(struct domain *d, unsigned long gfn);
>> 

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

[Julien Grall]

Hi Oleksandr,

On 15/03/17 20:05, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 

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.

Signed-off-by: Oleksandr Tyshchenko 
---
 xen/drivers/passthrough/iommu.c | 50 -
 xen/include/xen/iommu.h | 16 ++---
 2 files changed, 52 insertions(+), 14 deletions(-)

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 5e81813..115698f 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -249,22 +249,37 @@ void iommu_domain_destroy(struct domain *d)
 arch_iommu_domain_destroy(d);
 }

-int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
-   unsigned int flags)
+int iommu_map_pages(struct domain *d, unsigned long gfn, unsigned long mfn,
+unsigned long page_count, unsigned int flags)


It would be nice if you can use mfn_t and gfn_t instead of unsigned long 
for any new functions. They are typesafe and avoid confusion between gfn 
and mfn.



 {
 const struct domain_iommu *hd = dom_iommu(d);
-int rc;
+int rc = 0;
+unsigned long i;

 if ( !iommu_enabled || !hd->platform_ops )
 return 0;

-rc = hd->platform_ops->map_page(d, gfn, mfn, flags);
+if ( hd->platform_ops->map_pages )
+rc = hd->platform_ops->map_pages(d, gfn, mfn, page_count, flags);
+else
+{
+for ( i = 0; i < page_count; i++ )
+{
+rc = hd->platform_ops->map_page(d, gfn + i, mfn + i, flags);
+if ( unlikely(rc) )
+{
+/* TODO Do we need to unmap if map failed? */
+break;
+}
+}
+}
+
 if ( unlikely(rc) )
 {
 if ( !d->is_shutting_down && printk_ratelimit() )
 printk(XENLOG_ERR
-   "d%d: IOMMU mapping gfn %#lx to mfn %#lx failed: %d\n",
-   d->domain_id, gfn, mfn, rc);
+   "d%d: IOMMU mapping gfn %#lx to mfn %#lx page count %lu failed: 
%d\n",
+   d->domain_id, gfn, mfn, page_count, rc);

 if ( !is_hardware_domain(d) )
 domain_crash(d);
@@ -273,21 +288,34 @@ int iommu_map_page(struct domain *d, unsigned long gfn, 
unsigned long mfn,
 return rc;
 }

-int iommu_unmap_page(struct domain *d, unsigned long gfn)
+int iommu_unmap_pages(struct domain *d, unsigned long gfn,
+  unsigned long page_count)
 {
 const struct domain_iommu *hd = dom_iommu(d);
-int rc;
+int ret, rc = 0;
+unsigned long i;

 if ( !iommu_enabled || !hd->platform_ops )
 return 0;

-rc = hd->platform_ops->unmap_page(d, gfn);
+if ( hd->platform_ops->unmap_pages )
+rc = hd->platform_ops->unmap_pages(d, gfn, page_count);
+else
+{
+for ( i = 0; i < page_count; i++ )
+{
+ret = hd->platform_ops->unmap_page(d, gfn + i);
+if ( likely(!rc) )
+rc = ret;
+}
+}
+
 if ( unlikely(rc) )
 {
 if ( !d->is_shutting_down && printk_ratelimit() )
 printk(XENLOG_ERR
-   "d%d: IOMMU unmapping gfn %#lx failed: %d\n",
-   d->domain_id, gfn, rc);
+   "d%d: IOMMU unmapping gfn %#lx page count %lu failed: %d\n",
+   d->domain_id, gfn, page_count, rc);

 if ( !is_hardware_domain(d) )
 domain_crash(d);
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 5803e3f..0446ed3 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -76,9 +76,14 @@ void iommu_teardown(struct domain *d);
 #define IOMMUF_readable  (1u<<_IOMMUF_readable)
 #define _IOMMUF_writable 1
 #define IOMMUF_writable  (1u<<_IOMMUF_writable)
-int __must_check iommu_map_page(struct domain *d, unsigned long gfn,
-unsigned long mfn, unsigned int flags);
-int __must_check iommu_unmap_page(struct domain *d, unsigned long gfn);
+int __must_check iommu_map_pages(struct domain *d, unsigned long gfn,
+ unsigned long mfn, unsigned long page_count,
+ unsigned int flags);
+int __must_check iommu_unmap_pages(struct domain *d, unsigned long gfn,
+   unsigned long page_count);
+
+#define iommu_map_page(d,gfn,mfn,flags) (iommu_map_pages(d,gfn,mfn,1,flags))
+#define iommu_unmap_page(d,gfn) 

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

[Oleksandr Tyshchenko]

On Thu, Mar 23, 2017 at 11:07 AM, Jan Beulich  wrote:
 On 22.03.17 at 19:01,  wrote:
>> Hi Jan
>>
>> On Wed, Mar 22, 2017 at 5:44 PM, Jan Beulich  wrote:
>> On 15.03.17 at 21:05,  wrote:
 From: Oleksandr Tyshchenko 

 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.

>
> Jan
>


-- 
Regards,

Oleksandr Tyshchenko

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

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

[Jan Beulich]

>>> On 22.03.17 at 19:01,  wrote:
> Hi Jan
> 
> On Wed, Mar 22, 2017 at 5:44 PM, Jan Beulich  wrote:
> On 15.03.17 at 21:05,  wrote:
>>> From: Oleksandr Tyshchenko 
>>>
>>> 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.

Jan


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

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

[Oleksandr Tyshchenko]

Hi Jan

On Wed, Mar 22, 2017 at 5:44 PM, Jan Beulich  wrote:
 On 15.03.17 at 21:05,  wrote:
>> From: Oleksandr Tyshchenko 
>>
>> 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.
>
> Jan
>

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?

Don't mind to use order instead of page count.

-- 
Regards,

Oleksandr Tyshchenko

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

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

[Jan Beulich]

>>> On 15.03.17 at 21:05,  wrote:
> From: Oleksandr Tyshchenko 
> 
> 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.

Jan


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

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

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko 

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.

Signed-off-by: Oleksandr Tyshchenko 
---
 xen/drivers/passthrough/iommu.c | 50 -
 xen/include/xen/iommu.h | 16 ++---
 2 files changed, 52 insertions(+), 14 deletions(-)

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 5e81813..115698f 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -249,22 +249,37 @@ void iommu_domain_destroy(struct domain *d)
 arch_iommu_domain_destroy(d);
 }
 
-int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
-   unsigned int flags)
+int iommu_map_pages(struct domain *d, unsigned long gfn, unsigned long mfn,
+unsigned long page_count, unsigned int flags)
 {
 const struct domain_iommu *hd = dom_iommu(d);
-int rc;
+int rc = 0;
+unsigned long i;
 
 if ( !iommu_enabled || !hd->platform_ops )
 return 0;
 
-rc = hd->platform_ops->map_page(d, gfn, mfn, flags);
+if ( hd->platform_ops->map_pages )
+rc = hd->platform_ops->map_pages(d, gfn, mfn, page_count, flags);
+else
+{
+for ( i = 0; i < page_count; i++ )
+{
+rc = hd->platform_ops->map_page(d, gfn + i, mfn + i, flags);
+if ( unlikely(rc) )
+{
+/* TODO Do we need to unmap if map failed? */
+break;
+}
+}
+}
+
 if ( unlikely(rc) )
 {
 if ( !d->is_shutting_down && printk_ratelimit() )
 printk(XENLOG_ERR
-   "d%d: IOMMU mapping gfn %#lx to mfn %#lx failed: %d\n",
-   d->domain_id, gfn, mfn, rc);
+   "d%d: IOMMU mapping gfn %#lx to mfn %#lx page count %lu 
failed: %d\n",
+   d->domain_id, gfn, mfn, page_count, rc);
 
 if ( !is_hardware_domain(d) )
 domain_crash(d);
@@ -273,21 +288,34 @@ int iommu_map_page(struct domain *d, unsigned long gfn, 
unsigned long mfn,
 return rc;
 }
 
-int iommu_unmap_page(struct domain *d, unsigned long gfn)
+int iommu_unmap_pages(struct domain *d, unsigned long gfn,
+  unsigned long page_count)
 {
 const struct domain_iommu *hd = dom_iommu(d);
-int rc;
+int ret, rc = 0;
+unsigned long i;
 
 if ( !iommu_enabled || !hd->platform_ops )
 return 0;
 
-rc = hd->platform_ops->unmap_page(d, gfn);
+if ( hd->platform_ops->unmap_pages )
+rc = hd->platform_ops->unmap_pages(d, gfn, page_count);
+else
+{
+for ( i = 0; i < page_count; i++ )
+{
+ret = hd->platform_ops->unmap_page(d, gfn + i);
+if ( likely(!rc) )
+rc = ret;
+}
+}
+
 if ( unlikely(rc) )
 {
 if ( !d->is_shutting_down && printk_ratelimit() )
 printk(XENLOG_ERR
-   "d%d: IOMMU unmapping gfn %#lx failed: %d\n",
-   d->domain_id, gfn, rc);
+   "d%d: IOMMU unmapping gfn %#lx page count %lu failed: %d\n",
+   d->domain_id, gfn, page_count, rc);
 
 if ( !is_hardware_domain(d) )
 domain_crash(d);
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 5803e3f..0446ed3 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -76,9 +76,14 @@ void iommu_teardown(struct domain *d);
 #define IOMMUF_readable  (1u<<_IOMMUF_readable)
 #define _IOMMUF_writable 1
 #define IOMMUF_writable  (1u<<_IOMMUF_writable)
-int __must_check iommu_map_page(struct domain *d, unsigned long gfn,
-unsigned long mfn, unsigned int flags);
-int __must_check iommu_unmap_page(struct domain *d, unsigned long gfn);
+int __must_check iommu_map_pages(struct domain *d, unsigned long gfn,
+ unsigned long mfn, unsigned long page_count,
+ unsigned int flags);
+int __must_check iommu_unmap_pages(struct domain *d, unsigned long gfn,
+   unsigned long page_count);
+
+#define iommu_map_page(d,gfn,mfn,flags) (iommu_map_pages(d,gfn,mfn,1,flags))
+#define iommu_unmap_page(d,gfn) (iommu_unmap_pages(d,gfn,1))
 
 enum iommu_feature
 {
@@ -170,7 +175,12 @@ struct iommu_ops {
 void (*teardown)(struct domain *d);
 int __must_check (*map_page)(struct domain *d, unsigned long gfn,