Re: [Qemu-devel] [PATCH v3 05/21] RISC-V CPU Helpers

[Richard Henderson <richard.henderson@linaro.org>]

On 01/10/2018 06:21 PM, Michael Clark wrote:
> +    int ptshift = (levels - 1) * ptidxbits;
> +    int i;
> +    for (i = 0; i < levels; i++, ptshift -= ptidxbits) {
> +        target_ulong idx = (addr >> (PGSHIFT + ptshift)) &
> +                           ((1 << ptidxbits) - 1);
> +
> +        /* check that physical address of PTE is legal */
> +        target_ulong pte_addr = base + idx * ptesize;
> +        target_ulong pte = ldq_phys(cs->as, pte_addr);
> +        target_ulong ppn = pte >> PTE_PPN_SHIFT;
> +
> +        if (PTE_TABLE(pte)) { /* next level of page table */
> +            base = ppn << PGSHIFT;
> +        } else if ((pte & PTE_U) ? (mode == PRV_S) && !sum : !(mode == PRV_S)) {
> +            break;
> +        } else if (!(pte & PTE_V) || (!(pte & PTE_R) && (pte & PTE_W))) {
> +            break;

It might be clearer to return TRANSLATION_FAILED directly here.

> +        } else if (access_type == MMU_INST_FETCH ? !(pte & PTE_X) :
> +                  access_type == MMU_DATA_LOAD ?  !(pte & PTE_R) &&
> +                  !(mxr && (pte & PTE_X)) : !((pte & PTE_R) && (pte & PTE_W))) {
> +            break;

It might be clearer to pre-compute the access required and the access granted
by this page table level.  E.g. before the loop

  access_requested = (access_type == MMU_INST_FETCH ? PTE_X
                      : access_type == MMU_DATA_LOAD ? PTE_R : PTE_W);

and then here

  access_granted = pte & (PTE_X | PTE_R | PTE_W);
  if (mxr && (pte & PTE_X)) {
    access_granted |= PTE_R;
  }
  if ((access_requested & access_granted) == 0) {
    return TRANSLATION_FAILED;
  }

> +            /* set accessed and possibly dirty bits.
> +               we only put it in the TLB if it has the right stuff */
> +            stq_phys(cs->as, pte_addr, ldq_phys(cs->as, pte_addr) | PTE_A |
> +                    ((access_type == MMU_DATA_STORE) * PTE_D));

Surely you only want to do this if the bits in question are not already set.

You want

    set_bits = PTE_A + (access_type == MMU_DATA_STORE) * PTE_D);
    if ((pte & set_bits) != set_bits) {
        stq_phys(...);
        pte |= set_bits;  /* see below */
    }

> +            /* we do not give all prots indicated by the PTE
> +             * this is because future accesses need to do things like set the
> +             * dirty bit on the PTE
> +             *
> +             * at this point, we assume that protection checks have occurred */
> +            if (mode == PRV_S) {
> +                if ((pte & PTE_X) && access_type == MMU_INST_FETCH) {
> +                    *prot |= PAGE_EXEC;
> +                } else if ((pte & PTE_W) && access_type == MMU_DATA_STORE) {
> +                    *prot |= PAGE_WRITE;
> +                } else if ((pte & PTE_R) && access_type == MMU_DATA_LOAD) {
> +                    *prot |= PAGE_READ;
> +                } else {
> +                    g_assert_not_reached();
> +                }
> +            } else {
> +                if ((pte & PTE_X) && access_type == MMU_INST_FETCH) {
> +                    *prot |= PAGE_EXEC;
> +                } else if ((pte & PTE_W) && access_type == MMU_DATA_STORE) {
> +                    *prot |= PAGE_WRITE;
> +                } else if ((pte & PTE_R) && access_type == MMU_DATA_LOAD) {
> +                    *prot |= PAGE_READ;
> +                } else {
> +                    g_assert_not_reached();
> +                }
> +            }

This is wrong.  First, it isn't taking MXR into account.  Second, you don't
want to only grant what is required by the access, but also what is allowable.
Otherwise we will keep coming back to this routine any different kind of
access.  Third, what has PRV_S got to do with it?

You want

  /* If the dirty bit is not yet set, remove write permission from
     the page so that we come back here to set it.  */
  if ((pte & PTE_D) == 0) {
    access_granted &= ~PTE_W;
  }

  *prot = (access_granted & PTE_X ? PAGE_EXEC : 0)
        | (access_granted & PTE_R ? PAGE_READ : 0)
        | (access_granted & PTE_W ? PAGE_WRITE : 0);


Did I miss the check for misaligned superpages (step 6 in 4.3.2
Virtual Address Translation Process)?


> +/*
> + * Handle writes to CSRs and any resulting special behavior
> + *
> + * Adapted from Spike's processor_t::set_csr
> + */
> +inline void csr_write_helper(CPURISCVState *env, target_ulong val_to_write,
> +        target_ulong csrno)

Again, drop the inline.  (This is a huge function anyway, why would you want that?)

> +inline target_ulong csr_read_helper(CPURISCVState *env, target_ulong csrno)

Likewise.

> +target_ulong helper_sret(CPURISCVState *env, target_ulong cpu_pc_deb)
> +{
> +    if (!(env->priv >= PRV_S)) {
> +        helper_raise_exception(env, RISCV_EXCP_ILLEGAL_INST);
> +    }

Again, I strongly suspect that you need to be using an unwinding exception
here, and elsewhere within this file.  Otherwise the PC against which the
exception is reported will be wrong.


r~