On 7/16/2025 8:24 PM, Fuad Tabba wrote:
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?
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
