> -----Original Message----- > From: kvm-ppc-owner@xxxxxxxxxxxxxxx > [mailto:kvm-ppc-owner@xxxxxxxxxxxxxxx] On Behalf Of Hollis Blanchard > Sent: Friday, December 05, 2008 7:12 AM > To: Liu Yu-B13201 > Cc: kvm-ppc@xxxxxxxxxxxxxxx > Subject: Re: [PATCH 2/3] kvmppc: Add e500 support > > I haven't gone through this all (since it won't apply/build now), but > just a couple quick comments: Thanks. > > 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@xxxxxxxxxxxxx, Aug, 2008 > > + * > > + * Description: > > + * This file is based on arch/powerpc/kvm/44x_tlb.h, > > + * by Hollis Blanchard <hollisb@xxxxxxxxxx>. > > + * > > + * 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. Fixed. > > > +#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. They are used to describe TLB0 physical structure. Maintain its logical structure can avoid illogical TLB0 size and can provide correct content when guest read appointed entry. (TLB0 also provide a way to read a certain entry) But kernel now doesn't concern its structure, its size or even content of a certain entry, it only use the auto-update mechanism to update TLB0.... So they're unnecessary for linux now, but I think they can avoid possible problem. > > > +#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. Right. But kernel doesn't concern it now... > > > +#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. Yes. In fact, I don't like them either. Suggestions are always welcomed. :-) > > > +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. I'll think about it. Thanks. -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
