Re: [PATCH] tests/tcg/multiarch: add vma-pthread.c

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

On 12/23/22 04:02, Ilya Leoshkevich wrote:
> +static void *thread_write(void *arg)
> +{
> +    struct context *ctx = arg;
> +    struct timespec *ts;
> +    size_t i, j;
> +    int ret;
> +
> +    for (i = 0; ctx->mutator_count; i++) {
> +        j = (i & PAGE_IDX_MASK) | PAGE_IDX_W_MASK;
> +        /* Write directly. */
> +        memcpy(&ctx->ptr[j * ctx->pagesize], nop_func, sizeof(nop_func));
> +        /* Write using a syscall. */
> +        ts = (struct timespec *)(&ctx->ptr[(j + 1) * ctx->pagesize] -
> +                                 sizeof(struct timespec));
> +        ret = clock_gettime(CLOCK_REALTIME, ts);
> +        assert(ret == 0);

With the 3 issues that you pointed out in the other email, this is the only remaining 
failure.  This happens because of two issues:

(1) When checking for writability, we actually check for both read+write:

#define VERIFY_WRITE (PAGE_READ | PAGE_WRITE)

This is very likely a bug, but we'd need to audit all uses to find out where we might 
really want read+write.

> +static void *thread_mutate(void *arg)
> +{
> +    size_t i, start_idx, end_idx, page_idx, tmp;
> +    struct context *ctx = arg;
> +    unsigned int seed;
> +    int prot, ret;
> +
> +    seed = (unsigned int)time(NULL);
> +    for (i = 0; i < 50000; i++) {
> +        start_idx = rand_r(&seed) & PAGE_IDX_MASK;
> +        end_idx = rand_r(&seed) & PAGE_IDX_MASK;
> +        if (start_idx > end_idx) {
> +            tmp = start_idx;
> +            start_idx = end_idx;
> +            end_idx = tmp;
> +        }
> +        prot = rand_r(&seed) & (PROT_READ | PROT_WRITE | PROT_EXEC);
> +        for (page_idx = start_idx & REGION_MASK; page_idx <= end_idx;
> +             page_idx += PAGES_PER_REGION) {
> +            if (page_idx & PAGE_IDX_R_MASK) {
> +                prot |= PROT_READ;
> +            }
> +            if (page_idx & PAGE_IDX_W_MASK) {
> +                prot |= PROT_WRITE;
> +            }
> +            if (page_idx & PAGE_IDX_X_MASK) {
> +                prot |= PROT_EXEC;
> +            }

... and here we can wind up with write-only pages.

(2) Certain hardware, like x86_64, does not support write-only pages -- writable implies 
readable -- so the testcase runs on some hardware.  It is a bug that we don't model this 
as well.


r~