Date: Thu, 11 Apr 2024 07:22:44 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Rick P Edgecombe <rick.p.edgecombe@...el.com>
Cc: "davidskidmore@...gle.com" <davidskidmore@...gle.com>, Xiaoyao Li <xiaoyao.li@...el.com>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"srutherford@...gle.com" <srutherford@...gle.com>, "pankaj.gupta@....com" <pankaj.gupta@....com>, 
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>, Isaku Yamahata <isaku.yamahata@...el.com>, 
	Wei W Wang <wei.w.wang@...el.com>
Subject: Re: [ANNOUNCE] PUCK Notes - 2024.04.03 - TDX Upstreaming Strategy

On Thu, Apr 11, 2024, Rick P Edgecombe wrote:
> On Tue, 2024-04-09 at 09:26 -0700, Sean Christopherson wrote:
> > > Haha, if this is the confusion, I see why you reacted that way to "JSON".
> > > That would be quite the curious choice for a TDX module API.
> > > 
> > > So it is easy to convert it to a C struct and embed it in KVM. It's just not
> > > that useful because it will not necessarily be valid for future TDX modules.
> > 
> > No, I don't want to embed anything in KVM, that's the exact same as hardcoding
> > crud into KVM, which is what I want to avoid.  I want to be able to roll out a
> > new TDX module with any kernel changes, and I want userspace to be able to
> > assert
> > that, for a given TDX module, the effective guest CPUID configuration aligns
> > with
> > userspace's desired the vCPU model, i.e. that the value of fixed bits match up
> > with the guest CPUID that userspace wants to define.
> > 
> > Maybe that just means converting the JSON file into some binary format that
> > the
> > kernel can already parse.  But I want Intel to commit to providing that
> > metadata
> > along with every TDX module.
> 
> Oof. It turns out in one of the JSON files there is a description of a different
> interface (TDX module runtime interface) that provides a way to read CPUID data
> that is configured in a TD, including fixed bits. It works like:
> 1. VMM queries which CPUID bits are directly configurable.
> 2. VMM provides directly configurable CPUID bits, along with XFAM and
> ATTRIBUTES, via TDH.MNG.INIT. (KVM_TDX_INIT_VM)
> 3. Then VMM can use this other interface via TDH.MNG.RD, to query the resulting
> values of specific CPUID leafs.
> 
> This does not provide a way to query the fixed bits specifically, it tells you
> what ended up getting configuring in a specific TD, which includes the fixed
> bits and anything else. So we need to do KVM_TDX_INIT_VM before KVM_SET_CPUID in
> order to have something to check against. But there was discussion of
> KVM_SET_CPUID on CPU0 having the CPUID state to pass to KVM_TDX_INIT_VM. So that
> would need to be sorted.
> 
> If we pass the directly configurable values with KVM_TDX_INIT_VM, like we do
> today, then the data provided by this interface should allow us to check
> consistency between KVM_SET_CPUID and the actual configured TD CPUID behavior.

I think it would be a good (optional?) sanity check, e.g. KVM_BUG_ON() if the
post-KVM_TDX_INIT_VM CPUID set doesn't match KVM's internal data.  But that alone
provides a terrible experience for userspace.

 - The VMM would still need to hardcode knowledge of fixed bits, without a way
   to do a sanity check of its own.

 - Lack of a sanity check means the VMM can't fail VM creation early.

 - KVM_SET_CPUID2 doesn't have a way to inform userspace _which_ CPUID bits are
   "bad".

 - Neither userspace nor KVM can programming detect when bits are fixed vs.
   flexible.  E.g. it's not impossible that userspace would want to do X if a
   feature is fixed, but Y if it's flexible.

Powered by blists - more mailing lists