RE: [patch 0/4] add e500 platform support for KVM

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

On Fri, 2008-09-12 at 10:28 +0800, Liu Yu-B13201 wrote:
> > > > Some of the refactoring you've done, like creating 
> > completely separate
> > > > kvmppc_handle_tlb_miss() functions, surprises me. For example, I'd
> > > > expect the DTLB miss code to be refactored like this:
> > > > 
> > > >         case BOOKE_INTERRUPT_DTLB_MISS:	
> > > >         	gtlbe = kvmppc_dtlb_search(vcpu, eaddr); <- CORE HOOK
> > > >         	if (!gtlbe) {
> > > >         		/* The guest didn't have a mapping for it. */
> > > >         		kvmppc_queue_exception(vcpu, exit_nr);
> > > >         		vcpu->arch.dear = vcpu->arch.fault_dear;
> > > >         		vcpu->arch.esr = vcpu->arch.fault_esr;
> > > >         		kvmppc_deliver_dtlb_miss(vcpu); <- CORE HOOK
> > > >         		vcpu->stat.dtlb_real_miss_exits++;
> > > >         		r = RESUME_GUEST;
> > > >         		break;
> > > >         	}
> > > >         
> > > >         	vcpu->arch.paddr_accessed = tlb_xlate(gtlbe, eaddr);
> > > >         	gfn = vcpu->arch.paddr_accessed >> PAGE_SHIFT;
> > > >         
> > > >         	if (kvm_is_visible_gfn(vcpu->kvm, gfn)) {
> > > >         		kvmppc_mmu_map(vcpu, eaddr, gfn, gtlbe->tid,
> > > >         		               gtlbe->word2); <- CORE HOOK
> > > >         		vcpu->stat.dtlb_virt_miss_exits++;
> > > >         		r = RESUME_GUEST;
> > > >         	} else
> > > >         		r = kvmppc_emulate_mmio(run, vcpu);
> > > >         
> > > >         	break;
> > > > 
> > > 
> > > Hollis, I'm reconsiderring this place.
> > > The problem is that e500 has 2 TLB, so I need to get the 
> > tlb index and
> > > entry index from kvmppc_dtlb_search
> > > And if kvm_is_visible_gfn() returns nonzero, the two index 
> > are needed to
> > > manipulate TLB0 or TLB1.
> > 
> > OK, sounds reasonable. Feel free to change the prototypes of 
> > these hooks
> > to whatever you need, as long as it's clean and still lets me 
> > implement
> > 440.
> > 
> > For example, maybe something like this:
> >         int kvmppc_dtlb_index(struct kvm_vcpu *vcpu, u32 eaddr);
> > 
> > You could then encode the TLB index in the high bits of the return
> > index. Alternatively, you could pass &tlb and &index, but I 
> > think I like
> > that less.
> > 
> 
> I just wanted you to know this place has some dependent factors.
> Never mind. I think I could use the your former suggestion. 
> I could calculate the index by offset or save index in vcpu->arch.
> What's your opinion?

I would definitely prefer using the index as a return value, rather than
saving to vcpu->arch.

-- 
Hollis Blanchard
IBM Linux Technology Center