Re: [PATCH v2 06/15] KVM: x86/mmu: Support GFN direct mask

[Paolo Bonzini]

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