Re: [Xen-devel] [PATCH 4/5] xen/gnttab: Refactor gnttab_clear_flag() to be gnttab_clear_flags()

Re: [Xen-devel] [PATCH 4/5] xen/gnttab: Refactor gnttab_clear_flag() to be gnttab_clear_flags()

[Jan Beulich]

>>> On 21.06.19 at 11:36, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/include/asm-x86/grant_table.h
> +++ b/xen/include/asm-x86/grant_table.h
> @@ -60,14 +60,11 @@ static inline int replace_grant_host_mapping(uint64_t 
> addr, mfn_t frame,
>  
>  #define gnttab_mark_dirty(d, f) paging_mark_dirty(d, f)
>  
> -static inline void gnttab_clear_flag(struct domain *d, unsigned int nr,
> -                                     uint16_t *st)
> +static inline void gnttab_clear_flags(struct domain *d,
> +                                      uint16_t mask, uint16_t *addr)
>  {
> -    /*
> -     * Note that this cannot be clear_bit(), as the access must be
> -     * confined to the specified 2 bytes.
> -     */
> -    asm volatile ("lock btrw %w1,%0" : "+m" (*st) : "Ir" (nr));
> +    /* Access must be confined to the specified 2 bytes. */
> +    asm volatile ("lock andw %w1,%0" : "+m" (*addr) : "Ir" (~mask));

The I constraint limits literal numbers to 5 bits, which is not what
you want here (albeit it's fine for the two bits we currently want
this for). "N" would widen the range to 8 bits, while "e" and "i"
would go beyond the 16 we want. Therefore I think you want
to cast ~mask to uint16_t and use "e" or "i". (This would then
also allow dropping the w modifier.)

Speaking of uint16_t - the "mask" parameter could easily be
unsigned int, possibly allowing the compiler to avoid a 66 prefix
here and there.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 0/5] xen/gnttab: XSA-295 followup

[Andrew Cooper]

This series came out of observations during the development of XSA-295.

It has been tested on x86, but only compile tested on arm.  It can be obtained
in git form from:

  http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/xen-grant-status

Andrew Cooper (5):
  xen/gnttab: Reduce complexity when reading grant_entry_header_t
  xen/gnttab: Reduce code volume when using union grant_combo
  arm/gnttab: Implement stub helpers as static inlines
  xen/gnttab: Refactor gnttab_clear_flag() to be gnttab_clear_flags()
  xen/gnttab: Fold adjacent calls to gnttab_clear_flags()

 xen/arch/arm/mm.c                 |  16 ----
 xen/common/grant_table.c          | 160 ++++++++++++++++++++------------------
 xen/include/asm-arm/grant_table.h |  17 +++-
 xen/include/asm-x86/grant_table.h |  11 +--
 4 files changed, 104 insertions(+), 100 deletions(-)

-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 4/5] xen/gnttab: Refactor gnttab_clear_flag() to be gnttab_clear_flags()

[Andrew Cooper]

To allow for further improvements, it is useful to be able to clear more than
a single flag at once.  Rework gnttab_clear_flag() into gnttab_clear_flags()
which takes a bitmask rather than a bit number.

