Re: [PATCH v3 33/33] target/ppc: Add a macro to check for page protection bit

[Nicholas Piggin]

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