Received-SPF: pass (google.com: domain of linux-kernel+bounces-141533-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Thu, 11 Apr 2024 12:26:45 -0700
From: Isaku Yamahata <isaku.yamahata@intel.com>
To: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Yamahata, Isaku" <isaku.yamahata@intel.com>,
	"Zhang, Tina" <tina.zhang@intel.com>,
	"seanjc@google.com" <seanjc@google.com>,
	"Yuan, Hang" <hang.yuan@intel.com>,
	"Huang, Kai" <kai.huang@intel.com>, "Chen, Bo2" <chen.bo@intel.com>,
	"sagis@google.com" <sagis@google.com>,
	"isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>,
	"Aktas, Erdem" <erdemaktas@google.com>,
	"Li, Xiaoyao" <xiaoyao.li@intel.com>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	isaku.yamahata@linux.intel.com
Subject: Re: [PATCH v19 039/130] KVM: TDX: initialize VM with TDX specific
 parameters
Message-ID: <20240411192645.GE3039520@ls.amr.corp.intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <5eca97e6a3978cf4dcf1cff21be6ec8b639a66b9.1708933498.git.isaku.yamahata@intel.com>
 <c11cd64487f8971f9cfa880bface2076eb5b8b6d.camel@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <c11cd64487f8971f9cfa880bface2076eb5b8b6d.camel@intel.com>

On Mon, Apr 08, 2024 at 06:38:56PM +0000,
"Edgecombe, Rick P" <rick.p.edgecombe@intel.com> wrote:

> On Mon, 2024-02-26 at 00:25 -0800, isaku.yamahata@intel.com wrote:
> > +static int setup_tdparams_xfam(struct kvm_cpuid2 *cpuid, struct td_params *td_params)
> > +{
> > +       const struct kvm_cpuid_entry2 *entry;
> > +       u64 guest_supported_xcr0;
> > +       u64 guest_supported_xss;
> > +
> > +       /* Setup td_params.xfam */
> > +       entry = kvm_find_cpuid_entry2(cpuid->entries, cpuid->nent, 0xd, 0);
> > +       if (entry)
> > +               guest_supported_xcr0 = (entry->eax | ((u64)entry->edx << 32));
> > +       else
> > +               guest_supported_xcr0 = 0;
> > +       guest_supported_xcr0 &= kvm_caps.supported_xcr0;
> > +
> > +       entry = kvm_find_cpuid_entry2(cpuid->entries, cpuid->nent, 0xd, 1);
> > +       if (entry)
> > +               guest_supported_xss = (entry->ecx | ((u64)entry->edx << 32));
> > +       else
> > +               guest_supported_xss = 0;
> > +
> > +       /*
> > +        * PT and CET can be exposed to TD guest regardless of KVM's XSS, PT
> > +        * and, CET support.
> > +        */
> > +       guest_supported_xss &=
> > +               (kvm_caps.supported_xss | XFEATURE_MASK_PT | TDX_TD_XFAM_CET);
> 
> So this enables features based on xss support in the passed CPUID, but these features are not
> dependent xsave. You could have CET without xsave support. And in fact Kernel IBT doesn't use it. To
> utilize CPUID leafs to configure features, but diverge from the HW meaning seems like asking for
> trouble.

TDX module checks the consistency.  KVM can rely on it not to re-implement it.
The TDX Base Architecture specification describes what check is done.
Table 11.4: Extended Features Enumeration and Execution Control

> > +
> > +       td_params->xfam = guest_supported_xcr0 | guest_supported_xss;
> > +       if (td_params->xfam & XFEATURE_MASK_LBR) {
> > +               /*
> > +                * TODO: once KVM supports LBR(save/restore LBR related
> > +                * registers around TDENTER), remove this guard.
> > +                */
> > +#define MSG_LBR        "TD doesn't support LBR yet. KVM needs to save/restore IA32_LBR_DEPTH
> > properly.\n"
> > +               pr_warn(MSG_LBR);
> > +               return -EOPNOTSUPP;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int setup_tdparams(struct kvm *kvm, struct td_params *td_params,
> > +                       struct kvm_tdx_init_vm *init_vm)
> > +{
> > +       struct kvm_cpuid2 *cpuid = &init_vm->cpuid;
> > +       int ret;
> > +
> > +       if (kvm->created_vcpus)
> > +               return -EBUSY;
> > +
> > +       if (init_vm->attributes & TDX_TD_ATTRIBUTE_PERFMON) {
> > +               /*
> > +                * TODO: save/restore PMU related registers around TDENTER.
> > +                * Once it's done, remove this guard.
> > +                */
> > +#define MSG_PERFMON    "TD doesn't support perfmon yet. KVM needs to save/restore host perf
> > registers properly.\n"
> > +               pr_warn(MSG_PERFMON);
> 
> We need to remove the TODOs and a warn doesn't seem appropriate.

Sure, let me drop them.


> > +               return -EOPNOTSUPP;
> > +       }
> > +
> > +       td_params->max_vcpus = kvm->max_vcpus;
> > +       td_params->attributes = init_vm->attributes;
> 
> Don't we need to sanitize this for a selection of features known to KVM. For example what if
> something else like TDX_TD_ATTRIBUTE_PERFMON is added to a future TDX module and then suddenly
> userspace can configure it.
> 
> So xfam is how to control features that are tied to save (CET, etc). And ATTRIBUTES are tied to
> features without xsave support (PKS, etc).
> 
> If we are going to use CPUID for specifying which features should get enabled in the TDX module, we
> should match the arch definitions of the leafs. For things like CET whether xfam controls the value
> of multiple CPUID leafs, then we need should check that they are all set to some consistent values
> and otherwise reject them. So for CET we would need to check the SHSTK and IBT bits, as well as two
> XCR0 bits.
> 
> If we are going to do that for XFAM based features, then why not do the same for ATTRIBUTE based
> features?
> 
> We would need something like GET_SUPPORTED_CPUID for TDX, but also since some features can be forced
> on we would need to expose something like GET_SUPPORTED_CPUID_REQUIRED as well. 

I agree to reject attributes unknown to KVM.  Let's add the check.

The TDX module checks consistency between attributes, xfam, and cpuids as
described in the spec, KVM can rely on it.  When TDX module finds inconsistency
(or anything bad), it returns error as SEAMCALL error status code.  It includes
which cpuid is bad.  KVM returns it to the userspace VMM in struct
kvm_tdx_cmd.error.  We don't have to re-implement similar checks.
-- 
Isaku Yamahata <isaku.yamahata@intel.com>