Received-SPF: pass (google.com: domain of linux-kernel+bounces-206982-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240530210714.364118-1-rick.p.edgecombe@intel.com>
 <20240530210714.364118-7-rick.p.edgecombe@intel.com> <CABgObfZuv45Bphz=VLCO4AF=W+iQbmMbNVk4Q0CAsVd+sqfJLw@mail.gmail.com>
 <9423e6b83523c0a3828a88f38ffc3275a08e11dd.camel@intel.com>
In-Reply-To: <9423e6b83523c0a3828a88f38ffc3275a08e11dd.camel@intel.com>
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Sat, 8 Jun 2024 11:08:21 +0200
Message-ID: <CABgObfbGeMoKKEMwY6108Z5UT1y=NzRhg-oBC-jpEpugD5_=Mg@mail.gmail.com>
Subject: Re: [PATCH v2 06/15] KVM: x86/mmu: Support GFN direct mask
To: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
Cc: "seanjc@google.com" <seanjc@google.com>, "Huang, Kai" <kai.huang@intel.com>, 
	"sagis@google.com" <sagis@google.com>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "Aktas, Erdem" <erdemaktas@google.com>, 
	"Zhao, Yan Y" <yan.y.zhao@intel.com>, "dmatlack@google.com" <dmatlack@google.com>, 
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>, "Yamahata, Isaku" <isaku.yamahata@intel.com>, 
	"isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Jun 7, 2024 at 8:39=E2=80=AFPM Edgecombe, Rick P
<rick.p.edgecombe@intel.com> 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 re=
turns
> -EOPNOTSUPP? Which version of the patches is this? I couldn't find anythi=
ng 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 alia=
ses is
> implemented in core MMU code. While it's fine to pollute tdx.c with TDX s=
pecific
> knowledge of course, removing the handling of this corner from mmu.c migh=
t 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, hav=
ing 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