[Xen-devel] [PATCH, RFC] Re: x86: gnttab_clear_flag() abusing clear_bit()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.02.12 at 18:06, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> Back in c/s 17194:af33f2054f47 bitops got restricted to 4-bytes and
> larger operands only. gnttab_clear_flag() cheats in casting a uint16_t *
> to unsigned long * - how is that not a problem in the context of the
> goal that said c/s set, in particular for v2 of the interface? (Given that
> this is being implemented as arch-specific routine - so far for reasons
> that escape me - this should be simple to fix by using inline assembly
> rather than clear_bit().)
> 
> Further I just spotted one instance where the or of two bit positions
> gets passed to this function - that's quite definitely a bug I would say.
> 
> And, quite the opposite, __fixup_status_for_pin() ands out the
> negation of bit positions rather than masks... (Which also raises
> the question whether it really would need to be clear_bit() above
> instead of the cheaper __clear_bit().)

Below the tentative fix for all of the above problems. In the light
of the comment at the top of x86's bitops.h I'm awaiting our gcc
experts' response regarding the safety of using "+m" here.

Jan

--- 2012-02-06.orig/xen/common/grant_table.c
+++ 2012-02-06/xen/common/grant_table.c
@@ -397,7 +397,8 @@ static int _set_status_v2(domid_t  domid
              (id != domid) ||
              (!readonly && (flags & GTF_readonly)) )
         {
-            gnttab_clear_flag(_GTF_reading | _GTF_writing, status);
+            gnttab_clear_flag(_GTF_writing, status);
+            gnttab_clear_flag(_GTF_reading, status);
             PIN_FAIL(done, GNTST_general_error,
                      "Unstable flags (%x) or dom (%d). (expected dom %d) "
                      "(r/w: %d)\n",
@@ -1715,14 +1716,14 @@ __release_grant_for_copy(
    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_pin(struct active_grant_entry *act,
+static void __fixup_status_for_pin(const struct active_grant_entry *act,
                                    uint16_t *status)
 {
     if ( !(act->pin & GNTPIN_hstw_mask) )
-        *status &= ~_GTF_writing;
+        *status &= ~GTF_writing;
 
     if ( !(act->pin & GNTPIN_hstr_mask) )
-        *status &= ~_GTF_reading;
+        *status &= ~GTF_reading;
 }
 
 /* Grab a frame number from a grant entry and update the flags and pin
--- 2012-02-06.orig/xen/include/asm-ia64/grant_table.h
+++ 2012-02-06/xen/include/asm-ia64/grant_table.h
@@ -5,6 +5,8 @@
 #ifndef __ASM_GRANT_TABLE_H__
 #define __ASM_GRANT_TABLE_H__
 
+#include <asm/intrinsics.h>
+
 #define INITIAL_NR_GRANT_FRAMES 1
 
 // for grant map/unmap
@@ -82,9 +84,15 @@ int guest_physmap_add_page(struct domain
 
 #define gnttab_mark_dirty(d, f) ((void)f)
 
-static inline void gnttab_clear_flag(unsigned long nr, uint16_t *addr)
+static inline void gnttab_clear_flag(unsigned int nr, volatile uint16_t *st)
 {
-       clear_bit(nr, addr);
+       uint16_t mask = ~(1 << nr), old;
+       CMPXCHG_BUGCHECK_DECL
+
+       do {
+               CMPXCHG_BUGCHECK(st);
+               old = *st;
+       } while (cmpxchg_rel(st, old, old & mask) != old);
 }
 
 #define gnttab_host_mapping_get_page_type(op, ld, rd)   \
--- 2012-02-06.orig/xen/include/asm-x86/grant_table.h
+++ 2012-02-06/xen/include/asm-x86/grant_table.h
@@ -48,9 +48,11 @@ int replace_grant_host_mapping(
 
 #define gnttab_mark_dirty(d, f) paging_mark_dirty((d), (f))
 
-static inline void gnttab_clear_flag(unsigned long nr, uint16_t *addr)
+static inline void gnttab_clear_flag(unsigned int nr, uint16_t *addr)
 {
-    clear_bit(nr, (unsigned long *)addr);
+    asm volatile ("lock btrw %1,%0"
+                  : "+m" (*addr)
+                  : "Ir" (nr));
 }
 
 /* Foreign mappings of HHVM-guest pages do not modify the type count. */



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