On Sat, 2024-06-08 at 11:13 +0200, Paolo Bonzini wrote: > > I was not loving the amount of indirection here in the patch, but thought it > > centralized the logic a bit better. This way seems good, given that the > > actual > > logic is not that complex. > > My proposed implementation is a bit TDX-specific though... Something > like this is more agnostic, and it exploits nicely the difference > between fault->addr and fault->gfn: > > if (!kvm_gfn_direct_mask(kvm) || > (gpa_to_gfn(fault->addr) & kvm_gfn_direct_mask(kvm)) > root_hpa = vcpu->arch.mmu->root.hpa; > else > root_hpa = vcpu->arch.mmu->mirror_root_hpa; > return root_to_sp(root_hpa); Agreed that this is less TDX specific and it means that this part of the generic MMU code doesn't need to know that the mirror/direct matches to private vs shared. I don't love that it has such a complicated conditional for the normal VM case, though. Just for readability. The previous versions checked kvm_gfn_shared_mask() more readily in various open coded spots. In this v2 we tried to reduce this and instead always rely on the "private" concept to switch between the roots in the generic code. I think it's arguably a little easier to understand if we stick to a single way of deciding which root to use. But I don't feel like any of these solutions discussed is perfectly clean. So I'm ok taking the benefits you prefer. I guess doing bitwise operations when possible is kind of the KVM way, haha. :)
