From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42414) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVNQD-0004C6-OL for qemu-devel@nongnu.org; Tue, 10 Mar 2015 12:54:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YVNQ9-0006of-88 for qemu-devel@nongnu.org; Tue, 10 Mar 2015 12:54:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38142) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVNQ9-0006oX-00 for qemu-devel@nongnu.org; Tue, 10 Mar 2015 12:54:17 -0400 Date: Tue, 10 Mar 2015 17:54:11 +0100 From: Andrew Jones Message-ID: <20150310165410.GD6320@hawk.usersys.redhat.com> References: <1423753507-30542-1-git-send-email-drjones@redhat.com> <1423753507-30542-6-git-send-email-drjones@redhat.com> <20150212170827.GA10193@hawk.usersys.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH 5/5] target-arm: apply get_S1prot to get_phys_addr_v6 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: QEMU Developers On Tue, Mar 10, 2015 at 03:57:27PM +0000, Peter Maydell wrote: > On 12 February 2015 at 17:08, Andrew Jones wrote: > > On Thu, Feb 12, 2015 at 04:05:07PM +0100, Andrew Jones wrote: > >> Now that we have get_S1prot, we can apply it to get_phys_addr_v6 > >> for a minor code cleanup. > > I think this is a bad idea -- better to keep the long > and short descriptor code paths separate. It's too easy > to get confused otherwise. I don't mind keeping them separate, but I disagree with it being confusing keeping them together :-) > > > Actually, I should point out that this isn't just a cleanup, but > > also a fix. See below. > > > The original code didn't take into account that it may be calling check_ap > > with a simple AP, AP[2:1]. > > No, because check_ap() always takes AP[2:0]... No, it's really wrong. It's not the 2 vs. 3 bit issue that's the problem, it's the cases. You snipped most of my reply to myself. This part is pertinent > As a simple AP wouldn't be properly translated to protection flags with > check_ap (except for case 6), then I think this should have caused some > problems. Maybe this path just hasn't been tested? I don't see CR_AFE > getting used by Linux, so possibly not. So, yes, a simple (3-bit) ap would be handled by the 8-case switch with cases 0, 2, 4, and 6, but only case 6 would give the correct result. Thanks for the review. drew