Re: [PATCH 4/7] target/ppc: Introduce ppc_radix64_xlate() for Radix tree translation

[Greg Kurz]

On Mon, 30 Mar 2020 17:34:40 +0200
Cédric Le Goater <address@hidden> wrote:

> >>> +        /* No valid pte or access denied due to protection */
> >>> +        if (cause_excp) {
> >>> +            ppc_radix64_raise_si(cpu, rwx, eaddr, fault_cause);
> >>> +        }
> >>> +        return 1;
> >>> +    }
> >>> +
> >>> +    ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, g_prot);
> >>> +
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx,
> >>> +                             uint64_t lpid, uint64_t pid, bool 
> >>> relocation,
> >>> +                             hwaddr *raddr, int *psizep, int *protp,
> >>> +                             bool cause_excp)
> >>> +{
> >>> +    ppc_v3_pate_t pate;
> >>> +    int psize, prot;
> >>> +    hwaddr g_raddr;
> >>> +
> >>> +    *psizep = INT_MAX;
> >>> +    *protp = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> >>> +
> >>> +    /* Get Process Table */
> >>> +    if (cpu->vhyp && lpid == 0) {
> >>
> >> Current code doesn't check lpid == 0. Not sure to see what it's for...
> > 
> > cpu->vhyp means a pseries machine, lpid == 0 means accessing quadrant3, 
> > so it's the kernel.
> 
> Sorry. I misread that. It would pid == 0 for the kernel. 
> 

Yes.

> So yes, the test cpu->vhyp && lpid == 0 might be a bit overkill, given 
> that lpid is always 0 when running under a QEMU pseries machine.
> 

That's my thinking as well.

> 
> C.
> 
> > 
> >> especially env->spr[SPR_LPIDR] is always 0 with pseries machine types
> >> AFAICT... is it even possible to have lpid != 0 here ?
> > 
> > When under PowerNV, SPR_LPIDR can be set, but not under pseries.
> > 
> > C.
> > 
> >>
> >>
> >> Rest LGTM.
> >>

Just a few nits below...

> >>> +        PPCVirtualHypervisorClass *vhc;
> >>> +        vhc = PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
> >>> +        vhc->get_pate(cpu->vhyp, &pate);
> >>> +    } else {
> >>> +        if (!ppc64_v3_get_pate(cpu, lpid, &pate)) {
> >>> +            if (cause_excp) {
> >>> +                ppc_radix64_raise_si(cpu, rwx, eaddr, DSISR_NOPTE);
> >>> +            }
> >>> +            return 1;
> >>> +        }
> >>> +        if (!validate_pate(cpu, lpid, &pate)) {
> >>> +            if (cause_excp) {
> >>> +                ppc_radix64_raise_si(cpu, rwx, eaddr, DSISR_R_BADCONFIG);
> >>> +            }
> >>> +            return 1;
> >>> +        }
> >>> +        /* We don't support guest mode yet */
> >>> +        if (lpid != 0) {
> >>> +            error_report("PowerNV guest support Unimplemented");
> >>> +            exit(1);
> >>> +        }
> >>> +    }
> >>> +
> >>> +    /*
> >>> +     * Perform process-scoped translation if relocation enabled.
> >>> +     *
> >>> +     * - Translates an effective address to a host real address in
> >>> +     *   quadrants 0 and 3 when HV=1.
> >>> +     */
> >>> +    if (relocation) {
> >>> +        int ret = ppc_radix64_process_scoped_xlate(cpu, rwx, eaddr, 
> >>> lpid, pid,
> >>> +                                                   pate, &g_raddr, &prot,
> >>> +                                                   &psize, cause_excp);
> >>> +        if (ret) {
> >>> +            return ret;
> >>> +        }
> >>> +        *psizep = MIN(*psizep, psize);
> >>> +        *protp &= prot;
> >>> +    } else {
> >>> +        g_raddr = eaddr & R_EADDR_MASK;
> >>> +    }
> >>> +
> >>> +    *raddr = g_raddr;
> >>> +    return 0;
> >>> +}
> >>> +
> >>>  int ppc_radix64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr, int rwx,
> >>>                                   int mmu_idx)
> >>>  {
> >>>      CPUState *cs = CPU(cpu);
> >>>      CPUPPCState *env = &cpu->env;
> >>> -    PPCVirtualHypervisorClass *vhc;
> >>> -    hwaddr raddr, pte_addr;
> >>> -    uint64_t lpid = 0, pid = 0, offset, size, prtbe0, pte;
> >>> -    int page_size, prot, fault_cause = 0;
> >>> -    ppc_v3_pate_t pate;
> >>> +    uint64_t lpid = 0, pid = 0;
> >>> +    int page_size, prot;
> >>>      bool relocation;
> >>> +    hwaddr raddr;
> >>>  
> >>>      assert(!(msr_hv && cpu->vhyp));
> >>>      assert((rwx == 0) || (rwx == 1) || (rwx == 2));
> >>> @@ -268,48 +370,11 @@ int ppc_radix64_handle_mmu_fault(PowerPCCPU *cpu, 
> >>> vaddr eaddr, int rwx,
> >>>          return 1;
> >>>      }
> >>>  
> >>> -    /* Get Process Table */
> >>> -    if (cpu->vhyp) {
> >>> -        vhc = PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
> >>> -        vhc->get_pate(cpu->vhyp, &pate);
> >>> -    } else {
> >>> -        if (!ppc64_v3_get_pate(cpu, lpid, &pate)) {
> >>> -            ppc_radix64_raise_si(cpu, rwx, eaddr, DSISR_NOPTE);
> >>> -            return 1;
> >>> -        }
> >>> -        if (!validate_pate(cpu, lpid, &pate)) {
> >>> -            ppc_radix64_raise_si(cpu, rwx, eaddr, DSISR_R_BADCONFIG);
> >>> -        }
> >>> -        /* We don't support guest mode yet */
> >>> -        if (lpid != 0) {
> >>> -            error_report("PowerNV guest support Unimplemented");
> >>> -            exit(1);
> >>> -       }
> >>> -    }
> >>> -
> >>> -    /* Index Process Table by PID to Find Corresponding Process Table 
> >>> Entry */
> >>> -    offset = pid * sizeof(struct prtb_entry);
> >>> -    size = 1ULL << ((pate.dw1 & PATE1_R_PRTS) + 12);
> >>> -    if (offset >= size) {
> >>> -        /* offset exceeds size of the process table */
> >>> -        ppc_radix64_raise_si(cpu, rwx, eaddr, DSISR_NOPTE);
> >>> +    /* Translate eaddr to raddr (where raddr is addr qemu needs for 
> >>> access) */
> >>> +    if (ppc_radix64_xlate(cpu, eaddr, rwx, lpid, pid, relocation, &raddr,
> >>> +                          &page_size, &prot, 1)) {

... passe true instead of 1 and...

> >>>          return 1;
> >>>      }
> >>> -    prtbe0 = ldq_phys(cs->as, (pate.dw1 & PATE1_R_PRTB) + offset);
> >>> -
> >>> -    /* Walk Radix Tree from Process Table Entry to Convert EA to RA */
> >>> -    page_size = PRTBE_R_GET_RTS(prtbe0);
> >>> -    pte = ppc_radix64_walk_tree(cpu, eaddr & R_EADDR_MASK,
> >>> -                                prtbe0 & PRTBE_R_RPDB, prtbe0 & 
> >>> PRTBE_R_RPDS,
> >>> -                                &raddr, &page_size, &fault_cause, 
> >>> &pte_addr);
> >>> -    if (!pte || ppc_radix64_check_prot(cpu, rwx, pte, &fault_cause, 
> >>> &prot)) {
> >>> -        /* Couldn't get pte or access denied due to protection */
> >>> -        ppc_radix64_raise_si(cpu, rwx, eaddr, fault_cause);
> >>> -        return 1;
> >>> -    }
> >>> -
> >>> -    /* Update Reference and Change Bits */
> >>> -    ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, &prot);
> >>>  
> >>>      tlb_set_page(cs, eaddr & TARGET_PAGE_MASK, raddr & TARGET_PAGE_MASK,
> >>>                   prot, mmu_idx, 1UL << page_size);
> >>> @@ -318,16 +383,13 @@ int ppc_radix64_handle_mmu_fault(PowerPCCPU *cpu, 
> >>> vaddr eaddr, int rwx,
> >>>  
> >>>  hwaddr ppc_radix64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong 
> >>> eaddr)
> >>>  {
> >>> -    CPUState *cs = CPU(cpu);
> >>>      CPUPPCState *env = &cpu->env;
> >>> -    PPCVirtualHypervisorClass *vhc;
> >>> -    hwaddr raddr, pte_addr;
> >>> -    uint64_t lpid = 0, pid = 0, offset, size, prtbe0, pte;
> >>> -    int page_size, fault_cause = 0;
> >>> -    ppc_v3_pate_t pate;
> >>> +    uint64_t lpid = 0, pid = 0;
> >>> +    int psize, prot;
> >>> +    hwaddr raddr;
> >>>  
> >>>      /* Handle Real Mode */
> >>> -    if (msr_dr == 0) {
> >>> +    if ((msr_dr == 0) && (msr_hv || (cpu->vhyp && lpid == 0))) {
> >>>          /* In real mode top 4 effective addr bits (mostly) ignored */
> >>>          return eaddr & 0x0FFFFFFFFFFFFFFFULL;
> >>>      }
> >>> @@ -337,39 +399,8 @@ hwaddr ppc_radix64_get_phys_page_debug(PowerPCCPU 
> >>> *cpu, target_ulong eaddr)
> >>>          return -1;
> >>>      }
> >>>  
> >>> -    /* Get Process Table */
> >>> -    if (cpu->vhyp) {
> >>> -        vhc = PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
> >>> -        vhc->get_pate(cpu->vhyp, &pate);
> >>> -    } else {
> >>> -        if (!ppc64_v3_get_pate(cpu, lpid, &pate)) {
> >>> -            return -1;
> >>> -        }
> >>> -        if (!validate_pate(cpu, lpid, &pate)) {
> >>> -            return -1;
> >>> -        }
> >>> -        /* We don't support guest mode yet */
> >>> -        if (lpid != 0) {
> >>> -            error_report("PowerNV guest support Unimplemented");
> >>> -            exit(1);
> >>> -       }
> >>> -    }
> >>> -
> >>> -    /* Index Process Table by PID to Find Corresponding Process Table 
> >>> Entry */
> >>> -    offset = pid * sizeof(struct prtb_entry);
> >>> -    size = 1ULL << ((pate.dw1 & PATE1_R_PRTS) + 12);
> >>> -    if (offset >= size) {
> >>> -        /* offset exceeds size of the process table */
> >>> -        return -1;
> >>> -    }
> >>> -    prtbe0 = ldq_phys(cs->as, (pate.dw1 & PATE1_R_PRTB) + offset);
> >>> -
> >>> -    /* Walk Radix Tree from Process Table Entry to Convert EA to RA */
> >>> -    page_size = PRTBE_R_GET_RTS(prtbe0);
> >>> -    pte = ppc_radix64_walk_tree(cpu, eaddr & R_EADDR_MASK,
> >>> -                                prtbe0 & PRTBE_R_RPDB, prtbe0 & 
> >>> PRTBE_R_RPDS,
> >>> -                                &raddr, &page_size, &fault_cause, 
> >>> &pte_addr);
> >>> -    if (!pte) {
> >>> +    if (ppc_radix64_xlate(cpu, eaddr, 0, lpid, pid, msr_dr, &raddr, 
> >>> &psize,
> >>> +                          &prot, 0)) {

... false here since ppc_radix64_xlate() takes a bool argument.

> >>>          return -1;
> >>>      }
> >>>  
> >>
> > 
>