Re: [Qemu-devel] [PATCH v1 2/4] s390x/tcg: Introduce probe_read_access()

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

On 8/21/19 2:22 AM, David Hildenbrand wrote:
> +/*
> + * Make sure the read access is permitted and TLB entries are created. In
> + * very rare cases it might happen that the actual accesses might need
> + * new MMU translations. If the page tables were changed in between, we
> + * might still trigger a fault. However, this seems to barely happen, so we
> + * can ignore this for now.
> + */
> +void probe_read_access(CPUS390XState *env, uint64_t addr, uint64_t len,
> +                       uintptr_t ra)
> +{
> +#ifdef CONFIG_USER_ONLY
> +    if (!guest_addr_valid(addr) || !guest_addr_valid(addr + len - 1) ||
> +        page_check_range(addr, len, PAGE_READ) < 0) {
> +        s390_program_interrupt(env, PGM_ADDRESSING, ILEN_AUTO, ra);
> +    }
> +#else
> +    while (len) {
> +        const uint64_t pagelen = -(addr | -TARGET_PAGE_MASK);
> +        const uint64_t curlen = MIN(pagelen, len);
> +
> +        cpu_ldub_data_ra(env, addr, ra);
> +        addr = wrap_address(env, addr + curlen);
> +        len -= curlen;
> +    }
> +#endif
> +}

I don't think this is really the right approach, precisely because of the
comment above.

I think we should

(1) Modify the generic probe_write to return the host address,
    akin to tlb_vaddr_to_host except it *will* fault.

(2) Create a generic version of probe_write for CONFIG_USER_ONLY,
    much like the one you have done for target/s390x.

(3) Create generic version of probe_read that does the same.

(4) Rewrite fast_memset and fast_memmove to fetch all of the host
    addresses before doing any modifications.  The functions are
    currently written as if len can be very large, handling any
    number of pages.  Except that's not true.  While there are
    several kinds of users apart from MVC, two pages are sufficient
    for all users.

    Well, should be.  We would need to adjust do_mvcl to limit the
    operation to TARGET_PAGE_SIZE (CC=3, cpu-determined number of
    bytes moved without reaching end of first operand).
    Which is probably a good idea anyway.  System mode should not
    spend forever executing one instruction, as it would if you
    pass in a 64-bit length from MVCLE.


Something like

static void fast_memmove_idx(CPUS390XState *env, uint64_t dst,
                             uint64_t src, uint32_t len,
                             int dst_idx, int src_idx,
                             uintptr_t ra)
{
    void *dst1, *dst2, *dst3;
    void *src1, *src2, *src3;
    uint32_t len1, len2, lenr;
    uint64_t dstr, srcr;

    if (unlikely(len == 0)) {
        return;
    }
    assert(len <= TARGET_PAGE_SIZE);

    dst1 = probe_write(env, dst, 1, dst_idx, ra);
    src1 = probe_read(env, src, 1, src_idx, ra);

    if (dst1 == NULL || src1 == NULL) {
        goto io_memmove;
    }

    /* Crop length so that neither SRC+LEN nor DST+LEN crosses a page. */
    len1 = adj_len_to_page(adj_len_to_page(len, src), dst);
    lenr = len - len1;

    if (likely(lenr == 0)) {
        memmove(dst1, src1, len);
        return;
    }

    /* Probe for the second page and range.  */
    dstr = dst + len1;
    srcr = src + len1;
    dst2 = probe_write(env, dstr, 1, dst_idx, ra);
    src2 = probe_read(env, srcr, 1, src_idx, ra);

    len2 = adj_len_to_page(adj_len_to_page(lenr, srcr), dstr);
    lenr -= len2;

    if (likely(lenr == 0)) {
        memmove(dst1, src1, len1);
        if (dst2 != NULL && src2 != NULL) {
            memmove(dst2, src2, len2);
            return;
        }
        dst = dstr;
        src = srcr;
        len = len2;
        goto io_memmove;
    }

    /*
     * There is a chance for a third page and range.  E.g.
     *   dst = 0xaaaff0, src = 0xbbbff8, len = 32
     * len1 = 8, bringing src to its page crossing, then
     * len2 = 8, bringing dst to its page crossing, then
     * lenr = 16, finishing the rest of the operation.
     */
    dstr += len2;
    srcr += len2;
    dst3 = probe_write(env, dstr, 1, dst_idx, ra);
    src3 = probe_read(env, srcr, 1, src_idx, ra);

    memmove(dst1, src1, len1);
    memmove(dst2, src2, len2);
    if (dst3 != NULL && src3 != NULL) {
        memmove(dst3, src3, lenr);
        return;
    }
    dst = dstr;
    src = srcr;
    len = lenr;

 io_memmove:
#ifdef CONFIG_USER_ONLY
    /*
     * There is no I/O space, so probe_{read,write} raise exceptions
     * for unmapped pages and never return NULL.
     */
    g_assert_not_reached();
#else
    TCGMemOpIdx oi_dst = make_memop_idx(MO_UB, dst_idx);
    TCGMemOpIdx oi_src = make_memop_idx(MO_UB, src_idx);
    do {
        uint8_t x = helper_ret_ldub_mmu(env, src, oi_src, ra);
        helper_ret_stb_mmu(env, dest, x, oi_dst, ra);
    } while (--len != 0);
#endif
}

static void fast_memmove(CPUS390XState *env, uint64_t dst, uint64_t src,
                         uint32_t len, uintptr_t ra)
{
    int mmu_idx = cpu_mmu_index(env, false);
    fast_memmove_idx(env, dst, src, len, mmu_idx, mmu_idx, ra);
}


r~