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~
