From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@redhat.com>
Subject: Re: [PATCH RFC 1/5] KVM: introduce a set_bit function for bitmaps
 in user space
Date: Sun, 11 Apr 2010 20:08:27 +0300
Message-ID: <4BC2020B.5030402@redhat.com>
References: <20100409182732.857de4db.yoshikawa.takuya@oss.ntt.co.jp> <20100409183021.843ca432.yoshikawa.takuya@oss.ntt.co.jp>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: mtosatti@redhat.com, kvm@vger.kernel.org, fernando@oss.ntt.co.jp
To: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:55583 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752336Ab0DKRIe (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sun, 11 Apr 2010 13:08:34 -0400
In-Reply-To: <20100409183021.843ca432.yoshikawa.takuya@oss.ntt.co.jp>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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.

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

> +/**
> + * 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.

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.

Can you replace the macros with inline functions?

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.