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

[Marc Zyngier]

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@xxxxxxx>
> Cc: Marc Zyngier <marc.zyngier@xxxxxxx>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> ---
>  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.

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.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...