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

[Keir Fraser <keir.xen@gmail.com>]

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