[PATCH] Fix rcu domain locking for transitive grants

[PATCH] Fix rcu domain locking for transitive grants

[George Dunlap]

When acquiring a transitive grant for copy then the owning domain needs to
be locked down as well as the granting domain. This was being done, but the
unlocking was not. The acquire code now stores the struct domain * of the
owning domain (rather than the domid) in the active entry in the granting
domain. The release code then does the unlock on the owning domain.
Note that I believe I also fixed a bug where, for non-transitive grants
the active entry contained a reference to the acquiring domain rather
than the granting domain. From my reading of the code this would stop the
release code for transitive grants from terminating its recursion
correctly.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
CC: Steven Smith <steven.smith@citrix.com>

diff -r f071d8e9f744 -r 14211e98efac xen/common/grant_table.c
--- a/xen/common/grant_table.c	Tue Mar 08 10:23:52 2011 +0000
+++ b/xen/common/grant_table.c	Tue Mar 08 14:39:03 2011 +0000
@@ -1626,11 +1626,10 @@
     struct active_grant_entry *act;
     unsigned long r_frame;
     uint16_t *status;
-    domid_t trans_domid;
     grant_ref_t trans_gref;
     int released_read;
     int released_write;
-    struct domain *trans_dom;
+    struct domain *td;
 
     released_read = 0;
     released_write = 0;
@@ -1644,15 +1643,13 @@
     if (rd->grant_table->gt_version == 1)
     {
         status = &sha->flags;
-        trans_domid = rd->domain_id;
-        /* Shut the compiler up.  This'll never be used, because
-           trans_domid == rd->domain_id, but gcc doesn't know that. */
-        trans_gref = 0x1234567;
+        td = rd;
+        trans_gref = gref;
     }
     else
     {
         status = &status_entry(rd->grant_table, gref);
-        trans_domid = act->trans_dom;
+        td = act->trans_domain;
         trans_gref = act->trans_gref;
     }
 
@@ -1680,21 +1677,16 @@
 
     spin_unlock(&rd->grant_table->lock);
 
-    if ( trans_domid != rd->domain_id )
+    if ( td != rd )
     {
-        if ( released_write || released_read )
-        {
-            trans_dom = rcu_lock_domain_by_id(trans_domid);
-            if ( trans_dom != NULL )
-            {
-                /* Recursive calls, but they're tail calls, so it's
-                   okay. */
-                if ( released_write )
-                    __release_grant_for_copy(trans_dom, trans_gref, 0);
-                else if ( released_read )
-                    __release_grant_for_copy(trans_dom, trans_gref, 1);
-            }
-        }
+        /* Recursive calls, but they're tail calls, so it's
+           okay. */
+        if ( released_write )
+            __release_grant_for_copy(td, trans_gref, 0);
+        else if ( released_read )
+            __release_grant_for_copy(td, trans_gref, 1);
+
+	rcu_unlock_domain(td);
     }
 }
 
@@ -1731,7 +1723,7 @@
     uint32_t old_pin;
     domid_t trans_domid;
     grant_ref_t trans_gref;
-    struct domain *rrd;
+    struct domain *td;
     unsigned long gfn;
     unsigned long grant_frame;
     unsigned trans_page_off;
@@ -1785,8 +1777,8 @@
                                status) ) != GNTST_okay )
              goto unlock_out;
 
-        trans_domid = ld->domain_id;
-        trans_gref = 0;
+        td = rd;
+        trans_gref = gref;
         if ( sha2 && (shah->flags & GTF_type_mask) == GTF_transitive )
         {
             if ( !allow_transitive )
@@ -1808,14 +1800,15 @@
                that you don't need to go out of your way to avoid it
                in the guest. */
 
-            rrd = rcu_lock_domain_by_id(trans_domid);
-            if ( rrd == NULL )
+            /* We need to leave the rrd locked during the grant copy */
+            td = rcu_lock_domain_by_id(trans_domid);
+            if ( td == NULL )
                 PIN_FAIL(unlock_out, GNTST_general_error,
                          "transitive grant referenced bad domain %d\n",
                          trans_domid);
             spin_unlock(&rd->grant_table->lock);
 
-            rc = __acquire_grant_for_copy(rrd, trans_gref, rd,
+            rc = __acquire_grant_for_copy(td, trans_gref, rd,
                                           readonly, &grant_frame,
                                           &trans_page_off, &trans_length,
                                           0, &ignore);
@@ -1823,6 +1816,7 @@
             spin_lock(&rd->grant_table->lock);
             if ( rc != GNTST_okay ) {
                 __fixup_status_for_pin(act, status);
+	        rcu_unlock_domain(td);
                 spin_unlock(&rd->grant_table->lock);
                 return rc;
             }
@@ -1834,6 +1828,7 @@
             if ( act->pin != old_pin )
             {
                 __fixup_status_for_pin(act, status);
+	        rcu_unlock_domain(td);
                 spin_unlock(&rd->grant_table->lock);
                 return __acquire_grant_for_copy(rd, gref, ld, readonly,
                                                 frame, page_off, length,
@@ -1845,7 +1840,7 @@
                sub-page, but we always treat it as one because that
                blocks mappings of transitive grants. */
             is_sub_page = 1;
-            *owning_domain = rrd;
+            *owning_domain = td;
             act->gfn = -1ul;
         }
         else if ( sha1 )
@@ -1891,7 +1886,7 @@
             act->is_sub_page = is_sub_page;
             act->start = trans_page_off;
             act->length = trans_length;
-            act->trans_dom = trans_domid;
+            act->trans_domain = td;
             act->trans_gref = trans_gref;
             act->frame = grant_frame;
         }
diff -r f071d8e9f744 -r 14211e98efac xen/include/xen/grant_table.h
--- a/xen/include/xen/grant_table.h	Tue Mar 08 10:23:52 2011 +0000
+++ b/xen/include/xen/grant_table.h	Tue Mar 08 14:39:03 2011 +0000
@@ -32,7 +32,7 @@
 struct active_grant_entry {
     u32           pin;    /* Reference count information.             */
     domid_t       domid;  /* Domain being granted access.             */
-    domid_t       trans_dom;
+    struct domain *trans_domain;
     uint32_t      trans_gref;
     unsigned long frame;  /* Frame being granted.                     */
     unsigned long gfn;    /* Guest's idea of the frame being granted. */

Re: [PATCH] Fix rcu domain locking for transitive grants

[George Dunlap]

This should be backported to the 4.1 branch; it causes a hypervisor
BUG() if guests are using netchannel2 transtiive grants to talk to
each other when debug mode is on.

 -George

On Tue, Mar 8, 2011 at 3:02 PM, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> When acquiring a transitive grant for copy then the owning domain needs to
> be locked down as well as the granting domain. This was being done, but the
> unlocking was not. The acquire code now stores the struct domain * of the
> owning domain (rather than the domid) in the active entry in the granting
> domain. The release code then does the unlock on the owning domain.
> Note that I believe I also fixed a bug where, for non-transitive grants
> the active entry contained a reference to the acquiring domain rather
> than the granting domain. From my reading of the code this would stop the
> release code for transitive grants from terminating its recursion
> correctly.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> CC: Steven Smith <steven.smith@citrix.com>
>
> diff -r f071d8e9f744 -r 14211e98efac xen/common/grant_table.c
> --- a/xen/common/grant_table.c  Tue Mar 08 10:23:52 2011 +0000
> +++ b/xen/common/grant_table.c  Tue Mar 08 14:39:03 2011 +0000
> @@ -1626,11 +1626,10 @@
>     struct active_grant_entry *act;
>     unsigned long r_frame;
>     uint16_t *status;
> -    domid_t trans_domid;
>     grant_ref_t trans_gref;
>     int released_read;
>     int released_write;
> -    struct domain *trans_dom;
> +    struct domain *td;
>
>     released_read = 0;
>     released_write = 0;
> @@ -1644,15 +1643,13 @@
>     if (rd->grant_table->gt_version == 1)
>     {
>         status = &sha->flags;
> -        trans_domid = rd->domain_id;
> -        /* Shut the compiler up.  This'll never be used, because
> -           trans_domid == rd->domain_id, but gcc doesn't know that. */
> -        trans_gref = 0x1234567;
> +        td = rd;
> +        trans_gref = gref;
>     }
>     else
>     {
>         status = &status_entry(rd->grant_table, gref);
> -        trans_domid = act->trans_dom;
> +        td = act->trans_domain;
>         trans_gref = act->trans_gref;
>     }
>
> @@ -1680,21 +1677,16 @@
>
>     spin_unlock(&rd->grant_table->lock);
>
> -    if ( trans_domid != rd->domain_id )
> +    if ( td != rd )
>     {
> -        if ( released_write || released_read )
> -        {
> -            trans_dom = rcu_lock_domain_by_id(trans_domid);
> -            if ( trans_dom != NULL )
> -            {
> -                /* Recursive calls, but they're tail calls, so it's
> -                   okay. */
> -                if ( released_write )
> -                    __release_grant_for_copy(trans_dom, trans_gref, 0);
> -                else if ( released_read )
> -                    __release_grant_for_copy(trans_dom, trans_gref, 1);
> -            }
> -        }
> +        /* Recursive calls, but they're tail calls, so it's
> +           okay. */
> +        if ( released_write )
> +            __release_grant_for_copy(td, trans_gref, 0);
> +        else if ( released_read )
> +            __release_grant_for_copy(td, trans_gref, 1);
> +
> +       rcu_unlock_domain(td);
>     }
>  }
>
> @@ -1731,7 +1723,7 @@
>     uint32_t old_pin;
>     domid_t trans_domid;
>     grant_ref_t trans_gref;
> -    struct domain *rrd;
> +    struct domain *td;
>     unsigned long gfn;
>     unsigned long grant_frame;
>     unsigned trans_page_off;
> @@ -1785,8 +1777,8 @@
>                                status) ) != GNTST_okay )
>              goto unlock_out;
>
> -        trans_domid = ld->domain_id;
> -        trans_gref = 0;
> +        td = rd;
> +        trans_gref = gref;
>         if ( sha2 && (shah->flags & GTF_type_mask) == GTF_transitive )
>         {
>             if ( !allow_transitive )
> @@ -1808,14 +1800,15 @@
>                that you don't need to go out of your way to avoid it
>                in the guest. */
>
> -            rrd = rcu_lock_domain_by_id(trans_domid);
> -            if ( rrd == NULL )
> +            /* We need to leave the rrd locked during the grant copy */
> +            td = rcu_lock_domain_by_id(trans_domid);
> +            if ( td == NULL )
>                 PIN_FAIL(unlock_out, GNTST_general_error,
>                          "transitive grant referenced bad domain %d\n",
>                          trans_domid);
>             spin_unlock(&rd->grant_table->lock);
>
> -            rc = __acquire_grant_for_copy(rrd, trans_gref, rd,
> +            rc = __acquire_grant_for_copy(td, trans_gref, rd,
>                                           readonly, &grant_frame,
>                                           &trans_page_off, &trans_length,
>                                           0, &ignore);
> @@ -1823,6 +1816,7 @@
>             spin_lock(&rd->grant_table->lock);
>             if ( rc != GNTST_okay ) {
>                 __fixup_status_for_pin(act, status);
> +               rcu_unlock_domain(td);
>                 spin_unlock(&rd->grant_table->lock);
>                 return rc;
>             }
> @@ -1834,6 +1828,7 @@
>             if ( act->pin != old_pin )
>             {
>                 __fixup_status_for_pin(act, status);
> +               rcu_unlock_domain(td);
>                 spin_unlock(&rd->grant_table->lock);
>                 return __acquire_grant_for_copy(rd, gref, ld, readonly,
>                                                 frame, page_off, length,
> @@ -1845,7 +1840,7 @@
>                sub-page, but we always treat it as one because that
>                blocks mappings of transitive grants. */
>             is_sub_page = 1;
> -            *owning_domain = rrd;
> +            *owning_domain = td;
>             act->gfn = -1ul;
>         }
>         else if ( sha1 )
> @@ -1891,7 +1886,7 @@
>             act->is_sub_page = is_sub_page;
>             act->start = trans_page_off;
>             act->length = trans_length;
> -            act->trans_dom = trans_domid;
> +            act->trans_domain = td;
>             act->trans_gref = trans_gref;
>             act->frame = grant_frame;
>         }
> diff -r f071d8e9f744 -r 14211e98efac xen/include/xen/grant_table.h
> --- a/xen/include/xen/grant_table.h     Tue Mar 08 10:23:52 2011 +0000
> +++ b/xen/include/xen/grant_table.h     Tue Mar 08 14:39:03 2011 +0000
> @@ -32,7 +32,7 @@
>  struct active_grant_entry {
>     u32           pin;    /* Reference count information.             */
>     domid_t       domid;  /* Domain being granted access.             */
> -    domid_t       trans_dom;
> +    struct domain *trans_domain;
>     uint32_t      trans_gref;
>     unsigned long frame;  /* Frame being granted.                     */
>     unsigned long gfn;    /* Guest's idea of the frame being granted. */
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>

Re: [PATCH] Fix rcu domain locking for transitive grants

[Keir Fraser]

On 08/03/2011 15:11, "George Dunlap" <George.Dunlap@eu.citrix.com> wrote:

> This should be backported to the 4.1 branch; it causes a hypervisor
> BUG() if guests are using netchannel2 transtiive grants to talk to
> each other when debug mode is on.

I stubbed out the preemption checking stuff in 4.1 branch (it's not really
needed since there are no users of waitqueues in 4.1), so this patch is not
required. And that's fortunate, since it's quite non-trivial.

 -- Keir

>  -George
> 
> On Tue, Mar 8, 2011 at 3:02 PM, George Dunlap
> <george.dunlap@eu.citrix.com> wrote:
>> When acquiring a transitive grant for copy then the owning domain needs to
>> be locked down as well as the granting domain. This was being done, but the
>> unlocking was not. The acquire code now stores the struct domain * of the
>> owning domain (rather than the domid) in the active entry in the granting
>> domain. The release code then does the unlock on the owning domain.
>> Note that I believe I also fixed a bug where, for non-transitive grants
>> the active entry contained a reference to the acquiring domain rather
>> than the granting domain. From my reading of the code this would stop the
>> release code for transitive grants from terminating its recursion
>> correctly.
>> 
>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>> CC: Steven Smith <steven.smith@citrix.com>
>> 
>> diff -r f071d8e9f744 -r 14211e98efac xen/common/grant_table.c
>> --- a/xen/common/grant_table.c  Tue Mar 08 10:23:52 2011 +0000
>> +++ b/xen/common/grant_table.c  Tue Mar 08 14:39:03 2011 +0000
>> @@ -1626,11 +1626,10 @@
>>     struct active_grant_entry *act;
>>     unsigned long r_frame;
>>     uint16_t *status;
>> -    domid_t trans_domid;
>>     grant_ref_t trans_gref;
>>     int released_read;
>>     int released_write;
>> -    struct domain *trans_dom;
>> +    struct domain *td;
>> 
>>     released_read = 0;
>>     released_write = 0;
>> @@ -1644,15 +1643,13 @@
>>     if (rd->grant_table->gt_version == 1)
>>     {
>>         status = &sha->flags;
>> -        trans_domid = rd->domain_id;
>> -        /* Shut the compiler up.  This'll never be used, because
>> -           trans_domid == rd->domain_id, but gcc doesn't know that. */
>> -        trans_gref = 0x1234567;
>> +        td = rd;
>> +        trans_gref = gref;
>>     }
>>     else
>>     {
>>         status = &status_entry(rd->grant_table, gref);
>> -        trans_domid = act->trans_dom;
>> +        td = act->trans_domain;
>>         trans_gref = act->trans_gref;
>>     }
>> 
>> @@ -1680,21 +1677,16 @@
>> 
>>     spin_unlock(&rd->grant_table->lock);
>> 
>> -    if ( trans_domid != rd->domain_id )
>> +    if ( td != rd )
>>     {
>> -        if ( released_write || released_read )
>> -        {
>> -            trans_dom = rcu_lock_domain_by_id(trans_domid);
>> -            if ( trans_dom != NULL )
>> -            {
>> -                /* Recursive calls, but they're tail calls, so it's
>> -                   okay. */
>> -                if ( released_write )
>> -                    __release_grant_for_copy(trans_dom, trans_gref, 0);
>> -                else if ( released_read )
>> -                    __release_grant_for_copy(trans_dom, trans_gref, 1);
>> -            }
>> -        }
>> +        /* Recursive calls, but they're tail calls, so it's
>> +           okay. */
>> +        if ( released_write )
>> +            __release_grant_for_copy(td, trans_gref, 0);
>> +        else if ( released_read )
>> +            __release_grant_for_copy(td, trans_gref, 1);
>> +
>> +       rcu_unlock_domain(td);
>>     }
>>  }
>> 
>> @@ -1731,7 +1723,7 @@
>>     uint32_t old_pin;
>>     domid_t trans_domid;
>>     grant_ref_t trans_gref;
>> -    struct domain *rrd;
>> +    struct domain *td;
>>     unsigned long gfn;
>>     unsigned long grant_frame;
>>     unsigned trans_page_off;
>> @@ -1785,8 +1777,8 @@
>>                                status) ) != GNTST_okay )
>>              goto unlock_out;
>> 
>> -        trans_domid = ld->domain_id;
>> -        trans_gref = 0;
>> +        td = rd;
>> +        trans_gref = gref;
>>         if ( sha2 && (shah->flags & GTF_type_mask) == GTF_transitive )
>>         {
>>             if ( !allow_transitive )
>> @@ -1808,14 +1800,15 @@
>>                that you don't need to go out of your way to avoid it
>>                in the guest. */
>> 
>> -            rrd = rcu_lock_domain_by_id(trans_domid);
>> -            if ( rrd == NULL )
>> +            /* We need to leave the rrd locked during the grant copy */
>> +            td = rcu_lock_domain_by_id(trans_domid);
>> +            if ( td == NULL )
>>                 PIN_FAIL(unlock_out, GNTST_general_error,
>>                          "transitive grant referenced bad domain %d\n",
>>                          trans_domid);
>>             spin_unlock(&rd->grant_table->lock);
>> 
>> -            rc = __acquire_grant_for_copy(rrd, trans_gref, rd,
>> +            rc = __acquire_grant_for_copy(td, trans_gref, rd,
>>                                           readonly, &grant_frame,
>>                                           &trans_page_off, &trans_length,
>>                                           0, &ignore);
>> @@ -1823,6 +1816,7 @@
>>             spin_lock(&rd->grant_table->lock);
>>             if ( rc != GNTST_okay ) {
>>                 __fixup_status_for_pin(act, status);
>> +               rcu_unlock_domain(td);
>>                 spin_unlock(&rd->grant_table->lock);
>>                 return rc;
>>             }
>> @@ -1834,6 +1828,7 @@
>>             if ( act->pin != old_pin )
>>             {
>>                 __fixup_status_for_pin(act, status);
>> +               rcu_unlock_domain(td);
>>                 spin_unlock(&rd->grant_table->lock);
>>                 return __acquire_grant_for_copy(rd, gref, ld, readonly,
>>                                                 frame, page_off, length,
>> @@ -1845,7 +1840,7 @@
>>                sub-page, but we always treat it as one because that
>>                blocks mappings of transitive grants. */
>>             is_sub_page = 1;
>> -            *owning_domain = rrd;
>> +            *owning_domain = td;
>>             act->gfn = -1ul;
>>         }
>>         else if ( sha1 )
>> @@ -1891,7 +1886,7 @@
>>             act->is_sub_page = is_sub_page;
>>             act->start = trans_page_off;
>>             act->length = trans_length;
>> -            act->trans_dom = trans_domid;
>> +            act->trans_domain = td;
>>             act->trans_gref = trans_gref;
>>             act->frame = grant_frame;
>>         }
>> diff -r f071d8e9f744 -r 14211e98efac xen/include/xen/grant_table.h
>> --- a/xen/include/xen/grant_table.h     Tue Mar 08 10:23:52 2011 +0000
>> +++ b/xen/include/xen/grant_table.h     Tue Mar 08 14:39:03 2011 +0000
>> @@ -32,7 +32,7 @@
>>  struct active_grant_entry {
>>     u32           pin;    /* Reference count information.             */
>>     domid_t       domid;  /* Domain being granted access.             */
>> -    domid_t       trans_dom;
>> +    struct domain *trans_domain;
>>     uint32_t      trans_gref;
>>     unsigned long frame;  /* Frame being granted.                     */
>>     unsigned long gfn;    /* Guest's idea of the frame being granted. */
>> 
>> _______________________________________________
>> 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: [PATCH] Fix rcu domain locking for transitive grants

[George Dunlap]

On Tue, 2011-03-08 at 15:38 +0000, Keir Fraser wrote:
> On 08/03/2011 15:11, "George Dunlap" <George.Dunlap@eu.citrix.com> wrote:
> 
> > This should be backported to the 4.1 branch; it causes a hypervisor
> > BUG() if guests are using netchannel2 transtiive grants to talk to
> > each other when debug mode is on.
> 
> I stubbed out the preemption checking stuff in 4.1 branch (it's not really
> needed since there are no users of waitqueues in 4.1), so this patch is not
> required. And that's fortunate, since it's quite non-trivial.

Are the rcu locks still noops then?

 -George


> 
>  -- Keir
> 
> >  -George
> > 
> > On Tue, Mar 8, 2011 at 3:02 PM, George Dunlap
> > <george.dunlap@eu.citrix.com> wrote:
> >> When acquiring a transitive grant for copy then the owning domain needs to
> >> be locked down as well as the granting domain. This was being done, but the
> >> unlocking was not. The acquire code now stores the struct domain * of the
> >> owning domain (rather than the domid) in the active entry in the granting
> >> domain. The release code then does the unlock on the owning domain.
> >> Note that I believe I also fixed a bug where, for non-transitive grants
> >> the active entry contained a reference to the acquiring domain rather
> >> than the granting domain. From my reading of the code this would stop the
> >> release code for transitive grants from terminating its recursion
> >> correctly.
> >> 
> >> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> >> CC: Steven Smith <steven.smith@citrix.com>
> >> 
> >> diff -r f071d8e9f744 -r 14211e98efac xen/common/grant_table.c
> >> --- a/xen/common/grant_table.c  Tue Mar 08 10:23:52 2011 +0000
> >> +++ b/xen/common/grant_table.c  Tue Mar 08 14:39:03 2011 +0000
> >> @@ -1626,11 +1626,10 @@
> >>     struct active_grant_entry *act;
> >>     unsigned long r_frame;
> >>     uint16_t *status;
> >> -    domid_t trans_domid;
> >>     grant_ref_t trans_gref;
> >>     int released_read;
> >>     int released_write;
> >> -    struct domain *trans_dom;
> >> +    struct domain *td;
> >> 
> >>     released_read = 0;
> >>     released_write = 0;
> >> @@ -1644,15 +1643,13 @@
> >>     if (rd->grant_table->gt_version == 1)
> >>     {
> >>         status = &sha->flags;
> >> -        trans_domid = rd->domain_id;
> >> -        /* Shut the compiler up.  This'll never be used, because
> >> -           trans_domid == rd->domain_id, but gcc doesn't know that. */
> >> -        trans_gref = 0x1234567;
> >> +        td = rd;
> >> +        trans_gref = gref;
> >>     }
> >>     else
> >>     {
> >>         status = &status_entry(rd->grant_table, gref);
> >> -        trans_domid = act->trans_dom;
> >> +        td = act->trans_domain;
> >>         trans_gref = act->trans_gref;
> >>     }
> >> 
> >> @@ -1680,21 +1677,16 @@
> >> 
> >>     spin_unlock(&rd->grant_table->lock);
> >> 
> >> -    if ( trans_domid != rd->domain_id )
> >> +    if ( td != rd )
> >>     {
> >> -        if ( released_write || released_read )
> >> -        {
> >> -            trans_dom = rcu_lock_domain_by_id(trans_domid);
> >> -            if ( trans_dom != NULL )
> >> -            {
> >> -                /* Recursive calls, but they're tail calls, so it's
> >> -                   okay. */
> >> -                if ( released_write )
> >> -                    __release_grant_for_copy(trans_dom, trans_gref, 0);
> >> -                else if ( released_read )
> >> -                    __release_grant_for_copy(trans_dom, trans_gref, 1);
> >> -            }
> >> -        }
> >> +        /* Recursive calls, but they're tail calls, so it's
> >> +           okay. */
> >> +        if ( released_write )
> >> +            __release_grant_for_copy(td, trans_gref, 0);
> >> +        else if ( released_read )
> >> +            __release_grant_for_copy(td, trans_gref, 1);
> >> +
> >> +       rcu_unlock_domain(td);
> >>     }
> >>  }
> >> 
> >> @@ -1731,7 +1723,7 @@
> >>     uint32_t old_pin;
> >>     domid_t trans_domid;
> >>     grant_ref_t trans_gref;
> >> -    struct domain *rrd;
> >> +    struct domain *td;
> >>     unsigned long gfn;
> >>     unsigned long grant_frame;
> >>     unsigned trans_page_off;
> >> @@ -1785,8 +1777,8 @@
> >>                                status) ) != GNTST_okay )
> >>              goto unlock_out;
> >> 
> >> -        trans_domid = ld->domain_id;
> >> -        trans_gref = 0;
> >> +        td = rd;
> >> +        trans_gref = gref;
> >>         if ( sha2 && (shah->flags & GTF_type_mask) == GTF_transitive )
> >>         {
> >>             if ( !allow_transitive )
> >> @@ -1808,14 +1800,15 @@
> >>                that you don't need to go out of your way to avoid it
> >>                in the guest. */
> >> 
> >> -            rrd = rcu_lock_domain_by_id(trans_domid);
> >> -            if ( rrd == NULL )
> >> +            /* We need to leave the rrd locked during the grant copy */
> >> +            td = rcu_lock_domain_by_id(trans_domid);
> >> +            if ( td == NULL )
> >>                 PIN_FAIL(unlock_out, GNTST_general_error,
> >>                          "transitive grant referenced bad domain %d\n",
> >>                          trans_domid);
> >>             spin_unlock(&rd->grant_table->lock);
> >> 
> >> -            rc = __acquire_grant_for_copy(rrd, trans_gref, rd,
> >> +            rc = __acquire_grant_for_copy(td, trans_gref, rd,
> >>                                           readonly, &grant_frame,
> >>                                           &trans_page_off, &trans_length,
> >>                                           0, &ignore);
> >> @@ -1823,6 +1816,7 @@
> >>             spin_lock(&rd->grant_table->lock);
> >>             if ( rc != GNTST_okay ) {
> >>                 __fixup_status_for_pin(act, status);
> >> +               rcu_unlock_domain(td);
> >>                 spin_unlock(&rd->grant_table->lock);
> >>                 return rc;
> >>             }
> >> @@ -1834,6 +1828,7 @@
> >>             if ( act->pin != old_pin )
> >>             {
> >>                 __fixup_status_for_pin(act, status);
> >> +               rcu_unlock_domain(td);
> >>                 spin_unlock(&rd->grant_table->lock);
> >>                 return __acquire_grant_for_copy(rd, gref, ld, readonly,
> >>                                                 frame, page_off, length,
> >> @@ -1845,7 +1840,7 @@
> >>                sub-page, but we always treat it as one because that
> >>                blocks mappings of transitive grants. */
> >>             is_sub_page = 1;
> >> -            *owning_domain = rrd;
> >> +            *owning_domain = td;
> >>             act->gfn = -1ul;
> >>         }
> >>         else if ( sha1 )
> >> @@ -1891,7 +1886,7 @@
> >>             act->is_sub_page = is_sub_page;
> >>             act->start = trans_page_off;
> >>             act->length = trans_length;
> >> -            act->trans_dom = trans_domid;
> >> +            act->trans_domain = td;
> >>             act->trans_gref = trans_gref;
> >>             act->frame = grant_frame;
> >>         }
> >> diff -r f071d8e9f744 -r 14211e98efac xen/include/xen/grant_table.h
> >> --- a/xen/include/xen/grant_table.h     Tue Mar 08 10:23:52 2011 +0000
> >> +++ b/xen/include/xen/grant_table.h     Tue Mar 08 14:39:03 2011 +0000
> >> @@ -32,7 +32,7 @@
> >>  struct active_grant_entry {
> >>     u32           pin;    /* Reference count information.             */
> >>     domid_t       domid;  /* Domain being granted access.             */
> >> -    domid_t       trans_dom;
> >> +    struct domain *trans_domain;
> >>     uint32_t      trans_gref;
> >>     unsigned long frame;  /* Frame being granted.                     */
> >>     unsigned long gfn;    /* Guest's idea of the frame being granted. */
> >> 
> >> _______________________________________________
> >> 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: [PATCH] Fix rcu domain locking for transitive grants

[Keir Fraser]

On 08/03/2011 15:42, "George Dunlap" <george.dunlap@citrix.com> wrote:

> On Tue, 2011-03-08 at 15:38 +0000, Keir Fraser wrote:
>> On 08/03/2011 15:11, "George Dunlap" <George.Dunlap@eu.citrix.com> wrote:
>> 
>>> This should be backported to the 4.1 branch; it causes a hypervisor
>>> BUG() if guests are using netchannel2 transtiive grants to talk to
>>> each other when debug mode is on.
>> 
>> I stubbed out the preemption checking stuff in 4.1 branch (it's not really
>> needed since there are no users of waitqueues in 4.1), so this patch is not
>> required. And that's fortunate, since it's quite non-trivial.
> 
> Are the rcu locks still noops then?

Yes, since preempt_disable/enable are noops.

 -- Keir

>  -George
> 
> 
>> 
>>  -- Keir
>> 
>>>  -George
>>> 
>>> On Tue, Mar 8, 2011 at 3:02 PM, George Dunlap
>>> <george.dunlap@eu.citrix.com> wrote:
>>>> When acquiring a transitive grant for copy then the owning domain needs to
>>>> be locked down as well as the granting domain. This was being done, but the
>>>> unlocking was not. The acquire code now stores the struct domain * of the
>>>> owning domain (rather than the domid) in the active entry in the granting
>>>> domain. The release code then does the unlock on the owning domain.
>>>> Note that I believe I also fixed a bug where, for non-transitive grants
>>>> the active entry contained a reference to the acquiring domain rather
>>>> than the granting domain. From my reading of the code this would stop the
>>>> release code for transitive grants from terminating its recursion
>>>> correctly.
>>>> 
>>>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>>>> CC: Steven Smith <steven.smith@citrix.com>
>>>> 
>>>> diff -r f071d8e9f744 -r 14211e98efac xen/common/grant_table.c
>>>> --- a/xen/common/grant_table.c  Tue Mar 08 10:23:52 2011 +0000
>>>> +++ b/xen/common/grant_table.c  Tue Mar 08 14:39:03 2011 +0000
>>>> @@ -1626,11 +1626,10 @@
>>>>     struct active_grant_entry *act;
>>>>     unsigned long r_frame;
>>>>     uint16_t *status;
>>>> -    domid_t trans_domid;
>>>>     grant_ref_t trans_gref;
>>>>     int released_read;
>>>>     int released_write;
>>>> -    struct domain *trans_dom;
>>>> +    struct domain *td;
>>>> 
>>>>     released_read = 0;
>>>>     released_write = 0;
>>>> @@ -1644,15 +1643,13 @@
>>>>     if (rd->grant_table->gt_version == 1)
>>>>     {
>>>>         status = &sha->flags;
>>>> -        trans_domid = rd->domain_id;
>>>> -        /* Shut the compiler up.  This'll never be used, because
>>>> -           trans_domid == rd->domain_id, but gcc doesn't know that. */
>>>> -        trans_gref = 0x1234567;
>>>> +        td = rd;
>>>> +        trans_gref = gref;
>>>>     }
>>>>     else
>>>>     {
>>>>         status = &status_entry(rd->grant_table, gref);
>>>> -        trans_domid = act->trans_dom;
>>>> +        td = act->trans_domain;
>>>>         trans_gref = act->trans_gref;
>>>>     }
>>>> 
>>>> @@ -1680,21 +1677,16 @@
>>>> 
>>>>     spin_unlock(&rd->grant_table->lock);
>>>> 
>>>> -    if ( trans_domid != rd->domain_id )
>>>> +    if ( td != rd )
>>>>     {
>>>> -        if ( released_write || released_read )
>>>> -        {
>>>> -            trans_dom = rcu_lock_domain_by_id(trans_domid);
>>>> -            if ( trans_dom != NULL )
>>>> -            {
>>>> -                /* Recursive calls, but they're tail calls, so it's
>>>> -                   okay. */
>>>> -                if ( released_write )
>>>> -                    __release_grant_for_copy(trans_dom, trans_gref, 0);
>>>> -                else if ( released_read )
>>>> -                    __release_grant_for_copy(trans_dom, trans_gref, 1);
>>>> -            }
>>>> -        }
>>>> +        /* Recursive calls, but they're tail calls, so it's
>>>> +           okay. */
>>>> +        if ( released_write )
>>>> +            __release_grant_for_copy(td, trans_gref, 0);
>>>> +        else if ( released_read )
>>>> +            __release_grant_for_copy(td, trans_gref, 1);
>>>> +
>>>> +       rcu_unlock_domain(td);
>>>>     }
>>>>  }
>>>> 
>>>> @@ -1731,7 +1723,7 @@
>>>>     uint32_t old_pin;
>>>>     domid_t trans_domid;
>>>>     grant_ref_t trans_gref;
>>>> -    struct domain *rrd;
>>>> +    struct domain *td;
>>>>     unsigned long gfn;
>>>>     unsigned long grant_frame;
>>>>     unsigned trans_page_off;
>>>> @@ -1785,8 +1777,8 @@
>>>>                                status) ) != GNTST_okay )
>>>>              goto unlock_out;
>>>> 
>>>> -        trans_domid = ld->domain_id;
>>>> -        trans_gref = 0;
>>>> +        td = rd;
>>>> +        trans_gref = gref;
>>>>         if ( sha2 && (shah->flags & GTF_type_mask) == GTF_transitive )
>>>>         {
>>>>             if ( !allow_transitive )
>>>> @@ -1808,14 +1800,15 @@
>>>>                that you don't need to go out of your way to avoid it
>>>>                in the guest. */
>>>> 
>>>> -            rrd = rcu_lock_domain_by_id(trans_domid);
>>>> -            if ( rrd == NULL )
>>>> +            /* We need to leave the rrd locked during the grant copy */
>>>> +            td = rcu_lock_domain_by_id(trans_domid);
>>>> +            if ( td == NULL )
>>>>                 PIN_FAIL(unlock_out, GNTST_general_error,
>>>>                          "transitive grant referenced bad domain %d\n",
>>>>                          trans_domid);
>>>>             spin_unlock(&rd->grant_table->lock);
>>>> 
>>>> -            rc = __acquire_grant_for_copy(rrd, trans_gref, rd,
>>>> +            rc = __acquire_grant_for_copy(td, trans_gref, rd,
>>>>                                           readonly, &grant_frame,
>>>>                                           &trans_page_off, &trans_length,
>>>>                                           0, &ignore);
>>>> @@ -1823,6 +1816,7 @@
>>>>             spin_lock(&rd->grant_table->lock);
>>>>             if ( rc != GNTST_okay ) {
>>>>                 __fixup_status_for_pin(act, status);
>>>> +               rcu_unlock_domain(td);
>>>>                 spin_unlock(&rd->grant_table->lock);
>>>>                 return rc;
>>>>             }
>>>> @@ -1834,6 +1828,7 @@
>>>>             if ( act->pin != old_pin )
>>>>             {
>>>>                 __fixup_status_for_pin(act, status);
>>>> +               rcu_unlock_domain(td);
>>>>                 spin_unlock(&rd->grant_table->lock);
>>>>                 return __acquire_grant_for_copy(rd, gref, ld, readonly,
>>>>                                                 frame, page_off, length,
>>>> @@ -1845,7 +1840,7 @@
>>>>                sub-page, but we always treat it as one because that
>>>>                blocks mappings of transitive grants. */
>>>>             is_sub_page = 1;
>>>> -            *owning_domain = rrd;
>>>> +            *owning_domain = td;
>>>>             act->gfn = -1ul;
>>>>         }
>>>>         else if ( sha1 )
>>>> @@ -1891,7 +1886,7 @@
>>>>             act->is_sub_page = is_sub_page;
>>>>             act->start = trans_page_off;
>>>>             act->length = trans_length;
>>>> -            act->trans_dom = trans_domid;
>>>> +            act->trans_domain = td;
>>>>             act->trans_gref = trans_gref;
>>>>             act->frame = grant_frame;
>>>>         }
>>>> diff -r f071d8e9f744 -r 14211e98efac xen/include/xen/grant_table.h
>>>> --- a/xen/include/xen/grant_table.h     Tue Mar 08 10:23:52 2011 +0000
>>>> +++ b/xen/include/xen/grant_table.h     Tue Mar 08 14:39:03 2011 +0000
>>>> @@ -32,7 +32,7 @@
>>>>  struct active_grant_entry {
>>>>     u32           pin;    /* Reference count information.             */
>>>>     domid_t       domid;  /* Domain being granted access.             */
>>>> -    domid_t       trans_dom;
>>>> +    struct domain *trans_domain;
>>>>     uint32_t      trans_gref;
>>>>     unsigned long frame;  /* Frame being granted.                     */
>>>>     unsigned long gfn;    /* Guest's idea of the frame being granted. */
>>>> 
>>>> _______________________________________________
>>>> 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: [PATCH] Fix rcu domain locking for transitive grants

[Paul Durrant]

I still think this patch should stand. The locking around transitive grants is just borked and if someone actually does implement the rcu locks in future they will get a nasty surprise.

  Paul

> -----Original Message-----
> From: xen-devel-bounces@lists.xensource.com [mailto:xen-devel-
> bounces@lists.xensource.com] On Behalf Of Keir Fraser
> Sent: 08 March 2011 15:39
> To: George Dunlap; xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH] Fix rcu domain locking for
> transitive grants
> 
> On 08/03/2011 15:11, "George Dunlap" <George.Dunlap@eu.citrix.com>
> wrote:
> 
> > This should be backported to the 4.1 branch; it causes a
> hypervisor
> > BUG() if guests are using netchannel2 transtiive grants to talk to
> > each other when debug mode is on.
> 
> I stubbed out the preemption checking stuff in 4.1 branch (it's not
> really needed since there are no users of waitqueues in 4.1), so
> this patch is not required. And that's fortunate, since it's quite
> non-trivial.
> 
>  -- Keir
> 
> >  -George
> >
> > On Tue, Mar 8, 2011 at 3:02 PM, George Dunlap
> > <george.dunlap@eu.citrix.com> wrote:
> >> When acquiring a transitive grant for copy then the owning domain
> >> needs to be locked down as well as the granting domain. This was
> >> being done, but the unlocking was not. The acquire code now
> stores
> >> the struct domain * of the owning domain (rather than the domid)
> in
> >> the active entry in the granting domain. The release code then
> does the unlock on the owning domain.
> >> Note that I believe I also fixed a bug where, for non-transitive
> >> grants the active entry contained a reference to the acquiring
> domain
> >> rather than the granting domain. From my reading of the code this
> >> would stop the release code for transitive grants from
> terminating
> >> its recursion correctly.
> >>
> >> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> >> CC: Steven Smith <steven.smith@citrix.com>
> >>
> >> diff -r f071d8e9f744 -r 14211e98efac xen/common/grant_table.c
> >> --- a/xen/common/grant_table.c  Tue Mar 08 10:23:52 2011 +0000
> >> +++ b/xen/common/grant_table.c  Tue Mar 08 14:39:03 2011 +0000
> >> @@ -1626,11 +1626,10 @@
> >>     struct active_grant_entry *act;
> >>     unsigned long r_frame;
> >>     uint16_t *status;
> >> -    domid_t trans_domid;
> >>     grant_ref_t trans_gref;
> >>     int released_read;
> >>     int released_write;
> >> -    struct domain *trans_dom;
> >> +    struct domain *td;
> >>
> >>     released_read = 0;
> >>     released_write = 0;
> >> @@ -1644,15 +1643,13 @@
> >>     if (rd->grant_table->gt_version == 1)
> >>     {
> >>         status = &sha->flags;
> >> -        trans_domid = rd->domain_id;
> >> -        /* Shut the compiler up.  This'll never be used, because
> >> -           trans_domid == rd->domain_id, but gcc doesn't know
> that.
> >> */
> >> -        trans_gref = 0x1234567;
> >> +        td = rd;
> >> +        trans_gref = gref;
> >>     }
> >>     else
> >>     {
> >>         status = &status_entry(rd->grant_table, gref);
> >> -        trans_domid = act->trans_dom;
> >> +        td = act->trans_domain;
> >>         trans_gref = act->trans_gref;
> >>     }
> >>
> >> @@ -1680,21 +1677,16 @@
> >>
> >>     spin_unlock(&rd->grant_table->lock);
> >>
> >> -    if ( trans_domid != rd->domain_id )
> >> +    if ( td != rd )
> >>     {
> >> -        if ( released_write || released_read )
> >> -        {
> >> -            trans_dom = rcu_lock_domain_by_id(trans_domid);
> >> -            if ( trans_dom != NULL )
> >> -            {
> >> -                /* Recursive calls, but they're tail calls, so
> it's
> >> -                   okay. */
> >> -                if ( released_write )
> >> -                    __release_grant_for_copy(trans_dom,
> trans_gref,
> >> 0);
> >> -                else if ( released_read )
> >> -                    __release_grant_for_copy(trans_dom,
> trans_gref,
> >> 1);
> >> -            }
> >> -        }
> >> +        /* Recursive calls, but they're tail calls, so it's
> >> +           okay. */
> >> +        if ( released_write )
> >> +            __release_grant_for_copy(td, trans_gref, 0);
> >> +        else if ( released_read )
> >> +            __release_grant_for_copy(td, trans_gref, 1);
> >> +
> >> +       rcu_unlock_domain(td);
> >>     }
> >>  }
> >>
> >> @@ -1731,7 +1723,7 @@
> >>     uint32_t old_pin;
> >>     domid_t trans_domid;
> >>     grant_ref_t trans_gref;
> >> -    struct domain *rrd;
> >> +    struct domain *td;
> >>     unsigned long gfn;
> >>     unsigned long grant_frame;
> >>     unsigned trans_page_off;
> >> @@ -1785,8 +1777,8 @@
> >>                                status) ) != GNTST_okay )
> >>              goto unlock_out;
> >>
> >> -        trans_domid = ld->domain_id;
> >> -        trans_gref = 0;
> >> +        td = rd;
> >> +        trans_gref = gref;
> >>         if ( sha2 && (shah->flags & GTF_type_mask) ==
> GTF_transitive
> >> )
> >>         {
> >>             if ( !allow_transitive )
> >> @@ -1808,14 +1800,15 @@
> >>                that you don't need to go out of your way to avoid
> it
> >>                in the guest. */
> >>
> >> -            rrd = rcu_lock_domain_by_id(trans_domid);
> >> -            if ( rrd == NULL )
> >> +            /* We need to leave the rrd locked during the grant
> copy
> >> + */
> >> +            td = rcu_lock_domain_by_id(trans_domid);
> >> +            if ( td == NULL )
> >>                 PIN_FAIL(unlock_out, GNTST_general_error,
> >>                          "transitive grant referenced bad domain
> >> %d\n",
> >>                          trans_domid);
> >>             spin_unlock(&rd->grant_table->lock);
> >>
> >> -            rc = __acquire_grant_for_copy(rrd, trans_gref, rd,
> >> +            rc = __acquire_grant_for_copy(td, trans_gref, rd,
> >>                                           readonly, &grant_frame,
> >>                                           &trans_page_off,
> >> &trans_length,
> >>                                           0, &ignore); @@ -1823,6
> >> +1816,7 @@
> >>             spin_lock(&rd->grant_table->lock);
> >>             if ( rc != GNTST_okay ) {
> >>                 __fixup_status_for_pin(act, status);
> >> +               rcu_unlock_domain(td);
> >>                 spin_unlock(&rd->grant_table->lock);
> >>                 return rc;
> >>             }
> >> @@ -1834,6 +1828,7 @@
> >>             if ( act->pin != old_pin )
> >>             {
> >>                 __fixup_status_for_pin(act, status);
> >> +               rcu_unlock_domain(td);
> >>                 spin_unlock(&rd->grant_table->lock);
> >>                 return __acquire_grant_for_copy(rd, gref, ld,
> >> readonly,
> >>                                                 frame, page_off,
> >> length, @@ -1845,7 +1840,7 @@
> >>                sub-page, but we always treat it as one because
> that
> >>                blocks mappings of transitive grants. */
> >>             is_sub_page = 1;
> >> -            *owning_domain = rrd;
> >> +            *owning_domain = td;
> >>             act->gfn = -1ul;
> >>         }
> >>         else if ( sha1 )
> >> @@ -1891,7 +1886,7 @@
> >>             act->is_sub_page = is_sub_page;
> >>             act->start = trans_page_off;
> >>             act->length = trans_length;
> >> -            act->trans_dom = trans_domid;
> >> +            act->trans_domain = td;
> >>             act->trans_gref = trans_gref;
> >>             act->frame = grant_frame;
> >>         }
> >> diff -r f071d8e9f744 -r 14211e98efac
> xen/include/xen/grant_table.h
> >> --- a/xen/include/xen/grant_table.h     Tue Mar 08 10:23:52 2011
> >> +0000
> >> +++ b/xen/include/xen/grant_table.h     Tue Mar 08 14:39:03 2011
> >> +++ +0000
> >> @@ -32,7 +32,7 @@
> >>  struct active_grant_entry {
> >>     u32           pin;    /* Reference count
> information.
> >> */
> >>     domid_t       domid;  /* Domain being granted
> access.
> >> */
> >> -    domid_t       trans_dom;
> >> +    struct domain *trans_domain;
> >>     uint32_t      trans_gref;
> >>     unsigned long frame;  /* Frame being
> granted.
> >> */
> >>     unsigned long gfn;    /* Guest's idea of the frame being
> granted.
> >> */
> >>
> >> _______________________________________________
> >> 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
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] Fix rcu domain locking for transitive grants

[Keir Fraser]

On 08/03/2011 15:56, "Paul Durrant" <Paul.Durrant@citrix.com> wrote:

> I still think this patch should stand. The locking around transitive grants is
> just borked and if someone actually does implement the rcu locks in future
> they will get a nasty surprise.

It will stand, in xen-unstable.

 K.

>   Paul
> 
>> -----Original Message-----
>> From: xen-devel-bounces@lists.xensource.com [mailto:xen-devel-
>> bounces@lists.xensource.com] On Behalf Of Keir Fraser
>> Sent: 08 March 2011 15:39
>> To: George Dunlap; xen-devel@lists.xensource.com
>> Subject: Re: [Xen-devel] [PATCH] Fix rcu domain locking for
>> transitive grants
>> 
>> On 08/03/2011 15:11, "George Dunlap" <George.Dunlap@eu.citrix.com>
>> wrote:
>> 
>>> This should be backported to the 4.1 branch; it causes a
>> hypervisor
>>> BUG() if guests are using netchannel2 transtiive grants to talk to
>>> each other when debug mode is on.
>> 
>> I stubbed out the preemption checking stuff in 4.1 branch (it's not
>> really needed since there are no users of waitqueues in 4.1), so
>> this patch is not required. And that's fortunate, since it's quite
>> non-trivial.
>> 
>>  -- Keir
>> 
>>>  -George
>>> 
>>> On Tue, Mar 8, 2011 at 3:02 PM, George Dunlap
>>> <george.dunlap@eu.citrix.com> wrote:
>>>> When acquiring a transitive grant for copy then the owning domain
>>>> needs to be locked down as well as the granting domain. This was
>>>> being done, but the unlocking was not. The acquire code now
>> stores
>>>> the struct domain * of the owning domain (rather than the domid)
>> in
>>>> the active entry in the granting domain. The release code then
>> does the unlock on the owning domain.
>>>> Note that I believe I also fixed a bug where, for non-transitive
>>>> grants the active entry contained a reference to the acquiring
>> domain
>>>> rather than the granting domain. From my reading of the code this
>>>> would stop the release code for transitive grants from
>> terminating
>>>> its recursion correctly.
>>>> 
>>>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>>>> CC: Steven Smith <steven.smith@citrix.com>
>>>> 
>>>> diff -r f071d8e9f744 -r 14211e98efac xen/common/grant_table.c
>>>> --- a/xen/common/grant_table.c  Tue Mar 08 10:23:52 2011 +0000
>>>> +++ b/xen/common/grant_table.c  Tue Mar 08 14:39:03 2011 +0000
>>>> @@ -1626,11 +1626,10 @@
>>>>     struct active_grant_entry *act;
>>>>     unsigned long r_frame;
>>>>     uint16_t *status;
>>>> -    domid_t trans_domid;
>>>>     grant_ref_t trans_gref;
>>>>     int released_read;
>>>>     int released_write;
>>>> -    struct domain *trans_dom;
>>>> +    struct domain *td;
>>>> 
>>>>     released_read = 0;
>>>>     released_write = 0;
>>>> @@ -1644,15 +1643,13 @@
>>>>     if (rd->grant_table->gt_version == 1)
>>>>     {
>>>>         status = &sha->flags;
>>>> -        trans_domid = rd->domain_id;
>>>> -        /* Shut the compiler up.  This'll never be used, because
>>>> -           trans_domid == rd->domain_id, but gcc doesn't know
>> that.
>>>> */
>>>> -        trans_gref = 0x1234567;
>>>> +        td = rd;
>>>> +        trans_gref = gref;
>>>>     }
>>>>     else
>>>>     {
>>>>         status = &status_entry(rd->grant_table, gref);
>>>> -        trans_domid = act->trans_dom;
>>>> +        td = act->trans_domain;
>>>>         trans_gref = act->trans_gref;
>>>>     }
>>>> 
>>>> @@ -1680,21 +1677,16 @@
>>>> 
>>>>     spin_unlock(&rd->grant_table->lock);
>>>> 
>>>> -    if ( trans_domid != rd->domain_id )
>>>> +    if ( td != rd )
>>>>     {
>>>> -        if ( released_write || released_read )
>>>> -        {
>>>> -            trans_dom = rcu_lock_domain_by_id(trans_domid);
>>>> -            if ( trans_dom != NULL )
>>>> -            {
>>>> -                /* Recursive calls, but they're tail calls, so
>> it's
>>>> -                   okay. */
>>>> -                if ( released_write )
>>>> -                    __release_grant_for_copy(trans_dom,
>> trans_gref,
>>>> 0);
>>>> -                else if ( released_read )
>>>> -                    __release_grant_for_copy(trans_dom,
>> trans_gref,
>>>> 1);
>>>> -            }
>>>> -        }
>>>> +        /* Recursive calls, but they're tail calls, so it's
>>>> +           okay. */
>>>> +        if ( released_write )
>>>> +            __release_grant_for_copy(td, trans_gref, 0);
>>>> +        else if ( released_read )
>>>> +            __release_grant_for_copy(td, trans_gref, 1);
>>>> +
>>>> +       rcu_unlock_domain(td);
>>>>     }
>>>>  }
>>>> 
>>>> @@ -1731,7 +1723,7 @@
>>>>     uint32_t old_pin;
>>>>     domid_t trans_domid;
>>>>     grant_ref_t trans_gref;
>>>> -    struct domain *rrd;
>>>> +    struct domain *td;
>>>>     unsigned long gfn;
>>>>     unsigned long grant_frame;
>>>>     unsigned trans_page_off;
>>>> @@ -1785,8 +1777,8 @@
>>>>                                status) ) != GNTST_okay )
>>>>              goto unlock_out;
>>>> 
>>>> -        trans_domid = ld->domain_id;
>>>> -        trans_gref = 0;
>>>> +        td = rd;
>>>> +        trans_gref = gref;
>>>>         if ( sha2 && (shah->flags & GTF_type_mask) ==
>> GTF_transitive
>>>> )
>>>>         {
>>>>             if ( !allow_transitive )
>>>> @@ -1808,14 +1800,15 @@
>>>>                that you don't need to go out of your way to avoid
>> it
>>>>                in the guest. */
>>>> 
>>>> -            rrd = rcu_lock_domain_by_id(trans_domid);
>>>> -            if ( rrd == NULL )
>>>> +            /* We need to leave the rrd locked during the grant
>> copy
>>>> + */
>>>> +            td = rcu_lock_domain_by_id(trans_domid);
>>>> +            if ( td == NULL )
>>>>                 PIN_FAIL(unlock_out, GNTST_general_error,
>>>>                          "transitive grant referenced bad domain
>>>> %d\n",
>>>>                          trans_domid);
>>>>             spin_unlock(&rd->grant_table->lock);
>>>> 
>>>> -            rc = __acquire_grant_for_copy(rrd, trans_gref, rd,
>>>> +            rc = __acquire_grant_for_copy(td, trans_gref, rd,
>>>>                                           readonly, &grant_frame,
>>>>                                           &trans_page_off,
>>>> &trans_length,
>>>>                                           0, &ignore); @@ -1823,6
>>>> +1816,7 @@
>>>>             spin_lock(&rd->grant_table->lock);
>>>>             if ( rc != GNTST_okay ) {
>>>>                 __fixup_status_for_pin(act, status);
>>>> +               rcu_unlock_domain(td);
>>>>                 spin_unlock(&rd->grant_table->lock);
>>>>                 return rc;
>>>>             }
>>>> @@ -1834,6 +1828,7 @@
>>>>             if ( act->pin != old_pin )
>>>>             {
>>>>                 __fixup_status_for_pin(act, status);
>>>> +               rcu_unlock_domain(td);
>>>>                 spin_unlock(&rd->grant_table->lock);
>>>>                 return __acquire_grant_for_copy(rd, gref, ld,
>>>> readonly,
>>>>                                                 frame, page_off,
>>>> length, @@ -1845,7 +1840,7 @@
>>>>                sub-page, but we always treat it as one because
>> that
>>>>                blocks mappings of transitive grants. */
>>>>             is_sub_page = 1;
>>>> -            *owning_domain = rrd;
>>>> +            *owning_domain = td;
>>>>             act->gfn = -1ul;
>>>>         }
>>>>         else if ( sha1 )
>>>> @@ -1891,7 +1886,7 @@
>>>>             act->is_sub_page = is_sub_page;
>>>>             act->start = trans_page_off;
>>>>             act->length = trans_length;
>>>> -            act->trans_dom = trans_domid;
>>>> +            act->trans_domain = td;
>>>>             act->trans_gref = trans_gref;
>>>>             act->frame = grant_frame;
>>>>         }
>>>> diff -r f071d8e9f744 -r 14211e98efac
>> xen/include/xen/grant_table.h
>>>> --- a/xen/include/xen/grant_table.h     Tue Mar 08 10:23:52 2011
>>>> +0000
>>>> +++ b/xen/include/xen/grant_table.h     Tue Mar 08 14:39:03 2011
>>>> +++ +0000
>>>> @@ -32,7 +32,7 @@
>>>>  struct active_grant_entry {
>>>>     u32           pin;    /* Reference count
>> information.
>>>> */
>>>>     domid_t       domid;  /* Domain being granted
>> access.
>>>> */
>>>> -    domid_t       trans_dom;
>>>> +    struct domain *trans_domain;
>>>>     uint32_t      trans_gref;
>>>>     unsigned long frame;  /* Frame being
>> granted.
>>>> */
>>>>     unsigned long gfn;    /* Guest's idea of the frame being
>> granted.
>>>> */
>>>> 
>>>> _______________________________________________
>>>> 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
>> 
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel

RE: [PATCH] Fix rcu domain locking for transitive grants

[George Dunlap]

As I understood him, Keir was saying he wasn't going to apply it 4.1 --
at least not before the official release.  (Perhaps in 4.1.1?)

I don't really understand the logic of having locks which act as noops
at all -- I think no matter what, if someone decides to implement RCU,
they're in for a boatload of nasty surprises.  It seems to me like the
current locks will be little more than a hint to whatever future dev
does that work that this is an area that they might want to look at.
But it seems to me like they'll end up having to write the locks and
locking disciplines from scratch anyway.

 -George

On Tue, 2011-03-08 at 15:56 +0000, Paul Durrant wrote:
> I still think this patch should stand. The locking around transitive grants is just borked and if someone actually does implement the rcu locks in future they will get a nasty surprise.
> 
>   Paul
> 
> > -----Original Message-----
> > From: xen-devel-bounces@lists.xensource.com [mailto:xen-devel-
> > bounces@lists.xensource.com] On Behalf Of Keir Fraser
> > Sent: 08 March 2011 15:39
> > To: George Dunlap; xen-devel@lists.xensource.com
> > Subject: Re: [Xen-devel] [PATCH] Fix rcu domain locking for
> > transitive grants
> > 
> > On 08/03/2011 15:11, "George Dunlap" <George.Dunlap@eu.citrix.com>
> > wrote:
> > 
> > > This should be backported to the 4.1 branch; it causes a
> > hypervisor
> > > BUG() if guests are using netchannel2 transtiive grants to talk to
> > > each other when debug mode is on.
> > 
> > I stubbed out the preemption checking stuff in 4.1 branch (it's not
> > really needed since there are no users of waitqueues in 4.1), so
> > this patch is not required. And that's fortunate, since it's quite
> > non-trivial.
> > 
> >  -- Keir
> > 
> > >  -George
> > >
> > > On Tue, Mar 8, 2011 at 3:02 PM, George Dunlap
> > > <george.dunlap@eu.citrix.com> wrote:
> > >> When acquiring a transitive grant for copy then the owning domain
> > >> needs to be locked down as well as the granting domain. This was
> > >> being done, but the unlocking was not. The acquire code now
> > stores
> > >> the struct domain * of the owning domain (rather than the domid)
> > in
> > >> the active entry in the granting domain. The release code then
> > does the unlock on the owning domain.
> > >> Note that I believe I also fixed a bug where, for non-transitive
> > >> grants the active entry contained a reference to the acquiring
> > domain
> > >> rather than the granting domain. From my reading of the code this
> > >> would stop the release code for transitive grants from
> > terminating
> > >> its recursion correctly.
> > >>
> > >> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > >> CC: Steven Smith <steven.smith@citrix.com>
> > >>
> > >> diff -r f071d8e9f744 -r 14211e98efac xen/common/grant_table.c
> > >> --- a/xen/common/grant_table.c  Tue Mar 08 10:23:52 2011 +0000
> > >> +++ b/xen/common/grant_table.c  Tue Mar 08 14:39:03 2011 +0000
> > >> @@ -1626,11 +1626,10 @@
> > >>     struct active_grant_entry *act;
> > >>     unsigned long r_frame;
> > >>     uint16_t *status;
> > >> -    domid_t trans_domid;
> > >>     grant_ref_t trans_gref;
> > >>     int released_read;
> > >>     int released_write;
> > >> -    struct domain *trans_dom;
> > >> +    struct domain *td;
> > >>
> > >>     released_read = 0;
> > >>     released_write = 0;
> > >> @@ -1644,15 +1643,13 @@
> > >>     if (rd->grant_table->gt_version == 1)
> > >>     {
> > >>         status = &sha->flags;
> > >> -        trans_domid = rd->domain_id;
> > >> -        /* Shut the compiler up.  This'll never be used, because
> > >> -           trans_domid == rd->domain_id, but gcc doesn't know
> > that.
> > >> */
> > >> -        trans_gref = 0x1234567;
> > >> +        td = rd;
> > >> +        trans_gref = gref;
> > >>     }
> > >>     else
> > >>     {
> > >>         status = &status_entry(rd->grant_table, gref);
> > >> -        trans_domid = act->trans_dom;
> > >> +        td = act->trans_domain;
> > >>         trans_gref = act->trans_gref;
> > >>     }
> > >>
> > >> @@ -1680,21 +1677,16 @@
> > >>
> > >>     spin_unlock(&rd->grant_table->lock);
> > >>
> > >> -    if ( trans_domid != rd->domain_id )
> > >> +    if ( td != rd )
> > >>     {
> > >> -        if ( released_write || released_read )
> > >> -        {
> > >> -            trans_dom = rcu_lock_domain_by_id(trans_domid);
> > >> -            if ( trans_dom != NULL )
> > >> -            {
> > >> -                /* Recursive calls, but they're tail calls, so
> > it's
> > >> -                   okay. */
> > >> -                if ( released_write )
> > >> -                    __release_grant_for_copy(trans_dom,
> > trans_gref,
> > >> 0);
> > >> -                else if ( released_read )
> > >> -                    __release_grant_for_copy(trans_dom,
> > trans_gref,
> > >> 1);
> > >> -            }
> > >> -        }
> > >> +        /* Recursive calls, but they're tail calls, so it's
> > >> +           okay. */
> > >> +        if ( released_write )
> > >> +            __release_grant_for_copy(td, trans_gref, 0);
> > >> +        else if ( released_read )
> > >> +            __release_grant_for_copy(td, trans_gref, 1);
> > >> +
> > >> +       rcu_unlock_domain(td);
> > >>     }
> > >>  }
> > >>
> > >> @@ -1731,7 +1723,7 @@
> > >>     uint32_t old_pin;
> > >>     domid_t trans_domid;
> > >>     grant_ref_t trans_gref;
> > >> -    struct domain *rrd;
> > >> +    struct domain *td;
> > >>     unsigned long gfn;
> > >>     unsigned long grant_frame;
> > >>     unsigned trans_page_off;
> > >> @@ -1785,8 +1777,8 @@
> > >>                                status) ) != GNTST_okay )
> > >>              goto unlock_out;
> > >>
> > >> -        trans_domid = ld->domain_id;
> > >> -        trans_gref = 0;
> > >> +        td = rd;
> > >> +        trans_gref = gref;
> > >>         if ( sha2 && (shah->flags & GTF_type_mask) ==
> > GTF_transitive
> > >> )
> > >>         {
> > >>             if ( !allow_transitive )
> > >> @@ -1808,14 +1800,15 @@
> > >>                that you don't need to go out of your way to avoid
> > it
> > >>                in the guest. */
> > >>
> > >> -            rrd = rcu_lock_domain_by_id(trans_domid);
> > >> -            if ( rrd == NULL )
> > >> +            /* We need to leave the rrd locked during the grant
> > copy
> > >> + */
> > >> +            td = rcu_lock_domain_by_id(trans_domid);
> > >> +            if ( td == NULL )
> > >>                 PIN_FAIL(unlock_out, GNTST_general_error,
> > >>                          "transitive grant referenced bad domain
> > >> %d\n",
> > >>                          trans_domid);
> > >>             spin_unlock(&rd->grant_table->lock);
> > >>
> > >> -            rc = __acquire_grant_for_copy(rrd, trans_gref, rd,
> > >> +            rc = __acquire_grant_for_copy(td, trans_gref, rd,
> > >>                                           readonly, &grant_frame,
> > >>                                           &trans_page_off,
> > >> &trans_length,
> > >>                                           0, &ignore); @@ -1823,6
> > >> +1816,7 @@
> > >>             spin_lock(&rd->grant_table->lock);
> > >>             if ( rc != GNTST_okay ) {
> > >>                 __fixup_status_for_pin(act, status);
> > >> +               rcu_unlock_domain(td);
> > >>                 spin_unlock(&rd->grant_table->lock);
> > >>                 return rc;
> > >>             }
> > >> @@ -1834,6 +1828,7 @@
> > >>             if ( act->pin != old_pin )
> > >>             {
> > >>                 __fixup_status_for_pin(act, status);
> > >> +               rcu_unlock_domain(td);
> > >>                 spin_unlock(&rd->grant_table->lock);
> > >>                 return __acquire_grant_for_copy(rd, gref, ld,
> > >> readonly,
> > >>                                                 frame, page_off,
> > >> length, @@ -1845,7 +1840,7 @@
> > >>                sub-page, but we always treat it as one because
> > that
> > >>                blocks mappings of transitive grants. */
> > >>             is_sub_page = 1;
> > >> -            *owning_domain = rrd;
> > >> +            *owning_domain = td;
> > >>             act->gfn = -1ul;
> > >>         }
> > >>         else if ( sha1 )
> > >> @@ -1891,7 +1886,7 @@
> > >>             act->is_sub_page = is_sub_page;
> > >>             act->start = trans_page_off;
> > >>             act->length = trans_length;
> > >> -            act->trans_dom = trans_domid;
> > >> +            act->trans_domain = td;
> > >>             act->trans_gref = trans_gref;
> > >>             act->frame = grant_frame;
> > >>         }
> > >> diff -r f071d8e9f744 -r 14211e98efac
> > xen/include/xen/grant_table.h
> > >> --- a/xen/include/xen/grant_table.h     Tue Mar 08 10:23:52 2011
> > >> +0000
> > >> +++ b/xen/include/xen/grant_table.h     Tue Mar 08 14:39:03 2011
> > >> +++ +0000
> > >> @@ -32,7 +32,7 @@
> > >>  struct active_grant_entry {
> > >>     u32           pin;    /* Reference count
> > information.
> > >> */
> > >>     domid_t       domid;  /* Domain being granted
> > access.
> > >> */
> > >> -    domid_t       trans_dom;
> > >> +    struct domain *trans_domain;
> > >>     uint32_t      trans_gref;
> > >>     unsigned long frame;  /* Frame being
> > granted.
> > >> */
> > >>     unsigned long gfn;    /* Guest's idea of the frame being
> > granted.
> > >> */
> > >>
> > >> _______________________________________________
> > >> 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
> > 
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel

Re: [PATCH] Fix rcu domain locking for transitive grants

[Keir Fraser]

On 08/03/2011 16:08, "George Dunlap" <george.dunlap@citrix.com> wrote:

> As I understood him, Keir was saying he wasn't going to apply it 4.1 --
> at least not before the official release.  (Perhaps in 4.1.1?)
> 
> I don't really understand the logic of having locks which act as noops
> at all -- I think no matter what, if someone decides to implement RCU,
> they're in for a boatload of nasty surprises.  It seems to me like the
> current locks will be little more than a hint to whatever future dev
> does that work that this is an area that they might want to look at.
> But it seems to me like they'll end up having to write the locks and
> locking disciplines from scratch anyway.

There is no doubt that it has been a lot easier to fix up a few bugs in the
existing no-op RCU read-lock usage than retrofit RCU read locks from
scratch, for the waitqueue work.

 -- Keir

>  -George
> 
> On Tue, 2011-03-08 at 15:56 +0000, Paul Durrant wrote:
>> I still think this patch should stand. The locking around transitive grants
>> is just borked and if someone actually does implement the rcu locks in future
>> they will get a nasty surprise.
>> 
>>   Paul
>> 
>>> -----Original Message-----
>>> From: xen-devel-bounces@lists.xensource.com [mailto:xen-devel-
>>> bounces@lists.xensource.com] On Behalf Of Keir Fraser
>>> Sent: 08 March 2011 15:39
>>> To: George Dunlap; xen-devel@lists.xensource.com
>>> Subject: Re: [Xen-devel] [PATCH] Fix rcu domain locking for
>>> transitive grants
>>> 
>>> On 08/03/2011 15:11, "George Dunlap" <George.Dunlap@eu.citrix.com>
>>> wrote:
>>> 
>>>> This should be backported to the 4.1 branch; it causes a
>>> hypervisor
>>>> BUG() if guests are using netchannel2 transtiive grants to talk to
>>>> each other when debug mode is on.
>>> 
>>> I stubbed out the preemption checking stuff in 4.1 branch (it's not
>>> really needed since there are no users of waitqueues in 4.1), so
>>> this patch is not required. And that's fortunate, since it's quite
>>> non-trivial.
>>> 
>>>  -- Keir
>>> 
>>>>  -George
>>>> 
>>>> On Tue, Mar 8, 2011 at 3:02 PM, George Dunlap
>>>> <george.dunlap@eu.citrix.com> wrote:
>>>>> When acquiring a transitive grant for copy then the owning domain
>>>>> needs to be locked down as well as the granting domain. This was
>>>>> being done, but the unlocking was not. The acquire code now
>>> stores
>>>>> the struct domain * of the owning domain (rather than the domid)
>>> in
>>>>> the active entry in the granting domain. The release code then
>>> does the unlock on the owning domain.
>>>>> Note that I believe I also fixed a bug where, for non-transitive
>>>>> grants the active entry contained a reference to the acquiring
>>> domain
>>>>> rather than the granting domain. From my reading of the code this
>>>>> would stop the release code for transitive grants from
>>> terminating
>>>>> its recursion correctly.
>>>>> 
>>>>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>>>>> CC: Steven Smith <steven.smith@citrix.com>
>>>>> 
>>>>> diff -r f071d8e9f744 -r 14211e98efac xen/common/grant_table.c
>>>>> --- a/xen/common/grant_table.c  Tue Mar 08 10:23:52 2011 +0000
>>>>> +++ b/xen/common/grant_table.c  Tue Mar 08 14:39:03 2011 +0000
>>>>> @@ -1626,11 +1626,10 @@
>>>>>     struct active_grant_entry *act;
>>>>>     unsigned long r_frame;
>>>>>     uint16_t *status;
>>>>> -    domid_t trans_domid;
>>>>>     grant_ref_t trans_gref;
>>>>>     int released_read;
>>>>>     int released_write;
>>>>> -    struct domain *trans_dom;
>>>>> +    struct domain *td;
>>>>> 
>>>>>     released_read = 0;
>>>>>     released_write = 0;
>>>>> @@ -1644,15 +1643,13 @@
>>>>>     if (rd->grant_table->gt_version == 1)
>>>>>     {
>>>>>         status = &sha->flags;
>>>>> -        trans_domid = rd->domain_id;
>>>>> -        /* Shut the compiler up.  This'll never be used, because
>>>>> -           trans_domid == rd->domain_id, but gcc doesn't know
>>> that.
>>>>> */
>>>>> -        trans_gref = 0x1234567;
>>>>> +        td = rd;
>>>>> +        trans_gref = gref;
>>>>>     }
>>>>>     else
>>>>>     {
>>>>>         status = &status_entry(rd->grant_table, gref);
>>>>> -        trans_domid = act->trans_dom;
>>>>> +        td = act->trans_domain;
>>>>>         trans_gref = act->trans_gref;
>>>>>     }
>>>>> 
>>>>> @@ -1680,21 +1677,16 @@
>>>>> 
>>>>>     spin_unlock(&rd->grant_table->lock);
>>>>> 
>>>>> -    if ( trans_domid != rd->domain_id )
>>>>> +    if ( td != rd )
>>>>>     {
>>>>> -        if ( released_write || released_read )
>>>>> -        {
>>>>> -            trans_dom = rcu_lock_domain_by_id(trans_domid);
>>>>> -            if ( trans_dom != NULL )
>>>>> -            {
>>>>> -                /* Recursive calls, but they're tail calls, so
>>> it's
>>>>> -                   okay. */
>>>>> -                if ( released_write )
>>>>> -                    __release_grant_for_copy(trans_dom,
>>> trans_gref,
>>>>> 0);
>>>>> -                else if ( released_read )
>>>>> -                    __release_grant_for_copy(trans_dom,
>>> trans_gref,
>>>>> 1);
>>>>> -            }
>>>>> -        }
>>>>> +        /* Recursive calls, but they're tail calls, so it's
>>>>> +           okay. */
>>>>> +        if ( released_write )
>>>>> +            __release_grant_for_copy(td, trans_gref, 0);
>>>>> +        else if ( released_read )
>>>>> +            __release_grant_for_copy(td, trans_gref, 1);
>>>>> +
>>>>> +       rcu_unlock_domain(td);
>>>>>     }
>>>>>  }
>>>>> 
>>>>> @@ -1731,7 +1723,7 @@
>>>>>     uint32_t old_pin;
>>>>>     domid_t trans_domid;
>>>>>     grant_ref_t trans_gref;
>>>>> -    struct domain *rrd;
>>>>> +    struct domain *td;
>>>>>     unsigned long gfn;
>>>>>     unsigned long grant_frame;
>>>>>     unsigned trans_page_off;
>>>>> @@ -1785,8 +1777,8 @@
>>>>>                                status) ) != GNTST_okay )
>>>>>              goto unlock_out;
>>>>> 
>>>>> -        trans_domid = ld->domain_id;
>>>>> -        trans_gref = 0;
>>>>> +        td = rd;
>>>>> +        trans_gref = gref;
>>>>>         if ( sha2 && (shah->flags & GTF_type_mask) ==
>>> GTF_transitive
>>>>> )
>>>>>         {
>>>>>             if ( !allow_transitive )
>>>>> @@ -1808,14 +1800,15 @@
>>>>>                that you don't need to go out of your way to avoid
>>> it
>>>>>                in the guest. */
>>>>> 
>>>>> -            rrd = rcu_lock_domain_by_id(trans_domid);
>>>>> -            if ( rrd == NULL )
>>>>> +            /* We need to leave the rrd locked during the grant
>>> copy
>>>>> + */
>>>>> +            td = rcu_lock_domain_by_id(trans_domid);
>>>>> +            if ( td == NULL )
>>>>>                 PIN_FAIL(unlock_out, GNTST_general_error,
>>>>>                          "transitive grant referenced bad domain
>>>>> %d\n",
>>>>>                          trans_domid);
>>>>>             spin_unlock(&rd->grant_table->lock);
>>>>> 
>>>>> -            rc = __acquire_grant_for_copy(rrd, trans_gref, rd,
>>>>> +            rc = __acquire_grant_for_copy(td, trans_gref, rd,
>>>>>                                           readonly, &grant_frame,
>>>>>                                           &trans_page_off,
>>>>> &trans_length,
>>>>>                                           0, &ignore); @@ -1823,6
>>>>> +1816,7 @@
>>>>>             spin_lock(&rd->grant_table->lock);
>>>>>             if ( rc != GNTST_okay ) {
>>>>>                 __fixup_status_for_pin(act, status);
>>>>> +               rcu_unlock_domain(td);
>>>>>                 spin_unlock(&rd->grant_table->lock);
>>>>>                 return rc;
>>>>>             }
>>>>> @@ -1834,6 +1828,7 @@
>>>>>             if ( act->pin != old_pin )
>>>>>             {
>>>>>                 __fixup_status_for_pin(act, status);
>>>>> +               rcu_unlock_domain(td);
>>>>>                 spin_unlock(&rd->grant_table->lock);
>>>>>                 return __acquire_grant_for_copy(rd, gref, ld,
>>>>> readonly,
>>>>>                                                 frame, page_off,
>>>>> length, @@ -1845,7 +1840,7 @@
>>>>>                sub-page, but we always treat it as one because
>>> that
>>>>>                blocks mappings of transitive grants. */
>>>>>             is_sub_page = 1;
>>>>> -            *owning_domain = rrd;
>>>>> +            *owning_domain = td;
>>>>>             act->gfn = -1ul;
>>>>>         }
>>>>>         else if ( sha1 )
>>>>> @@ -1891,7 +1886,7 @@
>>>>>             act->is_sub_page = is_sub_page;
>>>>>             act->start = trans_page_off;
>>>>>             act->length = trans_length;
>>>>> -            act->trans_dom = trans_domid;
>>>>> +            act->trans_domain = td;
>>>>>             act->trans_gref = trans_gref;
>>>>>             act->frame = grant_frame;
>>>>>         }
>>>>> diff -r f071d8e9f744 -r 14211e98efac
>>> xen/include/xen/grant_table.h
>>>>> --- a/xen/include/xen/grant_table.h     Tue Mar 08 10:23:52 2011
>>>>> +0000
>>>>> +++ b/xen/include/xen/grant_table.h     Tue Mar 08 14:39:03 2011
>>>>> +++ +0000
>>>>> @@ -32,7 +32,7 @@
>>>>>  struct active_grant_entry {
>>>>>     u32           pin;    /* Reference count
>>> information.
>>>>> */
>>>>>     domid_t       domid;  /* Domain being granted
>>> access.
>>>>> */
>>>>> -    domid_t       trans_dom;
>>>>> +    struct domain *trans_domain;
>>>>>     uint32_t      trans_gref;
>>>>>     unsigned long frame;  /* Frame being
>>> granted.
>>>>> */
>>>>>     unsigned long gfn;    /* Guest's idea of the frame being
>>> granted.
>>>>> */
>>>>> 
>>>>> _______________________________________________
>>>>> 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
>>> 
>>> 
>>> 
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xensource.com
>>> http://lists.xensource.com/xen-devel
> 
>