Re: [RFC PATCH v3 31/34] Hexagon (target/hexagon) translation

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

On 8/30/20 12:37 PM, Taylor Simpson wrote:
> I'm actually checking two conditions here.
> 1) packet crossing a page boundary
> 2) reading too many words without finding the end of the packet.
> I guess it would be better to separate them.
> 
> What is the correct behavior for the second case?  Should we return an error code from here and have the higher level code generate the invalid packet exception?

I would return an error code.

In fact, I would also pass in the max number of words to read:

static int read_packet_words(CPUHexagonState *env,
                             DisasContext *ctx,
                             uint32_t words[PACKET_WORDS_MAX],
                             int max_words)
{
   // stuff
   return !found_end ? 0 : nwords;
}


Then, in translate_packet,


    uint32_t words[PACKET_WORDS_MAX];
    int nwords = read_packet_words(env, ctx, words,
                                   PACKET_WORDS_MAX);

    if (nwords == 0) {
        // raise exception
        return;
    }

    decode_and_translate_packet(env, ctx, words, nwords);

    /* If we're going to try for another packet... */
    if (ctx->base.is_jmp == DISAS_NEXT &&
        ctx->base.num_insns < ctx->base.num_insns) {
        /*
         * Remember the end of the page containing the
         * first packet.  Note that the first packet
         * is allowed to span two pages, so this is not
         * necessarily the same as the end of the page
         * containing ctx->base.pc_start.
         */
        if (ctx->base.num_insns == 1) {
            ctx->page_end
                = TARGET_PAGE_ALIGN(ctx->base.pc_next);
        }

        /*
         * If there are not PACKET_WORDS_MAX remaining on
         * the page, check to see if a full packet remains.
         * If not, split the TB so that the packet that
         * crosses the page begins the next TB.
         */
        target_long left = ctx->page_end - ctx->base.pc_next;
        tcg_debug_assert(left >= 0);
        if (left == 0
            || (left < PACKET_WORDS_MAX * 4 &&
                !read_packet_words(env, ctx, words, left / 4)) {
            ctx->base.is_jmp = DISAS_TOO_MANY;
        }
    }


The reason for all this is to properly capture the behaviour of instruction
execution vs SIGSEGV.

First, during translate we do not want to read from the next page unless
absolutely necessary.  Doing so could raise SIGSEGV before it would be
appropriate, e.g. because the TB should have branched away (or raised SIGFPE,
or anything else) before getting that far.

Second, when dispatching a TB, we check the 1 or 2 pages that the TB occupies
for validity.  If the second page is invalid, we raise SIGSEGV without
executing the TB at all.  Which makes it appear as if the SIGSEGV happened at
the first insn of the TB.  Which is correct if and only if the first insn is
the one that did cross the page.

>> Surely you don't need to actually set PC for every PC?
> 
> What do other targets do?

If you have a pc-relative instruction, e.g. x86_64's

  lea  offset(%rip), %rax

then you just use the known immediate for %rip:

  tcg_gen_movi_tl(cpu_reg[eax], ctx->base.pc_next + offset);

Normally, PC is only valid when explicitly returning to the cpu loop
(tcg_gen_exit_tb, static exception), for indirect branching
(tcg_gen_lookup_and_goto_ptr), or after dynamic exception unwinding
(restore_state_to_opc).

When using goto_tb, we can get away with *assuming* static state, because the
values get baked into the link to a specific next-TB.  That's why the general
form is

  tcg_gen_goto_tb(n);
  gen_set_pc_im(s, dest);
  tcg_gen_exit_tb(s->base.tb, n);

The first time we cross link N, the link is unset, which causes the goto_tb to
continue to the next tcg opcode.  Which then sets all of the static state that
has been assumed (often, as here, just the pc).  We then exit, telling the
cpu_loop to examine cpu state, locate the next TB, and fill in link N from the
current TB.

The second time we cross link N, the link is set, which causes the goto_tb to
continue immediately to the next TB.  We do not execute the store to PC, as it
is implied by next_tb->pc.


r~