Hi, First I want to mention that I really appreciate your feedback, which has already been quite helpful. I would like you to please consider this to be an RFC, and let's use these patches as a basis for discussion and how they can be improved when I respin them, even if that means waiting until the kvm fd-based proposal is finalized. Now to answer your question... <snip> > > My reasoning for allocating all memory with memfd is that it's one > > ring to rule them all :) By that I mean, with memfd, we can allocate > > normal memory, hugetlb memory, in the future guest private memory, and > > easily expand it to support things like IPC memory sharing in the > > future. > > Allocating anonymous memory is more complex now. And I could argue than the > hugetlbfs case is also more complex because there are now two branches that > do different things based whether it's hugetlbfs or not, instead of one. The additional complexity now comes not from using memfd, but from unmapping and aligning code, which I think does benefit kvmtool in general. Hugetlbfd already had a different path before, now at least the other path it has just has to do with setting flags for memfd_create(), rather than allocating memory differently. > As I stands right now, my opinion is that using memfd for anonymous RAM > only adds complexity for zero benefits. > > > > > > > > > > > > Moreover, using an fd would be more generic and flexible, which allows > > > > for other use cases (such as IPC), or to map that memory in userspace > > > > when appropriate. It also allows us to use the same interface for > > > > hugetlb. Considering that other VMMs (e.g., qemu [2], crosvm [3]) > > > > already back guest memory with memfd, and looking at how private > > > > memory would work [4], it seemed to me that the best way to unify all > > > > of these needs is to have the backend of guest memory be fd-based. > > > > > > > > It would be possible to have that as a separate kvmtool option, where > > > > fd-backed memory would be only for guests that use the new private > > > > memory extensions. However, that would mean more code to maintain that > > > > is essentially doing the same thing (allocating and mapping memory). > > > > > > > > I thought that it would be worth having these patches in kvmtool now > > > > rather than wait until the guest private memory has made it into kvm. > > > > These patches simplify the code as an end result, make it easier to > > > > > > In the non-hugetlbfs case, before: > > > > > > kvm->arch.ram_alloc_size = kvm->ram_size + SZ_2M; > > > kvm->arch.ram_alloc_start = mmap_anon_or_hugetlbfs(kvm, kvm->cfg.hugetlbfs_path, kvm->arch.ram_alloc_size); > > > > > > /* > > > * mmap_anon_or_hugetlbfs expands to: > > > * getpagesize() > > > * mmap() > > > */ > > > > > > kvm->ram_start = (void *)ALIGN((unsigned long)kvm->arch.ram_alloc_start, SZ_2M); > > > > > > After: > > > /* mmap_anon_or_hugetlbfs: */ > > > getpagesize(); > > > mmap(NULL, total_map, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); > > > memfd_alloc(size, htlbfs_path, blk_size); > > > > > > /* > > > * memfd_alloc() expands to: > > > * memfd_create() > > > * ftruncate > > > */ > > > > > > addr_align = (void *)ALIGN((u64)addr_map, align_sz); > > > mmap(addr_align, size, PROT_RW, MAP_PRIVATE | MAP_FIXED, fd, 0); > > > > > > I'm counting one extra mmap(), one memfd_create() and one ftruncate() that > > > this series adds (not to mention all the boiler plate code to check for > > > errors). > > > > > > Let's use another metric, let's count the number of lines of code. Before: > > > 9 lines of code, after: -3 lines removed from arm/kvm.c and 86 lines of > > > code for memfd_alloc() and mmap_anon_or_hugetlbfs_align(). > > > > > > I'm struggling to find a metric by which the resulting code is simpler, as > > > you suggest. > > > > With simpler I didn't mean fewer lines of code, rather that it's > > easier to reason about, more shared code. With this series, hugetlb > > How is all of the code that has been added easier to reason about than one > single mmap call? > > > and normal memory creation follow the same path, and with the > > refactoring a lot of arch-specific code is gone. > > Can you point me to the arch-specific code that this series removes? As far > as I can tell, the only arch specfic change is replacing > kvm_arch_delete_ram with kvm_delete_ram, which can be done independently of > this series. If it's only that function, I wouldn't call that "a lot" of > arch-specific code. kvmtool is an old and well established project. So I think that being able to remove the memory-alignment code from the arm and riscv kvm.c, two fields from the arm and riscv struct kvm_arch, as well as kvm__arch_delete_ram from all architectures, is not that little for a mature project such as this one. I agree that this could have been done without using memfd, but at least for me, as a person who has just posted their first contribution to kvmtool, it was easier to make these changes when the tracking of the memory is based on an fd rather than a userspace address (makes alignment and unmapping unused memory much easier). > > > > > > > > > > allocate and map aligned memory without overallocating, and bring > > > > > > If your goal is to remove the overallocting of memory, you can just munmap > > > the extra memory after alignment is performed. To do that you don't need to > > > allocate everything using a memfd. > > > > > > > kvmtool closer to a more consistent way of allocating guest memory, in > > > > a similar manner to other VMMs. > > > > > > I would really appreciate pointing me to where qemu allocates memory using > > > memfd when invoked with -m <size>. I was able to follow the hostmem-ram > > > backend allocation function until g_malloc0(), but I couldn't find the > > > implementation for that. > > > > You're right. I thought that the memfd backend was the default, but > > looking again it's not. > > > > > > > > > > Moreover, with the private memory proposal [1], whether the fd-based > > > > support available can be queried by a KVM capability. If it's > > > > available kvmtool would use the fd, if it's not available, it would > > > > use the host-mapped address. Therefore, there isn’t a need for a > > > > command line option, unless for testing. > > > > > > Why would anyone want to use private memory by default for a > > > non-confidential VM? > > > > The idea is that, at least when pKVM is enabled, we would use the > > proposed extensions for all guests, i.e., memory via a file > > descriptor, regardless whether the guest is protected (thus the memory > > would be private), or not. > > kvmtool can be used to run virtual machines when pKVM is not enabled. In > fact, it has been used that way for way longer than pKVM has existed. What > about those users? This does not affect these users, which is the point of these patches. This allows new uses as well as maintaining the existing one, and enables potentially new ones in the future. > > > > > > > > > > > > I have implemented this all the way to support the private memory > > > > proposal in kvmtool [5], but I haven’t posted these since the private > > > > memory proposal itself is still in flux. If you’re interested you > > > > > > Are you saying that you are not really sure how the userspace API will end > > > up looking? If that's the case, wouldn't it make more sense to wait for the > > > API to stabilize and then send support for it as one nice series? > > > > Yes, I'm not sure how it will end up looking. We know that it will be > > fd-based though, which is why I thought it might be good to start with > > that. > > If you're not sure how it will end up looking, then why change kvmtool now? Because we are sure that it will be fd-based, and because I thought that getting a head start to set the scene would be helpful. The part that is uncertain is the kvm capabilities, flags, and names of the new memory region extensions, none of which I address in these patches. Cheers, /fuad > Thanks, > Alex > > > > > Cheers, > > /fuad > > > > > > > > > Thanks, > > > Alex > > > > > > > could have a look on how I would go ahead building on these patches > > > > for full support of private memory backed by an fd. > > > > > > > > > Regarding IPC memory sharing, is mmap'ing an memfd file enough to enable > > > > > that? If more work is needed for it, then wouldn't it make more sense to do > > > > > all the changes at once? This change might look sensible right now, but it > > > > > might turn out that it was the wrong way to go about it when someone > > > > > actually starts implementing memory sharing. > > > > > > > > I don’t plan on supporting IPC memory sharing. I just mentioned that > > > > as yet another use case that would benefit from guest memory being > > > > fd-based, should kvmtool decide to support it in the future. > > > > > > > > Cheers, > > > > /fuad > > > > > > > > [1] https://lore.kernel.org/all/20221025151344.3784230-1-chao.p.peng@xxxxxxxxxxxxxxx/ > > > > [2] https://github.com/qemu/qemu > > > > [3] https://chromium.googlesource.com/chromiumos/platform/crosvm/ > > > > [4] https://github.com/chao-p/qemu/tree/privmem-v9 > > > > [5] https://android-kvm.googlesource.com/kvmtool/+/refs/heads/tabba/fdmem-v9-core > > > > > > > > > > > > > > > > > > > > > > Regarding IPC memory sharing, is mmap'ing an memfd file enough to enable > > > > > that? If more work is needed for it, then wouldn't it make more sense to do > > > > > all the changes at once? This change might look sensible right now, but it > > > > > might turn out that it was the wrong way to go about it when someone > > > > > actually starts implementing memory sharing. > > > > > > > > > > Thanks, > > > > > Alex > > > > > > > > > > > > > > > > > Signed-off-by: Fuad Tabba <tabba@xxxxxxxxxx> > > > > > > > > > > > > [*] https://lore.kernel.org/all/20221025151344.3784230-1-chao.p.peng@xxxxxxxxxxxxxxx/ > > > > > > --- > > > > > > include/kvm/kvm.h | 1 + > > > > > > include/kvm/util.h | 3 +++ > > > > > > kvm.c | 4 ++++ > > > > > > util/util.c | 33 ++++++++++++++++++++------------- > > > > > > 4 files changed, 28 insertions(+), 13 deletions(-) > > > > > > > > > > > > diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h > > > > > > index 3872dc6..d0d519b 100644 > > > > > > --- a/include/kvm/kvm.h > > > > > > +++ b/include/kvm/kvm.h > > > > > > @@ -87,6 +87,7 @@ struct kvm { > > > > > > struct kvm_config cfg; > > > > > > int sys_fd; /* For system ioctls(), i.e. /dev/kvm */ > > > > > > int vm_fd; /* For VM ioctls() */ > > > > > > + int ram_fd; /* For guest memory. */ > > > > > > timer_t timerid; /* Posix timer for interrupts */ > > > > > > > > > > > > int nrcpus; /* Number of cpus to run */ > > > > > > diff --git a/include/kvm/util.h b/include/kvm/util.h > > > > > > index 61a205b..369603b 100644 > > > > > > --- a/include/kvm/util.h > > > > > > +++ b/include/kvm/util.h > > > > > > @@ -140,6 +140,9 @@ static inline int pow2_size(unsigned long x) > > > > > > } > > > > > > > > > > > > struct kvm; > > > > > > +int memfd_alloc(u64 size, bool hugetlb, u64 blk_size); > > > > > > +void *mmap_anon_or_hugetlbfs_align(struct kvm *kvm, const char *htlbfs_path, > > > > > > + u64 size, u64 align); > > > > > > void *mmap_anon_or_hugetlbfs(struct kvm *kvm, const char *htlbfs_path, u64 size); > > > > > > > > > > > > #endif /* KVM__UTIL_H */ > > > > > > diff --git a/kvm.c b/kvm.c > > > > > > index 78bc0d8..ed29d68 100644 > > > > > > --- a/kvm.c > > > > > > +++ b/kvm.c > > > > > > @@ -160,6 +160,7 @@ struct kvm *kvm__new(void) > > > > > > mutex_init(&kvm->mem_banks_lock); > > > > > > kvm->sys_fd = -1; > > > > > > kvm->vm_fd = -1; > > > > > > + kvm->ram_fd = -1; > > > > > > > > > > > > #ifdef KVM_BRLOCK_DEBUG > > > > > > kvm->brlock_sem = (pthread_rwlock_t) PTHREAD_RWLOCK_INITIALIZER; > > > > > > @@ -174,6 +175,9 @@ int kvm__exit(struct kvm *kvm) > > > > > > > > > > > > kvm__arch_delete_ram(kvm); > > > > > > > > > > > > + if (kvm->ram_fd >= 0) > > > > > > + close(kvm->ram_fd); > > > > > > + > > > > > > list_for_each_entry_safe(bank, tmp, &kvm->mem_banks, list) { > > > > > > list_del(&bank->list); > > > > > > free(bank); > > > > > > diff --git a/util/util.c b/util/util.c > > > > > > index d3483d8..278bcc2 100644 > > > > > > --- a/util/util.c > > > > > > +++ b/util/util.c > > > > > > @@ -102,36 +102,38 @@ static u64 get_hugepage_blk_size(const char *htlbfs_path) > > > > > > return sfs.f_bsize; > > > > > > } > > > > > > > > > > > > -static void *mmap_hugetlbfs(struct kvm *kvm, const char *htlbfs_path, u64 size, u64 blk_size) > > > > > > +int memfd_alloc(u64 size, bool hugetlb, u64 blk_size) > > > > > > { > > > > > > const char *name = "kvmtool"; > > > > > > unsigned int flags = 0; > > > > > > int fd; > > > > > > - void *addr; > > > > > > - int htsize = __builtin_ctzl(blk_size); > > > > > > > > > > > > - if ((1ULL << htsize) != blk_size) > > > > > > - die("Hugepage size must be a power of 2.\n"); > > > > > > + if (hugetlb) { > > > > > > + int htsize = __builtin_ctzl(blk_size); > > > > > > > > > > > > - flags |= MFD_HUGETLB; > > > > > > - flags |= htsize << MFD_HUGE_SHIFT; > > > > > > + if ((1ULL << htsize) != blk_size) > > > > > > + die("Hugepage size must be a power of 2.\n"); > > > > > > + > > > > > > + flags |= MFD_HUGETLB; > > > > > > + flags |= htsize << MFD_HUGE_SHIFT; > > > > > > + } > > > > > > > > > > > > fd = memfd_create(name, flags); > > > > > > if (fd < 0) > > > > > > - die("Can't memfd_create for hugetlbfs map\n"); > > > > > > + die("Can't memfd_create for memory map\n"); > > > > > > + > > > > > > if (ftruncate(fd, size) < 0) > > > > > > die("Can't ftruncate for mem mapping size %lld\n", > > > > > > (unsigned long long)size); > > > > > > - addr = mmap(NULL, size, PROT_RW, MAP_PRIVATE, fd, 0); > > > > > > - close(fd); > > > > > > > > > > > > - return addr; > > > > > > + return fd; > > > > > > } > > > > > > > > > > > > /* This function wraps the decision between hugetlbfs map (if requested) or normal mmap */ > > > > > > void *mmap_anon_or_hugetlbfs(struct kvm *kvm, const char *htlbfs_path, u64 size) > > > > > > { > > > > > > u64 blk_size = 0; > > > > > > + int fd; > > > > > > > > > > > > /* > > > > > > * We don't /need/ to map guest RAM from hugetlbfs, but we do so > > > > > > @@ -146,9 +148,14 @@ void *mmap_anon_or_hugetlbfs(struct kvm *kvm, const char *htlbfs_path, u64 size) > > > > > > } > > > > > > > > > > > > kvm->ram_pagesize = blk_size; > > > > > > - return mmap_hugetlbfs(kvm, htlbfs_path, size, blk_size); > > > > > > } else { > > > > > > kvm->ram_pagesize = getpagesize(); > > > > > > - return mmap(NULL, size, PROT_RW, MAP_ANON_NORESERVE, -1, 0); > > > > > > } > > > > > > + > > > > > > + fd = memfd_alloc(size, htlbfs_path, blk_size); > > > > > > + if (fd < 0) > > > > > > + return MAP_FAILED; > > > > > > + > > > > > > + kvm->ram_fd = fd; > > > > > > + return mmap(NULL, size, PROT_RW, MAP_PRIVATE, kvm->ram_fd, 0); > > > > > > } > > > > > > -- > > > > > > 2.38.1.431.g37b22c650d-goog > > > > > >
