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 <bala...@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