Re: [PATCH] KVM: x86/mmu: Embed direct bits into gpa for KVM_PRE_FAULT_MEMORY

[Paolo Bonzini]

On Wed, Jun 11, 2025 at 8:10 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > +     direct_bits = 0;
> >       if (kvm_arch_has_private_mem(vcpu->kvm) &&
> >           kvm_mem_is_private(vcpu->kvm, gpa_to_gfn(range->gpa)))
> >               error_code |= PFERR_PRIVATE_ACCESS;
> > +     else
> > +             direct_bits = gfn_to_gpa(kvm_gfn_direct_bits(vcpu->kvm));
>
> Eww.  It's bad enough that TDX bleeds it's mirror needs into common MMU code,
> but stuffing vendor specific GPA bits in common code goes too far.  Actually,
> all of this goes too far.  There's zero reason any code outside of TDX needs to
> *explicitly* care whether mirrors or "direct" MMUs have mandatory gfn bits.
>
> Back to the main topic, KVM needs to have a single source of truth when it comes
> to whether a fault is private and thus mirrored (or not).  Common KVM needs to be
> aware of aliased GFN bits, but absolute nothing outside of TDX (including common
> VMX code) should be aware the mirror vs. "direct" (I hate that terminology; KVM
> has far, far too much history and baggage with "direct") is tied to the existence
> and polarity of aliased GFN bits.
>
> To detect a mirror fault:
>
>   static inline bool kvm_is_mirror_fault(struct kvm *kvm, u64 error_code)
>   {
>         return kvm_has_mirrored_tdp(kvm) &&
>                error_code & PFERR_PRIVATE_ACCESS;
>   }
>
> And for TDX, it should darn well explicitly track the shared GPA mask:
>
>   static bool tdx_is_private_gpa(struct kvm *kvm, gpa_t gpa)
>   {
>         /* For TDX the direct mask is the shared mask. */
>         return !(gpa & to_kvm_tdx(kvm)->shared_gpa_mask);
>   }

My fault - this is more similar, at least in spirit, to what
Yan and Xiaoyao had tested earlier:

diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h
index 52acf99d40a0..209103bf0f30 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.h
+++ b/arch/x86/kvm/mmu/tdp_mmu.h
@@ -48,7 +48,7 @@ static inline enum kvm_tdp_mmu_root_types
 static inline struct kvm_mmu_page *tdp_mmu_get_root_for_fault(struct kvm_vcpu *vcpu,
 							      struct kvm_page_fault *fault)
 {
-	if (unlikely(!kvm_is_addr_direct(vcpu->kvm, fault->addr)))
+	if (unlikely(fault->is_private))
 		return root_to_sp(vcpu->arch.mmu->mirror_root_hpa);

and I instead proposed the version that you hate with such ardor.

My reasoning was that I preferred to have the pre-fault scenario "look like"
what you get while the VM runs.

> Outside of TDX, detecting mirrors, and anti-aliasing logic, the only use of
> kvm_gfn_direct_bits() is to constrain TDP MMU walks to the appropriate gfn range.
> And for that, we can simply use kvm_mmu_page.gfn, with a kvm_x86_ops hook to get
> the TDP MMU root GFN (root allocation is a slow path, the CALL+RET is a non-issue).
>
> Compile tested only, and obviously needs to be split into multiple patches.

Also obviously needs to be delayed to 6.17, since a working fix can be a
one line change. :) (Plus your kvm_is_gfn_alias() test which should be
included anyway and independently).

What do you hate less between Yan's idea above and this patch? Just tell me
and I'll handle posting v2.

Paolo