[PATCH] grant table and bogus mfns

[PATCH] grant table and bogus mfns

[Samuel Thibault]

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

Hi,

While developping a frontend, I noticed that if I gave a grant on mfn 0
to a backend (which is bogus), that backend would happily be allowed to
map it, and hence Oops...

This is because mfn 0 is considered as iomem, and in that case
gnttab_map_grant_ref only checks that the target domain is allowed to
access it.  The patch below makes it check that the source domain was
allowed to access it (and then give the target access to it).

Samuel

Signed-Off-By: Samuel Thibault <samuel.thibault@eu.citrix.com>

diff -r 4bf62459d45a xen/common/grant_table.c
--- a/xen/common/grant_table.c	Wed Nov 07 11:29:38 2007 +0000
+++ b/xen/common/grant_table.c	Fri Nov 09 15:01:57 2007 +0000
@@ -335,7 +335,8 @@ __gnttab_map_grant_ref(
         if ( iomem_page_test(frame, mfn_to_page(frame)) )
         {
             is_iomem = 1;
-            if ( iomem_permit_access(ld, frame, frame) )
+            if ( !iomem_access_permitted(rd, frame, frame)
+                  || iomem_permit_access(ld, frame, frame) )
             {
                 gdprintk(XENLOG_WARNING, 
                          "Could not permit access to grant frame "

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 617 bytes --]

diff -r 4bf62459d45a xen/common/grant_table.c
--- a/xen/common/grant_table.c	Wed Nov 07 11:29:38 2007 +0000
+++ b/xen/common/grant_table.c	Fri Nov 09 15:01:57 2007 +0000
@@ -335,7 +335,8 @@ __gnttab_map_grant_ref(
         if ( iomem_page_test(frame, mfn_to_page(frame)) )
         {
             is_iomem = 1;
-            if ( iomem_permit_access(ld, frame, frame) )
+            if ( !iomem_access_permitted(rd, frame, frame)
+                  || iomem_permit_access(ld, frame, frame) )
             {
                 gdprintk(XENLOG_WARNING, 
                          "Could not permit access to grant frame "

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

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

Re: [PATCH] grant table and bogus mfns

[Keir Fraser]

Thanks, but actually I think the iomem integration is more broken than it
first appears. I'm not sure why the local iomem permissions are modified at
all. The remote permissions should be interrogated, and that should suffice
alone. But you certainly have the gist of a fix here -- AFAICS the code
as-is allows unprivileged domains to 'grant' each other access to arbitrary
pages of I/O memory, which isn't good. I've cc'ed Kieran who was the
originator of that code.

 -- Keir

On 9/11/07 15:13, "Samuel Thibault" <samuel.thibault@eu.citrix.com> wrote:

> Hi,
> 
> While developping a frontend, I noticed that if I gave a grant on mfn 0
> to a backend (which is bogus), that backend would happily be allowed to
> map it, and hence Oops...
> 
> This is because mfn 0 is considered as iomem, and in that case
> gnttab_map_grant_ref only checks that the target domain is allowed to
> access it.  The patch below makes it check that the source domain was
> allowed to access it (and then give the target access to it).
> 
> Samuel
> 
> Signed-Off-By: Samuel Thibault <samuel.thibault@eu.citrix.com>
> 
> diff -r 4bf62459d45a xen/common/grant_table.c
> --- a/xen/common/grant_table.c Wed Nov 07 11:29:38 2007 +0000
> +++ b/xen/common/grant_table.c Fri Nov 09 15:01:57 2007 +0000
> @@ -335,7 +335,8 @@ __gnttab_map_grant_ref(
>          if ( iomem_page_test(frame, mfn_to_page(frame)) )
>          {
>              is_iomem = 1;
> -            if ( iomem_permit_access(ld, frame, frame) )
> +            if ( !iomem_access_permitted(rd, frame, frame)
> +                  || iomem_permit_access(ld, frame, frame) )
>              {
>                  gdprintk(XENLOG_WARNING,
>                           "Could not permit access to grant frame "
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] grant table and bogus mfns

[Kieran Mansley]

On Sat, 2007-11-10 at 09:28 +0000, Keir Fraser wrote:
> Thanks, but actually I think the iomem integration is more broken than it
> first appears. I'm not sure why the local iomem permissions are modified at
> all. The remote permissions should be interrogated, and that should suffice
> alone. But you certainly have the gist of a fix here -- AFAICS the code
> as-is allows unprivileged domains to 'grant' each other access to arbitrary
> pages of I/O memory, which isn't good. I've cc'ed Kieran who was the
> originator of that code.

As background: this code was written to provide a means to
programatically give I/O memory permissions to unprivileged domains,
roughly equivalent to a domctl iomem_permission operation.  The
suggestion at the time was that we should use the grant tables to
achieve this.  It's supposed to work as follows:

 1) dom0 does a grant op for a page of I/O memory; at this stage no
different to a normal grant.
 2) grant reference passed (e.g. through xenstore) to domU  
 3) domU performs a map operation on that grant
 4) hypervisor notices that the grant is for an I/O memory page and
instead of mapping it to a domU virtual address it instead sets up the
I/O mem permissions for that domain to access the region (ie. calls
iomem_permit_access())
 5) domU can then call ioremap() to get a kernel virtual address for the
I/O memory region, and access it as normal.

The bug I think stems from a weakness in iomem_page_test() (recently
renamed something like is_iomem_page() I think).  I had intended that
the call to check the owner of the page was dom_io would prevent
unprivileged domains granting access, but it of course needs to also
check that the granting domain owns the page.  i.e. a quick fix would be
to check where iomem_page_test() is called in 
__gnttab_map_grant_ref() that "rd == dom_io".

I can test and provide a patch for this later today.

If we wanted it to be more general (so that unprivileged domains could
legitimately give others access to their own regions of I/O memory) then
the suggestion of testing iomem_access_permitted() on the remote domain
would then be necessary, but if we restrict it to dom0 providing access
(as was initially intended) I'm not sure that is necessary.

Kieran

>  -- Keir
> 
> On 9/11/07 15:13, "Samuel Thibault" <samuel.thibault@eu.citrix.com> wrote:
> 
> > Hi,
> > 
> > While developping a frontend, I noticed that if I gave a grant on mfn 0
> > to a backend (which is bogus), that backend would happily be allowed to
> > map it, and hence Oops...
> > 
> > This is because mfn 0 is considered as iomem, and in that case
> > gnttab_map_grant_ref only checks that the target domain is allowed to
> > access it.  The patch below makes it check that the source domain was
> > allowed to access it (and then give the target access to it).
> > 
> > Samuel
> > 
> > Signed-Off-By: Samuel Thibault <samuel.thibault@eu.citrix.com>
> > 
> > diff -r 4bf62459d45a xen/common/grant_table.c
> > --- a/xen/common/grant_table.c Wed Nov 07 11:29:38 2007 +0000
> > +++ b/xen/common/grant_table.c Fri Nov 09 15:01:57 2007 +0000
> > @@ -335,7 +335,8 @@ __gnttab_map_grant_ref(
> >          if ( iomem_page_test(frame, mfn_to_page(frame)) )
> >          {
> >              is_iomem = 1;
> > -            if ( iomem_permit_access(ld, frame, frame) )
> > +            if ( !iomem_access_permitted(rd, frame, frame)
> > +                  || iomem_permit_access(ld, frame, frame) )
> >              {
> >                  gdprintk(XENLOG_WARNING,
> >                           "Could not permit access to grant frame "
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel
> 
> 

Re: [PATCH] grant table and bogus mfns

[Kieran Mansley]

On Mon, 2007-11-12 at 09:35 +0000, Kieran Mansley wrote:
> I can test and provide a patch for this later today.

Here it is.  I left in the check of the remote domain's iomem permission
as although it is unnecessary in the case of dom0 providing the grant it
still makes logical sense.  Thanks for pointing this problem out.

Signed-off-by Kieran Mansley <kmansley@solarflare.com>

diff -r e40591366a0f xen/common/grant_table.c
--- a/xen/common/grant_table.c	Mon Nov 12 09:53:14 2007 +0000
+++ b/xen/common/grant_table.c	Mon Nov 12 11:49:19 2007 +0000
@@ -332,7 +332,9 @@ __gnttab_map_grant_ref(
     if ( op->flags & GNTMAP_host_map ) 
     {
         /* Could be an iomem page for setting up permission */
-        if ( is_iomem_page(frame) )
+        if ( rd->domain_id == 0 && 
+             is_iomem_page(frame) &&
+             iomem_access_permitted(rd, frame, frame) )
         {
             is_iomem = 1;
             if ( iomem_permit_access(ld, frame, frame) )
@@ -527,7 +529,8 @@ __gnttab_unmap_common(
                                                   op->flags)) < 0 )
                 goto unmap_out;
         }
-        else if ( is_iomem_page(op->frame) &&
+        else if ( rd->domain_id == 0 &&
+                  is_iomem_page(op->frame) &&
                   iomem_access_permitted(ld, op->frame, op->frame) )
         {
             if ( (rc = iomem_deny_access(ld, op->frame, op->frame)) < 0 )

Re: [PATCH] grant table and bogus mfns

[Keir Fraser]

On 12/11/07 09:35, "Kieran Mansley" <kmansley@solarflare.com> wrote:

>  1) dom0 does a grant op for a page of I/O memory; at this stage no
> different to a normal grant.
>  2) grant reference passed (e.g. through xenstore) to domU
>  3) domU performs a map operation on that grant
>  4) hypervisor notices that the grant is for an I/O memory page and
> instead of mapping it to a domU virtual address it instead sets up the
> I/O mem permissions for that domain to access the region (ie. calls
> iomem_permit_access())
>  5) domU can then call ioremap() to get a kernel virtual address for the
> I/O memory region, and access it as normal.

I didn't realise this was how it worked. I think it's a bad idea -- mapping
the grantref should map the I/O page. The mapping domain's io capabilities
should not be affected. Apart from being the obvious semantics for
map_grant, using the current scheme we cannot be sure when all mappings to
the granted page have gone away.

 -- Keir

Re: [PATCH] grant table and bogus mfns

[Kieran Mansley]

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

On Tue, 2007-11-13 at 04:48 +0000, Keir Fraser wrote:
> On 12/11/07 09:35, "Kieran Mansley" <kmansley@solarflare.com> wrote:
> 
> >  1) dom0 does a grant op for a page of I/O memory; at this stage no
> > different to a normal grant.
> >  2) grant reference passed (e.g. through xenstore) to domU
> >  3) domU performs a map operation on that grant
> >  4) hypervisor notices that the grant is for an I/O memory page and
> > instead of mapping it to a domU virtual address it instead sets up the
> > I/O mem permissions for that domain to access the region (ie. calls
> > iomem_permit_access())
> >  5) domU can then call ioremap() to get a kernel virtual address for the
> > I/O memory region, and access it as normal.
> 
> I didn't realise this was how it worked. I think it's a bad idea -- mapping
> the grantref should map the I/O page. The mapping domain's io capabilities
> should not be affected. Apart from being the obvious semantics for
> map_grant,

I agree that mapping the I/O page when you map the grant ref makes for a
much better interface.  I'd just obviously got the wrong end of the
stick and thought that you wanted it the other way when we first
discussed this, and it hadn't occurred to me that doing the map without
first getting the right permissions was sane.

Attached is pair of patches against the current xen-
unstable.hg/linux-2.6.18-xen.hg (i.e. instead of not on top of the other
patch I posted in this thread) that should do the I/O page map when the
grant is mapped.  The I/O capabilities of the domain are no longer
modified.  It seems to work for me, is definitely an improvement in the
API, and should fix the original bug that was posted.

I added a GNTMAP_nocache flag to match the GNTMAP_readonly etc flags so
that you can get the equivalent behaviour of ioremap_nocache() vs
ioremap().  The grant is mapped in as normal; e.g.:

  struct gnttab_map_grant_ref op;
  gnttab_set_map_op(&op, (unsigned long)vaddr, GNTMAP_host_map | GNTMAP_nocache, gnt_ref, dev->otherend_id);
  BUG_ON(HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, &op, 1));

