Received-SPF: pass (google.com: domain of linux-kernel+bounces-108482-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 20 Mar 2024 14:12:49 +0800
From: Chao Gao <chao.gao@intel.com>
To: <isaku.yamahata@intel.com>
CC: <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>
Subject: Re: [PATCH v19 039/130] KVM: TDX: initialize VM with TDX specific
 parameters
Message-ID: <Zfp+YWzHV0DxVf1+@chao-email>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <5eca97e6a3978cf4dcf1cff21be6ec8b639a66b9.1708933498.git.isaku.yamahata@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <5eca97e6a3978cf4dcf1cff21be6ec8b639a66b9.1708933498.git.isaku.yamahata@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?elr4NxiGitdawmDNXxZpWQ9TxbQfFu6FuC8PD5XZmRvzNgnL3QBVeR6zxrB6?=
 =?us-ascii?Q?iDu8ysevCxYtwRrq3nx8E7qWjLdOcMLsEKQq/hEFwXGk9Av+Ppk143luxKkG?=
 =?us-ascii?Q?fvNx/8pSLkznBy3SZlObaXYiFH6zDo/ZWC8gU2ozPMH/6VZowU0GIg/XrFDO?=
 =?us-ascii?Q?y5Cjtj4W8Bm41e52HZ0mBHQAvkIf3UxOdxu1X6VzQqMrNYUKhuceSQMSn7iF?=
 =?us-ascii?Q?iCtKTp00W64CV2uLy0W5A3fV+4e7KcQ0HCjlebQIYrYa3tiZJKEDUC49uhEh?=
 =?us-ascii?Q?3UbA4Odzs43w6y2IVR0beL6sStzEhfBjGVGAaBK8PADXS2YBDBtpsp9lt2Yf?=
 =?us-ascii?Q?CeI5fd5p+XznTtnNNXcoUjjPPZ4LAwp20GpHkOfd/4cRxNDJte02Sc1x8ZJJ?=
 =?us-ascii?Q?VlyNjDdcs7xSWnxEIMT3VvDWWIflalEcbDZ5CWltIc1GRtgujYWjWpX+mfKe?=
 =?us-ascii?Q?3EbJeMFleW9+7hmAKwD15xzed6TjZiHhJPJAqLKsRnIolhBfNwA/C8qEXIsE?=
 =?us-ascii?Q?pQ2U+f8bdHuz2TibhyuZigTEnnYEXlErJbJrcjZ+lWVBkLUX30IAXgeL6fT2?=
 =?us-ascii?Q?HgtmaRkI/8hONu7RJfCoVGjTtHAEO8+pILi7ptFLqP7dSnvdD7hmf6+OF4jM?=
 =?us-ascii?Q?h6r8TrFy7YKvWXWynhHiTHW9fdVuNBFAtQPSDIxsjp3tW323j+Iueh9n77AT?=
 =?us-ascii?Q?ju3mSRY3sgtx41gxD8w0aJaSe6X+LVtYGzl3zDyGFa/ZE0nUlpY5H9A7fe3b?=
 =?us-ascii?Q?VN+WoAW/Fq92+HoiJhOwqVJ0pvvqrnVvAZB9mmpAslrkFPqybah5jJIg7Ccl?=
 =?us-ascii?Q?b6TWFyfYH1x3oempi5m6kSCt5HMnPJLdSt+DiqjR8RBCd1tSwg5xUs+bIzc0?=
 =?us-ascii?Q?RtjdPyiKxa/smjyvHYiyR2J6B52Q2YkekasBO9hEkgx6hc4Cbl7Bl0oGVWj2?=
 =?us-ascii?Q?7AWMPutMcHmtGfvojCP7dChwPgY2cs+sUIZHvIjb4FyWor2/mGSLuyJB3iyl?=
 =?us-ascii?Q?vbhqTt7dzm5fxj5dXvaOP/jA1JP5YEIAGdN+pnMRii00PfF/mzxipoE/pzcF?=
 =?us-ascii?Q?ErT9p4YJXzR2iN2F5mk+87y8sAUW32xthL+T9nc3L73u6C8QFN31fqgRsmd5?=
 =?us-ascii?Q?qo0us/la8/R6MtjSgnlpZL1Lhcf1HigRVRPQPfwWXfc9e4r+xUeNFJ4CuhpL?=
 =?us-ascii?Q?2JtdpvkF6ukyIzOevOROoZCwnE+OAQI2yqRFldspfbmBzRm710zmWikQ2hW2?=
 =?us-ascii?Q?loc+iP75xmUK8sTlX8slZVi/xZiDrZwl7iHDGgelDPlyigND6+SstiwffIjz?=
 =?us-ascii?Q?y1xil8IguoVooX44jQbpMFjR7DCzUffmgAQbQyLfROPAECCCRwZmGupCYrsS?=
 =?us-ascii?Q?G9ewlp7PMsgGj6j40VVyfdhyyfaQuoXI3VQHktnEybspcqAf2JPfe2O1OorZ?=
 =?us-ascii?Q?gmv6bYHemgA0Q4fZ9zA7CdJEZqa0G9/efw7Q+GVb1V8mjxckbhRv6IkQz9G9?=
 =?us-ascii?Q?gGhg8Z7J/mwTLwC0Q7rwnVLZ8Mg4JzEQRqKhPxsvMtpa3CFiOmDuQbQRn0tA?=
 =?us-ascii?Q?xnZhl4KPAcz8apNBEbtFoYTlz5C+C+6ST8IeumbJ?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f4e52258-f8d9-45e5-dae5-08dc48a4ca60
X-MS-Exchange-CrossTenant-AuthSource: CH3PR11MB8660.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Mar 2024 06:12:59.1112
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: fOl1ys9QKckKhDBSsg9Hc2mlPR7VxTXLDIjyWtpP96H4t1tg089lIHQ7ssOwcdkO6jfEzLdETXY44tnFYb6lbQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR11MB4746
X-OriginatorOrg: intel.com

On Mon, Feb 26, 2024 at 12:25:41AM -0800, isaku.yamahata@intel.com wrote:
>From: Isaku Yamahata <isaku.yamahata@intel.com>

..

>
>TDX requires additional parameters for TDX VM for confidential execution to
>protect the confidentiality of its memory contents and CPU state from any
>other software, including VMM.  When creating a guest TD VM before creating
>vcpu, the number of vcpu, TSC frequency (the values are the same among
>vcpus, and it can't change.)  CPUIDs which the TDX module emulates.  Guest
>TDs can trust those CPUIDs and sha384 values for measurement.
>
>Add a new subcommand, KVM_TDX_INIT_VM, to pass parameters for the TDX
>guest.  It assigns an encryption key to the TDX guest for memory
>encryption.  TDX encrypts memory per guest basis.  The device model, say
>qemu, passes per-VM parameters for the TDX guest.  The maximum number of
>vcpus, TSC frequency (TDX guest has fixed VM-wide TSC frequency, not per
>vcpu.  The TDX guest can not change it.), attributes (production or debug),
>available extended features (which configure guest XCR0, IA32_XSS MSR),
>CPUIDs, sha384 measurements, etc.
>
>Call this subcommand before creating vcpu and KVM_SET_CPUID2, i.e.  CPUID
>configurations aren't available yet.  So CPUIDs configuration values need
>to be passed in struct kvm_tdx_init_vm.  The device model's responsibility
>to make this CPUID config for KVM_TDX_INIT_VM and KVM_SET_CPUID2.
>
>Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>

the SOB chain makes no sense.

>+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).

>+	}
>+}
>+
>+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.

