Received-SPF: pass (google.com: domain of linux-kernel+bounces-136091-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 8 Apr 2024 18:37:31 -0700
In-Reply-To: <957b26d18ba7db611ed6582366066667267d10b8.camel@intel.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240405165844.1018872-1-seanjc@google.com> <73b40363-1063-4cb3-b744-9c90bae900b5@intel.com>
 <ZhQZYzkDPMxXe2RN@google.com> <a17c6f2a3b3fc6953eb64a0c181b947e28bb1de9.camel@intel.com>
 <ZhQ8UCf40UeGyfE_@google.com> <5faaeaa7bc66dbc4ea86a64ef8e8f9b22fd22ef4.camel@intel.com>
 <ZhRxWxRLbnrqwQYw@google.com> <957b26d18ba7db611ed6582366066667267d10b8.camel@intel.com>
Message-ID: <ZhSb28hHoyJ55-ga@google.com>
Subject: Re: [ANNOUNCE] PUCK Notes - 2024.04.03 - TDX Upstreaming Strategy
From: Sean Christopherson <seanjc@google.com>
To: Rick P Edgecombe <rick.p.edgecombe@intel.com>
Cc: "davidskidmore@google.com" <davidskidmore@google.com>, Xiaoyao Li <xiaoyao.li@intel.com>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, 
	"srutherford@google.com" <srutherford@google.com>, "pankaj.gupta@amd.com" <pankaj.gupta@amd.com>, 
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>, Isaku Yamahata <isaku.yamahata@intel.com>, 
	Wei W Wang <wei.w.wang@intel.com>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Apr 08, 2024, Rick P Edgecombe wrote:
> On Mon, 2024-04-08 at 15:36 -0700, Sean Christopherson wrote:
> > > Currently the values for the directly settable CPUID leafs come via a=
 TDX
> > > specific init VM userspace API.
> >=20
> > Is guest.MAXPHYADDR one of those?=C2=A0 If so, use that.
>=20
> No it is not configurable. I'm looking into make it configurable, but it =
is not
> likely to happen before we were hoping to get basic support upstream.

Yeah, love me some hardware defined software.

> An alternative would be to have the KVM API peak at the value, and then
> discard it (not pass the leaf value to the TDX module). Not ideal.

Heh, I typed up this idea before reading ahead.  This has my vote.  Unless =
I'm
misreading where things are headed, using guest.MAXPHYADDR to communicate w=
hat
is essentially GPAW to the guest is about to become the de facto standard.

At that point, KVM can basically treat the current TDX module behavior as a=
n
erratum, i.e. discarding guest.MAXPHYADDR becomes a workaround for a "CPU" =
bug,
not some goofy KVM quirk.

> Or have a dedicated GPAW field and expose the concept to userspace like
> Xiaoyao was talking about.

I'd prefer not to.  As above, it's not KVM's fault that the TDX module can'=
t move
fast enough to adapt.

> > > So should we look at making the TDX side follow a
> > > KVM_GET_SUPPORTED_CPUID/KVM_SET_CPUID pattern for feature enablement?=
 Or am
> > > I
> > > misreading general guidance out of this specific suggestion around GP=
AW?=20
> >=20
> > No?=C2=A0 Where I was going with that, is _if_ vCPUs can be created (in=
 KVM) before
> > the GPAW is set (in the TDX module), then using vCPU0's guest.MAXPHYADD=
R tokkk
> > compute the desired GPAW may be the least awful solution, all things
> > considered.
>=20
> Sorry, I was trying to uplevel the conversation to be about the general c=
oncept
> of matching TD configuration to CPUID bits. Let me try to articulate the =
problem
> a little better.
>=20
> Today, KVM=E2=80=99s KVM_GET_SUPPORTED_CPUID is a way to specify which fe=
atures are
> virtualizable by KVM. Communicating this via CPUID leaf values works for =
the
> most part, because CPUID is already designed to communicate which feature=
s are
> supported. But TDX has a different language to communicate which features=
 are
> supported. That is special fields that are passed when creating a VM: XFA=
M
> (matching XCR0 features) and ATTRIBUTES (TDX specific flags for MSR based
> features like PKS, etc). So compared to KVM_GET_SUPPORTED_CPUID/KVM_SET_C=
PUID,
> the TDX module instead accepts only a few CPUID bits to be set directly b=
y the
> VMM, and sets other CPUID leafs to match the configured features via XFAM=
 and
> ATTRIBUTES.
>=20
> There are also some bits/features that have fixed values. Which leafs are=
 fixed
> and what the values are isn't something provided by any current TDX modul=
e API.
> Instead they are only known via documentation, which is subject to change=
 The
> queryable information is limited to communicating which bits are directly
> configurable.=20

As I said in PUCK (and recorded in the notes), the fixed values should be p=
rovided
in a data format that is easily consumed by C code, so that KVM can report =
that
to userspace with

> So the current interface won't allow us to perfectly match the
> KVM_GET_SUPPORTED_CPUID/KVM_SET_CPUID. Even excluding the vm-scoped vs vc=
pu-
> scoped differences. However, we could try to match the general design a
> little better.

No, don't try to match KVM_GET_SUPPORTED_CPUID, it's a terrible API that no=
 one
likes.  The only reason we haven't replaced is because no one has come up w=
ith a
universally better idea.  For feature flags, communicating what KVM support=
s is
straightforward, mostly.  But for things like topology, communicating exact=
ly what
KVM "supports" is much more difficult.

The TDX fixed bits are very different.  It's the TDX module, and thus KVM, =
saying
"here are the bits that you _must_ set to these exact values".

> Here we were discussing making gpaw configurable via a dedicated named fi=
eld,
> but the suggestion is to instead include it in CPUID bits. The current AP=
I takes
> ATTRIBUTES as a dedicated field too. But there actually are CPUID bits fo=
r some
> of those features. Those CPUID bits are controlled instead via the associ=
ated
> ATTRIBUTES. So we could expose such features via CPUID as well. Userspace=
 would
> for example, pass the PKS CPUID bit in, and KVM would see it and configur=
e PKS
> via the ATTRIBUTES bit.
>=20
> So what I was looking to understand is, what is the enthusiasm for genera=
lly
> continuing to use CPUID has the main method for specifying which features=
 should
> be enabled/virtualized, if we can't match the existing
> KVM_GET_SUPPORTED_CPUID/KVM_SET_CPUID APIs. Is the hope just to make user=
space's
> code more unified between TDX and normal VMs?

I need to look at the TDX code more to form an (updated) opinion.  IIRC, my=
 opinion
from four years ago was to use ATTRIBUTES and then force CPUID to match.  W=
hether
or not that's still my preferred approach probably depends on how many, and=
 what,
things are shoved into attributes.