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

[Greg Kurz]

On Mon, 11 May 2020 11:43:48 +1000
David Gibson <address@hidden> 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 <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.

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

pgpXIlpSowg9M.pgp
Description: OpenPGP digital signature