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

[Richard Henderson]

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.


>> +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.

>> +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?

>> +
>> +    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...

> 
>> +    /*
>> +     * 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~