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(-)
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;
}
}
--
2.43.2On 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.
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;
> }
> }
>
© 2016 - 2026 Red Hat, Inc.