Re: [PATCH 4/8] kvm: Allow hva_pfn_fast to resolve read-only faults.

From: Oliver Upton <oliver.upton@linux.dev>
To: Anish Moorthy <amoorthy@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	Marc Zyngier <maz@kernel.org>,
	Sean Christopherson <seanjc@google.com>,
	James Houghton <jthoughton@google.com>,
	Ben Gardon <bgardon@google.com>,
	David Matlack <dmatlack@google.com>,
	Ricardo Koller <ricarkol@google.com>,
	Chao Peng <chao.p.peng@linux.intel.com>,
	Axel Rasmussen <axelrasmussen@google.com>,
	kvm@vger.kernel.org, kvmarm@lists.linux.dev
Subject: Re: [PATCH 4/8] kvm: Allow hva_pfn_fast to resolve read-only faults.
Date: Wed, 15 Feb 2023 09:01:56 +0000	[thread overview]
Message-ID: <Y+yfhELf/TbsosO9@linux.dev> (raw)
In-Reply-To: <20230215011614.725983-5-amoorthy@google.com>

On Wed, Feb 15, 2023 at 01:16:10AM +0000, Anish Moorthy wrote:
> The upcoming mem_fault_nowait commits will make it so that, when the
> relevant cap is enabled, hva_to_pfn will return after calling
> hva_to_pfn_fast without ever attempting to pin memory via
> hva_to_pfn_slow.
> 
> hva_to_pfn_fast currently just fails for read-only faults. However,
> there doesn't seem to be a reason that we can't just try pinning the
> page without FOLL_WRITE instead of immediately falling back to slow-GUP.
> This commit implements that behavior.
> 
> Suggested-by: James Houghton <jthoughton@google.com>
> Signed-off-by: Anish Moorthy <amoorthy@google.com>
> ---
>  virt/kvm/kvm_main.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index d255964ec331e..dae5f48151032 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2479,7 +2479,7 @@ static inline int check_user_page_hwpoison(unsigned long addr)
>  }
>  
>  /*
> - * The fast path to get the writable pfn which will be stored in @pfn,
> + * The fast path to get the pfn which will be stored in @pfn,
>   * true indicates success, otherwise false is returned.  It's also the
>   * only part that runs if we can in atomic context.
>   */
> @@ -2487,16 +2487,18 @@ static bool hva_to_pfn_fast(unsigned long addr, bool write_fault,
>  			    bool *writable, kvm_pfn_t *pfn)
>  {
>  	struct page *page[1];
> +	bool found_by_fast_gup =
> +		get_user_page_fast_only(
> +			addr,
> +			/*
> +			 * Fast pin a writable pfn only if it is a write fault request
> +			 * or the caller allows to map a writable pfn for a read fault
> +			 * request.
> +			 */
> +			(write_fault || writable) ? FOLL_WRITE : 0,
> +			page);
>  
> -	/*
> -	 * Fast pin a writable pfn only if it is a write fault request
> -	 * or the caller allows to map a writable pfn for a read fault
> -	 * request.
> -	 */
> -	if (!(write_fault || writable))
> -		return false;
> -
> -	if (get_user_page_fast_only(addr, FOLL_WRITE, page)) {
> +	if (found_by_fast_gup) {

You could have a smaller diff (and arrive at something more readable)
using a local for the gup flags:

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 9c60384b5ae0..57f92ff3728a 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2494,6 +2494,7 @@ static inline int check_user_page_hwpoison(unsigned long addr)
 static bool hva_to_pfn_fast(unsigned long addr, bool write_fault,
 			    bool *writable, kvm_pfn_t *pfn)
 {
+	unsigned int gup_flags;
 	struct page *page[1];
 
 	/*
@@ -2501,10 +2502,9 @@ static bool hva_to_pfn_fast(unsigned long addr, bool write_fault,
 	 * or the caller allows to map a writable pfn for a read fault
 	 * request.
 	 */
-	if (!(write_fault || writable))
-		return false;
+	gup_flags = (write_fault || writable) ? FOLL_WRITE : 0;
 
-	if (get_user_page_fast_only(addr, FOLL_WRITE, page)) {
+	if (get_user_page_fast_only(addr, gup_flags, page)) {
 		*pfn = page_to_pfn(page[0]);
 
 		if (writable)

-- 
Thanks,
Oliver
