[Xen-devel] [PATCH] Allow programatic iomem permissions

[Kieran Mansley <kmansley@xxxxxxxxxxxxxx>]

This patch addresses the problem of how to give iomem access to guests
from dom0.  At the moment, these permissions can only be set up using
the xen tools, which pretty much rules out doing so programmatically
from a backend driver for example.

After some discussion with Keir it was decided that the best approach
was to use the grant table to provide these permissions.  This patch
adds this facility.

In use:
 - backend in dom0 grants (using the normal grant table ops) access to
the region of iomemory and passes a grant handle to the guest.
 - frontend in guest tries to map this grant, and the hypervisor spots
that it's for a region of iomemory rather than RAM.
 - hypervisor sets up the relevant iomem access permissions 
 - when finished with, frontend in guest unmaps the grant, and
hypervisor similarly removes the iomem access permission.

I've posted this before, and have been using it myself for some time, so
would welcome comments, code review and/or applying to xen-unstable

While this patch solves the problem above, there is a chicken-and-egg
situation that results.  Currently, grant table operations are only
permitted if you have some iomem permissions.  This is because of the
following in iocap.h:

/*
 * Until TLB flushing issues are sorted out we consider it unsafe for
 * domains with no hardware-access privileges to perform grant
map/transfer
 * operations.
 */
#define grant_operation_permitted(d)                    \
    (!rangeset_is_empty((d)->iomem_caps))

This means that you can't do the grant table operation to provide the
iomemory capability because you don't yet have the iomemory capability.
There are a couple of obvious solutions to this: 
 - make grant_operation_permitted more sophisticated so it can realise
that iomem grant operations should be allowed.
 - replace grant_operation_permitted with something like the Xen
Security Policy stuff that is being proposed.
 - solve the TLB issues alluded to in the above comment so we don't have
to have this restriction.

As the definition of grant_operation_permitted sounds like a bit of a
temporary hack I've done nothing to address this yet, but I would favour
the Xen Security Policy approach as being the most useful.  I'd welcome
suggestions on this.

Thanks

Kieran


Allow iomem permissions to be set up through grant table ops

Signed-off-by Kieran Mansley <kmansley@xxxxxxxxxxxxxx>

diff -r 76781ee87984 xen/arch/x86/mm.c
--- a/xen/arch/x86/mm.c Mon Jul 09 12:49:31 2007 +0100
+++ b/xen/arch/x86/mm.c Mon Jul 09 16:05:43 2007 +0100
@@ -594,6 +594,14 @@ get_##level##_linear_pagetable(         
     return 1;                                                               \
 }
 
+
+int iomem_page_test(unsigned long mfn, struct page_info *page)
+{
+    return unlikely(!mfn_valid(mfn)) ||
+        unlikely(page_get_owner(page) == dom_io);
+}
+
+
 int
 get_page_from_l1e(
     l1_pgentry_t l1e, struct domain *d)
@@ -611,8 +619,7 @@ get_page_from_l1e(
         return 0;
     }
 
-    if ( unlikely(!mfn_valid(mfn)) ||
-         unlikely(page_get_owner(page) == dom_io) )
+    if ( iomem_page_test(mfn, page) )
     {
         /* DOMID_IO reverts to caller for privilege checks. */
         if ( d == dom_io )
diff -r 76781ee87984 xen/common/grant_table.c
--- a/xen/common/grant_table.c  Mon Jul 09 12:49:31 2007 +0100
+++ b/xen/common/grant_table.c  Mon Jul 09 16:06:36 2007 +0100
@@ -187,6 +187,7 @@ __gnttab_map_grant_ref(
     int            handle;
     unsigned long  frame = 0;
     int            rc = GNTST_okay;
+    int            is_iomem = 0;
     struct active_grant_entry *act;
     struct grant_mapping *mt;
     grant_entry_t *sha;
@@ -315,34 +316,52 @@ __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_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));
+    if ( op->flags & GNTMAP_host_map ) 
+    {
+        /* Could be an iomem page for setting up permission */
+        if( iomem_page_test(frame, mfn_to_page(frame)) ) {
+            is_iomem = 1;
+            if ( iomem_permit_access(ld, frame, frame) ) {
+                gdprintk(XENLOG_WARNING, 
+                         "Could not permit access to grant frame %lx as 
iomem\n",
+                         frame);
+                rc = GNTST_general_error;
+                goto undo_out;
+            }
+        }
+    } 
+
+    if (!is_iomem ) 
+    {
+        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_device_map )
-        {
-            (void)get_page(mfn_to_page(frame), rd);
-            if ( !(op->flags & GNTMAP_readonly) )
-                get_page_type(mfn_to_page(frame), PGT_writable_page);
+        
+        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;
+            }
+
+            if ( op->flags & GNTMAP_device_map )
+            {
+                (void)get_page(mfn_to_page(frame), rd);
+                if ( !(op->flags & GNTMAP_readonly) )
+                    get_page_type(mfn_to_page(frame), PGT_writable_page);
+            }
         }
     }
 
