Re: [Qemu-devel] [PATCH 3/4] target: [tcg] Add generic translation framework

[Lluís Vilanova]

Richard Henderson writes:

> On 07/15/2016 09:42 PM, Lluís Vilanova wrote:
>> Signed-off-by: Lluís Vilanova <address@hidden>
>> ---
>> include/exec/translate-all_template.h |   58 ++++++++++++
>> include/qom/cpu.h                     |   21 ++++
>> translate-all_template.h              |  160 
>> +++++++++++++++++++++++++++++++++
>> 3 files changed, 239 insertions(+)
>> create mode 100644 include/exec/translate-all_template.h
>> create mode 100644 translate-all_template.h
>> 
>> diff --git a/include/exec/translate-all_template.h 
>> b/include/exec/translate-all_template.h
>> new file mode 100644
>> index 0000000..9e0c361
>> --- /dev/null
>> +++ b/include/exec/translate-all_template.h
>> @@ -0,0 +1,58 @@
>> +/*
>> + * Generic intermediate code generation.
>> + *
>> + * Copyright (C) 2016 Lluís Vilanova <address@hidden>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef EXEC__TRANSLATE_ALL_TEMPLATE_H
>> +#define EXEC__TRANSLATE_ALL_TEMPLATE_H
>> +
>> +/*
>> + * Include this header from a target-specific file, and add a
>> + *
>> + *     DisasContextBase base;
>> + *
>> + * member in your target-specific DisasContext.
>> + */
>> +
>> +
>> +#include "exec/exec-all.h"
>> +
>> +
>> +/**
>> + * DisasJumpType:
>> + * @DJ_NEXT: Next instruction in program order
>> + * @DJ_TOO_MANY: Too many instructions executed
>> + * @DJ_TARGET: Start of target-specific conditions
>> + *
>> + * What instruction to disassemble next.
>> + */
>> +typedef enum DisasJumpType
>> +{
>> +    DJ_NEXT,
>> +    DJ_TOO_MANY,
>> +    DJ_TARGET,
>> +} DisasJumpType;

> I think you might as well add the common cases: exit tb via exception, exit 
> via
> goto_tb, exit via indirect jump (pc updated), exit for state change (pc not
> updated).

> See the set used for alpha.

Do you mean the "DISAS_*" values in "exec-all.h"? I looked into it, but could
not fully understand when they are supposed to be used (seem inconsistent across
targets), so I decided to defer this to the target.


>> +void gen_intermediate_code(CPUState *cpu, struct TranslationBlock *tb)
>> +{
>> +    CPUArchState *env = cpu->env_ptr;
>> +    DisasContext dc1, *dc = &dc1;
>> +    int num_insns;
>> +    int max_insns;
>> +
>> +    /* Initialize DisasContext */
>> +    dc->base.tb = tb;
>> +    dc->base.singlestep_enabled = cpu->singlestep_enabled;
>> +    dc->base.pc_first = tb->pc;
>> +    dc->base.pc_next = dc->base.pc_first;
>> +    dc->base.jmp_type = DJ_NEXT;
>> +    gen_intermediate_code_target_init_disas_context(dc, env);
>> +
>> +    /* Target-specific globals */
>> +    gen_intermediate_code_target_init_globals(dc, env);
>> +
>> +    /* Instruction counting */
>> +    num_insns = 0;
>> +    max_insns = dc->base.tb->cflags & CF_COUNT_MASK;
>> +    if (max_insns == 0) {
>> +        max_insns = CF_COUNT_MASK;
>> +    }
>> +    if (max_insns > TCG_MAX_INSNS) {
>> +        max_insns = TCG_MAX_INSNS;
>> +    }

> I've started adding the singlestep check here, outside the loop, setting
> max_insns to 1.

That makes sense, and could eliminate one field in DisasContextBase.


>> +
>> +    /* Start translating */
>> +    gen_tb_start(dc->base.tb);
>> +
>> +    while (true) {
>> +        CPUBreakpoint *bp;
>> +
>> +        tcg_gen_insn_start(dc->base.pc_next, dc->cc_op);

> You've probably discovered that this will have to be its own hook.

Yes, sorry about not fixing it before sending.


>> +        num_insns++;
>> +
>> +        /* Pass breakpoint hits to target for further processing */
>> +        bp = NULL;
>> +        do {
>> +            bp = cpu_breakpoint_get(cpu, dc->base.pc_next, bp);
>> +            if (unlikely(bp)) {
>> +                if (gen_intermediate_code_target_breakpoint_hit(dc, env, 
>> bp)) {
>> +                    goto done_generating;
>> +                }
>> +            }
>> +        } while (bp != NULL);

> Why would you need to loop here?

cpu_breakpoint_get() just returns breakpoints matching a PC, and delays other
checks (like flags) to target code. That's why cpu_breakpoint_get() is now used
as a looping construct (thus the change in qlist to continue a previous
foreach).


Thanks,
  Lluis