From: David Gibson <david@gibson.dropbear.id.au>
To: "Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br>
Cc: farosas@linux.ibm.com, richard.henderson@linaro.org,
qemu-devel@nongnu.org, Greg Kurz <groug@kaod.org>,
lucas.araujo@eldorado.org.br, fernando.valle@eldorado.org.br,
qemu-ppc@nongnu.org, matheus.ferst@eldorado.org.br,
luis.pires@eldorado.org.br
Subject: Re: [PATCH v4 2/3] target/ppc: change ppc_hash32_xlate to use mmu_idx
Date: Mon, 5 Jul 2021 14:24:18 +1000 [thread overview]
Message-ID: <YOKJco6ebWubvDwx@yekko> (raw)
In-Reply-To: <20210628133610.1143-3-bruno.larsen@eldorado.org.br>
[-- Attachment #1: Type: text/plain, Size: 9077 bytes --]
On Mon, Jun 28, 2021 at 10:36:09AM -0300, Bruno Larsen (billionai) wrote:
> Changed hash32 address translation to use the supplied mmu_idx, instead
> of using what was stored in the msr, for parity purposes (radix64
> already uses that).
Well.. parity and conceptual correctness. The translation is supposed
to use mmu_idx, not look at the CPU again to get the right context.
AFAIK there isn't a situation in hash32 where they'll get out of sync,
but nothing guarantees that.
>
> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> ---
> target/ppc/mmu-hash32.c | 40 +++++++++++++++++++---------------------
> target/ppc/mmu-hash32.h | 2 +-
> target/ppc/mmu_helper.c | 2 +-
> 3 files changed, 21 insertions(+), 23 deletions(-)
>
> diff --git a/target/ppc/mmu-hash32.c b/target/ppc/mmu-hash32.c
> index 6a07c345e4..0691d553a3 100644
> --- a/target/ppc/mmu-hash32.c
> +++ b/target/ppc/mmu-hash32.c
> @@ -25,6 +25,7 @@
> #include "kvm_ppc.h"
> #include "internal.h"
> #include "mmu-hash32.h"
> +#include "mmu-book3s-v3.h"
So, the mmu_idx values happen to have the same values for hash32 as
for book3sv3, but that's arguably a coincidence, and including a
book3sv3 header in hash32 code looks really dubious.
I think the right approach is to duplicate the helper macros in
mmu-hash32.h for now. We can unify them later with a more thorough
review (which would probably involve creating a new header for things
common to all BookS family MMUs).
Apart from those nits,
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> #include "exec/log.h"
>
> /* #define DEBUG_BAT */
> @@ -86,25 +87,22 @@ static int ppc_hash32_pp_prot(int key, int pp, int nx)
> return prot;
> }
>
> -static int ppc_hash32_pte_prot(PowerPCCPU *cpu,
> +static int ppc_hash32_pte_prot(int mmu_idx,
> target_ulong sr, ppc_hash_pte32_t pte)
> {
> - CPUPPCState *env = &cpu->env;
> unsigned pp, key;
>
> - key = !!(msr_pr ? (sr & SR32_KP) : (sr & SR32_KS));
> + key = !!(mmuidx_pr(mmu_idx) ? (sr & SR32_KP) : (sr & SR32_KS));
> pp = pte.pte1 & HPTE32_R_PP;
>
> return ppc_hash32_pp_prot(key, pp, !!(sr & SR32_NX));
> }
>
> -static target_ulong hash32_bat_size(PowerPCCPU *cpu,
> +static target_ulong hash32_bat_size(int mmu_idx,
> target_ulong batu, target_ulong batl)
> {
> - CPUPPCState *env = &cpu->env;
> -
> - if ((msr_pr && !(batu & BATU32_VP))
> - || (!msr_pr && !(batu & BATU32_VS))) {
> + if ((mmuidx_pr(mmu_idx) && !(batu & BATU32_VP))
> + || (!mmuidx_pr(mmu_idx) && !(batu & BATU32_VS))) {
> return 0;
> }
>
> @@ -137,14 +135,13 @@ static target_ulong hash32_bat_601_size(PowerPCCPU *cpu,
> return BATU32_BEPI & ~((batl & BATL32_601_BL) << 17);
> }
>
> -static int hash32_bat_601_prot(PowerPCCPU *cpu,
> +static int hash32_bat_601_prot(int mmu_idx,
> target_ulong batu, target_ulong batl)
> {
> - CPUPPCState *env = &cpu->env;
> int key, pp;
>
> pp = batu & BATU32_601_PP;
> - if (msr_pr == 0) {
> + if (mmuidx_pr(mmu_idx) == 0) {
> key = !!(batu & BATU32_601_KS);
> } else {
> key = !!(batu & BATU32_601_KP);
> @@ -153,7 +150,8 @@ static int hash32_bat_601_prot(PowerPCCPU *cpu,
> }
>
> static hwaddr ppc_hash32_bat_lookup(PowerPCCPU *cpu, target_ulong ea,
> - MMUAccessType access_type, int *prot)
> + MMUAccessType access_type, int *prot,
> + int mmu_idx)
> {
> CPUPPCState *env = &cpu->env;
> target_ulong *BATlt, *BATut;
> @@ -177,7 +175,7 @@ static hwaddr ppc_hash32_bat_lookup(PowerPCCPU *cpu, target_ulong ea,
> if (unlikely(env->mmu_model == POWERPC_MMU_601)) {
> mask = hash32_bat_601_size(cpu, batu, batl);
> } else {
> - mask = hash32_bat_size(cpu, batu, batl);
> + mask = hash32_bat_size(mmu_idx, batu, batl);
> }
> LOG_BATS("%s: %cBAT%d v " TARGET_FMT_lx " BATu " TARGET_FMT_lx
> " BATl " TARGET_FMT_lx "\n", __func__,
> @@ -187,7 +185,7 @@ static hwaddr ppc_hash32_bat_lookup(PowerPCCPU *cpu, target_ulong ea,
> hwaddr raddr = (batl & mask) | (ea & ~mask);
>
> if (unlikely(env->mmu_model == POWERPC_MMU_601)) {
> - *prot = hash32_bat_601_prot(cpu, batu, batl);
> + *prot = hash32_bat_601_prot(mmu_idx, batu, batl);
> } else {
> *prot = hash32_bat_prot(cpu, batu, batl);
> }
> @@ -221,12 +219,12 @@ static hwaddr ppc_hash32_bat_lookup(PowerPCCPU *cpu, target_ulong ea,
> static bool ppc_hash32_direct_store(PowerPCCPU *cpu, target_ulong sr,
> target_ulong eaddr,
> MMUAccessType access_type,
> - hwaddr *raddr, int *prot,
> + hwaddr *raddr, int *prot, int mmu_idx,
> bool guest_visible)
> {
> CPUState *cs = CPU(cpu);
> CPUPPCState *env = &cpu->env;
> - int key = !!(msr_pr ? (sr & SR32_KP) : (sr & SR32_KS));
> + int key = !!(mmuidx_pr(mmu_idx) ? (sr & SR32_KP) : (sr & SR32_KS));
>
> qemu_log_mask(CPU_LOG_MMU, "direct store...\n");
>
> @@ -425,7 +423,7 @@ static hwaddr ppc_hash32_pte_raddr(target_ulong sr, ppc_hash_pte32_t pte,
> }
>
> bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> - hwaddr *raddrp, int *psizep, int *protp,
> + hwaddr *raddrp, int *psizep, int *protp, int mmu_idx,
> bool guest_visible)
> {
> CPUState *cs = CPU(cpu);
> @@ -441,7 +439,7 @@ bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> *psizep = TARGET_PAGE_BITS;
>
> /* 1. Handle real mode accesses */
> - if (access_type == MMU_INST_FETCH ? !msr_ir : !msr_dr) {
> + if (mmuidx_real(mmu_idx)) {
> /* Translation is off */
> *raddrp = eaddr;
> *protp = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> @@ -452,7 +450,7 @@ bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
>
> /* 2. Check Block Address Translation entries (BATs) */
> if (env->nb_BATs != 0) {
> - raddr = ppc_hash32_bat_lookup(cpu, eaddr, access_type, protp);
> + raddr = ppc_hash32_bat_lookup(cpu, eaddr, access_type, protp, mmu_idx);
> if (raddr != -1) {
> if (need_prot & ~*protp) {
> if (guest_visible) {
> @@ -483,7 +481,7 @@ bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> /* 4. Handle direct store segments */
> if (sr & SR32_T) {
> return ppc_hash32_direct_store(cpu, sr, eaddr, access_type,
> - raddrp, protp, guest_visible);
> + raddrp, protp, mmu_idx, guest_visible);
> }
>
> /* 5. Check for segment level no-execute violation */
> @@ -520,7 +518,7 @@ bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
>
> /* 7. Check access permissions */
>
> - prot = ppc_hash32_pte_prot(cpu, sr, pte);
> + prot = ppc_hash32_pte_prot(mmu_idx, sr, pte);
>
> if (need_prot & ~prot) {
> /* Access right violation */
> diff --git a/target/ppc/mmu-hash32.h b/target/ppc/mmu-hash32.h
> index 8694eccabd..807d9bc6e8 100644
> --- a/target/ppc/mmu-hash32.h
> +++ b/target/ppc/mmu-hash32.h
> @@ -5,7 +5,7 @@
>
> hwaddr get_pteg_offset32(PowerPCCPU *cpu, hwaddr hash);
> bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> - hwaddr *raddrp, int *psizep, int *protp,
> + hwaddr *raddrp, int *psizep, int *protp, int mmu_idx,
> bool guest_visible);
>
> /*
> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> index 9dcdf88597..a3381e1aa0 100644
> --- a/target/ppc/mmu_helper.c
> +++ b/target/ppc/mmu_helper.c
> @@ -2922,7 +2922,7 @@ static bool ppc_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> case POWERPC_MMU_32B:
> case POWERPC_MMU_601:
> return ppc_hash32_xlate(cpu, eaddr, access_type,
> - raddrp, psizep, protp, guest_visible);
> + raddrp, psizep, protp, mmu_idx, guest_visible);
>
> default:
> return ppc_jumbo_xlate(cpu, eaddr, access_type, raddrp,
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2021-07-05 4:46 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-28 13:36 [PATCH v4 0/3] Clean up MMU translation Bruno Larsen (billionai)
2021-06-28 13:36 ` [PATCH v4 1/3] target/ppc: fix address translation bug for radix mmus Bruno Larsen (billionai)
2021-07-05 4:17 ` David Gibson
2021-06-28 13:36 ` [PATCH v4 2/3] target/ppc: change ppc_hash32_xlate to use mmu_idx Bruno Larsen (billionai)
2021-07-05 4:24 ` David Gibson [this message]
2021-07-05 14:31 ` Bruno Piazera Larsen
2021-07-06 1:35 ` David Gibson
2021-06-28 13:36 ` [PATCH v4 3/3] target/ppc: changed ppc_hash64_xlate " Bruno Larsen (billionai)
2021-07-05 4:26 ` David Gibson
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=YOKJco6ebWubvDwx@yekko \
--to=david@gibson.dropbear.id.au \
--cc=bruno.larsen@eldorado.org.br \
--cc=farosas@linux.ibm.com \
--cc=fernando.valle@eldorado.org.br \
--cc=groug@kaod.org \
--cc=lucas.araujo@eldorado.org.br \
--cc=luis.pires@eldorado.org.br \
--cc=matheus.ferst@eldorado.org.br \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
--cc=richard.henderson@linaro.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.