question on iommu _map_page

question on iommu _map_page

[Kaushik Kumar Ram]

Hi all,

I am trying to use the VT-d support with a PV driver domain which performs network I/O on behalf of guest domains. 
When I initiate any network I/O it results in I/O page faults. On taking a closer look I find that iommu_map_page() is called only for writable pages from __gnttab_map_grant_ref(). This results in page faults during packet transmission since pages are mapped read-only in this case. 
When I remove this restriction, I can get it working without any page faults. 

Is this a bug? or am I missing something here? I am using the latest unstable version of Xen. This part of the source code hasn't changed in a long time.

Thanks!
-Kaushik

Re: question on iommu _map_page

[Keir Fraser]

It looks like the person who implemented that never needed the read-only
case. We need a function to map pages into the iommu read-only and to call
that from the grant code.

A patch to just call iommu_map_page() for any kind of grant mapping would be
acceptable for now, if you want to submit a patch.

 -- Keir

On 27/05/2010 21:37, "Kaushik Kumar Ram" <kaushik@rice.edu> wrote:

> Hi all,
> 
> I am trying to use the VT-d support with a PV driver domain which performs
> network I/O on behalf of guest domains.
> When I initiate any network I/O it results in I/O page faults. On taking a
> closer look I find that iommu_map_page() is called only for writable pages
> from __gnttab_map_grant_ref(). This results in page faults during packet
> transmission since pages are mapped read-only in this case.
> When I remove this restriction, I can get it working without any page faults.
> 
> Is this a bug? or am I missing something here? I am using the latest unstable
> version of Xen. This part of the source code hasn't changed in a long time.
> 
> Thanks!
> -Kaushik
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: question on iommu _map_page

[Kaushik Kumar Ram]

On May 27, 2010, at 3:46 PM, Keir Fraser wrote:

> It looks like the person who implemented that never needed the read-only
> case. We need a function to map pages into the iommu read-only and to call
> that from the grant code.
> 
> A patch to just call iommu_map_page() for any kind of grant mapping would be
> acceptable for now, if you want to submit a patch.

OK. Attached.

Signed-off-by: Kaushik Kumar Ram <kaushik@rice.edu>

diff -r 26c2922da53c xen/common/grant_table.c
--- a/xen/common/grant_table.c  Thu May 27 09:39:47 2010 +0100
+++ b/xen/common/grant_table.c  Thu May 27 15:56:37 2010 -0500
@@ -596,9 +596,7 @@ __gnttab_map_grant_ref(
         goto undo_out;
     }
 
