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

Huang, Kai
2024-05-16T00:12:48Z

On 16/05/2024 11:59 am, Edgecombe, Rick P wrote:
> On Thu, 2024-05-16 at 11:44 +1200, Huang, Kai wrote:
>>>
>>> Sorry, still not clear. We need to strip the bit away, so we need to know
>>> what
>>> bit it is. The proposal is to not remember it on struct kvm, so where do we
>>> get
>>> it?
>>
>> The TDX specific code can get it when TDX guest is created.
>
> The TDX specific code sets it. It knows GPAW/shared bit location.
>
>>
>>>
>>> Actually, we used to allow it to be selected (via GPAW), but now we could
>>> determine it based on EPT level and MAXPA. So we could possibly recalculate
>>> it
>>> in some helper...
>>>
>>> But it seems you are suggesting to do away with the concept of knowing what
>>> the
>>> shared bit is.
>>
>> What I am suggesting is essentially to replace this
>> kvm_gfn_shared_mask() with some kvm_x86_ops callback (which can just
>> return the shared bit), assuming the common code somehow still need it
>> (e.g., setting up the SPTE for shared mapping, which must include the
>> shared bit to the GPA).
>>
>> The advantage of this we can get rid of the concept of 'gfn_shared_mask'
>> in the MMU common code.  All GFNs referenced in the common code is the
>> actual GFN (w/o the shared bit).
>
> When it is actually being used as the shared bit instead of as a way to check if
> a guest is a TD, what is the problem? I think the shared_mask serves a real
> (small) purpose, but it is misused for a bunch of other stuff. If we move that
> other stuff to new helpers, the shared mask will still be needed for it's
> original job.
>
> 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).

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);
+}