Suzuki K Poulose <[email protected]> writes: > > [...snip...] > >> +static int __kvm_gmem_set_attributes(struct inode *inode, pgoff_t start, >> + size_t nr_pages, uint64_t attrs) >> +{ >> + struct address_space *mapping = inode->i_mapping; >> + struct gmem_inode *gi = GMEM_I(inode); >> + pgoff_t end = start + nr_pages; >> + struct maple_tree *mt; >> + struct ma_state mas; >> + int r; >> + >> + mt = &gi->attributes; >> + >> + filemap_invalidate_lock(mapping); >> + >> + mas_init(&mas, mt, start); >> + r = kvm_gmem_mas_preallocate(&mas, attrs, start, nr_pages); >> + if (r) >> + goto out; >> + >> + /* >> + * From this point on guest_memfd has performed necessary >> + * checks and can proceed to do guest-breaking changes. >> + */ >> + >> + kvm_gmem_invalidate_start(inode, start, end); > > I added support for Arm CCA KVM patches with the inplace conversion and > I am hitting the following issue. > > 1. I am supporting INIT_SHARED + MMAP flags. > 2. VMM creates the Gmem_fd with both the flags above. > 3. Uses the shared gmem-mmap to load the initial payloads (kernel, dtb). > 4. At the VM finalization time, Populate the loaded regions one by one > by > a) copying the images to a temparory buffer - Since CCA can't really > load the contents in-place.
Sounds good :). I see that you blocked this in the kernel by returning -EOPNOTSUPP if (!src_page) [0]. > b) Set the "region" to Private in the gmem_fd (via > SET_MEMORY_ATTRIBUTES2) > c) Invoke CCA backend to populate the private memory via > ioctl(KVM_ARM_RMI_POPULATE,..) [0] > This flow sounds right. > [0] > https://lore.kernel.org/all/[email protected]/ > > > 5. Additionally, VMM can mark the entire RAM to be private before the VM > starts running, again via SET_MEMORY_ATTRIBUTES2. On CCA, this > action is measured and doesn't require the Host to "commit" memory to > the VM. > Instead the host can lazily donate memory on a fault. > For both TDX and SNP, the host can also lazily donate memory, guest_memfd supports this. > But step (5) triggers the invalidation of both private and shared > mappings of the gmem area, from the kvm_gmem_invalidate_start() > above. > > This is because, the entire DRAM now has, some portions PRIVATE (the > loaded regions) and the rest are SHARED (from the Gmem_fd creation). > Thus, kvm_gmem_get_invalidate_filter(Dram_start, Dram_end) causes the > invalidation of both "PRIVATE" and "SHARED" regions, which results > in the destruction of the already loaded data and things go south. > This destruction will happen for TDX as well. I think we managed to get around this because we didn't apply conversion on the already-private ranges. IIUC on SNP, zapping pages in the stage 2 page tables doesn't destroy the data, so that's probably why it has been fine for SNP. > When we know that the kvm_gmem_invalidate_xx is triggered by a > conversion, we don't need to invalidate the existing pages that > are in the requested state. i.e., the following patch on top of > this series does the trick for me : > > > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c > index a97fcac34a0e..62e0427a49f4 100644 > --- a/virt/kvm/guest_memfd.c > +++ b/virt/kvm/guest_memfd.c > @@ -250,16 +250,23 @@ static void __kvm_gmem_invalidate_start(struct > gmem_file *f, pgoff_t start, > KVM_MMU_UNLOCK(kvm); > } > > +static void kvm_gmem_invalidate_start_filter(struct inode *inode, > pgoff_t start, > + pgoff_t end, > + enum kvm_gfn_range_filter > attr_filter) > +{ > + struct gmem_file *f; > + > + kvm_gmem_for_each_file(f, inode) > + __kvm_gmem_invalidate_start(f, start, end, attr_filter); > +} > + > static void kvm_gmem_invalidate_start(struct inode *inode, pgoff_t start, > pgoff_t end) > { > enum kvm_gfn_range_filter attr_filter; > - struct gmem_file *f; > - > attr_filter = kvm_gmem_get_invalidate_filter(inode, start, end); > > - kvm_gmem_for_each_file(f, inode) > - __kvm_gmem_invalidate_start(f, start, end, attr_filter); > + kvm_gmem_invalidate_start_filter(inode, start, end, attr_filter); > } > > static void __kvm_gmem_invalidate_end(struct gmem_file *f, pgoff_t start, > @@ -724,9 +731,14 @@ static int __kvm_gmem_set_attributes(struct inode > *inode, pgoff_t start, > /* > * From this point on guest_memfd has performed necessary > * checks and can proceed to do guest-breaking changes. > + * Also, we don't have to invalidate the regions that > + * may already be in the requested state. Hence, we could > + * explicitly filter the invalidations to the opposite > + * state. > */ > > - kvm_gmem_invalidate_start(inode, start, end); > + kvm_gmem_invalidate_start_filter(inode, start, end, > + to_private ? KVM_FILTER_SHARED : > KVM_FILTER_PRIVATE); > I think this makes sense. Thanks for catching this. > if (!to_private) > kvm_gmem_invalidate(inode, start, end); > > > Thoughts ? > > Suzuki > > >> >> [...snip...] >>
