Received-SPF: pass (google.com: domain of linux-kernel+bounces-62917-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 12 Feb 2024 19:39:49 -0800
In-Reply-To: <ZcGn0t3l8OCL5mv6@google.com>
Precedence: bulk
Mime-Version: 1.0
References: <20230911021637.1941096-1-stevensd@google.com> <CAD=HUj5733eL9momi=V53njm85BQv_QkVrX92xReiq0_9JhqxQ@mail.gmail.com>
 <ZUEPn_nIoE-gLspp@google.com> <CAD=HUj5g9BoziHT5SbbZ1oFKv75UuXoo32x8DC3TYgLGZ6G_Bw@mail.gmail.com>
 <ZYJFPoFYkp4xajRO@google.com> <ZcGn0t3l8OCL5mv6@google.com>
Message-ID: <ZcrkhTn1Da5-vND2@google.com>
Subject: Re: [PATCH v9 0/6] KVM: allow mapping non-refcounted pages
From: Sean Christopherson <seanjc@google.com>
To: David Stevens <stevensd@chromium.org>
Cc: kvmarm@lists.linux.dev, linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Feb 05, 2024, Sean Christopherson wrote:
> On Tue, Dec 19, 2023, Sean Christopherson wrote:
> > On Tue, Dec 12, 2023, David Stevens wrote:
> > > On Tue, Oct 31, 2023 at 11:30=E2=80=AFPM Sean Christopherson <seanjc@=
google.com> wrote:
> > > >
> > > > On Tue, Oct 31, 2023, David Stevens wrote:
> > > > > Sean, have you been waiting for a new patch series with responses=
 to
> > > > > Maxim's comments? I'm not really familiar with kernel contributio=
n
> > > > > etiquette, but I was hoping to get your feedback before spending =
the
> > > > > time to put together another patch series.
> > > >
> > > > No, I'm working my way back toward it.  The guest_memfd series took=
 precedence
> > > > over everything that I wasn't confident would land in 6.7, i.e. lar=
ger series
> > > > effectively got put on the back burner.  Sorry :-(
> > >=20
> > > Is this series something that may be able to make it into 6.8 or 6.9?
> >=20
> > 6.8 isn't realistic.  Between LPC, vacation, and non-upstream stuff, I'=
ve done
> > frustratingly little code review since early November.  Sorry :-(
> >=20
> > I haven't paged this series back into memory, so take this with a grain=
 of salt,
> > but IIRC there was nothing that would block this from landing in 6.9.  =
Timing will
> > likely be tight though, especially for getting testing on all architect=
ures.
>=20
> I did a quick-ish pass today.  If you can hold off on v10 until later thi=
s week,
> I'll try to take a more in-depth look by EOD Thursday.

So I took a "deeper" look, but honestly it wasn't really any more in-depth =
than
the previous look.  I think I was somewhat surprised at the relatively smal=
l amount
of churn this ended up requiring.

Anywho, no major complaints.  This might be fodder for 6.9?  Maybe.  It'll =
be
tight.  At the very least though, I expect to shove v10 in a branch and sta=
rt
beating on it in anticipation of landing it no later than 6.10.

One question though: what happened to the !FOLL_GET logic in kvm_follow_ref=
counted_pfn()?

In a previous version, I suggested:

  static kvm_pfn_t kvm_follow_refcounted_pfn(struct kvm_follow_pfn *foll,
                                             struct page *page)
  {
       kvm_pfn_t pfn =3D page_to_pfn(page);

       foll->is_refcounted_page =3D true;

       /*
        * FIXME: Ideally, KVM wouldn't pass FOLL_GET to gup() when the call=
er
        * doesn't want to grab a reference, but gup() doesn't support getti=
ng
        * just the pfn, i.e. FOLL_GET is effectively mandatory.  If that ev=
er
        * changes, drop this and simply don't pass FOLL_GET to gup().
        */
       if (!(foll->flags & FOLL_GET))
               put_page(page);

       return pfn;
  }

but in v9 it's simply:

  static kvm_pfn_t kvm_follow_refcounted_pfn(struct kvm_follow_pfn *foll,
					     struct page *page)
  {
	kvm_pfn_t pfn =3D page_to_pfn(page);

	foll->is_refcounted_page =3D true;
	return pfn;
  }

And instead the x86 page fault handlers manually drop the reference.  Why i=
s that?