Received-SPF: pass (google.com: domain of linux-kernel+bounces-147726-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Tue, 16 Apr 2024 16:00:45 -0700
In-Reply-To: <CABgObfZq9dzvq3tsPMM3D+Zn-c77QrVd2Z1gW5ZKfb5fPu_8WA@mail.gmail.com>
Precedence: bulk
Mime-Version: 1.0
References: <cover.1712785629.git.isaku.yamahata@intel.com>
 <2f1de1b7b6512280fae4ac05e77ced80a585971b.1712785629.git.isaku.yamahata@intel.com>
 <116179545fafbf39ed01e1f0f5ac76e0467fc09a.camel@intel.com>
 <Zh2ZTt4tXXg0f0d9@google.com> <CABgObfZq9dzvq3tsPMM3D+Zn-c77QrVd2Z1gW5ZKfb5fPu_8WA@mail.gmail.com>
Message-ID: <Zh8DHbb8FzoVErgX@google.com>
Subject: Re: [PATCH v2 07/10] KVM: x86: Always populate L1 GPA for KVM_MAP_MEMORY
From: Sean Christopherson <seanjc@google.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Rick P Edgecombe <rick.p.edgecombe@intel.com>, "kvm@vger.kernel.org" <kvm@vger.kernel.org>, 
	Isaku Yamahata <isaku.yamahata@intel.com>, Kai Huang <kai.huang@intel.com>, 
	"federico.parola@polito.it" <federico.parola@polito.it>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, 
	"isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>, "dmatlack@google.com" <dmatlack@google.com>, 
	"michael.roth@amd.com" <michael.roth@amd.com>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Apr 16, 2024, Paolo Bonzini wrote:
> On Mon, Apr 15, 2024 at 11:17=E2=80=AFPM Sean Christopherson <seanjc@goog=
le.com> wrote:
> > The first question to answer is, do we want to return an error or "sile=
ntly"
> > install mappings for !SMM, !guest_mode.  And so this option becomes rel=
evant only
> > _if_ we want to unconditionally install mappings for the 'base" mode.
> >
> > > > - Return error on guest mode or SMM mode:  Without this patch.
> > > >   Pros: No additional patch.
> > > >   Cons: Difficult to use.
> > >
> > > Hmm... For the non-TDX use cases this is just an optimization, right?=
 For TDX
> > > there shouldn't be an issue. If so, maybe this last one is not so hor=
rible.
>=20
> It doesn't even have to be ABI that it gives an error. As you say,
> this ioctl can just be advisory only for !confidential machines. Even
> if it were implemented, the shadow MMU can drop roots at any moment

Sure, but there's a difference between KVM _potentially_ dropping roots and
guaranteed failure because userspace is trying to do something that's unsup=
ported.
But I think this is a non-issue, because it should really just be as simple=
 as:

	if (!mmu->pre_map_memory)
		return -EOPNOTSUPP;

Hmm, or probably this to avoid adding an MMU hook for a single MMU flavor:

	if (!tdp_mmu_enabled || !mmu->root_role.direct)
		return -EOPNOTSUPP;

> and/or kill the mapping via the shrinker.

Ugh, we really need to kill that code.

> That said, I can't fully shake the feeling that this ioctl should be
> an error for !TDX and that TDX_INIT_MEM_REGION wasn't that bad. The
> implementation was ugly but the API was fine.=20

Hmm, but IMO the implementation was ugly in no small part because of the co=
ntraints
put on KVM by the API.  Mapping S-EPT *and* doing TDH.MEM.PAGE.ADD in the s=
ame
ioctl() forced KVM to operate on vcpu0, and necessitated shoving temporary =
data
into a per-VM structure in order to get the source contents into TDH.MEM.PA=
GE.ADD.

We could eliminate the vcpu0 grossness, but it would require a massive refa=
ctor,
which is also not a problem per se, but it's obviously not free.  Eliminati=
ng
kvm_tdx.source_page is also doable, but it's not clear to me that end resul=
t would
be a net positive.

If userspace pre-maps the S-EPT entries ahead of time, then KVM should have=
 a
straight shot to PAGE.ADD, i.e. doesn't need to "pass" the source page via =
a
scratch field in kvm_tdx, and I think/hope would avoid the need to grab vcp=
u0
in order to get at an MMU to build the S-EPT.

And stating the obvious, TDX_INIT_MEM_REGION also doesn't allow pre-mapping=
 memory,
which is generally useful, and can be especially beneficial for confidentia=
l VMs
(and TDX in particular) due to the added cost of a page fault VM-Exit.

I'm not dead set on this generic ioctl(), but unless it ends up being a tra=
in wreck
for userspace, I think it will allow for cleaner and more reusable code in =
KVM.