| Message ID | 20240317221431.251515-4-svens@stackframe.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | few fixes for hppa target | expand |
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; > } > } >
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~
Richard Henderson <richard.henderson@linaro.org> writes: > On 3/17/24 12:14, Sven Schnelle wrote: >> /* 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. Oops, indeed. > Therefore I suggest > [..] > 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. Thanks, i'll take your version and update the patch.
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 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; + } *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; } }
PA2.0 provides 8 instead of 4 PID registers. Signed-off-by: Sven Schnelle <svens@stackframe.org> --- roms/SLOF | 2 +- target/hppa/mem_helper.c | 67 +++++++++++++++++++++++++++++++++++----- 2 files changed, 60 insertions(+), 9 deletions(-)