On Thu, Apr 25, 2024 at 12:32 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > On Thu, Apr 04, 2024, Paolo Bonzini wrote: > > +long kvm_gmem_populate(struct kvm *kvm, gfn_t gfn, void __user *src, long npages, > > + int (*post_populate)(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn, > > + void __user *src, int order, void *opaque), > > Add a typedef for callback? If only to make this prototype readable. > > > +long kvm_gmem_populate(struct kvm *kvm, gfn_t gfn, void __user *src, long npages, > > + int (*post_populate)(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn, > > + void __user *src, int order, void *opaque), > > + void *opaque) > > +{ > > + struct file *file; > > + struct kvm_memory_slot *slot; > > + > > + int ret = 0, max_order; > > + long i; > > + > > + lockdep_assert_held(&kvm->slots_lock); > > + if (npages < 0) > > + return -EINVAL; > > + > > + slot = gfn_to_memslot(kvm, gfn); > > + if (!kvm_slot_can_be_private(slot)) > > + return -EINVAL; > > + > > + file = kvm_gmem_get_file(slot); > > + if (!file) > > + return -EFAULT; > > + > > + filemap_invalidate_lock(file->f_mapping); > > + > > + npages = min_t(ulong, slot->npages - (gfn - slot->base_gfn), npages); > > + for (i = 0; i < npages; i += (1 << max_order)) { > > + gfn_t this_gfn = gfn + i; > > KVM usually does something like "start_gfn" or "base_gfn", and then uses "gfn" > for the one gfn that's being processed. And IMO that's much better because the > propotype for kvm_gmem_populate() does not make it obvious that @gfn is the base > of a range, not a singular gfn to process. > > > > + kvm_pfn_t pfn; > > + > > + ret = __kvm_gmem_get_pfn(file, slot, this_gfn, &pfn, &max_order, false); > > + if (ret) > > + break; > > + > > + if (!IS_ALIGNED(this_gfn, (1 << max_order)) || > > + (npages - i) < (1 << max_order)) > > + max_order = 0; > > + > > + if (post_populate) { > > Is there any use for this without @post_populate? I.e. why make this optional? Yeah, it probably does not need to be optional (before, the copy_from_user was optionally done from kvm_gmem_populate, but not anymore). Paolo