Date: Fri, 17 May 2024 01:51:03 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "pbonzini@...hat.com"
	<pbonzini@...hat.com>, "seanjc@...gle.com" <seanjc@...gle.com>, "Huang, Kai"
	<kai.huang@...el.com>
CC: "sagis@...gle.com" <sagis@...gle.com>, "dmatlack@...gle.com"
	<dmatlack@...gle.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "Zhao, Yan Y" <yan.y.zhao@...el.com>,
	"isaku.yamahata@...il.com" <isaku.yamahata@...il.com>, "Aktas, Erdem"
	<erdemaktas@...gle.com>
Subject: Re: [PATCH 04/16] KVM: x86/mmu: Add address conversion functions for
 TDX shared bit of GPA

We agreed to remove the abuse of kvm_gfn_shared_mask() and look at it again. I
was just checking back in on the name of the other function as I said I would.

Never-the-less...

On Fri, 2024-05-17 at 12:37 +1200, Huang, Kai wrote:
> 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);

No, like this:

static inline bool kvm_on_private_root(const struct kvm *kvm, gpa_t gpa)
{
	gfn_t mask = kvm_gfn_shared_mask(kvm);

	return kvm_has_private_root(kvm) && !(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.

These helpers are where we hide what will functionally be the same as "if tdx".
The other similar ones literally check for KVM_X86_TDX_VM.

> 
> 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, 

Everyone is in agreement here, you don't need to make the point again.

> and I am wondering whether we can 
> get rid of kvm_gfn_shared_mask() completely.

You mentioned...

> 
> 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.
> 

I think you are trying to say not to abuse kvm_gfn_shared_mask() as is currently
done in this logic. But we already agreed on this. So not sure.

> 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

Sigh...

>  (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.

Up the thread we have:
On Thu, 2024-05-16 at 12:12 +1200, Huang, Kai wrote:
> > 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).

So.. same question.


> 
> Anyway, it's not a very strong opinion from me that we should remove the 
> kvm_shared_gfn_mask()

This is a shock!

> , 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.

I don't think we can get rid of the shared mask. Even if we relied on
kvm_mem_is_private() to determine if a GPA is private or shared, at absolute
minimum we need to add the shared bit when we are zapping a GFN or mapping it.

Let's table the discussion until we have some code to look again.

Powered by blists - more mailing lists