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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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.




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux