Message-ID: <CABgObfbvhz10ikVbeguUwSDpxFr+wm73cTZCKU9JRDyZdUJapQ@mail.gmail.com>
Date: Tue, 11 Jun 2024 08:09:24 +0200
From: Paolo Bonzini <pbonzini@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Isaku Yamahata <isaku.yamahata@...el.com>, linux-kernel@...r.kernel.org, 
	kvm@...r.kernel.org, michael.roth@....com, isaku.yamahata@...ux.intel.com
Subject: Re: [PATCH 09/11] KVM: guest_memfd: Add interface for populating gmem
 pages with user data

On Tue, Jun 11, 2024 at 1:41 AM Sean Christopherson <seanjc@...gle.com> wrote:
>
> On Tue, Jun 11, 2024, Paolo Bonzini wrote:
> > On 6/10/24 23:48, Paolo Bonzini wrote:
> > > On Sat, Jun 8, 2024 at 1:03 AM Sean Christopherson <seanjc@...gle.com> wrote:
> > > > SNP folks and/or Paolo, what's the plan for this?  I don't see how what's sitting
> > > > in kvm/next can possibly be correct without conditioning population on the folio
> > > > being !uptodate.
> > >
> > > I don't think I have time to look at it closely until Friday; but
> > > thanks for reminding me.
> >
> > Ok, I'm officially confused.  I think I understand what you did in your
> > suggested code.  Limiting it to the bare minimum (keeping the callback
> > instead of CONFIG_HAVE_KVM_GMEM_INITIALIZE) it would be something
> > like what I include at the end of the message.
> >
> > But the discussion upthread was about whether to do the check for
> > RMP state in sev.c, or do it in common code using folio_mark_uptodate().
> > I am not sure what you mean by "cannot possibly be correct", and
> > whether it's referring to kvm_gmem_populate() in general or the
> > callback in sev_gmem_post_populate().
>
> Doing fallocate() before KVM_SEV_SNP_LAUNCH_UPDATE will cause the latter to fail.
> That likely works for QEMU, at least for now, but it's unnecessarily restrictive
> and IMO incorrect/wrong.

Ok, I interpreted incorrect as if it caused incorrect initialization
or something similarly fatal.  Being too restrictive can (almost)
always be lifted.

> E.g. a more convoluted, fallocate() + PUNCH_HOLE + KVM_SEV_SNP_LAUNCH_UPDATE will
> work (I think?  AFAICT adding and removing pages directly to/from the RMP doesn't
> affect SNP's measurement, only pages that are added via SNP_LAUNCH_UPDATE affect
> the measurement).

So the starting point is writing testcases (for which indeed I have to
wait until Friday).  It's not exactly a rewrite but almost.

> Punting the sanity check to vendor code is also gross and will make it harder to
> provide a consistent, unified ABI for all architectures.  E.g. SNP returns -EINVAL
> if the page is already assigned, which is quite misleading.
>
> > The change below looks like just an optimization to me, which
> > suggests that I'm missing something glaring.
>
> I really dislike @prepare.  There are two paths that should actually initialize
> the contents of the folio, and they are mutually exclusive and have meaningfully
> different behavior.  Faulting in memory via kvm_gmem_get_pfn() explicitly zeros
> the folio _if necessary_, whereas kvm_gmem_populate() initializes the folio with
> user-provided data _and_ requires that the folio be !uptodate.

No complaints there, I just wanted to start with the minimal change to
use the uptodate flag in kvm_gmem_populate(). And yeah,
kvm_gmem_get_folio() at this point can be basically replaced by
filemap_grab_folio() in the kvm_gmem_populate() path. What I need to
think about, is that there is still quite a bit of code in
__kvm_gmem_get_pfn() that is common to both paths.

Paolo

Powered by blists - more mailing lists