On Tue, Jun 11, 2024 at 1:41 AM Sean Christopherson <seanjc@xxxxxxxxxx> 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@xxxxxxxxxx> 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
