Re: [Qemu-devel] [PATCH 2/5] target-arm: enable get_rw_prot to take simple AP

[Peter Maydell]

On 12 February 2015 at 15:05, Andrew Jones <address@hidden> wrote:
> Teach get_rw_prot about the simple AP format AP[2:1]. An additional
> switch was added, as opposed to converting ap := AP[2:1] to AP[2:0]
> with a simple shift - and then modifying cases 0,2,4,6, because the
> resulting code is easier to read with the switch.
>
> Signed-off-by: Andrew Jones <address@hidden>
> ---
>  target-arm/helper.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 610f305c4d661..b63ec7b7979f9 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4698,12 +4698,32 @@ static inline bool regime_is_user(CPUARMState *env, 
> ARMMMUIdx mmu_idx)
>  static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
>                                int ap, int domain_prot)
>  {
> +    bool simple_ap = regime_using_lpae_format(env, mmu_idx)
> +                         || (regime_sctlr(env, mmu_idx) & SCTLR_AFE);

We should check arm_feature(env, ARM_FEATURE_V6K) && (SCTLR.AFE is set);
that bit isn't defined til v6K.

> +    bool domain_prot_valid = !regime_using_lpae_format(env, mmu_idx);

Given that the lpae code path is totally separate (and not even
calling this function yet), can't you just have it pass in a
zero domain_prot ? Or have the callers do the domain protection
check themselves...

>      bool is_user = regime_is_user(env, mmu_idx);
>
> -    if (domain_prot == 3) {
> +    if (domain_prot_valid && domain_prot == 3) {
>          return PAGE_READ | PAGE_WRITE;
>      }
>
> +    /* ap is AP[2:1] */
> +    if (simple_ap) {
> +        switch (ap) {
> +        case 0:
> +            return is_user ? 0 : PAGE_READ | PAGE_WRITE;
> +        case 1:
> +            return PAGE_READ | PAGE_WRITE;
> +        case 2:
> +            return is_user ? 0 : PAGE_READ;
> +        case 3:
> +            return PAGE_READ;
> +        default:
> +            g_assert_not_reached();
> +        }
> +    }

I'm confused. Even if we're using the simple-permissions
model, the ap parameter is still AP[2:0]. Shouldn't this
switch be for cases 0, 2, 4, 6 ?

> +
> +    /* ap is AP[2:0] */
>      switch (ap) {
>      case 0:
>          if (arm_feature(env, ARM_FEATURE_V7)) {
> --
> 1.9.3
>

-- PMM