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 (which if I recall
correctly, you had suggested), since the newer patch that you propose
makes this patch, and the function kvm_arch_supports_gmem()
unnecessary.
Fewer patches is fine by me :)
Thanks,
/fuad
> > ---
> > arch/x86/include/asm/kvm_host.h | 3 ++-
> > arch/x86/kvm/svm/svm.c | 1 +
> > arch/x86/kvm/vmx/tdx.c | 1 +
> > arch/x86/kvm/x86.c | 4 ++--
> > 4 files changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index bde811b2d303..938b5be03d33 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1348,6 +1348,7 @@ struct kvm_arch {
> > u8 mmu_valid_gen;
> > u8 vm_type;
> > bool has_private_mem;
> > + bool supports_gmem;
> > bool has_protected_state;
> > bool pre_fault_allowed;
> > struct hlist_head mmu_page_hash[KVM_NUM_MMU_PAGES];
> > @@ -2277,7 +2278,7 @@ void kvm_configure_mmu(bool enable_tdp, int tdp_forced_root_level,
> >
> > #ifdef CONFIG_KVM_GMEM
> > #define kvm_arch_has_private_mem(kvm) ((kvm)->arch.has_private_mem)
> > -#define kvm_arch_supports_gmem(kvm) kvm_arch_has_private_mem(kvm)
> > +#define kvm_arch_supports_gmem(kvm) ((kvm)->arch.supports_gmem)
> > #else
> > #define kvm_arch_has_private_mem(kvm) false
> > #define kvm_arch_supports_gmem(kvm) false
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index ab9b947dbf4f..d1c484eaa8ad 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -5181,6 +5181,7 @@ static int svm_vm_init(struct kvm *kvm)
> > to_kvm_sev_info(kvm)->need_init = true;
> >
> > kvm->arch.has_private_mem = (type == KVM_X86_SNP_VM);
> > + kvm->arch.supports_gmem = (type == KVM_X86_SNP_VM);
> > kvm->arch.pre_fault_allowed = !kvm->arch.has_private_mem;
> > }
> >
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index f31ccdeb905b..a3db6df245ee 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -632,6 +632,7 @@ int tdx_vm_init(struct kvm *kvm)
> >
> > kvm->arch.has_protected_state = true;
> > kvm->arch.has_private_mem = true;
> > + kvm->arch.supports_gmem = true;
> > kvm->arch.disabled_quirks |= KVM_X86_QUIRK_IGNORE_GUEST_PAT;
> >
> > /*
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 357b9e3a6cef..adbdc2cc97d4 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -12780,8 +12780,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> > return -EINVAL;
> >
> > kvm->arch.vm_type = type;
> > - kvm->arch.has_private_mem =
> > - (type == KVM_X86_SW_PROTECTED_VM);
> > + kvm->arch.has_private_mem = (type == KVM_X86_SW_PROTECTED_VM);
> > + kvm->arch.supports_gmem = (type == KVM_X86_SW_PROTECTED_VM);
> > /* Decided by the vendor code for other VM types. */
> > kvm->arch.pre_fault_allowed =
> > type == KVM_X86_DEFAULT_VM || type == KVM_X86_SW_PROTECTED_VM;
> > --
> > 2.50.0.727.gbf7dc18ff4-goog
> >
