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

[Huang, Kai]

On 17/05/2024 11:08 am, Edgecombe, Rick P wrote:
> On Wed, 2024-05-15 at 18:20 -0700, Rick Edgecombe wrote:
> > On Thu, 2024-05-16 at 13:04 +1200, Huang, Kai wrote:
> > > 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.
> >
> > Again, point taken. I'll try to think of a better name. Please share if you
> > do.
>
> What about:
> bool kvm_on_private_root(const struct kvm *kvm, gpa_t gpa);
>
> Since SNP doesn't have a private root, it can't get confused for SNP. For TDX
> it's a little weirder. We usually want to know if the GPA is to the private
> half. Whether it's on a separate root or not is not really important to the
> callers. But they could infer that if it's on a private root it must be a
> private GPA.
>
>
> Otherwise:
> bool kvm_is_private_gpa_bits(const struct kvm *kvm, gpa_t gpa);
>
> The bits indicates it's checking actual bits in the GPA and not the
> private/shared state of the GFN.

The kvm_on_private_root() is better to me, assuming this helper wants to 
achieve two goals:

  1) whether a given GPA is private;
  2) and when it is, whether to use private table;

And AFAICT we still want this implementation:

+	gfn_t mask = kvm_gfn_shared_mask(kvm);
+
+	return mask && !(gpa_to_gfn(gpa) & mask);

What I don't quite like is we use ...

	!(gpa_to_gfn(gpa) & mask);

.. to tell whether a GPA is private, because it is TDX specific logic 
cause it doesn't tell on SNP whether the GPA is private.

But as you said it certainly makes sense to say "we won't use a private 
table for this GPA" when the VM doesn't have a private table at all.  So 
it's also fine to me.

But my question is "why we need this helper at all".

As I expressed before, my concern is we already have too many mechanisms 
around private/shared memory/mapping, and I am wondering whether we can 
get rid of kvm_gfn_shared_mask() completely.

E.g,  why we cannot do:

	static bool kvm_use_private_root(struct kvm *kvm)
	{
		return kvm->arch.vm_type == VM_TYPE_TDX;
	}

Or,
	static bool kvm_use_private_root(struct kvm *kvm)
	{
		return kvm->arch.use_private_root;
	}

Or, assuming we would love to keep the kvm_gfn_shared_mask():

	static bool kvm_use_private_root(struct kvm *kvm)
	{
		return !!kvm_gfn_shared_mask(kvm);
	}

And then:

In fault handler:

	if (fault->is_private && kvm_use_private_root(kvm))
		// use private root
	else
		// use shared/normal root

When you zap:

	bool private_gpa = kvm_mem_is_private(kvm, gfn);
	
	if (private_gpa && kvm_use_private_root(kvm))
		// zap private root
	else
		// zap shared/normal root.

The benefit of this is we can clearly split the logic of:

  1) whether a GPN is private, and
  2) whether to use private table for private GFN

But it's certainly possible that I am missing something, though.

Do you see any problem of above?

Again, my main concern is whether we should just get rid of the 
kvm_gfn_shared_mask() completely (so we won't be able to abuse to use 
it) due to we already having so many mechanisms around private/shared 
memory/mapping here.

But I also understand we anyway will need to add the shared bit back 
when we setup page table or teardown of it, but for this purpose we can 
also use:

	kvm_x86_ops->get_shared_gfn_mask(kvm)

So to me the kvm_shared_gfn_mask() is at least not mandatory.

Anyway, it's not a very strong opinion from me that we should remove the 
kvm_shared_gfn_mask(), assuming we won't abuse to use it just for 
convenience in common code.

I hope I have expressed my view clearly.

And consider this as just my 2 cents.