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

From: Anish Moorthy <amoorthy@google.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Oliver Upton <oliver.upton@linux.dev>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Marc Zyngier <maz@kernel.org>,
	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 10:19:40 -0800	[thread overview]
Message-ID: <CAF7b7mprKmdoQ=aedfDkN85vi01fskAL_s_ROgkysWuTmSt9FQ@mail.gmail.com> (raw)
In-Reply-To: <Y+0QRsZ4yWyUdpnc@google.com>

On Wed, Feb 15, 2023 at 1:02 AM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> 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)

Good idea, will do.

On Wed, Feb 15, 2023 at 9:03 AM Sean Christopherson <seanjc@google.com> wrote:
>
> State what the patch does, avoid pronouns, and especially don't have "This commmit"
> or "This patch" anywhere.  From Documentation/process/submitting-patches.rst:
>
>   Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
>   instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
>   to do frotz", as if you are giving orders to the codebase to change
>   its behaviour.
>
> Heh, this whole series just screams "google3". :-)
>
> Anish, please read through
>
>   Documentation/process/coding-style.rst
>
> and
>
>   Documentation/process/submitting-patches.rst
>
> particularaly the "Describe your changes" and "Style-check your changes" your
> changes sections.  Bonus points if you work through the mostly redundant process/
> documentation, e.g. these have supplementary info.
>
>   Documentation/process/4.Coding.rst
>   Documentation/process/5.Posting.rst

Thanks for the resources Sean- I'll go through the series and rework the commit
messages/documentation appropriately.