Alex, what do you think about this? On Thu, 23 Aug 2012 16:35:15 +0800 Gavin Shan <shangw@xxxxxxxxxxxxxxxxxx> wrote: > On Thu, Aug 23, 2012 at 05:24:00PM +0900, Takuya Yoshikawa wrote: > >On Thu, 23 Aug 2012 15:42:49 +0800 > >Gavin Shan <shangw@xxxxxxxxxxxxxxxxxx> wrote: > > > >> The build error was caused by that builtin functions are calling > >> the functions implemented in modules. That was introduced by the > >> following commit. > >> > >> commit 4d8b81abc47b83a1939e59df2fdb0e98dfe0eedd > >> > >> The patches fix that to convert the gfn to hva in direct way. > > > >Not just that... > > > >> > >> Signed-off-by: Gavin Shan <shangw@xxxxxxxxxxxxxxxxxx> > >> --- > >> diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c > >> index 56ac1a5..0958523 100644 > >> --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c > >> +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c > >> @@ -197,7 +197,8 @@ long kvmppc_h_enter(struct kvm_vcpu *vcpu, unsigned long flags, > >> pa &= PAGE_MASK; > >> } else { > >> /* Translate to host virtual address */ > >> - hva = gfn_to_hva_memslot(memslot, gfn); > >> + hva = memslot->userspace_addr + > >> + (gfn - memslot->base_gfn) * PAGE_SIZE; > > > >Although I do not think what that commit wanted to do was wrong, > >I believe the gfn_to_hva_memslot() change should have been > >explained more explicitely, at least, to all users of > >the not-anymore-inline function. > > > >Probably kvm-ppc people should re-check the call sites: what they > >want to use may be newly introduced __gfn_to_hva_memslot() which > >is equivalent to the original gfn_to_hva_memslot(). > > > > Do you mean that move __gfn_to_hva_memslot() from kvm_main.c > to kvm_host.h and make that "inline"? $ grep -r gfn_to_hva_memslot . ./include/linux/kvm_host.h:unsigned long gfn_to_hva_memslot(struct kvm_memory_slot *slot, gfn_t gfn); ./arch/powerpc/kvm/book3s_hv_rm_mmu.c: hva = gfn_to_hva_memslot(memslot, gfn); ./arch/powerpc/kvm/book3s_64_mmu_hv.c: start = gfn_to_hva_memslot(memslot, gfn); ./arch/powerpc/kvm/book3s_64_mmu_hv.c: hva = gfn_to_hva_memslot(memslot, gfn); ./arch/powerpc/kvm/book3s_64_mmu_hv.c: hva = gfn_to_hva_memslot(memslot, gfn); ./arch/powerpc/kvm/e500_tlb.c: hva = gfn_to_hva_memslot(slot, gfn); ./virt/kvm/kvm_main.c:static unsigned long __gfn_to_hva_memslot(struct kvm_memory_slot *slot, ./virt/kvm/kvm_main.c: return __gfn_to_hva_memslot(slot, gfn); ./virt/kvm/kvm_main.c:unsigned long gfn_to_hva_memslot(struct kvm_memory_slot *slot, ./virt/kvm/kvm_main.c:EXPORT_SYMBOL_GPL(gfn_to_hva_memslot); Looks like only ppc is using this after that commit. If the change had any meaning, it should have been understood by its users: kvm-ppc developers in this case. Otherwise it just added extra overheads to that simple calculation: are the checks in __gfn_to_hva_many() going to have any meaning for kvm-ppc in the futuer? If not, I would suggest using __gfn_to_hva_memslot(). But if so, there will be no user of gfn_to_hva_memslot()... Thanks, Takuya -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html