On Fri, Jan 16, 2015 at 02:52:21PM +, Peter Maydell wrote:
> On 13 January 2015 at 15:48, Andrew Jones 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
> > ---
> > 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