Re: [Qemu-devel] [RFC PATCH 02/34] tcg+qom: QOMify core CPU defintions

[Peter Crosthwaite]

On Mon, May 11, 2015 at 1:18 PM, Richard Henderson <address@hidden> wrote:
> On 05/11/2015 03:24 AM, Paolo Bonzini wrote:
>>
>>
>> On 11/05/2015 12:18, Andreas Färber wrote:
>>>>> +    int (*cpu_mmu_index)(CPUState *cpu);
>>>>> +    void (*cpu_get_tb_cpu_state)(CPUState *cpu,
>>>>> +                                 void *pc, /* target_long * */
>>>>> +                                 void *cs_base, /* target_long */
>>>>> +                                 int *flags);
>>>>> +    void (*gen_intermediate_code)(void *env, struct TranslationBlock 
>>>>> *tb);
>>>>> +    void (*gen_intermediate_code_pc)(void *env, struct TranslationBlock 
>>>>> *tb);
>>>>> +    void (*restore_state_to_opc)(void *env, struct TranslationBlock *tb,
>>>>> +                                 int pc_pos);
>>>>> +    void (*tlb_fill)(CPUState *cs, uint64_t addr, int is_write, int 
>>>>> mmu_idx,
>>>>> +                     uintptr_t retaddr);
>>>>>  } CPUClass;
>>>>>
>>>>>  #ifdef HOST_WORDS_BIGENDIAN
>>> [snip]
>>>
>>> Paolo had objected to this when I tried it. The counter-suggestion was
>>> something about reworking how the cputlb code is built per target -
>>> please check the archives.
>>
>> Right.  My point was that these functions are not polymorphic.  Each
>> call to these should know exactly which function to call.
>
> That's some major surgery you have planned there.
>

Just tried this. It's ... rather hard.

> Especially the path via the qemu_ld/st helpers, where function to call is
> currently hard-coded into the tcg backend.
>

Yes. I actually seemed to make decent progress on the multi-compile
approach until I hit this brick wall:

tcg/i386/tcg-target.c:1131:17: error: ‘helper_ret_ldub_mmu’ undeclared
here (not in a function)
     [MO_UB]   = helper_ret_ldub_mmu,

In my multi-compile approach helper_*[ld|st]* needs to be renamed
per-arch for the multiple compiled cputlb.o. Hence I have no symbol
with the unqualified name. But even if I do solve my namespacing
problem, I still have an ambiguity of which cputlb.o provided
helper_*[ld|st]* to use from the TCG backend. This would mean all
those APIs would have to virtualised. The big question for Paolo, is
what complete set of APIs defines the common-code/non-common-code
boundary? tlb_fill does seem to do the job nicely and looking at the
architecture implementations it's not a super fast path (falling back
to a page table faulter).

Somewhere along the call path from the qemu_st_helpers uses
(tcg/i386/tcg-target.c) through to tlb_fill there has to be a
virtualised function unless I am missing something?

> I think that this is a decent step forward, modulo the conditionals along the
> use paths.  I think we ought to clean up all of the translators to the new QOM
> hooks.
>

So the conditional can be ditched by having the CPU base class
defaulting the hook to the globally defined function. Then arches can
be brought online one-by-one.

> I can't imagine that most of these hooks are called frequently enough that the
> indirect call really matters.  Certainly gen_intermediate_code need not use 
> the
> hook when initializing the mmu_idx in the DisasContext.
>

Ok so the solution to this is to opt-out of the hook via a re-#define
when we have a target-specific cpu.h handy. This will actually mean no
change to single-arch builds but multi-arch will use the hook from
core code only.

> That said, I'd approve of a goal to arrange for the correct qemu_ld/st helpers
> to be called, and a direct call to the proper tlb_fill.  But, one step at a 
> time...
>

I don't know what this means exactly. tlb_fill is called by functions
that are linked to common code (TCG backends) so I don't see a non
virtualized solution. Is this refactoring to move tlb_fill?

Regards,
Peter

>
> r~
>