Re: [PATCH v2 1/2] arm/ptw: make get_S1prot accept decoded AP

[Peter Maydell]

On Sun, 17 Nov 2024 at 13:50, Pavel Skripkin <paskripkin@gmail.com> wrote:
>
> AP in armv7 short descriptor mode has 3 bits and also domain, which
> makes it incompatible with other arm schemas.
>
> To make it possible to share get_S1prot between armv8, armv7 long
> format, armv7 short format and armv6 it's easier to make caller
> decode AP.

Yep, I agree that this is the best approach to sharing the
function between short and long desc.

> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> ---
>  target/arm/ptw.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 9849949508..50eed0f811 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -1357,25 +1357,24 @@ static int get_S2prot(CPUARMState *env, int s2ap, int 
> xn, bool s1_is_el0)
>   * @env:     CPUARMState
>   * @mmu_idx: MMU index indicating required translation regime
>   * @is_aa64: TRUE if AArch64
> - * @ap:      The 2-bit simple AP (AP[2:1])
> + * @user_rw: Translated AP for user access
> + * @prot_rw: Translated AP for privileged access
>   * @xn:      XN (execute-never) bit
>   * @pxn:     PXN (privileged execute-never) bit
>   * @in_pa:   The original input pa space
>   * @out_pa:  The output pa space, modified by NSTable, NS, and NSE
>   */
>  static int get_S1prot(CPUARMState *env, ARMMMUIdx mmu_idx, bool is_aa64,
> -                      int ap, int xn, int pxn,
> +                      int user_rw, int prot_rw, int xn, int pxn,
>                        ARMSecuritySpace in_pa, ARMSecuritySpace out_pa)
>  {
>      ARMCPU *cpu = env_archcpu(env);
>      bool is_user = regime_is_user(env, mmu_idx);
> -    int prot_rw, user_rw;
>      bool have_wxn;
>      int wxn = 0;
>
>      assert(!regime_is_stage2(mmu_idx));
>
> -    user_rw = simple_ap_to_rw_prot_is_user(ap, true);
>      if (is_user) {
>          prot_rw = user_rw;
>      } else {
> @@ -1393,8 +1392,6 @@ static int get_S1prot(CPUARMState *env, ARMMMUIdx 
> mmu_idx, bool is_aa64,
>                     regime_is_pan(env, mmu_idx) &&
>                     (regime_sctlr(env, mmu_idx) & SCTLR_EPAN) && !xn) {
>              prot_rw = 0;
> -        } else {
> -            prot_rw = simple_ap_to_rw_prot_is_user(ap, false);
>          }
>      }
>
> @@ -2044,6 +2041,8 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> S1Translate *ptw,
>          int nse, ns = extract32(attrs, 5, 1);
>          uint8_t attrindx;
>          uint64_t mair;
> +        int ap_usr = simple_ap_to_rw_prot_is_user(ap, true);
> +        int ap_priv = simple_ap_to_rw_prot_is_user(ap, false);

Doing the calls here is too early, because after this point
there is a FEAT_NV case which may change the value of ap.

Also, could we give the variables the same name as the
get_S1prot() argument names, for consistency? (The return
values from simple_ap_to_rw_prot_is_user() are not ap bit
fields, they're rw indications, so the S1prot argument names
seem more appropriate here anyway.)

>          switch (out_space) {
>          case ARMSS_Root:
> @@ -2114,7 +2113,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> S1Translate *ptw,
>           * Note that we modified ptw->in_space earlier for NSTable, but
>           * result->f.attrs retains a copy of the original security space.
>           */
> -        result->f.prot = get_S1prot(env, mmu_idx, aarch64, ap, xn, pxn,
> +        result->f.prot = get_S1prot(env, mmu_idx, aarch64, ap_usr, ap_priv, 
> xn, pxn,
>                                      result->f.attrs.space, out_space);
>
>          /* Index into MAIR registers for cache attributes */

thanks
-- PMM