Re: [PATCH 5/7] target/ppc: Rework ppc_radix64_walk_tree() for partition-scoped translation

[Greg Kurz]

On Tue, 31 Mar 2020 11:10:20 +0200
Cédric Le Goater <address@hidden> wrote:

> On 3/30/20 7:00 PM, Greg Kurz wrote:
> > On Mon, 30 Mar 2020 11:49:44 +0200
> > Cédric Le Goater <address@hidden> wrote:
> > 
> >> The ppc_radix64_walk_tree() routine walks through the nested radix
> >> tables to look for a PTE.
> >>
> >> Split it two and introduce a new routine ppc_radix64_next_level()
> > 
> > Split it in two...
> > 
> >> which we will use for partition-scoped Radix translation when
> >> translating the process tree addresses.
> >>
> >> Signed-off-by: Suraj Jitindar Singh <address@hidden>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> >> ---
> >>  target/ppc/mmu-radix64.c | 50 ++++++++++++++++++++++++++--------------
> >>  1 file changed, 33 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
> >> index b4e6abcd2d35..136498111f60 100644
> >> --- a/target/ppc/mmu-radix64.c
> >> +++ b/target/ppc/mmu-radix64.c
> >> @@ -162,44 +162,60 @@ static void ppc_radix64_set_rc(PowerPCCPU *cpu, int 
> >> rwx, uint64_t pte,
> >>      }
> >>  }
> >>  
> >> -static uint64_t ppc_radix64_walk_tree(PowerPCCPU *cpu, vaddr eaddr,
> >> -                                      uint64_t base_addr, uint64_t nls,
> >> -                                      hwaddr *raddr, int *psize,
> >> -                                      int *fault_cause, hwaddr *pte_addr)
> >> +static uint64_t ppc_radix64_next_level(PowerPCCPU *cpu, vaddr eaddr,
> >> +                                       uint64_t *pte_addr, uint64_t *nls,
> >> +                                       int *psize, int *fault_cause)
> >>  {
> >>      CPUState *cs = CPU(cpu);
> >>      uint64_t index, pde;
> >>  
> >> -    if (nls < 5) { /* Directory maps less than 2**5 entries */
> >> +    if (*nls < 5) { /* Directory maps less than 2**5 entries */
> >>          *fault_cause |= DSISR_R_BADCONFIG;
> >>          return 0;
> >>      }
> >>  
> > 
> > I think this should stay in the caller...
> > 
> >>      /* Read page <directory/table> entry from guest address space */
> >> -    index = eaddr >> (*psize - nls); /* Shift */
> >> -    index &= ((1UL << nls) - 1); /* Mask */
> >> -    pde = ldq_phys(cs->as, base_addr + (index * sizeof(pde)));
> >> -    if (!(pde & R_PTE_VALID)) { /* Invalid Entry */
> >> +    pde = ldq_phys(cs->as, *pte_addr);
> >> +    if (!(pde & R_PTE_VALID)) {         /* Invalid Entry */
> >>          *fault_cause |= DSISR_NOPTE;
> >>          return 0;
> >>      }
> >>  
> >> -    *psize -= nls;
> >> +    *psize -= *nls;
> >> +    if (!(pde & R_PTE_LEAF)) { /* Prepare for next iteration */
> >> +        *nls = pde & R_PDE_NLS;
> >> +        index = eaddr >> (*psize - *nls);       /* Shift */
> >> +        index &= ((1UL << *nls) - 1);           /* Mask */
> >> +        *pte_addr = (pde & R_PDE_NLB) + (index * sizeof(pde));
> >> +    }
> >> +    return pde;
> >> +}
> >> +
> >> +static uint64_t ppc_radix64_walk_tree(PowerPCCPU *cpu, vaddr eaddr,
> >> +                                      uint64_t base_addr, uint64_t nls,
> >> +                                      hwaddr *raddr, int *psize,
> >> +                                      int *fault_cause, hwaddr *pte_addr)
> >> +{
> >> +    uint64_t index, pde;
> >> +
> >> +    index = eaddr >> (*psize - nls);    /* Shift */
> >> +    index &= ((1UL << nls) - 1);       /* Mask */
> >> +    *pte_addr = base_addr + (index * sizeof(pde));
> >> +    do {
> > 
> > ... here. So that we have a well established "bad config" path at
> > the root level, just like the current code has.
> > 
> > Since the ppc_radix64_walk_tree() now calls ppc_radix64_next_level()
> > in a loop instead of recursing, and since ppc_radix64_next_level()
> > returns the nls value for the next level, it really makes sense to
> > have this check in ppc_radix64_walk_tree() and maybe put an assert
> > in ppc_radix64_next_level().
> 
> ppc_radix64_next_level() also covers the needs of partition-scoped 
> translation which translates each process table address. See PATCH 7.
> 
> I rather not duplicate more code and leave it as it is.
>  

These patches change the behaviour of some existing paths in a
non-trivial way. I'll wait for the respin and try to review again.

> Thanks,
> 
> C.
> 
> > 
> >> +        pde = ppc_radix64_next_level(cpu, eaddr, pte_addr, &nls, psize,
> >> +                                     fault_cause);
> >> +    } while ((pde & R_PTE_VALID) && !(pde & R_PTE_LEAF));
> >>  
> >> -    /* Check if Leaf Entry -> Page Table Entry -> Stop the Search */
> >> -    if (pde & R_PTE_LEAF) {
> >> +    /* Did we find a valid leaf? */
> >> +    if ((pde & R_PTE_VALID) && (pde & R_PTE_LEAF)) {
> >>          uint64_t rpn = pde & R_PTE_RPN;
> >>          uint64_t mask = (1UL << *psize) - 1;
> >>  
> >>          /* Or high bits of rpn and low bits to ea to form whole real addr 
> >> */
> >>          *raddr = (rpn & ~mask) | (eaddr & mask);
> >> -        *pte_addr = base_addr + (index * sizeof(pde));
> >> -        return pde;
> >>      }
> >>  
> >> -    /* Next Level of Radix Tree */
> >> -    return ppc_radix64_walk_tree(cpu, eaddr, pde & R_PDE_NLB, pde & 
> >> R_PDE_NLS,
> >> -                                 raddr, psize, fault_cause, pte_addr);
> >> +    return pde;
> >>  }
> >>  
> >>  static bool validate_pate(PowerPCCPU *cpu, uint64_t lpid, ppc_v3_pate_t 
> >> *pate)
> > 
>