* [PATCH v4 0/3] Clean up MMU translation
@ 2021-06-28 13:36 Bruno Larsen (billionai)
2021-06-28 13:36 ` [PATCH v4 1/3] target/ppc: fix address translation bug for radix mmus Bruno Larsen (billionai)
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Bruno Larsen (billionai) @ 2021-06-28 13:36 UTC (permalink / raw)
To: qemu-devel
Cc: farosas, richard.henderson, luis.pires, lucas.araujo,
fernando.valle, qemu-ppc, matheus.ferst, david
This is the final part of the MMU fixes that were needed to support
disable-tcg, focusing on a possible bug on the second level address
translation of radix64 MMUs, and some changes to hash32 and hash64 to
work the same as radix64.
Changes for v4:
* added r-b and t-b tags
* changes commit message of the first patch
* removed function parameters that were no longer used
Changes for v3:
* removed patches that were already applied
* fixed comments on last patch
* added 2 new patches
Changes for v2:
* rebase on ppc-for-6.1
* added the bugfix
Bruno Larsen (billionai) (3):
target/ppc: fix address translation bug for radix mmus
target/ppc: change ppc_hash32_xlate to use mmu_idx
target/ppc: changed ppc_hash64_xlate to use mmu_idx
target/ppc/mmu-book3s-v3.h | 13 ++++++++++++
target/ppc/mmu-hash32.c | 40 +++++++++++++++++------------------
target/ppc/mmu-hash32.h | 2 +-
target/ppc/mmu-hash64.c | 43 +++++++++++++++++++-------------------
target/ppc/mmu-hash64.h | 2 +-
target/ppc/mmu-radix64.c | 37 ++++++++++++++++++--------------
target/ppc/mmu-radix64.h | 2 +-
target/ppc/mmu_helper.c | 12 ++++++-----
8 files changed, 84 insertions(+), 67 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v4 1/3] target/ppc: fix address translation bug for radix mmus
2021-06-28 13:36 [PATCH v4 0/3] Clean up MMU translation Bruno Larsen (billionai)
@ 2021-06-28 13:36 ` 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-06-28 13:36 ` [PATCH v4 3/3] target/ppc: changed ppc_hash64_xlate " Bruno Larsen (billionai)
2 siblings, 1 reply; 9+ messages in thread
From: Bruno Larsen (billionai) @ 2021-06-28 13:36 UTC (permalink / raw)
To: qemu-devel
Cc: farosas, richard.henderson, luis.pires, Greg Kurz, lucas.araujo,
fernando.valle, qemu-ppc, matheus.ferst, david
This commit attempts to fix a technical hiccup first mentioned by Richard
Henderson in
https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06247.html
To sumarize the hiccup here, when radix-style mmus are translating an
address, they might need to call a second level of translation, with
hypervisor privileges. However, the way it was being done up until
this point meant that the second level translation had the same
privileges as the first level. It could lead to a bug in address
translation when running KVM inside a TCG guest, but this bug was never
experienced by users, so this isn't as much a bug fix as it is a
correctness cleanup.
This patch attempts that cleanup by making radix64_*_xlate functions
receive the mmu_idx, and passing one with the correct permission for the
second level translation.
The mmuidx macros added by this patch are only correct for non-bookE
mmus, because BookE style set the IS and DS bits inverted and there
might be other subtle differences. However, there doesn't seem to be
BookE cpus that have radix-style mmus, so we left a comment there to
document the issue, in case a machine does have that and was missed.
As part of this cleanup, we now need to send the correct mmmu_idx
when calling get_phys_page_debug, otherwise we might not be able to see the
memory that the CPU could
Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Tested-by: Cédric Le Goater <clg@kaod.org>
---
target/ppc/mmu-book3s-v3.h | 13 +++++++++++++
target/ppc/mmu-radix64.c | 37 +++++++++++++++++++++----------------
target/ppc/mmu-radix64.h | 2 +-
target/ppc/mmu_helper.c | 8 +++++---
4 files changed, 40 insertions(+), 20 deletions(-)
diff --git a/target/ppc/mmu-book3s-v3.h b/target/ppc/mmu-book3s-v3.h
index a1326df969..c89d0bccfd 100644
--- a/target/ppc/mmu-book3s-v3.h
+++ b/target/ppc/mmu-book3s-v3.h
@@ -47,6 +47,19 @@ struct prtb_entry {
uint64_t prtbe0, prtbe1;
};
+/*
+ * These correspond to the mmu_idx values computed in
+ * hreg_compute_hflags_value. See the tables therein
+ *
+ * They are here because some bits are inverted for BookE MMUs
+ * not necessarily because they only work for BookS. However,
+ * we only needed to change BookS MMUs, we left the functions
+ * here to avoid other possible bugs for untested MMUs
+ */
+static inline bool mmuidx_pr(int idx) { return !(idx & 1); }
+static inline bool mmuidx_real(int idx) { return idx & 2; }
+static inline bool mmuidx_hv(int idx) { return idx & 4; }
+
#ifdef TARGET_PPC64
static inline bool ppc64_use_proc_tbl(PowerPCCPU *cpu)
diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
index cbd404bfa4..5b0e62e676 100644
--- a/target/ppc/mmu-radix64.c
+++ b/target/ppc/mmu-radix64.c
@@ -155,7 +155,7 @@ static void ppc_radix64_raise_hsi(PowerPCCPU *cpu, MMUAccessType access_type,
static bool ppc_radix64_check_prot(PowerPCCPU *cpu, MMUAccessType access_type,
uint64_t pte, int *fault_cause, int *prot,
- bool partition_scoped)
+ int mmu_idx, bool partition_scoped)
{
CPUPPCState *env = &cpu->env;
int need_prot;
@@ -173,7 +173,8 @@ static bool ppc_radix64_check_prot(PowerPCCPU *cpu, MMUAccessType access_type,
/* Determine permissions allowed by Encoded Access Authority */
if (!partition_scoped && (pte & R_PTE_EAA_PRIV) && msr_pr) {
*prot = 0;
- } else if (msr_pr || (pte & R_PTE_EAA_PRIV) || partition_scoped) {
+ } else if (mmuidx_pr(mmu_idx) || (pte & R_PTE_EAA_PRIV) ||
+ partition_scoped) {
*prot = ppc_radix64_get_prot_eaa(pte);
} else { /* !msr_pr && !(pte & R_PTE_EAA_PRIV) && !partition_scoped */
*prot = ppc_radix64_get_prot_eaa(pte);
@@ -299,7 +300,7 @@ static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu,
ppc_v3_pate_t pate,
hwaddr *h_raddr, int *h_prot,
int *h_page_size, bool pde_addr,
- bool guest_visible)
+ int mmu_idx, bool guest_visible)
{
int fault_cause = 0;
hwaddr pte_addr;
@@ -310,7 +311,8 @@ static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu,
if (ppc_radix64_walk_tree(CPU(cpu)->as, g_raddr, pate.dw0 & PRTBE_R_RPDB,
pate.dw0 & PRTBE_R_RPDS, h_raddr, h_page_size,
&pte, &fault_cause, &pte_addr) ||
- ppc_radix64_check_prot(cpu, access_type, pte, &fault_cause, h_prot, true)) {
+ ppc_radix64_check_prot(cpu, access_type, pte,
+ &fault_cause, h_prot, mmu_idx, true)) {
if (pde_addr) { /* address being translated was that of a guest pde */
fault_cause |= DSISR_PRTABLE_FAULT;
}
@@ -332,7 +334,7 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
vaddr eaddr, uint64_t pid,
ppc_v3_pate_t pate, hwaddr *g_raddr,
int *g_prot, int *g_page_size,
- bool guest_visible)
+ int mmu_idx, bool guest_visible)
{
CPUState *cs = CPU(cpu);
CPUPPCState *env = &cpu->env;
@@ -367,7 +369,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
ret = ppc_radix64_partition_scoped_xlate(cpu, 0, eaddr, prtbe_addr,
pate, &h_raddr, &h_prot,
&h_page_size, true,
- guest_visible);
+ /* mmu_idx is 5 because we're translating from hypervisor scope */
+ 5, guest_visible);
if (ret) {
return ret;
}
@@ -407,7 +410,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
ret = ppc_radix64_partition_scoped_xlate(cpu, 0, eaddr, pte_addr,
pate, &h_raddr, &h_prot,
&h_page_size, true,
- guest_visible);
+ /* mmu_idx is 5 because we're translating from hypervisor scope */
+ 5, guest_visible);
if (ret) {
return ret;
}
@@ -431,7 +435,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
*g_raddr = (rpn & ~mask) | (eaddr & mask);
}
- if (ppc_radix64_check_prot(cpu, access_type, pte, &fault_cause, g_prot, false)) {
+ if (ppc_radix64_check_prot(cpu, access_type, pte, &fault_cause,
+ g_prot, mmu_idx, false)) {
/* Access denied due to protection */
if (guest_visible) {
ppc_radix64_raise_si(cpu, access_type, eaddr, fault_cause);
@@ -464,7 +469,7 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
* +-------------+----------------+---------------+
*/
bool ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
- hwaddr *raddr, int *psizep, int *protp,
+ hwaddr *raddr, int *psizep, int *protp, int mmu_idx,
bool guest_visible)
{
CPUPPCState *env = &cpu->env;
@@ -474,17 +479,17 @@ bool ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
hwaddr g_raddr;
bool relocation;
- assert(!(msr_hv && cpu->vhyp));
+ assert(!(mmuidx_hv(mmu_idx) && cpu->vhyp));
- relocation = (access_type == MMU_INST_FETCH ? msr_ir : msr_dr);
+ relocation = !mmuidx_real(mmu_idx);
/* HV or virtual hypervisor Real Mode Access */
- if (!relocation && (msr_hv || cpu->vhyp)) {
+ if (!relocation && (mmuidx_hv(mmu_idx) || cpu->vhyp)) {
/* In real mode top 4 effective addr bits (mostly) ignored */
*raddr = eaddr & 0x0FFFFFFFFFFFFFFFULL;
/* In HV mode, add HRMOR if top EA bit is clear */
- if (msr_hv || !env->has_hv_mode) {
+ if (mmuidx_hv(mmu_idx) || !env->has_hv_mode) {
if (!(eaddr >> 63)) {
*raddr |= env->spr[SPR_HRMOR];
}
@@ -546,7 +551,7 @@ bool ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
if (relocation) {
int ret = ppc_radix64_process_scoped_xlate(cpu, access_type, eaddr, pid,
pate, &g_raddr, &prot,
- &psize, guest_visible);
+ &psize, mmu_idx, guest_visible);
if (ret) {
return false;
}
@@ -564,13 +569,13 @@ bool ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
* quadrants 1 or 2. Translates a guest real address to a host
* real address.
*/
- if (lpid || !msr_hv) {
+ if (lpid || !mmuidx_hv(mmu_idx)) {
int ret;
ret = ppc_radix64_partition_scoped_xlate(cpu, access_type, eaddr,
g_raddr, pate, raddr,
&prot, &psize, false,
- guest_visible);
+ mmu_idx, guest_visible);
if (ret) {
return false;
}
diff --git a/target/ppc/mmu-radix64.h b/target/ppc/mmu-radix64.h
index 6b13b89b64..b70357cf34 100644
--- a/target/ppc/mmu-radix64.h
+++ b/target/ppc/mmu-radix64.h
@@ -45,7 +45,7 @@
#ifdef TARGET_PPC64
bool ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
- hwaddr *raddr, int *psizep, int *protp,
+ hwaddr *raddr, int *psizep, int *protp, int mmu_idx,
bool guest_visible);
static inline int ppc_radix64_get_prot_eaa(uint64_t pte)
diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
index ba1952c77d..9dcdf88597 100644
--- a/target/ppc/mmu_helper.c
+++ b/target/ppc/mmu_helper.c
@@ -2908,7 +2908,7 @@ static bool ppc_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
case POWERPC_MMU_3_00:
if (ppc64_v3_radix(cpu)) {
return ppc_radix64_xlate(cpu, eaddr, access_type,
- raddrp, psizep, protp, guest_visible);
+ raddrp, psizep, protp, mmu_idx, guest_visible);
}
/* fall through */
case POWERPC_MMU_64B:
@@ -2941,8 +2941,10 @@ hwaddr ppc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
* try an MMU_DATA_LOAD, we may not be able to read instructions
* mapped by code TLBs, so we also try a MMU_INST_FETCH.
*/
- if (ppc_xlate(cpu, addr, MMU_DATA_LOAD, &raddr, &s, &p, 0, false) ||
- ppc_xlate(cpu, addr, MMU_INST_FETCH, &raddr, &s, &p, 0, false)) {
+ if (ppc_xlate(cpu, addr, MMU_DATA_LOAD, &raddr, &s, &p,
+ cpu_mmu_index(&cpu->env, false), false) ||
+ ppc_xlate(cpu, addr, MMU_INST_FETCH, &raddr, &s, &p,
+ cpu_mmu_index(&cpu->env, true), false)) {
return raddr & TARGET_PAGE_MASK;
}
return -1;
--
2.17.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v4 2/3] target/ppc: change ppc_hash32_xlate to use mmu_idx
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-06-28 13:36 ` Bruno Larsen (billionai)
2021-07-05 4:24 ` David Gibson
2021-06-28 13:36 ` [PATCH v4 3/3] target/ppc: changed ppc_hash64_xlate " Bruno Larsen (billionai)
2 siblings, 1 reply; 9+ messages in thread
From: Bruno Larsen (billionai) @ 2021-06-28 13:36 UTC (permalink / raw)
To: qemu-devel
Cc: farosas, richard.henderson, luis.pires, Greg Kurz, lucas.araujo,
fernando.valle, qemu-ppc, matheus.ferst, david
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).
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"
#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,
--
2.17.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v4 3/3] target/ppc: changed ppc_hash64_xlate to use mmu_idx
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-06-28 13:36 ` [PATCH v4 2/3] target/ppc: change ppc_hash32_xlate to use mmu_idx Bruno Larsen (billionai)
@ 2021-06-28 13:36 ` Bruno Larsen (billionai)
2021-07-05 4:26 ` David Gibson
2 siblings, 1 reply; 9+ messages in thread
From: Bruno Larsen (billionai) @ 2021-06-28 13:36 UTC (permalink / raw)
To: qemu-devel
Cc: farosas, richard.henderson, luis.pires, Greg Kurz, lucas.araujo,
fernando.valle, qemu-ppc, matheus.ferst, david
Changed hash64 address translation to use the supplied mmu_idx instead
of using the one stored in the msr, for parity purposes (other book3s
MMUs already use it).
Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
target/ppc/mmu-hash64.c | 43 ++++++++++++++++++++---------------------
target/ppc/mmu-hash64.h | 2 +-
target/ppc/mmu_helper.c | 2 +-
3 files changed, 23 insertions(+), 24 deletions(-)
diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index c1b98a97e9..19832c4b46 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -366,10 +366,9 @@ static inline int ppc_hash64_pte_noexec_guard(PowerPCCPU *cpu,
}
/* Check Basic Storage Protection */
-static int ppc_hash64_pte_prot(PowerPCCPU *cpu,
+static int ppc_hash64_pte_prot(int mmu_idx,
ppc_slb_t *slb, ppc_hash_pte64_t pte)
{
- CPUPPCState *env = &cpu->env;
unsigned pp, key;
/*
* Some pp bit combinations have undefined behaviour, so default
@@ -377,7 +376,7 @@ static int ppc_hash64_pte_prot(PowerPCCPU *cpu,
*/
int prot = 0;
- key = !!(msr_pr ? (slb->vsid & SLB_VSID_KP)
+ key = !!(mmuidx_pr(mmu_idx) ? (slb->vsid & SLB_VSID_KP)
: (slb->vsid & SLB_VSID_KS));
pp = (pte.pte1 & HPTE64_R_PP) | ((pte.pte1 & HPTE64_R_PP0) >> 61);
@@ -744,17 +743,17 @@ static bool ppc_hash64_use_vrma(CPUPPCState *env)
}
}
-static void ppc_hash64_set_isi(CPUState *cs, uint64_t error_code)
+static void ppc_hash64_set_isi(CPUState *cs, int mmu_idx, uint64_t error_code)
{
CPUPPCState *env = &POWERPC_CPU(cs)->env;
bool vpm;
- if (msr_ir) {
+ if (!mmuidx_real(mmu_idx)) {
vpm = !!(env->spr[SPR_LPCR] & LPCR_VPM1);
} else {
vpm = ppc_hash64_use_vrma(env);
}
- if (vpm && !msr_hv) {
+ if (vpm && !mmuidx_hv(mmu_idx)) {
cs->exception_index = POWERPC_EXCP_HISI;
} else {
cs->exception_index = POWERPC_EXCP_ISI;
@@ -762,17 +761,17 @@ static void ppc_hash64_set_isi(CPUState *cs, uint64_t error_code)
env->error_code = error_code;
}
-static void ppc_hash64_set_dsi(CPUState *cs, uint64_t dar, uint64_t dsisr)
+static void ppc_hash64_set_dsi(CPUState *cs, int mmu_idx, uint64_t dar, uint64_t dsisr)
{
CPUPPCState *env = &POWERPC_CPU(cs)->env;
bool vpm;
- if (msr_dr) {
+ if (!mmuidx_real(mmu_idx)) {
vpm = !!(env->spr[SPR_LPCR] & LPCR_VPM1);
} else {
vpm = ppc_hash64_use_vrma(env);
}
- if (vpm && !msr_hv) {
+ if (vpm && !mmuidx_hv(mmu_idx)) {
cs->exception_index = POWERPC_EXCP_HDSI;
env->spr[SPR_HDAR] = dar;
env->spr[SPR_HDSISR] = dsisr;
@@ -874,7 +873,7 @@ static int build_vrma_slbe(PowerPCCPU *cpu, ppc_slb_t *slb)
}
bool ppc_hash64_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);
@@ -897,7 +896,7 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
*/
/* 1. Handle real mode accesses */
- if (access_type == MMU_INST_FETCH ? !msr_ir : !msr_dr) {
+ if (mmuidx_real(mmu_idx)) {
/*
* Translation is supposedly "off", but in real mode the top 4
* effective address bits are (mostly) ignored
@@ -909,7 +908,7 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
* In virtual hypervisor mode, there's nothing to do:
* EA == GPA == qemu guest address
*/
- } else if (msr_hv || !env->has_hv_mode) {
+ } else if (mmuidx_hv(mmu_idx) || !env->has_hv_mode) {
/* In HV mode, add HRMOR if top EA bit is clear */
if (!(eaddr >> 63)) {
raddr |= env->spr[SPR_HRMOR];
@@ -937,13 +936,13 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
}
switch (access_type) {
case MMU_INST_FETCH:
- ppc_hash64_set_isi(cs, SRR1_PROTFAULT);
+ ppc_hash64_set_isi(cs, mmu_idx, SRR1_PROTFAULT);
break;
case MMU_DATA_LOAD:
- ppc_hash64_set_dsi(cs, eaddr, DSISR_PROTFAULT);
+ ppc_hash64_set_dsi(cs, mmu_idx, eaddr, DSISR_PROTFAULT);
break;
case MMU_DATA_STORE:
- ppc_hash64_set_dsi(cs, eaddr,
+ ppc_hash64_set_dsi(cs, mmu_idx, eaddr,
DSISR_PROTFAULT | DSISR_ISSTORE);
break;
default:
@@ -996,7 +995,7 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
/* 3. Check for segment level no-execute violation */
if (access_type == MMU_INST_FETCH && (slb->vsid & SLB_VSID_N)) {
if (guest_visible) {
- ppc_hash64_set_isi(cs, SRR1_NOEXEC_GUARD);
+ ppc_hash64_set_isi(cs, mmu_idx, SRR1_NOEXEC_GUARD);
}
return false;
}
@@ -1009,13 +1008,13 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
}
switch (access_type) {
case MMU_INST_FETCH:
- ppc_hash64_set_isi(cs, SRR1_NOPTE);
+ ppc_hash64_set_isi(cs, mmu_idx, SRR1_NOPTE);
break;
case MMU_DATA_LOAD:
- ppc_hash64_set_dsi(cs, eaddr, DSISR_NOPTE);
+ ppc_hash64_set_dsi(cs, mmu_idx, eaddr, DSISR_NOPTE);
break;
case MMU_DATA_STORE:
- ppc_hash64_set_dsi(cs, eaddr, DSISR_NOPTE | DSISR_ISSTORE);
+ ppc_hash64_set_dsi(cs, mmu_idx, eaddr, DSISR_NOPTE | DSISR_ISSTORE);
break;
default:
g_assert_not_reached();
@@ -1028,7 +1027,7 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
/* 5. Check access permissions */
exec_prot = ppc_hash64_pte_noexec_guard(cpu, pte);
- pp_prot = ppc_hash64_pte_prot(cpu, slb, pte);
+ pp_prot = ppc_hash64_pte_prot(mmu_idx, slb, pte);
amr_prot = ppc_hash64_amr_prot(cpu, pte);
prot = exec_prot & pp_prot & amr_prot;
@@ -1049,7 +1048,7 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
if (PAGE_EXEC & ~amr_prot) {
srr1 |= SRR1_IAMR; /* Access violates virt pg class key prot */
}
- ppc_hash64_set_isi(cs, srr1);
+ ppc_hash64_set_isi(cs, mmu_idx, srr1);
} else {
int dsisr = 0;
if (need_prot & ~pp_prot) {
@@ -1061,7 +1060,7 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
if (need_prot & ~amr_prot) {
dsisr |= DSISR_AMR;
}
- ppc_hash64_set_dsi(cs, eaddr, dsisr);
+ ppc_hash64_set_dsi(cs, mmu_idx, eaddr, dsisr);
}
return false;
}
diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
index 9f338e1fe9..c5b2f97ff7 100644
--- a/target/ppc/mmu-hash64.h
+++ b/target/ppc/mmu-hash64.h
@@ -8,7 +8,7 @@ void dump_slb(PowerPCCPU *cpu);
int ppc_store_slb(PowerPCCPU *cpu, target_ulong slot,
target_ulong esid, target_ulong vsid);
bool ppc_hash64_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);
void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu,
target_ulong pte_index,
diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
index a3381e1aa0..0816c889a3 100644
--- a/target/ppc/mmu_helper.c
+++ b/target/ppc/mmu_helper.c
@@ -2916,7 +2916,7 @@ static bool ppc_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
case POWERPC_MMU_2_06:
case POWERPC_MMU_2_07:
return ppc_hash64_xlate(cpu, eaddr, access_type,
- raddrp, psizep, protp, guest_visible);
+ raddrp, psizep, protp, mmu_idx, guest_visible);
#endif
case POWERPC_MMU_32B:
--
2.17.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/3] target/ppc: fix address translation bug for radix mmus
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
0 siblings, 0 replies; 9+ messages in thread
From: David Gibson @ 2021-07-05 4:17 UTC (permalink / raw)
To: Bruno Larsen (billionai)
Cc: farosas, richard.henderson, qemu-devel, Greg Kurz, lucas.araujo,
fernando.valle, qemu-ppc, matheus.ferst, luis.pires
[-- Attachment #1: Type: text/plain, Size: 12793 bytes --]
On Mon, Jun 28, 2021 at 10:36:08AM -0300, Bruno Larsen (billionai) wrote:
> This commit attempts to fix a technical hiccup first mentioned by Richard
> Henderson in
> https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06247.html
>
> To sumarize the hiccup here, when radix-style mmus are translating an
> address, they might need to call a second level of translation, with
> hypervisor privileges. However, the way it was being done up until
> this point meant that the second level translation had the same
> privileges as the first level. It could lead to a bug in address
> translation when running KVM inside a TCG guest, but this bug was never
> experienced by users, so this isn't as much a bug fix as it is a
> correctness cleanup.
>
> This patch attempts that cleanup by making radix64_*_xlate functions
> receive the mmu_idx, and passing one with the correct permission for the
> second level translation.
>
> The mmuidx macros added by this patch are only correct for non-bookE
> mmus, because BookE style set the IS and DS bits inverted and there
> might be other subtle differences. However, there doesn't seem to be
> BookE cpus that have radix-style mmus, so we left a comment there to
> document the issue, in case a machine does have that and was missed.
>
> As part of this cleanup, we now need to send the correct mmmu_idx
> when calling get_phys_page_debug, otherwise we might not be able to see the
> memory that the CPU could
>
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Tested-by: Cédric Le Goater <clg@kaod.org>
Applied to ppc-for-6.1, thanks.
> ---
> target/ppc/mmu-book3s-v3.h | 13 +++++++++++++
> target/ppc/mmu-radix64.c | 37 +++++++++++++++++++++----------------
> target/ppc/mmu-radix64.h | 2 +-
> target/ppc/mmu_helper.c | 8 +++++---
> 4 files changed, 40 insertions(+), 20 deletions(-)
>
> diff --git a/target/ppc/mmu-book3s-v3.h b/target/ppc/mmu-book3s-v3.h
> index a1326df969..c89d0bccfd 100644
> --- a/target/ppc/mmu-book3s-v3.h
> +++ b/target/ppc/mmu-book3s-v3.h
> @@ -47,6 +47,19 @@ struct prtb_entry {
> uint64_t prtbe0, prtbe1;
> };
>
> +/*
> + * These correspond to the mmu_idx values computed in
> + * hreg_compute_hflags_value. See the tables therein
> + *
> + * They are here because some bits are inverted for BookE MMUs
> + * not necessarily because they only work for BookS. However,
> + * we only needed to change BookS MMUs, we left the functions
> + * here to avoid other possible bugs for untested MMUs
> + */
> +static inline bool mmuidx_pr(int idx) { return !(idx & 1); }
> +static inline bool mmuidx_real(int idx) { return idx & 2; }
> +static inline bool mmuidx_hv(int idx) { return idx & 4; }
> +
> #ifdef TARGET_PPC64
>
> static inline bool ppc64_use_proc_tbl(PowerPCCPU *cpu)
> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
> index cbd404bfa4..5b0e62e676 100644
> --- a/target/ppc/mmu-radix64.c
> +++ b/target/ppc/mmu-radix64.c
> @@ -155,7 +155,7 @@ static void ppc_radix64_raise_hsi(PowerPCCPU *cpu, MMUAccessType access_type,
>
> static bool ppc_radix64_check_prot(PowerPCCPU *cpu, MMUAccessType access_type,
> uint64_t pte, int *fault_cause, int *prot,
> - bool partition_scoped)
> + int mmu_idx, bool partition_scoped)
> {
> CPUPPCState *env = &cpu->env;
> int need_prot;
> @@ -173,7 +173,8 @@ static bool ppc_radix64_check_prot(PowerPCCPU *cpu, MMUAccessType access_type,
> /* Determine permissions allowed by Encoded Access Authority */
> if (!partition_scoped && (pte & R_PTE_EAA_PRIV) && msr_pr) {
> *prot = 0;
> - } else if (msr_pr || (pte & R_PTE_EAA_PRIV) || partition_scoped) {
> + } else if (mmuidx_pr(mmu_idx) || (pte & R_PTE_EAA_PRIV) ||
> + partition_scoped) {
> *prot = ppc_radix64_get_prot_eaa(pte);
> } else { /* !msr_pr && !(pte & R_PTE_EAA_PRIV) && !partition_scoped */
> *prot = ppc_radix64_get_prot_eaa(pte);
> @@ -299,7 +300,7 @@ static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu,
> ppc_v3_pate_t pate,
> hwaddr *h_raddr, int *h_prot,
> int *h_page_size, bool pde_addr,
> - bool guest_visible)
> + int mmu_idx, bool guest_visible)
> {
> int fault_cause = 0;
> hwaddr pte_addr;
> @@ -310,7 +311,8 @@ static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu,
> if (ppc_radix64_walk_tree(CPU(cpu)->as, g_raddr, pate.dw0 & PRTBE_R_RPDB,
> pate.dw0 & PRTBE_R_RPDS, h_raddr, h_page_size,
> &pte, &fault_cause, &pte_addr) ||
> - ppc_radix64_check_prot(cpu, access_type, pte, &fault_cause, h_prot, true)) {
> + ppc_radix64_check_prot(cpu, access_type, pte,
> + &fault_cause, h_prot, mmu_idx, true)) {
> if (pde_addr) { /* address being translated was that of a guest pde */
> fault_cause |= DSISR_PRTABLE_FAULT;
> }
> @@ -332,7 +334,7 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
> vaddr eaddr, uint64_t pid,
> ppc_v3_pate_t pate, hwaddr *g_raddr,
> int *g_prot, int *g_page_size,
> - bool guest_visible)
> + int mmu_idx, bool guest_visible)
> {
> CPUState *cs = CPU(cpu);
> CPUPPCState *env = &cpu->env;
> @@ -367,7 +369,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
> ret = ppc_radix64_partition_scoped_xlate(cpu, 0, eaddr, prtbe_addr,
> pate, &h_raddr, &h_prot,
> &h_page_size, true,
> - guest_visible);
> + /* mmu_idx is 5 because we're translating from hypervisor scope */
> + 5, guest_visible);
> if (ret) {
> return ret;
> }
> @@ -407,7 +410,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
> ret = ppc_radix64_partition_scoped_xlate(cpu, 0, eaddr, pte_addr,
> pate, &h_raddr, &h_prot,
> &h_page_size, true,
> - guest_visible);
> + /* mmu_idx is 5 because we're translating from hypervisor scope */
> + 5, guest_visible);
> if (ret) {
> return ret;
> }
> @@ -431,7 +435,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
> *g_raddr = (rpn & ~mask) | (eaddr & mask);
> }
>
> - if (ppc_radix64_check_prot(cpu, access_type, pte, &fault_cause, g_prot, false)) {
> + if (ppc_radix64_check_prot(cpu, access_type, pte, &fault_cause,
> + g_prot, mmu_idx, false)) {
> /* Access denied due to protection */
> if (guest_visible) {
> ppc_radix64_raise_si(cpu, access_type, eaddr, fault_cause);
> @@ -464,7 +469,7 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
> * +-------------+----------------+---------------+
> */
> bool ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> - hwaddr *raddr, int *psizep, int *protp,
> + hwaddr *raddr, int *psizep, int *protp, int mmu_idx,
> bool guest_visible)
> {
> CPUPPCState *env = &cpu->env;
> @@ -474,17 +479,17 @@ bool ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> hwaddr g_raddr;
> bool relocation;
>
> - assert(!(msr_hv && cpu->vhyp));
> + assert(!(mmuidx_hv(mmu_idx) && cpu->vhyp));
>
> - relocation = (access_type == MMU_INST_FETCH ? msr_ir : msr_dr);
> + relocation = !mmuidx_real(mmu_idx);
>
> /* HV or virtual hypervisor Real Mode Access */
> - if (!relocation && (msr_hv || cpu->vhyp)) {
> + if (!relocation && (mmuidx_hv(mmu_idx) || cpu->vhyp)) {
> /* In real mode top 4 effective addr bits (mostly) ignored */
> *raddr = eaddr & 0x0FFFFFFFFFFFFFFFULL;
>
> /* In HV mode, add HRMOR if top EA bit is clear */
> - if (msr_hv || !env->has_hv_mode) {
> + if (mmuidx_hv(mmu_idx) || !env->has_hv_mode) {
> if (!(eaddr >> 63)) {
> *raddr |= env->spr[SPR_HRMOR];
> }
> @@ -546,7 +551,7 @@ bool ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> if (relocation) {
> int ret = ppc_radix64_process_scoped_xlate(cpu, access_type, eaddr, pid,
> pate, &g_raddr, &prot,
> - &psize, guest_visible);
> + &psize, mmu_idx, guest_visible);
> if (ret) {
> return false;
> }
> @@ -564,13 +569,13 @@ bool ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> * quadrants 1 or 2. Translates a guest real address to a host
> * real address.
> */
> - if (lpid || !msr_hv) {
> + if (lpid || !mmuidx_hv(mmu_idx)) {
> int ret;
>
> ret = ppc_radix64_partition_scoped_xlate(cpu, access_type, eaddr,
> g_raddr, pate, raddr,
> &prot, &psize, false,
> - guest_visible);
> + mmu_idx, guest_visible);
> if (ret) {
> return false;
> }
> diff --git a/target/ppc/mmu-radix64.h b/target/ppc/mmu-radix64.h
> index 6b13b89b64..b70357cf34 100644
> --- a/target/ppc/mmu-radix64.h
> +++ b/target/ppc/mmu-radix64.h
> @@ -45,7 +45,7 @@
> #ifdef TARGET_PPC64
>
> bool ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> - hwaddr *raddr, int *psizep, int *protp,
> + hwaddr *raddr, int *psizep, int *protp, int mmu_idx,
> bool guest_visible);
>
> static inline int ppc_radix64_get_prot_eaa(uint64_t pte)
> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> index ba1952c77d..9dcdf88597 100644
> --- a/target/ppc/mmu_helper.c
> +++ b/target/ppc/mmu_helper.c
> @@ -2908,7 +2908,7 @@ static bool ppc_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> case POWERPC_MMU_3_00:
> if (ppc64_v3_radix(cpu)) {
> return ppc_radix64_xlate(cpu, eaddr, access_type,
> - raddrp, psizep, protp, guest_visible);
> + raddrp, psizep, protp, mmu_idx, guest_visible);
> }
> /* fall through */
> case POWERPC_MMU_64B:
> @@ -2941,8 +2941,10 @@ hwaddr ppc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
> * try an MMU_DATA_LOAD, we may not be able to read instructions
> * mapped by code TLBs, so we also try a MMU_INST_FETCH.
> */
> - if (ppc_xlate(cpu, addr, MMU_DATA_LOAD, &raddr, &s, &p, 0, false) ||
> - ppc_xlate(cpu, addr, MMU_INST_FETCH, &raddr, &s, &p, 0, false)) {
> + if (ppc_xlate(cpu, addr, MMU_DATA_LOAD, &raddr, &s, &p,
> + cpu_mmu_index(&cpu->env, false), false) ||
> + ppc_xlate(cpu, addr, MMU_INST_FETCH, &raddr, &s, &p,
> + cpu_mmu_index(&cpu->env, true), false)) {
> return raddr & TARGET_PAGE_MASK;
> }
> return -1;
--
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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 2/3] target/ppc: change ppc_hash32_xlate to use mmu_idx
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
2021-07-05 14:31 ` Bruno Piazera Larsen
0 siblings, 1 reply; 9+ messages in thread
From: David Gibson @ 2021-07-05 4:24 UTC (permalink / raw)
To: Bruno Larsen (billionai)
Cc: farosas, richard.henderson, qemu-devel, Greg Kurz, lucas.araujo,
fernando.valle, qemu-ppc, matheus.ferst, luis.pires
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 3/3] target/ppc: changed ppc_hash64_xlate to use mmu_idx
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
0 siblings, 0 replies; 9+ messages in thread
From: David Gibson @ 2021-07-05 4:26 UTC (permalink / raw)
To: Bruno Larsen (billionai)
Cc: farosas, richard.henderson, qemu-devel, Greg Kurz, lucas.araujo,
fernando.valle, qemu-ppc, matheus.ferst, luis.pires
[-- Attachment #1: Type: text/plain, Size: 9370 bytes --]
On Mon, Jun 28, 2021 at 10:36:10AM -0300, Bruno Larsen (billionai) wrote:
65;6402;1c> Changed hash64 address translation to use the supplied mmu_idx instead
> of using the one stored in the msr, for parity purposes (other book3s
> MMUs already use it).
>
> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Applied to ppc-for-6.1, thanks.
> ---
> target/ppc/mmu-hash64.c | 43 ++++++++++++++++++++---------------------
> target/ppc/mmu-hash64.h | 2 +-
> target/ppc/mmu_helper.c | 2 +-
> 3 files changed, 23 insertions(+), 24 deletions(-)
>
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index c1b98a97e9..19832c4b46 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -366,10 +366,9 @@ static inline int ppc_hash64_pte_noexec_guard(PowerPCCPU *cpu,
> }
>
> /* Check Basic Storage Protection */
> -static int ppc_hash64_pte_prot(PowerPCCPU *cpu,
> +static int ppc_hash64_pte_prot(int mmu_idx,
> ppc_slb_t *slb, ppc_hash_pte64_t pte)
> {
> - CPUPPCState *env = &cpu->env;
> unsigned pp, key;
> /*
> * Some pp bit combinations have undefined behaviour, so default
> @@ -377,7 +376,7 @@ static int ppc_hash64_pte_prot(PowerPCCPU *cpu,
> */
> int prot = 0;
>
> - key = !!(msr_pr ? (slb->vsid & SLB_VSID_KP)
> + key = !!(mmuidx_pr(mmu_idx) ? (slb->vsid & SLB_VSID_KP)
> : (slb->vsid & SLB_VSID_KS));
> pp = (pte.pte1 & HPTE64_R_PP) | ((pte.pte1 & HPTE64_R_PP0) >> 61);
>
> @@ -744,17 +743,17 @@ static bool ppc_hash64_use_vrma(CPUPPCState *env)
> }
> }
>
> -static void ppc_hash64_set_isi(CPUState *cs, uint64_t error_code)
> +static void ppc_hash64_set_isi(CPUState *cs, int mmu_idx, uint64_t error_code)
> {
> CPUPPCState *env = &POWERPC_CPU(cs)->env;
> bool vpm;
>
> - if (msr_ir) {
> + if (!mmuidx_real(mmu_idx)) {
> vpm = !!(env->spr[SPR_LPCR] & LPCR_VPM1);
> } else {
> vpm = ppc_hash64_use_vrma(env);
> }
> - if (vpm && !msr_hv) {
> + if (vpm && !mmuidx_hv(mmu_idx)) {
> cs->exception_index = POWERPC_EXCP_HISI;
> } else {
> cs->exception_index = POWERPC_EXCP_ISI;
> @@ -762,17 +761,17 @@ static void ppc_hash64_set_isi(CPUState *cs, uint64_t error_code)
> env->error_code = error_code;
> }
>
> -static void ppc_hash64_set_dsi(CPUState *cs, uint64_t dar, uint64_t dsisr)
> +static void ppc_hash64_set_dsi(CPUState *cs, int mmu_idx, uint64_t dar, uint64_t dsisr)
> {
> CPUPPCState *env = &POWERPC_CPU(cs)->env;
> bool vpm;
>
> - if (msr_dr) {
> + if (!mmuidx_real(mmu_idx)) {
> vpm = !!(env->spr[SPR_LPCR] & LPCR_VPM1);
> } else {
> vpm = ppc_hash64_use_vrma(env);
> }
> - if (vpm && !msr_hv) {
> + if (vpm && !mmuidx_hv(mmu_idx)) {
> cs->exception_index = POWERPC_EXCP_HDSI;
> env->spr[SPR_HDAR] = dar;
> env->spr[SPR_HDSISR] = dsisr;
> @@ -874,7 +873,7 @@ static int build_vrma_slbe(PowerPCCPU *cpu, ppc_slb_t *slb)
> }
>
> bool ppc_hash64_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);
> @@ -897,7 +896,7 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> */
>
> /* 1. Handle real mode accesses */
> - if (access_type == MMU_INST_FETCH ? !msr_ir : !msr_dr) {
> + if (mmuidx_real(mmu_idx)) {
> /*
> * Translation is supposedly "off", but in real mode the top 4
> * effective address bits are (mostly) ignored
> @@ -909,7 +908,7 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> * In virtual hypervisor mode, there's nothing to do:
> * EA == GPA == qemu guest address
> */
> - } else if (msr_hv || !env->has_hv_mode) {
> + } else if (mmuidx_hv(mmu_idx) || !env->has_hv_mode) {
> /* In HV mode, add HRMOR if top EA bit is clear */
> if (!(eaddr >> 63)) {
> raddr |= env->spr[SPR_HRMOR];
> @@ -937,13 +936,13 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> }
> switch (access_type) {
> case MMU_INST_FETCH:
> - ppc_hash64_set_isi(cs, SRR1_PROTFAULT);
> + ppc_hash64_set_isi(cs, mmu_idx, SRR1_PROTFAULT);
> break;
> case MMU_DATA_LOAD:
> - ppc_hash64_set_dsi(cs, eaddr, DSISR_PROTFAULT);
> + ppc_hash64_set_dsi(cs, mmu_idx, eaddr, DSISR_PROTFAULT);
> break;
> case MMU_DATA_STORE:
> - ppc_hash64_set_dsi(cs, eaddr,
> + ppc_hash64_set_dsi(cs, mmu_idx, eaddr,
> DSISR_PROTFAULT | DSISR_ISSTORE);
> break;
> default:
> @@ -996,7 +995,7 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> /* 3. Check for segment level no-execute violation */
> if (access_type == MMU_INST_FETCH && (slb->vsid & SLB_VSID_N)) {
> if (guest_visible) {
> - ppc_hash64_set_isi(cs, SRR1_NOEXEC_GUARD);
> + ppc_hash64_set_isi(cs, mmu_idx, SRR1_NOEXEC_GUARD);
> }
> return false;
> }
> @@ -1009,13 +1008,13 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> }
> switch (access_type) {
> case MMU_INST_FETCH:
> - ppc_hash64_set_isi(cs, SRR1_NOPTE);
> + ppc_hash64_set_isi(cs, mmu_idx, SRR1_NOPTE);
> break;
> case MMU_DATA_LOAD:
> - ppc_hash64_set_dsi(cs, eaddr, DSISR_NOPTE);
> + ppc_hash64_set_dsi(cs, mmu_idx, eaddr, DSISR_NOPTE);
> break;
> case MMU_DATA_STORE:
> - ppc_hash64_set_dsi(cs, eaddr, DSISR_NOPTE | DSISR_ISSTORE);
> + ppc_hash64_set_dsi(cs, mmu_idx, eaddr, DSISR_NOPTE | DSISR_ISSTORE);
> break;
> default:
> g_assert_not_reached();
> @@ -1028,7 +1027,7 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> /* 5. Check access permissions */
>
> exec_prot = ppc_hash64_pte_noexec_guard(cpu, pte);
> - pp_prot = ppc_hash64_pte_prot(cpu, slb, pte);
> + pp_prot = ppc_hash64_pte_prot(mmu_idx, slb, pte);
> amr_prot = ppc_hash64_amr_prot(cpu, pte);
> prot = exec_prot & pp_prot & amr_prot;
>
> @@ -1049,7 +1048,7 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> if (PAGE_EXEC & ~amr_prot) {
> srr1 |= SRR1_IAMR; /* Access violates virt pg class key prot */
> }
> - ppc_hash64_set_isi(cs, srr1);
> + ppc_hash64_set_isi(cs, mmu_idx, srr1);
> } else {
> int dsisr = 0;
> if (need_prot & ~pp_prot) {
> @@ -1061,7 +1060,7 @@ bool ppc_hash64_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> if (need_prot & ~amr_prot) {
> dsisr |= DSISR_AMR;
> }
> - ppc_hash64_set_dsi(cs, eaddr, dsisr);
> + ppc_hash64_set_dsi(cs, mmu_idx, eaddr, dsisr);
> }
> return false;
> }
> diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
> index 9f338e1fe9..c5b2f97ff7 100644
> --- a/target/ppc/mmu-hash64.h
> +++ b/target/ppc/mmu-hash64.h
> @@ -8,7 +8,7 @@ void dump_slb(PowerPCCPU *cpu);
> int ppc_store_slb(PowerPCCPU *cpu, target_ulong slot,
> target_ulong esid, target_ulong vsid);
> bool ppc_hash64_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);
> void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu,
> target_ulong pte_index,
> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> index a3381e1aa0..0816c889a3 100644
> --- a/target/ppc/mmu_helper.c
> +++ b/target/ppc/mmu_helper.c
> @@ -2916,7 +2916,7 @@ static bool ppc_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> case POWERPC_MMU_2_06:
> case POWERPC_MMU_2_07:
> return ppc_hash64_xlate(cpu, eaddr, access_type,
> - raddrp, psizep, protp, guest_visible);
> + raddrp, psizep, protp, mmu_idx, guest_visible);
> #endif
>
> case POWERPC_MMU_32B:
--
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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 2/3] target/ppc: change ppc_hash32_xlate to use mmu_idx
2021-07-05 4:24 ` David Gibson
@ 2021-07-05 14:31 ` Bruno Piazera Larsen
2021-07-06 1:35 ` David Gibson
0 siblings, 1 reply; 9+ messages in thread
From: Bruno Piazera Larsen @ 2021-07-05 14:31 UTC (permalink / raw)
To: David Gibson
Cc: farosas, richard.henderson, qemu-devel, Greg Kurz, lucas.araujo,
fernando.valle, qemu-ppc, matheus.ferst, luis.pires
[-- Attachment #1: Type: text/plain, Size: 1242 bytes --]
On 05/07/2021 01:24, David Gibson 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.
Fair point, I can change the description if I do end up with a new version, but
> 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).
This doesn't work directly. I'd need to put in an ifndef PPC_MMU_BOOK3S_V3_H, which also feels a bit dubious to me. I can go with whichever one you prefer
--
Bruno Piazera Larsen
Instituto de Pesquisas ELDORADO
<https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station>
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
[-- Attachment #2: Type: text/html, Size: 1719 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 2/3] target/ppc: change ppc_hash32_xlate to use mmu_idx
2021-07-05 14:31 ` Bruno Piazera Larsen
@ 2021-07-06 1:35 ` David Gibson
0 siblings, 0 replies; 9+ messages in thread
From: David Gibson @ 2021-07-06 1:35 UTC (permalink / raw)
To: Bruno Piazera Larsen
Cc: farosas, richard.henderson, qemu-devel, Greg Kurz, lucas.araujo,
fernando.valle, qemu-ppc, matheus.ferst, luis.pires
[-- Attachment #1: Type: text/plain, Size: 1685 bytes --]
On Mon, Jul 05, 2021 at 11:31:13AM -0300, Bruno Piazera Larsen wrote:
>
> On 05/07/2021 01:24, David Gibson 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.
>
>
> Fair point, I can change the description if I do end up with a new
> version, but
>
> > 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).
>
> This doesn't work directly. I'd need to put in an ifndef
> PPC_MMU_BOOK3S_V3_H, which also feels a bit dubious to me. I can go
> with whichever one you prefer
Ah... good point, because both headers are included in some places.
Ok, in that case let's jump ahead instead. Let's create a new
mmu-books.h to cover all MMUs based on the "classic" powerpc model,
and put them in there. hash32 and book3s-v3 can include that, BookE,
4xx etc. will not.
For now these will be the only things in there, but there will
probably be more we can add later on.
--
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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-07-06 1:41 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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
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.