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

[David Hildenbrand <david@redhat.com>]

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?

@fuad help me out, what am I missing?

-- 
Cheers,

David / dhildenb