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-ppc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
