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