Re: [PATCH] KVM: PPC: E500: Support hugetlbfs

[Scott Wood <scottwood@freescale.com>]

On 09/24/2011 02:47 AM, Alexander Graf wrote:
> 
> On 22.09.2011, at 08:50, Liu Yu-B13201 wrote:
> 
>>
>>
>>> -----Original Message-----
>>> From: kvm-ppc-owner@vger.kernel.org 
>>> [mailto:kvm-ppc-owner@vger.kernel.org] On Behalf Of Alexander Graf
>>> Sent: Tuesday, September 20, 2011 7:36 AM
>>> To: kvm-ppc@vger.kernel.org
>>> Cc: kvm@vger.kernel.org
>>> Subject: [PATCH] KVM: PPC: E500: Support hugetlbfs
>>>
>>> With hugetlbfs support emerging on e500, we should also support KVM
>>> backing its guest memory by it.
>>>
>>> This patch adds support for hugetlbfs into the e500 shadow mmu code.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>> arch/powerpc/kvm/e500_tlb.c |   22 ++++++++++++++++++++++
>>> 1 files changed, 22 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kvm/e500_tlb.c b/arch/powerpc/kvm/e500_tlb.c
>>> index ec17148..64f75eb 100644
>>> --- a/arch/powerpc/kvm/e500_tlb.c
>>> +++ b/arch/powerpc/kvm/e500_tlb.c
>>> @@ -24,6 +24,7 @@
>>> #include <linux/sched.h>
>>> #include <linux/rwsem.h>
>>> #include <linux/vmalloc.h>
>>> +#include <linux/hugetlb.h>
>>> #include <asm/kvm_ppc.h>
>>> #include <asm/kvm_e500.h>
>>>
>>> @@ -673,13 +674,34 @@ static inline void 
>>> kvmppc_e500_shadow_map(struct kvmppc_vcpu_e500 *vcpu_e500,
>>> 				pfn &= ~(tsize_pages - 1);
>>> 				break;
>>> 			}
>>> +		} else if (vma && hva >= vma->vm_start &&
>>> +                           (vma->vm_flags & VM_HUGETLB)) {
>>
>> Why check (vma && hva >= vma->vm_start) twice?
> 
> What would you do? :)

I think he's just complaining about doing the check twice, in which case
the answer could be "it avoids extra indentation and the compiler should
be able to factor out the common subexpression".

> In fact, I only copied the vm_start condition from the pfn code.
> Scott, why do we have to check this in the first place? We're calling
> find_vma. Can that return a vma that does not cover the hva we're
> passing in?

Yes, it can.  From find_vma():

/* Look up the first VMA which satisfies  addr < vm_end,  NULL if none. */

You'll find similar checks in a lot of other places where find_vma() is
used.

-Scott