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

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

On Fri, 2008-08-22 at 19:00 +0800, Liu Yu wrote: 
> > 
> > Could you talk a little about how it differs from the 440
> > implementation? For example, how are you using TLB0 and TLB1?
> 
> Sorry that I havent provided the detailed description.
> 
> The case of TLB1 is the same as 44x.
> Since TLB0 is 4K-fixed map, it has different implementation.
> 
> Guest always has TLB0 and TLB1.
> Host can chosen how to map them.
> 
> Currently if not define KVMPPC_E500_TLB0_ENABLE,
> both guest TLB1 and guest TLB0 map to host TLB1.
> Otherwise, guest TLB1 map to host TLB1 and guest TLB0 map to host TLB0.
> KVMPPC_E500_TLB0_ENABLE is an intergradation,
> as I first only use host TLB1, after TLB1 is ok then I debug host TLB0.

OK, I think you're saying that KVMPPC_E500_TLB0_ENABLE is a temporary
hack, since Wei's original code used only TLB1, and you're now extending
that to also use TLB0.

> Unlike TLB1(one-to-many mapping, the same as 44x), guest TLB0 and shadow
> TLB0 is one-one mapping. 
> That is to say, you can use the entry select or index to find the
> guest/shadow pair.
> 
> Moreover, TLB1 has fixed one-one mapping between shadow TLB1 and host
> TLB1.
> You can find the host TLB1 entry according to the index of shadow TLB1.
> For TLB0, it's a dynamic one-one mapping, you can not find which host
> entry the shadow ultimately be mapped.
> This is due to the auto-entry-select mechanism of E500. And the only way
> to destroy an entry is tibivax.

You lost me a little bit here.

It looks like your kvm_vcpu_arch structure holds a full copy of both
TLB0 and TLB1, guest and shadow. However, I can't see where you write
into shadow_tlb[0]; instead it looks like you directly insert those
entries into the hardware without saving a copy in the vcpu.

> Unlike 44x use TID=0 map userspace and TID=1 map kernel space,
> I plan to use host TLB1 to map kernel, and host TLB0 to map userspace.
> This category can make it convient to handle privilege switch: that is
> before enterring guest just need tlbivax TLB1.
> Userspace tlb enties with differnet TID can exist in host TLB0 till host
> kvm process switch or guest meet explicit tlbiax command.
> 
> I just have this idea and have not thought all the detail through.
> What do you think of it?

You might run into problems if you ever get large guest userspace
mappings. I know hugetlbfs doesn't exist for e500 Linux right now, but
it could in the future, and plus there are other kernels to consider.

> > 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:
> 
> In fact, I don't have much experience on refactor,
> I just thought we could merge as more common code as we can.
> 
> Do we need to separate dtlb and itlb?
> I tried to merge them so that the code became what it looks like. :-|

Your code does something similar to mine, just without the helper
functions:

        kvmppc_handle_tlb_miss(vcpu, eaddr, MSR_IS);
vs
        kvmppc_44x_itlb_search(...) {
                kvmppc_44x_tlb_index(..., !!(msr & MSR_IS))
        }

> > booke_fsl_interrupts.S looks like a lot of code copied and 
> > pasted, with
> > the obvious exception of the TLB handlers. Can't we work out 
> > some better
> > way to share the rest?
> 
> That would be great.
> I just considered it step-by-step, and I am not very familiar with gnu
> assemble macro, define things,
> and I nocited head_fsl_booke.S and head_44x.S...
> 
> I will think about it.

OK, so there are two basic differences in booke_interrupts.S, and
they're both in the common "lightweight exit" path: the additional PID
register switching and the modifications to the TLB save/restore.

The PID stuff should be easy enough to hide: just create a
"KVMPPC_SAVE_PID" macro.

For now we can probably do the same with the TLB manipulations.

-- 
Hollis Blanchard
IBM Linux Technology Center