Sean Christopherson <seanjc@xxxxxxxxxx> writes:
> On Wed, Jun 11, 2025, Fuad Tabba wrote:
>> The bool has_private_mem is used to indicate whether guest_memfd is
>> supported.
>
> No? This is at best weird, and at worst flat out wrong:
>
> if (kvm->arch.supports_gmem &&
> fault->is_private != kvm_mem_is_private(kvm, fault->gfn))
> return false;
>
> ditto for this code:
>
> if (kvm_arch_supports_gmem(vcpu->kvm) &&
> kvm_mem_is_private(vcpu->kvm, gpa_to_gfn(range->gpa)))i
> error_code |= PFERR_PRIVATE_ACCESS;
>
> and for the memory_attributes code. E.g. IIRC, with guest_memfd() mmap support,
> private vs. shared will become a property of the guest_memfd inode, i.e. this will
> become wrong:
>
> static u64 kvm_supported_mem_attributes(struct kvm *kvm)
> {
> if (!kvm || kvm_arch_supports_gmem(kvm))
> return KVM_MEMORY_ATTRIBUTE_PRIVATE;
>
> return 0;
> }
>
> Instead of renaming kvm_arch_has_private_mem() => kvm_arch_supports_gmem(), *add*
> kvm_arch_supports_gmem() and then kill off kvm_arch_has_private_mem() once non-x86
> usage is gone (i.e. query kvm->arch.has_private_mem directly).
>
> And then rather than rename has_private_mem, either add supports_gmem or do what
> you did for kvm_arch_supports_gmem_shared_mem() and explicitly check the VM type.
>
IIUC Fuad will be adding bool supports_gmem instead of renaming, but we
haven't discussed which usages will start using the new function.
Let me go over all the changes/usages related to has_private_mem and
supports_gmem.
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 6e0bbf4c2202..3d69da6d2d9e 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -2270,9 +2270,9 @@ void kvm_configure_mmu(bool enable_tdp, int tdp_forced_root_level,
>
>
> #ifdef CONFIG_KVM_GMEM
> -#define kvm_arch_has_private_mem(kvm) ((kvm)->arch.has_private_mem)
> +#define kvm_arch_supports_gmem(kvm) ((kvm)->arch.has_private_mem)
> #else
> -#define kvm_arch_has_private_mem(kvm) false
> +#define kvm_arch_supports_gmem(kvm) false
> #endif
>
*The* renaming vs adding-new-function change.
> #define kvm_arch_has_readonly_mem(kvm) (!(kvm)->arch.has_protected_state)
> @@ -2325,8 +2325,8 @@ enum {
> #define HF_SMM_INSIDE_NMI_MASK (1 << 2)
>
> # define KVM_MAX_NR_ADDRESS_SPACES 2
> -/* SMM is currently unsupported for guests with private memory. */
> -# define kvm_arch_nr_memslot_as_ids(kvm) (kvm_arch_has_private_mem(kvm) ? 1 : 2)
> +/* SMM is currently unsupported for guests with guest_memfd (esp private) memory. */
> +# define kvm_arch_nr_memslot_as_ids(kvm) (kvm_arch_supports_gmem(kvm) ? 1 : 2)
> # define kvm_arch_vcpu_memslots_id(vcpu) ((vcpu)->arch.hflags & HF_SMM_MASK ? 1 : 0)
> # define kvm_memslots_for_spte_role(kvm, role) __kvm_memslots(kvm, (role).smm)
> #else
IIUC from the discussion at guest_memfd call on 2025-05-15, SMM can't be
supported because SMM relies on memory being shared.
This should remain as kvm_arch_has_private_mem() - as long as the VM
supports private memory at all, kvm_arch_nr_memslot_as_ids() should
return 1 (no SMM support).
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index cbc84c6abc2e..e7ecf089780a 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -4910,7 +4910,7 @@ long kvm_arch_vcpu_pre_fault_memory(struct kvm_vcpu *vcpu,
> if (r)
> return r;
>
> - if (kvm_arch_has_private_mem(vcpu->kvm) &&
> + if (kvm_arch_supports_gmem(vcpu->kvm) &&
> kvm_mem_is_private(vcpu->kvm, gpa_to_gfn(range->gpa)))
> error_code |= PFERR_PRIVATE_ACCESS;
>
If the VM supports private mem and KVM knows the gfn to be private
(whether based on memory attributes or in future, guest_memfd's
shareability), prefault it as private.
Here technically the kvm_arch_has_private_mem() check just helps
short-circuit to save deeper lookups, but if kvm_arch_has_private_mem()
is false, kvm_mem_is_private() always return false anyway.
This should remain as kvm_arch_has_private_mem().
> @@ -7707,7 +7707,7 @@ bool kvm_arch_pre_set_memory_attributes(struct kvm *kvm,
> * Zapping SPTEs in this case ensures KVM will reassess whether or not
> * a hugepage can be used for affected ranges.
> */
> - if (WARN_ON_ONCE(!kvm_arch_has_private_mem(kvm)))
> + if (WARN_ON_ONCE(!kvm_arch_supports_gmem(kvm)))
> return false;
>
> if (WARN_ON_ONCE(range->end <= range->start))
Skip setting memory attributes if this kvm doesn't support private
memory.
This should remain as kvm_arch_has_private_mem().
> @@ -7786,7 +7786,7 @@ bool kvm_arch_post_set_memory_attributes(struct kvm *kvm,
> * a range that has PRIVATE GFNs, and conversely converting a range to
> * SHARED may now allow hugepages.
> */
> - if (WARN_ON_ONCE(!kvm_arch_has_private_mem(kvm)))
> + if (WARN_ON_ONCE(!kvm_arch_supports_gmem(kvm)))
> return false;
>
> /*
Skip setting memory attributes if this kvm doesn't support private
memory.
This should remain as kvm_arch_has_private_mem().
> @@ -7842,7 +7842,7 @@ void kvm_mmu_init_memslot_memory_attributes(struct kvm *kvm,
> {
> int level;
>
> - if (!kvm_arch_has_private_mem(kvm))
> + if (!kvm_arch_supports_gmem(kvm))
> return;
>
> for (level = PG_LEVEL_2M; level <= KVM_MAX_HUGEPAGE_LEVEL; level++) {
Skip initializing memory attributes if this kvm doesn't support private
memory, since for now KVM_MEMORY_ATTRIBUTE_PRIVATE is the only memory
attribute.
This should remain as kvm_arch_has_private_mem().
Or perhaps (separately from this series) this check can be changed to
kvm_supported_mem_attributes() != 0.
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 7700efc06e35..a0e661aa3f8a 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -719,11 +719,11 @@ static inline int kvm_arch_vcpu_memslots_id(struct kvm_vcpu *vcpu)
> #endif
>
> /*
> - * Arch code must define kvm_arch_has_private_mem if support for private memory
> + * Arch code must define kvm_arch_supports_gmem if support for guest_memfd
> * is enabled.
> */
> -#if !defined(kvm_arch_has_private_mem) && !IS_ENABLED(CONFIG_KVM_GMEM)
> -static inline bool kvm_arch_has_private_mem(struct kvm *kvm)
> +#if !defined(kvm_arch_supports_gmem) && !IS_ENABLED(CONFIG_KVM_GMEM)
> +static inline bool kvm_arch_supports_gmem(struct kvm *kvm)
> {
> return false;
> }
*The* renaming vs adding-new-function change.
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 898c3d5a7ba8..6efbea208fa6 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1588,7 +1588,7 @@ static int check_memory_region_flags(struct kvm *kvm,
> {
> u32 valid_flags = KVM_MEM_LOG_DIRTY_PAGES;
>
> - if (kvm_arch_has_private_mem(kvm))
> + if (kvm_arch_supports_gmem(kvm))
> valid_flags |= KVM_MEM_GUEST_MEMFD;
>
> /* Dirty logging private memory is not currently supported. */
This should be renamed - the flag is valid only if guest_memfd is
supported and squarely matches kvm_arch_supports_gmem().
> @@ -2419,7 +2419,7 @@ static int kvm_vm_ioctl_clear_dirty_log(struct kvm *kvm,
> #ifdef CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES
> static u64 kvm_supported_mem_attributes(struct kvm *kvm)
> {
> - if (!kvm || kvm_arch_has_private_mem(kvm))
> + if (!kvm || kvm_arch_supports_gmem(kvm))
> return KVM_MEMORY_ATTRIBUTE_PRIVATE;
>
> return 0;
This should remain as kvm_arch_has_private_mem().
There's a little issue when mmap support is added: generally for the VM
(for non-guest_memfd memslots in this VM), KVM_MEMORY_ATTRIBUTE_PRIVATE
should be supported, but specifically for some ranges that belong to
guest_memfd-only memslots, KVM_MEMORY_ATTRIBUTE_PRIVATE should not be
supported?
I think kvm_supported_mem_attributes() respond generically for the
entire VM, so leaving this as kvm_arch_has_private_mem() is correct.
> @@ -4912,7 +4912,7 @@ static int kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long arg)
> #endif
> #ifdef CONFIG_KVM_GMEM
> case KVM_CAP_GUEST_MEMFD:
> - return !kvm || kvm_arch_has_private_mem(kvm);
> + return !kvm || kvm_arch_supports_gmem(kvm);
> #endif
> default:
> break;
This should be renamed - the CAP is valid only if guest_memfd is
supported and squarely matches kvm_arch_supports_gmem().
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 3d69da6d2d9e..4bc50c1e21bd 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1341,7 +1341,7 @@ struct kvm_arch {
> unsigned int indirect_shadow_pages;
> u8 mmu_valid_gen;
> u8 vm_type;
> - bool has_private_mem;
> + bool supports_gmem;
> bool has_protected_state;
> bool pre_fault_allowed;
> struct hlist_head mmu_page_hash[KVM_NUM_MMU_PAGES];
> @@ -2270,7 +2270,7 @@ void kvm_configure_mmu(bool enable_tdp, int tdp_forced_root_level,
>
>
> #ifdef CONFIG_KVM_GMEM
> -#define kvm_arch_supports_gmem(kvm) ((kvm)->arch.has_private_mem)
> +#define kvm_arch_supports_gmem(kvm) ((kvm)->arch.supports_gmem)
> #else
> #define kvm_arch_supports_gmem(kvm) false
> #endif
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index e7ecf089780a..c4e10797610c 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -3488,7 +3488,7 @@ static bool page_fault_can_be_fast(struct kvm *kvm, struct kvm_page_fault *fault
> * on RET_PF_SPURIOUS until the update completes, or an actual spurious
> * case might go down the slow path. Either case will resolve itself.
> */
> - if (kvm->arch.has_private_mem &&
> + if (kvm->arch.supports_gmem &&
> fault->is_private != kvm_mem_is_private(kvm, fault->gfn))
> return false;
>
This check should remain as a check on has_private_mem.
If the VM supports private memory, skip fast page faults on fault type
and KVM memory privacy status mismatches.
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index ab9b947dbf4f..67ab05fd3517 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -5180,8 +5180,8 @@ static int svm_vm_init(struct kvm *kvm)
> (type == KVM_X86_SEV_ES_VM || type == KVM_X86_SNP_VM);
> to_kvm_sev_info(kvm)->need_init = true;
>
> - kvm->arch.has_private_mem = (type == KVM_X86_SNP_VM);
> - kvm->arch.pre_fault_allowed = !kvm->arch.has_private_mem;
> + kvm->arch.supports_gmem = (type == KVM_X86_SNP_VM);
> + kvm->arch.pre_fault_allowed = !kvm->arch.supports_gmem;
> }
>
> if (!pause_filter_count || !pause_filter_thresh)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b58a74c1722d..401256ee817f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -12778,8 +12778,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> return -EINVAL;
>
> kvm->arch.vm_type = type;
> - kvm->arch.has_private_mem =
> - (type == KVM_X86_SW_PROTECTED_VM);
> + kvm->arch.supports_gmem = (type == KVM_X86_SW_PROTECTED_VM);
> /* Decided by the vendor code for other VM types. */
> kvm->arch.pre_fault_allowed =
> type == KVM_X86_DEFAULT_VM || type == KVM_X86_SW_PROTECTED_VM;
and
> arch/x86/kvm/vmx/tdx.c: In function ‘tdx_vm_init’:
> arch/x86/kvm/vmx/tdx.c:627:18: error: ‘struct kvm_arch’ has no member named ‘has_private_mem’
> 627 | kvm->arch.has_private_mem = true;
> | ^
> make[5]: *** [scripts/Makefile.build:287: arch/x86/kvm/vmx/tdx.o] Error 1
These three changes make me think that maybe .has_private_mem shouldn't
be a field at all and can be removed.
What if kvm_arch_has_private_mem() for x86 always checks for a specific
list of VM types? Something like this: on x86,
* kvm_arch_has_private_mem() will return true for
KVM_X86_SW_PROTECTED_VM, KVM_X86_SNP_VM and KVM_X86_TDX_VM.
* kvm_arch_supports_gmem() will return true for KVM_X86_SW_PROTECTED_VM,
KVM_X86_SNP_VM and KVM_X86_TDX_VM as well.
After mmap support, kvm_arch_supports_gmem() also return true for
KVM_X86_DEFAULT_VM, in addition to the original SW_PROTECTED, SNP and
TDX.
Patrick, Nikita, am I right that for KVM_X86_DEFAULT_VM to work with
mmap-able guest_memfd, the usage in page_fault_can_be_fast() need not be
updated, and that patch 10/18 in this series will be sufficient?
>> [...]
