Re: [PATCH v14 02/21] KVM: Rename CONFIG_KVM_GENERIC_PRIVATE_MEM to CONFIG_KVM_GENERIC_GMEM_POPULATE

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 7/16/2025 7:15 PM, David Hildenbrand wrote:
> On 16.07.25 13:05, Fuad Tabba wrote:
>> On Wed, 16 Jul 2025 at 12:02, Xiaoyao Li <xiaoyao.li@intel.com> wrote:
>>>
>>> On 7/16/2025 6:25 PM, David Hildenbrand wrote:
>>>> On 16.07.25 10:31, Xiaoyao Li wrote:
>>>>> On 7/16/2025 4:11 PM, Fuad Tabba wrote:
>>>>>> On Wed, 16 Jul 2025 at 05:09, Xiaoyao Li<xiaoyao.li@intel.com> wrote:
>>>>>>> On 7/15/2025 5:33 PM, Fuad Tabba wrote:
>>>>>>>> The original name was vague regarding its functionality. This 
>>>>>>>> Kconfig
>>>>>>>> option specifically enables and gates the kvm_gmem_populate()
>>>>>>>> function,
>>>>>>>> which is responsible for populating a GPA range with guest data.
>>>>>>> Well, I disagree.
>>>>>>>
>>>>>>> The config KVM_GENERIC_PRIVATE_MEM was introduced by commit
>>>>>>> 89ea60c2c7b5
>>>>>>> ("KVM: x86: Add support for "protected VMs" that can utilize private
>>>>>>> memory"), which is a convenient config for vm types that requires
>>>>>>> private memory support, e.g., SNP, TDX, and KVM_X86_SW_PROTECTED_VM.
>>>>>>>
>>>>>>> It was commit e4ee54479273 ("KVM: guest_memfd: let 
>>>>>>> kvm_gmem_populate()
>>>>>>> operate only on private gfns") that started to use
>>>>>>> CONFIG_KVM_GENERIC_PRIVATE_MEM gates kvm_gmem_populate() 
>>>>>>> function. But
>>>>>>> CONFIG_KVM_GENERIC_PRIVATE_MEM is not for kvm_gmem_populate() only.
>>>>>>>
>>>>>>> If using CONFIG_KVM_GENERIC_PRIVATE_MEM to gate 
>>>>>>> kvm_gmem_populate() is
>>>>>>> vague and confusing, we can introduce KVM_GENERIC_GMEM_POPULATE 
>>>>>>> to gate
>>>>>>> kvm_gmem_populate() and select KVM_GENERIC_GMEM_POPULATE under
>>>>>>> CONFIG_KVM_GENERIC_PRIVATE_MEM.
>>>>>>>
>>>>>>> Directly replace CONFIG_KVM_GENERIC_PRIVATE_MEM with
>>>>>>> KVM_GENERIC_GMEM_POPULATE doesn't look correct to me.
>>>>>> I'll quote David's reply to an earlier version of this patch [*]:
>>>>>
>>>>> It's not related to my concern.
>>>>>
>>>>> My point is that CONFIG_KVM_GENERIC_PRIVATE_MEM is used for selecting
>>>>> the private memory support. Rename it to KVM_GENERIC_GMEM_POPULATE is
>>>>> not correct.
>>>>
>>>> It protects a function that is called kvm_gmem_populate().
>>>>
>>>> Can we stop the nitpicking?
>>>
>>> I don't think it's nitpicking.
>>>
>>> Could you loot into why it was named as KVM_GENERIC_PRIVATE_MEM in the
>>> first place, and why it was picked to protect kvm_gmem_populate()?
>>
>> That is, in part, the point of this patch. This flag protects
>> kvm_gmem_populate(), and the name didn't reflect that. Now it does. It
>> is the only thing it protects.
> 
> I'll note that the kconfig makes it clear that it depends on 
> KVM_GENERIC_MEMORY_ATTRIBUTES -- having support for private memory.
> 
> In any case, CONFIG_KVM_GENERIC_PRIVATE_MEM is a bad name: what on earth 
> is generic private memory.

"gmem" + "memory_attribute" is the generic private memory.

If KVM_GENERIC_PRIVATE_MEM is a bad name, we can drop it, but not rename 
it to CONFIG_KVM_GENERIC_GMEM_POPULATE.

> If CONFIG_KVM_GENERIC_GMEM_POPULATE is for some reason I don't 
> understand yet not the right name, can we have something that better 
> expresses that is is about KVM .. GMEM ... and POPULATE?

I'm not objecting the name of CONFIG_KVM_GENERIC_GMEM_POPULATE, but 
objecting the simple rename. Does something below look reasonable?

---
diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
index 2eeffcec5382..3f87dcaaae83 100644
--- a/arch/x86/kvm/Kconfig
+++ b/arch/x86/kvm/Kconfig
@@ -135,6 +135,7 @@ config KVM_INTEL_TDX
         bool "Intel Trust Domain Extensions (TDX) support"
         default y
         depends on INTEL_TDX_HOST
+       select KVM_GENERIC_GMEM_POPULATE
         help
           Provides support for launching Intel Trust Domain Extensions 
(TDX)
           confidential VMs on Intel processors.
@@ -158,6 +159,7 @@ config KVM_AMD_SEV
         depends on CRYPTO_DEV_SP_PSP && !(KVM_AMD=y && CRYPTO_DEV_CCP_DD=m)
         select ARCH_HAS_CC_PLATFORM
         select KVM_GENERIC_PRIVATE_MEM
+       select KVM_GENERIC_GMEM_POPULATE
         select HAVE_KVM_ARCH_GMEM_PREPARE
         select HAVE_KVM_ARCH_GMEM_INVALIDATE
         help
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 755b09dcafce..359baaae5e9f 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -2556,7 +2556,7 @@ static inline int kvm_gmem_get_pfn(struct kvm *kvm,
  int kvm_arch_gmem_prepare(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn, 
int max_order);
  #endif

-#ifdef CONFIG_KVM_GENERIC_PRIVATE_MEM
+#ifdef CONFIG_KVM_GENERIC_GMEM_POPULATE
  /**
   * kvm_gmem_populate() - Populate/prepare a GPA range with guest data
   *
diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
index 49df4e32bff7..9b37ca009a22 100644
--- a/virt/kvm/Kconfig
+++ b/virt/kvm/Kconfig
@@ -121,6 +121,10 @@ config KVM_GENERIC_PRIVATE_MEM
         select KVM_GMEM
         bool

+config KVM_GENERIC_GMEM_POPULATE
+       bool
+       depends on KVM_GMEM && KVM_GENERIC_MEMORY_ATTRIBUTES
+
  config HAVE_KVM_ARCH_GMEM_PREPARE
         bool
         depends on KVM_GMEM
diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
index b2aa6bf24d3a..befea51bbc75 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -638,7 +638,7 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct 
kvm_memory_slot *slot,
  }
  EXPORT_SYMBOL_GPL(kvm_gmem_get_pfn);

-#ifdef CONFIG_KVM_GENERIC_PRIVATE_MEM
+#ifdef CONFIG_KVM_GENERIC_GMEM_POPULATE
  long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user 
*src, long npages,
                        kvm_gmem_populate_cb post_populate, void *opaque)
  {