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.
