From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Ng2GX-0004aX-E5 for qemu-devel@nongnu.org; Fri, 12 Feb 2010 15:37:29 -0500 Received: from [199.232.76.173] (port=36069 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Ng2GW-0004Zv-Ea for qemu-devel@nongnu.org; Fri, 12 Feb 2010 15:37:28 -0500 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1Ng2GV-000128-Px for qemu-devel@nongnu.org; Fri, 12 Feb 2010 15:37:28 -0500 Received: from mail-pz0-f187.google.com ([209.85.222.187]:56339) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Ng2GV-000122-FW for qemu-devel@nongnu.org; Fri, 12 Feb 2010 15:37:27 -0500 Received: by pzk17 with SMTP id 17so1705616pzk.4 for ; Fri, 12 Feb 2010 12:37:26 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <4B75B725.1040305@twiddle.net> References: <4B75B725.1040305@twiddle.net> From: Blue Swirl Date: Fri, 12 Feb 2010 22:37:06 +0200 Message-ID: Subject: Re: [Qemu-devel] [PATCH 3/6] Fix last page errors in page_set_flags and page_check_range. Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson Cc: qemu-devel@nongnu.org On Fri, Feb 12, 2010 at 10:16 PM, Richard Henderson wrote= : > On 02/12/2010 11:47 AM, Blue Swirl wrote: >> >> Please make separate patches for unrelated changes. Now the essence of >> the patch is very hard to see. Also pure formatting changes are not >> very useful. > > Is this just about page_get_flags, or was there some other pure formattin= g > change to which you object? =C2=A0For instance: Also this one: - end =3D TARGET_PAGE_ALIGN(start+len); /* must do before we loose bits in the next step */ + /* END must be computed before we drop bits from START. */ + end =3D TARGET_PAGE_ALIGN(start + len); start =3D start & TARGET_PAGE_MASK; and these: - if( !p ) + if (!p) return -1; - if( !(p->flags & PAGE_VALID) ) + if (!(p->flags & PAGE_VALID)) > >>> - =C2=A0 =C2=A0if (start + len < start) >>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0/* we've wrapped around */ >>> + =C2=A0 =C2=A0if (start + len - 1 < start) { >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0/* We've wrapped around. =C2=A0*/ >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0return -1; >>> + =C2=A0 =C2=A0} > > Only the first line is required to fix an off-by-one error. =C2=A0But if = I don't > add the braces, generally the patch will be bounced for that. That change is OK. >>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0/* We may be called for host regions that = are outside guest >>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 address space. =C2=A0*/ >> >> Why remove the comment, is it no longer true? > > Yes, after the entire patch series is applied. =C2=A0Indeed, I believe th= at many > of the addresses rejected here were *not* in fact outside the guest addre= ss > space, merely that page_find_alloc did not accurately know what the guest > address space was. > > I think it was a mistake to ever consider usage of this function with > out-of-band addresses as valid -- it adds a great deal of confusion. > > I could add an assertion here to make sure, if you like. > > If this were C++, it might have been interesting to try to use the type > system to keep the host and guest address spaces forcibly separate. =C2= =A0But > since this is plain C, I think wrapping everything in structures and acce= ss > macros would just be too ugly. > > > r~ >