Re: [PATCH] kvm: arm64: Relax the restriction on using stage2 PUD huge mapping

[Suzuki K Poulose <suzuki.poulose@arm.com>]

Marc,

On 30/01/2019 10:21, Marc Zyngier wrote:
> On 29/01/2019 19:12, Suzuki K Poulose wrote:
>> We restrict mapping the PUD huge pages in stage2 to only when the
>> stage2 has 4 level page table, leaving the feature unused with
>> the default IPA size. But we could use it even with a 3
>> level page table, i.e, when the PUD level is folded into PGD,
>> just like the stage1. Relax the condition to allow using the
>> PUD huge page mappings at stage2 when it is possible.
>>
>> Cc: Christoffer Dall <christoffer.dall@arm.com>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>   virt/kvm/arm/mmu.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
>> index fbdf3ac..30251e2 100644
>> --- a/virt/kvm/arm/mmu.c
>> +++ b/virt/kvm/arm/mmu.c
>> @@ -1695,11 +1695,14 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>   
>>   	vma_pagesize = vma_kernel_pagesize(vma);
>>   	/*
>> -	 * PUD level may not exist for a VM but PMD is guaranteed to
>> -	 * exist.
>> +	 * The stage2 has a minimum of 2 level table (For arm64 see
>> +	 * kvm_arm_setup_stage2()). Hence, we are guaranteed that we can
>> +	 * use PMD_SIZE huge mappings (even when the PMD is folded into PGD).
>> +	 * As for PUD huge maps, we must make sure that we have at least
>> +	 * 3 levels, i.e, PMD is not folded.
>>   	 */
>>   	if ((vma_pagesize == PMD_SIZE ||
>> -	     (vma_pagesize == PUD_SIZE && kvm_stage2_has_pud(kvm))) &&
>> +	     (vma_pagesize == PUD_SIZE && kvm_stage2_has_pmd(kvm))) &&
>>   	    !force_pte) {
>>   		gfn = (fault_ipa & huge_page_mask(hstate_vma(vma))) >> PAGE_SHIFT;
>>   	}
>>
> 
> For the record, it took a 10 minute discussion with Suzuki  to
> understand that the above is actually correct, even if massively
> confusing. Why is it correct:
> 
> - In a 4 level stage-2, each of PGD/PUD/PMD exists on its own, and start
> level is 0
> - In a 3 level stage-2, PGD and PUD are fused as the start level is 1,
> and PMD exists on its own
> - In a 2 level stage-2, both PGD, PUD and PMD are fused, and start level
> is 2.
> 
>  From the above, we can extract that you can always have a block mapping
> at the PUD level if you have a standalone PMD, and that's the logic this
> patch is using.

Thanks for writing it up ! :-)

> 
> Now, the *real* reason is that you can map a given size in your S2PT,
> not that some level is fused or not. What we want is probably a helper
> that says:
> 
> bool kvm_stage2_can_map_block_size(struct kvm *kvm, size_t sz)
> {
> 	return sz >= PMD_SIZE && stage2_pgdir_size(kvm) >= sz);
> }
> 
> and the above becomes:
> 
> if (kvm_stage2_can_map_block_size(kvm, vma_pagesize)) && !force_pte)
> 
> which I find much nicer.
> 
> I guess I can still take the above as a fix if Christoffer is happy with
> it, but if you think my above hack is correct, I'd like things to be
> cleaned-up in the near future.

Sure, as we discussed that makes it much simpler and generic. I could
address that.

Suzuki