Re: [Qemu-devel] [PATCH 5/5] target-arm: apply get_S1prot to get_phys_addr_v6

[Andrew Jones <drjones@redhat.com>]

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 <drjones@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
> 
> 
>