@@ -485,23 +504,39 @@ __gnttab_unmap_common(
         }
     }
 
-    if ( (op->host_addr != 0) && (flags & GNTMAP_host_map) )
-    {
-        if ( (rc = replace_grant_host_mapping(op->host_addr,
-                                              frame, op->new_addr, flags)) < 0 
)
-            goto unmap_out;
-
-        ASSERT(act->pin & (GNTPIN_hstw_mask | GNTPIN_hstr_mask));
-        map->flags &= ~GNTMAP_host_map;
-        if ( flags & GNTMAP_readonly )
-        {
-            act->pin -= GNTPIN_hstr_inc;
-            put_page(mfn_to_page(frame));
-        }
-        else
-        {
-            act->pin -= GNTPIN_hstw_inc;
-            put_page_and_type(mfn_to_page(frame));
+    if ( flags & GNTMAP_host_map )
+    {
+        if ( op->host_addr != 0 )
+        {
+            if ( (rc = replace_grant_host_mapping(op->host_addr,
+                                                  frame, op->new_addr,
+                                                  flags)) < 0 )
+                goto unmap_out;
+             
+            ASSERT(act->pin & (GNTPIN_hstw_mask | GNTPIN_hstr_mask));
+            map->flags &= ~GNTMAP_host_map;
+            if ( flags & GNTMAP_readonly )
+            {
+                act->pin -= GNTPIN_hstr_inc;
+                put_page(mfn_to_page(frame));
+            }
+            else
+            {
+                act->pin -= GNTPIN_hstw_inc;
+                put_page_and_type(mfn_to_page(frame));
+            }
+        } else if ( iomem_page_test(frame, mfn_to_page(frame)) &&
+                    iomem_access_permitted(ld, frame, frame) ){
+
+            if ( (rc = iomem_deny_access(ld, frame, frame)) < 0 )
+                goto unmap_out;
+
+            ASSERT(act->pin & (GNTPIN_hstw_mask | GNTPIN_hstr_mask));
+            map->flags &= ~GNTMAP_host_map;
+            if ( flags & GNTMAP_readonly )
+                act->pin -= GNTPIN_hstr_inc;
+            else
+                act->pin -= GNTPIN_hstw_inc;
         }
     }
 
@@ -1429,6 +1464,7 @@ gnttab_release_mappings(
     struct domain        *rd;
     struct active_grant_entry *act;
     struct grant_entry   *sha;
+    int rc;
 
     BUG_ON(!d->is_dying);
 
@@ -1486,7 +1522,12 @@ gnttab_release_mappings(
             {
                 BUG_ON(!(act->pin & GNTPIN_hstw_mask));
                 act->pin -= GNTPIN_hstw_inc;
-                gnttab_release_put_page_and_type(mfn_to_page(act->frame));
+
+                if ( iomem_page_test(act->frame, mfn_to_page(act->frame)) &&
+                     iomem_access_permitted(rd, act->frame, act->frame) )
+                    rc = iomem_deny_access(rd, act->frame, act->frame);
+                else 
+                    gnttab_release_put_page_and_type(mfn_to_page(act->frame));
             }
 
             if ( (act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0 )
diff -r 76781ee87984 xen/include/asm-x86/mm.h
--- a/xen/include/asm-x86/mm.h  Mon Jul 09 12:49:31 2007 +0100
+++ b/xen/include/asm-x86/mm.h  Mon Jul 09 16:05:43 2007 +0100
@@ -197,6 +197,9 @@ static inline int get_page(struct page_i
     return 1;
 }
 
+/* Decide whether this page looks like iomem or real memory */
+int iomem_page_test(unsigned long mfn, struct page_info *page);
+
 void put_page_type(struct page_info *page);
 int  get_page_type(struct page_info *page, unsigned long type);
 int  get_page_from_l1e(l1_pgentry_t l1e, struct domain *d);

Attachment: iomem_grants
Description: Text document

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