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

[Alexander Graf]

Am 23.02.2013 um 09:28 schrieb David Gibson <address@hidden>:

> On Fri, Feb 22, 2013 at 05:15:22PM +0100, Alexander Graf wrote:
>> 
>> On 12.02.2013, at 03:00, David Gibson wrote:
>> 
>>> 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.
>> 
>> Looks like a nice and long overdue cleanup. A few points I'd like to see 
>> addressed:
>> 
>>  - Instead of exporting common helper functions, put them into a
>>  new mmu-hash.h as static inline. That way things can get optimized
>>  a lot better.
> 
> The common helpers are all gone again by the end of the series, so is
> there really any point?

Oh, really? I must have missed the bit where you remove / unexport them then ;).

> 
>>  - Please extract the 32bit hash functions into a separate file as
>>  - well. You're touching all that code anyways, so I'm sure you can
>>  - test it :).
> 
> Sigh.  If I must.
> 
>>  - QOM'ify things please :). Just make your mmu type specific
>>  - function calls function pointers in the class and invoke them
>>  - from there instead of the switchery.
> 
> Ok.  I think that works best as an extension to the series, rather
> than actually changing any of the existing patches.

Sure, if it makes life easier for you you can certainly append the qomification 
to the series.

Alex

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