RE: [PATCH] Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards

[Taylor Simpson <tsimpson@quicinc.com>]

Responses inline ...

Thanks,
Taylor


-----Original Message-----
From: Richard Henderson <richard.henderson@linaro.org>
Sent: Wednesday, November 20, 2019 2:07 AM
To: Taylor Simpson <tsimpson@quicinc.com>; laurent@vivier.eu; riku.voipio@iki.fi; qemu-devel@nongnu.org
Subject: Re: [PATCH] Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards

-------------------------------------------------------------------------
CAUTION: This email originated from outside of the organization.
-------------------------------------------------------------------------

On 11/20/19 6:15 AM, Taylor Simpson wrote:
>> +const char * const hexagon_prednames[] = {
>> +  "p0 ", "p1 ", "p2 ", "p3 "
>> +};
>
> Inter-string spacing probably belongs to the format not the name.
> [Taylor] Could you elaborate?  Should I put spacing  after the commas?

"p0" not "p0 ".  Typo on my part for "inter-"; sorry for the confusion.

>> +static inline target_ulong hack_stack_ptrs(CPUHexagonState *env,
>> +                                           target_ulong addr) {
>> +    target_ulong stack_start = env->stack_start;
>> +    target_ulong stack_size = 0x10000;
>> +    target_ulong stack_adjust = env->stack_adjust;
>> +
>> +    if (stack_start + 0x1000 >= addr && addr >= (stack_start - stack_size)) {
>> +        return addr - stack_adjust;
>> +    }
>> +    return addr;
>> +}
>
> An explanation would be welcome here.
> [Taylor]  I will add a comment.  One of the main debugging techniques is to use "-d cpu" and compare against LLDB output when single stepping.  However, the target and qemu put the stacks in different locations.  This is used to compensate so the diff is cleaner.

Ug.  I understand why you want this, but... ug.

[Taylor] Not sure what to say - I guess there's a fine line between elegance and mayhem.


>> +static void hexagon_dump(CPUHexagonState *env, FILE *f) {
>> +    static target_ulong last_pc;
>> +    int i;
>> +
>> +    /*
>> +     * When comparing with LLDB, it doesn't step through single-cycle
>> +     * hardware loops the same way.  So, we just skip them here
>> +     */
>> +    if (env->gpr[HEX_REG_PC] == last_pc) {
>> +        return;
>> +    }
>
> This seems mis-placed.
> [Taylor] Hexagon has hardware controlled loops, so we can have a single packet that executes multiple times.  We don't want the dump to print every time.

Certainly I do.  If I'm single-stepping, I want every packet present.  Just as if this were written as a traditional loop, with a branch back to the single packet in the loop body.

Also, you cannot expect a static variable to work in multi-threaded mode, and you cannot expect a __thread variable to work in single-threaded round-robin mode.

[Taylor] This is another place where it's important to match the output of LLDB for a clean diff.  In fact, LLDB uses hardware single step, and that will step to the end of a single-cycle hardware loop.  Also, the technique of comparing with LLDB only works for single threaded examples, so I'm not worried about the thread-safety of this code.

[Taylor] To clarify, this is for a rare case when a hardware loop body is a single packet.  Here's an example.  It's the portion of memset that gets called when the number of bytes is small
  400404:       10 40 02 60     60024010 {      loop0(0x40040c,r2)
  400408:       08 c0 02 10     1002c008        p0 = cmp.eq(r2,#0); if (p0.new) jump:nt 0x400414 <memset+0x34> }
  40040c:       01 81 03 a1     a1038101 {      memb(r3+#1) = r1
  400410:       10 c1 03 ab     ab03c110        memb(r3++#2) = r1 }  :endloop0
The loop0 instruction sets the start address and the iteration count.  The :endloop0 marks the end of the loop and tells the hardware to decrement the counter and go back to the start if it's not zero.  So, the loop at 0x40040c-0x400410 is a single packet.  In this case the hardware single step will execute the entire loop.  Finally, if the loop has more than one packet, the single stepping will go through each iteration as you describe.

>> +static void decode_packet(CPUHexagonState *env, DisasContext *ctx) {
>> +    size4u_t words[4];
>> +    int i;
>> +    /* Brute force way to make sure current PC is set */
>> +    tcg_gen_movi_tl(hex_gpr[HEX_REG_PC], ctx->base.pc_next);
>
> What's this for?
> [Taylor] Honestly, I'm not sure.  If I remove it, nothing works - not even "hello world".

Weird.  I would have expected that the update you do within hexagon_tr_tb_stop would be sufficient.  I guess I'll have to peek at it.

I presume your minimal test case is a bit of hand-crafted assembly that issues a write syscall and exit?

[Taylor] The minimal test case is
#define SYS_write 64
#define SYS_exit_group           94
#define SYS_exit                 93

#define FD_STDOUT                1

.typestr,@object
.section.rodata
str:
.string"Hello!\n"
.sizestr, 8

.text
.global _start
_start:
r6 = #SYS_write
r0 = #FD_STDOUT
r1 = ##str
r2 = #7
trap0(#1)

r6 = #SYS_exit_group
trap0(#1)

r6 = #SYS_exit
r0 = #0
trap0(#1)
Without that code, it prints "Hello!" twice.  With the full set of patches, I get segfaults ☹

>> +
>> +    for (i = 0; i < 4; i++) {
>> +        words[i] = cpu_ldl_code(env, ctx->base.pc_next + i * sizeof(size4u_t));
>> +    }
>
> And this?
> [Taylor] It's reading from the instruction memory.  The switch statement below uses it.

I thought packets are variable length and ended by a bit set.  Therefore why the fixed iteration to 4?  Also...


[Taylor] The maximum size of a packet is 4 words, so I go ahead and read that much.  Once the packet is decoded, I set ctx->base.pc_next appropriately.  Note that most of the instructions in the switch add 4, but one of them adds 8.

>
>> +    /*
>> +     * Brute force just enough to get the first program to execute.
>> +     */
>> +    switch (words[0]) {

... you only use 1 word, but you read 4.

>> +static void hexagon_tr_init_disas_context(DisasContextBase *dcbase,
>> +                                          CPUState *cs) {
>> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
>> +
>> +    ctx->mem_idx = ctx->base.tb->flags & TB_FLAGS_MMU_MASK;
>
> Since you're not initializing this in cpu_get_tb_cpu_state, you might
> as well just use
>
>     ctx->mem_idx = MMU_USER_IDX;
> [Taylor] Should I be initialize this in cpu_get_bt_cpu_state?

Not until there's something more interesting to put there, when you implement system state.  The constant initialization should be fine.


r~