Re: [PATCH 10/16] KVM: x86/tdp_mmu: Support TDX private mapping for TDP MMU

["Huang, Kai" <kai.huang@intel.com>]

On 17/05/2024 7:42 am, Isaku Yamahata wrote:
> On Thu, May 16, 2024 at 04:36:48PM +0000,
> "Edgecombe, Rick P" <rick.p.edgecombe@intel.com> wrote:
> 
>> On Thu, 2024-05-16 at 13:04 +0000, Huang, Kai wrote:
>>> On Thu, 2024-05-16 at 02:57 +0000, Edgecombe, Rick P wrote:
>>>> On Thu, 2024-05-16 at 14:07 +1200, Huang, Kai wrote:
>>>>>
>>>>> I meant it seems we should just strip shared bit away from the GPA in
>>>>> handle_ept_violation() and pass it as 'cr2_or_gpa' here, so fault->addr
>>>>> won't have the shared bit.
>>>>>
>>>>> Do you see any problem of doing so?
>>>>
>>>> We would need to add it back in "raw_gfn" in kvm_tdp_mmu_map().
>>>
>>> I don't see any big difference?
>>>
>>> Now in this patch the raw_gfn is directly from fault->addr:
>>>
>>>          raw_gfn = gpa_to_gfn(fault->addr);
>>>
>>>          tdp_mmu_for_each_pte(iter, mmu, is_private, raw_gfn, raw_gfn+1) {
>>>                  ...
>>>          }
>>>
>>> But there's nothing wrong to get the raw_gfn from the fault->gfn.  In
>>> fact, the zapping code just does this:
>>>
>>>          /*
>>>           * start and end doesn't have GFN shared bit.  This function zaps
>>>           * a region including alias.  Adjust shared bit of [start, end) if
>>>           * the root is shared.
>>>           */
>>>          start = kvm_gfn_for_root(kvm, root, start);
>>>          end = kvm_gfn_for_root(kvm, root, end);
>>>
>>> So there's nothing wrong to just do the same thing in both functions.
>>>
>>> The point is fault->gfn has shared bit stripped away at the beginning, and
>>> AFAICT there's no useful reason to keep shared bit in fault->addr.  The
>>> entire @fault is a temporary structure on the stack during fault handling
>>> anyway.
>>
>> I would like to avoid code churn at this point if there is not a real clear
>> benefit. >>
>> One small benefit of keeping the shared bit in the fault->addr is that it is
>> sort of consistent with how that field is used in other scenarios in KVM. In
>> shadow paging it's not even the GPA. So it is simply the "fault address" and has
>> to be interpreted in different ways in the fault handler. For TDX the fault
>> address *does* include the shared bit. And the EPT needs to be faulted in at
>> that address.

It's about how we want to define the semantic of fault->addr (forget 
about shadow MMU because for it fault->addr has different meaning from TDP):

1) It represents the faulting address which points to the actual guest 
memory (has no shared bit).

2) It represents the faulting address which is truly used as the 
hardware page table walk.

The fault->gfn always represents the location of actual guest memory 
(w/o shared bit) in both cases.

I originally thought 2) isn't consistent for both SNP and TDX, but 
thinking more, I now think actually both the two semantics are 
consistent for SNP and TDX, which is important in order to avoid confusion.

Anyway it's a trivial because fault->addr is only used for fault 
handling path.

So yes fine to me we choose to use 2) here.  But IMHO we should 
explicitly add a comment to 'struct kvm_page_fault' that the @addr 
represents 2).

And I think more important thing is how we handle this "gfn" and 
"raw_gfn" in tdp_iter and 'struct kvm_mmu_page'.  See below.

>>
>> If we strip the shared bit when setting fault->addr we have to reconstruct it
>> when we do the actual shared mapping. There is no way around that. Which helper
>> does it, isn't important I think. Doing the reconstruction inside
>> tdp_mmu_for_each_pte() could be neat, except that it doesn't know about the
>> shared bit position.
>>
>> The zapping code's use of kvm_gfn_for_root() is different because the gfn comes
>> without the shared bit. It's not stripped and then added back. Those are
>> operations that target GFNs really.
>>
>> I think the real problem is that we are gleaning whether the fault is to private
>> or shared memory from different things. Sometimes from fault->is_private,
>> sometimes the presence of the shared bits, and sometimes the role bit. I think
>> this is confusing, doubly so because we are using some of these things to infer
>> unrelated things (mirrored vs private).
> 
> It's confusing we don't check it in uniform way.
> 
> 
>> My guess is that you have noticed this and somehow zeroed in on the shared_mask.
>> I think we should straighten out the mirrored/private semantics and see what the
>> results look like. How does that sound to you?
> 
> I had closer look of the related code.  I think we can (mostly) uniformly use
> gpa/gfn without shared mask.  Here is the proposal.  We need a real patch to see
> how the outcome looks like anyway.  I think this is like what Kai is thinking
> about.
> 
> 
> - rename role.is_private => role.is_mirrored_pt
> 
> - sp->gfn: gfn without shared bit.

