On Wed, 16 Jul 2025 at 13:14, David Hildenbrand <david@xxxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> 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?
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.
Cheers,
/fuad
> --
> Cheers,
>
> David / dhildenb
>
