From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37172) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVN5I-0001EB-P0 for qemu-devel@nongnu.org; Tue, 10 Mar 2015 12:32:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YVN5F-00086P-9D for qemu-devel@nongnu.org; Tue, 10 Mar 2015 12:32:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48804) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVN5F-00086H-1O for qemu-devel@nongnu.org; Tue, 10 Mar 2015 12:32:41 -0400 Date: Tue, 10 Mar 2015 17:32:33 +0100 From: Andrew Jones Message-ID: <20150310163233.GB6320@hawk.usersys.redhat.com> References: <1423753507-30542-1-git-send-email-drjones@redhat.com> <1423753507-30542-3-git-send-email-drjones@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH 2/5] target-arm: enable get_rw_prot to take simple AP 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:22:55PM +0000, Peter Maydell wrote: > On 12 February 2015 at 15:05, Andrew Jones wrote: > > Teach get_rw_prot about the simple AP format AP[2:1]. An additional > > switch was added, as opposed to converting ap := AP[2:1] to AP[2:0] > > with a simple shift - and then modifying cases 0,2,4,6, because the > > resulting code is easier to read with the switch. > > > > Signed-off-by: Andrew Jones > > --- > > target-arm/helper.c | 22 +++++++++++++++++++++- > > 1 file changed, 21 insertions(+), 1 deletion(-) > > > > diff --git a/target-arm/helper.c b/target-arm/helper.c > > index 610f305c4d661..b63ec7b7979f9 100644 > > --- a/target-arm/helper.c > > +++ b/target-arm/helper.c > > @@ -4698,12 +4698,32 @@ static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx) > > static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, > > int ap, int domain_prot) > > { > > + bool simple_ap = regime_using_lpae_format(env, mmu_idx) > > + || (regime_sctlr(env, mmu_idx) & SCTLR_AFE); > > We should check arm_feature(env, ARM_FEATURE_V6K) && (SCTLR.AFE is set); > that bit isn't defined til v6K. Indeed. Will send v2 for that. > > > + bool domain_prot_valid = !regime_using_lpae_format(env, mmu_idx); > > Given that the lpae code path is totally separate (and not even > calling this function yet), can't you just have it pass in a > zero domain_prot ? Or have the callers do the domain protection > check themselves... domain_prot=0 is a valid access permission (no access), so I didn't want to overload the meaning with 'not used'. I can move the check to the callers that need it though. It would actually be nice to remove the need for a 0 place holder from the other callers. > > > bool is_user = regime_is_user(env, mmu_idx); > > > > - if (domain_prot == 3) { > > + if (domain_prot_valid && domain_prot == 3) { > > return PAGE_READ | PAGE_WRITE; > > } > > > > + /* ap is AP[2:1] */ > > + if (simple_ap) { > > + switch (ap) { > > + case 0: > > + return is_user ? 0 : PAGE_READ | PAGE_WRITE; > > + case 1: > > + return PAGE_READ | PAGE_WRITE; > > + case 2: > > + return is_user ? 0 : PAGE_READ; > > + case 3: > > + return PAGE_READ; > > + default: > > + g_assert_not_reached(); > > + } > > + } > > I'm confused. Even if we're using the simple-permissions > model, the ap parameter is still AP[2:0]. Shouldn't this > switch be for cases 0, 2, 4, 6 ? Depends on how we choose to implement the callers. Currently I only require the caller to send in 2 bits for the simple model. If we want to require them to send in 3, then we'll need to shift a zero in for the lpae caller, rather than shift a zero out for the v6 caller. > > > + > > + /* ap is AP[2:0] */ > > switch (ap) { > > case 0: > > if (arm_feature(env, ARM_FEATURE_V7)) { > > -- > > 1.9.3 > > > > -- PMM