Re: [Qemu-devel] [PATCH v11 04/29] target: [tcg] Add generic translation framework
On 4 July 2017 at 23:31, Richard Henderson wrote: > On 07/04/2017 09:14 AM, Peter Maydell wrote: >> I kind of like not having CPUState* in DisasContext, because >> it enforces the rule that you can't read from fields of >> it inside the target translate.c code without jumping through >> a hoop (ie copying the info from CPUState->foo to >> DisasContext->foo). That then acts as a useful flag in code >> review (or when writing the code) to confirm that foo really >> is constant for the life of the simulation (or to recommend >> using a TB flag instead). > > > I don't see how the spelling "cpu" vs "dc->cpu" really affects that. If you put it in dc->cpu then everywhere that gets the dc gets the cpu, unavoidably (which is why I'm suggesting it would be nicer not to do that). If you don't put it in dc then you can structure your translate code so that pretty much all of it gets the dc but not the cpu. target/arm/translate-a64.c does this, for instance. thanks -- PMM
Re: [Qemu-devel] [PATCH v11 04/29] target: [tcg] Add generic translation framework
On 07/04/2017 09:14 AM, Peter Maydell wrote: On 4 July 2017 at 19:59, Lluís Vilanova wrote: Richard Henderson writes: Any reason not to stuff the cpu pointer into the DisasContextBase instead of passing it around separately? None, really. I'll move it from DisasContext (in targets where it's present) into DisasContextBase, and use that one everywhere. I kind of like not having CPUState* in DisasContext, because it enforces the rule that you can't read from fields of it inside the target translate.c code without jumping through a hoop (ie copying the info from CPUState->foo to DisasContext->foo). That then acts as a useful flag in code review (or when writing the code) to confirm that foo really is constant for the life of the simulation (or to recommend using a TB flag instead). I don't see how the spelling "cpu" vs "dc->cpu" really affects that. More practically, I don't see that "cpu" will actually be used by most of those hooks. But because of things like cpu->some_target_feature, it's kind of hard to predict. r~
Re: [Qemu-devel] [PATCH v11 04/29] target: [tcg] Add generic translation framework
On 4 July 2017 at 19:59, Lluís Vilanova wrote: > Richard Henderson writes: > >> On 06/28/2017 05:32 AM, Lluís Vilanova wrote: >>> +void (*init_disas_context)(DisasContextBase *db, CPUState *cpu); >>> +void (*init_globals)(DisasContextBase *db, CPUState *cpu); >>> +void (*tb_start)(DisasContextBase *db, CPUState *cpu); >>> +void (*insn_start)(DisasContextBase *db, CPUState *cpu); >>> +BreakpointCheckType (*breakpoint_check)(DisasContextBase *db, CPUState >>> *cpu, >>> +const CPUBreakpoint *bp); >>> +target_ulong (*translate_insn)(DisasContextBase *db, CPUState *cpu); >>> +void (*tb_stop)(DisasContextBase *db, CPUState *cpu); >>> +void (*disas_log)(const DisasContextBase *db, CPUState *cpu); > >> Any reason not to stuff the cpu pointer into the DisasContextBase instead of >> passing it around separately? > > None, really. I'll move it from DisasContext (in targets where it's present) > into DisasContextBase, and use that one everywhere. I kind of like not having CPUState* in DisasContext, because it enforces the rule that you can't read from fields of it inside the target translate.c code without jumping through a hoop (ie copying the info from CPUState->foo to DisasContext->foo). That then acts as a useful flag in code review (or when writing the code) to confirm that foo really is constant for the life of the simulation (or to recommend using a TB flag instead). thanks -- PMM
Re: [Qemu-devel] [PATCH v11 04/29] target: [tcg] Add generic translation framework
Richard Henderson writes: > On 06/28/2017 05:32 AM, Lluís Vilanova wrote: >> +void (*init_disas_context)(DisasContextBase *db, CPUState *cpu); >> +void (*init_globals)(DisasContextBase *db, CPUState *cpu); >> +void (*tb_start)(DisasContextBase *db, CPUState *cpu); >> +void (*insn_start)(DisasContextBase *db, CPUState *cpu); >> +BreakpointCheckType (*breakpoint_check)(DisasContextBase *db, CPUState >> *cpu, >> +const CPUBreakpoint *bp); >> +target_ulong (*translate_insn)(DisasContextBase *db, CPUState *cpu); >> +void (*tb_stop)(DisasContextBase *db, CPUState *cpu); >> +void (*disas_log)(const DisasContextBase *db, CPUState *cpu); > Any reason not to stuff the cpu pointer into the DisasContextBase instead of > passing it around separately? None, really. I'll move it from DisasContext (in targets where it's present) into DisasContextBase, and use that one everywhere. > Otherwise, > Reviewed-by: Richard Henderson Thanks, Lluis
Re: [Qemu-devel] [PATCH v11 04/29] target: [tcg] Add generic translation framework
On 06/28/2017 05:32 AM, Lluís Vilanova wrote: +void (*init_disas_context)(DisasContextBase *db, CPUState *cpu); +void (*init_globals)(DisasContextBase *db, CPUState *cpu); +void (*tb_start)(DisasContextBase *db, CPUState *cpu); +void (*insn_start)(DisasContextBase *db, CPUState *cpu); +BreakpointCheckType (*breakpoint_check)(DisasContextBase *db, CPUState *cpu, +const CPUBreakpoint *bp); +target_ulong (*translate_insn)(DisasContextBase *db, CPUState *cpu); +void (*tb_stop)(DisasContextBase *db, CPUState *cpu); +void (*disas_log)(const DisasContextBase *db, CPUState *cpu); Any reason not to stuff the cpu pointer into the DisasContextBase instead of passing it around separately? Otherwise, Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH v11 04/29] target: [tcg] Add generic translation framework
On 06/29/2017 05:02 PM, Emilio G. Cota wrote: +void translate_block(const TranslatorOps *ops, DisasContextBase *db, + CPUState *cpu, TranslationBlock *tb); I'd rather avoid "block" here. Some alternatives: - tb_translate() - translate_tb() - translate() - translator_gen() - translator_loop() I like translator_loop. r~
Re: [Qemu-devel] [PATCH v11 04/29] target: [tcg] Add generic translation framework
On Wed, Jun 28, 2017 at 15:32:48 +0300, Lluís Vilanova wrote:
> Signed-off-by: Lluís Vilanova
> ---
(snip)
> /**
> * DisasJumpType:
> * @DISAS_NEXT: Next instruction in program order.
> @@ -33,6 +65,78 @@ typedef enum DisasJumpType {
> DISAS_TARGET_9,
> DISAS_TARGET_10,
> DISAS_TARGET_11,
> +DISAS_TARGET_12,
> +DISAS_TARGET_13,
> +DISAS_TARGET_14,
> } DisasJumpType;
These belong in patch 3.
E.
Re: [Qemu-devel] [PATCH v11 04/29] target: [tcg] Add generic translation framework
On Wed, Jun 28, 2017 at 15:32:48 +0300, Lluís Vilanova wrote:
> Signed-off-by: Lluís Vilanova
> ---
> accel/tcg/Makefile.objs |1
> accel/tcg/translator.c| 153
> +
> include/exec/gen-icount.h |2 -
> include/exec/translator.h | 104 +++
(snip)
> +/**
> + * BreakpointCheckType:
> + * @BC_MISS: No hit
> + * @BC_HIT_INSN: Hit, but continue translating TB
> + * @BC_HIT_TB: Hit, stop translating TB
> + *
> + * How to react to a breakpoint. A hit means no more breakpoints will be
> checked
> + * for the current instruction.
> + *
> + * Not all breakpoints associated to an address are necessarily raised by
> + * targets (e.g., due to conditions encoded in their flags), so tey can
> decide
s/tey/they/
> + * that a breakpoint missed the address (@BP_MISS).
> + */
> +typedef enum BreakpointCheckType {
> +BC_MISS,
> +BC_HIT_INSN,
> +BC_HIT_TB,
> +} BreakpointCheckType;
> +
> /**
> * DisasJumpType:
> * @DISAS_NEXT: Next instruction in program order.
> @@ -33,6 +65,78 @@ typedef enum DisasJumpType {
> DISAS_TARGET_9,
> DISAS_TARGET_10,
> DISAS_TARGET_11,
> +DISAS_TARGET_12,
> +DISAS_TARGET_13,
> +DISAS_TARGET_14,
> } DisasJumpType;
>
> +/**
> + * DisasContextBase:
> + * @tb: Translation block for this disassembly.
> + * @pc_first: Address of first guest instruction in this TB.
> + * @pc_next: Address of next guest instruction in this TB (current during
> + * disassembly).
> + * @is_jmp: What instruction to disassemble next.
> + * @num_insns: Number of translated instructions (including current).
> + * @singlestep_enabled: "Hardware" single stepping enabled.
> + *
> + * Architecture-agnostic disassembly context.
> + */
> +typedef struct DisasContextBase {
> +TranslationBlock *tb;
> +target_ulong pc_first;
> +target_ulong pc_next;
> +DisasJumpType is_jmp;
> +unsigned int num_insns;
> +bool singlestep_enabled;
> +} DisasContextBase;
> +
> +/**
> + * TranslatorOps:
> + * @init_disas_context: Initialize a DisasContext struct (DisasContextBase
> has
> + * already been initialized).
> + * @init_globals: Initialize global variables.
> + * @tb_start: Start translating a new TB.
> + * @insn_start: Start translating a new instruction.
> + * @breakpoint_check: Check if a breakpoint did hit. When called, the
> breakpoint
> + *has already been checked to match the PC.
> + * @disas_insn: Disassemble one instruction an return the PC for the next
s/disas_insn/translate_insn/
s/an return/and return/
> + * one. Can set db->is_jmp to DJ_TARGET or above to stop
s/DJ_TARGET/DISAS_TARGET/
> + * translation.
> + * @tb_stop: Stop translating a TB.
> + * @disas_flags: Get flags argument for log_target_disas().
s/disas_flags/disas_log/
> + *
> + * Target-specific operations for the generic translator loop.
> + */
> +typedef struct TranslatorOps {
> +void (*init_disas_context)(DisasContextBase *db, CPUState *cpu);
> +void (*init_globals)(DisasContextBase *db, CPUState *cpu);
> +void (*tb_start)(DisasContextBase *db, CPUState *cpu);
> +void (*insn_start)(DisasContextBase *db, CPUState *cpu);
> +BreakpointCheckType (*breakpoint_check)(DisasContextBase *db, CPUState
> *cpu,
> +const CPUBreakpoint *bp);
> +target_ulong (*translate_insn)(DisasContextBase *db, CPUState *cpu);
> +void (*tb_stop)(DisasContextBase *db, CPUState *cpu);
> +void (*disas_log)(const DisasContextBase *db, CPUState *cpu);
> +} TranslatorOps;
> +
> +/**
> + * translate_block:
> + * @ops: Target-specific operations.
> + * @db: Disassembly context.
> + * @cpu: Target vCPU.
> + * @tb: Translation block.
> + *
> + * Generic translator loop.
> + *
> + * Translation will stop in the following cases (in order):
> + * - When set by #TranslatorOps::insn_start.
> + * - When set by #TranslatorOps::translate_insn.
> + * - When the TCG operation buffer is full.
> + * - When single-stepping is enabled (system-wide or on the current vCPU).
> + * - When too many instructions have been translated.
> + */
> +void translate_block(const TranslatorOps *ops, DisasContextBase *db,
> + CPUState *cpu, TranslationBlock *tb);
I'd rather avoid "block" here. Some alternatives:
- tb_translate()
- translate_tb()
- translate()
- translator_gen()
- translator_loop()
E.
