Re: [Qemu-arm] [Qemu-devel] [PATCH v11 24/29] target/arm: [tcg, a64] Port to translate_insn

[Lluís Vilanova]

Richard Henderson writes:

> On 07/07/2017 01:18 AM, Lluís Vilanova wrote:
>>>> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
>>>> index 9c870f6d07..586a01a2de 100644
>>>> --- a/target/arm/translate-a64.c
>>>> +++ b/target/arm/translate-a64.c
>>>> @@ -11244,6 +11244,9 @@ static void 
>>>> aarch64_trblock_init_disas_context(DisasContextBase *dcbase,
dc-> is_ldex = false;
dc-> ss_same_el = (arm_debug_target_el(env) == dc->current_el);
>>>> +    dc->next_page_start =
>>>> +        (dc->base.pc_first & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE;
>> 
>>> I think a better solution for a fixed-length ISA is to adjust max_insns. 
>>> Perhaps
>>> the init_disas_context hook should be able to modify it?
>> 
>> ARM has the thumb instructions, so it really isn't a fixed-length ISA.

> From the filename above we're talking about AArch64 here.
> Whcih is most definitely a fixed-width ISA.


> That said, even for AArch32 we know by the TB flags whether or not we're going
> to be generating arm or thumb code.  I think that these hooks will allow arm 
> and
> thumb mode to finally be split apart cleanly, instead of the tangle that they
> are now.

> I see arm's gen_intermediate_code eventually looking like

>   const TranslatorOps *ops = &arm_translator_ops;
>   if (ARM_TBFLAG_THUMB(tb->flags)) {
>     ops = &thumb_translator_ops;
>   }
> #ifdef TARGET_AARCH64
>   if (ARM_TBFLAG_AARCH64_STATE(tb->flags)) {
>     ops = &aarch64_translator_ops;
>   }
> #endif
>   translator_loop(ops, &dc.base, cpu, tb);

> There would certainly be some amount of shared code, but it would also allow
> quite a few of the existing dc->thumb checks to be eliminated.

Does this really need to be addressed on this series?


>>> And, while I'm thinking of it -- why is the init_globals hook separate? 
>>> There's
>>> nothing in between the two hook calls, and the more modern target front ends
>>> won't need it.
>> 
>> You mean merging init_disas_context and init_globals? I wanted to keep
>> semantically different code on separate hooks, but I can pull the 
>> init_globals
>> code into init_disas_context (hoping that as targets get modernized, they 
>> won't
>> initialize any global there).

> I suppose you can leave the two hooks separate for now.  It doesn't hurt, and
> it's kind of a reminder of things that need cleaning up.

Well, I've sent a (too rushed) v12 that features the merge. Since I'll have to
send a v13, I can split them again if you want.


> I do wonder if we should provide a generic empty hook, so that a target that
> does not need a particular hook need not define an empty function.  It could
> just put e.g. "translator_noop" into the structure.  Ok, maybe a better name
> than that...

I'm not really sure it's worth the effort. The original version trated NULLs as
a "skip this operation" (as Emilio suggests to revive), but after discussing the
approach it was decided that defining an empty function was not too much effort,
so that's what I went for.

I can restore the NULL approach or add a default empty hook implementation
(translator_ignored_op?) if there's a strong feeling to change it.


Cheers,
  Lluis