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

[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 1/5] xen/gnttab: Reduce complexity when reading grant_entry_header_t

[Andrew Cooper]

_set_status_v{1,2}() and gnttab_prepare_for_transfer() read the shared header
by always casting to u32.  Despite grant_entry_header_t only having an
alignment of 2, this is actually safe because of the grant table ABI itself.

Switch to using an explicit uint32_t *, which removes all subsequent casting.

Furthermore, switch to using ACCESS_ONCE() for the read.  There is nothing in
the _set_status_v1() and gnttab_prepare_for_transfer() which prevents the
compiler from issuing multiple memory reads and creating a TOCTOU race around
the sanity checks, although the worst that can happen is Xen stamping a status
flag over a bad grant entry if the guest is misbehaving.

_set_status_v2() does use barrier() to try avoid multiple reads, but this is
overkill.  All that matters is that the shared header gets read in one go, and
this allows the compiler more room to optimise.

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>

Dropping the barrier() in _set_status_v2() allows the optimiser to notice that
initialiser for flags/id are redundant memory reads.  This is fixed in the
following patch.
---
 xen/common/grant_table.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 2bbde5c..e5d585f 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -679,6 +679,7 @@ static int _set_status_v1(const grant_entry_header_t *shah,
                           domid_t  ldomid)
 {
     int rc = GNTST_okay;
+    uint32_t *raw_shah = (uint32_t *)shah;
     union grant_combo scombo, prev_scombo, new_scombo;
     uint16_t mask = GTF_type_mask;
 
@@ -697,7 +698,7 @@ static int _set_status_v1(const grant_entry_header_t *shah,
     if ( mapflag )
         mask |= GTF_sub_page;
 
-    scombo.word = *(u32 *)shah;
+    scombo.word = ACCESS_ONCE(*raw_shah);
 
     /*
      * This loop attempts to set the access (reading/writing) flags
@@ -728,7 +729,7 @@ static int _set_status_v1(const grant_entry_header_t *shah,
                          "Attempt to write-pin a r/o grant entry\n");
         }
 
-        prev_scombo.word = guest_cmpxchg(rd, (u32 *)shah,
+        prev_scombo.word = guest_cmpxchg(rd, raw_shah,
                                          scombo.word, new_scombo.word);
         if ( likely(prev_scombo.word == scombo.word) )
             break;
@@ -753,17 +754,13 @@ static int _set_status_v2(const grant_entry_header_t *shah,
                           domid_t  ldomid)
 {
     int      rc    = GNTST_okay;
+    uint32_t *raw_shah = (uint32_t *)shah;
     union grant_combo scombo;
     uint16_t flags = shah->flags;
     domid_t  id    = shah->domid;
     uint16_t mask  = GTF_type_mask;
 
-    /* we read flags and domid in a single memory access.
-       this avoids the need for another memory barrier to
-       ensure access to these fields are not reordered */
-    scombo.word = *(u32 *)shah;
-    barrier(); /* but we still need to stop the compiler from turning
-                  it back into two reads */
+    scombo.word = ACCESS_ONCE(*raw_shah);
     flags = scombo.shorts.flags;
     id = scombo.shorts.domid;
 
@@ -797,8 +794,7 @@ static int _set_status_v2(const grant_entry_header_t *shah,
        still valid */
     smp_mb();
 
-    scombo.word = *(u32 *)shah;
-    barrier();
+    scombo.word = ACCESS_ONCE(*raw_shah);
     flags = scombo.shorts.flags;
     id = scombo.shorts.domid;
 
@@ -2041,7 +2037,7 @@ gnttab_prepare_for_transfer(
     struct domain *rd, struct domain *ld, grant_ref_t ref)
 {
     struct grant_table *rgt = rd->grant_table;
-    grant_entry_header_t *sha;
+    uint32_t *raw_shah;
     union grant_combo   scombo, prev_scombo, new_scombo;
     int                 retries = 0;
 
@@ -2055,9 +2051,8 @@ gnttab_prepare_for_transfer(
         goto fail;
     }
 
-    sha = shared_entry_header(rgt, ref);
-
-    scombo.word = *(u32 *)&sha->flags;
+    raw_shah = (uint32_t *)shared_entry_header(rgt, ref);
+    scombo.word = ACCESS_ONCE(*raw_shah);
 
     for ( ; ; )
     {
@@ -2074,7 +2069,7 @@ gnttab_prepare_for_transfer(
         new_scombo = scombo;
         new_scombo.shorts.flags |= GTF_transfer_committed;
 
-        prev_scombo.word = guest_cmpxchg(rd, (u32 *)&sha->flags,
+        prev_scombo.word = guest_cmpxchg(rd, raw_shah,
                                          scombo.word, new_scombo.word);
         if ( likely(prev_scombo.word == scombo.word) )
             break;
-- 
2.1.4


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

[Xen-devel] [PATCH 2/5] xen/gnttab: Reduce code volume when using union grant_combo

[Andrew Cooper]

There is no need for 'struct { ... } shorts' to be named.  Convert it to being
an anonymous struct, and rename 'word' to the more common 'raw'.

For _set_status_v1() and gnttab_prepare_for_transfer() which use a bounded
cmpxchg loop, rename {prev,new}_scombo to {prev,new} and reduce their scope to
within the loop.

For _set_status_v2(), the flags and id variables are completely unnecessary.
Drop them.

No functional change.

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 | 91 ++++++++++++++++++++++--------------------------
 1 file changed, 41 insertions(+), 50 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index e5d585f..6d8f17d 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -144,11 +144,11 @@ custom_param("gnttab", parse_gnttab);
  * The following union allows that to happen in an endian-neutral fashion.
  */
 union grant_combo {
-    uint32_t word;
+    uint32_t raw;
     struct {
         uint16_t flags;
         domid_t  domid;
-    } shorts;
+    };
 };
 
 /* Used to share code between unmap_grant_ref and unmap_and_replace. */
@@ -680,7 +680,7 @@ static int _set_status_v1(const grant_entry_header_t *shah,
 {
     int rc = GNTST_okay;
     uint32_t *raw_shah = (uint32_t *)shah;
-    union grant_combo scombo, prev_scombo, new_scombo;
+    union grant_combo scombo;
     uint16_t mask = GTF_type_mask;
 
     /*
@@ -698,7 +698,7 @@ static int _set_status_v1(const grant_entry_header_t *shah,
     if ( mapflag )
         mask |= GTF_sub_page;
 
-    scombo.word = ACCESS_ONCE(*raw_shah);
+    scombo.raw = ACCESS_ONCE(*raw_shah);
 
     /*
      * This loop attempts to set the access (reading/writing) flags
@@ -708,37 +708,35 @@ static int _set_status_v1(const grant_entry_header_t *shah,
      */
     for ( ; ; )
     {
+        union grant_combo prev, new;
+
         /* If not already pinned, check the grant domid and type. */
-        if ( !act->pin &&
-             (((scombo.shorts.flags & mask) !=
-               GTF_permit_access) ||
-              (scombo.shorts.domid != ldomid)) )
+        if ( !act->pin && (((scombo.flags & mask) != GTF_permit_access) ||
+                           (scombo.domid != ldomid)) )
             PIN_FAIL(done, GNTST_general_error,
                      "Bad flags (%x) or dom (%d); expected d%d\n",
-                     scombo.shorts.flags, scombo.shorts.domid,
-                     ldomid);
+                     scombo.flags, scombo.domid, ldomid);
 
-        new_scombo = scombo;
-        new_scombo.shorts.flags |= GTF_reading;
+        new = scombo;
+        new.flags |= GTF_reading;
 
         if ( !readonly )
         {
-            new_scombo.shorts.flags |= GTF_writing;
-            if ( unlikely(scombo.shorts.flags & GTF_readonly) )
+            new.flags |= GTF_writing;
+            if ( unlikely(scombo.flags & GTF_readonly) )
                 PIN_FAIL(done, GNTST_general_error,
                          "Attempt to write-pin a r/o grant entry\n");
         }
 
-        prev_scombo.word = guest_cmpxchg(rd, raw_shah,
-                                         scombo.word, new_scombo.word);
-        if ( likely(prev_scombo.word == scombo.word) )
+        prev.raw = guest_cmpxchg(rd, raw_shah, scombo.raw, new.raw);
+        if ( likely(prev.raw == scombo.raw) )
             break;
 
         if ( retries++ == 4 )
             PIN_FAIL(done, GNTST_general_error,
                      "Shared grant entry is unstable\n");
 
-        scombo = prev_scombo;
+        scombo = prev;
     }
 
 done:
@@ -756,13 +754,9 @@ static int _set_status_v2(const grant_entry_header_t *shah,
     int      rc    = GNTST_okay;
     uint32_t *raw_shah = (uint32_t *)shah;
     union grant_combo scombo;
-    uint16_t flags = shah->flags;
-    domid_t  id    = shah->domid;
     uint16_t mask  = GTF_type_mask;
 
-    scombo.word = ACCESS_ONCE(*raw_shah);
-    flags = scombo.shorts.flags;
-    id = scombo.shorts.domid;
+    scombo.raw = ACCESS_ONCE(*raw_shah);
 
     /* if this is a grant mapping operation we should ensure GTF_sub_page
        is not set */
@@ -770,13 +764,12 @@ static int _set_status_v2(const grant_entry_header_t *shah,
         mask |= GTF_sub_page;
 
     /* If not already pinned, check the grant domid and type. */
-    if ( !act->pin &&
-         ( (((flags & mask) != GTF_permit_access) &&
-            ((flags & mask) != GTF_transitive)) ||
-          (id != ldomid)) )
+    if ( !act->pin && ((((scombo.flags & mask) != GTF_permit_access) &&
+                        ((scombo.flags & mask) != GTF_transitive)) ||
+                       (scombo.domid != ldomid)) )
         PIN_FAIL(done, GNTST_general_error,
                  "Bad flags (%x) or dom (%d); expected d%d, flags %x\n",
-                 flags, id, ldomid, mask);
+                 scombo.flags, scombo.domid, ldomid, mask);
 
     if ( readonly )
     {
@@ -784,7 +777,7 @@ static int _set_status_v2(const grant_entry_header_t *shah,
     }
     else
     {
-        if ( unlikely(flags & GTF_readonly) )
+        if ( unlikely(scombo.flags & GTF_readonly) )
             PIN_FAIL(done, GNTST_general_error,
                      "Attempt to write-pin a r/o grant entry\n");
         *status |= GTF_reading | GTF_writing;
@@ -794,27 +787,25 @@ static int _set_status_v2(const grant_entry_header_t *shah,
        still valid */
     smp_mb();
 
-    scombo.word = ACCESS_ONCE(*raw_shah);
-    flags = scombo.shorts.flags;
-    id = scombo.shorts.domid;
+    scombo.raw = ACCESS_ONCE(*raw_shah);
 
     if ( !act->pin )
     {
-        if ( (((flags & mask) != GTF_permit_access) &&
-              ((flags & mask) != GTF_transitive)) ||
-             (id != ldomid) ||
-             (!readonly && (flags & GTF_readonly)) )
+        if ( (((scombo.flags & mask) != GTF_permit_access) &&
+              ((scombo.flags & mask) != GTF_transitive)) ||
+             (scombo.domid != ldomid) ||
+             (!readonly && (scombo.flags & GTF_readonly)) )
         {
             gnttab_clear_flag(rd, _GTF_writing, status);
             gnttab_clear_flag(rd, _GTF_reading, status);
             PIN_FAIL(done, GNTST_general_error,
                      "Unstable flags (%x) or dom (%d); expected d%d (r/w: %d)\n",
-                     flags, id, ldomid, !readonly);
+                     scombo.flags, scombo.domid, ldomid, !readonly);
         }
     }
     else
     {
-        if ( unlikely(flags & GTF_readonly) )
+        if ( unlikely(scombo.flags & GTF_readonly) )
         {
             gnttab_clear_flag(rd, _GTF_writing, status);
             PIN_FAIL(done, GNTST_general_error,
@@ -2038,7 +2029,7 @@ gnttab_prepare_for_transfer(
 {
     struct grant_table *rgt = rd->grant_table;
     uint32_t *raw_shah;
-    union grant_combo   scombo, prev_scombo, new_scombo;
+    union grant_combo scombo;
     int                 retries = 0;
 
     grant_read_lock(rgt);
@@ -2052,26 +2043,26 @@ gnttab_prepare_for_transfer(
     }
 
     raw_shah = (uint32_t *)shared_entry_header(rgt, ref);
-    scombo.word = ACCESS_ONCE(*raw_shah);
+    scombo.raw = ACCESS_ONCE(*raw_shah);
 
     for ( ; ; )
     {
-        if ( unlikely(scombo.shorts.flags != GTF_accept_transfer) ||
-             unlikely(scombo.shorts.domid != ld->domain_id) )
+        union grant_combo prev, new;
+
+        if ( unlikely(scombo.flags != GTF_accept_transfer) ||
+             unlikely(scombo.domid != ld->domain_id) )
         {
             gdprintk(XENLOG_INFO,
                      "Bad flags (%x) or dom (%d); expected d%d\n",
-                     scombo.shorts.flags, scombo.shorts.domid,
-                     ld->domain_id);
+                     scombo.flags, scombo.domid, ld->domain_id);
             goto fail;
         }
 
-        new_scombo = scombo;
-        new_scombo.shorts.flags |= GTF_transfer_committed;
+        new = scombo;
+        new.flags |= GTF_transfer_committed;
 
-        prev_scombo.word = guest_cmpxchg(rd, raw_shah,
-                                         scombo.word, new_scombo.word);
-        if ( likely(prev_scombo.word == scombo.word) )
+        prev.raw = guest_cmpxchg(rd, raw_shah, scombo.raw, new.raw);
+        if ( likely(prev.raw == scombo.raw) )
             break;
 
         if ( retries++ == 4 )
@@ -2080,7 +2071,7 @@ gnttab_prepare_for_transfer(
             goto fail;
         }
 
-        scombo = prev_scombo;
+        scombo = prev;
     }
 
     grant_read_unlock(rgt);
-- 
2.1.4


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

[Xen-devel] [PATCH 3/5] arm/gnttab: Implement stub helpers as static inlines

[Andrew Cooper]

It is inefficient to call into a different translation unit for a stub
function, when a static inline will work fine.  Replace an open-coded
printk_once() while moving it.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c                 | 16 ----------------
 xen/include/asm-arm/grant_table.h | 17 +++++++++++++++--
 2 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 35dc1f7..44258ad 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -41,7 +41,6 @@
 #include <xen/sizes.h>
 #include <xen/libfdt/libfdt.h>
 
-#include <asm/guest_atomics.h>
 #include <asm/setup.h>
 
 /* Override macros from asm/page.h to make them work with mfn_t */
@@ -1532,21 +1531,6 @@ void put_page_type(struct page_info *page)
     return;
 }
 
-void gnttab_clear_flag(struct domain *d, unsigned long nr, uint16_t *addr)
-{
-    guest_clear_mask16(d, BIT(nr, UL), addr);
-}
-
-void gnttab_mark_dirty(struct domain *d, mfn_t mfn)
-{
-    /* XXX: mark dirty */
-    static int warning;
-    if (!warning) {
-        gdprintk(XENLOG_WARNING, "gnttab_mark_dirty not implemented yet\n");
-        warning = 1;
-    }
-}
-
 int create_grant_host_mapping(unsigned long addr, mfn_t frame,
                               unsigned int flags, unsigned int cache_flags)
 {
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index 1ed0aef..b0d673b 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -6,6 +6,8 @@
 #include <xen/pfn.h>
 #include <xen/sched.h>
 
+#include <asm/guest_atomics.h>
+
 #define INITIAL_NR_GRANT_FRAMES 1U
 #define GNTTAB_MAX_VERSION 1
 
@@ -14,13 +16,24 @@ struct grant_table_arch {
     gfn_t *status_gfn;
 };
 
-void gnttab_clear_flag(struct domain *d, unsigned long nr, uint16_t *addr);
+static inline void gnttab_clear_flag(struct domain *d,
+                                     unsigned long nr, uint16_t *addr)
+{
+    guest_clear_mask16(d, BIT(nr, UL), addr);
+}
+
+static inline void gnttab_mark_dirty(struct domain *d, mfn_t mfn)
+{
+#ifndef NDEBUG
+    printk_once(XENLOG_G_WARNING "gnttab_mark_dirty not implemented yet\n");
+#endif
+}
+
 int create_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
                               unsigned int flags, unsigned int cache_flags);
 #define gnttab_host_mapping_get_page_type(ro, ld, rd) (0)
 int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
                                unsigned long new_gpaddr, unsigned int flags);
-void gnttab_mark_dirty(struct domain *d, mfn_t mfn);
 #define gnttab_release_host_mappings(domain) 1
 
 /*
-- 
2.1.4


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

[Xen-devel] Ping: [PATCH 3/5] arm/gnttab: Implement stub helpers as static inlines

[Andrew Cooper]

On 21/06/2019 10:36, Andrew Cooper wrote:
> It is inefficient to call into a different translation unit for a stub
> function, when a static inline will work fine.  Replace an open-coded
> printk_once() while moving it.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/mm.c                 | 16 ----------------
>  xen/include/asm-arm/grant_table.h | 17 +++++++++++++++--
>  2 files changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 35dc1f7..44258ad 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -41,7 +41,6 @@
>  #include <xen/sizes.h>
>  #include <xen/libfdt/libfdt.h>
>  
> -#include <asm/guest_atomics.h>
>  #include <asm/setup.h>
>  
>  /* Override macros from asm/page.h to make them work with mfn_t */
> @@ -1532,21 +1531,6 @@ void put_page_type(struct page_info *page)
>      return;
>  }
>  
> -void gnttab_clear_flag(struct domain *d, unsigned long nr, uint16_t *addr)
> -{
> -    guest_clear_mask16(d, BIT(nr, UL), addr);
> -}
> -
> -void gnttab_mark_dirty(struct domain *d, mfn_t mfn)
> -{
> -    /* XXX: mark dirty */
> -    static int warning;
> -    if (!warning) {
> -        gdprintk(XENLOG_WARNING, "gnttab_mark_dirty not implemented yet\n");
> -        warning = 1;
> -    }
> -}
> -
>  int create_grant_host_mapping(unsigned long addr, mfn_t frame,
>                                unsigned int flags, unsigned int cache_flags)
>  {
> diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
> index 1ed0aef..b0d673b 100644
> --- a/xen/include/asm-arm/grant_table.h
> +++ b/xen/include/asm-arm/grant_table.h
> @@ -6,6 +6,8 @@
>  #include <xen/pfn.h>
>  #include <xen/sched.h>
>  
> +#include <asm/guest_atomics.h>
> +
>  #define INITIAL_NR_GRANT_FRAMES 1U
>  #define GNTTAB_MAX_VERSION 1
>  
> @@ -14,13 +16,24 @@ struct grant_table_arch {
>      gfn_t *status_gfn;
>  };
>  
> -void gnttab_clear_flag(struct domain *d, unsigned long nr, uint16_t *addr);
> +static inline void gnttab_clear_flag(struct domain *d,
> +                                     unsigned long nr, uint16_t *addr)
> +{
> +    guest_clear_mask16(d, BIT(nr, UL), addr);
> +}
> +
> +static inline void gnttab_mark_dirty(struct domain *d, mfn_t mfn)
> +{
> +#ifndef NDEBUG
> +    printk_once(XENLOG_G_WARNING "gnttab_mark_dirty not implemented yet\n");
> +#endif
> +}
> +
>  int create_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>                                unsigned int flags, unsigned int cache_flags);
>  #define gnttab_host_mapping_get_page_type(ro, ld, rd) (0)
>  int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>                                 unsigned long new_gpaddr, unsigned int flags);
> -void gnttab_mark_dirty(struct domain *d, mfn_t mfn);
>  #define gnttab_release_host_mappings(domain) 1
>  
>  /*


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

Re: [Xen-devel] [PATCH 3/5] arm/gnttab: Implement stub helpers as static inlines

[Julien Grall]

Hi Andrew,

On 6/21/19 10:36 AM, Andrew Cooper wrote:
> It is inefficient to call into a different translation unit for a stub
> function, when a static inline will work fine.  Replace an open-coded
> printk_once() while moving it.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>   xen/arch/arm/mm.c                 | 16 ----------------
>   xen/include/asm-arm/grant_table.h | 17 +++++++++++++++--
>   2 files changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 35dc1f7..44258ad 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -41,7 +41,6 @@
>   #include <xen/sizes.h>
>   #include <xen/libfdt/libfdt.h>
>   
> -#include <asm/guest_atomics.h>
>   #include <asm/setup.h>
>   
>   /* Override macros from asm/page.h to make them work with mfn_t */
> @@ -1532,21 +1531,6 @@ void put_page_type(struct page_info *page)
>       return;
>   }
>   
> -void gnttab_clear_flag(struct domain *d, unsigned long nr, uint16_t *addr)
> -{
> -    guest_clear_mask16(d, BIT(nr, UL), addr);
> -}
> -
> -void gnttab_mark_dirty(struct domain *d, mfn_t mfn)
> -{
> -    /* XXX: mark dirty */
> -    static int warning;
> -    if (!warning) {
> -        gdprintk(XENLOG_WARNING, "gnttab_mark_dirty not implemented yet\n");
> -        warning = 1;
> -    }
> -}
> -
>   int create_grant_host_mapping(unsigned long addr, mfn_t frame,
>                                 unsigned int flags, unsigned int cache_flags)
>   {
> diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
> index 1ed0aef..b0d673b 100644
> --- a/xen/include/asm-arm/grant_table.h
> +++ b/xen/include/asm-arm/grant_table.h
> @@ -6,6 +6,8 @@
>   #include <xen/pfn.h>
>   #include <xen/sched.h>
>   
> +#include <asm/guest_atomics.h>
> +
>   #define INITIAL_NR_GRANT_FRAMES 1U
>   #define GNTTAB_MAX_VERSION 1
>   
> @@ -14,13 +16,24 @@ struct grant_table_arch {
>       gfn_t *status_gfn;
>   };
>   
> -void gnttab_clear_flag(struct domain *d, unsigned long nr, uint16_t *addr);
> +static inline void gnttab_clear_flag(struct domain *d,
> +                                     unsigned long nr, uint16_t *addr)
> +{
> +    guest_clear_mask16(d, BIT(nr, UL), addr);
> +}
> +
> +static inline void gnttab_mark_dirty(struct domain *d, mfn_t mfn)
> +{
> +#ifndef NDEBUG
> +    printk_once(XENLOG_G_WARNING "gnttab_mark_dirty not implemented yet\n");
> +#endif
> +}
> +
>   int create_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>                                 unsigned int flags, unsigned int cache_flags);
>   #define gnttab_host_mapping_get_page_type(ro, ld, rd) (0)
>   int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>                                  unsigned long new_gpaddr, unsigned int flags);
> -void gnttab_mark_dirty(struct domain *d, mfn_t mfn);
>   #define gnttab_release_host_mappings(domain) 1
>   
>   /*
> 

-- 
Julien Grall

_______________________________________________
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

[Xen-devel] [PATCH v2 4/5] xen/gnttab: Fold adjacent calls to gnttab_clear_flags()

[Andrew Cooper]

Atomic operations are expensive to use, especially following XSA-295 for ARM.
It is wasteful to use two of them back-to-back when one will do.

Especially for a misbehaving guest on ARM, this will reduce the system
disruption required to complete the grant operations.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.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 | 54 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 39 insertions(+), 15 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 4bd5777d29..e6a0f30a4b 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -796,8 +796,7 @@ static int _set_status_v2(const grant_entry_header_t *shah,
              (scombo.domid != ldomid) ||
              (!readonly && (scombo.flags & GTF_readonly)) )
         {
-            gnttab_clear_flags(rd, GTF_writing, status);
-            gnttab_clear_flags(rd, GTF_reading, status);
+            gnttab_clear_flags(rd, GTF_writing | 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);
@@ -919,7 +918,7 @@ map_grant_ref(
     int            rc = GNTST_okay;
     u32            old_pin;
     u32            act_pin;
-    unsigned int   cache_flags, refcnt = 0, typecnt = 0;
+    unsigned int   cache_flags, clear_flags = 0, refcnt = 0, typecnt = 0;
     bool           host_map_created = false;
     struct active_grant_entry *act = NULL;
     struct grant_mapping *mt;
@@ -1220,10 +1219,13 @@ map_grant_ref(
  unlock_out_clear:
     if ( !(op->flags & GNTMAP_readonly) &&
          !(act->pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
-        gnttab_clear_flags(rd, GTF_writing, status);
+        clear_flags |= GTF_writing;
 
     if ( !act->pin )
-        gnttab_clear_flags(rd, GTF_reading, status);
+        clear_flags |= GTF_reading;
+
+    if ( clear_flags )
+        gnttab_clear_flags(rd, clear_flags, status);
 
  act_release_out:
     active_entry_release(act);
@@ -1433,6 +1435,7 @@ unmap_common_complete(struct gnttab_unmap_common *op)
     grant_entry_header_t *sha;
     struct page_info *pg;
     uint16_t *status;
+    unsigned int clear_flags = 0;
 
     if ( !op->done )
     {
@@ -1493,10 +1496,13 @@ unmap_common_complete(struct gnttab_unmap_common *op)
 
     if ( ((act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0) &&
          !(op->done & GNTMAP_readonly) )
-        gnttab_clear_flags(rd, GTF_writing, status);
+        clear_flags |= GTF_writing;
 
     if ( act->pin == 0 )
-        gnttab_clear_flags(rd, GTF_reading, status);
+        clear_flags |= GTF_reading;
+
+    if ( clear_flags )
+        gnttab_clear_flags(rd, clear_flags, status);
 
     active_entry_release(act);
     grant_read_unlock(rgt);
@@ -2324,6 +2330,7 @@ release_grant_for_copy(
     uint16_t *status;
     grant_ref_t trans_gref;
     struct domain *td;
+    unsigned int clear_flags = 0;
 
     grant_read_lock(rgt);
 
@@ -2354,11 +2361,14 @@ release_grant_for_copy(
 
         act->pin -= GNTPIN_hstw_inc;
         if ( !(act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) )
-            gnttab_clear_flags(rd, GTF_writing, status);
+            clear_flags |= GTF_writing;
     }
 
     if ( !act->pin )
-        gnttab_clear_flags(rd, GTF_reading, status);
+        clear_flags |= GTF_reading;
+
+    if ( clear_flags )
+        gnttab_clear_flags(rd, clear_flags, status);
 
     active_entry_release(act);
     grant_read_unlock(rgt);
@@ -2384,11 +2394,16 @@ 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)) )
-        gnttab_clear_flags(rd, GTF_writing, status);
+        clear_flags |= GTF_writing;
 
     if ( !act->pin )
-        gnttab_clear_flags(rd, GTF_reading, status);
+        clear_flags |= GTF_reading;
+
+    if ( clear_flags )
+        gnttab_clear_flags(rd, clear_flags, status);
 }
 
 /*
@@ -2417,6 +2432,7 @@ acquire_grant_for_copy(
     uint16_t trans_length;
     bool is_sub_page;
     s16 rc = GNTST_okay;
+    unsigned int clear_flags = 0;
 
     *page = NULL;
 
@@ -2639,10 +2655,13 @@ acquire_grant_for_copy(
  unlock_out_clear:
     if ( !(readonly) &&
          !(act->pin & (GNTPIN_hstw_mask | GNTPIN_devw_mask)) )
-        gnttab_clear_flags(rd, GTF_writing, status);
+        clear_flags |= GTF_writing;
 
     if ( !act->pin )
-        gnttab_clear_flags(rd, GTF_reading, status);
+        clear_flags |= GTF_reading;
+
+    if ( clear_flags )
+        gnttab_clear_flags(rd, clear_flags, status);
 
  unlock_out:
     active_entry_release(act);
@@ -3603,6 +3622,8 @@ 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;
@@ -3677,11 +3698,14 @@ gnttab_release_mappings(
             }
 
             if ( (act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0 )
-                gnttab_clear_flags(rd, GTF_writing, status);
+                clear_flags |= GTF_writing;
         }
 
         if ( act->pin == 0 )
-            gnttab_clear_flags(rd, GTF_reading, status);
+            clear_flags |= GTF_reading;
+
+        if ( clear_flags )
+            gnttab_clear_flags(rd, clear_flags, status);
 
         active_entry_release(act);
         grant_read_unlock(rgt);
-- 
2.11.0


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

Re: [Xen-devel] [PATCH v2 4/5] xen/gnttab: Fold adjacent calls to gnttab_clear_flags()

[Andrew Cooper]

Please ignore this.  I had an error trying to merge a single v2 patch
into an existing series.

~Andrew

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

[Xen-devel] [PATCH v2 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>

v2:
 * Use unsigned int for the mask parameter
 * Correct I to i for the x86 constraint
---
 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 6d8f17d2ba..4bd5777d29 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 b0d673b0fe..ad120827ea 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,
+                                      unsigned int 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 121b33dc6e..568a6bb57c 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,
+                                      unsigned int 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 and %1,%0" : "+m" (*addr) : "ir" ((uint16_t)~mask));
 }
 
 /* Foreign mappings of HVM-guest pages do not modify the type count. */
-- 
2.11.0


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

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

[Jan Beulich]

On 04.07.2019 21:14, Andrew Cooper wrote:
> 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>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Julien Grall]

Hi Andrew,

On 7/4/19 8:14 PM, Andrew Cooper wrote:
> 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>
> 
> v2:
>   * Use unsigned int for the mask parameter

I don't seem to find the request on the ML. Technically the mask can 
only be 16-bit. May I ask the reason of this change?

Cheers,

-- 
Julien Grall

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

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

[Andrew Cooper]

On 07/07/2019 19:42, Julien Grall wrote:
> Hi Andrew,
>
> On 7/4/19 8:14 PM, Andrew Cooper wrote:
>> 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>
>>
>> v2:
>>   * Use unsigned int for the mask parameter
>
> I don't seem to find the request on the ML. Technically the mask can
> only be 16-bit. May I ask the reason of this change?

It is on the mailing list, but an orphaned email due to Jan's email changes.

https://lore.kernel.org/xen-devel/1561109798-8744-5-git-send-email-andrew.cooper3@citrix.com/T/#t

~Andrew

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

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

[Julien Grall]

Hi,

On 7/8/19 7:11 PM, Andrew Cooper wrote:
> On 07/07/2019 19:42, Julien Grall wrote:
>> Hi Andrew,
>>
>> On 7/4/19 8:14 PM, Andrew Cooper wrote:
>>> 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>
>>>
>>> v2:
>>>    * Use unsigned int for the mask parameter
>>
>> I don't seem to find the request on the ML. Technically the mask can
>> only be 16-bit. May I ask the reason of this change?
> 
> It is on the mailing list, but an orphaned email due to Jan's email changes.

Is it the same problem as I have seen the past 6 months between Juergen 
and Jan's e-mail?

> 
> https://lore.kernel.org/xen-devel/1561109798-8744-5-git-send-email-andrew.cooper3@citrix.com/T/#t

To be honest, I think it is wrong to try to micro-optimize a common 
prototype for the benefit of one architecture and one compiler version 
(or even none per the e-mail).

One could also argue that this may be not beneficial for the non-x86 
architecture depending on how the compiler decide to do the cast from 
32-bit to 16-bit...

Cheers,

-- 
Julien Grall

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

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

[Andrew Cooper]

On 08/07/2019 20:27, Julien Grall wrote:
> Hi,
>
> On 7/8/19 7:11 PM, Andrew Cooper wrote:
>> On 07/07/2019 19:42, Julien Grall wrote:
>>> Hi Andrew,
>>>
>>> On 7/4/19 8:14 PM, Andrew Cooper wrote:
>>>> 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>
>>>>
>>>> v2:
>>>>    * Use unsigned int for the mask parameter
>>>
>>> I don't seem to find the request on the ML. Technically the mask can
>>> only be 16-bit. May I ask the reason of this change?
>>
>> It is on the mailing list, but an orphaned email due to Jan's email
>> changes.
>
> Is it the same problem as I have seen the past 6 months between
> Juergen and Jan's e-mail?

I think its different, but I'm losing track tbh.

>
>>
>> https://lore.kernel.org/xen-devel/1561109798-8744-5-git-send-email-andrew.cooper3@citrix.com/T/#t
>>
>
> To be honest, I think it is wrong to try to micro-optimize a common
> prototype for the benefit of one architecture and one compiler version
> (or even none per the e-mail).

The prototype wasn't common.  Observe that before this patch, ARM used
unsigned long while x86 used uint16_t.  It should become common, however.

In practice, we're talking about bits 3 and 4, and this isn't liable to
change in a hurry.

> One could also argue that this may be not beneficial for the non-x86
> architecture depending on how the compiler decide to do the cast from
> 32-bit to 16-bit...

All architecture necessarily suffer the downcast somewhere, even x86. 
ARM's is in the prototype for guest_clear_mask16(), but in terms of the
common logic for calculating conditionally which bits to clear, keeping
everything as unsigned int for as long as possible offers the most
flexibility to the compiler, as it can see all the constants involved.

~Andrew

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

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

[Julien Grall]

Hi Andrew,

On 7/8/19 9:21 PM, Andrew Cooper wrote:
> On 08/07/2019 20:27, Julien Grall wrote:
>> Hi,
>>
>> On 7/8/19 7:11 PM, Andrew Cooper wrote:
>>> On 07/07/2019 19:42, Julien Grall wrote:
>>>> Hi Andrew,
>>>>
>>>> On 7/4/19 8:14 PM, Andrew Cooper wrote:
>>>>> 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>
>>>>>
>>>>> v2:
>>>>>     * Use unsigned int for the mask parameter
>>>>
>>>> I don't seem to find the request on the ML. Technically the mask can
>>>> only be 16-bit. May I ask the reason of this change?
>>>
>>> It is on the mailing list, but an orphaned email due to Jan's email
>>> changes.
>>
>> Is it the same problem as I have seen the past 6 months between
>> Juergen and Jan's e-mail?
> 
> I think its different, but I'm losing track tbh.

It would be nice to resolve it... It is a pain to try to match them with 
the correct thread.

> 
>>
>>>
>>> https://lore.kernel.org/xen-devel/1561109798-8744-5-git-send-email-andrew.cooper3@citrix.com/T/#t
>>>
>>
>> To be honest, I think it is wrong to try to micro-optimize a common
>> prototype for the benefit of one architecture and one compiler version
>> (or even none per the e-mail).
> 
> The prototype wasn't common.  Observe that before this patch, ARM used
> unsigned long while x86 used uint16_t.  It should become common, however.

I am not sure to follow this. AFAICT, we use uint16_t properly on Arm.

> 
> In practice, we're talking about bits 3 and 4, and this isn't liable to
> change in a hurry.
> 
>> One could also argue that this may be not beneficial for the non-x86
>> architecture depending on how the compiler decide to do the cast from
>> 32-bit to 16-bit...
> 
> All architecture necessarily suffer the downcast somewhere, even x86.
> ARM's is in the prototype for guest_clear_mask16(), but in terms of the
> common logic for calculating conditionally which bits to clear, keeping
> everything as unsigned int for as long as possible offers the most
> flexibility to the compiler, as it can see all the constants involved.

This is the argument I was looking for :). Thank you for writing it!

Cheers,

-- 
Julien Grall

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

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

[Andrew Cooper]

On 08/07/2019 21:28, Julien Grall wrote:
>>>>
>>>> https://lore.kernel.org/xen-devel/1561109798-8744-5-git-send-email-andrew.cooper3@citrix.com/T/#t
>>>>
>>>>
>>>
>>> To be honest, I think it is wrong to try to micro-optimize a common
>>> prototype for the benefit of one architecture and one compiler version
>>> (or even none per the e-mail).
>>
>> The prototype wasn't common.  Observe that before this patch, ARM used
>> unsigned long while x86 used uint16_t.  It should become common,
>> however.
>
> I am not sure to follow this. AFAICT, we use uint16_t properly on Arm.

Look at the modifications to gnttab_clear_flags(), and in particular,
the removals.  Before this patch, the API really is different between
ARM and x86.  (Although it differed between unsigned long and unsigned
int.  The uint16_t was a mistake on my behalf.)

>
>>
>> In practice, we're talking about bits 3 and 4, and this isn't liable to
>> change in a hurry.
>>
>>> One could also argue that this may be not beneficial for the non-x86
>>> architecture depending on how the compiler decide to do the cast from
>>> 32-bit to 16-bit...
>>
>> All architecture necessarily suffer the downcast somewhere, even x86.
>> ARM's is in the prototype for guest_clear_mask16(), but in terms of the
>> common logic for calculating conditionally which bits to clear, keeping
>> everything as unsigned int for as long as possible offers the most
>> flexibility to the compiler, as it can see all the constants involved.
>
> This is the argument I was looking for :). Thank you for writing it!

Can I take this as an ack, or a request for clarification in the commit
message, or something else?

(This detail is the final outstanding piece for the series to be committed.)

~Andrew

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

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

[Julien Grall]

Hi Andrew,

On 7/8/19 9:38 PM, Andrew Cooper wrote:
> On 08/07/2019 21:28, Julien Grall wrote:
>>>>>
>>>>> https://lore.kernel.org/xen-devel/1561109798-8744-5-git-send-email-andrew.cooper3@citrix.com/T/#t
>>>>>
>>>>>
>>>>
>>>> To be honest, I think it is wrong to try to micro-optimize a common
>>>> prototype for the benefit of one architecture and one compiler version
>>>> (or even none per the e-mail).
>>>
>>> The prototype wasn't common.  Observe that before this patch, ARM used
>>> unsigned long while x86 used uint16_t.  It should become common,
>>> however.
>>
>> I am not sure to follow this. AFAICT, we use uint16_t properly on Arm.
> 
> Look at the modifications to gnttab_clear_flags(), and in particular,
> the removals.  Before this patch, the API really is different between
> ARM and x86.  (Although it differed between unsigned long and unsigned
> int.  The uint16_t was a mistake on my behalf.)

Oh, yes. I am not sure why we use unsigned long for describing the 
shift. Anyway...

> 
>>
>>>
>>> In practice, we're talking about bits 3 and 4, and this isn't liable to
>>> change in a hurry.
>>>
>>>> One could also argue that this may be not beneficial for the non-x86
>>>> architecture depending on how the compiler decide to do the cast from
>>>> 32-bit to 16-bit...
>>>
>>> All architecture necessarily suffer the downcast somewhere, even x86.
>>> ARM's is in the prototype for guest_clear_mask16(), but in terms of the
>>> common logic for calculating conditionally which bits to clear, keeping
>>> everything as unsigned int for as long as possible offers the most
>>> flexibility to the compiler, as it can see all the constants involved.
>>
>> This is the argument I was looking for :). Thank you for writing it!
> 
> Can I take this as an ack, or a request for clarification in the commit
> message, or something else?

No need for clarification in the commit message.

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

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

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

[Jan Beulich]

On 08.07.2019 21:27, Julien Grall wrote:
> Hi,
> 
> On 7/8/19 7:11 PM, Andrew Cooper wrote:
>> On 07/07/2019 19:42, Julien Grall wrote:
>>> Hi Andrew,
>>>
>>> On 7/4/19 8:14 PM, Andrew Cooper wrote:
>>>> 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>
>>>>
>>>> v2:
>>>>    * Use unsigned int for the mask parameter
>>>
>>> I don't seem to find the request on the ML. Technically the mask can
>>> only be 16-bit. May I ask the reason of this change?
>>
>> It is on the mailing list, but an orphaned email due to Jan's email changes.
> 
> Is it the same problem as I have seen the past 6 months between Juergen and Jan's e-mail?

No, this was due to a disruptive (no data migration) change to our
email system. I'm hence unable to properly reply to mails still
sitting in my old mailbox (only).

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

[Xen-devel] [PATCH 5/5] xen/gnttab: Fold adjacent calls to gnttab_clear_flags()

[Andrew Cooper]

Atomic operations are expensive to use, especially following XSA-295 for ARM.
It is wasteful to use two of them back-to-back when one will do.

Especially for a misbehaving guest on ARM, this will reduce the system
disruption required to complete the grant operations.

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 | 54 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 39 insertions(+), 15 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 4bd5777..e6a0f30 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -796,8 +796,7 @@ static int _set_status_v2(const grant_entry_header_t *shah,
              (scombo.domid != ldomid) ||
              (!readonly && (scombo.flags & GTF_readonly)) )
         {
-            gnttab_clear_flags(rd, GTF_writing, status);
-            gnttab_clear_flags(rd, GTF_reading, status);
+            gnttab_clear_flags(rd, GTF_writing | 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);
@@ -919,7 +918,7 @@ map_grant_ref(
     int            rc = GNTST_okay;
     u32            old_pin;
     u32            act_pin;
-    unsigned int   cache_flags, refcnt = 0, typecnt = 0;
+    unsigned int   cache_flags, clear_flags = 0, refcnt = 0, typecnt = 0;
     bool           host_map_created = false;
     struct active_grant_entry *act = NULL;
     struct grant_mapping *mt;
@@ -1220,10 +1219,13 @@ map_grant_ref(
  unlock_out_clear:
     if ( !(op->flags & GNTMAP_readonly) &&
          !(act->pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
-        gnttab_clear_flags(rd, GTF_writing, status);
+        clear_flags |= GTF_writing;
 
     if ( !act->pin )
-        gnttab_clear_flags(rd, GTF_reading, status);
+        clear_flags |= GTF_reading;
+
+    if ( clear_flags )
+        gnttab_clear_flags(rd, clear_flags, status);
 
  act_release_out:
     active_entry_release(act);
@@ -1433,6 +1435,7 @@ unmap_common_complete(struct gnttab_unmap_common *op)
     grant_entry_header_t *sha;
     struct page_info *pg;
     uint16_t *status;
+    unsigned int clear_flags = 0;
 
     if ( !op->done )
     {
@@ -1493,10 +1496,13 @@ unmap_common_complete(struct gnttab_unmap_common *op)
 
     if ( ((act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0) &&
          !(op->done & GNTMAP_readonly) )
-        gnttab_clear_flags(rd, GTF_writing, status);
+        clear_flags |= GTF_writing;
 
     if ( act->pin == 0 )
-        gnttab_clear_flags(rd, GTF_reading, status);
+        clear_flags |= GTF_reading;
+
+    if ( clear_flags )
+        gnttab_clear_flags(rd, clear_flags, status);
 
     active_entry_release(act);
     grant_read_unlock(rgt);
@@ -2324,6 +2330,7 @@ release_grant_for_copy(
     uint16_t *status;
     grant_ref_t trans_gref;
     struct domain *td;
+    unsigned int clear_flags = 0;
 
     grant_read_lock(rgt);
 
@@ -2354,11 +2361,14 @@ release_grant_for_copy(
 
         act->pin -= GNTPIN_hstw_inc;
         if ( !(act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) )
-            gnttab_clear_flags(rd, GTF_writing, status);
+            clear_flags |= GTF_writing;
     }
 
     if ( !act->pin )
-        gnttab_clear_flags(rd, GTF_reading, status);
+        clear_flags |= GTF_reading;
+
+    if ( clear_flags )
+        gnttab_clear_flags(rd, clear_flags, status);
 
     active_entry_release(act);
     grant_read_unlock(rgt);
@@ -2384,11 +2394,16 @@ 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)) )
-        gnttab_clear_flags(rd, GTF_writing, status);
+        clear_flags |= GTF_writing;
 
     if ( !act->pin )
-        gnttab_clear_flags(rd, GTF_reading, status);
+        clear_flags |= GTF_reading;
+
+    if ( clear_flags )
+        gnttab_clear_flags(rd, clear_flags, status);
 }
 
 /*
@@ -2417,6 +2432,7 @@ acquire_grant_for_copy(
     uint16_t trans_length;
     bool is_sub_page;
     s16 rc = GNTST_okay;
+    unsigned int clear_flags = 0;
 
     *page = NULL;
 
@@ -2639,10 +2655,13 @@ acquire_grant_for_copy(
  unlock_out_clear:
     if ( !(readonly) &&
          !(act->pin & (GNTPIN_hstw_mask | GNTPIN_devw_mask)) )
-        gnttab_clear_flags(rd, GTF_writing, status);
+        clear_flags |= GTF_writing;
 
     if ( !act->pin )
-        gnttab_clear_flags(rd, GTF_reading, status);
+        clear_flags |= GTF_reading;
+
+    if ( clear_flags )
+        gnttab_clear_flags(rd, clear_flags, status);
 
  unlock_out:
     active_entry_release(act);
@@ -3603,6 +3622,8 @@ 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;
@@ -3677,11 +3698,14 @@ gnttab_release_mappings(
             }
 
             if ( (act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0 )
-                gnttab_clear_flags(rd, GTF_writing, status);
+                clear_flags |= GTF_writing;
         }
 
         if ( act->pin == 0 )
-            gnttab_clear_flags(rd, GTF_reading, status);
+            clear_flags |= GTF_reading;
+
+        if ( clear_flags )
+            gnttab_clear_flags(rd, clear_flags, status);
 
         active_entry_release(act);
         grant_read_unlock(rgt);
-- 
2.1.4


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