Re: RFC: Using grant table to give iomem permission

Re: RFC: Using grant table to give iomem permission

[Lamia M. Youseff]

> When the hypervisor tries to do this grant in xen/common/grant_table.c
> it (i) notices that it is an iomem_map rather than device_map or
> host_map; (ii) it is from dom0; and (iii) it is not a RAM page.  It then
> attempts to call iomem_permit_access().

Hi Karen, 
Thanks for the patch, I find it useful... but I am wondering why you need to check it is from a dom0 ... I maybe not getting the reason, but iomem_map should be available for page sharing among all domains, shouldn't it ?

Thank you,
-- Lamia Youseff

Re: RFC: Using grant table to give iomem permission

[Kieran Mansley]

On Fri, 2007-05-18 at 12:11 -0700, Lamia M. Youseff wrote:
> > When the hypervisor tries to do this grant in xen/common/grant_table.c
> > it (i) notices that it is an iomem_map rather than device_map or
> > host_map; (ii) it is from dom0; and (iii) it is not a RAM page.  It then
> > attempts to call iomem_permit_access().
> 
> Hi Karen, 
> Thanks for the patch, I find it useful... but I am wondering why you
> need to check it is from a dom0 ... I maybe not getting the reason,
> but iomem_map should be available for page sharing among all domains,
> shouldn't it ?

There's no absolute requirement for that check to be there, but it is
something that came out of discussion with Keir.  It seems sensible to
me to only allow dom0 to programatically give access to iomem, as
typically this is used to access physical hardware.  I think most guest
domains wouldn't have any iomem regions available to them that they
would want to share.  If they did, access to this could be currently
configured using the Xen tools (I think - not actually tried this).

Kieran

RFC: Using grant table to give iomem permission

[Kieran Mansley]

[-- Attachment #1: Type: text/plain, Size: 2324 bytes --]

It used to be possible to use domctl to programatically set up
permissions for other domains to map iomem regions.  However, domctl is
no longer accessible from drivers (only from tools).  After some
discussion with Keir about how to achieve this functionality, it was
suggested that it might be possible to use the grant table operations to
do it.  

Attached is a patch that has a go at this and seems to work.  However,
it is quite primitive and I would welcome the comments of others who
know more about the grant tables than I do!

The patch adds a new type of grant (GNTMAP_iomem_map) to complement
GNTMAP_device_map and GNTMAP_host_map.

The granting domain would do a normal grant operation to specify the
region that can be used as iomem:
        err = gnttab_grant_foreign_access(dev->otherend_id, mfn, 0)
and then pass the grant ref to the mapping domain.

The mapping domain would then do a map op, along the lines of:
  	struct gnttab_map_grant_ref op;
	gnttab_set_map_op(&op, 0, GNTMAP_iomem_map, gnt_ref, dev->otherend_id);
	if (HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, &op, 1))
		BUG();

When the hypervisor tries to do this grant in xen/common/grant_table.c
it (i) notices that it is an iomem_map rather than device_map or
host_map; (ii) it is from dom0; and (iii) it is not a RAM page.  It then
attempts to call iomem_permit_access().

If successful, on return, the mapping domain should then be able to call
ioremap() to access the page in question.

When finished, the mapping domain can similarly unmap the grant, which
removes its ability to ioremap() the page:
  	struct gnttab_unmap_grant_ref op;
	gnttab_set_unmap_op(&op, 0, GNTMAP_iomem_map, handle);
	if (HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1))
		BUG();

Some questions from me:
 - does this approach seem sane?

 - any problem with adding the GNTTAB_iomem_map type?

 - how about the current test for it being a RAM page: !mfn_valid
(frame).  I've seen this return "valid" on iomem pages on a machine with
4GB of RAM, so it may not be a good/sufficient test.

 - the current patch doesn't actually do the ioremap() as part of the
grant operation, it just sets up the permissions.  I can see that others
might prefer it to do both.  Any thoughts?

 - is there some gross omission from what I've done?

Thanks

Kieran

