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

[Huang, Kai]

On 16/05/2024 12:35 pm, Edgecombe, Rick P wrote:
> On Thu, 2024-05-16 at 12:25 +1200, Huang, Kai wrote:
> > On 16/05/2024 12:19 pm, Edgecombe, Rick P wrote:
> > > On Thu, 2024-05-16 at 12:12 +1200, Huang, Kai wrote:
> > > > 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);
> > > > +}
> > >
> > > You mean the name? SNP doesn't have a concept of "private GPA" IIUC. The C
> > > bit
> > > is more like an permission bit. So SNP doesn't have private GPAs, and the
> > > function would always return false for SNP. So I'm not sure it's too
> > > horrible.
> >
> > Hmm.. Why SNP doesn't have private GPAs?  They are crypto-protected and
> > KVM cannot access directly correct?
>
> I suppose a GPA could be pointing to memory that is private. But I think in SNP
> it is more the memory that is private. Now I see more how it could be confusing.
>
> > > If it's the name, can you suggest something?
> >
> > The name make sense, but it has to reflect the fact that a given GPA is
> > truly private (crypto-protected, inaccessible to KVM).
>
> If this was a function that tested whether memory is private and took a GPA, I
> would call it is_private_mem() or something. Because it's testing the memory and
> takes a GPA, not testing the GPA. Usually a function name should be about what
> the function does, not what arguments it takes.
>
> I can't think of a better name, but point taken that it is not ideal. I'll try
> to think of something.

I really don't see difference between ...

	is_private_mem(gpa)

.. and

	is_private_gpa(gpa)

If it confuses me, it can confuses other people.

The point is there's really no need to distinguish the two.  The GPA is 
only meaningful when it refers to the memory that it points to.

So far I am not convinced we need this helper, because such info we can 
already get from:

  1) fault->is_private;
  2) Xarray which records memtype for given GFN.

So we should just get rid of it.