* [PATCH v2 1/2] target/ppc: fix address translation bug for radix mmus
@ 2021-06-14 19:16 Bruno Larsen (billionai)
2021-06-14 19:16 ` [RFC PATCH v2 2/2] target/ppc: make gdb able to translate priviledged addresses Bruno Larsen (billionai)
` (4 more replies)
0 siblings, 5 replies; 24+ messages in thread
From: Bruno Larsen (billionai) @ 2021-06-14 19:16 UTC (permalink / raw)
To: qemu-devel
Cc: farosas, richard.henderson, luis.pires, Greg Kurz, lucas.araujo,
fernando.valle, qemu-ppc, matheus.ferst, david
Based-on: <20210518201146.794854-1-richard.henderson@linaro.org>
This commit attempts to fix the first bug mentioned by Richard Henderson in
https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06247.html
To sumarize the bug here, when radix-style mmus are translating an
address, they might need to call a second level of translation, with
hypvervisor priviledges. However, the way it was being done up until
this point meant that the second level translation had the same
priviledges as the first level. This would only happen when a TCG guest
was emulating KVM, which is why it hasn't been discovered yet.
This patch attempts to correct that 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>
---
target/ppc/internal.h | 12 ++++++++++++
target/ppc/mmu-radix64.c | 38 ++++++++++++++++++++++----------------
target/ppc/mmu-radix64.h | 2 +-
target/ppc/mmu_helper.c | 8 +++++---
4 files changed, 40 insertions(+), 20 deletions(-)
diff --git a/target/ppc/internal.h b/target/ppc/internal.h
index f1fd3c8d04..003df7e8a9 100644
--- a/target/ppc/internal.h
+++ b/target/ppc/internal.h
@@ -245,4 +245,16 @@ static inline int prot_for_access_type(MMUAccessType access_type)
g_assert_not_reached();
}
+/*
+ * These correspond to the mmu_idx values computed in
+ * hreg_compute_hflags_value. See the tables therein
+ */
+static inline bool mmuidx_pr(int idx) { return !(idx & 1); }
+/*
+ * If we want to use these macros for hash-style MMUs, we need to
+ * add an if or another macro here.
+ */
+static inline bool mmuidx_real(int idx) { return idx & 2; }
+static inline bool mmuidx_hv(int idx) { return idx & 4; }
+
#endif /* PPC_INTERNAL_H */
diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
index cbd404bfa4..0ae8f6b572 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,9 @@ 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 +335,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 +370,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 +411,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 +436,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 +470,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 +480,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 +552,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 +570,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] 24+ messages in thread
* [RFC PATCH v2 2/2] target/ppc: make gdb able to translate priviledged addresses
2021-06-14 19:16 [PATCH v2 1/2] target/ppc: fix address translation bug for radix mmus Bruno Larsen (billionai)
@ 2021-06-14 19:16 ` Bruno Larsen (billionai)
2021-06-14 19:37 ` Philippe Mathieu-Daudé
` (2 more replies)
2021-06-14 19:29 ` [PATCH v2 1/2] target/ppc: fix address translation bug for radix mmus Greg Kurz
` (3 subsequent siblings)
4 siblings, 3 replies; 24+ messages in thread
From: Bruno Larsen (billionai) @ 2021-06-14 19:16 UTC (permalink / raw)
To: qemu-devel
Cc: farosas, richard.henderson, luis.pires, Greg Kurz, lucas.araujo,
fernando.valle, qemu-ppc, matheus.ferst, david
This patch changes ppc_cpu_get_phys_page_debug so that it is now
able to translate both, priviledged and real mode addresses
independently of whether the CPU executing it has those permissions
This was mentioned by Fabiano as something that would be very useful to
help with debugging, but could possibly constitute a security issue if
that debug function can be called in some way by prodution code. the
solution was implemented such that it would be trivial to wrap it around
ifdefs for building only with --enable-debug, for instance, but we are
not sure this is the best approach, hence why it is an RFC.
Suggested-by: Fabiano Rosas <farosas@linux.ibm.com>
Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
---
target/ppc/mmu_helper.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
index 9dcdf88597..41c727c690 100644
--- a/target/ppc/mmu_helper.c
+++ b/target/ppc/mmu_helper.c
@@ -2947,6 +2947,29 @@ hwaddr ppc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
cpu_mmu_index(&cpu->env, true), false)) {
return raddr & TARGET_PAGE_MASK;
}
+
+ /*
+ * This is a fallback, in case we're asking for priviledged memory to
+ * be printed, but the PCU is not executing in a priviledged manner.
+ *
+ * The code could be considered a security vulnerability if
+ * this function can be called in a scenario that does not involve
+ * debugging.
+ * Given the name and how useful using real addresses may be for
+ * actually debugging, however, we decided to include it anyway and
+ * discuss how to best avoid the possible security concerns.
+ * The current plan is that, if there is a chance this code is called in
+ * a production environment, we can surround it with ifdefs so that it
+ * is only compiled with --enable-debug
+ */
+ /* attempt to translate first with virtual addresses */
+ if (ppc_xlate(cpu, addr, MMU_DATA_LOAD, &raddr, &s, &p, 1, false) ||
+ ppc_xlate(cpu, addr, MMU_INST_FETCH, &raddr, &s, &p, 1, false) ||
+ /* if didn't work, attempt to translate with real addresses */
+ ppc_xlate(cpu, addr, MMU_DATA_LOAD, &raddr, &s, &p, 3, false) ||
+ ppc_xlate(cpu, addr, MMU_INST_FETCH, &raddr, &s, &p, 3, false)) {
+ return raddr & TARGET_PAGE_MASK;
+ }
return -1;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] target/ppc: fix address translation bug for radix mmus
2021-06-14 19:16 [PATCH v2 1/2] target/ppc: fix address translation bug for radix mmus Bruno Larsen (billionai)
2021-06-14 19:16 ` [RFC PATCH v2 2/2] target/ppc: make gdb able to translate priviledged addresses Bruno Larsen (billionai)
@ 2021-06-14 19:29 ` Greg Kurz
2021-06-14 21:04 ` Fabiano Rosas
` (2 subsequent siblings)
4 siblings, 0 replies; 24+ messages in thread
From: Greg Kurz @ 2021-06-14 19:29 UTC (permalink / raw)
To: Bruno Larsen (billionai)
Cc: farosas, richard.henderson, luis.pires, qemu-devel, lucas.araujo,
fernando.valle, qemu-ppc, Cédric Le Goater, matheus.ferst,
david
On Mon, 14 Jun 2021 16:16:29 -0300
"Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> wrote:
> Based-on: <20210518201146.794854-1-richard.henderson@linaro.org>
>
> This commit attempts to fix the first bug mentioned by Richard Henderson in
> https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06247.html
>
> To sumarize the bug here, when radix-style mmus are translating an
> address, they might need to call a second level of translation, with
> hypvervisor priviledges. However, the way it was being done up until
> this point meant that the second level translation had the same
> priviledges as the first level. This would only happen when a TCG guest
> was emulating KVM, which is why it hasn't been discovered yet.
>
Cédric is definitely the person to Cc: for stuff like this :)
> This patch attempts to correct that 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>
> ---
> target/ppc/internal.h | 12 ++++++++++++
> target/ppc/mmu-radix64.c | 38 ++++++++++++++++++++++----------------
> target/ppc/mmu-radix64.h | 2 +-
> target/ppc/mmu_helper.c | 8 +++++---
> 4 files changed, 40 insertions(+), 20 deletions(-)
>
> diff --git a/target/ppc/internal.h b/target/ppc/internal.h
> index f1fd3c8d04..003df7e8a9 100644
> --- a/target/ppc/internal.h
> +++ b/target/ppc/internal.h
> @@ -245,4 +245,16 @@ static inline int prot_for_access_type(MMUAccessType access_type)
> g_assert_not_reached();
> }
>
> +/*
> + * These correspond to the mmu_idx values computed in
> + * hreg_compute_hflags_value. See the tables therein
> + */
> +static inline bool mmuidx_pr(int idx) { return !(idx & 1); }
> +/*
> + * If we want to use these macros for hash-style MMUs, we need to
> + * add an if or another macro here.
> + */
> +static inline bool mmuidx_real(int idx) { return idx & 2; }
> +static inline bool mmuidx_hv(int idx) { return idx & 4; }
> +
> #endif /* PPC_INTERNAL_H */
> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
> index cbd404bfa4..0ae8f6b572 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,9 @@ 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 +335,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 +370,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 +411,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 +436,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 +470,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 +480,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 +552,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 +570,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;
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 2/2] target/ppc: make gdb able to translate priviledged addresses
2021-06-14 19:16 ` [RFC PATCH v2 2/2] target/ppc: make gdb able to translate priviledged addresses Bruno Larsen (billionai)
@ 2021-06-14 19:37 ` Philippe Mathieu-Daudé
2021-06-15 1:41 ` David Gibson
2021-06-15 12:12 ` Bruno Piazera Larsen
2021-06-14 21:25 ` Fabiano Rosas
2021-06-14 22:37 ` Richard Henderson
2 siblings, 2 replies; 24+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-14 19:37 UTC (permalink / raw)
To: Bruno Larsen (billionai), qemu-devel
Cc: farosas, richard.henderson, luis.pires, Greg Kurz, lucas.araujo,
fernando.valle, qemu-ppc, Miroslav Rezanina, matheus.ferst,
david
On 6/14/21 9:16 PM, Bruno Larsen (billionai) wrote:
> This patch changes ppc_cpu_get_phys_page_debug so that it is now
> able to translate both, priviledged and real mode addresses
> independently of whether the CPU executing it has those permissions
>
> This was mentioned by Fabiano as something that would be very useful to
> help with debugging, but could possibly constitute a security issue if
> that debug function can be called in some way by prodution code. the
> solution was implemented such that it would be trivial to wrap it around
> ifdefs for building only with --enable-debug, for instance, but we are
> not sure this is the best approach, hence why it is an RFC.
>
> Suggested-by: Fabiano Rosas <farosas@linux.ibm.com>
> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> ---
> target/ppc/mmu_helper.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> index 9dcdf88597..41c727c690 100644
> --- a/target/ppc/mmu_helper.c
> +++ b/target/ppc/mmu_helper.c
> @@ -2947,6 +2947,29 @@ hwaddr ppc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
> cpu_mmu_index(&cpu->env, true), false)) {
> return raddr & TARGET_PAGE_MASK;
> }
> +
> + /*
> + * This is a fallback, in case we're asking for priviledged memory to
> + * be printed, but the PCU is not executing in a priviledged manner.
> + *
> + * The code could be considered a security vulnerability if
> + * this function can be called in a scenario that does not involve
> + * debugging.
> + * Given the name and how useful using real addresses may be for
> + * actually debugging, however, we decided to include it anyway and
> + * discuss how to best avoid the possible security concerns.
> + * The current plan is that, if there is a chance this code is called in
> + * a production environment, we can surround it with ifdefs so that it
> + * is only compiled with --enable-debug
Nothing forbid us to use --enable-debug in a production environment.
> + */
> + /* attempt to translate first with virtual addresses */
> + if (ppc_xlate(cpu, addr, MMU_DATA_LOAD, &raddr, &s, &p, 1, false) ||
> + ppc_xlate(cpu, addr, MMU_INST_FETCH, &raddr, &s, &p, 1, false) ||
> + /* if didn't work, attempt to translate with real addresses */
> + ppc_xlate(cpu, addr, MMU_DATA_LOAD, &raddr, &s, &p, 3, false) ||
> + ppc_xlate(cpu, addr, MMU_INST_FETCH, &raddr, &s, &p, 3, false)) {
> + return raddr & TARGET_PAGE_MASK;
> + }
> return -1;
> }
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] target/ppc: fix address translation bug for radix mmus
2021-06-14 19:16 [PATCH v2 1/2] target/ppc: fix address translation bug for radix mmus Bruno Larsen (billionai)
2021-06-14 19:16 ` [RFC PATCH v2 2/2] target/ppc: make gdb able to translate priviledged addresses Bruno Larsen (billionai)
2021-06-14 19:29 ` [PATCH v2 1/2] target/ppc: fix address translation bug for radix mmus Greg Kurz
@ 2021-06-14 21:04 ` Fabiano Rosas
2021-06-15 1:18 ` David Gibson
2021-06-15 1:41 ` David Gibson
2021-06-15 13:57 ` Cédric Le Goater
4 siblings, 1 reply; 24+ messages in thread
From: Fabiano Rosas @ 2021-06-14 21:04 UTC (permalink / raw)
To: Bruno Larsen (billionai), qemu-devel
Cc: richard.henderson, luis.pires, Greg Kurz, lucas.araujo,
fernando.valle, qemu-ppc, matheus.ferst, david
"Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> writes:
> +/*
> + * These correspond to the mmu_idx values computed in
> + * hreg_compute_hflags_value. See the tables therein
> + */
> +static inline bool mmuidx_pr(int idx) { return !(idx & 1); }
> +/*
> + * If we want to use these macros for hash-style MMUs, we need to
> + * add an if or another macro here.
> + */
Don't these work just fine for hash as well? Except for Booke.
> +static inline bool mmuidx_real(int idx) { return idx & 2; }
> +static inline bool mmuidx_hv(int idx) { return idx & 4; }
> +
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 2/2] target/ppc: make gdb able to translate priviledged addresses
2021-06-14 19:16 ` [RFC PATCH v2 2/2] target/ppc: make gdb able to translate priviledged addresses Bruno Larsen (billionai)
2021-06-14 19:37 ` Philippe Mathieu-Daudé
@ 2021-06-14 21:25 ` Fabiano Rosas
2021-06-15 11:59 ` Bruno Piazera Larsen
2021-06-14 22:37 ` Richard Henderson
2 siblings, 1 reply; 24+ messages in thread
From: Fabiano Rosas @ 2021-06-14 21:25 UTC (permalink / raw)
To: Bruno Larsen (billionai), qemu-devel
Cc: richard.henderson, luis.pires, Greg Kurz, lucas.araujo,
fernando.valle, qemu-ppc, matheus.ferst, david
"Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> writes:
> This patch changes ppc_cpu_get_phys_page_debug so that it is now
> able to translate both, priviledged and real mode addresses
> independently of whether the CPU executing it has those permissions
>
> This was mentioned by Fabiano as something that would be very useful to
> help with debugging, but could possibly constitute a security issue if
> that debug function can be called in some way by prodution code.
Thinking a bit more about this, I think we just need to make sure that
this is not called during the regular operation of the virtual
machine. So not as much a security issue, more of a correctness one.
> the
> solution was implemented such that it would be trivial to wrap it around
> ifdefs for building only with --enable-debug, for instance, but we are
> not sure this is the best approach, hence why it is an RFC.
>
> Suggested-by: Fabiano Rosas <farosas@linux.ibm.com>
> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> ---
> target/ppc/mmu_helper.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> index 9dcdf88597..41c727c690 100644
> --- a/target/ppc/mmu_helper.c
> +++ b/target/ppc/mmu_helper.c
> @@ -2947,6 +2947,29 @@ hwaddr ppc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
> cpu_mmu_index(&cpu->env, true), false)) {
> return raddr & TARGET_PAGE_MASK;
> }
> +
> + /*
> + * This is a fallback, in case we're asking for priviledged memory to
> + * be printed, but the PCU is not executing in a priviledged manner.
> + *
> + * The code could be considered a security vulnerability if
> + * this function can be called in a scenario that does not involve
> + * debugging.
> + * Given the name and how useful using real addresses may be for
> + * actually debugging, however, we decided to include it anyway and
> + * discuss how to best avoid the possible security concerns.
> + * The current plan is that, if there is a chance this code is called in
> + * a production environment, we can surround it with ifdefs so that it
> + * is only compiled with --enable-debug
> + */
> + /* attempt to translate first with virtual addresses */
> + if (ppc_xlate(cpu, addr, MMU_DATA_LOAD, &raddr, &s, &p, 1, false) ||
> + ppc_xlate(cpu, addr, MMU_INST_FETCH, &raddr, &s, &p, 1, false) ||
> + /* if didn't work, attempt to translate with real addresses */
> + ppc_xlate(cpu, addr, MMU_DATA_LOAD, &raddr, &s, &p, 3, false) ||
> + ppc_xlate(cpu, addr, MMU_INST_FETCH, &raddr, &s, &p, 3, false)) {
> + return raddr & TARGET_PAGE_MASK;
> + }
If this is only used during debug we could just give it the highest
mmu_idx, no need for a fallback.
Now, it might be possible that people use GDB to debug some aspect of
the MMU emulation, in which case it would be more useful to have the GDB
access fail just as the CPU would. But from my perspective it would be
better to have GDB access all of the guest memory without restriction.
> return -1;
> }
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 2/2] target/ppc: make gdb able to translate priviledged addresses
2021-06-14 19:16 ` [RFC PATCH v2 2/2] target/ppc: make gdb able to translate priviledged addresses Bruno Larsen (billionai)
2021-06-14 19:37 ` Philippe Mathieu-Daudé
2021-06-14 21:25 ` Fabiano Rosas
@ 2021-06-14 22:37 ` Richard Henderson
2021-06-15 11:32 ` Bruno Piazera Larsen
2 siblings, 1 reply; 24+ messages in thread
From: Richard Henderson @ 2021-06-14 22:37 UTC (permalink / raw)
To: Bruno Larsen (billionai), qemu-devel
Cc: farosas, luis.pires, Greg Kurz, lucas.araujo, fernando.valle,
qemu-ppc, matheus.ferst, david
On 6/14/21 12:16 PM, Bruno Larsen (billionai) wrote:
> This patch changes ppc_cpu_get_phys_page_debug so that it is now
> able to translate both, priviledged and real mode addresses
> independently of whether the CPU executing it has those permissions
>
> This was mentioned by Fabiano as something that would be very useful to
> help with debugging, but could possibly constitute a security issue if
> that debug function can be called in some way by prodution code. the
> solution was implemented such that it would be trivial to wrap it around
> ifdefs for building only with --enable-debug, for instance, but we are
> not sure this is the best approach, hence why it is an RFC.
>
> Suggested-by: Fabiano Rosas<farosas@linux.ibm.com>
> Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br>
> ---
> target/ppc/mmu_helper.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
I think the first part is unnecessary. Either the cpu is in supervisor mode or it isn't,
and gdb should use the correct address space. If you really want to force supervisor
lookup from a guest that is paused in usermode, I suppose you could force MSR.PR=1 while
you're performing the access and set it back afterward.
I think the second part is actively wrong -- real-mode address lookup will (for the most
part) always succeed. Moreover, the gdb user will have no idea that you've silently
changed addressing methods.
r~
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] target/ppc: fix address translation bug for radix mmus
2021-06-14 21:04 ` Fabiano Rosas
@ 2021-06-15 1:18 ` David Gibson
0 siblings, 0 replies; 24+ messages in thread
From: David Gibson @ 2021-06-15 1:18 UTC (permalink / raw)
To: Fabiano Rosas
Cc: richard.henderson, lucas.araujo, qemu-devel, luis.pires,
fernando.valle, qemu-ppc, matheus.ferst, Greg Kurz
[-- Attachment #1: Type: text/plain, Size: 994 bytes --]
On Mon, Jun 14, 2021 at 06:04:13PM -0300, Fabiano Rosas wrote:
> "Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> writes:
>
> > +/*
> > + * These correspond to the mmu_idx values computed in
> > + * hreg_compute_hflags_value. See the tables therein
> > + */
> > +static inline bool mmuidx_pr(int idx) { return !(idx & 1); }
> > +/*
> > + * If we want to use these macros for hash-style MMUs, we need to
> > + * add an if or another macro here.
> > + */
>
> Don't these work just fine for hash as well? Except for Booke.
Right, I think we want to restrict these to BookS mmus, but I don't
think we need to restrict to radix.
>
> > +static inline bool mmuidx_real(int idx) { return idx & 2; }
> > +static inline bool mmuidx_hv(int idx) { return idx & 4; }
> > +
>
--
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] 24+ messages in thread
* Re: [PATCH v2 1/2] target/ppc: fix address translation bug for radix mmus
2021-06-14 19:16 [PATCH v2 1/2] target/ppc: fix address translation bug for radix mmus Bruno Larsen (billionai)
` (2 preceding siblings ...)
2021-06-14 21:04 ` Fabiano Rosas
@ 2021-06-15 1:41 ` David Gibson
2021-06-15 3:20 ` Richard Henderson
2021-06-15 13:57 ` Cédric Le Goater
4 siblings, 1 reply; 24+ messages in thread
From: David Gibson @ 2021-06-15 1:41 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: 12764 bytes --]
On Mon, Jun 14, 2021 at 04:16:29PM -0300, Bruno Larsen (billionai) wrote:
> Based-on: <20210518201146.794854-1-richard.henderson@linaro.org>
>
> This commit attempts to fix the first bug mentioned by Richard Henderson in
> https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06247.html
>
> To sumarize the bug here, when radix-style mmus are translating an
> address, they might need to call a second level of translation, with
> hypvervisor priviledges. However, the way it was being done up until
s/hypvervisor/hypervisor/
And... it's a super-common error, even amongst native speakers, but
s/priviledge/privilege/g
> this point meant that the second level translation had the same
> priviledges as the first level. This would only happen when a TCG guest
> was emulating KVM, which is why it hasn't been discovered yet.
>
> This patch attempts to correct that 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.
Right, the mmuidx values are basically local to a cpu family. Radix
is only present on BookS 64-bit cpus.
> 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>
> ---
> target/ppc/internal.h | 12 ++++++++++++
> target/ppc/mmu-radix64.c | 38 ++++++++++++++++++++++----------------
> target/ppc/mmu-radix64.h | 2 +-
> target/ppc/mmu_helper.c | 8 +++++---
> 4 files changed, 40 insertions(+), 20 deletions(-)
>
> diff --git a/target/ppc/internal.h b/target/ppc/internal.h
> index f1fd3c8d04..003df7e8a9 100644
> --- a/target/ppc/internal.h
> +++ b/target/ppc/internal.h
> @@ -245,4 +245,16 @@ static inline int prot_for_access_type(MMUAccessType access_type)
> g_assert_not_reached();
> }
>
> +/*
> + * These correspond to the mmu_idx values computed in
> + * hreg_compute_hflags_value. See the tables therein
> + */
> +static inline bool mmuidx_pr(int idx) { return !(idx & 1); }
> +/*
> + * If we want to use these macros for hash-style MMUs, we need to
> + * add an if or another macro here
I think move these to mmu-book3s-v3.h, since they're correct for both
the radix and hash sides of the modern book3s mmu.
.
> + */
> +static inline bool mmuidx_real(int idx) { return idx & 2; }
> +static inline bool mmuidx_hv(int idx) { return idx & 4; }
> +
> #endif /* PPC_INTERNAL_H */
> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
> index cbd404bfa4..0ae8f6b572 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,9 @@ 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 +335,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 +370,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 +411,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 +436,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 +470,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 +480,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 +552,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 +570,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] 24+ messages in thread
* Re: [RFC PATCH v2 2/2] target/ppc: make gdb able to translate priviledged addresses
2021-06-14 19:37 ` Philippe Mathieu-Daudé
@ 2021-06-15 1:41 ` David Gibson
2021-06-15 12:12 ` Bruno Piazera Larsen
1 sibling, 0 replies; 24+ messages in thread
From: David Gibson @ 2021-06-15 1:41 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: farosas, richard.henderson, Greg Kurz, qemu-devel, lucas.araujo,
luis.pires, fernando.valle, qemu-ppc, Miroslav Rezanina,
matheus.ferst
[-- Attachment #1: Type: text/plain, Size: 3115 bytes --]
On Mon, Jun 14, 2021 at 09:37:54PM +0200, Philippe Mathieu-Daudé wrote:
> On 6/14/21 9:16 PM, Bruno Larsen (billionai) wrote:
> > This patch changes ppc_cpu_get_phys_page_debug so that it is now
> > able to translate both, priviledged and real mode addresses
> > independently of whether the CPU executing it has those permissions
> >
> > This was mentioned by Fabiano as something that would be very useful to
> > help with debugging, but could possibly constitute a security issue if
> > that debug function can be called in some way by prodution code. the
> > solution was implemented such that it would be trivial to wrap it around
> > ifdefs for building only with --enable-debug, for instance, but we are
> > not sure this is the best approach, hence why it is an RFC.
> >
> > Suggested-by: Fabiano Rosas <farosas@linux.ibm.com>
> > Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> > ---
> > target/ppc/mmu_helper.c | 23 +++++++++++++++++++++++
> > 1 file changed, 23 insertions(+)
> >
> > diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> > index 9dcdf88597..41c727c690 100644
> > --- a/target/ppc/mmu_helper.c
> > +++ b/target/ppc/mmu_helper.c
> > @@ -2947,6 +2947,29 @@ hwaddr ppc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
> > cpu_mmu_index(&cpu->env, true), false)) {
> > return raddr & TARGET_PAGE_MASK;
> > }
> > +
> > + /*
> > + * This is a fallback, in case we're asking for priviledged memory to
> > + * be printed, but the PCU is not executing in a priviledged
> > manner.
s/PCU/CPU/
> > + *
> > + * The code could be considered a security vulnerability if
> > + * this function can be called in a scenario that does not involve
> > + * debugging.
> > + * Given the name and how useful using real addresses may be for
> > + * actually debugging, however, we decided to include it anyway and
> > + * discuss how to best avoid the possible security concerns.
> > + * The current plan is that, if there is a chance this code is called in
> > + * a production environment, we can surround it with ifdefs so that it
> > + * is only compiled with --enable-debug
>
> Nothing forbid us to use --enable-debug in a production environment.
>
> > + */
> > + /* attempt to translate first with virtual addresses */
> > + if (ppc_xlate(cpu, addr, MMU_DATA_LOAD, &raddr, &s, &p, 1, false) ||
> > + ppc_xlate(cpu, addr, MMU_INST_FETCH, &raddr, &s, &p, 1, false) ||
> > + /* if didn't work, attempt to translate with real addresses */
> > + ppc_xlate(cpu, addr, MMU_DATA_LOAD, &raddr, &s, &p, 3, false) ||
> > + ppc_xlate(cpu, addr, MMU_INST_FETCH, &raddr, &s, &p, 3, 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] 24+ messages in thread
* Re: [PATCH v2 1/2] target/ppc: fix address translation bug for radix mmus
2021-06-15 1:41 ` David Gibson
@ 2021-06-15 3:20 ` Richard Henderson
2021-06-15 12:25 ` Bruno Piazera Larsen
0 siblings, 1 reply; 24+ messages in thread
From: Richard Henderson @ 2021-06-15 3:20 UTC (permalink / raw)
To: David Gibson, Bruno Larsen (billionai)
Cc: farosas, qemu-devel, Greg Kurz, lucas.araujo, fernando.valle,
qemu-ppc, matheus.ferst, luis.pires
On 6/14/21 6:41 PM, David Gibson wrote:
> I think move these to mmu-book3s-v3.h, since they're correct for both
> the radix and hash sides of the modern book3s mmu.
They're also correct for all non-booke mmus, e.g. hash32 and 6xx, which is why I
recommended internal.h (or some new mmu-internal.h).
While neither hash32 nor 6xx have HV, and thus there is no second tlb bug, it would still
be More Correct to use mmu_idx instead of direct references to msr_pr et al.
r~
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 2/2] target/ppc: make gdb able to translate priviledged addresses
2021-06-14 22:37 ` Richard Henderson
@ 2021-06-15 11:32 ` Bruno Piazera Larsen
2021-06-15 20:00 ` Richard Henderson
2021-06-16 6:18 ` David Gibson
0 siblings, 2 replies; 24+ messages in thread
From: Bruno Piazera Larsen @ 2021-06-15 11:32 UTC (permalink / raw)
To: Richard Henderson, qemu-devel
Cc: farosas, luis.pires, Greg Kurz, lucas.araujo, fernando.valle,
qemu-ppc, matheus.ferst, david
[-- Attachment #1: Type: text/plain, Size: 2755 bytes --]
On 14/06/2021 19:37, Richard Henderson wrote:
> On 6/14/21 12:16 PM, Bruno Larsen (billionai) wrote:
>> This patch changes ppc_cpu_get_phys_page_debug so that it is now
>> able to translate both, priviledged and real mode addresses
>> independently of whether the CPU executing it has those permissions
>>
>> This was mentioned by Fabiano as something that would be very useful to
>> help with debugging, but could possibly constitute a security issue if
>> that debug function can be called in some way by prodution code. the
>> solution was implemented such that it would be trivial to wrap it around
>> ifdefs for building only with --enable-debug, for instance, but we are
>> not sure this is the best approach, hence why it is an RFC.
>>
>> Suggested-by: Fabiano Rosas<farosas@linux.ibm.com>
>> Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br>
>> ---
>> target/ppc/mmu_helper.c | 23 +++++++++++++++++++++++
>> 1 file changed, 23 insertions(+)
>
> I think the first part is unnecessary. Either the cpu is in
> supervisor mode or it isn't, and gdb should use the correct address
> space. If you really want to force supervisor lookup from a guest
> that is paused in usermode, I suppose you could force MSR.PR=1 while
> you're performing the access and set it back afterward.
I don't see why GDB should not be able to see supervisor level addresses
just because the CPU can't. when debugging, we wanna see exactly what
QEMU sees, not what the guest sees, right? Now, if this is changing more
than just privilege level, I agree there is a problem, but I wouldn't
think it is the case...
>
> I think the second part is actively wrong -- real-mode address lookup
> will (for the most part) always succeed. Moreover, the gdb user will
> have no idea that you've silently changed addressing methods.
I disagree. Real-mode address will mostly fail, since during the boot
process Linux kernels set the MMU to use only virtual addresses, so real
mode addresses only work when debugging the firmware or the early setup
of the kernel. After that, GDB can basically only see virtual addresses.
Maybe there is a better way to handle this by having GDB warn the user
that the CPU can not decode the address in it's current state, but I do
think it is a good tool to have, as it would've made debugging the first
RFC on this topic a bit easier, and farosas was actively complaining
that isn't a feature yet.
>
> r~
--
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: 4161 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 2/2] target/ppc: make gdb able to translate priviledged addresses
2021-06-14 21:25 ` Fabiano Rosas
@ 2021-06-15 11:59 ` Bruno Piazera Larsen
0 siblings, 0 replies; 24+ messages in thread
From: Bruno Piazera Larsen @ 2021-06-15 11:59 UTC (permalink / raw)
To: Fabiano Rosas, qemu-devel
Cc: richard.henderson, luis.pires, Greg Kurz, lucas.araujo,
fernando.valle, qemu-ppc, matheus.ferst, david
[-- Attachment #1: Type: text/plain, Size: 4087 bytes --]
On 14/06/2021 18:25, Fabiano Rosas wrote:
> "Bruno Larsen (billionai)" <bruno.larsen@eldorado.org.br> writes:
>
>> This patch changes ppc_cpu_get_phys_page_debug so that it is now
>> able to translate both, priviledged and real mode addresses
>> independently of whether the CPU executing it has those permissions
>>
>> This was mentioned by Fabiano as something that would be very useful to
>> help with debugging, but could possibly constitute a security issue if
>> that debug function can be called in some way by prodution code.
> Thinking a bit more about this, I think we just need to make sure that
> this is not called during the regular operation of the virtual
> machine. So not as much a security issue, more of a correctness one.
yeah, but it's an issue of correctness that can lead to a security
issue, so I think it's worth documenting at the very least
>
>> the
>> solution was implemented such that it would be trivial to wrap it around
>> ifdefs for building only with --enable-debug, for instance, but we are
>> not sure this is the best approach, hence why it is an RFC.
>>
>> Suggested-by: Fabiano Rosas <farosas@linux.ibm.com>
>> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
>> ---
>> target/ppc/mmu_helper.c | 23 +++++++++++++++++++++++
>> 1 file changed, 23 insertions(+)
>>
>> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
>> index 9dcdf88597..41c727c690 100644
>> --- a/target/ppc/mmu_helper.c
>> +++ b/target/ppc/mmu_helper.c
>> @@ -2947,6 +2947,29 @@ hwaddr ppc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
>> cpu_mmu_index(&cpu->env, true), false)) {
>> return raddr & TARGET_PAGE_MASK;
>> }
>> +
>> + /*
>> + * This is a fallback, in case we're asking for priviledged memory to
>> + * be printed, but the PCU is not executing in a priviledged manner.
>> + *
>> + * The code could be considered a security vulnerability if
>> + * this function can be called in a scenario that does not involve
>> + * debugging.
>> + * Given the name and how useful using real addresses may be for
>> + * actually debugging, however, we decided to include it anyway and
>> + * discuss how to best avoid the possible security concerns.
>> + * The current plan is that, if there is a chance this code is called in
>> + * a production environment, we can surround it with ifdefs so that it
>> + * is only compiled with --enable-debug
>> + */
>> + /* attempt to translate first with virtual addresses */
>> + if (ppc_xlate(cpu, addr, MMU_DATA_LOAD, &raddr, &s, &p, 1, false) ||
>> + ppc_xlate(cpu, addr, MMU_INST_FETCH, &raddr, &s, &p, 1, false) ||
>> + /* if didn't work, attempt to translate with real addresses */
>> + ppc_xlate(cpu, addr, MMU_DATA_LOAD, &raddr, &s, &p, 3, false) ||
>> + ppc_xlate(cpu, addr, MMU_INST_FETCH, &raddr, &s, &p, 3, false)) {
>> + return raddr & TARGET_PAGE_MASK;
>> + }
> If this is only used during debug we could just give it the highest
> mmu_idx, no need for a fallback.
we actually don't want to set the HV bit, because gdb is using the
virtual hypervisor, so it'd trigger an assert that both HV and vhyp are
set.
>
> Now, it might be possible that people use GDB to debug some aspect of
> the MMU emulation, in which case it would be more useful to have the GDB
> access fail just as the CPU would. But from my perspective it would be
> better to have GDB access all of the guest memory without restriction.
Yeah, could also be a thing. I really don't know, though, because before
the mmu_idx was 0, so it wouldn't work even before this patch. Some more
discussion is appreciated for the people who implement MMUs :)
>
>> return -1;
>> }
--
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: 5518 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 2/2] target/ppc: make gdb able to translate priviledged addresses
2021-06-14 19:37 ` Philippe Mathieu-Daudé
2021-06-15 1:41 ` David Gibson
@ 2021-06-15 12:12 ` Bruno Piazera Larsen
1 sibling, 0 replies; 24+ messages in thread
From: Bruno Piazera Larsen @ 2021-06-15 12:12 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: farosas, richard.henderson, luis.pires, Greg Kurz, lucas.araujo,
fernando.valle, qemu-ppc, Miroslav Rezanina, matheus.ferst,
david
[-- Attachment #1: Type: text/plain, Size: 3546 bytes --]
On 14/06/2021 16:37, Philippe Mathieu-Daudé wrote:
> On 6/14/21 9:16 PM, Bruno Larsen (billionai) wrote:
>> This patch changes ppc_cpu_get_phys_page_debug so that it is now
>> able to translate both, priviledged and real mode addresses
>> independently of whether the CPU executing it has those permissions
>>
>> This was mentioned by Fabiano as something that would be very useful to
>> help with debugging, but could possibly constitute a security issue if
>> that debug function can be called in some way by prodution code. the
>> solution was implemented such that it would be trivial to wrap it around
>> ifdefs for building only with --enable-debug, for instance, but we are
>> not sure this is the best approach, hence why it is an RFC.
>>
>> Suggested-by: Fabiano Rosas <farosas@linux.ibm.com>
>> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
>> ---
>> target/ppc/mmu_helper.c | 23 +++++++++++++++++++++++
>> 1 file changed, 23 insertions(+)
>>
>> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
>> index 9dcdf88597..41c727c690 100644
>> --- a/target/ppc/mmu_helper.c
>> +++ b/target/ppc/mmu_helper.c
>> @@ -2947,6 +2947,29 @@ hwaddr ppc_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
>> cpu_mmu_index(&cpu->env, true), false)) {
>> return raddr & TARGET_PAGE_MASK;
>> }
>> +
>> + /*
>> + * This is a fallback, in case we're asking for priviledged memory to
>> + * be printed, but the PCU is not executing in a priviledged manner.
>> + *
>> + * The code could be considered a security vulnerability if
>> + * this function can be called in a scenario that does not involve
>> + * debugging.
>> + * Given the name and how useful using real addresses may be for
>> + * actually debugging, however, we decided to include it anyway and
>> + * discuss how to best avoid the possible security concerns.
>> + * The current plan is that, if there is a chance this code is called in
>> + * a production environment, we can surround it with ifdefs so that it
>> + * is only compiled with --enable-debug
> Nothing forbid us to use --enable-debug in a production environment.
True, but in general, running a debug build is considered a path to
vulnerabilities one would consider to be avoidable. Also, I am not sure
of a much better way to help devs that use QEMU without opening up to
these kinds of info-leak vulnerabilities, since it's literally what we
want the code to do.
Having it require source code manipulation, like DO_CPU_STATISTICS did,
could work, but it could also bit rot quickly.
Definitely open to more discussion on the topic! :)
>
>> + */
>> + /* attempt to translate first with virtual addresses */
>> + if (ppc_xlate(cpu, addr, MMU_DATA_LOAD, &raddr, &s, &p, 1, false) ||
>> + ppc_xlate(cpu, addr, MMU_INST_FETCH, &raddr, &s, &p, 1, false) ||
>> + /* if didn't work, attempt to translate with real addresses */
>> + ppc_xlate(cpu, addr, MMU_DATA_LOAD, &raddr, &s, &p, 3, false) ||
>> + ppc_xlate(cpu, addr, MMU_INST_FETCH, &raddr, &s, &p, 3, false)) {
>> + return raddr & TARGET_PAGE_MASK;
>> + }
>> return -1;
>> }
>>
>>
--
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: 4641 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] target/ppc: fix address translation bug for radix mmus
2021-06-15 3:20 ` Richard Henderson
@ 2021-06-15 12:25 ` Bruno Piazera Larsen
2021-06-16 6:16 ` David Gibson
0 siblings, 1 reply; 24+ messages in thread
From: Bruno Piazera Larsen @ 2021-06-15 12:25 UTC (permalink / raw)
To: Richard Henderson, David Gibson
Cc: farosas, qemu-devel, Greg Kurz, lucas.araujo, fernando.valle,
qemu-ppc, matheus.ferst, luis.pires
[-- Attachment #1: Type: text/plain, Size: 1116 bytes --]
On 15/06/2021 00:20, Richard Henderson wrote:
> On 6/14/21 6:41 PM, David Gibson wrote:
>> I think move these to mmu-book3s-v3.h, since they're correct for both
>> the radix and hash sides of the modern book3s mmu.
>
> They're also correct for all non-booke mmus, e.g. hash32 and 6xx,
> which is why I recommended internal.h (or some new mmu-internal.h).
>
> While neither hash32 nor 6xx have HV, and thus there is no second tlb
> bug, it would still be More Correct to use mmu_idx instead of direct
> references to msr_pr et al.
yeah, I agree that I should change the documentation. Before I send a
new version with everything corrected, I wanna make sure if I should
leave them in internal and use it for hash32 and 6xx MMUs, or do I put
them in mmu-book3s-v3.h, since only radix64 has that the bug?
>
>
> r~
--
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: 1890 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] target/ppc: fix address translation bug for radix mmus
2021-06-14 19:16 [PATCH v2 1/2] target/ppc: fix address translation bug for radix mmus Bruno Larsen (billionai)
` (3 preceding siblings ...)
2021-06-15 1:41 ` David Gibson
@ 2021-06-15 13:57 ` Cédric Le Goater
2021-06-15 14:14 ` Philippe Mathieu-Daudé
2021-06-15 14:57 ` Bruno Piazera Larsen
4 siblings, 2 replies; 24+ messages in thread
From: Cédric Le Goater @ 2021-06-15 13:57 UTC (permalink / raw)
To: Bruno Larsen (billionai), qemu-devel
Cc: farosas, richard.henderson, luis.pires, Greg Kurz, lucas.araujo,
fernando.valle, qemu-ppc, matheus.ferst, david
On 6/14/21 9:16 PM, Bruno Larsen (billionai) wrote:
> Based-on: <20210518201146.794854-1-richard.henderson@linaro.org>
>
> This commit attempts to fix the first bug mentioned by Richard Henderson in
> https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06247.html
>
> To sumarize the bug here, when radix-style mmus are translating an
> address, they might need to call a second level of translation, with
> hypvervisor priviledges. However, the way it was being done up until
> this point meant that the second level translation had the same
> priviledges as the first level. This would only happen when a TCG guest
> was emulating KVM, which is why it hasn't been discovered yet.
What do you mean ? The QEMU PowerNV machine emulates baremetal and
can run KVM pseries guests.
It has some issues under load but not related to memory translation.
This patch is certainly improving the model and it is worth testing
but this version does not apply on ppc-6.1.
Thanks,
C.
> This patch attempts to correct that 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>
> ---
> target/ppc/internal.h | 12 ++++++++++++
> target/ppc/mmu-radix64.c | 38 ++++++++++++++++++++++----------------
> target/ppc/mmu-radix64.h | 2 +-
> target/ppc/mmu_helper.c | 8 +++++---
> 4 files changed, 40 insertions(+), 20 deletions(-)
>
> diff --git a/target/ppc/internal.h b/target/ppc/internal.h
> index f1fd3c8d04..003df7e8a9 100644
> --- a/target/ppc/internal.h
> +++ b/target/ppc/internal.h
> @@ -245,4 +245,16 @@ static inline int prot_for_access_type(MMUAccessType access_type)
> g_assert_not_reached();
> }
>
> +/*
> + * These correspond to the mmu_idx values computed in
> + * hreg_compute_hflags_value. See the tables therein
> + */
> +static inline bool mmuidx_pr(int idx) { return !(idx & 1); }
> +/*
> + * If we want to use these macros for hash-style MMUs, we need to
> + * add an if or another macro here.
> + */
> +static inline bool mmuidx_real(int idx) { return idx & 2; }
> +static inline bool mmuidx_hv(int idx) { return idx & 4; }
> +
> #endif /* PPC_INTERNAL_H */
> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
> index cbd404bfa4..0ae8f6b572 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,9 @@ 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 +335,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 +370,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 +411,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 +436,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 +470,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 +480,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 +552,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 +570,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;
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] target/ppc: fix address translation bug for radix mmus
2021-06-15 13:57 ` Cédric Le Goater
@ 2021-06-15 14:14 ` Philippe Mathieu-Daudé
2021-06-15 14:57 ` Bruno Piazera Larsen
1 sibling, 0 replies; 24+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-15 14:14 UTC (permalink / raw)
To: Cédric Le Goater, Bruno Larsen (billionai), qemu-devel
Cc: farosas, richard.henderson, luis.pires, Greg Kurz, lucas.araujo,
fernando.valle, qemu-ppc, matheus.ferst, david
On 6/15/21 3:57 PM, Cédric Le Goater wrote:
> On 6/14/21 9:16 PM, Bruno Larsen (billionai) wrote:
>> Based-on: <20210518201146.794854-1-richard.henderson@linaro.org>
>>
>> This commit attempts to fix the first bug mentioned by Richard Henderson in
>> https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06247.html
>>
>> To sumarize the bug here, when radix-style mmus are translating an
>> address, they might need to call a second level of translation, with
>> hypvervisor priviledges. However, the way it was being done up until
>> this point meant that the second level translation had the same
>> priviledges as the first level. This would only happen when a TCG guest
>> was emulating KVM, which is why it hasn't been discovered yet.
>
> What do you mean ? The QEMU PowerNV machine emulates baremetal and
> can run KVM pseries guests.
>
> It has some issues under load but not related to memory translation.
> This patch is certainly improving the model and it is worth testing
> but this version does not apply on ppc-6.1.
Unfortunately this series misses a cover letter.
The base series is mentioned in the first patch:
Based-on: <20210518201146.794854-1-richard.henderson@linaro.org>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] target/ppc: fix address translation bug for radix mmus
2021-06-15 13:57 ` Cédric Le Goater
2021-06-15 14:14 ` Philippe Mathieu-Daudé
@ 2021-06-15 14:57 ` Bruno Piazera Larsen
2021-06-15 15:57 ` Cédric Le Goater
1 sibling, 1 reply; 24+ messages in thread
From: Bruno Piazera Larsen @ 2021-06-15 14:57 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: farosas, richard.henderson, luis.pires, Greg Kurz, lucas.araujo,
fernando.valle, qemu-ppc, matheus.ferst, david
[-- Attachment #1: Type: text/plain, Size: 13607 bytes --]
On 15/06/2021 10:57, Cédric Le Goater wrote:
> On 6/14/21 9:16 PM, Bruno Larsen (billionai) wrote:
>> Based-on: <20210518201146.794854-1-richard.henderson@linaro.org>
>>
>> This commit attempts to fix the first bug mentioned by Richard Henderson in
>> https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06247.html
>>
>> To sumarize the bug here, when radix-style mmus are translating an
>> address, they might need to call a second level of translation, with
>> hypvervisor priviledges. However, the way it was being done up until
>> this point meant that the second level translation had the same
>> priviledges as the first level. This would only happen when a TCG guest
>> was emulating KVM, which is why it hasn't been discovered yet.
> What do you mean ? The QEMU PowerNV machine emulates baremetal and
> can run KVM pseries guests.
Is the PowerNV using radix? The bug only happens when guest1 (the
emulated PowerNV, not it's guest), and if so, we might need to
reevaluate if the bug actually happens, or if it's just a theoretical
hiccup. And also change the commit message.
Either way, if my explanation is a bit confusing, there are more details
on the link. I sort of just implemented the solutions suggested in that
e-mail chain.
>
> It has some issues under load but not related to memory translation.
> This patch is certainly improving the model and it is worth testing
> but this version does not apply on ppc-6.1.
Ah, yes, it is based on a patch series made by Richard Henderson a few
weeks ago. Since we need that patch to be accepted to finally support
disable-tcg and I don't want to delay that by adding a change that might
change a lot of his remaining patches.
>
> Thanks,
>
> C.
>
>
>> This patch attempts to correct that 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>
>> ---
>> target/ppc/internal.h | 12 ++++++++++++
>> target/ppc/mmu-radix64.c | 38 ++++++++++++++++++++++----------------
>> target/ppc/mmu-radix64.h | 2 +-
>> target/ppc/mmu_helper.c | 8 +++++---
>> 4 files changed, 40 insertions(+), 20 deletions(-)
>>
>> diff --git a/target/ppc/internal.h b/target/ppc/internal.h
>> index f1fd3c8d04..003df7e8a9 100644
>> --- a/target/ppc/internal.h
>> +++ b/target/ppc/internal.h
>> @@ -245,4 +245,16 @@ static inline int prot_for_access_type(MMUAccessType access_type)
>> g_assert_not_reached();
>> }
>>
>> +/*
>> + * These correspond to the mmu_idx values computed in
>> + * hreg_compute_hflags_value. See the tables therein
>> + */
>> +static inline bool mmuidx_pr(int idx) { return !(idx & 1); }
>> +/*
>> + * If we want to use these macros for hash-style MMUs, we need to
>> + * add an if or another macro here.
>> + */
>> +static inline bool mmuidx_real(int idx) { return idx & 2; }
>> +static inline bool mmuidx_hv(int idx) { return idx & 4; }
>> +
>> #endif /* PPC_INTERNAL_H */
>> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
>> index cbd404bfa4..0ae8f6b572 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,9 @@ 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 +335,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 +370,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 +411,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 +436,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 +470,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 +480,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 +552,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 +570,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;
>>
--
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: 14675 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] target/ppc: fix address translation bug for radix mmus
2021-06-15 14:57 ` Bruno Piazera Larsen
@ 2021-06-15 15:57 ` Cédric Le Goater
0 siblings, 0 replies; 24+ messages in thread
From: Cédric Le Goater @ 2021-06-15 15:57 UTC (permalink / raw)
To: Bruno Piazera Larsen, qemu-devel
Cc: farosas, richard.henderson, luis.pires, Greg Kurz, lucas.araujo,
fernando.valle, qemu-ppc, matheus.ferst, david
On 6/15/21 4:57 PM, Bruno Piazera Larsen wrote:
>
> On 15/06/2021 10:57, Cédric Le Goater wrote:
>> On 6/14/21 9:16 PM, Bruno Larsen (billionai) wrote:
>>> Based-on: <20210518201146.794854-1-richard.henderson@linaro.org>
>>>
>>> This commit attempts to fix the first bug mentioned by Richard Henderson in
>>> https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06247.html
>>>
>>> To sumarize the bug here, when radix-style mmus are translating an
>>> address, they might need to call a second level of translation, with
>>> hypvervisor priviledges. However, the way it was being done up until
>>> this point meant that the second level translation had the same
>>> priviledges as the first level. This would only happen when a TCG guest
>>> was emulating KVM, which is why it hasn't been discovered yet.
>> What do you mean ? The QEMU PowerNV machine emulates baremetal and
>> can run KVM pseries guests.
>
> Is the PowerNV using radix?
Yes. Radix MMU is the default for Linux under POWER9 but it can also
use Hash.
> The bug only happens when guest1 (the emulated PowerNV, not it's guest),
For PowerNV, "machine" would be more appropriate. We sometime refer to
it as L0 and the sublevel guests as L1 and L2.
> and if so, we might need to reevaluate if the bug actually happens,
> or if it's just a theoretical hiccup. And also change the commit message.
I think this would be a fix for the model.
> Either way, if my explanation is a bit confusing, there are more details
> on the link. I sort of just implemented the solutions suggested in that
> e-mail chain.
It seems to makes sense. I just want to give it a try using an emulated
QEMU PowerNV machine running a KVM pseries guest to see if it doesn't
break anything. Which would mean that the fix is incomplete.
>> It has some issues under load but not related to memory translation.
>> This patch is certainly improving the model and it is worth testing
>> but this version does not apply on ppc-6.1.
>
> Ah, yes, it is based on a patch series made by Richard Henderson a
> few weeks ago. Since we need that patch to be accepted to finally
> support disable-tcg and I don't want to delay that by adding a
> change that might change a lot of his remaining patches.
OK.
Thanks,
C.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 2/2] target/ppc: make gdb able to translate priviledged addresses
2021-06-15 11:32 ` Bruno Piazera Larsen
@ 2021-06-15 20:00 ` Richard Henderson
2021-06-15 21:37 ` Fabiano Rosas
2021-06-16 6:18 ` David Gibson
1 sibling, 1 reply; 24+ messages in thread
From: Richard Henderson @ 2021-06-15 20:00 UTC (permalink / raw)
To: Bruno Piazera Larsen, qemu-devel
Cc: farosas, luis.pires, Greg Kurz, lucas.araujo, fernando.valle,
qemu-ppc, matheus.ferst, david
On 6/15/21 4:32 AM, Bruno Piazera Larsen wrote:
> On 14/06/2021 19:37, Richard Henderson wrote:
>> On 6/14/21 12:16 PM, Bruno Larsen (billionai) wrote:
>>> This patch changes ppc_cpu_get_phys_page_debug so that it is now
>>> able to translate both, priviledged and real mode addresses
>>> independently of whether the CPU executing it has those permissions
>>>
>>> This was mentioned by Fabiano as something that would be very useful to
>>> help with debugging, but could possibly constitute a security issue if
>>> that debug function can be called in some way by prodution code. the
>>> solution was implemented such that it would be trivial to wrap it around
>>> ifdefs for building only with --enable-debug, for instance, but we are
>>> not sure this is the best approach, hence why it is an RFC.
>>>
>>> Suggested-by: Fabiano Rosas<farosas@linux.ibm.com>
>>> Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br>
>>> ---
>>> target/ppc/mmu_helper.c | 23 +++++++++++++++++++++++
>>> 1 file changed, 23 insertions(+)
>>
>> I think the first part is unnecessary. Either the cpu is in supervisor mode or it
>> isn't, and gdb should use the correct address space. If you really want to force
>> supervisor lookup from a guest that is paused in usermode, I suppose you could force
>> MSR.PR=1 while you're performing the access and set it back afterward.
> I don't see why GDB should not be able to see supervisor level addresses just because the
> CPU can't.
Because then when you are debugging, you then don't know whether the address is actually
accessible in the current cpu context.
>> I think the second part is actively wrong -- real-mode address lookup will (for the most
>> part) always succeed. Moreover, the gdb user will have no idea that you've silently
>> changed addressing methods.
>
> I disagree. Real-mode address will mostly fail, since during the boot process Linux
> kernels set the MMU to use only virtual addresses, so real mode addresses only work when
> debugging the firmware or the early setup of the kernel. After that, GDB can basically
> only see virtual addresses.
Exactly. But you changed that so that any unmapped address will re-try with real-mode,
which (outside of hv) simply maps real->physical and returns the input.
One should have to perform some special action to see addresses in a different cpu
context. I don't think that gdb supports such a special action at the moment. If you
want that feature though, that's where you should start.
r~
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 2/2] target/ppc: make gdb able to translate priviledged addresses
2021-06-15 20:00 ` Richard Henderson
@ 2021-06-15 21:37 ` Fabiano Rosas
2021-06-16 12:07 ` Bruno Piazera Larsen
0 siblings, 1 reply; 24+ messages in thread
From: Fabiano Rosas @ 2021-06-15 21:37 UTC (permalink / raw)
To: Richard Henderson, Bruno Piazera Larsen, qemu-devel
Cc: luis.pires, Greg Kurz, lucas.araujo, fernando.valle, qemu-ppc,
matheus.ferst, david
Richard Henderson <richard.henderson@linaro.org> writes:
> On 6/15/21 4:32 AM, Bruno Piazera Larsen wrote:
>> On 14/06/2021 19:37, Richard Henderson wrote:
>>> On 6/14/21 12:16 PM, Bruno Larsen (billionai) wrote:
>>>> This patch changes ppc_cpu_get_phys_page_debug so that it is now
>>>> able to translate both, priviledged and real mode addresses
>>>> independently of whether the CPU executing it has those permissions
>>>>
>>>> This was mentioned by Fabiano as something that would be very useful to
>>>> help with debugging, but could possibly constitute a security issue if
>>>> that debug function can be called in some way by prodution code. the
>>>> solution was implemented such that it would be trivial to wrap it around
>>>> ifdefs for building only with --enable-debug, for instance, but we are
>>>> not sure this is the best approach, hence why it is an RFC.
>>>>
>>>> Suggested-by: Fabiano Rosas<farosas@linux.ibm.com>
>>>> Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br>
>>>> ---
>>>> target/ppc/mmu_helper.c | 23 +++++++++++++++++++++++
>>>> 1 file changed, 23 insertions(+)
>>>
>>> I think the first part is unnecessary. Either the cpu is in supervisor mode or it
>>> isn't, and gdb should use the correct address space. If you really want to force
>>> supervisor lookup from a guest that is paused in usermode, I suppose you could force
>>> MSR.PR=1 while you're performing the access and set it back afterward.
>> I don't see why GDB should not be able to see supervisor level addresses just because the
>> CPU can't.
>
> Because then when you are debugging, you then don't know whether the address is actually
> accessible in the current cpu context.
>
@Bruno, so this is what I referred to somewhere else on the thread,
people expect GDB to have the same access level of the currently
executing code. So implementing my suggestion would break their
workflow.
>>> I think the second part is actively wrong -- real-mode address lookup will (for the most
>>> part) always succeed. Moreover, the gdb user will have no idea that you've silently
>>> changed addressing methods.
>>
>> I disagree. Real-mode address will mostly fail, since during the boot process Linux
>> kernels set the MMU to use only virtual addresses, so real mode addresses only work when
>> debugging the firmware or the early setup of the kernel. After that, GDB can basically
>> only see virtual addresses.
>
> Exactly. But you changed that so that any unmapped address will re-try with real-mode,
> which (outside of hv) simply maps real->physical and returns the input.
>
> One should have to perform some special action to see addresses in a different cpu
> context. I don't think that gdb supports such a special action at the moment. If you
> want that feature though, that's where you should start.
I think we can just drop this patch. The scenarios where debugging
across MMU contexts happen are quite limited.
My use case was a while back when implementing single-step for KVM
guests; there were some situations where GDB would have issues setting
breakpoints around kernel code that altered MSR_IR/DR. But that is
mostly anecdotal at this point. If I ever run into that again, now I
know where to look.
>
>
> r~
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] target/ppc: fix address translation bug for radix mmus
2021-06-15 12:25 ` Bruno Piazera Larsen
@ 2021-06-16 6:16 ` David Gibson
0 siblings, 0 replies; 24+ messages in thread
From: David Gibson @ 2021-06-16 6:16 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: 1395 bytes --]
On Tue, Jun 15, 2021 at 09:25:06AM -0300, Bruno Piazera Larsen wrote:
>
> On 15/06/2021 00:20, Richard Henderson wrote:
> > On 6/14/21 6:41 PM, David Gibson wrote:
> > > I think move these to mmu-book3s-v3.h, since they're correct for both
> > > the radix and hash sides of the modern book3s mmu.
> >
> > They're also correct for all non-booke mmus, e.g. hash32 and 6xx, which
> > is why I recommended internal.h (or some new mmu-internal.h).
> >
> > While neither hash32 nor 6xx have HV, and thus there is no second tlb
> > bug, it would still be More Correct to use mmu_idx instead of direct
> > references to msr_pr et al.
> yeah, I agree that I should change the documentation. Before I send a new
> version with everything corrected, I wanna make sure if I should leave them
> in internal and use it for hash32 and 6xx MMUs, or do I put them in
> mmu-book3s-v3.h, since only radix64 has that the bug?
Put them in mmu-book3s-v3.h for the time being, we can move it later
if it makes sense. I want to move to a mode model-first approach to
the softmmu code, so I think of these values as logically
per-MMU-style, even if they are the same in practice for many of them.
--
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] 24+ messages in thread
* Re: [RFC PATCH v2 2/2] target/ppc: make gdb able to translate priviledged addresses
2021-06-15 11:32 ` Bruno Piazera Larsen
2021-06-15 20:00 ` Richard Henderson
@ 2021-06-16 6:18 ` David Gibson
1 sibling, 0 replies; 24+ messages in thread
From: David Gibson @ 2021-06-16 6:18 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: 3140 bytes --]
On Tue, Jun 15, 2021 at 08:32:32AM -0300, Bruno Piazera Larsen wrote:
> On 14/06/2021 19:37, Richard Henderson wrote:
> > On 6/14/21 12:16 PM, Bruno Larsen (billionai) wrote:
> > > This patch changes ppc_cpu_get_phys_page_debug so that it is now
> > > able to translate both, priviledged and real mode addresses
> > > independently of whether the CPU executing it has those permissions
> > >
> > > This was mentioned by Fabiano as something that would be very useful to
> > > help with debugging, but could possibly constitute a security issue if
> > > that debug function can be called in some way by prodution code. the
> > > solution was implemented such that it would be trivial to wrap it around
> > > ifdefs for building only with --enable-debug, for instance, but we are
> > > not sure this is the best approach, hence why it is an RFC.
> > >
> > > Suggested-by: Fabiano Rosas<farosas@linux.ibm.com>
> > > Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br>
> > > ---
> > > target/ppc/mmu_helper.c | 23 +++++++++++++++++++++++
> > > 1 file changed, 23 insertions(+)
> >
> > I think the first part is unnecessary. Either the cpu is in supervisor
> > mode or it isn't, and gdb should use the correct address space. If you
> > really want to force supervisor lookup from a guest that is paused in
> > usermode, I suppose you could force MSR.PR=1 while you're performing the
> > access and set it back afterward.
> I don't see why GDB should not be able to see supervisor level addresses
> just because the CPU can't. when debugging, we wanna see exactly what QEMU
> sees, not what the guest sees, right?
That kind of depends whether you mean gdb attached to the gdb socket
provided by qemu - in which case I think you want it to see what the
guest sees - or gdb debugging qemu itself, in which case it does want
to see what qemu sees, but doesn't use this code path AFAIK.
> Now, if this is changing more than
> just privilege level, I agree there is a problem, but I wouldn't think it is
> the case...
> > I think the second part is actively wrong -- real-mode address lookup
> > will (for the most part) always succeed. Moreover, the gdb user will
> > have no idea that you've silently changed addressing methods.
>
> I disagree. Real-mode address will mostly fail, since during the boot
> process Linux kernels set the MMU to use only virtual addresses, so real
> mode addresses only work when debugging the firmware or the early setup of
> the kernel. After that, GDB can basically only see virtual addresses.
>
> Maybe there is a better way to handle this by having GDB warn the user that
> the CPU can not decode the address in it's current state, but I do think it
> is a good tool to have, as it would've made debugging the first RFC on this
> topic a bit easier, and farosas was actively complaining that isn't a
> feature yet.
>
> >
> > r~
--
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] 24+ messages in thread
* Re: [RFC PATCH v2 2/2] target/ppc: make gdb able to translate priviledged addresses
2021-06-15 21:37 ` Fabiano Rosas
@ 2021-06-16 12:07 ` Bruno Piazera Larsen
0 siblings, 0 replies; 24+ messages in thread
From: Bruno Piazera Larsen @ 2021-06-16 12:07 UTC (permalink / raw)
To: Fabiano Rosas, Richard Henderson, qemu-devel
Cc: luis.pires, Greg Kurz, lucas.araujo, fernando.valle, qemu-ppc,
matheus.ferst, david
[-- Attachment #1: Type: text/plain, Size: 3793 bytes --]
On 15/06/2021 18:37, Fabiano Rosas wrote:
> Richard Henderson <richard.henderson@linaro.org> writes:
>
>> On 6/15/21 4:32 AM, Bruno Piazera Larsen wrote:
>>> On 14/06/2021 19:37, Richard Henderson wrote:
>>>> On 6/14/21 12:16 PM, Bruno Larsen (billionai) wrote:
>>>>> This patch changes ppc_cpu_get_phys_page_debug so that it is now
>>>>> able to translate both, priviledged and real mode addresses
>>>>> independently of whether the CPU executing it has those permissions
>>>>>
>>>>> This was mentioned by Fabiano as something that would be very useful to
>>>>> help with debugging, but could possibly constitute a security issue if
>>>>> that debug function can be called in some way by prodution code. the
>>>>> solution was implemented such that it would be trivial to wrap it around
>>>>> ifdefs for building only with --enable-debug, for instance, but we are
>>>>> not sure this is the best approach, hence why it is an RFC.
>>>>>
>>>>> Suggested-by: Fabiano Rosas<farosas@linux.ibm.com>
>>>>> Signed-off-by: Bruno Larsen (billionai)<bruno.larsen@eldorado.org.br>
>>>>> ---
>>>>> target/ppc/mmu_helper.c | 23 +++++++++++++++++++++++
>>>>> 1 file changed, 23 insertions(+)
>>>> I think the first part is unnecessary. Either the cpu is in supervisor mode or it
>>>> isn't, and gdb should use the correct address space. If you really want to force
>>>> supervisor lookup from a guest that is paused in usermode, I suppose you could force
>>>> MSR.PR=1 while you're performing the access and set it back afterward.
>>> I don't see why GDB should not be able to see supervisor level addresses just because the
>>> CPU can't.
>> Because then when you are debugging, you then don't know whether the address is actually
>> accessible in the current cpu context.
>>
> @Bruno, so this is what I referred to somewhere else on the thread,
> people expect GDB to have the same access level of the currently
> executing code. So implementing my suggestion would break their
> workflow.
Ok, that makes sense. I'll drop this patch, then
>
>>>> I think the second part is actively wrong -- real-mode address lookup will (for the most
>>>> part) always succeed. Moreover, the gdb user will have no idea that you've silently
>>>> changed addressing methods.
>>> I disagree. Real-mode address will mostly fail, since during the boot process Linux
>>> kernels set the MMU to use only virtual addresses, so real mode addresses only work when
>>> debugging the firmware or the early setup of the kernel. After that, GDB can basically
>>> only see virtual addresses.
>> Exactly. But you changed that so that any unmapped address will re-try with real-mode,
>> which (outside of hv) simply maps real->physical and returns the input.
>>
>> One should have to perform some special action to see addresses in a different cpu
>> context. I don't think that gdb supports such a special action at the moment. If you
>> want that feature though, that's where you should start.
> I think we can just drop this patch. The scenarios where debugging
> across MMU contexts happen are quite limited.
>
> My use case was a while back when implementing single-step for KVM
> guests; there were some situations where GDB would have issues setting
> breakpoints around kernel code that altered MSR_IR/DR. But that is
> mostly anecdotal at this point. If I ever run into that again, now I
> know where to look.
At least now it's documented how to make it work, if someone else ever
needs it as well :)
>
>>
>> r~
--
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: 5713 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2021-06-16 12:09 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-14 19:16 [PATCH v2 1/2] target/ppc: fix address translation bug for radix mmus Bruno Larsen (billionai)
2021-06-14 19:16 ` [RFC PATCH v2 2/2] target/ppc: make gdb able to translate priviledged addresses Bruno Larsen (billionai)
2021-06-14 19:37 ` Philippe Mathieu-Daudé
2021-06-15 1:41 ` David Gibson
2021-06-15 12:12 ` Bruno Piazera Larsen
2021-06-14 21:25 ` Fabiano Rosas
2021-06-15 11:59 ` Bruno Piazera Larsen
2021-06-14 22:37 ` Richard Henderson
2021-06-15 11:32 ` Bruno Piazera Larsen
2021-06-15 20:00 ` Richard Henderson
2021-06-15 21:37 ` Fabiano Rosas
2021-06-16 12:07 ` Bruno Piazera Larsen
2021-06-16 6:18 ` David Gibson
2021-06-14 19:29 ` [PATCH v2 1/2] target/ppc: fix address translation bug for radix mmus Greg Kurz
2021-06-14 21:04 ` Fabiano Rosas
2021-06-15 1:18 ` David Gibson
2021-06-15 1:41 ` David Gibson
2021-06-15 3:20 ` Richard Henderson
2021-06-15 12:25 ` Bruno Piazera Larsen
2021-06-16 6:16 ` David Gibson
2021-06-15 13:57 ` Cédric Le Goater
2021-06-15 14:14 ` Philippe Mathieu-Daudé
2021-06-15 14:57 ` Bruno Piazera Larsen
2021-06-15 15:57 ` Cédric Le Goater
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.