Re: [Qemu-devel] [PATCH qom-next 57/59] cpu: Introduce mandatory tlb_flush callback

[Blue Swirl <blauwirbel@gmail.com>]

On Thu, May 31, 2012 at 6:53 PM, Paul Brook <paul@codesourcery.com> wrote:
>> >> +void cpu_tlb_flush(CPUState *cpu, bool flush_global)
>> >> +{
>> >> +    CPUClass *cc = CPU_GET_CLASS(cpu);
>> >> +
>> >> +    g_assert(cc->tlb_flush != NULL);
>> >> +
>> >> +    cc->tlb_flush(cpu, flush_global);
>> >> +}
>> >
>> > This needs to be able to call tlb_flush() itself
>> > rather than having to have every single subclass of CPUState
>> > implement an identical tlb_flush method. You could do this
>> > if there was a CPU_GET_ENV()...
>>
>> Which is exactly the point: CPUState does not know about the
>> target-specific "env". And CPU_GET_ENV() is just plain wrong
>> conceptually because it adds yet another cpu.h dependency.
>
> Maybe so, but having every single taget implement its own copy of the exact
> same target independent wrapper seems even more wrong.
>
>> There's a separation between old code using env and new, clean code:
>> Just like Anthony doesn't want old concepts rewritten with the new type
>> (cf. object_realize() discussion) I don't want the old cpu.h #define
>> mess leaking into code that I'm redesigning specifically to get rid of
>> that target-*/cpu.h dependency in favor of a single qemu/cpu.h.
>> qom/cpu.c is by definition not compiled per target so it cannot contain
>> any target-specific code.
>
> At minimum it should be clearly documented[1] that this is a transitional
> hack, and how it should be removed.  There have already been two posts in this
> thread suggesting this is a feature, implying that this operation is somehow
> target specific.  I think the opposite is true:  This is a target agnostic
> detail of the TCG implementation, and implementing architecturally defined
> MMU/TLB behavior here is activley wrong.

The advantage of making the TLB more target specific (it already
depends on sizes of both target_ulong and target_phys_addr_t) is that
we can push some run time MMU model dependent code to translation
time.

The translator currently generates calls to qemu_ld, which generates a
TLB lookup with TCG. In the miss case, the TCG helper calls tlb_fill
and then cpu_xxx_handle_mmu_fault(). Most of those functions have
conditional code for current MMU mode, data access vs code access,
switch() based on MMU type etc.

With changes to TCG TLB handling, the translator could specify the
function to be called on TLB miss (or different TLB functions can be
generated with softmmu templates). Then the conditional code can be
pushed to translator.

In some TLB based MMU cases, we could predict or even calculate in
advance the MMU TLB index and other parameters but there's no way to
pass those to the miss/fault handler now.

Taking x86 as an example, we could have different functions for each case of:
- paging disabled
- x86_64 long mode enabled
- PAE enabled
- maybe permuted with the above, PSE enabled

PPC (or later Sparc64) MMU models do not change during execution, so
getting rid of the run time switches would be nice.

In some cases the QEMU TLB and target MMU TLBs could be combined,
provided that the target MMU is reasonable. For example UltraSPARC-IIi
I/D MMU TLBs have only 64 (though variable size) entries, whereas the
QEMU TLB can have many more, so I probably would not consider that
case.

Some MMUs have a notion of contexts (e.g. mapped to PID or thread ID),
but currently a TLB flush is needed when the context is changed which
can be suboptimal. To solve that, we'd need to change the TLB access
to target specific code to consider both context and address when
calculating the index.

In general, the interface between generated code and the TCG helper
should be more optimized.

>
> Paul
>
> [1] In the code, not the commit message.  Commit logs are not documentation.
> Commit logs are transient information valid only when the patch is applied.
> After that point they become archeological evidence, and you should not expect
> subsequent developers to be aware of them.