Received-SPF: pass (google.com: domain of linux-kernel+bounces-145921-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Mon, 15 Apr 2024 14:17:02 -0700
In-Reply-To: <116179545fafbf39ed01e1f0f5ac76e0467fc09a.camel@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>
Message-ID: <Zh2ZTt4tXXg0f0d9@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: Rick P Edgecombe <rick.p.edgecombe@intel.com>
Cc: "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>, "pbonzini@redhat.com" <pbonzini@redhat.com>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Apr 15, 2024, Rick P Edgecombe wrote:
> I wouldn't call myself much of an expert on nested, but...
>=20
> On Wed, 2024-04-10 at 15:07 -0700, isaku.yamahata@intel.com wrote:
> > There are several options to populate L1 GPA irrelevant to vCPU mode.
> > - Switch vCPU MMU only: This patch.
> > =C2=A0 Pros: Concise implementation.
> > =C2=A0 Cons: Heavily dependent on the KVM MMU implementation.

Con: Makes it impossible to support other MMUs/modes without extending the =
uAPI.

> Is switching just the MMU enough here? Won't the MTRRs and other vcpu bit=
s be
> wrong?
>=20
> > - Use kvm_x86_nested_ops.get/set_state() to switch to/from guest mode.
> > =C2=A0 Use __get/set_sregs2() to switch to/from SMM mode.
> > =C2=A0 Pros: straightforward.
> > =C2=A0 Cons: This may cause unintended side effects.
>=20
> Cons make sense.
>=20
> > - Refactor KVM page fault handler not to pass vCPU. Pass around necessa=
ry
> > =C2=A0 parameters and struct kvm.
> > =C2=A0 Pros: The end result will have clearly no side effects.
> > =C2=A0 Cons: This will require big refactoring.
>=20
> But doesn't the fault handler need the vCPU state?

Ignoring guest MTRRs, which will hopefully soon be a non-issue, no.  There =
are
only six possible roots if TDP is enabled:

      1. 4-level !SMM !guest_mode
      2. 4-level  SMM !guest_mode
      3. 5-level !SMM !guest_mode
      4. 5-level  SMM !guest_mode
      5. 4-level !SMM guest_mode
      6. 5-level !SMM guest_mode

4-level vs. 5-level is a guest MAXPHYADDR thing, and swapping the MMU elimi=
nates
the SMM and guest_mode issues.  If there is per-vCPU state that makes its w=
ay into
the TDP page tables, then we have problems, because it means that there is =
per-vCPU
state in per-VM structures that isn't accounted for.

There are a few edge cases where KVM treads carefully, e.g. if the fault is=
 to
the vCPU's APIC-access page, but KVM manually handles those to avoid consum=
ing
per-vCPU state.

That said, I think this option is effectively 1b, because dropping the SMM =
vs.
guest_mode state has the same uAPI problems as forcibly swapping the MMU, i=
t's
just a different way of doing so.

The first question to answer is, do we want to return an error or "silently=
"
install mappings for !SMM, !guest_mode.  And so this option becomes relevan=
t only
_if_ we want to unconditionally install mappings for the 'base" mode.

> > - 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 horribl=
e.

And the fact there are so variables to control (MAXPHADDR, SMM, and guest_m=
ode)
basically invalidates the argument that returning an error makes the ioctl(=
) hard
to use.  I can imagine it might be hard to squeeze this ioctl() into QEMU's
existing code, but I don't buy that the ioctl() itself is hard to use.

Literally the only thing userspace needs to do is set CPUID to implicitly s=
elect
between 4-level and 5-level paging.  If userspace wants to pre-map memory d=
uring
live migration, or when jump-starting the guest with pre-defined state, sim=
ply
pre-map memory before stuffing guest state.  In and of itself, that doesn'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 hug=
e
disclaimer that I only know enough about how QEMU manages vCPUs to be dange=
rous).

I would describe the overall cons for this patch versus returning an error
differently.  Switching MMU state puts the complexity in the kernel.  Retur=
ning
an error punts any complexity to userspace.  Specifically, anything that KV=
M 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 usual=
ly
ends badly, and I don't see a good argument for this patch (or any variant
thereof).