Re: [PATCH 04/16] KVM: x86/mmu: Add address conversion functions for TDX shared bit of GPA

[Huang, Kai]

On 16/05/2024 11:59 am, Edgecombe, Rick P wrote:
> On Thu, 2024-05-16 at 11:44 +1200, Huang, Kai wrote:
> > > Sorry, still not clear. We need to strip the bit away, so we need to know
> > > what
> > > bit it is. The proposal is to not remember it on struct kvm, so where do we
> > > get
> > > it?
> >
> > The TDX specific code can get it when TDX guest is created.
>
> The TDX specific code sets it. It knows GPAW/shared bit location.
>
> > > Actually, we used to allow it to be selected (via GPAW), but now we could
> > > determine it based on EPT level and MAXPA. So we could possibly recalculate
> > > it
> > > in some helper...
> > >
> > > But it seems you are suggesting to do away with the concept of knowing what
> > > the
> > > shared bit is.
> >
> > What I am suggesting is essentially to replace this
> > kvm_gfn_shared_mask() with some kvm_x86_ops callback (which can just
> > return the shared bit), assuming the common code somehow still need it
> > (e.g., setting up the SPTE for shared mapping, which must include the
> > shared bit to the GPA).
> >
> > The advantage of this we can get rid of the concept of 'gfn_shared_mask'
> > in the MMU common code.  All GFNs referenced in the common code is the
> > actual GFN (w/o the shared bit).
>
> When it is actually being used as the shared bit instead of as a way to check if
> a guest is a TD, what is the problem? I think the shared_mask serves a real
> (small) purpose, but it is misused for a bunch of other stuff. If we move that
> other stuff to new helpers, the shared mask will still be needed for it's
> original job.
>
> What is the benefit of the x86_ops over a static inline?

I don't have strong objection if the use of kvm_gfn_shared_mask() is 
contained in smaller areas that truly need it.  Let's discuss in 
relevant patch(es).

However I do think the helpers like below makes no sense (for SEV-SNP):

+static inline bool kvm_is_private_gpa(const struct kvm *kvm, gpa_t gpa)
+{
+	gfn_t mask = kvm_gfn_shared_mask(kvm);
+
+	return mask && !(gpa_to_gfn(gpa) & mask);
+}