On Fri, Jan 16, 2015 at 02:52:21PM +0000, Peter Maydell wrote: > On 13 January 2015 at 15:48, Andrew Jones <drjo...@redhat.com> wrote: > > Table D4-32 shows that execute access from EL0 doesn't depend > > on AP[1]. > > This commit message is a bit sparse, which confused me > for a bit. It would be worth beefing it up a bit: > > target-arm: 64-bit EL0 code can execute from unreadable pages > > In AArch64 mode, a page can be executable even if it is not > readable (a difference from AArch32). Instead of bailing out > early if the page is not readable, just add "32 bit and > page not readable" to the list of conditions that make a > page non-executable, and check whether the protections and > the access type are compatible once at the end of the function.
OK > > > Signed-off-by: Andrew Jones <drjo...@redhat.com> > > --- > > target-arm/helper.c | 27 ++++++++++++++++----------- > > 1 file changed, 16 insertions(+), 11 deletions(-) > > > > diff --git a/target-arm/helper.c b/target-arm/helper.c > > index 3ef0f1f38eda5..7c30a2669a0f2 100644 > > --- a/target-arm/helper.c > > +++ b/target-arm/helper.c > > @@ -4787,7 +4787,7 @@ static int get_phys_addr_lpae(CPUARMState *env, > > target_ulong address, > > hwaddr descaddr, descmask; > > uint32_t tableattrs; > > target_ulong page_size; > > - uint32_t attrs; > > + uint32_t attrs, ap; > > int32_t granule_sz = 9; > > int32_t va_size = 32; > > int32_t tbi = 0; > > @@ -4952,14 +4952,20 @@ static int get_phys_addr_lpae(CPUARMState *env, > > target_ulong address, > > /* Access flag */ > > goto do_fault; > > } > > + > > fault_type = permission_fault; > > - if (is_user && !(attrs & (1 << 4))) { > > - /* Unprivileged access not enabled */ > > - goto do_fault; > > + ap = extract32(attrs, 4, 2); /* AP[2:1] */ > > + > > + *prot = 0; > > + if (!is_user || (ap & 1)) { > > + *prot |= PAGE_READ; > > + *prot |= !(ap & 2) ? PAGE_WRITE : 0; > > Personally I would find > if (!(ap & 2)) { > *prot |= PAGE_WRITE; > } > > clearer. OK > > > } > > - *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC; > > + > > + *prot |= PAGE_EXEC; > > if ((arm_feature(env, ARM_FEATURE_V8) && is_user && (attrs & (1 << > > 12))) || > > (!arm_feature(env, ARM_FEATURE_V8) && (attrs & (1 << 12))) || > > + (!arm_el_is_aa64(env, 1) && is_user && !(ap & 1)) || > > (!is_user && (attrs & (1 << 11)))) { > > /* XN/UXN or PXN. Since we only implement EL0/EL1 we > > unconditionally > > * treat XN/UXN as UXN for v8. > > @@ -4969,12 +4975,11 @@ static int get_phys_addr_lpae(CPUARMState *env, > > target_ulong address, > > } > > There is a "if access_type == 2 goto do_fault" check just > above this hunk which you can delete, because we're now > doing that check in the code you add below. Right. Will do, alternatively I should have brought the PAGE_EXEC handling below in with patch 2/2, which was my plan, but forgot to split it out. > > > *prot &= ~PAGE_EXEC; > > } > > - if (attrs & (1 << 5)) { > > - /* Write access forbidden */ > > - if (access_type == 1) { > > - goto do_fault; > > - } > > - *prot &= ~PAGE_WRITE; > > + > > + if ((*prot == 0) > > + || (!(*prot & PAGE_WRITE) && access_type == 1) > > + || (!(*prot & PAGE_EXEC) && access_type == 2)) { > > + goto do_fault; > > Why isn't this just > if (!(*prot & (1 << access_type))) { yeah, that would be better > > ? (Or at least, why doesn't it treat PAGE_READ the same way > as the other two bits?) As it is I think we'll treat a page > that is marked exec-not-readable as if it were readable. Oh yes, we should check PAGE_READ as well Thanks for the review. I see from another mail that you'll be sending some patches I should base the next version on. So I'll hold off on sending a revised patch until I see that. drew