Re: [PATCH RFC 1/5] KVM: introduce a set_bit function for bitmaps in user space

["Fernando Luis Vázquez Cao" <fernando@oss.ntt.co.jp>]

On 04/12/2010 02:08 AM, Avi Kivity wrote:
>> +#define __set_bit_user_asm(nr, addr, err, errret)            \
>> +    asm volatile("1:    bts %1,%2\n"                \
>> +             "2:\n"                        \
>> +             ".section .fixup,\"ax\"\n"                \
>> +             "3:    mov %3,%0\n"                \
>> +             "    jmp 2b\n"                    \
>> +             ".previous\n"                    \
>> +             _ASM_EXTABLE(1b, 3b)                \
>> +             : "=r"(err)                    \
>> +             : "r" (nr), "m" (__m(addr)), "i" (errret), "0" (err))
>> +
>> +#define set_bit_user(nr, addr)                        \
>> +({                                    \
>> +    int __ret_sbu = 0;                        \
>> +                                    \
>> +    might_fault();                            \
>> +    if (access_ok(VERIFY_WRITE, addr, nr/8 + 1))            \
>> +        __set_bit_user_asm(nr, addr, __ret_sbu, -EFAULT);    \
>> +    else                                \
>> +        __ret_sbu = -EFAULT;                    \
>> +                                    \
>> +    __ret_sbu;                            \
>> +})
>> +
>>    
> 
> Should be called __set_bit_user() since it is non-atomic.
> 
> An interesting wart is that this will use the kernel's word size instead
> of userspace word size for access.  So, a 32-bit process might allocate
> a 4-byte bitmap, and a 64-bit kernel will use a 64-bit access to touch
> it, which might result in a fault.  This might be resolved by
> documenting that userspace bitmaps must be a multiple of 64-bits in size
> and recommending that they be 64-bit aligned as well.

Yes, the inline assembler above generates a REX prefixed bts with the W field
set (48 0f ab), which means we have a 64 bit operand size. In addition to the
solution you propose we could also implement a legacy mode version that uses
a 32bit bts. Compat ioctls and that ilk could pontentially benefit from this.