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

[PATCH v2 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.

Changes since v1:
- fix build break in patch 3
- introduce guest_visible argument in patch 6

--
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 |   53 ++++++++++++++++++++++++++++------------------
 target/ppc/mmu-radix64.h |    4 ++-
 2 files changed, 34 insertions(+), 23 deletions(-)

[PATCH v2 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 */
 

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

[Cédric Le Goater]

On 5/14/20 12:56 AM, Greg Kurz wrote:
> This doesn't require write access to the CPU structure.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Cédric Le Goater <clg@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 v2 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 */

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

[Cédric Le Goater]

On 5/14/20 12:56 AM, Greg Kurz wrote:
> This doesn't require write access to the CPU registers.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Cédric Le Goater <clg@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 v2 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.

Some recent versions of gcc (eg. gcc-9.3.1-2.fc30) may still think
that lpid or pid is used uninitialized though, so this also adds
default cases in the switch statements to make it clear this cannot
happen.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 target/ppc/mmu-radix64.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
index c76879f65b78..07f956c9864f 100644
--- a/target/ppc/mmu-radix64.c
+++ b/target/ppc/mmu-radix64.c
@@ -50,6 +50,8 @@ static bool ppc_radix64_get_fully_qualified_addr(const CPUPPCState *env,
             *lpid = 0;
             *pid = 0;
             break;
+        default:
+            g_assert_not_reached();
         }
     } else {  /* !MSR[HV] -> Guest */
         switch (eaddr & R_EADDR_QUADRANT) {
@@ -64,6 +66,8 @@ static bool ppc_radix64_get_fully_qualified_addr(const CPUPPCState *env,
             *lpid = env->spr[SPR_LPIDR];
             *pid = 0; /* pid set to 0 -> addresses guest operating system */
             break;
+        default:
+            g_assert_not_reached();
         }
     }
 
@@ -433,7 +437,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;

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

[Cédric Le Goater]

On 5/14/20 12:57 AM, 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.
> 
> Some recent versions of gcc (eg. gcc-9.3.1-2.fc30) may still think
> that lpid or pid is used uninitialized though, so this also adds
> default cases in the switch statements to make it clear this cannot
> happen.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  target/ppc/mmu-radix64.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
> index c76879f65b78..07f956c9864f 100644
> --- a/target/ppc/mmu-radix64.c
> +++ b/target/ppc/mmu-radix64.c
> @@ -50,6 +50,8 @@ static bool ppc_radix64_get_fully_qualified_addr(const CPUPPCState *env,
>              *lpid = 0;
>              *pid = 0;
>              break;
> +        default:
> +            g_assert_not_reached();
>          }
>      } else {  /* !MSR[HV] -> Guest */
>          switch (eaddr & R_EADDR_QUADRANT) {
> @@ -64,6 +66,8 @@ static bool ppc_radix64_get_fully_qualified_addr(const CPUPPCState *env,
>              *lpid = env->spr[SPR_LPIDR];
>              *pid = 0; /* pid set to 0 -> addresses guest operating system */
>              break;
> +        default:
> +            g_assert_not_reached();
>          }
>      }
>  
> @@ -433,7 +437,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 v2 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 07f956c9864f..fb7dfe25ba6f 100644
--- a/target/ppc/mmu-radix64.c
+++ b/target/ppc/mmu-radix64.c
@@ -286,8 +286,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);
         }

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

[Cédric Le Goater]

On 5/14/20 12:57 AM, Greg Kurz wrote:
> As per CODING_STYLE.
> 
> Fixes: d04ea940c597 "target/ppc: Add support for Radix partition-scoped translation"
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Cédric Le Goater <clg@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 07f956c9864f..fb7dfe25ba6f 100644
> --- a/target/ppc/mmu-radix64.c
> +++ b/target/ppc/mmu-radix64.c
> @@ -286,8 +286,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 v2 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 fb7dfe25ba6f..7ce37cb778db 100644
--- a/target/ppc/mmu-radix64.c
+++ b/target/ppc/mmu-radix64.c
@@ -339,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, 1, 1);
+                                                 &h_page_size, true,
+                                                 cause_excp);
         if (ret) {
             return ret;
         }
@@ -378,7 +379,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;
             }

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

[Cédric Le Goater]

On 5/14/20 12:57 AM, Greg Kurz wrote:
> 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>

