Re: [Qemu-ppc] [QEMU-ppc for-2.10][PATCH V4 5/6] target/ppc: Implement ISA V3.00 radix page fault handler

[David Gibson]

On Tue, Apr 18, 2017 at 04:45:29PM +1000, Suraj Jitindar Singh wrote:
> On Tue, 2017-04-18 at 15:51 +1000, David Gibson wrote:
> > On Thu, Apr 13, 2017 at 04:02:39PM +1000, Suraj Jitindar Singh
> wrote:
[snip]
> > > +static bool ppc_radix64_check_prot(PowerPCCPU *cpu, int rwx,
> > > uint64_t pte,
> > > +                                   int *fault_cause, int *prot)
> > > +{
> > > +    CPUPPCState *env = &cpu->env;
> > > +    const int need_prot[] = { PAGE_READ, PAGE_WRITE, PAGE_EXEC };
> > > +
> > > +    /* Check Page Attributes (pte58:59) */
> > > +    if (((pte & R_PTE_ATT) == R_PTE_ATT_G) && (rwx == 2)) {
> > I think you just want (pte & R_PTE_ATT_G) here.  I don't know what
> > the
> > other attribute is for, but I'm guessing its presence won't mean
> > execution of guarded storage beconmes allowed.
> 
> So the four possible combinations of these two bits each map to a
> distinct hash-like WIMG value in a rather non-obvious way. It happens
> that the R_PTE_ATT_G value is the only one which leads to the guard bit
> being set.
> 
> I'm not sure which combinations we can even get in tcg, but accoring to
> the ISA what I have implemented is the correct behaviour (it states
> that this value in the PTE_ATT bits results in no-execute storage).

Ah... right.  So now I partially understand what the "decode" stuff
was about in the previous version.

So the logic is correct.  However it's kind of confusing to anyone
familiar with previous MMUs, who will see a constant named ..._G and
assume it's simply a Guard bit, independent of those around it.

<looks at the V3 architecture docs...>

I think the way to go here is to make new constants for the various
attribute values based on the names used in the architecture, rather
than the older WIMG names.  So, say

        R_PTE_ATT_NORMAL
        R_PTE_ATT_SAO
        R_PTE_ATT_IO
        R_PTE_ATT_TOLERANT_IO

If we do need a new-attribute-to-wimg conversion function at some
point we can do that, but for now we can just use the logic from this
patch, but translated to the new names.

> 
> > 
> > > 
> > > +        /* Guarded Storage */
> > > +        *fault_cause |= SRR1_NOEXEC_GUARD;
> > > +        return true;
> > > +    }
> > > +
> > > +    /* Determine permissions allowed by Encoded Access Authority
> > > */
> > > +    if ((pte & R_PTE_EAA_PRIV) && msr_pr) { /* Insufficient
> > > Privilege */
> > > +        *prot = 0;
> > > +    } else if (msr_pr || (pte & R_PTE_EAA_PRIV)) {
> > > +        *prot = ppc_radix64_get_prot_eaa(pte);
> > > +    } else { /* !msr_pr && !(pte & R_PTE_EAA_PRIV) */
> > > +        *prot = ppc_radix64_get_prot_eaa(pte);
> > > +        *prot &= ppc_radix64_get_prot_amr(cpu); /* Least combined
> > > permissions */
> > > +    }
> > > +
> > > +    /* Check if requested access type is allowed */
> > > +    if (need_prot[rwx] & ~(*prot)) { /* Page Protected for that
> > > Access */
> > > +        *fault_cause |= DSISR_PROTFAULT;
> > > +        return true;
> > > +    }
> > > +
> > > +    return false;
> > > +}
> > > +
> > > +static void ppc_radix64_set_rc(PowerPCCPU *cpu, int rwx, uint64_t
> > > pte,
> > > +                               hwaddr pte_addr, int *prot)
> > > +{
> > > +    CPUState *cs = CPU(cpu);
> > > +    uint64_t npte;
> > > +
> > > +    npte = pte | R_PTE_R; /* Always set reference bit */
> > > +
> > > +    if (rwx == 1) { /* Store/Write */
> > > +        npte |= R_PTE_C; /* Set change bit */
> > > +    } else {
> > > +        /*
> > > +         * Treat the page as read-only for now, so that a later
> > > write
> > > +         * will pass through this function again to set the C bit.
> > > +         */
> > > +        *prot &= ~PAGE_WRITE;
> > > +    }
> > > +
> > > +    if (pte ^ npte) { /* If pte has changed then write it back */
> > > +        stq_phys(cs->as, pte_addr, npte);
> > > +    }
> > > +}
> > > +
> > > +static uint64_t ppc_radix64_walk_tree(PowerPCCPU *cpu, int rwx,
> > > vaddr eaddr,
> > > +                                      uint64_t base_addr, uint64_t
> > > nls,
> > > +                                      hwaddr *raddr, int *psize,
> > > +                                      int *fault_cause, int *prot,
> > > +                                      hwaddr *pte_addr)
> > > +{
> > > +    CPUState *cs = CPU(cpu);
> > > +    uint64_t index, pde;
> > > +
> > > +    if (nls < 5) { /* Directory maps less than 2**5 entries */
> > > +        *fault_cause |= DSISR_R_BADCONFIG;
> > > +        return 0;
> > > +    }
> > > +
> > > +    /* 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 */
> > > +        *fault_cause |= DSISR_NOPTE;
> > > +        return 0;
> > > +    }
> > > +
> > > +    *psize -= nls;
> > > +
> > > +    /* Check if Leaf Entry -> Page Table Entry -> Stop the Search
> > > */
> > > +    if (pde & R_PTE_LEAF) {
> > > +        uint64_t rpn = pde & R_PTE_RPN;
> > > +        uint64_t mask = (1UL << *psize) - 1;
> > > +
> > > +        if (ppc_radix64_check_prot(cpu, rwx, pde, fault_cause,
> > > prot)) {
> > > +            return 0; /* Protection Denied Access */
> > > +        }
> > > +
> > > +        /* 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, rwx, eaddr, pde & R_PDE_NLB,
> > > +                                 pde & R_PDE_NLS, raddr, psize,
> > > +                                 fault_cause, prot, pte_addr);
> > > +}
> > > +
> > > +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 =
> > > +        PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
> > > +    hwaddr raddr, pte_addr;
> > > +    uint64_t lpid = 0, pid = 0, offset, size, patbe, prtbe0, pte;
> > > +    int page_size, prot, fault_cause = 0;
> > > +
> > > +    assert((rwx == 0) || (rwx == 1) || (rwx == 2));
> > > +    assert(!msr_hv); /* For now there is no Radix PowerNV Support
> > > */
> > > +    assert(cpu->vhyp);
> > > +    assert(ppc64_use_proc_tbl(cpu));
> > > +
> > > +    /* Real Mode Access */
> > > +    if (((rwx == 2) && (msr_ir == 0)) || ((rwx != 2) && (msr_dr ==
> > > 0))) {
> > > +        /* In real mode top 4 effective addr bits (mostly) ignored
> > > */
> > > +        raddr = eaddr & 0x0FFFFFFFFFFFFFFFULL;
> > > +
> > > +        tlb_set_page(cs, eaddr & TARGET_PAGE_MASK, raddr &
> > > TARGET_PAGE_MASK,
> > > +                     PAGE_READ | PAGE_WRITE | PAGE_EXEC, mmu_idx,
> > > +                     TARGET_PAGE_SIZE);
> > > +        return 0;
> > > +    }
> > > +
> > > +    /* Virtual Mode Access - get the fully qualified address */
> > > +    if (!ppc_radix64_get_fully_qualified_addr(env, eaddr, &lpid,
> > > &pid)) {
> > > +        ppc_radix64_raise_segi(cpu, rwx, eaddr);
> > > +        return 1;
> > > +    }
> > > +
> > > +    /* Get Process Table */
> > > +    patbe = vhc->get_patbe(cpu->vhyp);
> > > +
> > > +    /* Index Process Table by PID to Find Corresponding Process
> > > Table Entry */
> > > +    offset = pid * sizeof(struct prtb_entry);
> > > +    size = 1ULL << ((patbe & PATBE1_R_PRTS) + 12);
> > > +    if (offset >= size) {
> > > +        /* offset exceeds size of the process table */
> > > +        ppc_radix64_raise_si(cpu, rwx, eaddr, DSISR_NOPTE);
> > > +        return 1;
> > > +    }
> > > +    prtbe0 = ldq_phys(cs->as, (patbe & PATBE1_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, rwx, eaddr & R_EADDR_MASK,
> > > +                                prtbe0 & PRTBE_R_RPDB, prtbe0 &
> > > PRTBE_R_RPDS,
> > > +                                &raddr, &page_size, &fault_cause,
> > > &prot,
> > > +                                &pte_addr);
> > > +    if (!pte) {
> > > +        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);
> > > +    return 1;
> > > +}
> > > diff --git a/target/ppc/mmu-radix64.h b/target/ppc/mmu-radix64.h
> > > new file mode 100644
> > > index 0000000..adc9e22
> > > --- /dev/null
> > > +++ b/target/ppc/mmu-radix64.h
> > > @@ -0,0 +1,68 @@
> > > +#ifndef MMU_RADIX64_H
> > > +#define MMU_RADIX64_H
> > > +
> > > +#ifndef CONFIG_USER_ONLY
> > > +
> > > +/* Radix Quadrants */
> > > +#define R_EADDR_MASK            0x3FFFFFFFFFFFFFFF
> > > +#define R_EADDR_QUADRANT        0xC000000000000000
> > > +#define R_EADDR_QUADRANT0       0x0000000000000000
> > > +#define R_EADDR_QUADRANT1       0x4000000000000000
> > > +#define R_EADDR_QUADRANT2       0x8000000000000000
> > > +#define R_EADDR_QUADRANT3       0xC000000000000000
> > > +
> > > +/* Radix Partition Table Entry Fields */
> > > +#define PATBE1_R_PRTB           0x0FFFFFFFFFFFF000
> > > +#define PATBE1_R_PRTS           0x000000000000001F
> > > +
> > > +/* Radix Process Table Entry Fields */
> > > +#define PRTBE_R_GET_RTS(rts)    (((rts >> 58) & 0x18) | ((rts >>
> > > 5) & 0x7)) + 31
> > > +#define PRTBE_R_RPDB            0x0FFFFFFFFFFFFF00
> > > +#define PRTBE_R_RPDS            0x000000000000001F
> > > +
> > > +/* Radix Page Directory/Table Entry Fields */
> > > +#define R_PTE_VALID             0x8000000000000000
> > > +#define R_PTE_LEAF              0x4000000000000000
> > > +#define R_PTE_SW0               0x2000000000000000
> > > +#define R_PTE_RPN               0x01FFFFFFFFFFF000
> > > +#define R_PTE_SW1               0x0000000000000E00
> > > +#define R_GET_SW(sw)            (((sw >> 58) & 0x8) | ((sw >> 9) &
> > > 0x7))
> > > +#define R_PTE_R                 0x0000000000000100
> > > +#define R_PTE_C                 0x0000000000000080
> > > +#define R_PTE_ATT               0x0000000000000030
> > > +#define R_PTE_ATT_G             0x0000000000000020
> > > +#define R_PTE_EAA_PRIV          0x0000000000000008
> > > +#define R_PTE_EAA_R             0x0000000000000004
> > > +#define R_PTE_EAA_RW            0x0000000000000002
> > > +#define R_PTE_EAA_X             0x0000000000000001
> > > +#define R_PDE_NLB               PRTBE_R_RPDB
> > > +#define R_PDE_NLS               PRTBE_R_RPDS
> > > +
> > > +#ifdef TARGET_PPC64
> > > +
> > > +int ppc_radix64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr, int
> > > rwx,
> > > +                                 int mmu_idx);
> > > +
> > > +static inline int ppc_radix64_get_prot_eaa(uint64_t pte)
> > > +{
> > > +    return (pte & R_PTE_EAA_R ? PAGE_READ : 0) |
> > > +           (pte & R_PTE_EAA_RW ? PAGE_READ | PAGE_WRITE : 0) |
> > > +           (pte & R_PTE_EAA_X ? PAGE_EXEC : 0);
> > > +}
> > > +
> > > +static inline int ppc_radix64_get_prot_amr(PowerPCCPU *cpu)
> > > +{
> > > +    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 */
> > > +
> > > +    return (amr & 0x2 ? 0 : PAGE_WRITE) | /* Access denied if bit
> > > is set */
> > > +           (amr & 0x1 ? 0 : PAGE_READ) |
> > > +           (iamr & 0x1 ? 0 : PAGE_EXEC);
> > > +}
> > > +
> > > +#endif /* TARGET_PPC64 */
> > > +
> > > +#endif /* CONFIG_USER_ONLY */
> > > +
> > > +#endif /* MMU_RADIX64_H */
> 

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