I'm slightly surprised that without the call to iomem_permit_access() I
don't trip over the iomem_access_permitted() check in
xen/arch/x86/mm.c:get_page_from_l1e() when the grant is mapped.  I hope
this doesn't indicate that I've missed something.

Thanks

Kieran

[-- Attachment #2: iomem_page_test_fix --]
[-- Type: text/x-patch, Size: 6319 bytes --]

diff -r e40591366a0f xen/arch/x86/mm.c
--- a/xen/arch/x86/mm.c	Mon Nov 12 09:53:14 2007 +0000
+++ b/xen/arch/x86/mm.c	Tue Nov 13 16:22:08 2007 +0000
@@ -849,12 +849,14 @@ void put_page_from_l1e(l1_pgentry_t l1e,
 void put_page_from_l1e(l1_pgentry_t l1e, struct domain *d)
 {
     unsigned long    pfn  = l1e_get_pfn(l1e);
-    struct page_info *page = mfn_to_page(pfn);
+    struct page_info *page;
     struct domain   *e;
     struct vcpu     *v;
 
     if ( !(l1e_get_flags(l1e) & _PAGE_PRESENT) || !mfn_valid(pfn) )
         return;
+
+    page = mfn_to_page(pfn);
 
     e = page_get_owner(page);
 
@@ -2772,6 +2774,8 @@ int create_grant_host_mapping(
         l1e_add_flags(pte,_PAGE_USER);
     if ( !(flags & GNTMAP_readonly) )
         l1e_add_flags(pte,_PAGE_RW);
+    if ( (flags & GNTMAP_nocache) )
+        l1e_add_flags(pte,_PAGE_PCD);
 
     if ( flags & GNTMAP_contains_pte )
         return create_grant_pte_mapping(addr, pte, current);
diff -r e40591366a0f xen/common/grant_table.c
--- a/xen/common/grant_table.c	Mon Nov 12 09:53:14 2007 +0000
+++ b/xen/common/grant_table.c	Tue Nov 13 15:50:01 2007 +0000
@@ -198,7 +198,6 @@ __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;
@@ -329,24 +328,38 @@ __gnttab_map_grant_ref(
 
     spin_unlock(&rd->grant_table->lock);
 
-    if ( op->flags & GNTMAP_host_map ) 
-    {
-        /* Could be an iomem page for setting up permission */
-        if ( is_iomem_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);
+    /* Could be an iomem page for remapping */
+    if ( rd->domain_id == 0 && 
+         is_iomem_page(frame) &&
+         iomem_access_permitted(rd, frame, frame) )
+    {
+        if ( op->flags & GNTMAP_host_map )
+        {
+            /* Reference counting for in-range I/O pages. */
+            if ( mfn_valid(frame) &&
+                 !((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 ( !is_iomem ) 
+        
+            rc = create_grant_host_mapping(op->host_addr, frame, op->flags);
+            if ( rc != GNTST_okay )
+            {
+                if ( mfn_valid(frame) && 
+                     !(op->flags & GNTMAP_readonly) )
+                    put_page_type(mfn_to_page(frame));
+                put_page(mfn_to_page(frame));
+                goto undo_out;
+            }
+        }
+    }
+    else
     {
         if ( unlikely(!mfn_valid(frame)) ||
              unlikely(!((op->flags & GNTMAP_readonly) ?
@@ -527,12 +540,6 @@ __gnttab_unmap_common(
                                                   op->flags)) < 0 )
                 goto unmap_out;
         }
-        else if ( is_iomem_page(op->frame) &&
-                  iomem_access_permitted(ld, op->frame, op->frame) )
-        {
-            if ( (rc = iomem_deny_access(ld, op->frame, op->frame)) < 0 )
-                goto unmap_out;
-        }
 
         ASSERT(act->pin & (GNTPIN_hstw_mask | GNTPIN_hstr_mask));
         op->map->flags &= ~GNTMAP_host_map;
@@ -607,10 +614,25 @@ __gnttab_unmap_common_complete(struct gn
             goto unmap_out;
         }
 
-        if ( op->flags & GNTMAP_readonly )
-            put_page(mfn_to_page(op->frame));
-        else
-            put_page_and_type(mfn_to_page(op->frame));
+        if ( rd->domain_id == 0 && 
+             is_iomem_page(op->frame) ) 
+        {
+            /* Only do refcounting on in-range mfns */
+            if ( mfn_valid(op->frame) ) 
+            {
+                if ( op->flags & GNTMAP_readonly )
+                    put_page(mfn_to_page(op->frame));
+                else
+                    put_page_and_type(mfn_to_page(op->frame));
+            }
+        }
+        else 
+        {
+            if ( op->flags & GNTMAP_readonly )
+                put_page(mfn_to_page(op->frame));
+            else
+                put_page_and_type(mfn_to_page(op->frame));
+        }
     }
 
     if ( (op->map->flags & (GNTMAP_device_map|GNTMAP_host_map)) == 0 )
@@ -1592,7 +1614,6 @@ gnttab_release_mappings(
     struct domain        *rd;
     struct active_grant_entry *act;
     struct grant_entry   *sha;
-    int rc;
 
     BUG_ON(!d->is_dying);
 
@@ -1651,9 +1672,14 @@ gnttab_release_mappings(
                 BUG_ON(!(act->pin & GNTPIN_hstw_mask));
                 act->pin -= GNTPIN_hstw_inc;
 
-                if ( is_iomem_page(act->frame) &&
-                     iomem_access_permitted(rd, act->frame, act->frame) )
-                    rc = iomem_deny_access(rd, act->frame, act->frame);
+                if ( rd->domain_id == 0 &&
+                     is_iomem_page(act->frame) )
+                {
+                    /* Only do reference count on in-range mfns */
+                    if( mfn_valid(act->frame) )
+                        gnttab_release_put_page_and_type
+                            (mfn_to_page(act->frame));
+                }
                 else 
                     gnttab_release_put_page_and_type(mfn_to_page(act->frame));
             }
diff -r e40591366a0f xen/include/public/grant_table.h
--- a/xen/include/public/grant_table.h	Mon Nov 12 09:53:14 2007 +0000
+++ b/xen/include/public/grant_table.h	Tue Nov 13 11:26:26 2007 +0000
@@ -380,6 +380,9 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_unmap_and
 #define _GNTMAP_contains_pte    (4)
 #define GNTMAP_contains_pte     (1<<_GNTMAP_contains_pte)
 
+#define _GNTMAP_nocache         (5)
+#define GNTMAP_nocache          (1<<_GNTMAP_nocache)
+
 /*
  * Values for error status returns. All errors are -ve.
  */

[-- Attachment #3: add_nocache_grant_flag --]
[-- Type: text/x-patch, Size: 521 bytes --]

diff -r 4c96da4993b8 include/xen/interface/grant_table.h
--- a/include/xen/interface/grant_table.h	Mon Nov 12 10:17:44 2007 +0000
+++ b/include/xen/interface/grant_table.h	Tue Nov 13 16:20:54 2007 +0000
@@ -380,6 +380,9 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_unmap_and
 #define _GNTMAP_contains_pte    (4)
 #define GNTMAP_contains_pte     (1<<_GNTMAP_contains_pte)
 
+#define _GNTMAP_nocache         (5)
+#define GNTMAP_nocache          (1<<_GNTMAP_nocache)
+
 /*
  * Values for error status returns. All errors are -ve.
  */

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

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

Re: [PATCH] grant table and bogus mfns

[Keir Fraser]

On 13/11/07 16:40, "Kieran Mansley" <kmansley@solarflare.com> wrote:

> I'm slightly surprised that without the call to iomem_permit_access() I
> don't trip over the iomem_access_permitted() check in
> xen/arch/x86/mm.c:get_page_from_l1e() when the grant is mapped.  I hope
> this doesn't indicate that I've missed something.

get_page_from_l1e() is not executed on the map_grant path, which explains
this behaviour.

As for GNTMAP_nocache, I think actually we should not trust the mapper to
get the cache attributes correct (because sometimes if the attributes are
wrong the system can be destabilised). Actually we now in xen-unstable have
cache-attribute tracking for RAM pages. The cache attributes for the granted
mapping should pick up PAT/PWT/PCD from the page's count_info field. If dom0
has the page mapped with the right attributes this will then do the right
thing immediately. If dom0 doesn't have the page mapped then we'll need to
do a bit more work...

 -- Keir

Re: [PATCH] grant table and bogus mfns

[Kieran Mansley]

On Tue, 2007-11-13 at 16:45 +0000, Keir Fraser wrote:
> As for GNTMAP_nocache, I think actually we should not trust the mapper to
> get the cache attributes correct (because sometimes if the attributes are
> wrong the system can be destabilised).

Is this just (or mainly?) a concern for RAM pages?  If not, you'll have
the same problem when using the domctl iomem_permission and
ioremap_nocache() in the guest.

>  Actually we now in xen-unstable have
> cache-attribute tracking for RAM pages. The cache attributes for the granted
> mapping should pick up PAT/PWT/PCD from the page's count_info field. If dom0
> has the page mapped with the right attributes this will then do the right
> thing immediately. If dom0 doesn't have the page mapped then we'll need to
> do a bit more work...

dom0 does have the page mapped, but I think there is no page_info struct
(and so no count_info) for some I/O memory pages.  Perhaps a short-term
fix would be to remove the GNTMAP_nocache option that I've added and
instead just make all I/O memory pages that get mapped through this
mechanism forced to _PAGE_PCD.  That way the mapper doesn't get to
choose (and we loose the ability to do an ioremap() equivalent as
opposed to an ioremap_nocache() equivalent, but that's not a big problem
right now).  

If that sounds agreeable I can spin another patch.

Kieran 

Re: [PATCH] grant table and bogus mfns

[Keir Fraser]

You make a good point. Let's instead allow the granter to explicitly specify
the cache attributes in the grant entry. We have space in the flags field.
Let's use bits 5,6,7 of that field. They can have the same format as the
three contiguous bits we have added in page->count_info. Conveniently, all
zeroes means map-WB-cacheable, which is the correct default.

 -- Keir

On 13/11/07 17:06, "Kieran Mansley" <kmansley@solarflare.com> wrote:

> On Tue, 2007-11-13 at 16:45 +0000, Keir Fraser wrote:
>> As for GNTMAP_nocache, I think actually we should not trust the mapper to
>> get the cache attributes correct (because sometimes if the attributes are
>> wrong the system can be destabilised).
> 
> Is this just (or mainly?) a concern for RAM pages?  If not, you'll have
> the same problem when using the domctl iomem_permission and
> ioremap_nocache() in the guest.
> 
>>  Actually we now in xen-unstable have
>> cache-attribute tracking for RAM pages. The cache attributes for the granted
>> mapping should pick up PAT/PWT/PCD from the page's count_info field. If dom0
>> has the page mapped with the right attributes this will then do the right
>> thing immediately. If dom0 doesn't have the page mapped then we'll need to
>> do a bit more work...
> 
> dom0 does have the page mapped, but I think there is no page_info struct
> (and so no count_info) for some I/O memory pages.  Perhaps a short-term
> fix would be to remove the GNTMAP_nocache option that I've added and
> instead just make all I/O memory pages that get mapped through this
> mechanism forced to _PAGE_PCD.  That way the mapper doesn't get to
> choose (and we loose the ability to do an ioremap() equivalent as
> opposed to an ioremap_nocache() equivalent, but that's not a big problem
> right now).  
> 
> If that sounds agreeable I can spin another patch.
> 
> Kieran 
> 

Re: [PATCH] grant table and bogus mfns

[Kieran Mansley]

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

On Tue, 2007-11-13 at 17:12 +0000, Keir Fraser wrote:
> You make a good point. Let's instead allow the granter to explicitly specify
> the cache attributes in the grant entry. We have space in the flags field.
> Let's use bits 5,6,7 of that field. They can have the same format as the
> three contiguous bits we have added in page->count_info. Conveniently, all
> zeroes means map-WB-cacheable, which is the correct default.

Attached is another spin of the patches to do this, based on a tree with
16067 reverted as requested.

>From the guest API point of view, previously gnttab_grant_foreign_access
() (and _ref()) took a "readonly" flag as an argument.  This is now a
more general flags field to allow the granter to specify the cache
attributes.  I also took the opportunity to remove the "readonly" flag
from gnttab_remove_foreign_access() as it was unused and I couldn't see
how to make useful in light of this change.

Within the hypervisor, the granter's specified cache attributes are
ignored at the point of mapping unless it is an I/O memory grant; for
RAM grants the cache attributes should I think be picked up
automatically using the cache-attribute tracking you mentioned, and this
seemed safer than allowing the granter to override.

In the case of an I/O memory map the granter-specified flags are passed
into create_grant_host_mapping() to add to the page table entry's flags.

I/0 memory maps using the grant table are only allowed in the case where
dom0 is doing the granting, the granter has iomem_access_permitted(),
GNTMAP_host_map is specified and GNTMAP_device_map is not.  Reference
counting using put_page/get_page is only done for those I/0 memory pages
that have mfn_valid()

I'm pretty happy with this now.  

Signed-off-by Kieran Mansley <kmansley@solarflare.com>

Thanks

Kieran

[-- Attachment #2: grant_cache_flags --]
[-- Type: text/x-patch, Size: 9731 bytes --]

diff -r 4c96da4993b8 drivers/char/tpm/tpm_xen.c
--- a/drivers/char/tpm/tpm_xen.c	Mon Nov 12 10:17:44 2007 +0000
+++ b/drivers/char/tpm/tpm_xen.c	Wed Nov 14 11:47:09 2007 +0000
@@ -266,8 +266,7 @@ static void destroy_tpmring(struct tpm_p
 	tpmif_set_connected_state(tp, 0);
 
 	if (tp->ring_ref != GRANT_INVALID_REF) {
-		gnttab_end_foreign_access(tp->ring_ref, 0,
-					  (unsigned long)tp->tx);
+		gnttab_end_foreign_access(tp->ring_ref, (unsigned long)tp->tx);
 		tp->ring_ref = GRANT_INVALID_REF;
 		tp->tx = NULL;
 	}
diff -r 4c96da4993b8 drivers/xen/blkfront/blkfront.c
--- a/drivers/xen/blkfront/blkfront.c	Mon Nov 12 10:17:44 2007 +0000
+++ b/drivers/xen/blkfront/blkfront.c	Wed Nov 14 16:22:32 2007 +0000
@@ -622,7 +622,7 @@ static int blkif_queue_request(struct re
 				ref,
 				info->xbdev->otherend_id,
 				buffer_mfn,
-				rq_data_dir(req) );
+				rq_data_dir(req) ? GTF_readonly : 0 );
 
 			info->shadow[id].frame[ring_req->nr_segments] =
 				mfn_to_pfn(buffer_mfn);
@@ -790,7 +790,7 @@ static void blkif_free(struct blkfront_i
 
 	/* Free resources associated with old device channel. */
 	if (info->ring_ref != GRANT_INVALID_REF) {
-		gnttab_end_foreign_access(info->ring_ref, 0,
+		gnttab_end_foreign_access(info->ring_ref, 
 					  (unsigned long)info->ring.sring);
 		info->ring_ref = GRANT_INVALID_REF;
 		info->ring.sring = NULL;
@@ -804,7 +804,7 @@ static void blkif_completion(struct blk_
 {
 	int i;
 	for (i = 0; i < s->req.nr_segments; i++)
-		gnttab_end_foreign_access(s->req.seg[i].gref, 0, 0UL);
+		gnttab_end_foreign_access(s->req.seg[i].gref, 0UL);
 }
 
 static void blkif_recover(struct blkfront_info *info)
@@ -846,9 +846,9 @@ static void blkif_recover(struct blkfron
 				req->seg[j].gref,
 				info->xbdev->otherend_id,
 				pfn_to_mfn(info->shadow[req->id].frame[j]),
-				rq_data_dir(
-					(struct request *)
-					info->shadow[req->id].request));
+				rq_data_dir((struct request *)
+					    info->shadow[req->id].request) ?
+				GTF_readonly : 0);
 		info->shadow[req->id].req = *req;
 
 		info->ring.req_prod_pvt++;
diff -r 4c96da4993b8 drivers/xen/core/gnttab.c
--- a/drivers/xen/core/gnttab.c	Mon Nov 12 10:17:44 2007 +0000
+++ b/drivers/xen/core/gnttab.c	Wed Nov 14 16:25:45 2007 +0000
@@ -140,7 +140,7 @@ static void put_free_entry(grant_ref_t r
  */
 
 int gnttab_grant_foreign_access(domid_t domid, unsigned long frame,
-				int readonly)
+				int flags)
 {
 	int ref;
 
@@ -150,19 +150,21 @@ int gnttab_grant_foreign_access(domid_t 
 	shared[ref].frame = frame;
 	shared[ref].domid = domid;
 	wmb();
-	shared[ref].flags = GTF_permit_access | (readonly ? GTF_readonly : 0);
+	BUG_ON(flags & (GTF_accept_transfer | GTF_reading | GTF_writing));
+	shared[ref].flags = GTF_permit_access | flags;
 
 	return ref;
 }
 EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access);
 
 void gnttab_grant_foreign_access_ref(grant_ref_t ref, domid_t domid,
-				     unsigned long frame, int readonly)
+				     unsigned long frame, int flags)
 {
 	shared[ref].frame = frame;
 	shared[ref].domid = domid;
 	wmb();
-	shared[ref].flags = GTF_permit_access | (readonly ? GTF_readonly : 0);
+	BUG_ON(flags & (GTF_accept_transfer | GTF_reading | GTF_writing));
+	shared[ref].flags = GTF_permit_access | flags;
 }
 EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_ref);
 
@@ -177,7 +179,7 @@ int gnttab_query_foreign_access(grant_re
 }
 EXPORT_SYMBOL_GPL(gnttab_query_foreign_access);
 
-int gnttab_end_foreign_access_ref(grant_ref_t ref, int readonly)
+int gnttab_end_foreign_access_ref(grant_ref_t ref)
 {
 	u16 flags, nflags;
 
@@ -194,10 +196,9 @@ int gnttab_end_foreign_access_ref(grant_
 }
 EXPORT_SYMBOL_GPL(gnttab_end_foreign_access_ref);
 
-void gnttab_end_foreign_access(grant_ref_t ref, int readonly,
-			       unsigned long page)
-{
-	if (gnttab_end_foreign_access_ref(ref, readonly)) {
+void gnttab_end_foreign_access(grant_ref_t ref, unsigned long page)
+{
+	if (gnttab_end_foreign_access_ref(ref)) {
 		put_free_entry(ref);
 		if (page != 0)
 			free_page(page);
diff -r 4c96da4993b8 drivers/xen/netfront/netfront.c
--- a/drivers/xen/netfront/netfront.c	Mon Nov 12 10:17:44 2007 +0000
+++ b/drivers/xen/netfront/netfront.c	Wed Nov 14 16:22:57 2007 +0000
@@ -663,8 +663,7 @@ static void network_tx_buf_gc(struct net
 				       "domain.\n");
 				BUG();
 			}
-			gnttab_end_foreign_access_ref(
-				np->grant_tx_ref[id], GNTMAP_readonly);
+			gnttab_end_foreign_access_ref(np->grant_tx_ref[id]);
 			gnttab_release_grant_reference(
 				&np->gref_tx_head, np->grant_tx_ref[id]);
 			np->grant_tx_ref[id] = GRANT_INVALID_REF;
@@ -888,7 +887,7 @@ static void xennet_make_frags(struct sk_
 
 		mfn = virt_to_mfn(data);
 		gnttab_grant_foreign_access_ref(ref, np->xbdev->otherend_id,
-						mfn, GNTMAP_readonly);
+						mfn, GTF_readonly);
 
 		tx->gref = np->grant_tx_ref[id] = ref;
 		tx->offset = offset;
@@ -910,7 +909,7 @@ static void xennet_make_frags(struct sk_
 
 		mfn = pfn_to_mfn(page_to_pfn(frag->page));
 		gnttab_grant_foreign_access_ref(ref, np->xbdev->otherend_id,
-						mfn, GNTMAP_readonly);
+						mfn, GTF_readonly);
 
 		tx->gref = np->grant_tx_ref[id] = ref;
 		tx->offset = frag->page_offset;
@@ -972,7 +971,7 @@ static int network_start_xmit(struct sk_
 	BUG_ON((signed short)ref < 0);
 	mfn = virt_to_mfn(data);
 	gnttab_grant_foreign_access_ref(
-		ref, np->xbdev->otherend_id, mfn, GNTMAP_readonly);
+		ref, np->xbdev->otherend_id, mfn, GTF_readonly);
 	tx->gref = np->grant_tx_ref[id] = ref;
 	tx->offset = offset;
 	tx->size = len;
@@ -1198,7 +1197,7 @@ static int xennet_get_responses(struct n
 			}
 			pages_flipped++;
 		} else {
-			ret = gnttab_end_foreign_access_ref(ref, 0);
+			ret = gnttab_end_foreign_access_ref(ref);
 			BUG_ON(!ret);
 		}
 
@@ -1531,8 +1530,7 @@ static void netif_release_tx_bufs(struct
 			continue;
 
 		skb = np->tx_skbs[i];
-		gnttab_end_foreign_access_ref(
-			np->grant_tx_ref[i], GNTMAP_readonly);
+		gnttab_end_foreign_access_ref(np->grant_tx_ref[i]);
 		gnttab_release_grant_reference(
 			&np->gref_tx_head, np->grant_tx_ref[i]);
 		np->grant_tx_ref[i] = GRANT_INVALID_REF;
@@ -1642,7 +1640,7 @@ static void netif_release_rx_bufs_copy(s
 
 		skb = np->rx_skbs[i];
 
-		if (!gnttab_end_foreign_access_ref(ref, 0))
+		if (!gnttab_end_foreign_access_ref(ref))
 		{
 			busy++;
 			continue;
@@ -2151,7 +2149,7 @@ static void end_access(int ref, void *pa
 static void end_access(int ref, void *page)
 {
 	if (ref != GRANT_INVALID_REF)
-		gnttab_end_foreign_access(ref, 0, (unsigned long)page);
+		gnttab_end_foreign_access(ref, (unsigned long)page);
 }
 
 
diff -r 4c96da4993b8 drivers/xen/pcifront/xenbus.c
--- a/drivers/xen/pcifront/xenbus.c	Mon Nov 12 10:17:44 2007 +0000
+++ b/drivers/xen/pcifront/xenbus.c	Wed Nov 14 11:47:07 2007 +0000
@@ -57,7 +57,7 @@ static void free_pdev(struct pcifront_de
 		xenbus_free_evtchn(pdev->xdev, pdev->evtchn);
 
 	if (pdev->gnt_ref != INVALID_GRANT_REF)
-		gnttab_end_foreign_access(pdev->gnt_ref, 0,
+		gnttab_end_foreign_access(pdev->gnt_ref,
 					  (unsigned long)pdev->sh_info);
 
 	pdev->xdev->dev.driver_data = NULL;
diff -r 4c96da4993b8 include/xen/gnttab.h
--- a/include/xen/gnttab.h	Mon Nov 12 10:17:44 2007 +0000
+++ b/include/xen/gnttab.h	Wed Nov 14 09:51:06 2007 +0000
@@ -51,14 +51,14 @@ struct gnttab_free_callback {
 };
 
 int gnttab_grant_foreign_access(domid_t domid, unsigned long frame,
-				int readonly);
+				int flags);
 
 /*
  * End access through the given grant reference, iff the grant entry is no
  * longer in use.  Return 1 if the grant entry was freed, 0 if it is still in
  * use.
  */
-int gnttab_end_foreign_access_ref(grant_ref_t ref, int readonly);
+int gnttab_end_foreign_access_ref(grant_ref_t ref);
 
 /*
  * Eventually end access through the given grant reference, and once that
@@ -66,8 +66,7 @@ int gnttab_end_foreign_access_ref(grant_
  * immediately iff the grant entry is not in use, otherwise it will happen
  * some time later.  page may be 0, in which case no freeing will occur.
  */
-void gnttab_end_foreign_access(grant_ref_t ref, int readonly,
-			       unsigned long page);
+void gnttab_end_foreign_access(grant_ref_t ref, unsigned long page);
 
 int gnttab_grant_foreign_transfer(domid_t domid, unsigned long pfn);
 
@@ -97,7 +96,7 @@ void gnttab_cancel_free_callback(struct 
 void gnttab_cancel_free_callback(struct gnttab_free_callback *callback);
 
 void gnttab_grant_foreign_access_ref(grant_ref_t ref, domid_t domid,
-				     unsigned long frame, int readonly);
+				     unsigned long frame, int flags);
 
 void gnttab_grant_foreign_transfer_ref(grant_ref_t, domid_t domid,
 				       unsigned long pfn);
diff -r 4c96da4993b8 include/xen/interface/grant_table.h
--- a/include/xen/interface/grant_table.h	Mon Nov 12 10:17:44 2007 +0000
+++ b/include/xen/interface/grant_table.h	Wed Nov 14 09:46:25 2007 +0000
@@ -119,6 +119,7 @@ typedef struct grant_entry grant_entry_t
  *  GTF_readonly: Restrict @domid to read-only mappings and accesses. [GST]
  *  GTF_reading: Grant entry is currently mapped for reading by @domid. [XEN]
  *  GTF_writing: Grant entry is currently mapped for writing by @domid. [XEN]
+ *  GTF_PAT, GTF_PWT, GTF_PCD: Cache attribute flags for the grant [GST]
  */
 #define _GTF_readonly       (2)
 #define GTF_readonly        (1U<<_GTF_readonly)
@@ -126,6 +127,12 @@ typedef struct grant_entry grant_entry_t
 #define GTF_reading         (1U<<_GTF_reading)
 #define _GTF_writing        (4)
 #define GTF_writing         (1U<<_GTF_writing)
+#define _GTF_PAT            (5)
+#define GTF_PAT             (1U<<_GTF_PAT)
+#define _GTF_PWT            (6)
+#define GTF_PWT             (1U<<_GTF_PWT)
+#define _GTF_PCD            (7)
+#define GTF_PCD             (1U<<_GTF_PCD)
 
 /*
  * Subflags for GTF_accept_transfer:

[-- Attachment #3: iomem_page_test_fix --]
[-- Type: text/x-patch, Size: 13781 bytes --]

diff -r b9071cba1c2d xen/arch/ia64/xen/mm.c
--- a/xen/arch/ia64/xen/mm.c	Wed Nov 14 09:17:29 2007 +0000
+++ b/xen/arch/ia64/xen/mm.c	Wed Nov 14 11:20:52 2007 +0000
@@ -2144,16 +2144,18 @@ dom0vp_unexpose_foreign_p2m(struct domai
 // mfn: frame: machine page frame
 // flags: GNTMAP_readonly | GNTMAP_application_map | GNTMAP_contains_pte
 int
-create_grant_host_mapping(unsigned long gpaddr,
-              unsigned long mfn, unsigned int flags)
+create_grant_host_mapping(unsigned long gpaddr, unsigned long mfn, 
+                          unsigned int flags, unsigned int cache_flags)
 {
     struct domain* d = current->domain;
     struct page_info* page;
     int ret;
 
-    if (flags & (GNTMAP_device_map |
-                 GNTMAP_application_map | GNTMAP_contains_pte)) {
-        gdprintk(XENLOG_INFO, "%s: flags 0x%x\n", __func__, flags);
+    if ((flags & (GNTMAP_device_map | 
+                  GNTMAP_application_map | GNTMAP_contains_pte)) ||
+        (cache_flags)) {
+        gdprintk(XENLOG_INFO, "%s: flags 0x%x cache_flags 0x%x\n",
+                 __func__, flags, cache_flags);
         return GNTST_general_error;
     }
 
diff -r b9071cba1c2d xen/arch/powerpc/mm.c
--- a/xen/arch/powerpc/mm.c	Wed Nov 14 09:17:29 2007 +0000
+++ b/xen/arch/powerpc/mm.c	Wed Nov 14 10:35:22 2007 +0000
@@ -168,7 +168,7 @@ static int destroy_grant_va_mapping(
 }
 
 int create_grant_host_mapping(
-    unsigned long addr, unsigned long frame, unsigned int flags)
+    unsigned long addr, unsigned long frame, unsigned int flags, unsigned int cache_flags)
 {
     if (flags & GNTMAP_application_map) {
         printk("%s: GNTMAP_application_map not supported\n", __func__);
@@ -177,6 +177,11 @@ int create_grant_host_mapping(
     }
     if (flags & GNTMAP_contains_pte) {
         printk("%s: GNTMAP_contains_pte not supported\n", __func__);
+        BUG();
+        return GNTST_general_error;
+    }
+    if (cache_flags) {
+        printk("%s: cache_flags not supported\n", __func__);
         BUG();
         return GNTST_general_error;
     }
diff -r b9071cba1c2d xen/arch/x86/mm.c
--- a/xen/arch/x86/mm.c	Wed Nov 14 09:17:29 2007 +0000
+++ b/xen/arch/x86/mm.c	Wed Nov 14 10:32:22 2007 +0000
@@ -849,12 +849,14 @@ void put_page_from_l1e(l1_pgentry_t l1e,
 void put_page_from_l1e(l1_pgentry_t l1e, struct domain *d)
 {
     unsigned long    pfn  = l1e_get_pfn(l1e);
-    struct page_info *page = mfn_to_page(pfn);
+    struct page_info *page;
     struct domain   *e;
     struct vcpu     *v;
 
     if ( !(l1e_get_flags(l1e) & _PAGE_PRESENT) || !mfn_valid(pfn) )
         return;
+
+    page = mfn_to_page(pfn);
 
     e = page_get_owner(page);
 
@@ -2763,8 +2765,8 @@ static int destroy_grant_va_mapping(
     return replace_grant_va_mapping(addr, frame, l1e_empty(), v);
 }
 
-int create_grant_host_mapping(
-    uint64_t addr, unsigned long frame, unsigned int flags)
+int create_grant_host_mapping(uint64_t addr, unsigned long frame, 
+                              unsigned int flags, unsigned int cache_flags)
 {
     l1_pgentry_t pte = l1e_from_pfn(frame, GRANT_PTE_FLAGS);
 
@@ -2772,6 +2774,13 @@ int create_grant_host_mapping(
         l1e_add_flags(pte,_PAGE_USER);
     if ( !(flags & GNTMAP_readonly) )
         l1e_add_flags(pte,_PAGE_RW);
+
+    if ( (cache_flags & GTF_PAT) )
+        l1e_add_flags(pte,_PAGE_PAT);
+    if ( (cache_flags & GTF_PWT) )
+        l1e_add_flags(pte,_PAGE_PWT);
+    if ( (cache_flags & GTF_PCD) )
+        l1e_add_flags(pte,_PAGE_PCD);
 
     if ( flags & GNTMAP_contains_pte )
         return create_grant_pte_mapping(addr, pte, current);
diff -r b9071cba1c2d xen/common/grant_table.c
--- a/xen/common/grant_table.c	Wed Nov 14 09:17:29 2007 +0000
+++ b/xen/common/grant_table.c	Wed Nov 14 15:17:00 2007 +0000
@@ -198,6 +198,7 @@ __gnttab_map_grant_ref(
     int            handle;
     unsigned long  frame = 0;
     int            rc = GNTST_okay;
+    unsigned int   cache_flags = 0;
     struct active_grant_entry *act;
     struct grant_mapping *mt;
     grant_entry_t *sha;
@@ -326,36 +327,86 @@ __gnttab_map_grant_ref(
 
     frame = act->frame;
 
+    cache_flags = (sha->flags & (GTF_PAT | GTF_PWT | GTF_PCD) );
+
     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;
-    }
+    /* Could be an iomem page for remapping */
+    if ( is_iomem_page(frame) )
+    {
+        /*
+         * Only allow iomem GNTMAP_host_map mappings of pages from
+         * dom0 that have the correct iomem permissions set up.
+         * GNTMAP_device_map of iomem page makes no sense
+         */
+        if ( rd->domain_id != 0 ||
+             !iomem_access_permitted(rd, frame, frame) ||
+             !(op->flags & GNTMAP_host_map) ||
+             op->flags & GNTMAP_device_map)
+        {
+            gdprintk(XENLOG_WARNING,
+                     "Iomem mapping not permitted %lx (domain %d)\n", 
+                     frame, rd->domain_id);
+            rc = GNTST_general_error;
+            goto undo_out;
+        }
+
+        /* Reference counting for in-range I/O pages. */
+        if ( mfn_valid(frame) &&
+             !((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);
+        rc = create_grant_host_mapping(op->host_addr, frame, op->flags,
+                                       cache_flags);
         if ( rc != GNTST_okay )
         {
-            if ( !(op->flags & GNTMAP_readonly) )
+            if ( mfn_valid(frame) && 
+                 !(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);
+    }
+    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 )
+        {
+            rc = create_grant_host_mapping(op->host_addr, frame, op->flags, 0);
+            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);
+            }
         }
     }
 
@@ -576,10 +627,27 @@ __gnttab_unmap_common_complete(struct gn
             goto unmap_out;
         }
 
-        if ( op->flags & GNTMAP_readonly )
-            put_page(mfn_to_page(op->frame));
-        else
-            put_page_and_type(mfn_to_page(op->frame));
+        if ( is_iomem_page(op->frame) ) 
+        {
+            /* Shouldn't be able to get a mapping of iomem from domain != 0 */
+            BUG_ON(rd->domain_id != 0);
+             
+            /* Only do refcounting on in-range mfns */
+            if ( mfn_valid(op->frame) ) 
+            {
+                if ( op->flags & GNTMAP_readonly )
+                    put_page(mfn_to_page(op->frame));
+                else
+                    put_page_and_type(mfn_to_page(op->frame));
+            }
+        }
+        else 
+        {
+            if ( op->flags & GNTMAP_readonly )
+                put_page(mfn_to_page(op->frame));
+            else
+                put_page_and_type(mfn_to_page(op->frame));
+        }
     }
 
     if ( (op->map->flags & (GNTMAP_device_map|GNTMAP_host_map)) == 0 )
@@ -1602,7 +1670,16 @@ gnttab_release_mappings(
             {
                 BUG_ON(!(act->pin & GNTPIN_hstr_mask));
                 act->pin -= GNTPIN_hstr_inc;
-                gnttab_release_put_page(mfn_to_page(act->frame));
+                if ( is_iomem_page(act->frame) )
+                {
+                    BUG_ON(rd->domain_id != 0);
+                    /* Only do reference count on in-range mfns */
+                    if( mfn_valid(act->frame) )
+                        gnttab_release_put_page_and_type
+                            (mfn_to_page(act->frame));
+                }
+                else
+                    gnttab_release_put_page(mfn_to_page(act->frame));
             }
         }
         else
@@ -1618,7 +1695,16 @@ 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 ( is_iomem_page(act->frame) )
+                {
+                    BUG_ON(rd->domain_id != 0);
+                    /* Only do reference count on in-range mfns */
+                    if( mfn_valid(act->frame) )
+                        gnttab_release_put_page_and_type
+                            (mfn_to_page(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 b9071cba1c2d xen/include/asm-ia64/grant_table.h
--- a/xen/include/asm-ia64/grant_table.h	Wed Nov 14 09:17:29 2007 +0000
+++ b/xen/include/asm-ia64/grant_table.h	Wed Nov 14 11:47:06 2007 +0000
@@ -8,7 +8,8 @@
 #define INITIAL_NR_GRANT_FRAMES 1
 
 // for grant map/unmap
-int create_grant_host_mapping(unsigned long gpaddr, unsigned long mfn, unsigned int flags);
+int create_grant_host_mapping(unsigned long gpaddr, unsigned long mfn, 
+			      unsigned int flags, unsigned int cache_flags);
 int replace_grant_host_mapping(unsigned long gpaddr, unsigned long mfn, unsigned long new_gpaddr, unsigned int flags);
 
 // for grant transfer
diff -r b9071cba1c2d xen/include/asm-powerpc/grant_table.h
--- a/xen/include/asm-powerpc/grant_table.h	Wed Nov 14 09:17:29 2007 +0000
+++ b/xen/include/asm-powerpc/grant_table.h	Wed Nov 14 11:47:05 2007 +0000
@@ -33,8 +33,8 @@ extern long pte_remove(ulong flags, ulon
 extern long pte_remove(ulong flags, ulong ptex, ulong avpn,
                        ulong *hi, ulong *lo);
 
-int create_grant_host_mapping(
-    unsigned long addr, unsigned long frame, unsigned int flags);
+int create_grant_host_mapping(unsigned long addr, unsigned long frame, 
+			      unsigned int flags, unsigned int cache_flags);
 int replace_grant_host_mapping(
     unsigned long addr, unsigned long frame, unsigned long new_addr,
     unsigned int flags);
diff -r b9071cba1c2d xen/include/asm-x86/grant_table.h
--- a/xen/include/asm-x86/grant_table.h	Wed Nov 14 09:17:29 2007 +0000
+++ b/xen/include/asm-x86/grant_table.h	Wed Nov 14 13:48:18 2007 +0000
@@ -13,8 +13,8 @@
  * Caller must own caller's BIGLOCK, is responsible for flushing the TLB, and
  * must hold a reference to the page.
  */
-int create_grant_host_mapping(
-    uint64_t addr, unsigned long frame, unsigned int flags);
+int create_grant_host_mapping(uint64_t addr, unsigned long frame,
+			      unsigned int flags, unsigned int cache_flags);
 int replace_grant_host_mapping(
     uint64_t addr, unsigned long frame, uint64_t new_addr, unsigned int flags);
 
diff -r b9071cba1c2d xen/include/public/grant_table.h
--- a/xen/include/public/grant_table.h	Wed Nov 14 09:17:29 2007 +0000
+++ b/xen/include/public/grant_table.h	Wed Nov 14 11:59:43 2007 +0000
@@ -119,6 +119,7 @@ typedef struct grant_entry grant_entry_t
  *  GTF_readonly: Restrict @domid to read-only mappings and accesses. [GST]
  *  GTF_reading: Grant entry is currently mapped for reading by @domid. [XEN]
  *  GTF_writing: Grant entry is currently mapped for writing by @domid. [XEN]
+ *  GTF_PAT, GTF_PWT, GTF_PCD: Cache attribute flags for the grant [GST]
  */
 #define _GTF_readonly       (2)
 #define GTF_readonly        (1U<<_GTF_readonly)
@@ -126,6 +127,12 @@ typedef struct grant_entry grant_entry_t
 #define GTF_reading         (1U<<_GTF_reading)
 #define _GTF_writing        (4)
 #define GTF_writing         (1U<<_GTF_writing)
+#define _GTF_PAT            (5)
+#define GTF_PAT             (1U<<_GTF_PAT)
+#define _GTF_PWT            (6)
+#define GTF_PWT             (1U<<_GTF_PWT)
+#define _GTF_PCD            (7)
+#define GTF_PCD             (1U<<_GTF_PCD)
 
 /*
  * Subflags for GTF_accept_transfer:

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

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

about parallex fs

[tgh]

hi
I have read the paper about parallex FS in xen , and I am interested in
it ,but I notice that parallex FS which is in the tree for earlier
version ,has been dropped out in the later version, what is the reason
for dropping it out, in my opinion,it is an interesting and promising
technech,is it ? or are there some disadvantages in it ,or are there
some even more interesting substituent tech for it?
I am confused about it ,could someone give me some exeplaination

thanks in advance

Re: about parallex fs

[Mark Williamson]

> I have read the paper about parallex FS in xen , and I am interested in
> it ,but I notice that parallex FS which is in the tree for earlier
> version ,has been dropped out in the later version, what is the reason
> for dropping it out, in my opinion,it is an interesting and promising
> technech,is it ? or are there some disadvantages in it ,or are there
> some even more interesting substituent tech for it?
> I am confused about it ,could someone give me some exeplaination

I don't think the parallax in the mainline tree was ever developed very 
heavily; it seems likely that the version written up in the paper was based 
on a modified source tree.

But the code went unmaintained for some time and was presumably removed from 
the source tree due to it getting old and not working properly...  I agree it 
seemed like a really interesting technology and it's a shame not to see more 
done with it.  I get the impression that most enterprises are using NAS/SAN 
solutions already, so perhaps there wasn't sufficient demand for Parallax as 
a competing solution...

Cheers,
Mark

-- 
Dave: Just a question. What use is a unicyle with no seat?  And no pedals!
Mark: To answer a question with a question: What use is a skateboard?
Dave: Skateboards have wheels.
Mark: My wheel has a wheel!

Re: about parallex fs

[Andrew Warfield]

Actually, Parallax is currently under active development.  Dutch Meyer
recently presented a progress report on it at the Xen summit.  I
believe Dutch is planning a release of the updated Parallax source
RSN, but I'll leave it to him (copied) to commit to a date. ;)

a.

On Dec 1, 2007 9:18 PM, Mark Williamson <mark.williamson@cl.cam.ac.uk> wrote:
> > I have read the paper about parallex FS in xen , and I am interested in
> > it ,but I notice that parallex FS which is in the tree for earlier
> > version ,has been dropped out in the later version, what is the reason
> > for dropping it out, in my opinion,it is an interesting and promising
> > technech,is it ? or are there some disadvantages in it ,or are there
> > some even more interesting substituent tech for it?
> > I am confused about it ,could someone give me some exeplaination
>
> I don't think the parallax in the mainline tree was ever developed very
> heavily; it seems likely that the version written up in the paper was based
> on a modified source tree.
>
> But the code went unmaintained for some time and was presumably removed from
> the source tree due to it getting old and not working properly...  I agree it
> seemed like a really interesting technology and it's a shame not to see more
> done with it.  I get the impression that most enterprises are using NAS/SAN
> solutions already, so perhaps there wasn't sufficient demand for Parallax as
> a competing solution...
>
> Cheers,
> Mark
>
> --
> Dave: Just a question. What use is a unicyle with no seat?  And no pedals!
> Mark: To answer a question with a question: What use is a skateboard?
> Dave: Skateboards have wheels.
> Mark: My wheel has a wheel!
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>

Re: about parallex fs

[Mark Williamson]

> Actually, Parallax is currently under active development.  Dutch Meyer
> recently presented a progress report on it at the Xen summit.  I
> believe Dutch is planning a release of the updated Parallax source
> RSN, but I'll leave it to him (copied) to commit to a date. ;)

Ah, that's excellent news!  I had the impression that you'd moved onto other 
things and was worried that it'd lost priority.  It'll be awesome to see a 
parallax release.

I hope the summit proceedings get online soon so I can find out what's going 
on in Xenland ;-)

Cheers,
Mark

> a.
>
> On Dec 1, 2007 9:18 PM, Mark Williamson <mark.williamson@cl.cam.ac.uk> 
wrote:
> > > I have read the paper about parallex FS in xen , and I am interested in
> > > it ,but I notice that parallex FS which is in the tree for earlier
> > > version ,has been dropped out in the later version, what is the reason
> > > for dropping it out, in my opinion,it is an interesting and promising
> > > technech,is it ? or are there some disadvantages in it ,or are there
> > > some even more interesting substituent tech for it?
> > > I am confused about it ,could someone give me some exeplaination
> >
> > I don't think the parallax in the mainline tree was ever developed very
> > heavily; it seems likely that the version written up in the paper was
> > based on a modified source tree.
> >
> > But the code went unmaintained for some time and was presumably removed
> > from the source tree due to it getting old and not working properly...  I
> > agree it seemed like a really interesting technology and it's a shame not
> > to see more done with it.  I get the impression that most enterprises are
> > using NAS/SAN solutions already, so perhaps there wasn't sufficient
> > demand for Parallax as a competing solution...
> >
> > Cheers,
> > Mark
> >
> > --
> > Dave: Just a question. What use is a unicyle with no seat?  And no
> > pedals! Mark: To answer a question with a question: What use is a
> > skateboard? Dave: Skateboards have wheels.
> > Mark: My wheel has a wheel!
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel



-- 
Dave: Just a question. What use is a unicyle with no seat?  And no pedals!
Mark: To answer a question with a question: What use is a skateboard?
Dave: Skateboards have wheels.
Mark: My wheel has a wheel!