Reviewed-by: Cédric Le Goater <clg@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 fb7dfe25ba6f..7ce37cb778db 100644
> --- a/target/ppc/mmu-radix64.c
> +++ b/target/ppc/mmu-radix64.c
> @@ -339,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, 1, 1);
> +                                                 &h_page_size, true,
> +                                                 cause_excp);
>          if (ret) {
>              return ret;
>          }
> @@ -378,7 +379,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 v2 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. Since the R/C bits can only be updated when handling a MMU
fault, let's reuse the cause_excp flag and rename it to guest_visible.
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 |   39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
index 7ce37cb778db..0d3922537c4c 100644
--- a/target/ppc/mmu-radix64.c
+++ b/target/ppc/mmu-radix64.c
@@ -274,7 +274,7 @@ 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 guest_visible)
 {
     int fault_cause = 0;
     hwaddr pte_addr;
@@ -289,14 +289,15 @@ static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu, int rwx,
         if (pde_addr) { /* address being translated was that of a guest pde */
             fault_cause |= DSISR_PRTABLE_FAULT;
         }
-        if (cause_excp) {
+        if (guest_visible) {
             ppc_radix64_raise_hsi(cpu, rwx, eaddr, g_raddr, fault_cause);
         }
         return 1;
     }
 
-    /* Update Reference and Change Bits */
-    ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, h_prot);
+    if (guest_visible) {
+        ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, h_prot);
+    }
 
     return 0;
 }
@@ -305,7 +306,7 @@ 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 guest_visible)
 {
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
@@ -319,7 +320,7 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx,
     size = 1ULL << ((pate.dw1 & PATE1_R_PRTS) + 12);
     if (offset >= size) {
         /* offset exceeds size of the process table */
-        if (cause_excp) {
+        if (guest_visible) {
             ppc_radix64_raise_si(cpu, rwx, eaddr, DSISR_NOPTE);
         }
         return 1;
@@ -340,7 +341,7 @@ 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);
+                                                 guest_visible);
         if (ret) {
             return ret;
         }
@@ -360,7 +361,7 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx,
                                     &fault_cause, &pte_addr);
         if (ret) {
             /* No valid PTE */
-            if (cause_excp) {
+            if (guest_visible) {
                 ppc_radix64_raise_si(cpu, rwx, eaddr, fault_cause);
             }
             return ret;
@@ -380,7 +381,7 @@ 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);
+                                                     guest_visible);
             if (ret) {
                 return ret;
             }
@@ -389,7 +390,7 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx,
                                          &nls, g_page_size, &pte, &fault_cause);
             if (ret) {
                 /* No valid pte */
-                if (cause_excp) {
+                if (guest_visible) {
                     ppc_radix64_raise_si(cpu, rwx, eaddr, fault_cause);
                 }
                 return ret;
@@ -406,13 +407,15 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx,
 
     if (ppc_radix64_check_prot(cpu, rwx, pte, &fault_cause, g_prot, false)) {
         /* Access denied due to protection */
-        if (cause_excp) {
+        if (guest_visible) {
             ppc_radix64_raise_si(cpu, rwx, eaddr, fault_cause);
         }
         return 1;
     }
 
-    ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, g_prot);
+    if (guest_visible) {
+        ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, g_prot);
+    }
 
     return 0;
 }
