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

[David Gibson]

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 <address@hidden>

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

signature.asc
Description: PGP signature