[PATCH 0/6] target/ppc: Various clean-up and fixes for radix64

[PATCH 0/6] target/ppc: Various clean-up and fixes for radix64

[Greg Kurz]

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(-)

[PATCH 1/6] target/ppc: Pass const pointer to ppc_radix64_get_prot_amr()

[Greg Kurz]

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 */
 

[PATCH 2/6] target/ppc: Pass const pointer to ppc_radix64_get_fully_qualified_addr()

[Greg Kurz]

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 */

[PATCH 3/6] target/ppc: Don't initialize some local variables in ppc_radix64_xlate()

[Greg Kurz]

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;

[PATCH 4/6] target/ppc: Add missing braces in ppc_radix64_partition_scoped_xlate()

[Greg Kurz]

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);
         }

[PATCH 5/6] target/ppc: Fix arguments to ppc_radix64_partition_scoped_xlate()

[Greg Kurz]

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;
             }

[PATCH 6/6] target/ppc: Don't update radix PTE R/C bits with gdbstub

[Greg Kurz]

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;
     }
 

Re: [PATCH 6/6] target/ppc: Don't update radix PTE R/C bits with gdbstub

[David Gibson]

[-- 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 --]

Re: [PATCH 0/6] target/ppc: Various clean-up and fixes for radix64

[David Gibson]

[-- 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 --]

Re: [PATCH 3/6] target/ppc: Don't initialize some local variables in ppc_radix64_xlate()

[Cédric Le Goater]

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.

Re: [PATCH 6/6] target/ppc: Don't update radix PTE R/C bits with gdbstub

[Greg Kurz]

[-- 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 --]

Re: [PATCH 3/6] target/ppc: Don't initialize some local variables in ppc_radix64_xlate()

[Greg Kurz]

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.

Re: [PATCH 0/6] target/ppc: Various clean-up and fixes for radix64

[Greg Kurz]

[-- 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 --]

Re: [PATCH 0/6] target/ppc: Various clean-up and fixes for radix64

[David Gibson]

[-- 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 --]