Re: [Qemu-ppc] [Qemu-devel] [0/8] RFC: target-ppc: Start disentangling different MMU types

[Blue Swirl]

On Tue, Feb 12, 2013 at 10:40 AM, Andreas Färber <address@hidden> wrote:
> Am 12.02.2013 03:00, schrieb David Gibson:
>> The target-ppc code supports CPUs with a number of different MMU
>> types: there's both the 32-bit and 64-bit versions of the "classic"
>> hash page table based powerpc mmu and there's also the BookE and 40x
>> MMUs.
>>
>> Currently handling of all these has a roughly shared path in
>> mmu_helper.c.  Although code sharing is usually a good idea, in this
>> case the MMUs really aren't similar enough for this to be useful.
>> Instead it results in checking and behaving differently at many, many
>> different points in the path leading to an unreadable tangle of code.
>>
>> This patch series is a first step to cleaning this up, moving to a
>> model where we have a single switch on the MMU family at the top-level
>> entry points, then have a simpler, clearer separate code path for each
>> MMU type.  More specifically, it disentangles the path for the 64-bit
>> classic hash MMU into its own new file.  The other MMU types keep the
>> existing code (minus 64-bit hash specific conditionals) for now.
>> Disentangling those as well would be a good idea, but would be best
>> done by someone with more resources to test those other platforms than
>> I have.
>>
>> For now, the resulting 64-bit hash path retains the same structure as
>> the original shared code, just the obvious conditionals on other mmu
>> types are removed.  This path is fairly ugly in itself, but cleaning
>> that up is a later step, now much simpler without the other MMU types
>> to deal with at the same time.
>
> Some general comments: The idea of the ongoing QOM work just sent out is
> to change the hierarchy from:
>
> Object
> - DeviceState
>   - CPUState
>     - PowerPCCPU
>       - 970 vX.Y
>       - POWER7 vX.Y
>       ...
>
> to:
>
> Object
> - DeviceState
>   - CPUState
>     - PowerPCCPU
>       - 970 family
>         - 970 vX.Y
>       - POWER7 family
>         - POWER7 vX.Y
>       ...
>
> PowerPCCPUClass is expected to grow methods overridden per family or
> model. I.e., where sensible the class should serve as indirection for
> which MMU/... implementation to choose rather than ifs or #ifdefs or
> <prefix>_family glue sprinkled througout code.
>
> As reminded repeatedly, please do not introduce new static or global
> helper functions using CPUPPCState, use PowerPCCPU instead.
>
> If you introduce global functions, please make them unique by using ppc_
> prefix for arbitrary functions and ppc_cpu_ for functions taking
> PowerPCCPU as first argument.

I'd also add that separating the MMU functions could help if one day
we introduce CPU model specific TCG memory access helpers.

>
> Thanks,
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>