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

[David Gibson]

On Tue, Feb 12, 2013 at 11:40:37AM +0100, Andreas Färber 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.

So currently MMU selection is actually basde on the mmu_model field in
the env structure, rather than the cpu type directly.  In any cases my
cleanups greatly reduce the number of places we check the mmu type, so
it won't be very difficult to adjust for the new scheme.

> As reminded repeatedly, please do not introduce new static or global
> helper functions using CPUPPCState, use PowerPCCPU instead.

Ok, I'll keep that in mind.  At the moment I've been using CPUPPCState
because that's what's available at the top-level and passed down.
I'll look at this if I add global functions, but in any case it should
be pretty easy to update either way around.

> 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.

Ok, noted.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: Digital signature