* [PATCH 0/6] target/ppc: Various clean-up and fixes for radix64 @ 2020-05-07 17:26 Greg Kurz 2020-05-07 17:26 ` [PATCH 1/6] target/ppc: Pass const pointer to ppc_radix64_get_prot_amr() Greg Kurz ` (6 more replies) 0 siblings, 7 replies; 14+ messages in thread From: Greg Kurz @ 2020-05-07 17:26 UTC (permalink / raw) To: David Gibson; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel First three patches of this series are simple cleanups. The other ones fix some regressions introduced by Cedric's recent addition of partition-scoped translation. -- Greg --- Greg Kurz (6): target/ppc: Pass const pointer to ppc_radix64_get_prot_amr() target/ppc: Pass const pointer to ppc_radix64_get_fully_qualified_addr() target/ppc: Don't initialize some local variables in ppc_radix64_xlate() target/ppc: Add missing braces in ppc_radix64_partition_scoped_xlate() target/ppc: Fix arguments to ppc_radix64_partition_scoped_xlate() target/ppc: Don't update radix PTE R/C bits with gdbstub target/ppc/mmu-radix64.c | 46 +++++++++++++++++++++++++++++++--------------- target/ppc/mmu-radix64.h | 4 ++-- 2 files changed, 33 insertions(+), 17 deletions(-) ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/6] target/ppc: Pass const pointer to ppc_radix64_get_prot_amr() 2020-05-07 17:26 [PATCH 0/6] target/ppc: Various clean-up and fixes for radix64 Greg Kurz @ 2020-05-07 17:26 ` Greg Kurz 2020-05-07 17:26 ` [PATCH 2/6] target/ppc: Pass const pointer to ppc_radix64_get_fully_qualified_addr() Greg Kurz ` (5 subsequent siblings) 6 siblings, 0 replies; 14+ messages in thread From: Greg Kurz @ 2020-05-07 17:26 UTC (permalink / raw) To: David Gibson; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel This doesn't require write access to the CPU structure. Signed-off-by: Greg Kurz <groug@kaod.org> --- target/ppc/mmu-radix64.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/target/ppc/mmu-radix64.h b/target/ppc/mmu-radix64.h index 96228546aa85..f28c5794d071 100644 --- a/target/ppc/mmu-radix64.h +++ b/target/ppc/mmu-radix64.h @@ -55,9 +55,9 @@ static inline int ppc_radix64_get_prot_eaa(uint64_t pte) (pte & R_PTE_EAA_X ? PAGE_EXEC : 0); } -static inline int ppc_radix64_get_prot_amr(PowerPCCPU *cpu) +static inline int ppc_radix64_get_prot_amr(const PowerPCCPU *cpu) { - CPUPPCState *env = &cpu->env; + const CPUPPCState *env = &cpu->env; int amr = env->spr[SPR_AMR] >> 62; /* We only care about key0 AMR63:62 */ int iamr = env->spr[SPR_IAMR] >> 62; /* We only care about key0 IAMR63:62 */ ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/6] target/ppc: Pass const pointer to ppc_radix64_get_fully_qualified_addr() 2020-05-07 17:26 [PATCH 0/6] target/ppc: Various clean-up and fixes for radix64 Greg Kurz 2020-05-07 17:26 ` [PATCH 1/6] target/ppc: Pass const pointer to ppc_radix64_get_prot_amr() Greg Kurz @ 2020-05-07 17:26 ` Greg Kurz 2020-05-07 17:26 ` [PATCH 3/6] target/ppc: Don't initialize some local variables in ppc_radix64_xlate() Greg Kurz ` (4 subsequent siblings) 6 siblings, 0 replies; 14+ messages in thread From: Greg Kurz @ 2020-05-07 17:26 UTC (permalink / raw) To: David Gibson; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel This doesn't require write access to the CPU registers. Signed-off-by: Greg Kurz <groug@kaod.org> --- target/ppc/mmu-radix64.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c index 1404e53deca8..c76879f65b78 100644 --- a/target/ppc/mmu-radix64.c +++ b/target/ppc/mmu-radix64.c @@ -28,7 +28,8 @@ #include "mmu-radix64.h" #include "mmu-book3s-v3.h" -static bool ppc_radix64_get_fully_qualified_addr(CPUPPCState *env, vaddr eaddr, +static bool ppc_radix64_get_fully_qualified_addr(const CPUPPCState *env, + vaddr eaddr, uint64_t *lpid, uint64_t *pid) { if (msr_hv) { /* MSR[HV] -> Hypervisor/bare metal */ ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/6] target/ppc: Don't initialize some local variables in ppc_radix64_xlate() 2020-05-07 17:26 [PATCH 0/6] target/ppc: Various clean-up and fixes for radix64 Greg Kurz 2020-05-07 17:26 ` [PATCH 1/6] target/ppc: Pass const pointer to ppc_radix64_get_prot_amr() Greg Kurz 2020-05-07 17:26 ` [PATCH 2/6] target/ppc: Pass const pointer to ppc_radix64_get_fully_qualified_addr() Greg Kurz @ 2020-05-07 17:26 ` Greg Kurz 2020-05-11 9:07 ` Cédric Le Goater 2020-05-07 17:27 ` [PATCH 4/6] target/ppc: Add missing braces in ppc_radix64_partition_scoped_xlate() Greg Kurz ` (3 subsequent siblings) 6 siblings, 1 reply; 14+ messages in thread From: Greg Kurz @ 2020-05-07 17:26 UTC (permalink / raw) To: David Gibson; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel It is the job of the ppc_radix64_get_fully_qualified_addr() function which is called at the beginning of ppc_radix64_xlate() to set both lpid *and* pid. It doesn't buy us anything to initialize them first. Worse, a bug in ppc_radix64_get_fully_qualified_addr(), eg. failing to set either lpid or pid, would be undetectable by static analysis tools like coverity. Signed-off-by: Greg Kurz <groug@kaod.org> --- target/ppc/mmu-radix64.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c index c76879f65b78..5e2d912ee346 100644 --- a/target/ppc/mmu-radix64.c +++ b/target/ppc/mmu-radix64.c @@ -433,7 +433,7 @@ static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx, bool cause_excp) { CPUPPCState *env = &cpu->env; - uint64_t lpid = 0, pid = 0; + uint64_t lpid, pid; ppc_v3_pate_t pate; int psize, prot; hwaddr g_raddr; ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/6] target/ppc: Don't initialize some local variables in ppc_radix64_xlate() 2020-05-07 17:26 ` [PATCH 3/6] target/ppc: Don't initialize some local variables in ppc_radix64_xlate() Greg Kurz @ 2020-05-11 9:07 ` Cédric Le Goater 2020-05-11 10:12 ` Greg Kurz 0 siblings, 1 reply; 14+ messages in thread From: Cédric Le Goater @ 2020-05-11 9:07 UTC (permalink / raw) To: Greg Kurz, David Gibson; +Cc: qemu-ppc, qemu-devel On 5/7/20 7:26 PM, Greg Kurz wrote: > It is the job of the ppc_radix64_get_fully_qualified_addr() function > which is called at the beginning of ppc_radix64_xlate() to set both > lpid *and* pid. It doesn't buy us anything to initialize them first. > > Worse, a bug in ppc_radix64_get_fully_qualified_addr(), eg. failing to > set either lpid or pid, would be undetectable by static analysis tools > like coverity. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > target/ppc/mmu-radix64.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c > index c76879f65b78..5e2d912ee346 100644 > --- a/target/ppc/mmu-radix64.c > +++ b/target/ppc/mmu-radix64.c > @@ -433,7 +433,7 @@ static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx, > bool cause_excp) > { > CPUPPCState *env = &cpu->env; > - uint64_t lpid = 0, pid = 0; > + uint64_t lpid, pid; > ppc_v3_pate_t pate; > int psize, prot; > hwaddr g_raddr; > I am seeing this failure with gcc version 9.3.1 20200408 (Red Hat 9.3.1-2) (GCC) target/ppc/mmu-radix64.c: In function ‘ppc_radix64_xlate’: target/ppc/mmu-radix64.c:314:12: error: ‘pid’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 314 | offset = pid * sizeof(struct prtb_entry); | ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ target/ppc/mmu-radix64.c:439:20: note: ‘pid’ was declared here 439 | uint64_t lpid, pid; | ^~~ target/ppc/mmu-radix64.c:458:14: error: ‘lpid’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 458 | if (!ppc64_v3_get_pate(cpu, lpid, &pate)) { | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ CC ppc64-softmmu/target/ppc/fpu_helper.o This seems like a compiler optimization issue. C. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/6] target/ppc: Don't initialize some local variables in ppc_radix64_xlate() 2020-05-11 9:07 ` Cédric Le Goater @ 2020-05-11 10:12 ` Greg Kurz 0 siblings, 0 replies; 14+ messages in thread From: Greg Kurz @ 2020-05-11 10:12 UTC (permalink / raw) To: Cédric Le Goater; +Cc: qemu-ppc, qemu-devel, David Gibson On Mon, 11 May 2020 11:07:06 +0200 Cédric Le Goater <clg@kaod.org> wrote: > On 5/7/20 7:26 PM, Greg Kurz wrote: > > It is the job of the ppc_radix64_get_fully_qualified_addr() function > > which is called at the beginning of ppc_radix64_xlate() to set both > > lpid *and* pid. It doesn't buy us anything to initialize them first. > > > > Worse, a bug in ppc_radix64_get_fully_qualified_addr(), eg. failing to > > set either lpid or pid, would be undetectable by static analysis tools > > like coverity. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > target/ppc/mmu-radix64.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c > > index c76879f65b78..5e2d912ee346 100644 > > --- a/target/ppc/mmu-radix64.c > > +++ b/target/ppc/mmu-radix64.c > > @@ -433,7 +433,7 @@ static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx, > > bool cause_excp) > > { > > CPUPPCState *env = &cpu->env; > > - uint64_t lpid = 0, pid = 0; > > + uint64_t lpid, pid; > > ppc_v3_pate_t pate; > > int psize, prot; > > hwaddr g_raddr; > > > > I am seeing this failure with gcc version 9.3.1 20200408 (Red Hat 9.3.1-2) (GCC) > > target/ppc/mmu-radix64.c: In function ‘ppc_radix64_xlate’: > target/ppc/mmu-radix64.c:314:12: error: ‘pid’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > 314 | offset = pid * sizeof(struct prtb_entry); > | ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > target/ppc/mmu-radix64.c:439:20: note: ‘pid’ was declared here > 439 | uint64_t lpid, pid; > | ^~~ > target/ppc/mmu-radix64.c:458:14: error: ‘lpid’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > 458 | if (!ppc64_v3_get_pate(cpu, lpid, &pate)) { > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > CC ppc64-softmmu/target/ppc/fpu_helper.o > > > This seems like a compiler optimization issue. > Ah... it seems that gcc is trying to be smart but it doesn't realize that ppc_radix64_get_fully_qualified_addr() doesn't have any path that leaves lpid or pid unset... :-\ Adding a default: case in both switch statements is enough to silent gcc. I guess it may be easier for David if I post a v2 of the entire series that addresses all the comments. Thanks! > C. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/6] target/ppc: Add missing braces in ppc_radix64_partition_scoped_xlate() 2020-05-07 17:26 [PATCH 0/6] target/ppc: Various clean-up and fixes for radix64 Greg Kurz ` (2 preceding siblings ...) 2020-05-07 17:26 ` [PATCH 3/6] target/ppc: Don't initialize some local variables in ppc_radix64_xlate() Greg Kurz @ 2020-05-07 17:27 ` Greg Kurz 2020-05-07 17:27 ` [PATCH 5/6] target/ppc: Fix arguments to ppc_radix64_partition_scoped_xlate() Greg Kurz ` (2 subsequent siblings) 6 siblings, 0 replies; 14+ messages in thread From: Greg Kurz @ 2020-05-07 17:27 UTC (permalink / raw) To: David Gibson; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel As per CODING_STYLE. Fixes: d04ea940c597 "target/ppc: Add support for Radix partition-scoped translation" Signed-off-by: Greg Kurz <groug@kaod.org> --- target/ppc/mmu-radix64.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c index 5e2d912ee346..956519b3ad20 100644 --- a/target/ppc/mmu-radix64.c +++ b/target/ppc/mmu-radix64.c @@ -282,8 +282,9 @@ static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu, int rwx, pate.dw0 & PRTBE_R_RPDS, h_raddr, h_page_size, &pte, &fault_cause, &pte_addr) || ppc_radix64_check_prot(cpu, rwx, pte, &fault_cause, h_prot, true)) { - if (pde_addr) /* address being translated was that of a guest pde */ + if (pde_addr) { /* address being translated was that of a guest pde */ fault_cause |= DSISR_PRTABLE_FAULT; + } if (cause_excp) { ppc_radix64_raise_hsi(cpu, rwx, eaddr, g_raddr, fault_cause); } ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 5/6] target/ppc: Fix arguments to ppc_radix64_partition_scoped_xlate() 2020-05-07 17:26 [PATCH 0/6] target/ppc: Various clean-up and fixes for radix64 Greg Kurz ` (3 preceding siblings ...) 2020-05-07 17:27 ` [PATCH 4/6] target/ppc: Add missing braces in ppc_radix64_partition_scoped_xlate() Greg Kurz @ 2020-05-07 17:27 ` Greg Kurz 2020-05-07 17:27 ` [PATCH 6/6] target/ppc: Don't update radix PTE R/C bits with gdbstub Greg Kurz 2020-05-11 1:44 ` [PATCH 0/6] target/ppc: Various clean-up and fixes for radix64 David Gibson 6 siblings, 0 replies; 14+ messages in thread From: Greg Kurz @ 2020-05-07 17:27 UTC (permalink / raw) To: David Gibson; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel The last two arguments have the bool type. Also, we shouldn't raise an exception when using gdbstub. This was found while reading the code. Since it only affects the powernv machine, I didn't dig further to find an actual bug. Fixes: d04ea940c597 "target/ppc: Add support for Radix partition-scoped translation" Signed-off-by: Greg Kurz <groug@kaod.org> --- target/ppc/mmu-radix64.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c index 956519b3ad20..ceeb3dfe2d49 100644 --- a/target/ppc/mmu-radix64.c +++ b/target/ppc/mmu-radix64.c @@ -335,7 +335,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx, */ ret = ppc_radix64_partition_scoped_xlate(cpu, 0, eaddr, prtbe_addr, pate, &h_raddr, &h_prot, - &h_page_size, 1, 1); + &h_page_size, true, + cause_excp); if (ret) { return ret; } @@ -374,7 +375,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx, do { ret = ppc_radix64_partition_scoped_xlate(cpu, 0, eaddr, pte_addr, pate, &h_raddr, &h_prot, - &h_page_size, 1, 1); + &h_page_size, true, + cause_excp); if (ret) { return ret; } ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 6/6] target/ppc: Don't update radix PTE R/C bits with gdbstub 2020-05-07 17:26 [PATCH 0/6] target/ppc: Various clean-up and fixes for radix64 Greg Kurz ` (4 preceding siblings ...) 2020-05-07 17:27 ` [PATCH 5/6] target/ppc: Fix arguments to ppc_radix64_partition_scoped_xlate() Greg Kurz @ 2020-05-07 17:27 ` Greg Kurz 2020-05-11 1:43 ` David Gibson 2020-05-11 1:44 ` [PATCH 0/6] target/ppc: Various clean-up and fixes for radix64 David Gibson 6 siblings, 1 reply; 14+ messages in thread From: Greg Kurz @ 2020-05-07 17:27 UTC (permalink / raw) To: David Gibson; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel gdbstub shouldn't silently change guest visible state when doing address translation. While here drop a not very useful comment. This was found while reading the code. I could verify that this affects both powernv and pseries, but I failed to observe any actual bug. Fixes: d04ea940c597 "target/ppc: Add support for Radix partition-scoped translation" Signed-off-by: Greg Kurz <groug@kaod.org> --- target/ppc/mmu-radix64.c | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c index ceeb3dfe2d49..bc51cd89a079 100644 --- a/target/ppc/mmu-radix64.c +++ b/target/ppc/mmu-radix64.c @@ -270,7 +270,8 @@ static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu, int rwx, ppc_v3_pate_t pate, hwaddr *h_raddr, int *h_prot, int *h_page_size, bool pde_addr, - bool cause_excp) + bool cause_excp, + bool cause_rc_update) { int fault_cause = 0; hwaddr pte_addr; @@ -291,8 +292,9 @@ static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu, int rwx, return 1; } - /* Update Reference and Change Bits */ - ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, h_prot); + if (cause_rc_update) { + ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, h_prot); + } return 0; } @@ -301,7 +303,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx, vaddr eaddr, uint64_t pid, ppc_v3_pate_t pate, hwaddr *g_raddr, int *g_prot, int *g_page_size, - bool cause_excp) + bool cause_excp, + bool cause_rc_update) { CPUState *cs = CPU(cpu); CPUPPCState *env = &cpu->env; @@ -336,7 +339,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx, ret = ppc_radix64_partition_scoped_xlate(cpu, 0, eaddr, prtbe_addr, pate, &h_raddr, &h_prot, &h_page_size, true, - cause_excp); + cause_excp, + cause_rc_update); if (ret) { return ret; } @@ -376,7 +380,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx, ret = ppc_radix64_partition_scoped_xlate(cpu, 0, eaddr, pte_addr, pate, &h_raddr, &h_prot, &h_page_size, true, - cause_excp); + cause_excp, + cause_rc_update); if (ret) { return ret; } @@ -408,7 +413,9 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx, return 1; } - ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, g_prot); + if (cause_rc_update) { + ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, g_prot); + } return 0; } @@ -433,7 +440,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx, static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx, bool relocation, hwaddr *raddr, int *psizep, int *protp, - bool cause_excp) + bool cause_excp, + bool cause_rc_update) { CPUPPCState *env = &cpu->env; uint64_t lpid, pid; @@ -483,7 +491,9 @@ static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx, if (relocation) { int ret = ppc_radix64_process_scoped_xlate(cpu, rwx, eaddr, pid, pate, &g_raddr, &prot, - &psize, cause_excp); + &psize, + cause_excp, + cause_rc_update); if (ret) { return ret; } @@ -506,7 +516,9 @@ static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx, ret = ppc_radix64_partition_scoped_xlate(cpu, rwx, eaddr, g_raddr, pate, raddr, &prot, &psize, - 0, cause_excp); + 0, + cause_excp, + cause_rc_update); if (ret) { return ret; } @@ -562,7 +574,7 @@ int ppc_radix64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr, int rwx, /* Translate eaddr to raddr (where raddr is addr qemu needs for access) */ if (ppc_radix64_xlate(cpu, eaddr, rwx, relocation, &raddr, - &page_size, &prot, true)) { + &page_size, &prot, true, true)) { return 1; } @@ -584,7 +596,7 @@ hwaddr ppc_radix64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong eaddr) } if (ppc_radix64_xlate(cpu, eaddr, 0, msr_dr, &raddr, &psize, - &prot, false)) { + &prot, false, false)) { return -1; } ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 6/6] target/ppc: Don't update radix PTE R/C bits with gdbstub 2020-05-07 17:27 ` [PATCH 6/6] target/ppc: Don't update radix PTE R/C bits with gdbstub Greg Kurz @ 2020-05-11 1:43 ` David Gibson 2020-05-11 9:30 ` Greg Kurz 0 siblings, 1 reply; 14+ messages in thread From: David Gibson @ 2020-05-11 1:43 UTC (permalink / raw) To: Greg Kurz; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel [-- Attachment #1: Type: text/plain, Size: 6832 bytes --] On Thu, May 07, 2020 at 07:27:15PM +0200, Greg Kurz wrote: > gdbstub shouldn't silently change guest visible state when doing address > translation. While here drop a not very useful comment. > > This was found while reading the code. I could verify that this affects > both powernv and pseries, but I failed to observe any actual bug. > > Fixes: d04ea940c597 "target/ppc: Add support for Radix partition-scoped translation" > Signed-off-by: Greg Kurz <groug@kaod.org> It's a real fix. But AFAICT we'll always have cause_excp == cause_rc_update, and I can't see any reason we'd ever them different. So I'd prefer to just rename the flag and use it for both tests. Maybe just 'guest_visible' ? > --- > target/ppc/mmu-radix64.c | 36 ++++++++++++++++++++++++------------ > 1 file changed, 24 insertions(+), 12 deletions(-) > > diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c > index ceeb3dfe2d49..bc51cd89a079 100644 > --- a/target/ppc/mmu-radix64.c > +++ b/target/ppc/mmu-radix64.c > @@ -270,7 +270,8 @@ static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu, int rwx, > ppc_v3_pate_t pate, > hwaddr *h_raddr, int *h_prot, > int *h_page_size, bool pde_addr, > - bool cause_excp) > + bool cause_excp, > + bool cause_rc_update) > { > int fault_cause = 0; > hwaddr pte_addr; > @@ -291,8 +292,9 @@ static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu, int rwx, > return 1; > } > > - /* Update Reference and Change Bits */ > - ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, h_prot); > + if (cause_rc_update) { > + ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, h_prot); > + } > > return 0; > } > @@ -301,7 +303,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx, > vaddr eaddr, uint64_t pid, > ppc_v3_pate_t pate, hwaddr *g_raddr, > int *g_prot, int *g_page_size, > - bool cause_excp) > + bool cause_excp, > + bool cause_rc_update) > { > CPUState *cs = CPU(cpu); > CPUPPCState *env = &cpu->env; > @@ -336,7 +339,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx, > ret = ppc_radix64_partition_scoped_xlate(cpu, 0, eaddr, prtbe_addr, > pate, &h_raddr, &h_prot, > &h_page_size, true, > - cause_excp); > + cause_excp, > + cause_rc_update); > if (ret) { > return ret; > } > @@ -376,7 +380,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx, > ret = ppc_radix64_partition_scoped_xlate(cpu, 0, eaddr, pte_addr, > pate, &h_raddr, &h_prot, > &h_page_size, true, > - cause_excp); > + cause_excp, > + cause_rc_update); > if (ret) { > return ret; > } > @@ -408,7 +413,9 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx, > return 1; > } > > - ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, g_prot); > + if (cause_rc_update) { > + ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, g_prot); > + } > > return 0; > } > @@ -433,7 +440,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx, > static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx, > bool relocation, > hwaddr *raddr, int *psizep, int *protp, > - bool cause_excp) > + bool cause_excp, > + bool cause_rc_update) > { > CPUPPCState *env = &cpu->env; > uint64_t lpid, pid; > @@ -483,7 +491,9 @@ static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx, > if (relocation) { > int ret = ppc_radix64_process_scoped_xlate(cpu, rwx, eaddr, pid, > pate, &g_raddr, &prot, > - &psize, cause_excp); > + &psize, > + cause_excp, > + cause_rc_update); > if (ret) { > return ret; > } > @@ -506,7 +516,9 @@ static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx, > > ret = ppc_radix64_partition_scoped_xlate(cpu, rwx, eaddr, g_raddr, > pate, raddr, &prot, &psize, > - 0, cause_excp); > + 0, > + cause_excp, > + cause_rc_update); > if (ret) { > return ret; > } > @@ -562,7 +574,7 @@ int ppc_radix64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr, int rwx, > > /* Translate eaddr to raddr (where raddr is addr qemu needs for access) */ > if (ppc_radix64_xlate(cpu, eaddr, rwx, relocation, &raddr, > - &page_size, &prot, true)) { > + &page_size, &prot, true, true)) { > return 1; > } > > @@ -584,7 +596,7 @@ hwaddr ppc_radix64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong eaddr) > } > > if (ppc_radix64_xlate(cpu, eaddr, 0, msr_dr, &raddr, &psize, > - &prot, false)) { > + &prot, false, false)) { > 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] 14+ messages in thread
* Re: [PATCH 6/6] target/ppc: Don't update radix PTE R/C bits with gdbstub 2020-05-11 1:43 ` David Gibson @ 2020-05-11 9:30 ` Greg Kurz 0 siblings, 0 replies; 14+ messages in thread From: Greg Kurz @ 2020-05-11 9:30 UTC (permalink / raw) To: David Gibson; +Cc: qemu-ppc, Cédric Le Goater, Nicholas Piggin, qemu-devel [-- Attachment #1: Type: text/plain, Size: 7763 bytes --] On Mon, 11 May 2020 11:43:48 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Thu, May 07, 2020 at 07:27:15PM +0200, Greg Kurz wrote: > > gdbstub shouldn't silently change guest visible state when doing address > > translation. While here drop a not very useful comment. > > > > This was found while reading the code. I could verify that this affects > > both powernv and pseries, but I failed to observe any actual bug. > > > > Fixes: d04ea940c597 "target/ppc: Add support for Radix partition-scoped translation" > > Signed-off-by: Greg Kurz <groug@kaod.org> > > It's a real fix. But AFAICT we'll always have cause_excp == > cause_rc_update, and I can't see any reason we'd ever them different. This is definitely true as of today because all memory accesses are performed by a CPU, but POWER9 has accelerator agents (eg. NPU) that can also issue load/store operations on the PowerBus. I'm currently doing some experiments to model the NPU as used with OpenCAPI (the ultimate goal being to have another user for XIVE). This requires to be able to do EA->RA translation without a CPU context, as done by the NestMMU in real HW. This requires quite some code refactoring in mmu-radix64.c and I opted to keep these flags separate as a first step... but you're right, since page faults are always handled on behalf of a CPU, I don't see any reason for them to be different. Cc'ing Nick in case I've missed something. > So I'd prefer to just rename the flag and use it for both tests. > > Maybe just 'guest_visible' ? > Sounds good. > > --- > > target/ppc/mmu-radix64.c | 36 ++++++++++++++++++++++++------------ > > 1 file changed, 24 insertions(+), 12 deletions(-) > > > > diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c > > index ceeb3dfe2d49..bc51cd89a079 100644 > > --- a/target/ppc/mmu-radix64.c > > +++ b/target/ppc/mmu-radix64.c > > @@ -270,7 +270,8 @@ static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu, int rwx, > > ppc_v3_pate_t pate, > > hwaddr *h_raddr, int *h_prot, > > int *h_page_size, bool pde_addr, > > - bool cause_excp) > > + bool cause_excp, > > + bool cause_rc_update) > > { > > int fault_cause = 0; > > hwaddr pte_addr; > > @@ -291,8 +292,9 @@ static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu, int rwx, > > return 1; > > } > > > > - /* Update Reference and Change Bits */ > > - ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, h_prot); > > + if (cause_rc_update) { > > + ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, h_prot); > > + } > > > > return 0; > > } > > @@ -301,7 +303,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx, > > vaddr eaddr, uint64_t pid, > > ppc_v3_pate_t pate, hwaddr *g_raddr, > > int *g_prot, int *g_page_size, > > - bool cause_excp) > > + bool cause_excp, > > + bool cause_rc_update) > > { > > CPUState *cs = CPU(cpu); > > CPUPPCState *env = &cpu->env; > > @@ -336,7 +339,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx, > > ret = ppc_radix64_partition_scoped_xlate(cpu, 0, eaddr, prtbe_addr, > > pate, &h_raddr, &h_prot, > > &h_page_size, true, > > - cause_excp); > > + cause_excp, > > + cause_rc_update); > > if (ret) { > > return ret; > > } > > @@ -376,7 +380,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx, > > ret = ppc_radix64_partition_scoped_xlate(cpu, 0, eaddr, pte_addr, > > pate, &h_raddr, &h_prot, > > &h_page_size, true, > > - cause_excp); > > + cause_excp, > > + cause_rc_update); > > if (ret) { > > return ret; > > } > > @@ -408,7 +413,9 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx, > > return 1; > > } > > > > - ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, g_prot); > > + if (cause_rc_update) { > > + ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, g_prot); > > + } > > > > return 0; > > } > > @@ -433,7 +440,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx, > > static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx, > > bool relocation, > > hwaddr *raddr, int *psizep, int *protp, > > - bool cause_excp) > > + bool cause_excp, > > + bool cause_rc_update) > > { > > CPUPPCState *env = &cpu->env; > > uint64_t lpid, pid; > > @@ -483,7 +491,9 @@ static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx, > > if (relocation) { > > int ret = ppc_radix64_process_scoped_xlate(cpu, rwx, eaddr, pid, > > pate, &g_raddr, &prot, > > - &psize, cause_excp); > > + &psize, > > + cause_excp, > > + cause_rc_update); > > if (ret) { > > return ret; > > } > > @@ -506,7 +516,9 @@ static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx, > > > > ret = ppc_radix64_partition_scoped_xlate(cpu, rwx, eaddr, g_raddr, > > pate, raddr, &prot, &psize, > > - 0, cause_excp); > > + 0, > > + cause_excp, > > + cause_rc_update); > > if (ret) { > > return ret; > > } > > @@ -562,7 +574,7 @@ int ppc_radix64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr, int rwx, > > > > /* Translate eaddr to raddr (where raddr is addr qemu needs for access) */ > > if (ppc_radix64_xlate(cpu, eaddr, rwx, relocation, &raddr, > > - &page_size, &prot, true)) { > > + &page_size, &prot, true, true)) { > > return 1; > > } > > > > @@ -584,7 +596,7 @@ hwaddr ppc_radix64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong eaddr) > > } > > > > if (ppc_radix64_xlate(cpu, eaddr, 0, msr_dr, &raddr, &psize, > > - &prot, false)) { > > + &prot, false, false)) { > > return -1; > > } > > > > > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/6] target/ppc: Various clean-up and fixes for radix64 2020-05-07 17:26 [PATCH 0/6] target/ppc: Various clean-up and fixes for radix64 Greg Kurz ` (5 preceding siblings ...) 2020-05-07 17:27 ` [PATCH 6/6] target/ppc: Don't update radix PTE R/C bits with gdbstub Greg Kurz @ 2020-05-11 1:44 ` David Gibson 2020-05-11 16:55 ` Greg Kurz 6 siblings, 1 reply; 14+ messages in thread From: David Gibson @ 2020-05-11 1:44 UTC (permalink / raw) To: Greg Kurz; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel [-- Attachment #1: Type: text/plain, Size: 492 bytes --] On Thu, May 07, 2020 at 07:26:32PM +0200, Greg Kurz wrote: > First three patches of this series are simple cleanups. The other > ones fix some regressions introduced by Cedric's recent addition > of partition-scoped translation. 1-5/6 applied to ppc-for-5.1. I have some comments on 6/6. -- 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] 14+ messages in thread
* Re: [PATCH 0/6] target/ppc: Various clean-up and fixes for radix64 2020-05-11 1:44 ` [PATCH 0/6] target/ppc: Various clean-up and fixes for radix64 David Gibson @ 2020-05-11 16:55 ` Greg Kurz 2020-05-12 0:00 ` David Gibson 0 siblings, 1 reply; 14+ messages in thread From: Greg Kurz @ 2020-05-11 16:55 UTC (permalink / raw) To: David Gibson; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel [-- Attachment #1: Type: text/plain, Size: 596 bytes --] On Mon, 11 May 2020 11:44:26 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Thu, May 07, 2020 at 07:26:32PM +0200, Greg Kurz wrote: > > First three patches of this series are simple cleanups. The other > > ones fix some regressions introduced by Cedric's recent addition > > of partition-scoped translation. > > 1-5/6 applied to ppc-for-5.1. I have some comments on 6/6. > As said in another mail, since patch 3 breaks build with gcc-9.3.1, I intend to send a v2 for the whole series later this week. I suggest you simply drop the patches you've applied for now. [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/6] target/ppc: Various clean-up and fixes for radix64 2020-05-11 16:55 ` Greg Kurz @ 2020-05-12 0:00 ` David Gibson 0 siblings, 0 replies; 14+ messages in thread From: David Gibson @ 2020-05-12 0:00 UTC (permalink / raw) To: Greg Kurz; +Cc: qemu-ppc, Cédric Le Goater, qemu-devel [-- Attachment #1: Type: text/plain, Size: 893 bytes --] On Mon, May 11, 2020 at 06:55:21PM +0200, Greg Kurz wrote: > On Mon, 11 May 2020 11:44:26 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Thu, May 07, 2020 at 07:26:32PM +0200, Greg Kurz wrote: > > > First three patches of this series are simple cleanups. The other > > > ones fix some regressions introduced by Cedric's recent addition > > > of partition-scoped translation. > > > > 1-5/6 applied to ppc-for-5.1. I have some comments on 6/6. > > > > As said in another mail, since patch 3 breaks build with gcc-9.3.1, I > intend to send a v2 for the whole series later this week. I suggest > you simply drop the patches you've applied for now. Ok, done. -- 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] 14+ messages in thread
end of thread, other threads:[~2020-05-12 3:35 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-07 17:26 [PATCH 0/6] target/ppc: Various clean-up and fixes for radix64 Greg Kurz 2020-05-07 17:26 ` [PATCH 1/6] target/ppc: Pass const pointer to ppc_radix64_get_prot_amr() Greg Kurz 2020-05-07 17:26 ` [PATCH 2/6] target/ppc: Pass const pointer to ppc_radix64_get_fully_qualified_addr() Greg Kurz 2020-05-07 17:26 ` [PATCH 3/6] target/ppc: Don't initialize some local variables in ppc_radix64_xlate() Greg Kurz 2020-05-11 9:07 ` Cédric Le Goater 2020-05-11 10:12 ` Greg Kurz 2020-05-07 17:27 ` [PATCH 4/6] target/ppc: Add missing braces in ppc_radix64_partition_scoped_xlate() Greg Kurz 2020-05-07 17:27 ` [PATCH 5/6] target/ppc: Fix arguments to ppc_radix64_partition_scoped_xlate() Greg Kurz 2020-05-07 17:27 ` [PATCH 6/6] target/ppc: Don't update radix PTE R/C bits with gdbstub Greg Kurz 2020-05-11 1:43 ` David Gibson 2020-05-11 9:30 ` Greg Kurz 2020-05-11 1:44 ` [PATCH 0/6] target/ppc: Various clean-up and fixes for radix64 David Gibson 2020-05-11 16:55 ` Greg Kurz 2020-05-12 0:00 ` David Gibson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.