| Message ID | 20250611133330.1514028-5-tabba@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: Mapping guest_memfd backed memory at the host for software protected VMs | expand |
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. > Rename it to supports_gmem to make its meaning clearer and to decouple memory > being private from guest_memfd. > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > Reviewed-by: Gavin Shan <gshan@redhat.com> > Reviewed-by: Shivank Garg <shivankg@amd.com> > Reviewed-by: Vlastimil Babka <vbabka@suse.cz> > Co-developed-by: David Hildenbrand <david@redhat.com> > Signed-off-by: David Hildenbrand <david@redhat.com> > Signed-off-by: Fuad Tabba <tabba@google.com> > --- > arch/x86/include/asm/kvm_host.h | 4 ++-- > arch/x86/kvm/mmu/mmu.c | 2 +- > arch/x86/kvm/svm/svm.c | 4 ++-- > arch/x86/kvm/x86.c | 3 +-- > 4 files changed, 6 insertions(+), 7 deletions(-) This missed the usage in TDX (it's not a staleness problem, because this series was based on 6.16-rc1, which has the relevant code). 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
Hi Sean, On Fri, 13 Jun 2025 at 21:35, Sean Christopherson <seanjc@google.com> wrote: > > 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. Will do. To make sure we're on the same page, we should add `supports_gmem` and keep `has_private_mem`, and continue using it for x86 code by querying it directly once the helpers are added. > > Rename it to supports_gmem to make its meaning clearer and to decouple memory > > being private from guest_memfd. > > > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > > Reviewed-by: Gavin Shan <gshan@redhat.com> > > Reviewed-by: Shivank Garg <shivankg@amd.com> > > Reviewed-by: Vlastimil Babka <vbabka@suse.cz> > > Co-developed-by: David Hildenbrand <david@redhat.com> > > Signed-off-by: David Hildenbrand <david@redhat.com> > > Signed-off-by: Fuad Tabba <tabba@google.com> > > --- > > arch/x86/include/asm/kvm_host.h | 4 ++-- > > arch/x86/kvm/mmu/mmu.c | 2 +- > > arch/x86/kvm/svm/svm.c | 4 ++-- > > arch/x86/kvm/x86.c | 3 +-- > > 4 files changed, 6 insertions(+), 7 deletions(-) > > This missed the usage in TDX (it's not a staleness problem, because this series > was based on 6.16-rc1, which has the relevant code). > > 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 I did test and run this before submitting the series. Building it on x86 with x86_64_defconfig and with allmodconfig pass (I obviously missed TDX though, apologies for that). I should have grepped for has_private_mem. That said, if I understood your suggestion correctly, this problem wouldn't happen again. Cheers, /fuad
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; 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;