From: "Nicholas Piggin" <npiggin@gmail.com>
To: "BALATON Zoltan" <balaton@eik.bme.hu>
Cc: <qemu-devel@nongnu.org>, <qemu-ppc@nongnu.org>,
"Daniel Henrique Barboza" <danielhb413@gmail.com>
Subject: Re: [PATCH v3 33/33] target/ppc: Add a macro to check for page protection bit
Date: Thu, 09 May 2024 15:58:49 +1000 [thread overview]
Message-ID: <D14VTP1B4TNR.M9SZAKYV0101@gmail.com> (raw)
In-Reply-To: <7c4e51de-fdff-37b6-ffe5-2e7e26cffc17@eik.bme.hu>
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
prev parent reply other threads:[~2024-05-09 5:59 UTC|newest]
Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-08 0:14 [PATCH v3 00/33] Misc PPC exception and BookE MMU clean ups BALATON Zoltan
2024-05-08 0:14 ` [PATCH v3 01/33] target/ppc: Fix gen_sc to use correct nip BALATON Zoltan
2024-05-08 13:36 ` Nicholas Piggin
2024-05-08 15:17 ` BALATON Zoltan
2024-05-09 5:48 ` Nicholas Piggin
2024-05-08 0:14 ` [PATCH v3 02/33] target/ppc: Move patching nip from exception handler to helper_scv BALATON Zoltan
2024-05-08 0:14 ` [PATCH v3 03/33] target/ppc: Simplify syscall exception handlers BALATON Zoltan
2024-05-08 0:14 ` [PATCH v3 04/33] target/ppc: Remove unused helper BALATON Zoltan
2024-05-08 0:14 ` [PATCH v3 05/33] target/ppc/mmu_common.c: Move calculation of a value closer to its usage BALATON Zoltan
2024-05-08 0:14 ` [PATCH v3 06/33] target/ppc/mmu_common.c: Remove unneeded local variable BALATON Zoltan
2024-05-08 0:14 ` [PATCH v3 07/33] target/ppc/mmu_common.c: Simplify checking for real mode BALATON Zoltan
2024-05-08 0:15 ` [PATCH v3 08/33] target/ppc/mmu_common.c: Drop cases for unimplemented MPC8xx MMU BALATON Zoltan
2024-05-08 0:15 ` [PATCH v3 09/33] target/ppc/mmu_common.c: Introduce mmu6xx_get_physical_address() BALATON Zoltan
2024-05-08 12:40 ` Nicholas Piggin
2024-05-08 0:15 ` [PATCH v3 10/33] target/ppc/mmu_common.c: Move else branch to avoid large if block BALATON Zoltan
2024-05-08 12:43 ` Nicholas Piggin
2024-05-08 0:15 ` [PATCH v3 11/33] target/ppc/mmu_common.c: Move some debug logging BALATON Zoltan
2024-05-08 12:47 ` Nicholas Piggin
2024-05-08 0:15 ` [PATCH v3 12/33] target/ppc/mmu_common.c: Eliminate ret from mmu6xx_get_physical_address() BALATON Zoltan
2024-05-08 12:48 ` Nicholas Piggin
2024-05-08 0:15 ` [PATCH v3 13/33] target/ppc/mmu_common.c: Split out BookE cases before checking real mode BALATON Zoltan
2024-05-08 0:15 ` [PATCH v3 14/33] target/ppc/mmu_common.c: Split off real mode cases in get_physical_address_wtlb() BALATON Zoltan
2024-05-08 0:15 ` [PATCH v3 15/33] target/ppc/mmu_common.c: Inline and remove check_physical() BALATON Zoltan
2024-05-08 0:15 ` [PATCH v3 16/33] target/ppc/mmu_common.c: Simplify mmubooke_get_physical_address() BALATON Zoltan
2024-05-08 0:15 ` [PATCH v3 17/33] target/ppc/mmu_common.c: Simplify mmubooke206_get_physical_address() BALATON Zoltan
2024-05-08 0:15 ` [PATCH v3 18/33] target/ppc/mmu_common.c: Fix misindented qemu_log_mask() calls BALATON Zoltan
2024-05-08 0:15 ` [PATCH v3 19/33] target/ppc/mmu_common.c: Deindent ppc_jumbo_xlate() BALATON Zoltan
2024-05-08 0:15 ` [PATCH v3 20/33] target/ppc/mmu_common.c: Replace hard coded constants in ppc_jumbo_xlate() BALATON Zoltan
2024-05-08 0:15 ` [PATCH v3 21/33] target/ppc/mmu_common.c: Make get_physical_address_wtlb() static BALATON Zoltan
2024-05-08 12:58 ` Nicholas Piggin
2024-05-08 0:15 ` [PATCH v3 22/33] target/ppc: Remove pp_check() and reuse ppc_hash32_pp_prot() BALATON Zoltan
2024-05-08 12:59 ` Nicholas Piggin
2024-05-08 0:15 ` [PATCH v3 23/33] target/ppc/mmu_common.c: Remove BookE from direct store handling BALATON Zoltan
2024-05-08 12:59 ` Nicholas Piggin
2024-05-08 0:15 ` [PATCH v3 24/33] target/ppc/mmu_common.c: Split off BookE handling from ppc_jumbo_xlate() BALATON Zoltan
2024-05-08 13:01 ` Nicholas Piggin
2024-05-08 0:15 ` [PATCH v3 25/33] target/ppc/mmu_common.c: Remove BookE handling from get_physical_address_wtlb() BALATON Zoltan
2024-05-08 13:12 ` Nicholas Piggin
2024-05-08 0:15 ` [PATCH v3 26/33] target/ppc/mmu_common.c: Simplify ppc_booke_xlate() part 1 BALATON Zoltan
2024-05-08 13:17 ` Nicholas Piggin
2024-05-08 15:25 ` BALATON Zoltan
2024-05-09 5:53 ` Nicholas Piggin
2024-05-08 0:15 ` [PATCH v3 27/33] target/ppc/mmu_common.c: Simplify ppc_booke_xlate() part 2 BALATON Zoltan
2024-05-08 13:21 ` Nicholas Piggin
2024-05-08 0:15 ` [PATCH v3 28/33] target/ppc/mmu_common.c: Move BookE MMU functions together BALATON Zoltan
2024-05-08 13:21 ` Nicholas Piggin
2024-05-08 0:15 ` [PATCH v3 29/33] target/ppc: Remove id_tlbs flag from CPU env BALATON Zoltan
2024-05-08 0:15 ` [PATCH v3 30/33] target/ppc: Split off common embedded TLB init BALATON Zoltan
2024-05-08 0:15 ` [PATCH v3 31/33] target/ppc/mmu-hash32.c: Drop a local variable BALATON Zoltan
2024-05-08 13:22 ` Nicholas Piggin
2024-05-08 0:15 ` [PATCH v3 32/33] target/ppc/mmu-radix64.c: " BALATON Zoltan
2024-05-08 13:22 ` Nicholas Piggin
2024-05-08 0:15 ` [PATCH v3 33/33] target/ppc: Add a macro to check for page protection bit BALATON Zoltan
2024-05-08 13:29 ` Nicholas Piggin
2024-05-08 15:23 ` BALATON Zoltan
2024-05-09 5:52 ` Nicholas Piggin
2024-05-08 23:35 ` BALATON Zoltan
2024-05-09 5:58 ` Nicholas Piggin [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=D14VTP1B4TNR.M9SZAKYV0101@gmail.com \
--to=npiggin@gmail.com \
--cc=balaton@eik.bme.hu \
--cc=danielhb413@gmail.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).