From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37279) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVkei-00089O-K9 for qemu-devel@nongnu.org; Wed, 11 Mar 2015 13:42:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YVkeh-000519-9b for qemu-devel@nongnu.org; Wed, 11 Mar 2015 13:42:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56589) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVkeh-00050g-2h for qemu-devel@nongnu.org; Wed, 11 Mar 2015 13:42:51 -0400 Date: Wed, 11 Mar 2015 18:42:42 +0100 From: Andrew Jones Message-ID: <20150311174241.GA10903@hawk.usersys.redhat.com> References: <1426021590-4834-1-git-send-email-drjones@redhat.com> <1426021590-4834-4-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 v2 3/3] target-arm: get_phys_addr_lpae: more xn control List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: QEMU Developers On Wed, Mar 11, 2015 at 05:02:00PM +0000, Peter Maydell wrote: > On 10 March 2015 at 21:06, Andrew Jones wrote: > > This patch makes the following changes to the determination of > > whether an address is executable, when translating addresses > > using LPAE. > > > > 1. No longer assumes that PL0 can't execute when it can't read. > > It can in AArch64, a difference from AArch32. > > 2. Use va_size == 64 to determine we're in AArch64, rather than > > arm_feature(env, ARM_FEATURE_V8), which is insufficient. > > 3. Add additional XN determinants > > - NS && is_secure && (SCR & SCR_SIF) > > - WXN && (prot & PAGE_WRITE) > > - AArch64: (prot_PL0 & PAGE_WRITE) > > - AArch32: UWXN && (prot_PL0 & PAGE_WRITE) > > - XN determination should also work in secure mode (untested) > > - XN may even work in EL2 (currently impossible to test) > > 4. Cleans up the bloated PAGE_EXEC condition - by removing it. > > > > The helper get_S1prot is introduced. It may even work in EL2, > > when support for that comes, but, as the function name implies, > > it only works for stage 1 translations. > > > > Signed-off-by: Andrew Jones > > I like the general shape of this patch. Minor comment below: > > > --- > > target-arm/helper.c | 129 ++++++++++++++++++++++++++++++++++++++++------------ > > 1 file changed, 100 insertions(+), 29 deletions(-) > > > > diff --git a/target-arm/helper.c b/target-arm/helper.c > > index d996659652f8d..c457e9ab8c85a 100644 > > --- a/target-arm/helper.c > > +++ b/target-arm/helper.c > > @@ -4962,15 +4962,11 @@ static inline int ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, > > /* Translate section/page access permissions to page > > * R/W protection flags. > > * > > - * @env: CPUARMState > > - * @mmu_idx: MMU index indicating required translation regime > > * @ap: The 2-bit simple AP (AP[2:1]) > > + * @is_user: TRUE if accessing from PL0 > > */ > > -static inline int > > -simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap) > > +static inline int simple_ap_to_rw_prot_is_user(int ap, bool is_user) > > { > > - bool is_user = regime_is_user(env, mmu_idx); > > - > > switch (ap) { > > case 0: > > return is_user ? 0 : PAGE_READ | PAGE_WRITE; > > @@ -4985,6 +4981,94 @@ simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap) > > } > > } > > > > +static inline int > > +simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap) > > +{ > > + return simple_ap_to_rw_prot_is_user(ap, regime_is_user(env, mmu_idx)); > > +} > > + > > +/* Translate section/page access permissions to protection flags > > + * > > + * @env: CPUARMState > > + * @mmu_idx: MMU index indicating required translation regime > > + * @is_aa64: TRUE if AArch64 > > + * @ap: The 2-bit simple AP (AP[2:1]) > > + * @ns: NS (non-secure) bit > > + * @xn: XN (execute-never) bit > > + * @pxn: PXN (privileged execute-never) bit > > + */ > > +static int get_S1prot(CPUARMState *env, ARMMMUIdx mmu_idx, bool is_aa64, > > + int ap, int ns, int xn, int pxn) > > +{ > > + bool is_user = regime_is_user(env, mmu_idx); > > + int prot_rw, user_rw; > > + bool have_wxn; > > + int wxn = 0; > > + > > + assert(mmu_idx != ARMMMUIdx_S2NS); > > + > > + user_rw = simple_ap_to_rw_prot_is_user(ap, true); > > + if (is_user) { > > + prot_rw = user_rw; > > + } else { > > + prot_rw = simple_ap_to_rw_prot_is_user(ap, false); > > + } > > + > > + if (ns && arm_is_secure(env) && (env->cp15.scr_el3 & SCR_SIF)) { > > + return prot_rw; > > + } > > + > > + /* TODO have_wxn should be replaced with > > + * ARM_FEATURE_V8 || (ARM_FEATURE_V7 && ARM_FEATURE_EL2) > > + * when ARM_FEATURE_EL2 starts getting set. For now we assume all LPAE > > + * compatible processors have EL2, which is required for [U]WXN. > > + */ > > + have_wxn = arm_feature(env, ARM_FEATURE_LPAE); > > + > > + if (have_wxn) { > > + wxn = regime_sctlr(env, mmu_idx) & SCTLR_WXN; > > + } > > + > > + if (is_aa64) { > > + switch (regime_el(env, mmu_idx)) { > > + case 1: > > + if (is_user && !user_rw) { > > + wxn = 0; > > I don't understand this. We ignore the WXN bit if this is > a user access and the page is not readable ? Yup. If the page is not readable or writeable, AP[1]=0. I almost submitted an errata to the ARM ARM when I saw this on the 2nd line of table D4-32. I thought it must be a typo. However I tested it on hardware, and it works this way. So at least the weirdness has been implemented consistently... > > I also find the naming of this variable "user_rw" (and > to a lesser extent "prot_rw") very confusing. I keep > misreading "if (user_rw)" as meaning "if this page is > read-write for the user", when in fact it only means > "if this page is readable for the user". Right, user_rw really means ("user prot" & (PAGE_READ | PAGE_WRITE)), but as you can't have PAGE_WRITE without PAGE_READ, then when user_rw is non-zero we know it means PAGE_READ is set, but we still have to check for PAGE_WRITE explicitly when we care about it. > > Maybe it would be less confusing if we always did tests > against a set of PAGE_* flags rather than doing an > is/is-not-zero test? Sounds good. Will respin with this change, and also adding that hunk I forgot that collects NSTable bits. > > The rest looked OK to me. Thanks! drew