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.