I think we can treat 'tdp_iter' and 'struct kvm_mmu_page' in the same 
way, because conceptually they both reflects the page table.

So I think both of them can have "gfn" or "raw_gfn", and "shared_gfn_mask".

Or have both "raw_gfn" or "gfn" but w/o "shared_gfn_mask". This may be 
more straightforward because we can just use them when needed w/o 
needing to play with gfn_shared_mask.

> 
> - fault->address: without gfn_shared_mask
>    Actually it doesn't matter much.  We can use gpa with gfn_shared_mask.

See above.  I think it makes sense to include the shared bit here.  It's 
trivial anyway though.

> 
> - Update struct tdp_iter
>    struct tdp_iter
>      gfn: gfn without shared bit

Or "raw_gfn"?

Which may be more straightforward because it can be just from:

	raw_gfn = gpa_to_gfn(fault->addr);
	gfn = fault->gfn;

	tdp_mmu_for_each_pte(..., raw_gfn, raw_gfn + 1, gfn)

Which is the reason to make fault->addr include the shared bit AFAICT.

> 
>      /* Add new members */
> 
>      /* Indicates which PT to walk. */
>      bool mirrored_pt;

I don't think you need this?  It's only used to select the root for page 
table walk.  Once it's done, we already have the @sptep to operate on.

And I think you can just get @mirrored_pt from the sptep:

	mirrored_pt = sptep_to_sp(sptep)->role.mirrored_pt;

Instead, I think we should keep the @is_private to indicate whether the 
GFN is private or not, which should be distinguished with 'mirrored_pt', 
which the root page table (and the @sptep) already reflects.

Of course if the @root/@sptep is mirrored_pt, the is_private should be 
always true, like:

	WARN_ON_ONCE(sptep_to_sp(sptep)->role.is_mirrored_pt
			&& !is_private);

Am I missing anything?

> 
>      // This is used tdp_iter_refresh_sptep()
>      // shared gfn_mask if mirrored_pt
>      // 0 if !mirrored_pt
>      gfn_shared_mask >
> - Pass mirrored_pt and gfn_shared_mask to
>    tdp_iter_start(..., mirrored_pt, gfn_shared_mask)

As mentioned above, I am not sure whether we need @mirrored_pt, because 
it already have the @root.  Instead we should pass is_private, which 
indicates the GFN is private.

> 
>    and update tdp_iter_refresh_sptep()
>    static void tdp_iter_refresh_sptep(struct tdp_iter *iter)
>          ...
>          iter->sptep = iter->pt_path[iter->level - 1] +
>                  SPTE_INDEX((iter->gfn << PAGE_SHIFT) | iter->gfn_shared_mask, iter->level);
> 
>    Change for_each_tdp_mte_min_level() accordingly.
>    Also the iteretor to call this.
>     
>    #define for_each_tdp_pte_min_level(kvm, iter, root, min_level, start, end)      \
>            for (tdp_iter_start(&iter, root, min_level, start,                      \
>                 mirrored_root, mirrored_root ? kvm_gfn_shared_mask(kvm) : 0);      \
>                 iter.valid && iter.gfn < kvm_gfn_for_root(kvm, root, end);         \
>                 tdp_iter_next(&iter))

See above.

> 
> - trace point: update to include mirroredd_pt. Or Leave it as is for now.
> 
> - pr_err() that log gfn in handle_changed_spte()
>    Update to include mirrored_pt. Or Leave it as is for now.
> 
> - Update spte handler (handle_changed_spte(), handle_removed_pt()...),
>    use iter->mirror_pt or pass down mirror_pt.
> 

IIUC only sp->role.is_mirrored_pt is needed, tdp_iter->is_mirrored_pt 
isn't necessary.  But when the @sp is created, we need to initialize 
whether it is mirrored_pt.

Am I missing anything?