No practical change yet.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/common/grant_table.c          | 30 +++++++++++++++---------------
 xen/include/asm-arm/grant_table.h |  6 +++---
 xen/include/asm-x86/grant_table.h | 11 ++++-------
 3 files changed, 22 insertions(+), 25 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 6d8f17d..4bd5777 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -796,8 +796,8 @@ static int _set_status_v2(const grant_entry_header_t *shah,
              (scombo.domid != ldomid) ||
              (!readonly && (scombo.flags & GTF_readonly)) )
         {
-            gnttab_clear_flag(rd, _GTF_writing, status);
-            gnttab_clear_flag(rd, _GTF_reading, status);
+            gnttab_clear_flags(rd, GTF_writing, status);
+            gnttab_clear_flags(rd, GTF_reading, status);
             PIN_FAIL(done, GNTST_general_error,
                      "Unstable flags (%x) or dom (%d); expected d%d (r/w: %d)\n",
                      scombo.flags, scombo.domid, ldomid, !readonly);
@@ -807,7 +807,7 @@ static int _set_status_v2(const grant_entry_header_t *shah,
     {
         if ( unlikely(scombo.flags & GTF_readonly) )
         {
-            gnttab_clear_flag(rd, _GTF_writing, status);
+            gnttab_clear_flags(rd, GTF_writing, status);
             PIN_FAIL(done, GNTST_general_error,
                      "Unstable grant readonly flag\n");
         }
@@ -1220,10 +1220,10 @@ map_grant_ref(
  unlock_out_clear:
     if ( !(op->flags & GNTMAP_readonly) &&
          !(act->pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
-        gnttab_clear_flag(rd, _GTF_writing, status);
+        gnttab_clear_flags(rd, GTF_writing, status);
 
     if ( !act->pin )
-        gnttab_clear_flag(rd, _GTF_reading, status);
+        gnttab_clear_flags(rd, GTF_reading, status);
 
  act_release_out:
     active_entry_release(act);
@@ -1493,10 +1493,10 @@ unmap_common_complete(struct gnttab_unmap_common *op)
 
     if ( ((act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0) &&
          !(op->done & GNTMAP_readonly) )
-        gnttab_clear_flag(rd, _GTF_writing, status);
+        gnttab_clear_flags(rd, GTF_writing, status);
 
     if ( act->pin == 0 )
-        gnttab_clear_flag(rd, _GTF_reading, status);
+        gnttab_clear_flags(rd, GTF_reading, status);
 
     active_entry_release(act);
     grant_read_unlock(rgt);
@@ -2354,11 +2354,11 @@ release_grant_for_copy(
 
         act->pin -= GNTPIN_hstw_inc;
         if ( !(act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) )
-            gnttab_clear_flag(rd, _GTF_writing, status);
+            gnttab_clear_flags(rd, GTF_writing, status);
     }
 
     if ( !act->pin )
-        gnttab_clear_flag(rd, _GTF_reading, status);
+        gnttab_clear_flags(rd, GTF_reading, status);
 
     active_entry_release(act);
     grant_read_unlock(rgt);
@@ -2385,10 +2385,10 @@ static void fixup_status_for_copy_pin(struct domain *rd,
                                       uint16_t *status)
 {
     if ( !(act->pin & (GNTPIN_hstw_mask | GNTPIN_devw_mask)) )
-        gnttab_clear_flag(rd, _GTF_writing, status);
+        gnttab_clear_flags(rd, GTF_writing, status);
 
     if ( !act->pin )
-        gnttab_clear_flag(rd, _GTF_reading, status);
+        gnttab_clear_flags(rd, GTF_reading, status);
 }
 
 /*
@@ -2639,10 +2639,10 @@ acquire_grant_for_copy(
  unlock_out_clear:
     if ( !(readonly) &&
          !(act->pin & (GNTPIN_hstw_mask | GNTPIN_devw_mask)) )
-        gnttab_clear_flag(rd, _GTF_writing, status);
+        gnttab_clear_flags(rd, GTF_writing, status);
 
     if ( !act->pin )
-        gnttab_clear_flag(rd, _GTF_reading, status);
+        gnttab_clear_flags(rd, GTF_reading, status);
 
  unlock_out:
     active_entry_release(act);
@@ -3677,11 +3677,11 @@ gnttab_release_mappings(
             }
 
             if ( (act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0 )
-                gnttab_clear_flag(rd, _GTF_writing, status);
+                gnttab_clear_flags(rd, GTF_writing, status);
         }
 
         if ( act->pin == 0 )
-            gnttab_clear_flag(rd, _GTF_reading, status);
+            gnttab_clear_flags(rd, GTF_reading, status);
 
         active_entry_release(act);
         grant_read_unlock(rgt);
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index b0d673b..8ccad69 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -16,10 +16,10 @@ struct grant_table_arch {
     gfn_t *status_gfn;
 };
 
-static inline void gnttab_clear_flag(struct domain *d,
-                                     unsigned long nr, uint16_t *addr)
+static inline void gnttab_clear_flags(struct domain *d,
+                                      uint16_t mask, uint16_t *addr)
 {
-    guest_clear_mask16(d, BIT(nr, UL), addr);
+    guest_clear_mask16(d, mask, addr);
 }
 
 static inline void gnttab_mark_dirty(struct domain *d, mfn_t mfn)
diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm-x86/grant_table.h
index 121b33d..4691258 100644
--- a/xen/include/asm-x86/grant_table.h
+++ b/xen/include/asm-x86/grant_table.h
@@ -60,14 +60,11 @@ static inline int replace_grant_host_mapping(uint64_t addr, mfn_t frame,
 
 #define gnttab_mark_dirty(d, f) paging_mark_dirty(d, f)
 
-static inline void gnttab_clear_flag(struct domain *d, unsigned int nr,
-                                     uint16_t *st)
+static inline void gnttab_clear_flags(struct domain *d,
+                                      uint16_t mask, uint16_t *addr)
 {
-    /*
-     * Note that this cannot be clear_bit(), as the access must be
-     * confined to the specified 2 bytes.
-     */
-    asm volatile ("lock btrw %w1,%0" : "+m" (*st) : "Ir" (nr));
+    /* Access must be confined to the specified 2 bytes. */
+    asm volatile ("lock andw %w1,%0" : "+m" (*addr) : "Ir" (~mask));
 }
 
 /* Foreign mappings of HVM-guest pages do not modify the type count. */
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel