On Sat, Jul 13, 2024 at 3:28 AM Edgecombe, Rick P <rick.p.edgecombe@xxxxxxxxx> wrote: > On Thu, 2024-07-11 at 18:27 -0400, Paolo Bonzini wrote: > > Do not allow populating the same page twice with startup data. In the > > case of SEV-SNP, for example, the firmware does not allow it anyway, > > since the launch-update operation is only possible on pages that are > > still shared in the RMP. > > > > Even if it worked, kvm_gmem_populate()'s callback is meant to have side > > effects such as updating launch measurements, and updating the same > > page twice is unlikely to have the desired results. > > > > Races between calls to the ioctl are not possible because kvm_gmem_populate() > > holds slots_lock and the VM should not be running. But again, even if > > this worked on other confidential computing technology, it doesn't matter > > to guest_memfd.c whether this is an intentional attempt to do something > > fishy, or missing synchronization in userspace, or even something > > intentional. One of the racers wins, and the page is initialized by > > either kvm_gmem_prepare_folio() or kvm_gmem_populate(). > > > > Anyway, out of paranoia, adjust sev_gmem_post_populate() anyway to use > > the same errno that kvm_gmem_populate() is using. > > This patch breaks our rebased TDX development tree. First > kvm_gmem_prepare_folio() is called during the KVM_PRE_FAULT_MEMORY operation, > then next kvm_gmem_populate() is called during the KVM_TDX_INIT_MEM_REGION ioctl > to actually populate the memory, which hits the new -EEXIST error path. It's not a problem to only keep patches 1-8 for 6.11, and move the rest to 6.12 (except for the bit that returns -EEXIST in sev.c). Could you push a branch for me to take a look? I've never liked that you have to do the explicit prefault before the VM setup is finished; it's a TDX-specific detail that is transpiring into the API. > Given we are not actually populating during KVM_PRE_FAULT_MEMORY and try to > avoid booting a TD until we've done so, maybe setting folio_mark_uptodate() in > kvm_gmem_prepare_folio() is not appropriate in that case? But it may not be easy > to separate. It would be easy (just return a boolean value from kvm_arch_gmem_prepare() to skip folio_mark_uptodate() before the VM is ready, and implement it for TDX) but it's ugly. You're also clearing the memory unnecessarily before overwriting it. Paolo