[-- Attachment #2: iomem_grants --]
[-- Type: text/x-patch, Size: 4612 bytes --]

diff -r 08e2c03a47e0 xen/common/grant_table.c
--- a/xen/common/grant_table.c	Tue May 15 17:47:46 2007 +0100
+++ b/xen/common/grant_table.c	Thu May 17 09:53:29 2007 +0100
@@ -195,7 +195,9 @@ __gnttab_map_grant_ref(
     led = current;
     ld = led->domain;
 
-    if ( unlikely((op->flags & (GNTMAP_device_map|GNTMAP_host_map)) == 0) )
+    if ( unlikely((op->flags & (GNTMAP_device_map |
+                                GNTMAP_host_map | 
+                                GNTMAP_iomem_map)) == 0) )
     {
         gdprintk(XENLOG_INFO, "Bad flags in grant map op (%x).\n", op->flags);
         op->status = GNTST_bad_gntref;
@@ -305,16 +307,36 @@ __gnttab_map_grant_ref(
 
     spin_unlock(&rd->grant_table->lock);
 
-    if ( unlikely(!mfn_valid(frame)) ||
-         unlikely(!((op->flags & GNTMAP_readonly) ?
-                    get_page(mfn_to_page(frame), rd) :
-                    get_page_and_type(mfn_to_page(frame), rd,
-                                      PGT_writable_page))) )
-    {
-        if ( !rd->is_dying )
-            gdprintk(XENLOG_WARNING, "Could not pin grant frame %lx\n", frame);
-        rc = GNTST_general_error;
-        goto undo_out;
+    if ( op->flags & GNTMAP_iomem_map )
+    {
+        if( op->dom == 0 && !mfn_valid(frame) ) {
+            /* Could be an iomem page for setting up permission */
+            if ( iomem_permit_access(ld, frame, frame) != 0 ) {
+                gdprintk(XENLOG_WARNING, 
+                         "Could not permit access to grant frame %lx as iomem\n",
+                         frame);
+                rc = GNTST_general_error;
+                goto undo_out;
+            }
+        } else {
+            gdprintk(XENLOG_WARNING, 
+                     "Invalid domain (%d) or frame (%lx) for iomem map\n",
+                     op->dom, frame);
+            rc = GNTST_general_error;
+            goto undo_out;
+        }
+    } else {
+        if ( unlikely(!mfn_valid(frame)) ||
+             unlikely(!((op->flags & GNTMAP_readonly) ?
+                        get_page(mfn_to_page(frame), rd) :
+                        get_page_and_type(mfn_to_page(frame), rd,
+                                          PGT_writable_page))) )
+        {
+            if ( !rd->is_dying )
+                gdprintk(XENLOG_WARNING, "Could not pin grant frame %lx\n", frame);
+            rc = GNTST_general_error;
+            goto undo_out;
+        }
     }
 
     if ( op->flags & GNTMAP_host_map )
@@ -494,8 +516,17 @@ __gnttab_unmap_grant_ref(
             put_page_and_type(mfn_to_page(frame));
         }
     }
-
-    if ( (map->flags & (GNTMAP_device_map|GNTMAP_host_map)) == 0 )
+    
+    if ( flags & GNTMAP_iomem_map ) 
+    {
+        map->flags &= ~GNTMAP_iomem_map;
+
+        rc = iomem_deny_access(ld, frame, frame);
+    }
+
+    if ( (map->flags & (GNTMAP_device_map |
+                        GNTMAP_host_map |
+                        GNTMAP_iomem_map)) == 0 )
     {
         map->flags = 0;
         put_maptrack_handle(ld->grant_table, op->handle);
@@ -1352,13 +1383,16 @@ gnttab_release_mappings(
     struct domain        *rd;
     struct active_grant_entry *act;
     struct grant_entry   *sha;
+    int rc;
 
     BUG_ON(!d->is_dying);
 
     for ( handle = 0; handle < gt->maptrack_limit; handle++ )
     {
         map = &maptrack_entry(gt, handle);
-        if ( !(map->flags & (GNTMAP_device_map|GNTMAP_host_map)) )
+        if ( !(map->flags & (GNTMAP_device_map |
+                             GNTMAP_host_map |
+                             GNTMAP_iomem_map)) )
             continue;
 
         ref = map->ref;
@@ -1415,6 +1449,9 @@ gnttab_release_mappings(
             if ( (act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0 )
                 gnttab_clear_flag(_GTF_writing, &sha->flags);
         }
+
+        if( map->flags & GNTMAP_iomem_map )
+            rc = iomem_deny_access(rd, act->frame, act->frame);
 
         if ( act->pin == 0 )
             gnttab_clear_flag(_GTF_reading, &sha->flags);
diff -r 08e2c03a47e0 xen/include/public/grant_table.h
--- a/xen/include/public/grant_table.h	Tue May 15 17:47:46 2007 +0100
+++ b/xen/include/public/grant_table.h	Thu May 17 09:53:29 2007 +0100
@@ -358,6 +358,12 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_query_siz
 #define GNTMAP_contains_pte     (1<<_GNTMAP_contains_pte)
 
 /*
+ * Establish iomem permission
+ */
+#define _GNTMAP_iomem_map       (5)
+#define GNTMAP_iomem_map       (1<<_GNTMAP_iomem_map)
+
+/*
  * Values for error status returns. All errors are -ve.
  */
 #define GNTST_okay             (0)  /* Normal return.                        */

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: RFC: Using grant table to give iomem permission

[Keir Fraser]

On 18/5/07 15:01, "Kieran Mansley" <kmansley@solarflare.com> wrote:

> When finished, the mapping domain can similarly unmap the grant, which
> removes its ability to ioremap() the page:
> struct gnttab_unmap_grant_ref op;
> gnttab_set_unmap_op(&op, 0, GNTMAP_iomem_map, handle);
> if (HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1))
> BUG();
> 
> Some questions from me:
>  - does this approach seem sane?

There's no reason you shouldn't be able to use GNTMAP_host_map as usual, and
do refcounting in the active grant entry, also as usual.

 -- Keir

Re: RFC: Using grant table to give iomem permission

[Kieran Mansley]

On Fri, 2007-05-18 at 15:05 +0100, Keir Fraser wrote:
> On 18/5/07 15:01, "Kieran Mansley" <kmansley@solarflare.com> wrote:
> 
> > When finished, the mapping domain can similarly unmap the grant, which
> > removes its ability to ioremap() the page:
> > struct gnttab_unmap_grant_ref op;
> > gnttab_set_unmap_op(&op, 0, GNTMAP_iomem_map, handle);
> > if (HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1))
> > BUG();
> > 
> > Some questions from me:
> >  - does this approach seem sane?
> 
> There's no reason you shouldn't be able to use GNTMAP_host_map as usual, and
> do refcounting in the active grant entry, also as usual.

OK.  My reluctance to do that was simply that I wasn't sure if the
operations that take place when doing a GNTMAP_host_map would conflict
with those when doing an iomem_map.  If you think they shouldn't, I'll
give it a go.

That raises one of my other questions which is how to test for it being
a valid RAM page, as in the absence of it GNTMAP_iomem_map that's pretty
much the only indicator that it's iomem we're dealing with.  As I said,
mfn_valid() doesn't seem to be sufficient.

Kieran 

Re: RFC: Using grant table to give iomem permission

[Keir Fraser]

On 18/5/07 15:14, "Kieran Mansley" <kmansley@solarflare.com> wrote:

>>> Some questions from me:
>>>  - does this approach seem sane?
>> 
>> There's no reason you shouldn't be able to use GNTMAP_host_map as usual, and
>> do refcounting in the active grant entry, also as usual.
> 
> OK.  My reluctance to do that was simply that I wasn't sure if the
> operations that take place when doing a GNTMAP_host_map would conflict
> with those when doing an iomem_map.  If you think they shouldn't, I'll
> give it a go.

I'm not sure what you mean.

> That raises one of my other questions which is how to test for it being
> a valid RAM page, as in the absence of it GNTMAP_iomem_map that's pretty
> much the only indicator that it's iomem we're dealing with.  As I said,
> mfn_valid() doesn't seem to be sufficient.

Yes, I misled you here. You want the middle bit of get_page_from_l1e(). It
checks for !mfn_valid()||page_get_owner()==dom_io. Then you'd do an
iomem_access_permitted check passing in a pointer to the granting domain.

You should perhaps pull the core of get_page_from_l1e() into a supporting
function which you can then make available for common/grant_table.c to call
into.

 -- Keir

Re: RFC: Using grant table to give iomem permission

[Kieran Mansley]

On Fri, 2007-05-18 at 15:45 +0100, Keir Fraser wrote:
> On 18/5/07 15:14, "Kieran Mansley" <kmansley@solarflare.com> wrote:
> 
> >>> Some questions from me:
> >>>  - does this approach seem sane?
> >> 
> >> There's no reason you shouldn't be able to use GNTMAP_host_map as usual, and
> >> do refcounting in the active grant entry, also as usual.
> > 
> > OK.  My reluctance to do that was simply that I wasn't sure if the
> > operations that take place when doing a GNTMAP_host_map would conflict
> > with those when doing an iomem_map.  If you think they shouldn't, I'll
> > give it a go.
> 
> I'm not sure what you mean.

That if GNTMAP_host_map is specified, the map/unmap operations do a
number of things that might not be appropriate.  eg.

    if ( op->flags & GNTMAP_host_map )
    {
        rc = create_grant_host_mapping(op->host_addr, frame, op->flags);
        if ( rc != GNTST_okay )
        {
            if ( !(op->flags & GNTMAP_readonly) )
                put_page_type(mfn_to_page(frame));
            put_page(mfn_to_page(frame));
            goto undo_out;
        }

I could prevent it doing these things in the case of it being iomem, but
in that case I felt that it was no longer really doing a host_map
operation, and so to avoid confusion (e.g. others maintaining the code
might not realise that GNTMAP_host_map can mean two different things)
thought it better to give it a separate type.

> > That raises one of my other questions which is how to test for it being
> > a valid RAM page, as in the absence of it GNTMAP_iomem_map that's pretty
> > much the only indicator that it's iomem we're dealing with.  As I said,
> > mfn_valid() doesn't seem to be sufficient.
> 
> Yes, I misled you here. You want the middle bit of get_page_from_l1e(). It
> checks for !mfn_valid()||page_get_owner()==dom_io. Then you'd do an
> iomem_access_permitted check passing in a pointer to the granting domain.
> 
> You should perhaps pull the core of get_page_from_l1e() into a supporting
> function which you can then make available for common/grant_table.c to call
> into.

Very helpful, thanks

Kieran