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.
Actually, I should point out that this isn't just a cleanup, but also a fix. See below. > > Signed-off-by: Andrew Jones <drjo...@redhat.com> > --- > target-arm/helper.c | 27 ++++++++------------------- > 1 file changed, 8 insertions(+), 19 deletions(-) > > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 20e5753bd216d..c41305e7e2bdf 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -5064,30 +5064,19 @@ static int get_phys_addr_v6(CPUARMState *env, > uint32_t address, int access_type, > } > code = 15; > } > - if (domain_prot == 3) { > - *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC; > - } else { > - bool is_user = regime_is_user(env, mmu_idx); > - > - if (pxn && !is_user) { > - xn = 1; > - } > - if (xn && access_type == 2) > - goto do_fault; > - > + if (regime_sctlr(env, mmu_idx) & SCTLR_AFE) { > /* The simplified model uses AP[0] as an access control bit. */ > - if ((regime_sctlr(env, mmu_idx) & SCTLR_AFE) > - && (ap & 1) == 0) { > + if ((ap & 1) == 0) { > /* Access flag fault. */ > code = (code == 15) ? 6 : 3; > goto do_fault; > } > - *prot = get_rw_prot(env, mmu_idx, is_user, ap, domain_prot); > - *prot |= *prot && !xn ? PAGE_EXEC : 0; > - if (!(*prot & (1 << access_type))) { > - /* Access permission fault. */ > - goto do_fault; > - } > + ap >>= 1; The original code didn't take into account that it may be calling check_ap with a simple AP, AP[2:1]. The code should have always been similar to the above, i.e. if (regime_sctlr(env, mmu_idx) & SCTLR_AFE) { /* The simplified model uses AP[0] as an access control bit. */ if ((ap & 1) == 0) { /* Access flag fault. */ code = (code == 15) ? 6 : 3; goto do_fault; } *prot = <handle simple AP somehow>; } else { *prot = check_ap(env, mmu_idx, ap, domain_prot, access_type); } if (!*prot) { /* Access permission fault. */ goto do_fault; } if (!xn) { *prot |= PAGE_EXEC; } 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. I should update the commit message to point this fix out. Or, actually, I should probably add another patch to the series (3/6), which addresses just this issue, and builds it on patch 2 "...to take simple AP". Peter, please let me know your preference. Thanks, drew > + } > + *prot = get_S1prot(env, mmu_idx, false, ap, domain_prot, 0, xn, pxn); > + if (!(*prot & (1 << access_type))) { > + /* Access permission fault. */ > + goto do_fault; > } > *phys_ptr = phys_addr; > return 0; > -- > 1.9.3 > > >