Checking if a page protection bit is set for a given access type is a
common operation. Add a macro to avoid repeating the same check at
multiple places and also avoid a function call. As this relies on
access type and page protection bit values having certain relation
also add an assert to ensure that this assumption holds.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
target/ppc/cpu_init.c | 4 ++++
target/ppc/internal.h | 20 ++------------------
target/ppc/mmu-hash32.c | 6 +++---
target/ppc/mmu-hash64.c | 2 +-
target/ppc/mmu-radix64.c | 2 +-
target/ppc/mmu_common.c | 26 +++++++++++++-------------
6 files changed, 24 insertions(+), 36 deletions(-)
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 92c71b2a09..6639235544 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -7377,6 +7377,10 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
resettable_class_set_parent_phases(rc, NULL, ppc_cpu_reset_hold, NULL,
&pcc->parent_phases);
+ /* CHECK_PROT_ACCESS relies on this MMU access and PAGE bits relation */
+ assert(MMU_DATA_LOAD == 0 && MMU_DATA_STORE == 1 && MMU_INST_FETCH == 2 &&
+ PAGE_READ == 1 && PAGE_WRITE == 2 && PAGE_EXEC == 4);
+
cc->class_by_name = ppc_cpu_class_by_name;
cc->has_work = ppc_cpu_has_work;
cc->mmu_index = ppc_cpu_mmu_index;
diff --git a/target/ppc/internal.h b/target/ppc/internal.h
index 46176c4711..9880422ce3 100644
--- a/target/ppc/internal.h
+++ b/target/ppc/internal.h
@@ -234,24 +234,8 @@ void destroy_ppc_opcodes(PowerPCCPU *cpu);
void ppc_gdb_init(CPUState *cs, PowerPCCPUClass *ppc);
const gchar *ppc_gdb_arch_name(CPUState *cs);
-/**
- * prot_for_access_type:
- * @access_type: Access type
- *
- * Return the protection bit required for the given access type.
- */
-static inline int prot_for_access_type(MMUAccessType access_type)
-{
- switch (access_type) {
- case MMU_INST_FETCH:
- return PAGE_EXEC;
- case MMU_DATA_LOAD:
- return PAGE_READ;
- case MMU_DATA_STORE:
- return PAGE_WRITE;
- }
- g_assert_not_reached();
-}
+/* Check if permission bit required for the access_type is set in prot */
+#define CHECK_PROT_ACCESS(prot, access_type) ((prot) & (1 << (access_type)))
#ifndef CONFIG_USER_ONLY
diff --git a/target/ppc/mmu-hash32.c b/target/ppc/mmu-hash32.c
index b5d7aeed4e..fa4a4ced6d 100644
--- a/target/ppc/mmu-hash32.c
+++ b/target/ppc/mmu-hash32.c
@@ -213,7 +213,7 @@ static bool ppc_hash32_direct_store(PowerPCCPU *cpu, target_ulong sr,
}
*prot = key ? PAGE_READ | PAGE_WRITE : PAGE_READ;
- if (*prot & prot_for_access_type(access_type)) {
+ if (CHECK_PROT_ACCESS(*prot, access_type)) {
*raddr = eaddr;
return true;
}
@@ -364,7 +364,7 @@ bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
if (env->nb_BATs != 0) {
raddr = ppc_hash32_bat_lookup(cpu, eaddr, access_type, protp, mmu_idx);
if (raddr != -1) {
- if (prot_for_access_type(access_type) & ~*protp) {
+ if (!CHECK_PROT_ACCESS(*protp, access_type)) {
if (guest_visible) {
if (access_type == MMU_INST_FETCH) {
cs->exception_index = POWERPC_EXCP_ISI;
@@ -432,7 +432,7 @@ bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
prot = ppc_hash32_pte_prot(mmu_idx, sr, pte);
- if (prot_for_access_type(access_type) & ~prot) {
+ if (!CHECK_PROT_ACCESS(prot, access_type)) {
/* Access right violation */
qemu_log_mask(CPU_LOG_MMU, "PTE access rejected\n");
if (guest_visible) {
diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index 5a0d80feda..14c2116ae7 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -1097,7 +1097,7 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
amr_prot = ppc_hash64_amr_prot(cpu, pte);
prot = exec_prot & pp_prot & amr_prot;
- need_prot = prot_for_access_type(access_type);
+ need_prot = CHECK_PROT_ACCESS(PAGE_RWX, access_type);
if (need_prot & ~prot) {
/* Access right violation */
qemu_log_mask(CPU_LOG_MMU, "PTE access rejected\n");
diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
index 395ce3b782..a72cd927c4 100644
--- a/target/ppc/mmu-radix64.c
+++ b/target/ppc/mmu-radix64.c
@@ -209,7 +209,7 @@ static bool ppc_radix64_check_prot(PowerPCCPU *cpu, MMUAccessType access_type,
}
/* Check if requested access type is allowed */
- if (prot_for_access_type(access_type) & ~*prot) {
+ if (!CHECK_PROT_ACCESS(*prot, access_type)) {
/* Page Protected for that Access */
*fault_cause |= access_type == MMU_INST_FETCH ? SRR1_NOEXEC_GUARD :
DSISR_PROTFAULT;
diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
index eff015066d..24d68926b4 100644
--- a/target/ppc/mmu_common.c
+++ b/target/ppc/mmu_common.c
@@ -76,11 +76,6 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
/*****************************************************************************/
/* PowerPC MMU emulation */
-static int check_prot(int prot, MMUAccessType access_type)
-{
- return prot & prot_for_access_type(access_type) ? 0 : -2;
-}
-
int ppc6xx_tlb_getnum(CPUPPCState *env, target_ulong eaddr,
int way, int is_code)
{
@@ -125,13 +120,14 @@ static int ppc6xx_tlb_pte_check(mmu_ctx_t *ctx, target_ulong pte0,
/* Keep the matching PTE information */
ctx->raddr = pte1;
ctx->prot = ppc_hash32_pp_prot(ctx->key, pp, ctx->nx);
- ret = check_prot(ctx->prot, access_type);
- if (ret == 0) {
+ if (CHECK_PROT_ACCESS(ctx->prot, access_type)) {
/* Access granted */
qemu_log_mask(CPU_LOG_MMU, "PTE access granted !\n");
+ ret = 0;
} else {
/* Access right violation */
qemu_log_mask(CPU_LOG_MMU, "PTE access rejected\n");
+ ret = -2;
}
}
}
@@ -317,12 +313,14 @@ static int get_bat_6xx_tlb(CPUPPCState *env, mmu_ctx_t *ctx,
(virtual & 0x0001F000);
/* Compute access rights */
ctx->prot = prot;
- ret = check_prot(ctx->prot, access_type);
- if (ret == 0) {
+ if (CHECK_PROT_ACCESS(ctx->prot, access_type)) {
qemu_log_mask(CPU_LOG_MMU, "BAT %d match: r " HWADDR_FMT_plx
" prot=%c%c\n", i, ctx->raddr,
ctx->prot & PAGE_READ ? 'R' : '-',
ctx->prot & PAGE_WRITE ? 'W' : '-');
+ ret = 0;
+ } else {
+ ret = -2;
}
break;
}
@@ -540,9 +538,11 @@ static int mmu40x_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx,
check_perms:
/* Check from TLB entry */
ctx->prot = tlb->prot;
- ret = check_prot(ctx->prot, access_type);
- if (ret == -2) {
+ if (CHECK_PROT_ACCESS(ctx->prot, access_type)) {
+ ret = 0;
+ } else {
env->spr[SPR_40x_ESR] = 0;
+ ret = -2;
}
break;
}
@@ -607,7 +607,7 @@ static int mmubooke_check_tlb(CPUPPCState *env, ppcemb_tlb_t *tlb,
} else {
*prot = (tlb->prot >> 4) & 0xF;
}
- if (*prot & prot_for_access_type(access_type)) {
+ if (CHECK_PROT_ACCESS(*prot, access_type)) {
qemu_log_mask(CPU_LOG_MMU, "%s: good TLB!\n", __func__);
return 0;
}
@@ -811,7 +811,7 @@ found_tlb:
*prot |= PAGE_EXEC;
}
}
- if (*prot & prot_for_access_type(access_type)) {
+ if (CHECK_PROT_ACCESS(*prot, access_type)) {
qemu_log_mask(CPU_LOG_MMU, "%s: good TLB!\n", __func__);
return 0;
}
--
2.30.9On Wed May 8, 2024 at 10:15 AM AEST, BALATON Zoltan wrote:
> Checking if a page protection bit is set for a given access type is a
> common operation. Add a macro to avoid repeating the same check at
> multiple places and also avoid a function call. As this relies on
> access type and page protection bit values having certain relation
> also add an assert to ensure that this assumption holds.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> target/ppc/cpu_init.c | 4 ++++
> target/ppc/internal.h | 20 ++------------------
> target/ppc/mmu-hash32.c | 6 +++---
> target/ppc/mmu-hash64.c | 2 +-
> target/ppc/mmu-radix64.c | 2 +-
> target/ppc/mmu_common.c | 26 +++++++++++++-------------
> 6 files changed, 24 insertions(+), 36 deletions(-)
>
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 92c71b2a09..6639235544 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -7377,6 +7377,10 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
> resettable_class_set_parent_phases(rc, NULL, ppc_cpu_reset_hold, NULL,
> &pcc->parent_phases);
>
> + /* CHECK_PROT_ACCESS relies on this MMU access and PAGE bits relation */
> + assert(MMU_DATA_LOAD == 0 && MMU_DATA_STORE == 1 && MMU_INST_FETCH == 2 &&
> + PAGE_READ == 1 && PAGE_WRITE == 2 && PAGE_EXEC == 4);
> +
Can you use qemu_build_assert() for this?
> cc->class_by_name = ppc_cpu_class_by_name;
> cc->has_work = ppc_cpu_has_work;
> cc->mmu_index = ppc_cpu_mmu_index;
> diff --git a/target/ppc/internal.h b/target/ppc/internal.h
> index 46176c4711..9880422ce3 100644
> --- a/target/ppc/internal.h
> +++ b/target/ppc/internal.h
> @@ -234,24 +234,8 @@ void destroy_ppc_opcodes(PowerPCCPU *cpu);
> void ppc_gdb_init(CPUState *cs, PowerPCCPUClass *ppc);
> const gchar *ppc_gdb_arch_name(CPUState *cs);
>
> -/**
> - * prot_for_access_type:
> - * @access_type: Access type
> - *
> - * Return the protection bit required for the given access type.
> - */
> -static inline int prot_for_access_type(MMUAccessType access_type)
> -{
> - switch (access_type) {
> - case MMU_INST_FETCH:
> - return PAGE_EXEC;
> - case MMU_DATA_LOAD:
> - return PAGE_READ;
> - case MMU_DATA_STORE:
> - return PAGE_WRITE;
> - }
> - g_assert_not_reached();
> -}
> +/* Check if permission bit required for the access_type is set in prot */
> +#define CHECK_PROT_ACCESS(prot, access_type) ((prot) & (1 << (access_type)))
We don't want to use a macro when an inline function will work.
Does the compiler not see the pattern and transform the existing
code into a shift? If it does then I would leave it. If not, then
just keep prot_for_access_type but make it a shift and maybe
comment the logic.
I would call the new function check_prot_for_access_type().
>
> #ifndef CONFIG_USER_ONLY
>
> diff --git a/target/ppc/mmu-hash32.c b/target/ppc/mmu-hash32.c
> index b5d7aeed4e..fa4a4ced6d 100644
> --- a/target/ppc/mmu-hash32.c
> +++ b/target/ppc/mmu-hash32.c
> @@ -213,7 +213,7 @@ static bool ppc_hash32_direct_store(PowerPCCPU *cpu, target_ulong sr,
> }
>
> *prot = key ? PAGE_READ | PAGE_WRITE : PAGE_READ;
> - if (*prot & prot_for_access_type(access_type)) {
> + if (CHECK_PROT_ACCESS(*prot, access_type)) {
> *raddr = eaddr;
> return true;
> }
This does read better, and better than the check_prot() below
that it replaces too, so I like the cleanup.
Thanks,
Nick
> @@ -364,7 +364,7 @@ bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> if (env->nb_BATs != 0) {
> raddr = ppc_hash32_bat_lookup(cpu, eaddr, access_type, protp, mmu_idx);
> if (raddr != -1) {
> - if (prot_for_access_type(access_type) & ~*protp) {
> + if (!CHECK_PROT_ACCESS(*protp, access_type)) {
> if (guest_visible) {
> if (access_type == MMU_INST_FETCH) {
> cs->exception_index = POWERPC_EXCP_ISI;
> @@ -432,7 +432,7 @@ bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
>
> prot = ppc_hash32_pte_prot(mmu_idx, sr, pte);
>
> - if (prot_for_access_type(access_type) & ~prot) {
> + if (!CHECK_PROT_ACCESS(prot, access_type)) {
> /* Access right violation */
> qemu_log_mask(CPU_LOG_MMU, "PTE access rejected\n");
> if (guest_visible) {
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index 5a0d80feda..14c2116ae7 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -1097,7 +1097,7 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> amr_prot = ppc_hash64_amr_prot(cpu, pte);
> prot = exec_prot & pp_prot & amr_prot;
>
> - need_prot = prot_for_access_type(access_type);
> + need_prot = CHECK_PROT_ACCESS(PAGE_RWX, access_type);
> if (need_prot & ~prot) {
> /* Access right violation */
> qemu_log_mask(CPU_LOG_MMU, "PTE access rejected\n");
> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
> index 395ce3b782..a72cd927c4 100644
> --- a/target/ppc/mmu-radix64.c
> +++ b/target/ppc/mmu-radix64.c
> @@ -209,7 +209,7 @@ static bool ppc_radix64_check_prot(PowerPCCPU *cpu, MMUAccessType access_type,
> }
>
> /* Check if requested access type is allowed */
> - if (prot_for_access_type(access_type) & ~*prot) {
> + if (!CHECK_PROT_ACCESS(*prot, access_type)) {
> /* Page Protected for that Access */
> *fault_cause |= access_type == MMU_INST_FETCH ? SRR1_NOEXEC_GUARD :
> DSISR_PROTFAULT;
> diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
> index eff015066d..24d68926b4 100644
> --- a/target/ppc/mmu_common.c
> +++ b/target/ppc/mmu_common.c
> @@ -76,11 +76,6 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
> /*****************************************************************************/
> /* PowerPC MMU emulation */
>
> -static int check_prot(int prot, MMUAccessType access_type)
> -{
> - return prot & prot_for_access_type(access_type) ? 0 : -2;
> -}
> -
> int ppc6xx_tlb_getnum(CPUPPCState *env, target_ulong eaddr,
> int way, int is_code)
> {
> @@ -125,13 +120,14 @@ static int ppc6xx_tlb_pte_check(mmu_ctx_t *ctx, target_ulong pte0,
> /* Keep the matching PTE information */
> ctx->raddr = pte1;
> ctx->prot = ppc_hash32_pp_prot(ctx->key, pp, ctx->nx);
> - ret = check_prot(ctx->prot, access_type);
> - if (ret == 0) {
> + if (CHECK_PROT_ACCESS(ctx->prot, access_type)) {
> /* Access granted */
> qemu_log_mask(CPU_LOG_MMU, "PTE access granted !\n");
> + ret = 0;
> } else {
> /* Access right violation */
> qemu_log_mask(CPU_LOG_MMU, "PTE access rejected\n");
> + ret = -2;
> }
> }
> }
> @@ -317,12 +313,14 @@ static int get_bat_6xx_tlb(CPUPPCState *env, mmu_ctx_t *ctx,
> (virtual & 0x0001F000);
> /* Compute access rights */
> ctx->prot = prot;
> - ret = check_prot(ctx->prot, access_type);
> - if (ret == 0) {
> + if (CHECK_PROT_ACCESS(ctx->prot, access_type)) {
> qemu_log_mask(CPU_LOG_MMU, "BAT %d match: r " HWADDR_FMT_plx
> " prot=%c%c\n", i, ctx->raddr,
> ctx->prot & PAGE_READ ? 'R' : '-',
> ctx->prot & PAGE_WRITE ? 'W' : '-');
> + ret = 0;
> + } else {
> + ret = -2;
> }
> break;
> }
> @@ -540,9 +538,11 @@ static int mmu40x_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx,
> check_perms:
> /* Check from TLB entry */
> ctx->prot = tlb->prot;
> - ret = check_prot(ctx->prot, access_type);
> - if (ret == -2) {
> + if (CHECK_PROT_ACCESS(ctx->prot, access_type)) {
> + ret = 0;
> + } else {
> env->spr[SPR_40x_ESR] = 0;
> + ret = -2;
> }
> break;
> }
> @@ -607,7 +607,7 @@ static int mmubooke_check_tlb(CPUPPCState *env, ppcemb_tlb_t *tlb,
> } else {
> *prot = (tlb->prot >> 4) & 0xF;
> }
> - if (*prot & prot_for_access_type(access_type)) {
> + if (CHECK_PROT_ACCESS(*prot, access_type)) {
> qemu_log_mask(CPU_LOG_MMU, "%s: good TLB!\n", __func__);
> return 0;
> }
> @@ -811,7 +811,7 @@ found_tlb:
> *prot |= PAGE_EXEC;
> }
> }
> - if (*prot & prot_for_access_type(access_type)) {
> + if (CHECK_PROT_ACCESS(*prot, access_type)) {
> qemu_log_mask(CPU_LOG_MMU, "%s: good TLB!\n", __func__);
> return 0;
> }
On Wed, 8 May 2024, Nicholas Piggin wrote:
> On Wed May 8, 2024 at 10:15 AM AEST, BALATON Zoltan wrote:
>> Checking if a page protection bit is set for a given access type is a
>> common operation. Add a macro to avoid repeating the same check at
>> multiple places and also avoid a function call. As this relies on
>> access type and page protection bit values having certain relation
>> also add an assert to ensure that this assumption holds.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> target/ppc/cpu_init.c | 4 ++++
>> target/ppc/internal.h | 20 ++------------------
>> target/ppc/mmu-hash32.c | 6 +++---
>> target/ppc/mmu-hash64.c | 2 +-
>> target/ppc/mmu-radix64.c | 2 +-
>> target/ppc/mmu_common.c | 26 +++++++++++++-------------
>> 6 files changed, 24 insertions(+), 36 deletions(-)
>>
>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
>> index 92c71b2a09..6639235544 100644
>> --- a/target/ppc/cpu_init.c
>> +++ b/target/ppc/cpu_init.c
>> @@ -7377,6 +7377,10 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>> resettable_class_set_parent_phases(rc, NULL, ppc_cpu_reset_hold, NULL,
>> &pcc->parent_phases);
>>
>> + /* CHECK_PROT_ACCESS relies on this MMU access and PAGE bits relation */
>> + assert(MMU_DATA_LOAD == 0 && MMU_DATA_STORE == 1 && MMU_INST_FETCH == 2 &&
>> + PAGE_READ == 1 && PAGE_WRITE == 2 && PAGE_EXEC == 4);
>> +
>
> Can you use qemu_build_assert() for this?
I've changed it to qemu_build_assert and seems to work.
>> cc->class_by_name = ppc_cpu_class_by_name;
>> cc->has_work = ppc_cpu_has_work;
>> cc->mmu_index = ppc_cpu_mmu_index;
>> diff --git a/target/ppc/internal.h b/target/ppc/internal.h
>> index 46176c4711..9880422ce3 100644
>> --- a/target/ppc/internal.h
>> +++ b/target/ppc/internal.h
>> @@ -234,24 +234,8 @@ void destroy_ppc_opcodes(PowerPCCPU *cpu);
>> void ppc_gdb_init(CPUState *cs, PowerPCCPUClass *ppc);
>> const gchar *ppc_gdb_arch_name(CPUState *cs);
>>
>> -/**
>> - * prot_for_access_type:
>> - * @access_type: Access type
>> - *
>> - * Return the protection bit required for the given access type.
>> - */
>> -static inline int prot_for_access_type(MMUAccessType access_type)
>> -{
>> - switch (access_type) {
>> - case MMU_INST_FETCH:
>> - return PAGE_EXEC;
>> - case MMU_DATA_LOAD:
>> - return PAGE_READ;
>> - case MMU_DATA_STORE:
>> - return PAGE_WRITE;
>> - }
>> - g_assert_not_reached();
>> -}
>> +/* Check if permission bit required for the access_type is set in prot */
>> +#define CHECK_PROT_ACCESS(prot, access_type) ((prot) & (1 << (access_type)))
>
> We don't want to use a macro when an inline function will work.
>
> Does the compiler not see the pattern and transform the existing
> code into a shift? If it does then I would leave it. If not, then
> just keep prot_for_access_type but make it a shift and maybe
> comment the logic.
>
> I would call the new function check_prot_for_access_type().
That would be too long and does not fit on one line. Long names with
underscore and 80 char line limit does not go well together. I've left
this unchanged for now and wait for your reply on this.
Regards,
BALATON Zoltan
On Thu May 9, 2024 at 9:35 AM AEST, BALATON Zoltan wrote:
> On Wed, 8 May 2024, Nicholas Piggin wrote:
> > On Wed May 8, 2024 at 10:15 AM AEST, BALATON Zoltan wrote:
> >> Checking if a page protection bit is set for a given access type is a
> >> common operation. Add a macro to avoid repeating the same check at
> >> multiple places and also avoid a function call. As this relies on
> >> access type and page protection bit values having certain relation
> >> also add an assert to ensure that this assumption holds.
> >>
> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> >> ---
> >> target/ppc/cpu_init.c | 4 ++++
> >> target/ppc/internal.h | 20 ++------------------
> >> target/ppc/mmu-hash32.c | 6 +++---
> >> target/ppc/mmu-hash64.c | 2 +-
> >> target/ppc/mmu-radix64.c | 2 +-
> >> target/ppc/mmu_common.c | 26 +++++++++++++-------------
> >> 6 files changed, 24 insertions(+), 36 deletions(-)
> >>
> >> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> >> index 92c71b2a09..6639235544 100644
> >> --- a/target/ppc/cpu_init.c
> >> +++ b/target/ppc/cpu_init.c
> >> @@ -7377,6 +7377,10 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
> >> resettable_class_set_parent_phases(rc, NULL, ppc_cpu_reset_hold, NULL,
> >> &pcc->parent_phases);
> >>
> >> + /* CHECK_PROT_ACCESS relies on this MMU access and PAGE bits relation */
> >> + assert(MMU_DATA_LOAD == 0 && MMU_DATA_STORE == 1 && MMU_INST_FETCH == 2 &&
> >> + PAGE_READ == 1 && PAGE_WRITE == 2 && PAGE_EXEC == 4);
> >> +
> >
> > Can you use qemu_build_assert() for this?
>
> I've changed it to qemu_build_assert and seems to work.
>
> >> cc->class_by_name = ppc_cpu_class_by_name;
> >> cc->has_work = ppc_cpu_has_work;
> >> cc->mmu_index = ppc_cpu_mmu_index;
> >> diff --git a/target/ppc/internal.h b/target/ppc/internal.h
> >> index 46176c4711..9880422ce3 100644
> >> --- a/target/ppc/internal.h
> >> +++ b/target/ppc/internal.h
> >> @@ -234,24 +234,8 @@ void destroy_ppc_opcodes(PowerPCCPU *cpu);
> >> void ppc_gdb_init(CPUState *cs, PowerPCCPUClass *ppc);
> >> const gchar *ppc_gdb_arch_name(CPUState *cs);
> >>
> >> -/**
> >> - * prot_for_access_type:
> >> - * @access_type: Access type
> >> - *
> >> - * Return the protection bit required for the given access type.
> >> - */
> >> -static inline int prot_for_access_type(MMUAccessType access_type)
> >> -{
> >> - switch (access_type) {
> >> - case MMU_INST_FETCH:
> >> - return PAGE_EXEC;
> >> - case MMU_DATA_LOAD:
> >> - return PAGE_READ;
> >> - case MMU_DATA_STORE:
> >> - return PAGE_WRITE;
> >> - }
> >> - g_assert_not_reached();
> >> -}
> >> +/* Check if permission bit required for the access_type is set in prot */
> >> +#define CHECK_PROT_ACCESS(prot, access_type) ((prot) & (1 << (access_type)))
> >
> > We don't want to use a macro when an inline function will work.
> >
> > Does the compiler not see the pattern and transform the existing
> > code into a shift? If it does then I would leave it. If not, then
> > just keep prot_for_access_type but make it a shift and maybe
> > comment the logic.
> >
> > I would call the new function check_prot_for_access_type().
>
> That would be too long and does not fit on one line. Long names with
> underscore and 80 char line limit does not go well together. I've left
> this unchanged for now and wait for your reply on this.
Just split the line at the second argument. Better name is more
important than minimising line count.
Thanks,
Nick
On Wed, 8 May 2024, Nicholas Piggin wrote:
> On Wed May 8, 2024 at 10:15 AM AEST, BALATON Zoltan wrote:
>> Checking if a page protection bit is set for a given access type is a
>> common operation. Add a macro to avoid repeating the same check at
>> multiple places and also avoid a function call. As this relies on
>> access type and page protection bit values having certain relation
>> also add an assert to ensure that this assumption holds.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> target/ppc/cpu_init.c | 4 ++++
>> target/ppc/internal.h | 20 ++------------------
>> target/ppc/mmu-hash32.c | 6 +++---
>> target/ppc/mmu-hash64.c | 2 +-
>> target/ppc/mmu-radix64.c | 2 +-
>> target/ppc/mmu_common.c | 26 +++++++++++++-------------
>> 6 files changed, 24 insertions(+), 36 deletions(-)
>>
>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
>> index 92c71b2a09..6639235544 100644
>> --- a/target/ppc/cpu_init.c
>> +++ b/target/ppc/cpu_init.c
>> @@ -7377,6 +7377,10 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>> resettable_class_set_parent_phases(rc, NULL, ppc_cpu_reset_hold, NULL,
>> &pcc->parent_phases);
>>
>> + /* CHECK_PROT_ACCESS relies on this MMU access and PAGE bits relation */
>> + assert(MMU_DATA_LOAD == 0 && MMU_DATA_STORE == 1 && MMU_INST_FETCH == 2 &&
>> + PAGE_READ == 1 && PAGE_WRITE == 2 && PAGE_EXEC == 4);
>> +
>
> Can you use qemu_build_assert() for this?
First I've try #if and #error but seems access_type is an enum and the
preprocessor does not see those. If qemu_build_assert is a wrapper around
the same then it might not work but I'll check.
>> cc->class_by_name = ppc_cpu_class_by_name;
>> cc->has_work = ppc_cpu_has_work;
>> cc->mmu_index = ppc_cpu_mmu_index;
>> diff --git a/target/ppc/internal.h b/target/ppc/internal.h
>> index 46176c4711..9880422ce3 100644
>> --- a/target/ppc/internal.h
>> +++ b/target/ppc/internal.h
>> @@ -234,24 +234,8 @@ void destroy_ppc_opcodes(PowerPCCPU *cpu);
>> void ppc_gdb_init(CPUState *cs, PowerPCCPUClass *ppc);
>> const gchar *ppc_gdb_arch_name(CPUState *cs);
>>
>> -/**
>> - * prot_for_access_type:
>> - * @access_type: Access type
>> - *
>> - * Return the protection bit required for the given access type.
>> - */
>> -static inline int prot_for_access_type(MMUAccessType access_type)
>> -{
>> - switch (access_type) {
>> - case MMU_INST_FETCH:
>> - return PAGE_EXEC;
>> - case MMU_DATA_LOAD:
>> - return PAGE_READ;
>> - case MMU_DATA_STORE:
>> - return PAGE_WRITE;
>> - }
>> - g_assert_not_reached();
>> -}
>> +/* Check if permission bit required for the access_type is set in prot */
>> +#define CHECK_PROT_ACCESS(prot, access_type) ((prot) & (1 << (access_type)))
>
> We don't want to use a macro when an inline function will work.
What's wrong with a macro? This has no local variables or any complex
operation that would warant a function IMO it's just a conditional named
for convenience so we don't have to type it everywhere and easier to see
what is it for. A macro is just right for that.
> Does the compiler not see the pattern and transform the existing
> code into a shift? If it does then I would leave it. If not, then
> just keep prot_for_access_type but make it a shift and maybe
> comment the logic.
I don't know but prot_for_access_type is not even needed because this will
work for that too passing PAGE_RWX for prot as done below at one place so
no need for another function for that.
> I would call the new function check_prot_for_access_type().
I can rename it and could make it static inline but I like a macro for
this better.
Regards,
BALATON Zoltan
>>
>> #ifndef CONFIG_USER_ONLY
>>
>> diff --git a/target/ppc/mmu-hash32.c b/target/ppc/mmu-hash32.c
>> index b5d7aeed4e..fa4a4ced6d 100644
>> --- a/target/ppc/mmu-hash32.c
>> +++ b/target/ppc/mmu-hash32.c
>> @@ -213,7 +213,7 @@ static bool ppc_hash32_direct_store(PowerPCCPU *cpu, target_ulong sr,
>> }
>>
>> *prot = key ? PAGE_READ | PAGE_WRITE : PAGE_READ;
>> - if (*prot & prot_for_access_type(access_type)) {
>> + if (CHECK_PROT_ACCESS(*prot, access_type)) {
>> *raddr = eaddr;
>> return true;
>> }
>
> This does read better, and better than the check_prot() below
> that it replaces too, so I like the cleanup.
>
> Thanks,
> Nick
>
>> @@ -364,7 +364,7 @@ bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
>> if (env->nb_BATs != 0) {
>> raddr = ppc_hash32_bat_lookup(cpu, eaddr, access_type, protp, mmu_idx);
>> if (raddr != -1) {
>> - if (prot_for_access_type(access_type) & ~*protp) {
>> + if (!CHECK_PROT_ACCESS(*protp, access_type)) {
>> if (guest_visible) {
>> if (access_type == MMU_INST_FETCH) {
>> cs->exception_index = POWERPC_EXCP_ISI;
>> @@ -432,7 +432,7 @@ bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
>>
>> prot = ppc_hash32_pte_prot(mmu_idx, sr, pte);
>>
>> - if (prot_for_access_type(access_type) & ~prot) {
>> + if (!CHECK_PROT_ACCESS(prot, access_type)) {
>> /* Access right violation */
>> qemu_log_mask(CPU_LOG_MMU, "PTE access rejected\n");
>> if (guest_visible) {
>> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
>> index 5a0d80feda..14c2116ae7 100644
>> --- a/target/ppc/mmu-hash64.c
>> +++ b/target/ppc/mmu-hash64.c
>> @@ -1097,7 +1097,7 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
>> amr_prot = ppc_hash64_amr_prot(cpu, pte);
>> prot = exec_prot & pp_prot & amr_prot;
>>
>> - need_prot = prot_for_access_type(access_type);
>> + need_prot = CHECK_PROT_ACCESS(PAGE_RWX, access_type);
>> if (need_prot & ~prot) {
>> /* Access right violation */
>> qemu_log_mask(CPU_LOG_MMU, "PTE access rejected\n");
>> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
>> index 395ce3b782..a72cd927c4 100644
>> --- a/target/ppc/mmu-radix64.c
>> +++ b/target/ppc/mmu-radix64.c
>> @@ -209,7 +209,7 @@ static bool ppc_radix64_check_prot(PowerPCCPU *cpu, MMUAccessType access_type,
>> }
>>
>> /* Check if requested access type is allowed */
>> - if (prot_for_access_type(access_type) & ~*prot) {
>> + if (!CHECK_PROT_ACCESS(*prot, access_type)) {
>> /* Page Protected for that Access */
>> *fault_cause |= access_type == MMU_INST_FETCH ? SRR1_NOEXEC_GUARD :
>> DSISR_PROTFAULT;
>> diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
>> index eff015066d..24d68926b4 100644
>> --- a/target/ppc/mmu_common.c
>> +++ b/target/ppc/mmu_common.c
>> @@ -76,11 +76,6 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
>> /*****************************************************************************/
>> /* PowerPC MMU emulation */
>>
>> -static int check_prot(int prot, MMUAccessType access_type)
>> -{
>> - return prot & prot_for_access_type(access_type) ? 0 : -2;
>> -}
>> -
>> int ppc6xx_tlb_getnum(CPUPPCState *env, target_ulong eaddr,
>> int way, int is_code)
>> {
>> @@ -125,13 +120,14 @@ static int ppc6xx_tlb_pte_check(mmu_ctx_t *ctx, target_ulong pte0,
>> /* Keep the matching PTE information */
>> ctx->raddr = pte1;
>> ctx->prot = ppc_hash32_pp_prot(ctx->key, pp, ctx->nx);
>> - ret = check_prot(ctx->prot, access_type);
>> - if (ret == 0) {
>> + if (CHECK_PROT_ACCESS(ctx->prot, access_type)) {
>> /* Access granted */
>> qemu_log_mask(CPU_LOG_MMU, "PTE access granted !\n");
>> + ret = 0;
>> } else {
>> /* Access right violation */
>> qemu_log_mask(CPU_LOG_MMU, "PTE access rejected\n");
>> + ret = -2;
>> }
>> }
>> }
>> @@ -317,12 +313,14 @@ static int get_bat_6xx_tlb(CPUPPCState *env, mmu_ctx_t *ctx,
>> (virtual & 0x0001F000);
>> /* Compute access rights */
>> ctx->prot = prot;
>> - ret = check_prot(ctx->prot, access_type);
>> - if (ret == 0) {
>> + if (CHECK_PROT_ACCESS(ctx->prot, access_type)) {
>> qemu_log_mask(CPU_LOG_MMU, "BAT %d match: r " HWADDR_FMT_plx
>> " prot=%c%c\n", i, ctx->raddr,
>> ctx->prot & PAGE_READ ? 'R' : '-',
>> ctx->prot & PAGE_WRITE ? 'W' : '-');
>> + ret = 0;
>> + } else {
>> + ret = -2;
>> }
>> break;
>> }
>> @@ -540,9 +538,11 @@ static int mmu40x_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx,
>> check_perms:
>> /* Check from TLB entry */
>> ctx->prot = tlb->prot;
>> - ret = check_prot(ctx->prot, access_type);
>> - if (ret == -2) {
>> + if (CHECK_PROT_ACCESS(ctx->prot, access_type)) {
>> + ret = 0;
>> + } else {
>> env->spr[SPR_40x_ESR] = 0;
>> + ret = -2;
>> }
>> break;
>> }
>> @@ -607,7 +607,7 @@ static int mmubooke_check_tlb(CPUPPCState *env, ppcemb_tlb_t *tlb,
>> } else {
>> *prot = (tlb->prot >> 4) & 0xF;
>> }
>> - if (*prot & prot_for_access_type(access_type)) {
>> + if (CHECK_PROT_ACCESS(*prot, access_type)) {
>> qemu_log_mask(CPU_LOG_MMU, "%s: good TLB!\n", __func__);
>> return 0;
>> }
>> @@ -811,7 +811,7 @@ found_tlb:
>> *prot |= PAGE_EXEC;
>> }
>> }
>> - if (*prot & prot_for_access_type(access_type)) {
>> + if (CHECK_PROT_ACCESS(*prot, access_type)) {
>> qemu_log_mask(CPU_LOG_MMU, "%s: good TLB!\n", __func__);
>> return 0;
>> }
>
>
>
On Thu May 9, 2024 at 1:23 AM AEST, BALATON Zoltan wrote:
> On Wed, 8 May 2024, Nicholas Piggin wrote:
> > On Wed May 8, 2024 at 10:15 AM AEST, BALATON Zoltan wrote:
> >> Checking if a page protection bit is set for a given access type is a
> >> common operation. Add a macro to avoid repeating the same check at
> >> multiple places and also avoid a function call. As this relies on
> >> access type and page protection bit values having certain relation
> >> also add an assert to ensure that this assumption holds.
> >>
> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> >> ---
> >> target/ppc/cpu_init.c | 4 ++++
> >> target/ppc/internal.h | 20 ++------------------
> >> target/ppc/mmu-hash32.c | 6 +++---
> >> target/ppc/mmu-hash64.c | 2 +-
> >> target/ppc/mmu-radix64.c | 2 +-
> >> target/ppc/mmu_common.c | 26 +++++++++++++-------------
> >> 6 files changed, 24 insertions(+), 36 deletions(-)
> >>
> >> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> >> index 92c71b2a09..6639235544 100644
> >> --- a/target/ppc/cpu_init.c
> >> +++ b/target/ppc/cpu_init.c
> >> @@ -7377,6 +7377,10 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
> >> resettable_class_set_parent_phases(rc, NULL, ppc_cpu_reset_hold, NULL,
> >> &pcc->parent_phases);
> >>
> >> + /* CHECK_PROT_ACCESS relies on this MMU access and PAGE bits relation */
> >> + assert(MMU_DATA_LOAD == 0 && MMU_DATA_STORE == 1 && MMU_INST_FETCH == 2 &&
> >> + PAGE_READ == 1 && PAGE_WRITE == 2 && PAGE_EXEC == 4);
> >> +
> >
> > Can you use qemu_build_assert() for this?
>
> First I've try #if and #error but seems access_type is an enum and the
> preprocessor does not see those. If qemu_build_assert is a wrapper around
> the same then it might not work but I'll check.
>
> >> cc->class_by_name = ppc_cpu_class_by_name;
> >> cc->has_work = ppc_cpu_has_work;
> >> cc->mmu_index = ppc_cpu_mmu_index;
> >> diff --git a/target/ppc/internal.h b/target/ppc/internal.h
> >> index 46176c4711..9880422ce3 100644
> >> --- a/target/ppc/internal.h
> >> +++ b/target/ppc/internal.h
> >> @@ -234,24 +234,8 @@ void destroy_ppc_opcodes(PowerPCCPU *cpu);
> >> void ppc_gdb_init(CPUState *cs, PowerPCCPUClass *ppc);
> >> const gchar *ppc_gdb_arch_name(CPUState *cs);
> >>
> >> -/**
> >> - * prot_for_access_type:
> >> - * @access_type: Access type
> >> - *
> >> - * Return the protection bit required for the given access type.
> >> - */
> >> -static inline int prot_for_access_type(MMUAccessType access_type)
> >> -{
> >> - switch (access_type) {
> >> - case MMU_INST_FETCH:
> >> - return PAGE_EXEC;
> >> - case MMU_DATA_LOAD:
> >> - return PAGE_READ;
> >> - case MMU_DATA_STORE:
> >> - return PAGE_WRITE;
> >> - }
> >> - g_assert_not_reached();
> >> -}
> >> +/* Check if permission bit required for the access_type is set in prot */
> >> +#define CHECK_PROT_ACCESS(prot, access_type) ((prot) & (1 << (access_type)))
> >
> > We don't want to use a macro when an inline function will work.
>
> What's wrong with a macro? This has no local variables or any complex
> operation that would warant a function IMO it's just a conditional named
> for convenience so we don't have to type it everywhere and easier to see
> what is it for. A macro is just right for that.
Macro does not get parameter or return type check, and has a bunch of
other potential issues
https://gcc.gnu.org/onlinedocs/cpp/macros/macro-pitfalls.html
Macro should not be used unless you can not do it with inline function.
There is no benefit to macro here.
>
> > Does the compiler not see the pattern and transform the existing
> > code into a shift? If it does then I would leave it. If not, then
> > just keep prot_for_access_type but make it a shift and maybe
> > comment the logic.
>
> I don't know but prot_for_access_type is not even needed because this will
> work for that too passing PAGE_RWX for prot as done below at one place so
> no need for another function for that.
>
> > I would call the new function check_prot_for_access_type().
>
> I can rename it and could make it static inline but I like a macro for
> this better.
Thanks,
Nick
© 2016 - 2024 Red Hat, Inc.