Received-SPF: pass (google.com: domain of linux-kernel+bounces-110288-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 21 Mar 2024 08:55:13 -0700
From: Isaku Yamahata <isaku.yamahata@intel.com>
To: Chao Gao <chao.gao@intel.com>
Cc: isaku.yamahata@intel.com, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org, isaku.yamahata@gmail.com,
	Paolo Bonzini <pbonzini@redhat.com>, erdemaktas@google.com,
	Sean Christopherson <seanjc@google.com>,
	Sagi Shahar <sagis@google.com>, Kai Huang <kai.huang@intel.com>,
	chen.bo@intel.com, hang.yuan@intel.com, tina.zhang@intel.com,
	Xiaoyao Li <xiaoyao.li@intel.com>, isaku.yamahata@linux.intel.com
Subject: Re: [PATCH v19 039/130] KVM: TDX: initialize VM with TDX specific
 parameters
Message-ID: <20240321155513.GL1994522@ls.amr.corp.intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <5eca97e6a3978cf4dcf1cff21be6ec8b639a66b9.1708933498.git.isaku.yamahata@intel.com>
 <Zfp+YWzHV0DxVf1+@chao-email>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <Zfp+YWzHV0DxVf1+@chao-email>

On Wed, Mar 20, 2024 at 02:12:49PM +0800,
Chao Gao <chao.gao@intel.com> wrote:

> >+static void setup_tdparams_cpuids(struct kvm_cpuid2 *cpuid,
> >+				  struct td_params *td_params)
> >+{
> >+	int i;
> >+
> >+	/*
> >+	 * td_params.cpuid_values: The number and the order of cpuid_value must
> >+	 * be same to the one of struct tdsysinfo.{num_cpuid_config, cpuid_configs}
> >+	 * It's assumed that td_params was zeroed.
> >+	 */
> >+	for (i = 0; i < tdx_info->num_cpuid_config; i++) {
> >+		const struct kvm_tdx_cpuid_config *c = &tdx_info->cpuid_configs[i];
> >+		/* KVM_TDX_CPUID_NO_SUBLEAF means index = 0. */
> >+		u32 index = c->sub_leaf == KVM_TDX_CPUID_NO_SUBLEAF ? 0 : c->sub_leaf;
> >+		const struct kvm_cpuid_entry2 *entry =
> >+			kvm_find_cpuid_entry2(cpuid->entries, cpuid->nent,
> >+					      c->leaf, index);
> >+		struct tdx_cpuid_value *value = &td_params->cpuid_values[i];
> >+
> >+		if (!entry)
> >+			continue;
> >+
> >+		/*
> >+		 * tdsysinfo.cpuid_configs[].{eax, ebx, ecx, edx}
> >+		 * bit 1 means it can be configured to zero or one.
> >+		 * bit 0 means it must be zero.
> >+		 * Mask out non-configurable bits.
> >+		 */
> >+		value->eax = entry->eax & c->eax;
> >+		value->ebx = entry->ebx & c->ebx;
> >+		value->ecx = entry->ecx & c->ecx;
> >+		value->edx = entry->edx & c->edx;
> 
> Any reason to mask off non-configurable bits rather than return an error? this
> is misleading to userspace because guest sees the values emulated by TDX module
> instead of the values passed from userspace (i.e., the request from userspace
> isn't done but there is no indication of that to userspace).

Ok, I'll eliminate them.  If user space passes wrong cpuids, TDX module will
return error. I'll leave the error check to the TDX module.


> >+	}
> >+}
> >+
> >+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);
> >+
> >+	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);
> 
> Drop the pr_warn() because userspace can trigger it at will.
> 
> I don't think KVM needs to relay TDX module capabilities to userspace as-is.
> KVM should advertise a feature only if both TDX module's and KVM's support
> are in place. if KVM masked out LBR and PERFMON, it should be a problem of
> userspace and we don't need to warn here.

Makes sense. Drop those message and don't advertise those features to user
space.
-- 
Isaku Yamahata <isaku.yamahata@intel.com>