Hi Sean,
On Fri, 13 Jun 2025 at 21:35, Sean Christopherson <seanjc@xxxxxxxxxx> 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@xxxxxxxxx>
> > Reviewed-by: Gavin Shan <gshan@xxxxxxxxxx>
> > Reviewed-by: Shivank Garg <shivankg@xxxxxxx>
> > Reviewed-by: Vlastimil Babka <vbabka@xxxxxxx>
> > Co-developed-by: David Hildenbrand <david@xxxxxxxxxx>
> > Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
> > Signed-off-by: Fuad Tabba <tabba@xxxxxxxxxx>
> > ---
> > 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
