On Wed, 23 Jul 2025 at 00:42, Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > On Tue, Jul 22, 2025, Fuad Tabba wrote: > > On Tue, 22 Jul 2025 at 16:58, Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > > > > > On Tue, Jul 22, 2025, Fuad Tabba wrote: > > > > On Mon, 21 Jul 2025 at 18:33, Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > > > > > > > > > On Mon, Jul 21, 2025, Fuad Tabba wrote: > > > > > > > The below diff applies on top. I'm guessing there may be some intermediate > > > > > > > ugliness (I haven't mapped out exactly where/how to squash this throughout the > > > > > > > series, and there is feedback relevant to future patches), but IMO this is a much > > > > > > > cleaner resting state (see the diff stats). > > > > > > > > > > > > So just so that I am clear, applying the diff below to the appropriate > > > > > > patches would address all the concerns that you have mentioned in this > > > > > > email? > > > > > > > > > > Yes? It should, I just don't want to pinky swear in case I botched something. > > > > > > > > Other than this patch not applying, nah, I think it's all good ;P. I > > > > guess base-commit: 9eba3a9ac9cd5922da7f6e966c01190f909ed640 is > > > > somewhere in a local tree of yours. There are quite a few conflicts > > > > and I don't think it would build even if based on the right tree, > > > > e.g., KVM_CAP_GUEST_MEMFD_MMAP is a rename of KVM_CAP_GMEM_MMAP, > > > > rather an addition of an undeclared identifier. > > > > > > > > That said, I think I understand what you mean, and I can apply the > > > > spirit of this patch. > > > > > > > > Stay tuned for v16. > > > > > > Want to point me at your branch? I can run it through my battery of tests, and > > > maybe save you/us from having to spin a v17. > > > > That would be great. Here it is: > > > > https://android-kvm.googlesource.com/linux/+/refs/heads/tabba/guestmem-basic-6.16-v16 > > > > No known issues from my end. But can you have a look at the patch: > > > > KVM: guest_memfd: Consolidate Kconfig and guest_memfd enable checks > > > > In that I collected the changes to the config/enable checks that > > didn't seem to fit well in any of the other patches. > > Regarding config stuff, patch 02, KVM: Rename CONFIG_KVM_GENERIC_PRIVATE_MEM to > CONFIG_HAVE_KVM_ARCH_GMEM_POPULATE, is missing a KVM_GMEM => KVM_GUEST_MEMFD rename. > > While playing with this, I also discovered why this code lives in the KVM_X86 config: > > select KVM_GENERIC_PRIVATE_MEM if KVM_SW_PROTECTED_VM > > Commit ea4290d77bda ("KVM: x86: leave kvm.ko out of the build if no vendor module > is requested") didn't have all the vendor netural configs depend on KVM_X86, and > so it's possible to end up with unmet dependencies. E.g. KVM_SW_PROTECTED_VM can > be selected with KVM_X86=n, and thus with KVM_GUEST_MEMFD=n. > > We could punt on that mess until after this series, but that'd be a even more > churn, and I'm not sure I could stomach giving acks for the continued addition > of ugly kconfig dependencies. :-) > > Lastly, regarding "Consolidate Kconfig and guest_memfd enable checks", that needs > to land before f6a5f3a22bbe ("KVM: guest_memfd: Allow host to map guest_memfd pages"), > otherwise KVM will present a weird state where guest_memfd can be used for default > VMs, but if and only KVM_GUEST_MEMFD happens to be selected by something else. > That also provides a better shortlog: "KVM: x86: Enable KVM_GUEST_MEMFD for all > 64-bit builds". The config cleanups and consolidations are a nice side effect, > but what that patch is really doing is enabling KVM_GUEST_MEMFD more broadly. > > Actually, all of the arch patches need to come before f6a5f3a22bbe ("KVM: guest_memfd: > Allow host to map guest_memfd pages"), otherwise intermediate builds will have > half-baked support for guest_memfd mmap(). Or rather, KVM shouldn't let userspace > enable GUEST_MEMFD_FLAG_MMAP until all the plumbing is in place. I suspect that > trying to shuffle the full patches around will create cyclical dependency hell. > It's easy enough to hold off on adding GUEST_MEMFD_FLAG_MMAP until KVM is fully > ready, so I think it makes sense to just add GUEST_MEMFD_FLAG_MMAP along with the > capability. > > Rather than trying to pass partial patches around, I pushed a branch to: > > https://github.com/sean-jc/linux.git x86/gmem_mmap > > Outside of the x86 config crud, and deferring GUEST_MEMFD_FLAG_MMAP until KVM is > fully prepped, there _shouldn't_ be any changes relatively to what you have. > > Note, it's based on: > > https://github.com/kvm-x86/linux.git next > > as there are x86 kconfig dependencies/conflicts with changes that are destined > for 6.17 (and I don't think landing this in 6.17 is realistic, i.e. this series > will effectively follow kvm-x86/next no matter what). > > I haven't done a ton of runtime testing yet, but it passes all of my build tests > (I have far too many configs), so I'm reasonably confident all the kconfig stuff > isn't horribly broken. > > Oh, and I also squashed this into the very last patch. The curly braces, line > wrap, and hardcoded boolean are all superfluous. Thank you for this. These patches look good to me. I've tested them on x86 and arm64, and everything runs fine. I'll have a closer look at them, and probably send v16 later today. I know that it's probably too late for 6.17, but it would be great if we could queue this for 6.18. Cheers, /fuad > diff --git a/tools/testing/selftests/kvm/guest_memfd_test.c b/tools/testing/selftests/kvm/guest_memfd_test.c > index 4cdccabc160c..a0c5db8fd72d 100644 > --- a/tools/testing/selftests/kvm/guest_memfd_test.c > +++ b/tools/testing/selftests/kvm/guest_memfd_test.c > @@ -249,8 +249,7 @@ static bool check_vm_type(unsigned long vm_type) > return kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(vm_type); > } > > -static void test_with_type(unsigned long vm_type, uint64_t guest_memfd_flags, > - bool expect_mmap_allowed) > +static void test_with_type(unsigned long vm_type, uint64_t guest_memfd_flags) > { > struct kvm_vm *vm; > size_t total_size; > @@ -272,7 +271,7 @@ static void test_with_type(unsigned long vm_type, uint64_t guest_memfd_flags, > > test_file_read_write(fd); > > - if (expect_mmap_allowed) { > + if (guest_memfd_flags & GUEST_MEMFD_FLAG_MMAP) { > test_mmap_supported(fd, page_size, total_size); > test_fault_overflow(fd, page_size, total_size); > > @@ -343,13 +342,11 @@ int main(int argc, char *argv[]) > > test_gmem_flag_validity(); > > - test_with_type(VM_TYPE_DEFAULT, 0, false); > - if (kvm_has_cap(KVM_CAP_GUEST_MEMFD_MMAP)) { > - test_with_type(VM_TYPE_DEFAULT, GUEST_MEMFD_FLAG_MMAP, > - true); > - } > + test_with_type(VM_TYPE_DEFAULT, 0); > + if (kvm_has_cap(KVM_CAP_GUEST_MEMFD_MMAP)) > + test_with_type(VM_TYPE_DEFAULT, GUEST_MEMFD_FLAG_MMAP); > > #ifdef __x86_64__ > - test_with_type(KVM_X86_SW_PROTECTED_VM, 0, false); > + test_with_type(KVM_X86_SW_PROTECTED_VM, 0); > #endif > }
