| Message ID | 158887243487.1564424.7276382177976503972.stgit@bahia.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/ppc: Various clean-up and fixes for radix64 | expand |
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; > } > >
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; > > } > > > > >
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; }
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(-)