Re: [PATCH 3/7] target/hppa: fix access_id check

[Richard Henderson]

On 3/17/24 12:14, Sven Schnelle wrote:
> +static bool match_prot_id(CPUHPPAState *env, uint32_t access_id, uint32_t 
> *_pid)
> +{
> +    for (int i = 0; i < 8; i++) {
> +        uint32_t pid = get_pid(env, i);

There are only 4 pid's for pa1.x.

> +static uint32_t get_pid(CPUHPPAState *env, int num)
> +{
> +    const struct pid_map {
> +        int reg;
> +        bool shift;
> +    } *pid;
> +
> +    const struct pid_map pids64[] = {
> +        { .reg = 8,  .shift = true  },
> +        { .reg = 8,  .shift = false },
> +        { .reg = 9,  .shift = true  },
> +        { .reg = 9,  .shift = false },
> +        { .reg = 12, .shift = true  },
> +        { .reg = 12, .shift = false },
> +        { .reg = 13, .shift = true  },
> +        { .reg = 13, .shift = false }
> +    };
> +
> +    const struct pid_map pids32[] = {
> +        { .reg = 8,  .shift = false  },
> +        { .reg = 9,  .shift = false  },
> +        { .reg = 12, .shift = false  },
> +        { .reg = 13, .shift = false  },
> +    };
> +
> +    if (hppa_is_pa20(env)) {

This predicate is fairly expensive -- you don't want to put it deep inside a 
loop.
The table is very predictable.  Moreover, you don't need to test these in any particular 
order.

>          /* If bits [31:1] match, and bit 0 is set, suppress write.  */
> -        int match = ent->access_id * 2 + 1;
> -
> -        if (match == env->cr[CR_PID1] || match == env->cr[CR_PID2] ||
> -            match == env->cr[CR_PID3] || match == env->cr[CR_PID4]) {
> -            prot &= PAGE_READ | PAGE_EXEC;
> -            if (type == PAGE_WRITE) {
> -                ret = EXCP_DMPI;
> -                goto egress;
> +        uint32_t pid;
> +        if (match_prot_id(env, ent->access_id, &pid)) {
> +            if ((pid & 1) && (prot & PROT_WRITE)) {
> +                prot &= ~PROT_WRITE;
>              }
> +        } else {
> +            prot = 0;
>          }

You're losing the data memory protection id trap.

Therefore I suggest

/* Return the set of protections allowed by a PID match. */
static int match_prot_id_1(uint32_t access_id, uint32_t prot_id)
{
    if (((access_id ^ (prot_id >> 1) & ACCESS_ID_MASK) == 0) {
        return (prot_id & 1
                ? PROT_EXEC | PROT_READ
                : PROT_EXEC | PROT_READ | PROT_WRITE);
    }
    return 0;
}

static int match_prot_id32(CPUHPPAState *env, uint32_t access_id)
{
    int r, i;
    for (i = CR_PID1; i <= CR_PID4; ++i) {
        r = match_prot_id_1(access_id, env->cr[i]);
        if (r) {
            return r;
        }
    }
    return 0;
}

static int match_prot_id64(CPUHPPAState *env, uint32_t access_id)
{
    int r, i;
    for (i = CR_PID1; i <= CR_PID4; ++i) {
        r = match_prot_id_1(access_id, env->cr[i]);
        if (r) {
            return r;
        }
        r = match_prot_id_1(access_id, env->cr[i] >> 32);
        if (r) {
            return r;
        }
    }
    return 0;
}

---

    if (ent->access_id && MMU_IDX_TO_P(mmu_idx)) {
        int access_prot = (hppa_is_pa20(env)
                           ? match_prot_id64(env, ent->access_id)
                           : match_prot_id32(env, ent->access_id));
        if (prot & ~access_prot) {
            ret = EXCP_DMPI;
            goto egress;
        }
    }

At this point there are now a couple of hppa_is_pa20() calls within 
hppa_get_physical_address, which could be unified to a single local bool.


r~