-    if ( (!is_hvm_domain(ld) && need_iommu(ld)) &&
-         !(old_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) &&
-         (act_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
+    if ( !is_hvm_domain(ld) && need_iommu(ld))
     {
         /* Shouldn't happen, because you can't use iommu in a HVM
          * domain. */
@@ -779,9 +777,7 @@ __gnttab_unmap_common(
             act->pin -= GNTPIN_hstw_inc;
     }
 
-    if ( (!is_hvm_domain(ld) && need_iommu(ld)) &&
-         (old_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) &&
-         !(act->pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
+    if ( !is_hvm_domain(ld) && need_iommu(ld))
     {
         BUG_ON(paging_mode_translate(ld));
         if ( iommu_unmap_page(ld, op->frame) )

-Kaushik 

> On 27/05/2010 21:37, "Kaushik Kumar Ram" <kaushik@rice.edu> wrote:
> 
>> Hi all,
>> 
>> I am trying to use the VT-d support with a PV driver domain which performs
>> network I/O on behalf of guest domains.
>> When I initiate any network I/O it results in I/O page faults. On taking a
>> closer look I find that iommu_map_page() is called only for writable pages
>> from __gnttab_map_grant_ref(). This results in page faults during packet
>> transmission since pages are mapped read-only in this case.
>> When I remove this restriction, I can get it working without any page faults.
>> 
>> Is this a bug? or am I missing something here? I am using the latest unstable
>> version of Xen. This part of the source code hasn't changed in a long time.
>> 
>> Thanks!
>> -Kaushik
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 

Re: question on iommu _map_page

[Keir Fraser]

On 27/05/2010 21:59, "Kaushik Kumar Ram" <kaushik@rice.edu> wrote:

> On May 27, 2010, at 3:46 PM, Keir Fraser wrote:
> 
>> It looks like the person who implemented that never needed the read-only
>> case. We need a function to map pages into the iommu read-only and to call
>> that from the grant code.
>> 
>> A patch to just call iommu_map_page() for any kind of grant mapping would be
>> acceptable for now, if you want to submit a patch.
> 
> OK. Attached.

Actually I've implemented the better fix as xen-unstable:21476. Hope that
works okay for you.

 Thanks,
 Keir

Re: question on iommu _map_page

[Kaushik Kumar Ram]

On May 28, 2010, at 3:08 AM, Keir Fraser wrote:

> On 27/05/2010 21:59, "Kaushik Kumar Ram" <kaushik@rice.edu> wrote:
> 
>> On May 27, 2010, at 3:46 PM, Keir Fraser wrote:
>> 
>>> It looks like the person who implemented that never needed the read-only
>>> case. We need a function to map pages into the iommu read-only and to call
>>> that from the grant code.
>>> 
>>> A patch to just call iommu_map_page() for any kind of grant mapping would be
>>> acceptable for now, if you want to submit a patch.
>> 
>> OK. Attached.
> 
> Actually I've implemented the better fix as xen-unstable:21476. Hope that
> works okay for you.

I don't see this changeset....

Thanks.
-Kaushik

Re: question on iommu _map_page

[Keir Fraser]

On 29/05/2010 00:20, "Kaushik Kumar Ram" <kaushik@rice.edu> wrote:

>> Actually I've implemented the better fix as xen-unstable:21476. Hope that
>> works okay for you.
> 
> I don't see this changeset....

Should be there now. In general you can also try pulling from:
http://xenbits.xensource.com/staging/xen-unstable.hg
Which is the intermediate (pre-testing) staging tree.

 -- Keir

Re: question on iommu _map_page

[Kaushik Kumar Ram]

On May 29, 2010, at 2:29 AM, Keir Fraser wrote:

> On 29/05/2010 00:20, "Kaushik Kumar Ram" <kaushik@rice.edu> wrote:
> 
>>> Actually I've implemented the better fix as xen-unstable:21476. Hope that
>>> works okay for you.
>> 
>> I don't see this changeset....
> 
> Should be there now.

This should work. I can test it on my machine in a couple of days.

> In general you can also try pulling from:
> http://xenbits.xensource.com/staging/xen-unstable.hg
> Which is the intermediate (pre-testing) staging tree.

OK.

Thanks.
-Kaushik

Re: question on iommu _map_page

[Kaushik Kumar Ram]

On May 28, 2010, at 3:08 AM, Keir Fraser wrote:

> On 27/05/2010 21:59, "Kaushik Kumar Ram" <kaushik@rice.edu> wrote:
> 
>> On May 27, 2010, at 3:46 PM, Keir Fraser wrote:
>> 
>>> It looks like the person who implemented that never needed the read-only
>>> case. We need a function to map pages into the iommu read-only and to call
>>> that from the grant code.
>>> 
>>> A patch to just call iommu_map_page() for any kind of grant mapping would be
>>> acceptable for now, if you want to submit a patch.
>> 
>> OK. Attached.
> 
> Actually I've implemented the better fix as xen-unstable:21476. Hope that
> works okay for you.


Checking if act->pin is zero before calling iommu_unmap_page() (in grant_table.c) is not sufficient since there can be multiple
active grants all referring to the same mfn. In fact I came across iommu page faults because pages were 
getting unmapped from the IOMMU when active grants referring to these pages were still around.
Ideally, there needs to be a per-page count of how many IOMMU mappings exists for a page. 
I can't think of an obvious fix for this problem. For my purposes, I hacked page_info to add another counter which I guess is not an acceptable solution! 

-Kaushik

Re: question on iommu _map_page

[Keir Fraser]

On 10/06/2010 22:59, "Kaushik Kumar Ram" <kaushik@rice.edu> wrote:

>> Actually I've implemented the better fix as xen-unstable:21476. Hope that
>> works okay for you.
> 
> 
> Checking if act->pin is zero before calling iommu_unmap_page() (in
> grant_table.c) is not sufficient since there can be multiple
> active grants all referring to the same mfn. In fact I came across iommu page
> faults because pages were
> getting unmapped from the IOMMU when active grants referring to these pages
> were still around.
> Ideally, there needs to be a per-page count of how many IOMMU mappings exists
> for a page. 
> I can't think of an obvious fix for this problem. For my purposes, I hacked
> page_info to add another counter which I guess is not an acceptable solution!

See if xen-unstable:21597 works for you.

 -- Keir

Re: question on iommu _map_page

[Kaushik Kumar Ram]

On Jun 11, 2010, at 3:32 AM, Keir Fraser wrote:

> On 10/06/2010 22:59, "Kaushik Kumar Ram" <kaushik@rice.edu> wrote:
>> 
>> Checking if act->pin is zero before calling iommu_unmap_page() (in
>> grant_table.c) is not sufficient since there can be multiple
>> active grants all referring to the same mfn. In fact I came across iommu page
>> faults because pages were
>> getting unmapped from the IOMMU when active grants referring to these pages
>> were still around.
>> Ideally, there needs to be a per-page count of how many IOMMU mappings exists
>> for a page. 
>> I can't think of an obvious fix for this problem. For my purposes, I hacked
>> page_info to add another counter which I guess is not an acceptable solution!
> 
> See if xen-unstable:21597 works for you.

Keir,

I finally found some time to test your patch. While it seems to fix the problem, it *significantly* degrades performance.
On running netperf, there is a 10X reduction in throughput to a guest VM. On profiling, I find a significant number of cycles
being spent in mapcount(). In the current solution, the entire grant table is searched every time (right ?). The mapping info 
ought to be stored in some per-page location... for efficient access.

-Kaushik

Re: question on iommu _map_page

[Keir Fraser]

On 02/07/2010 07:22, "Kaushik Kumar Ram" <kaushik@rice.edu> wrote:

>> See if xen-unstable:21597 works for you.
> 
> Keir,
> 
> I finally found some time to test your patch. While it seems to fix the
> problem, it *significantly* degrades performance.
> On running netperf, there is a 10X reduction in throughput to a guest VM. On
> profiling, I find a significant number of cycles
> being spent in mapcount(). In the current solution, the entire grant table is
> searched every time (right ?). The mapping info
> ought to be stored in some per-page location... for efficient access.

It needs to be a per-mapping-domain location. Like the VT-d pte itself.
Patches welcome.

 -- Keir

Re: question on iommu _map_page

[Keir Fraser]

On 02/07/2010 08:13, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

>> I finally found some time to test your patch. While it seems to fix the
>> problem, it *significantly* degrades performance.
>> On running netperf, there is a 10X reduction in throughput to a guest VM. On
>> profiling, I find a significant number of cycles
>> being spent in mapcount(). In the current solution, the entire grant table is
>> searched every time (right ?). The mapping info
>> ought to be stored in some per-page location... for efficient access.
> 
> It needs to be a per-mapping-domain location. Like the VT-d pte itself.
> Patches welcome.

Scrub that. It would be better to allocate a chunk of otherwised unused
machine address space (e.g., 1GB starting at max(4GB, top_of_ram)), and
allocate each grant entry that needs an iommu mapping a page in that range,
map at that address and return that address as dev_bus_addr from the
grant-map hypercall. Then we would have no IOMMU pte sharing between grants,
and no need for any extra reference counting. Also it would be easy to batch
iommu flushes in this scenario, if you don't care about finished-with grant
mappings being flushed a bit lazily.

 -- Keir