On 08/24/2012 09:54 AM, Takuya Yoshikawa wrote:
> 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);
>
Only book3s_hv_rm_mmu.c has this issue since it built-in kernel, other files
are compiled as kvm module, it is fine.
>
> 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.
The current gfn_to_hva_memslot do the properly check for slot->flags, though
only x86 supports READONLY memslot now, i think it is also useful for Power.
BTW, we are doing the work on the qemu side now.
And we can not prevent other architectures (like x86) use this interface in
the further.
>
> 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?
It is no hard to inline gfn_to_hva_memslot, since it only checks slot->flags,
if need, we can put all the things in kvm_host.h.
--
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
