Re: [PATCH 2/3] kvmppc: Add e500 support

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

I haven't gone through this all (since it won't apply/build now), but
just a couple quick comments:

On Wed, 2008-12-03 at 17:28 +0800, Liu Yu wrote:
> diff --git a/arch/powerpc/kvm/e500_tlb.h b/arch/powerpc/kvm/e500_tlb.h
> new file mode 100644
> index 0000000..6adda39
> --- /dev/null
> +++ b/arch/powerpc/kvm/e500_tlb.h
> @@ -0,0 +1,290 @@
> +/*
> + * Copyright (C) 2008 Freescale Semiconductor, Inc. All rights reserved.
> + *
> + * Author: Yu Liu, yu.liu@freescale.com, Aug, 2008
> + *
> + * Description:
> + * This file is based on arch/powerpc/kvm/44x_tlb.h,
> + * by Hollis Blanchard <hollisb@us.ibm.com>.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License, version 2, as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef __KVM_E500_TLB_H__
> +#define __KVM_E500_TLB_H__
> +
> +#include <linux/kvm_host.h>
> +#include <asm/mmu-fsl-booke.h>
> +#include <asm/tlb.h>
> +#include <asm/kvm_e500.h>
> +
> +#define KVM_E500_TLB0_ENABLE   /* Enable it to utilize TLB0 */

Just remove this ifdef completely.

> +#define KVM_E500_TLB0_WAY_SIZE_BIT     7       /* Fixed */
> +#define KVM_E500_TLB0_WAY_SIZE         (1UL << KVM_E500_TLB0_WAY_SIZE_BIT)
> +#define KVM_E500_TLB0_WAY_SIZE_MASK    (KVM_E500_TLB0_WAY_SIZE - 1)
> +
> +#define KVM_E500_TLB0_WAY_NUM_BIT      1       /* No greater than 7 */
> +#define KVM_E500_TLB0_WAY_NUM          (1UL << KVM_E500_TLB0_WAY_NUM_BIT)
> +#define KVM_E500_TLB0_WAY_NUM_MASK     (KVM_E500_TLB0_WAY_NUM - 1)

Are these really KVM constants, or e500 in general? If general e500, you
should put them in a general e500 header.

> +#define KVM_E500_TLB0_SIZE  (KVM_E500_TLB0_WAY_SIZE * KVM_E500_TLB0_WAY_NUM)
> +#define KVM_E500_TLB1_SIZE  16

You probably want to distinguish between hardware TLB size and guest TLB
size, since TLBnCFG allows you to trivially expose a larger virtual TLB
to the guest than the hardware really supports.

> +#define index_of(tlbsel, esel) (((tlbsel) << 16) | ((esel) & 0xFFFF))
> +#define tlbsel_of(index)       ((index) >> 16)
> +#define esel_of(index)         ((index) & 0xFFFF)
> +#define to_htlb1_esel(esel) (num_tlbcam_entries - (esel) - 1)

I don't really like these names, but they're completely local so it's
not too big a deal.

> +extern unsigned int tlbcam_index;
> +extern unsigned int num_tlbcam_entries;

You should put these externs somewhere in a generic e500 header, since
they are generic e500 globals. They should probably also be renamed to
reflect that...

Either way it's something you should submit, as a small separate patch,
to the e500 maintainer to get their ack.

-- 
Hollis Blanchard
IBM Linux Technology Center