From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takuya Yoshikawa Subject: Re: [PATCH RFC 1/5] KVM: introduce a set_bit function for bitmaps in user space Date: Mon, 12 Apr 2010 10:29:43 +0900 Message-ID: <4BC27787.4020701@oss.ntt.co.jp> References: <20100409182732.857de4db.yoshikawa.takuya@oss.ntt.co.jp> <20100409183021.843ca432.yoshikawa.takuya@oss.ntt.co.jp> <4BC2020B.5030402@redhat.com> 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: Avi Kivity Return-path: Received: from serv2.oss.ntt.co.jp ([222.151.198.100]:42883 "EHLO serv2.oss.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753286Ab0DLB0l (ORCPT ); Sun, 11 Apr 2010 21:26:41 -0400 In-Reply-To: <4BC2020B.5030402@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: (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.