@@ -437,7 +440,7 @@ 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 guest_visible)
 {
     CPUPPCState *env = &cpu->env;
     uint64_t lpid, pid;
@@ -447,7 +450,7 @@ static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx,
 
     /* Virtual Mode Access - get the fully qualified address */
     if (!ppc_radix64_get_fully_qualified_addr(&cpu->env, eaddr, &lpid, &pid)) {
-        if (cause_excp) {
+        if (guest_visible) {
             ppc_radix64_raise_segi(cpu, rwx, eaddr);
         }
         return 1;
@@ -460,13 +463,13 @@ static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx,
         vhc->get_pate(cpu->vhyp, &pate);
     } else {
         if (!ppc64_v3_get_pate(cpu, lpid, &pate)) {
-            if (cause_excp) {
+            if (guest_visible) {
                 ppc_radix64_raise_si(cpu, rwx, eaddr, DSISR_NOPTE);
             }
             return 1;
         }
         if (!validate_pate(cpu, lpid, &pate)) {
-            if (cause_excp) {
+            if (guest_visible) {
                 ppc_radix64_raise_si(cpu, rwx, eaddr, DSISR_R_BADCONFIG);
             }
             return 1;
@@ -487,7 +490,7 @@ 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, guest_visible);
         if (ret) {
             return ret;
         }
@@ -510,7 +513,7 @@ 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, guest_visible);
             if (ret) {
                 return ret;
             }

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

[Cédric Le Goater]

On 5/14/20 12:57 AM, Greg Kurz wrote:
> gdbstub shouldn't silently change guest visible state when doing address
> translation. Since the R/C bits can only be updated when handling a MMU
> fault, let's reuse the cause_excp flag and rename it to guest_visible.
> 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>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  target/ppc/mmu-radix64.c |   39 +++++++++++++++++++++------------------
>  1 file changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
> index 7ce37cb778db..0d3922537c4c 100644
> --- a/target/ppc/mmu-radix64.c
> +++ b/target/ppc/mmu-radix64.c
> @@ -274,7 +274,7 @@ 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 guest_visible)
>  {
>      int fault_cause = 0;
>      hwaddr pte_addr;
> @@ -289,14 +289,15 @@ static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu, int rwx,
>          if (pde_addr) { /* address being translated was that of a guest pde */
>              fault_cause |= DSISR_PRTABLE_FAULT;
>          }
> -        if (cause_excp) {
> +        if (guest_visible) {
>              ppc_radix64_raise_hsi(cpu, rwx, eaddr, g_raddr, fault_cause);
>          }
>          return 1;
>      }
>  
> -    /* Update Reference and Change Bits */
> -    ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, h_prot);
> +    if (guest_visible) {
> +        ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, h_prot);
> +    }
>  
>      return 0;
>  }
> @@ -305,7 +306,7 @@ 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 guest_visible)
>  {
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
> @@ -319,7 +320,7 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx,
>      size = 1ULL << ((pate.dw1 & PATE1_R_PRTS) + 12);
>      if (offset >= size) {
>          /* offset exceeds size of the process table */
> -        if (cause_excp) {
> +        if (guest_visible) {
>              ppc_radix64_raise_si(cpu, rwx, eaddr, DSISR_NOPTE);
>          }
>          return 1;
> @@ -340,7 +341,7 @@ 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);
> +                                                 guest_visible);
>          if (ret) {
>              return ret;
>          }
> @@ -360,7 +361,7 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx,
>                                      &fault_cause, &pte_addr);
>          if (ret) {
>              /* No valid PTE */
> -            if (cause_excp) {
> +            if (guest_visible) {
>                  ppc_radix64_raise_si(cpu, rwx, eaddr, fault_cause);
>              }
>              return ret;
> @@ -380,7 +381,7 @@ 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);
> +                                                     guest_visible);
>              if (ret) {
>                  return ret;
>              }
> @@ -389,7 +390,7 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx,
>                                           &nls, g_page_size, &pte, &fault_cause);
>              if (ret) {
>                  /* No valid pte */
> -                if (cause_excp) {
> +                if (guest_visible) {
>                      ppc_radix64_raise_si(cpu, rwx, eaddr, fault_cause);
>                  }
>                  return ret;
> @@ -406,13 +407,15 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx,
>  
>      if (ppc_radix64_check_prot(cpu, rwx, pte, &fault_cause, g_prot, false)) {
>          /* Access denied due to protection */
> -        if (cause_excp) {
> +        if (guest_visible) {
>              ppc_radix64_raise_si(cpu, rwx, eaddr, fault_cause);
>          }
>          return 1;
>      }
>  
> -    ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, g_prot);
> +    if (guest_visible) {
> +        ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, g_prot);
> +    }
>  
>      return 0;
>  }
> @@ -437,7 +440,7 @@ 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 guest_visible)
>  {
>      CPUPPCState *env = &cpu->env;
>      uint64_t lpid, pid;
> @@ -447,7 +450,7 @@ static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx,
>  
>      /* Virtual Mode Access - get the fully qualified address */
>      if (!ppc_radix64_get_fully_qualified_addr(&cpu->env, eaddr, &lpid, &pid)) {
> -        if (cause_excp) {
> +        if (guest_visible) {
>              ppc_radix64_raise_segi(cpu, rwx, eaddr);
>          }
>          return 1;
> @@ -460,13 +463,13 @@ static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx,
>          vhc->get_pate(cpu->vhyp, &pate);
>      } else {
>          if (!ppc64_v3_get_pate(cpu, lpid, &pate)) {
> -            if (cause_excp) {
> +            if (guest_visible) {
>                  ppc_radix64_raise_si(cpu, rwx, eaddr, DSISR_NOPTE);
>              }
>              return 1;
>          }
>          if (!validate_pate(cpu, lpid, &pate)) {
> -            if (cause_excp) {
> +            if (guest_visible) {
>                  ppc_radix64_raise_si(cpu, rwx, eaddr, DSISR_R_BADCONFIG);
>              }
>              return 1;
> @@ -487,7 +490,7 @@ 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, guest_visible);
>          if (ret) {
>              return ret;
>          }
> @@ -510,7 +513,7 @@ 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, guest_visible);
>              if (ret) {
>                  return ret;
>              }
> 

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 576 bytes --]

On Thu, May 14, 2020 at 12:56:42AM +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.

Applied to ppc-for-5.1, thanks.

> 
> Changes since v1:
> - fix build break in patch 3
> - introduce guest_visible argument in patch 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 v2 0/6] target/ppc: Various clean-up and fixes for radix64

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 565 bytes --]

On Thu, 14 May 2020 16:52:49 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, May 14, 2020 at 12:56:42AM +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.
> 
> Applied to ppc-for-5.1, thanks.
> 

Could you please refresh your ppc-for-5.1 branch on github ?

> > 
> > Changes since v1:
> > - fix build break in patch 3
> > - introduce guest_visible argument in patch 6
> > 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 726 bytes --]

On Fri, May 15, 2020 at 08:58:07AM +0200, Greg Kurz wrote:
> On Thu, 14 May 2020 16:52:49 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, May 14, 2020 at 12:56:42AM +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.
> > 
> > Applied to ppc-for-5.1, thanks.
> > 
> 
> Could you please refresh your ppc-for-5.1 branch on github ?

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