On Mon, Jul 21, 2025, Fuad Tabba wrote: > Hi Sean, > > On Mon, 21 Jul 2025 at 17:45, Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > > > On Thu, Jul 17, 2025, Fuad Tabba wrote: > > > Introduce a new boolean member, supports_gmem, to kvm->arch. > > > > > > Previously, the has_private_mem boolean within kvm->arch was implicitly > > > used to indicate whether guest_memfd was supported for a KVM instance. > > > However, with the broader support for guest_memfd, it's not exclusively > > > for private or confidential memory. Therefore, it's necessary to > > > distinguish between a VM's general guest_memfd capabilities and its > > > support for private memory. > > > > > > This new supports_gmem member will now explicitly indicate guest_memfd > > > support for a given VM, allowing has_private_mem to represent only > > > support for private memory. > > > > > > Reviewed-by: Ira Weiny <ira.weiny@xxxxxxxxx> > > > Reviewed-by: Gavin Shan <gshan@xxxxxxxxxx> > > > Reviewed-by: Shivank Garg <shivankg@xxxxxxx> > > > Reviewed-by: Vlastimil Babka <vbabka@xxxxxxx> > > > Reviewed-by: Xiaoyao Li <xiaoyao.li@xxxxxxxxx> > > > Co-developed-by: David Hildenbrand <david@xxxxxxxxxx> > > > Signed-off-by: David Hildenbrand <david@xxxxxxxxxx> > > > Signed-off-by: Fuad Tabba <tabba@xxxxxxxxxx> > > > > NAK, this introduces unnecessary potential for bugs, e.g. KVM will get a false > > negative if kvm_arch_supports_gmem() is invoked before kvm_x86_ops.vm_init(). > > > > Patch 2 makes this a moot point because kvm_arch_supports_gmem() can simply go away. > > Just to reiterate, this is a NAK to the whole patch Ya, in effect. Well, more specifically to adding arch.supports_gmem, not to the idea of support guest_memfd broadly. > (which if I recall correctly, you had suggested), Sort of[*]. In that thread, I was reacting to the (ab)use of has_private_mem. And FWIW, I was envisioning supports_gmem being set in common x86.c super early on, though what pushed me into NAK territory was seeing the final usage, where "optimizing" kvm_arch_supports_gmem() isn't worth any amount of complexity. : And then rather than rename has_private_mem, either add supports_gmem or do what : you did for kvm_arch_supports_gmem_shared_mem() and explicitly check the VM type. [*] https://lore.kernel.org/all/aEyLlbyMmNEBCAVj@xxxxxxxxxx > since the newer patch that you propose makes this patch, and the function > kvm_arch_supports_gmem() unnecessary.
