On Fri, Jun 7, 2024 at 8:39 PM Edgecombe, Rick P
<rick.p.edgecombe@xxxxxxxxx> wrote:
> > I think the code need not check kvm_gfn_direct_mask() here? In the old
> > patches that I have it check kvm_gfn_direct_mask() in the vmx/main.c
> > callback.
>
> You mean a VMX/TDX implementation of flush_remote_tlbs_range that just returns
> -EOPNOTSUPP? Which version of the patches is this? I couldn't find anything like
> that.
Something from Intel's GitHub, roughly June 2023... Looking at the
whole history, it starts with
if (!kvm_x86_ops.flush_remote_tlbs_range)
return -EOPNOTSUPP;
return static_call(kvm_x86_flush_remote_tlbs_range)(kvm, gfn, nr_pages);
and it only assigns the callback in vmx.c (not main.c); then it adds
an implementation of the callback for TDX that has:
static int vt_flush_remote_tlbs_range(struct kvm *kvm, gfn_t gfn,
gfn_t nr_pages)
{
if (is_td(kvm))
return tdx_sept_flush_remote_tlbs_range(kvm, gfn, nr_pages);
/* fallback to flush_remote_tlbs method */
return -EOPNOTSUPP;
}
where the callback knows that it should flush both private GFN and
shared GFN. So I didn't remember it correctly, but still there is no
check for the presence of direct-mapping bits.
> The downside would be wider distribution of the concerns for dealing with
> multiple aliases for a GFN. Currently, the behavior to have multiple aliases is
> implemented in core MMU code. While it's fine to pollute tdx.c with TDX specific
> knowledge of course, removing the handling of this corner from mmu.c might make
> it less understandable for non-tdx readers who are working in MMU code.
> Basically, if a concept fits into some non-TDX abstraction like this, having it
> in core code seems the better default to me.
I am not sure why it's an MMU concept that "if you offset the shared
mappings you cannot implement flush_remote_tlbs_range". It seems more
like, you need to know what you're doing?
Right now it makes no difference because you don't set the callback;
but if you ever wanted to implement flush_remote_tlbs_range as an
optimization you'd have to remove the condition from the "if". So it's
better not to have it in the first place.
Perhaps add a comment instead, like:
if (!kvm_x86_ops.flush_remote_tlbs_range)
return -EOPNOTSUPP;
+ /*
+ * If applicable, the callback should flush GFNs both with and without
+ * the direct-mapping bits.
+ */
return static_call(kvm_x86_flush_remote_tlbs_range)(kvm, gfn, nr_pages);
Paolo
