Received-SPF: pass (google.com: domain of linux-kernel+bounces-192800-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <12afae41-906c-4bb7-956a-d73734c68010@intel.com>
 <1d247b658f3e9b14cefcfcf7bca01a652d0845a0.camel@intel.com>
 <a08779dc-056c-421c-a573-f0b1ba9da8ad@intel.com> <588d801796415df61136ce457156d9ff3f2a2661.camel@intel.com>
 <021e8ee11c87bfac90e886e78795d825ddab32ee.camel@intel.com>
 <eb7417cccf1065b9ac5762c4215195150c114ef8.camel@intel.com>
 <20240516194209.GL168153@ls.amr.corp.intel.com> <55c24448fdf42d383d45601ff6c0b07f44f61787.camel@intel.com>
 <20240517090348.GN168153@ls.amr.corp.intel.com> <d7b5a1e327d6a91e8c2596996df3ff100992dc6c.camel@intel.com>
 <20240517191630.GC412700@ls.amr.corp.intel.com>
In-Reply-To: <20240517191630.GC412700@ls.amr.corp.intel.com>
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Tue, 28 May 2024 19:16:51 +0200
Message-ID: <CABgObfY=RnDFcs8Mxt3RZYmA1nQ24dux3Rhs4hK0ZfeE_OtLUw@mail.gmail.com>
Subject: Re: [PATCH 10/16] KVM: x86/tdp_mmu: Support TDX private mapping for
 TDP MMU
To: Isaku Yamahata <isaku.yamahata@intel.com>
Cc: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>, "kvm@vger.kernel.org" <kvm@vger.kernel.org>, 
	"seanjc@google.com" <seanjc@google.com>, "Huang, Kai" <kai.huang@intel.com>, 
	"sagis@google.com" <sagis@google.com>, 
	"isaku.yamahata@linux.intel.com" <isaku.yamahata@linux.intel.com>, "Aktas, Erdem" <erdemaktas@google.com>, 
	"Zhao, Yan Y" <yan.y.zhao@intel.com>, 
	"isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>, "dmatlack@google.com" <dmatlack@google.com>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, May 17, 2024 at 9:16=E2=80=AFPM Isaku Yamahata <isaku.yamahata@inte=
l.com> wrote:
>
> On Fri, May 17, 2024 at 06:16:26PM +0000,
> "Edgecombe, Rick P" <rick.p.edgecombe@intel.com> wrote:
>
> > On Fri, 2024-05-17 at 02:03 -0700, Isaku Yamahata wrote:
> > >
> > > On top of your patch, I created the following patch to remove
> > > kvm_gfn_for_root().
> > > Although I haven't tested it yet, I think the following shows my idea=
.
> > >
> > > - Add gfn_shared_mask to struct tdp_iter.
> > > - Use iter.gfn_shared_mask to determine the starting sptep in the roo=
t.
> > > - Remove kvm_gfn_for_root()
> >
> > I investigated it.
>
> Thanks for looking at it.
>
> > After this, gfn_t's never have shared bit. It's a simple rule. The MMU =
mostly
> > thinks it's operating on a shared root that is mapped at the normal GFN=
 Only
> > the iterator knows that the shared PTEs are actually in a different loc=
ation.
> >
> > There are some negative side effects:
> > 1. The struct kvm_mmu_page's gfn doesn't match it's actual mapping anym=
ore.
> > 2. As a result of above, the code that flushes TLBs for a specific GFN =
will be
> > confused. It won't functionally matter for TDX, just look buggy to see =
flushing
> > code called with the wrong gfn.
>
> flush_remote_tlbs_range() is only for Hyper-V optimization.  In other cas=
es,
> x86_op.flush_remote_tlbs_range =3D NULL or the member isn't defined at co=
mpile
> time.  So the remote tlb flush falls back to flushing whole range.  I don=
't
> expect TDX in hyper-V guest.  I have to admit that the code looks superfi=
cially
> broken and confusing.

You could add an "&& kvm_has_private_root(kvm)" to
kvm_available_flush_remote_tlbs_range(), since
kvm_has_private_root(kvm) is sort of equivalent to "there is no 1:1
correspondence between gfn and PTE to be flushed".

I am conflicted myself, but the upsides below are pretty substantial.

Paolo

> > On the positive effects side:
> > 1. There is code that passes sp->gfn into things that it shouldn't (if =
it has
> > shared bits) like memslot lookups.
> > 2. Also code that passes iter.gfn into things it shouldn't like
> > kvm_mmu_max_mapping_level().
> >
> > These places are not called by TDX, but if you know that gfn's might in=
clude
> > shared bits, then that code looks buggy.
> >
> > I think the solution in the diff is more elegant then before, because i=
t hides
> > what is really going on with the shared root. That is both good and bad=
 Can we
> > accept the downsides?
>
> Kai, do you have any thoughts?
> --
> Isaku Yamahata <isaku.yamahata@intel.com>
>