Received-SPF: pass (google.com: domain of linux-kernel+bounces-147001-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 16 Apr 2024 07:22:21 -0700
In-Reply-To: <20240416014931.GW3039520@ls.amr.corp.intel.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> <20240416014931.GW3039520@ls.amr.corp.intel.com>
Message-ID: <Zh6JndoNGqEhCpCR@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: Isaku Yamahata <isaku.yamahata@intel.com>
Cc: Rick P Edgecombe <rick.p.edgecombe@intel.com>, "kvm@vger.kernel.org" <kvm@vger.kernel.org>, 
	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>, "pbonzini@redhat.com" <pbonzini@redhat.com>, 
	isaku.yamahata@linux.intel.com
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Apr 15, 2024, Isaku Yamahata wrote:
> On Mon, Apr 15, 2024 at 02:17:02PM -0700,
> Sean Christopherson <seanjc@google.com> wrote:
>=20
> > > > - Return error on guest mode or SMM mode:=C2=A0 Without this patch.
> > > > =C2=A0 Pros: No additional patch.
> > > > =C2=A0 Cons: Difficult to use.
> > >=20
> > > 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
> > And the fact there are so variables to control (MAXPHADDR, SMM, and gue=
st_mode)
> > basically invalidates the argument that returning an error makes the io=
ctl() hard
> > to use.  I can imagine it might be hard to squeeze this ioctl() into QE=
MU's
> > existing code, but I don't buy that the ioctl() itself is hard to use.
> >=20
> > Literally the only thing userspace needs to do is set CPUID to implicit=
ly select
> > between 4-level and 5-level paging.  If userspace wants to pre-map memo=
ry during
> > live migration, or when jump-starting the guest with pre-defined state,=
 simply
> > pre-map memory before stuffing guest state.  In and of itself, that doe=
sn't seem
> > difficult, e.g. at a quick glance, QEMU could add a hook somewhere in
> > kvm_vcpu_thread_fn() without too much trouble (though that comes with a=
 huge
> > disclaimer that I only know enough about how QEMU manages vCPUs to be d=
angerous).
> >=20
> > I would describe the overall cons for this patch versus returning an er=
ror
> > differently.  Switching MMU state puts the complexity in the kernel.  R=
eturning
> > an error punts any complexity to userspace.  Specifically, anything tha=
t KVM can
> > do regarding vCPU state to get the right MMU, userspace can do too.
> > =20
> > Add on that silently doing things that effectively ignore guest state u=
sually
> > ends badly, and I don't see a good argument for this patch (or any vari=
ant
> > thereof).
>=20
> Ok, here is a experimental patch on top of the 7/10 to return error.  Is =
this
> a direction? or do we want to invoke KVM page fault handler without any c=
heck?
>=20
> I can see the following options.
>=20
> - Error if vCPU is in SMM mode or guest mode: This patch
>   Defer the decision until the use cases come up.  We can utilize
>   KVM_CAP_MAP_MEMORY and struct kvm_map_memory.flags for future
>   enhancement.
>   Pro: Keep room for future enhancement for unclear use cases to defer
>        the decision.
>   Con: The use space VMM has to check/switch the vCPU mode.
>=20
> - No check of vCPU mode and go on
>   Pro: It works.
>   Con: Unclear how the uAPI should be without concrete use cases.
>=20
> - Always populate with L1 GPA:
>   This is a bad idea.

Not always.  If L1 is using shadow paging, then L1 and L2 GPAs are in the s=
ame
domain from KVM's perspective.

As I said in v1 (I think it was v1), whether or not mapping an L1 GPA is su=
pported
should be a property of the mmu, not an explicit check.  As far as the TDP =
MMU is
concerend, there's nothing special about SMM nor is there anything special =
about
guest_mode, except for the fact that they use different roots than "normal"=
 mode.
But that part Just Works.

And if L1 is using TDP, i.e. KVM is shadowing L1's TDP page tables, and L2 =
is
active, then vcpu->arch.mmu will point at a variation of the shadow MMU, e.=
g.
the PTTYPE_EPT MMU on Intel CPUs.  The shadow MMU won't support pre-mapping=
 GPAs
because it's non-sensical (legacy shadow paging needs a GVA, nested TDP nee=
ds an
L2 GPA), and so the ioctl() fails because mmu->map_gpa or whatever is NULL.

In other words, unless I'm forgetting something, explicit checks for "unsup=
ported"
modes shoud be unnecessary, because