Xen devel - Feb 2012 - x86: gnttab_clear_flag() abusing clear_bit()

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().)

Thanks, Jan

On 07/02/2012 10:34, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>> On 06.02.12 at 18:06, "Jan Beulich"
<JBeulich@suse.com> 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.
Looks fine to me, in principle. I would add a comment to the x86
gnttab_clear_flag() explaining why we have to open code something that looks
a lot like clear_bit().

 -- Keir
> 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@lists.xensource.com
> http://lists.xensource.com/xen-devel

>>> On 06.02.12 at 18:06, "Jan Beulich"
<JBeulich@suse.com> 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. */

>>> On 07.02.12 at 06:10, Keir Fraser <keir.xen@gmail.com> wrote:
> On 07/02/2012 10:34, "Jan Beulich" <JBeulich@suse.com>
wrote:
> 
>>>>> On 06.02.12 at 18:06, "Jan Beulich"
<JBeulich@suse.com> 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.
> 
> Looks fine to me, in principle. I would add a comment to the x86
> gnttab_clear_flag() explaining why we have to open code something that
looks
> a lot like clear_bit().
That one I already did, will submit soon (desiring clarification on the
below).

As to the "+m" constraint - I''m being told that
"+m" (var) is equivalent
to "=m" (var) : "m" (var), no matter what the documentation
says
regarding ''+'' (but they''re also not seeing a need to
adjust the docs
accordingly).

The question is whether we should go with the (documentation-wise
correct) form, or the shorter one (which they''re unlikely to change
the meaning of, given in how many places "+m" is used in e.g. Linux).

Jan

On 09/02/2012 01:01, "Jan Beulich" <JBeulich@suse.com> wrote:
>> Looks fine to me, in principle. I would add a comment to the x86
>> gnttab_clear_flag() explaining why we have to open code something that
looks
>> a lot like clear_bit().
> 
> That one I already did, will submit soon (desiring clarification on the
> below).
> 
> As to the "+m" constraint - I''m being told that
"+m" (var) is equivalent
> to "=m" (var) : "m" (var), no matter what the
documentation says
> regarding ''+'' (but they''re also not seeing a
need to adjust the docs
> accordingly).
> 
> The question is whether we should go with the (documentation-wise
> correct) form, or the shorter one (which they''re unlikely to
change
> the meaning of, given in how many places "+m" is used in e.g.
Linux).
You could switch us to "+m" and see how we get on.

 -- Keir