[PATCH v2 2/2] KVM: arm64: Use folio for THP adjustment

[Ryan Roberts]

On 30/10/2023 10:47, Vincent Donnefort wrote:
> [...]
> 
>>>  static struct kvm_pgtable_mm_ops kvm_s2_mm_ops = {
>>>  	.zalloc_page		= stage2_memcache_zalloc_page,
>>>  	.zalloc_pages_exact	= kvm_s2_zalloc_pages_exact,
>>> @@ -1274,7 +1229,7 @@ static bool fault_supports_stage2_huge_mapping(struct kvm_memory_slot *memslot,
>>>   *
>>>   * Returns the size of the mapping.
>>>   */
>>> -static long
>>> +static unsigned long
>>>  transparent_hugepage_adjust(struct kvm *kvm, struct kvm_memory_slot *memslot,
>>>  			    unsigned long hva, kvm_pfn_t *pfnp,
>>>  			    phys_addr_t *ipap)
>>> @@ -1287,10 +1242,7 @@ transparent_hugepage_adjust(struct kvm *kvm, struct kvm_memory_slot *memslot,
>>>  	 * block map is contained within the memslot.
>>>  	 */
>>>  	if (fault_supports_stage2_huge_mapping(memslot, hva, PMD_SIZE)) {
>>> -		int sz = get_user_mapping_size(kvm, hva);
>>> -
>>> -		if (sz < 0)
>>> -			return sz;
>>> +		size_t sz = folio_size(pfn_folio(pfn));
>>
>> Hi,
>>
>> Sorry this is an extremely late reply - I just noticed this because Marc
>> mentioned it in another thread.
>>
>> This doesn't look quite right to me; just because you have a folio of a given
>> size, that doesn't mean the whole thing is mapped into this particular address
>> space. For example, you could have a (PMD-sized) THP that gets partially
>> munmapped - the folio is still PMD-sized but only some of it is mapped and
>> should be accessible to the process. Or you could have a large file-backed folio
>> (from a filesystem that supports large folios - e.g. XFS) but the application
>> only mapped part of the file.
> 
> I thought originally this would break the block and the folio with it, but a
> quick expriment showed it's not the case.

That's not how it works unfortunately. Even if we wanted to do this in the
common case (which we might in future for anon memory) there are edge cases
where splitting the folio can fail (GUP). And it wouldn't make sense to split
for file-backed case since those folios may be mapped into other processes.

> 
>>
>> Perhaps I've misunderstood and those edge cases can't happen here for some reason?
> 
> And fault_supports_stage2_huge_mapping() would probably not be enough. we might
> end-up with a portion unmapped at stage-1 but mapped at stage-2. :-( 

Yes. Or even remapped if you consider mremapping a portion of the THP.

> 
>>
>> Thanks,
>> Ryan
>>
>>