[2/3] gnttab: consolidate pin-to-status syncing

Commit Message

Jan Beulich Jan. 14, 2021, 3:23 p.m. UTC

Forever since the fix for XSA-230 the 2nd of the comments ahead of
fixup_status_for_copy_pin() has been stale - there's nothing specific to
transitive grants there anymore.

Move the function up, drop the "copy" part from its name again, add a
"readonly" parameter, and use it also on other paths having decremented
one (or not having got to increment any) of the pin counts.

Signed-off-by: Jan Beulich <[e-mail]>

Comments

Andrew Cooper Jan. 15, 2021, 1:25 p.m. UTC | #1

On 14/01/2021 15:23, Jan Beulich wrote:
> Forever since the fix for XSA-230 the 2nd of the comments ahead of
> fixup_status_for_copy_pin() has been stale - there's nothing specific to
> transitive grants there anymore.
>
> Move the function up, drop the "copy" part from its name again, add a
> "readonly" parameter, and use it also on other paths having decremented
> one (or not having got to increment any) of the pin counts.
>
> Signed-off-by: Jan Beulich <[e-mail]>
>
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -908,6 +908,25 @@ static int _set_status(const grant_entry
>          return _set_status_v2(shah, status, rd, act, readonly, mapflag, ldomid);
>  }
>  
> +/*
> + * The status for a grant may indicate that we're taking more access than
> + * the pin requires.  Fix up the status to match the pin.

This sentence isn't correct.  It will only clear status flags to match a
reduction in the pinned references.  IOW it cannot be used in the case
that a grant goes from unpinned to pinned.

How about renaming to clear_status_for_pin() to make this clearer?  I
don't think it is worth trying to make the operation more generic.

If so (and with a suitable adjustment to the comment), Reviewed-by:
Andrew Cooper <[e-mail]>

Jan Beulich Jan. 15, 2021, 1:33 p.m. UTC | #2

On 15.01.2021 14:25, Andrew Cooper wrote:
> On 14/01/2021 15:23, Jan Beulich wrote:
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -908,6 +908,25 @@ static int _set_status(const grant_entry
>>          return _set_status_v2(shah, status, rd, act, readonly, mapflag, ldomid);
>>  }
>>  
>> +/*
>> + * The status for a grant may indicate that we're taking more access than
>> + * the pin requires.  Fix up the status to match the pin.
> 
> This sentence isn't correct.  It will only clear status flags to match a
> reduction in the pinned references.  IOW it cannot be used in the case
> that a grant goes from unpinned to pinned.

Of course not, hence "... more access than ...". But yes, I can
replace "Fix up" by "Reduce" if you think the wording isn't
unambiguous enough.

> How about renaming to clear_status_for_pin() to make this clearer?  I
> don't think it is worth trying to make the operation more generic.

Hmm, this name would suggest to me that the function clears
whatever the present pin count requires (e.g. acting on the
pre-update value when the post-update one is [going to be]
zero). Maybe reduce_status_for_pin(), matching the suggested
comment wording change?

> If so (and with a suitable adjustment to the comment), Reviewed-by:
> Andrew Cooper <[e-mail]>

As per above, I'll first wait for your further reply.

Jan

Andrew Cooper Jan. 15, 2021, 1:37 p.m. UTC | #3

On 15/01/2021 13:33, Jan Beulich wrote:
> On 15.01.2021 14:25, Andrew Cooper wrote:
>> On 14/01/2021 15:23, Jan Beulich wrote:
>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -908,6 +908,25 @@ static int _set_status(const grant_entry
>>>          return _set_status_v2(shah, status, rd, act, readonly, mapflag, ldomid);
>>>  }
>>>  
>>> +/*
>>> + * The status for a grant may indicate that we're taking more access than
>>> + * the pin requires.  Fix up the status to match the pin.
>> This sentence isn't correct.  It will only clear status flags to match a
>> reduction in the pinned references.  IOW it cannot be used in the case
>> that a grant goes from unpinned to pinned.
> Of course not, hence "... more access than ...". But yes, I can
> replace "Fix up" by "Reduce" if you think the wording isn't
> unambiguous enough.
>
>> How about renaming to clear_status_for_pin() to make this clearer?  I
>> don't think it is worth trying to make the operation more generic.
> Hmm, this name would suggest to me that the function clears
> whatever the present pin count requires (e.g. acting on the
> pre-update value when the post-update one is [going to be]
> zero). Maybe reduce_status_for_pin(), matching the suggested
> comment wording change?

Sounds good.  My R-by stands with this change.

~Andrew

Patch

--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -908,6 +908,25 @@  static int _set_status(const grant_entry
         return _set_status_v2(shah, status, rd, act, readonly, mapflag, ldomid);
 }
 
+/*
+ * The status for a grant may indicate that we're taking more access than
+ * the pin requires.  Fix up the status to match the pin.  Called with the
+ * domain's grant table lock held at least in read mode and with the active
+ * entry lock held (iow act->pin can't change behind our backs).
+ */
+static void fixup_status_for_pin(struct domain *rd,
+                                 const struct active_grant_entry *act,
+                                 uint16_t *status, bool readonly)
+{
+    unsigned int clear_flags = act->pin ? 0 : GTF_reading;
+
+    if ( !readonly && !(act->pin & (GNTPIN_hstw_mask | GNTPIN_devw_mask)) )
+        clear_flags |= GTF_writing;
+
+    if ( clear_flags )
+        gnttab_clear_flags(rd, clear_flags, status);
+}
+
 static struct active_grant_entry *grant_map_exists(const struct domain *ld,
                                                    struct grant_table *rgt,
                                                    mfn_t mfn,
@@ -991,8 +1010,7 @@  map_grant_ref(
     mfn_t mfn;
     struct page_info *pg = NULL;
     int            rc = GNTST_okay;
-    unsigned int   cache_flags, clear_flags = 0, refcnt = 0, typecnt = 0,
-                   pin_incr = 0;
+    unsigned int   cache_flags, refcnt = 0, typecnt = 0, pin_incr = 0;
     bool           host_map_created = false;
     struct active_grant_entry *act = NULL;
     struct grant_mapping *mt;
@@ -1284,15 +1302,7 @@  map_grant_ref(
     act->pin -= pin_incr;
 
  unlock_out_clear:
-    if ( !(op->flags & GNTMAP_readonly) &&
-         !(act->pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
-        clear_flags |= GTF_writing;
-
-    if ( !act->pin )
-        clear_flags |= GTF_reading;
-
-    if ( clear_flags )
-        gnttab_clear_flags(rd, clear_flags, status);
+    fixup_status_for_pin(rd, act, status, op->flags & GNTMAP_readonly);
 
  act_release_out:
     active_entry_release(act);
@@ -1507,7 +1517,6 @@  unmap_common_complete(struct gnttab_unma
     grant_entry_header_t *sha;
     struct page_info *pg;
     uint16_t *status;
-    unsigned int clear_flags = 0;
 
     if ( evaluate_nospec(!op->done) )
     {
@@ -1566,15 +1575,7 @@  unmap_common_complete(struct gnttab_unma
             act->pin -= GNTPIN_hstw_inc;
     }
 
-    if ( ((act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0) &&
-         !(op->done & GNTMAP_readonly) )
-        clear_flags |= GTF_writing;
-
-    if ( act->pin == 0 )
-        clear_flags |= GTF_reading;
-
-    if ( clear_flags )
-        gnttab_clear_flags(rd, clear_flags, status);
+    fixup_status_for_pin(rd, act, status, op->done & GNTMAP_readonly);
 
     active_entry_release(act);
     grant_read_unlock(rgt);
@@ -2414,7 +2415,6 @@  release_grant_for_copy(
     uint16_t *status;
     grant_ref_t trans_gref;
     struct domain *td;
-    unsigned int clear_flags = 0;
 
     grant_read_lock(rgt);
 
@@ -2444,15 +2444,9 @@  release_grant_for_copy(
         gnttab_mark_dirty(rd, mfn);
 
         act->pin -= GNTPIN_hstw_inc;
-        if ( !(act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) )
-            clear_flags |= GTF_writing;
     }
 
-    if ( !act->pin )
-        clear_flags |= GTF_reading;
-
-    if ( clear_flags )
-        gnttab_clear_flags(rd, clear_flags, status);
+    fixup_status_for_pin(rd, act, status, readonly);
 
     active_entry_release(act);
     grant_read_unlock(rgt);
@@ -2469,27 +2463,6 @@  release_grant_for_copy(
     }
 }
 
-/* The status for a grant indicates that we're taking more access than
-   the pin requires.  Fix up the status to match the pin.  Called
-   under the domain's grant table lock. */
-/* Only safe on transitive grants.  Even then, note that we don't
-   attempt to drop any pin on the referent grant. */
-static void fixup_status_for_copy_pin(struct domain *rd,
-                                      const struct active_grant_entry *act,
-                                      uint16_t *status)
-{
-    unsigned int clear_flags = 0;
-
-    if ( !(act->pin & (GNTPIN_hstw_mask | GNTPIN_devw_mask)) )
-        clear_flags |= GTF_writing;
-
-    if ( !act->pin )
-        clear_flags |= GTF_reading;
-
-    if ( clear_flags )
-        gnttab_clear_flags(rd, clear_flags, status);
-}
-
 /*
  * Grab a machine frame number from a grant entry and update the flags
  * and pin count as appropriate. If rc == GNTST_okay, note that this *does*
@@ -2517,7 +2490,6 @@  acquire_grant_for_copy(
     bool is_sub_page;
     s16 rc = GNTST_okay;
     unsigned int pin_incr = readonly ? GNTPIN_hstr_inc : GNTPIN_hstw_inc;
-    unsigned int clear_flags = 0;
 
     *page = NULL;
 
@@ -2604,8 +2576,8 @@  acquire_grant_for_copy(
 
         if ( rc != GNTST_okay )
         {
-            fixup_status_for_copy_pin(rd, act, status);
             rcu_unlock_domain(td);
+            fixup_status_for_pin(rd, act, status, readonly);
             active_entry_release(act);
             grant_read_unlock(rgt);
             return rc;
@@ -2627,8 +2599,8 @@  acquire_grant_for_copy(
                           !act->is_sub_page)) )
         {
             release_grant_for_copy(td, trans_gref, readonly);
-            fixup_status_for_copy_pin(rd, act, status);
             rcu_unlock_domain(td);
+            fixup_status_for_pin(rd, act, status, readonly);
             active_entry_release(act);
             grant_read_unlock(rgt);
             put_page(*page);
@@ -2742,15 +2714,7 @@  acquire_grant_for_copy(
     return rc;
 
  unlock_out_clear:
-    if ( !(readonly) &&
-         !(act->pin & (GNTPIN_hstw_mask | GNTPIN_devw_mask)) )
-        clear_flags |= GTF_writing;
-
-    if ( !act->pin )
-        clear_flags |= GTF_reading;
-
-    if ( clear_flags )
-        gnttab_clear_flags(rd, clear_flags, status);
+    fixup_status_for_pin(rd, act, status, readonly);
 
  unlock_out:
     active_entry_release(act);
@@ -3720,8 +3684,6 @@  gnttab_release_mappings(
 
     for ( handle = 0; handle < gt->maptrack_limit; handle++ )
     {
-        unsigned int clear_flags = 0;
-
         map = &maptrack_entry(gt, handle);
         if ( !(map->flags & (GNTMAP_device_map|GNTMAP_host_map)) )
             continue;
@@ -3794,16 +3756,9 @@  gnttab_release_mappings(
                     put_page(pg);
                 }
             }
-
-            if ( (act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0 )
-                clear_flags |= GTF_writing;
         }
 
-        if ( act->pin == 0 )
-            clear_flags |= GTF_reading;
-
-        if ( clear_flags )
-            gnttab_clear_flags(rd, clear_flags, status);
+        fixup_status_for_pin(rd, act, status, map->flags & GNTMAP_readonly);
 
         active_entry_release(act);
         grant_read_unlock(rgt);