Re: [PATCH 1/3] kvmppc: Some refactor for merging e500

[Hollis Blanchard <hollisb@us.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 2466 bytes --]

On Wed, 2008-12-03 at 17:28 +0800, Liu Yu wrote:
> Signed-off-by: Liu Yu <yu.liu@freescale.com>
> 
> ---
>  arch/powerpc/include/asm/kvm_44x.h  |    9 +++++
>  arch/powerpc/include/asm/kvm_host.h |    7 ----
>  arch/powerpc/include/asm/kvm_ppc.h  |    5 +--
>  arch/powerpc/kvm/44x.c              |   22 +++++++++++++-
>  arch/powerpc/kvm/44x_tlb.c          |   29 +++++------------
>  arch/powerpc/kvm/44x_tlb.h          |   19 +++++++++--
>  arch/powerpc/kvm/booke.c            |   49 +++++++++----------------------
>  arch/powerpc/kvm/booke.h            |    8 +++++
>  arch/powerpc/kvm/booke_interrupts.S |   56 ++++------------------------------
>  arch/powerpc/kvm/booke_interrupts.h |   54 +++++++++++++++++++++++++++++++++
>  10 files changed, 139 insertions(+), 119 deletions(-)
>  create mode 100644 arch/powerpc/kvm/booke_interrupts.h

This patch already doesn't apply. (I know it's awkward that I don't have
a published git tree, and I'm sorry about that.) Aside from that, the
patch is also a lot to review at once. Splitting it into smaller
logically separate patches would ease both problems.

For example, I see at least the following patches here: 
      * moving struct kvmppc_44x_tlbe 
      * kvmppc_booke_init() changes 
      * LOAD/SAVE_PID
      * moving #include "44x_tlb.h"
      * kvmppc_mmu_map() -> kvmppc_core_mmu_map() 
      * LOAD_GUEST_TLB -- this one is obsolete, so if it were a separate
        patch, we could just drop it and the rest would still apply

Some of these I disagree with, so we need to be able to discuss them
individually. Here are some of my comments right now:

I'm torn about the "map" changes because I wanted that function to be
generic (e.g. could apply for hash MMUs too), but it already wasn't very
generic, so we can refactor later as needed.

It looks like Linux (the host) never touches PID1 or PID2 after boot. In
that case, we don't need to save/restore them in our exit handlers.

I'm not sure the kvmppc_booke_init() refactoring is worth it. Can
kvmppc_e500_init() just memcpy the extra interrupt handlers itself?
There are only a few.

I really have to run, but what do you think about the attached patches?
I'll provide proper descriptions later but wanted to get these out for
you tonight. (You can apply with git-am or extract with mseries[1].)

By the way, don't forget to add a Signed-off-by line.

[1] http://oss.oracle.com/~mason/mseries/

-- 
Hollis Blanchard
IBM Linux Technology Center

[-- Attachment #2: kvmppc.mbox --]
[-- Type: application/mbox, Size: 17774 bytes --]