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

[Helge Deller]

On 3/17/24 23:14, Sven Schnelle wrote:
> PA2.0 provides 8 instead of 4 PID registers.
>
> Signed-off-by: Sven Schnelle <svens@stackframe.org>

Reviewed-by: Helge Deller <deller@gmx.de>
with a few comments below...

Helge

> ---
>   roms/SLOF                |  2 +-
>   target/hppa/mem_helper.c | 67 +++++++++++++++++++++++++++++++++++-----
>   2 files changed, 60 insertions(+), 9 deletions(-)
>
> diff --git a/roms/SLOF b/roms/SLOF
> index 3a259df244..6b6c16b4b4 160000
> --- a/roms/SLOF
> +++ b/roms/SLOF
> @@ -1 +1 @@
> -Subproject commit 3a259df2449fc4a4e43ab5f33f0b2c66484b4bc3
> +Subproject commit 6b6c16b4b40763507cf1f518096f3c3883c5cf2d

this doesn't belong here.


> diff --git a/target/hppa/mem_helper.c b/target/hppa/mem_helper.c
> index 80f51e753f..e4e3f6cdbe 100644
> --- a/target/hppa/mem_helper.c
> +++ b/target/hppa/mem_helper.c
> @@ -152,6 +152,59 @@ static HPPATLBEntry *hppa_alloc_tlb_ent(CPUHPPAState *env)
>       return ent;
>   }
>
> +static uint32_t get_pid(CPUHPPAState *env, int num)
> +{
> +    const struct pid_map {
> +        int reg;
> +        bool shift;

does it makes sense to condense it, e.g.:
 +        unsigned char reg:7,
 +        unsigned char shift:1;

Helge


> +    } *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)) {
> +        pid = pids64 + num;
> +    } else {
> +        pid = pids32 + num;
> +    }
> +    uint64_t cr = env->cr[pid->reg];
> +    if (pid->shift) {
> +        cr >>= 32;
> +    } else {
> +        cr &= 0xffffffff;
> +    }
> +    return cr;
> +}
> +
> +#define ACCESS_ID_MASK 0xffff
> +
> +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);
> +        if ((access_id & ACCESS_ID_MASK) == ((pid >> 1) & ACCESS_ID_MASK)) {
> +            *_pid = pid;
> +            return true;
> +        }
> +    }
> +    return false;
> +}
> +
>   int hppa_get_physical_address(CPUHPPAState *env, vaddr addr, int mmu_idx,
>                                 int type, hwaddr *pphys, int *pprot,
>                                 HPPATLBEntry **tlb_entry)
> @@ -227,15 +280,13 @@ int hppa_get_physical_address(CPUHPPAState *env, vaddr 
> addr, int mmu_idx,
>       /* access_id == 0 means public page and no check is performed */
>       if (ent->access_id && MMU_IDX_TO_P(mmu_idx)) {
>           /* 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;
>           }
>       }