>+		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;

-EINVAL

>+
>+	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);

drop the pr_warn().

>+		return -EOPNOTSUPP;
>+	}
>+
>+	td_params->max_vcpus = kvm->max_vcpus;
>+	td_params->attributes = init_vm->attributes;
>+	td_params->exec_controls = TDX_CONTROL_FLAG_NO_RBP_MOD;
>+	td_params->tsc_frequency = TDX_TSC_KHZ_TO_25MHZ(kvm->arch.default_tsc_khz);
>+
>+	ret = setup_tdparams_eptp_controls(cpuid, td_params);
>+	if (ret)
>+		return ret;
>+	setup_tdparams_cpuids(cpuid, td_params);
>+	ret = setup_tdparams_xfam(cpuid, td_params);
>+	if (ret)
>+		return ret;
>+
>+#define MEMCPY_SAME_SIZE(dst, src)				\
>+	do {							\
>+		BUILD_BUG_ON(sizeof(dst) != sizeof(src));	\
>+		memcpy((dst), (src), sizeof(dst));		\
>+	} while (0)
>+
>+	MEMCPY_SAME_SIZE(td_params->mrconfigid, init_vm->mrconfigid);
>+	MEMCPY_SAME_SIZE(td_params->mrowner, init_vm->mrowner);
>+	MEMCPY_SAME_SIZE(td_params->mrownerconfig, init_vm->mrownerconfig);
>+
>+	return 0;
>+}
>+
>+static int __tdx_td_init(struct kvm *kvm, struct td_params *td_params,
>+			 u64 *seamcall_err)
> {
> 	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
>+	struct tdx_module_args out;
> 	cpumask_var_t packages;
> 	unsigned long *tdcs_pa = NULL;
> 	unsigned long tdr_pa = 0;
>@@ -426,6 +581,7 @@ static int __tdx_td_init(struct kvm *kvm)
> 	int ret, i;
> 	u64 err;
> 
>+	*seamcall_err = 0;
> 	ret = tdx_guest_keyid_alloc();
> 	if (ret < 0)
> 		return ret;
>@@ -540,10 +696,23 @@ static int __tdx_td_init(struct kvm *kvm)
> 		}
> 	}
> 
>-	/*
>-	 * Note, TDH_MNG_INIT cannot be invoked here.  TDH_MNG_INIT requires a dedicated
>-	 * ioctl() to define the configure CPUID values for the TD.
>-	 */
>+	err = tdh_mng_init(kvm_tdx->tdr_pa, __pa(td_params), &out);
>+	if ((err & TDX_SEAMCALL_STATUS_MASK) == TDX_OPERAND_INVALID) {
>+		/*
>+		 * Because a user gives operands, don't warn.
>+		 * Return a hint to the user because it's sometimes hard for the
>+		 * user to figure out which operand is invalid.  SEAMCALL status
>+		 * code includes which operand caused invalid operand error.
>+		 */
>+		*seamcall_err = err;
>+		ret = -EINVAL;
>+		goto teardown;
>+	} else if (WARN_ON_ONCE(err)) {
>+		pr_tdx_error(TDH_MNG_INIT, err, &out);
>+		ret = -EIO;
>+		goto teardown;
>+	}
>+
> 	return 0;
> 
> 	/*
>@@ -586,6 +755,76 @@ static int __tdx_td_init(struct kvm *kvm)
> 	return ret;
> }
> 
>+static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
>+{
>+	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
>+	struct kvm_tdx_init_vm *init_vm = NULL;

no need to initialize it to NULL.

>+	struct td_params *td_params = NULL;
>+	int ret;
>+
>+	BUILD_BUG_ON(sizeof(*init_vm) != 8 * 1024);
>+	BUILD_BUG_ON(sizeof(struct td_params) != 1024);
>+
>+	if (is_hkid_assigned(kvm_tdx))
>+		return -EINVAL;
>+
>+	if (cmd->flags)
>+		return -EINVAL;
>+
>+	init_vm = kzalloc(sizeof(*init_vm) +
>+			  sizeof(init_vm->cpuid.entries[0]) * KVM_MAX_CPUID_ENTRIES,
>+			  GFP_KERNEL);

no need to zero the memory given ...

>+	if (!init_vm)
>+		return -ENOMEM;
>+	if (copy_from_user(init_vm, (void __user *)cmd->data, sizeof(*init_vm))) {

.. this.

>+		ret = -EFAULT;
>+		goto out;
>+	}
>+	if (init_vm->cpuid.nent > KVM_MAX_CPUID_ENTRIES) {
>+		ret = -E2BIG;
>+		goto out;
>+	}
>+	if (copy_from_user(init_vm->cpuid.entries,
>+			   (void __user *)cmd->data + sizeof(*init_vm),
>+			   flex_array_size(init_vm, cpuid.entries, init_vm->cpuid.nent))) {
>+		ret = -EFAULT;
>+		goto out;
>+	}
>+
>+	if (memchr_inv(init_vm->reserved, 0, sizeof(init_vm->reserved))) {
>+		ret = -EINVAL;
>+		goto out;
>+	}