Re: [PATCH v8 1/9] KVM: arm/arm64: Ensure only THP is candidate for adjustment

[Punit Agrawal]

Punit Agrawal <punit.agrawal@xxxxxxx> writes:

> Christoffer Dall <christoffer.dall@xxxxxxx> writes:
>
>> On Mon, Oct 01, 2018 at 04:54:35PM +0100, Punit Agrawal wrote:
>>> PageTransCompoundMap() returns true for hugetlbfs and THP
>>> hugepages. This behaviour incorrectly leads to stage 2 faults for
>>> unsupported hugepage sizes (e.g., 64K hugepage with 4K pages) to be
>>> treated as THP faults.
>>> 
>>> Tighten the check to filter out hugetlbfs pages. This also leads to
>>> consistently mapping all unsupported hugepage sizes as PTE level
>>> entries at stage 2.
>>> 
>>> Signed-off-by: Punit Agrawal <punit.agrawal@xxxxxxx>
>>> Reviewed-by: Suzuki Poulose <suzuki.poulose@xxxxxxx>
>>> Cc: Christoffer Dall <christoffer.dall@xxxxxxx>
>>> Cc: Marc Zyngier <marc.zyngier@xxxxxxx>
>>> Cc: stable@xxxxxxxxxxxxxxx # v4.13+
>>
>>
>> Hmm, this function is only actually called from user_mem_abort() if we
>> have (!hugetlb), so I'm not sure the cc stable here was actually
>> warranted, nor that this patch is strictly necessary.
>>
>> It doesn't hurt, and makes the code potentially more robust for the
>> future though.
>>
>> Am I missing something?
>
> !hugetlb is only true for hugepage sizes supported at stage 2. 

Of course I meant "hugetlb" above (Note the lack of "!").

> The function also got called for unsupported hugepage size at stage 2,
> e.g., 64k hugepage with 4k page size, which then ended up doing the
> wrong thing.
>
> Hope that adds some context. I should've added this to the commit log.
>
>>
>> Thanks,
>>
>>     Christoffer
>>
>>> ---
>>>  virt/kvm/arm/mmu.c | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
>>> index 7e477b3cae5b..c23a1b323aad 100644
>>> --- a/virt/kvm/arm/mmu.c
>>> +++ b/virt/kvm/arm/mmu.c
>>> @@ -1231,8 +1231,14 @@ static bool transparent_hugepage_adjust(kvm_pfn_t *pfnp, phys_addr_t *ipap)
>>>  {
>>>  	kvm_pfn_t pfn = *pfnp;
>>>  	gfn_t gfn = *ipap >> PAGE_SHIFT;
>>> +	struct page *page = pfn_to_page(pfn);
>>>  
>>> -	if (PageTransCompoundMap(pfn_to_page(pfn))) {
>>> +	/*
>>> +	 * PageTransCompoungMap() returns true for THP and
>>> +	 * hugetlbfs. Make sure the adjustment is done only for THP
>>> +	 * pages.
>>> +	 */
>>> +	if (!PageHuge(page) && PageTransCompoundMap(page)) {
>>>  		unsigned long mask;
>>>  		/*
>>>  		 * The address we faulted on is backed by a transparent huge
>>> -- 
>>> 2.18.0
>>> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@xxxxxxxxxxxxxxxxxxxxx
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm