Re: [PATCH v6 8/9] KVM: guest_memfd: Explicitly pass number of pages to make_private() hook

[Ackerley Tng]

Sean Christopherson <seanjc@xxxxxxxxxx> writes:

> Tweak the guest_memfd make_private() hook to explicitly pass the number of
> pages to align with the signature of the make_shared() hook, and because
> the existing code is outright broken if a guest_memfd folio is comprised of
> more than one page (which can't happen, yet).  The SNP code *tries* to
> create a corresponding huge entry, but if the RMP must use 4KiB entries for
> whatever reason, KVM will only convert the first pfn, and not the entire
> range of pfns that will be mapped into the guest.
>
> Alternatively, @max_order could simply be repurposed as _the_ @order, but
> that will fall apart when in-place conversion comes along, at which point
> KVM will need to deal with conversions that aren't bound 1:1 to a folio.
> I.e. the number of pages to convert may not be exactly be a power-of-2 (and
> folios don't strictly guarantee power-of-2 pages anyways).
>
> WARN in the SNP code if the number of pages to prepare is anything other
> than '1', i.e. if guest_memfd is trying to prepare/convert more than a
> single 4KiB page, as sev_gmem_prepare() doesn't actually handle conversion
> greater than order-0 folios.
>
> Opportunistically swap the ordering of @pfn and @gfn params for
> kvm_x86_ops.gmem_make_private() to match kvm_arch_gmem_make_private().
>
> Fixes: b85524314a3d ("KVM: guest_memfd: delay kvm_gmem_prepare_folio() until the memory is passed to the guest")
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
>  arch/x86/include/asm/kvm_host.h |  3 ++-
>  arch/x86/kvm/svm/sev.c          | 27 +++++++++++----------------
>  arch/x86/kvm/svm/svm.h          |  2 +-
>  arch/x86/kvm/x86.c              |  5 +++--
>  include/linux/kvm_host.h        |  2 +-
>  virt/kvm/guest_memfd.c          |  2 +-
>  6 files changed, 19 insertions(+), 22 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 4be57157136b..230267b2203b 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1725,7 +1725,8 @@ struct kvm_x86_ops {
>  	gva_t (*get_untagged_addr)(struct kvm_vcpu *vcpu, gva_t gva, unsigned int flags);
>  	void *(*alloc_apic_backing_page)(struct kvm_vcpu *vcpu);
>  #ifdef CONFIG_HAVE_KVM_ARCH_GMEM_CONVERT
> -	int (*gmem_make_private)(struct kvm *kvm, kvm_pfn_t pfn, gfn_t gfn, int max_order);
> +	int (*gmem_make_private)(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
> +				 kvm_pfn_t nr_pages);
>  #endif
>  #ifdef CONFIG_HAVE_KVM_ARCH_GMEM_RECLAIM
>  	void (*gmem_make_shared)(kvm_pfn_t pfn, kvm_pfn_t nr_pages);
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 0ecba768c153..bf7f94d1d7f9 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -5090,15 +5090,7 @@ static bool is_pfn_range_shared(kvm_pfn_t start, kvm_pfn_t end)
>  	return true;
>  }
>
> -static u8 max_level_for_order(int order)
> -{
> -	if (order >= KVM_HPAGE_GFN_SHIFT(PG_LEVEL_2M))
> -		return PG_LEVEL_2M;
> -
> -	return PG_LEVEL_4K;
> -}
> -
> -static bool is_large_rmp_possible(struct kvm *kvm, kvm_pfn_t pfn, int order)
> +static bool is_large_rmp_possible(kvm_pfn_t pfn, kvm_pfn_t nr_pages)
>  {
>  	kvm_pfn_t pfn_aligned = ALIGN_DOWN(pfn, PTRS_PER_PMD);
>
> @@ -5107,14 +5099,14 @@ static bool is_large_rmp_possible(struct kvm *kvm, kvm_pfn_t pfn, int order)
>  	 * PFN is currently shared, then the entire 2M-aligned range can be
>  	 * set to private via a single 2M RMP entry.
>  	 */
> -	if (max_level_for_order(order) > PG_LEVEL_4K &&
> +	if (nr_pages >= KVM_PAGES_PER_HPAGE(PG_LEVEL_2M) &&
>  	    is_pfn_range_shared(pfn_aligned, pfn_aligned + PTRS_PER_PMD))

This could actually just be return (everything in the if condition).

Thinking more about this, asking is_large_rmp_possible() of a pfn and
nr_pages is perhaps the wrong question, since if nr_pages stretches into
the next 2M range, then you might have a different answer for the start
and end (and in-between) 2M ranges.

We can solve all these when we get to huge pages :)

>  		return true;
>
>  	return false;
>  }
>

Reviewed-by: Ackerley Tng <ackerleytng@xxxxxxxxxx>

>
> [...snip...]
>