On Tue, Feb 12, 2013 at 10:40 AM, Andreas Färber <afaer...@suse.de> 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 >
