| Message ID | 1423753507-30542-2-git-send-email-drjones@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 12 February 2015 at 15:05, Andrew Jones <drjones@redhat.com> wrote: > Instead of mixing access permission checking with access permissions > to page protection flags translation, just do the translation, and > leave it to the caller to check the protection flags against the access > type. As this function only considers READ/WRITE, not EXEC, then name > it accordingly. > > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > target-arm/helper.c | 47 +++++++++++++++++------------------------------ > 1 file changed, 17 insertions(+), 30 deletions(-) > > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 1a1a00577e780..610f305c4d661 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -4692,34 +4692,23 @@ static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx) > } > } > > -/* Check section/page access permissions. > - Returns the page protection flags, or zero if the access is not > - permitted. */ > -static inline int check_ap(CPUARMState *env, ARMMMUIdx mmu_idx, > - int ap, int domain_prot, > - int access_type) > -{ > - int prot_ro; > +/* Translate section/page access permissions to page > + * R/W protection flags > + */ Given that the 'ap' parameter isn't just the AP bits this could use a mention in the comment: /* Translate section/page access permissions to page * R/W protection flags. The 'ap' parameter is the concatenation * of the APX:AP bits (with APX zero for the descriptor formats * which don't have it). */ Otherwise Reviewed-by: Peter Maydell <peter.maydell@linaro.org> -- PMM
On 12 February 2015 at 15:05, Andrew Jones <drjones@redhat.com> wrote: > + *prot = get_rw_prot(env, mmu_idx, ap, domain_prot); > + *prot |= *prot && !xn ? PAGE_EXEC : 0; I'm not a great fan of complicated ?: expressions, incidentally; I think this is clearer to read as if (*prot && !xn) { *prot |= PAGE_EXEC; } -- PMM
On 10 March 2015 at 15:07, Peter Maydell <peter.maydell@linaro.org> wrote: > Given that the 'ap' parameter isn't just the AP bits this > could use a mention in the comment: > > /* Translate section/page access permissions to page > * R/W protection flags. The 'ap' parameter is the concatenation > * of the APX:AP bits (with APX zero for the descriptor formats > * which don't have it). > */ Ignore this -- "APX" is an ARMv6-ism, and that bit is just AP[2] in v7 and up. -- PMM
On Tue, Mar 10, 2015 at 03:12:20PM +0000, Peter Maydell wrote: > On 12 February 2015 at 15:05, Andrew Jones <drjones@redhat.com> wrote: > > + *prot = get_rw_prot(env, mmu_idx, ap, domain_prot); > > + *prot |= *prot && !xn ? PAGE_EXEC : 0; > > I'm not a great fan of complicated ?: expressions, incidentally; > I think this is clearer to read as > if (*prot && !xn) { > *prot |= PAGE_EXEC; > } > > -- PMM OK, I'll change it if we need a v2. Thanks, drew
diff --git a/target-arm/helper.c b/target-arm/helper.c index 1a1a00577e780..610f305c4d661 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -4692,34 +4692,23 @@ static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx) } } -/* Check section/page access permissions. - Returns the page protection flags, or zero if the access is not - permitted. */ -static inline int check_ap(CPUARMState *env, ARMMMUIdx mmu_idx, - int ap, int domain_prot, - int access_type) -{ - int prot_ro; +/* Translate section/page access permissions to page + * R/W protection flags + */ +static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, + int ap, int domain_prot) +{ bool is_user = regime_is_user(env, mmu_idx); if (domain_prot == 3) { return PAGE_READ | PAGE_WRITE; } - if (access_type == 1) { - prot_ro = 0; - } else { - prot_ro = PAGE_READ; - } - switch (ap) { case 0: if (arm_feature(env, ARM_FEATURE_V7)) { return 0; } - if (access_type == 1) { - return 0; - } switch (regime_sctlr(env, mmu_idx) & (SCTLR_S | SCTLR_R)) { case SCTLR_S: return is_user ? 0 : PAGE_READ; @@ -4732,7 +4721,7 @@ static inline int check_ap(CPUARMState *env, ARMMMUIdx mmu_idx, return is_user ? 0 : PAGE_READ | PAGE_WRITE; case 2: if (is_user) { - return prot_ro; + return PAGE_READ; } else { return PAGE_READ | PAGE_WRITE; } @@ -4741,16 +4730,16 @@ static inline int check_ap(CPUARMState *env, ARMMMUIdx mmu_idx, case 4: /* Reserved. */ return 0; case 5: - return is_user ? 0 : prot_ro; + return is_user ? 0 : PAGE_READ; case 6: - return prot_ro; + return PAGE_READ; case 7: if (!arm_feature(env, ARM_FEATURE_V6K)) { return 0; } - return prot_ro; + return PAGE_READ; default: - abort(); + g_assert_not_reached(); } } @@ -4872,12 +4861,12 @@ static int get_phys_addr_v5(CPUARMState *env, uint32_t address, int access_type, } code = 15; } - *prot = check_ap(env, mmu_idx, ap, domain_prot, access_type); - if (!*prot) { + *prot = get_rw_prot(env, mmu_idx, ap, domain_prot); + *prot |= *prot ? PAGE_EXEC : 0; + if (!(*prot & (1 << access_type))) { /* Access permission fault. */ goto do_fault; } - *prot |= PAGE_EXEC; *phys_ptr = phys_addr; return 0; do_fault: @@ -4993,14 +4982,12 @@ static int get_phys_addr_v6(CPUARMState *env, uint32_t address, int access_type, code = (code == 15) ? 6 : 3; goto do_fault; } - *prot = check_ap(env, mmu_idx, ap, domain_prot, access_type); - if (!*prot) { + *prot = get_rw_prot(env, mmu_idx, ap, domain_prot); + *prot |= *prot && !xn ? PAGE_EXEC : 0; + if (!(*prot & (1 << access_type))) { /* Access permission fault. */ goto do_fault; } - if (!xn) { - *prot |= PAGE_EXEC; - } } *phys_ptr = phys_addr; return 0;
Instead of mixing access permission checking with access permissions to page protection flags translation, just do the translation, and leave it to the caller to check the protection flags against the access type. As this function only considers READ/WRITE, not EXEC, then name it accordingly. Signed-off-by: Andrew Jones <drjones@redhat.com> --- target-arm/helper.c | 47 +++++++++++++++++------------------------------ 1 file changed, 17 insertions(+), 30 deletions(-)