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

[Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>]

(2010/04/12 2:08), Avi Kivity wrote:
> On 04/09/2010 12:30 PM, Takuya Yoshikawa wrote:
>> This work is initially suggested by Avi Kivity for moving the
>> dirty bitmaps used by KVM to user space: This makes it possible
>> to manipulate the bitmaps from qemu without copying from KVM.
>>
>> Note: We are now brushing up this code before sending to x86's
>> maintainers.
>>
>
> The subject prefix will need to be x86:, not KVM:, since it isn't kvm
> specific, and you will need to beef up the description since you will
> undoubtedly be asked why this is needed.

OK, I'll do my best. I wish somebody other than KVM people also want to
use this.

>
> Also, please add the generic implementation (in a separate patch). We
> have dirty bitmaps for ppc as well.

Yes, I've already implemented asm-generic set bit user, but need to finish
other dirty bitmap related parts for ppc to explain why it is needed.

>
>> +/**
>> + * set_bit_user: - Set a bit of a bitmap in user space.
>> + * @nr: Bit offset to set.
>> + * @addr: Base address, in user space.
>> + *
>> + * Context: User context only. This function may sleep.
>> + *
>> + * This macro sets a bit of a bitmap in user space. Note that this
>> + * is same as __set_bit but not set_bit in the sense that setting
>> + * the bit is not done atomically.
>> + *
>> + * Returns zero on success, -EFAULT on error.
>> + */
>> +#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.

Actually I first named it like that and then noticed that in the uaccess'
convention, __ prefix means it is "with less checking" version.

I don't know which is better in this case, should be "set_bit_user_non_atomic"
though bit long? May be judged by x86 people.

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

Oh, I didn't care about that. I'll explain in the next version.

>
> Can you replace the macros with inline functions?
>

Yes, I can. I have both inline and macro versions and talking with Fernando
which will be preferred.