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 8:24 PM, Fuad Tabba wrote:
> On Wed, 16 Jul 2025 at 13:14, David Hildenbrand <david@redhat.com> wrote:
>>
>> On 16.07.25 14:01, Xiaoyao Li wrote:
>>> 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)
>>>     {
>>>
>>>
>>
>> $ git grep KVM_GENERIC_PRIVATE_MEM
>> arch/x86/kvm/Kconfig:   select KVM_GENERIC_PRIVATE_MEM if KVM_SW_PROTECTED_VM
>> arch/x86/kvm/Kconfig:   select KVM_GENERIC_PRIVATE_MEM if INTEL_TDX_HOST
>> arch/x86/kvm/Kconfig:   select KVM_GENERIC_PRIVATE_MEM
>> include/linux/kvm_host.h:#ifdef CONFIG_KVM_GENERIC_PRIVATE_MEM
>> virt/kvm/Kconfig:config KVM_GENERIC_PRIVATE_MEM
>> virt/kvm/guest_memfd.c:#ifdef CONFIG_KVM_GENERIC_PRIVATE_MEM
>>
>>
>> Why should we leave KVM_GENERIC_PRIVATE_MEM around when there are no other users?
>>

If we don't want KVM_GENERIC_PRIVATE_MEM, we can further clean it up 
with another patch:

------8<-----------
diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
index 3f87dcaaae83..12afc877c677 100644
--- a/arch/x86/kvm/Kconfig
+++ b/arch/x86/kvm/Kconfig
@@ -46,7 +46,8 @@ config KVM_X86
         select HAVE_KVM_PM_NOTIFIER if PM
         select KVM_GENERIC_HARDWARE_ENABLING
         select KVM_GENERIC_PRE_FAULT_MEMORY
-       select KVM_GENERIC_PRIVATE_MEM if KVM_SW_PROTECTED_VM
+       select KVM_GENERIC_MEMORY_ATTRIBUTES if KVM_SW_PROTECTED_VM
+       select KVM_GMEM if KVM_SW_PROTECTED_VM
         select KVM_WERROR if WERROR

  config KVM
@@ -95,7 +96,7 @@ config KVM_SW_PROTECTED_VM
  config KVM_INTEL
         tristate "KVM for Intel (and compatible) processors support"
         depends on KVM && IA32_FEAT_CTL
-       select KVM_GENERIC_PRIVATE_MEM if INTEL_TDX_HOST
+       select KVM_GMEM if INTEL_TDX_HOST
         select KVM_GENERIC_MEMORY_ATTRIBUTES if INTEL_TDX_HOST
         help
           Provides support for KVM on processors equipped with Intel's VT
@@ -158,7 +159,8 @@ config KVM_AMD_SEV
         depends on KVM_AMD && X86_64
         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_GMEM
+       select KVM_GENERIC_MEMORY_ATTRIBUTES
         select KVM_GENERIC_GMEM_POPULATE
         select HAVE_KVM_ARCH_GMEM_PREPARE
         select HAVE_KVM_ARCH_GMEM_INVALIDATE
diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
index 9b37ca009a22..67c626b1a637 100644
--- a/virt/kvm/Kconfig
+++ b/virt/kvm/Kconfig
@@ -116,11 +116,6 @@ config KVM_GMEM
         select XARRAY_MULTI
         bool

-config KVM_GENERIC_PRIVATE_MEM
-       select KVM_GENERIC_MEMORY_ATTRIBUTES
-       select KVM_GMEM
-       bool
-
  config KVM_GENERIC_GMEM_POPULATE
         bool
         depends on KVM_GMEM && KVM_GENERIC_MEMORY_ATTRIBUTES

>> @fuad help me out, what am I missing?
> 
> I'm not sure. Splitting it into two patches, one that introduces
> CONFIG_KVM_GENERIC_GMEM_POPULATE followed by one that drops
> CONFIG_KVM_GENERIC_PRIVATE_MEM ends up with the same result.

Not really the same result.

The two-step patches I proposed doesn't produce the below thing of this 
original patch. It doesn't make sense to select 
KVM_GENERIC_GMEM_POPULATE for KVM_SW_PROTECTED_VM from the name.

diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
index 2eeffcec5382..df1fdbb4024b 100644
--- a/arch/x86/kvm/Kconfig
+++ b/arch/x86/kvm/Kconfig
@@ -46,7 +46,7 @@ config KVM_X86
  	select HAVE_KVM_PM_NOTIFIER if PM
  	select KVM_GENERIC_HARDWARE_ENABLING
  	select KVM_GENERIC_PRE_FAULT_MEMORY
-	select KVM_GENERIC_PRIVATE_MEM if KVM_SW_PROTECTED_VM
+	select KVM_GENERIC_GMEM_POPULATE if KVM_SW_PROTECTED_VM
  	select KVM_WERROR if WERROR

> Cheers,
> /fuad
> 
> 
>> --
>> Cheers,
>>
>> David / dhildenb
>>