Received-SPF: pass (google.com: domain of linux-kernel+bounces-122187-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <9f5c6259-78e1-4470-a013-91392bf3cea5@intel.com>
Date: Thu, 28 Mar 2024 12:33:35 +1300
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v19 038/130] KVM: TDX: create/destroy VM structure
To: <isaku.yamahata@intel.com>, <kvm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
CC: <isaku.yamahata@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>,
	<erdemaktas@google.com>, Sean Christopherson <seanjc@google.com>, Sagi Shahar
	<sagis@google.com>, <chen.bo@intel.com>, <hang.yuan@intel.com>,
	<tina.zhang@intel.com>, Sean Christopherson <sean.j.christopherson@intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <7a508f88e8c8b5199da85b7a9959882ddf390796.1708933498.git.isaku.yamahata@intel.com>
Content-Language: en-US
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <7a508f88e8c8b5199da85b7a9959882ddf390796.1708933498.git.isaku.yamahata@intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MlY0OUNnSlBvU1QzNE9DWmF5bVd0U1JXS0JNek9HZ1ZWcVlwem1qZzVtWFFk?=
 =?utf-8?B?RC9mNWNjUlJNS3k2ejJkWllVWFlML1hRYktJSDlGckQ2WDNwaU05NHhmTUR6?=
 =?utf-8?B?cWVRTHpjL1g1R3BhSk10TkkrZWRXeG5BcG1oQUlUN2hQY0IxZm8yWXhPanYw?=
 =?utf-8?B?RGRETHEzUmZTN0pkMGpvUXIyUTljVGpmWnIzajRIamU5dDZSSUJmY3lubTA3?=
 =?utf-8?B?cGVnOURKemJWMmE0bDcyVFp6bFJDVHB6ZzhsdnhqRW9yZU1HSFNpQmxPYWpO?=
 =?utf-8?B?eFZ1Z081d0JwaFpaU0Z0MWgwUEFuZ3QzRllhdUd6WEhaeXA2WWRHWXFEb1pC?=
 =?utf-8?B?dHE3ZkFQRnpWVHVHTzFRbStCSnBhY3JCa3h6QStOekYwM0l5WXNMbWcvZTVq?=
 =?utf-8?B?eHBsYmlmaUZFcVVOcDllSlhEc29BY0RQQUdKNXdnY3IyRkJvMnR6SENmQTNw?=
 =?utf-8?B?bTBZdUNPVFdLVml6SEVDK01oVUZ1UEd1bURVQW9GTnkwaFBXd0o5YWRhTHg0?=
 =?utf-8?B?NHl5aHVZWldFYkZrRnZNam1QQy9raEQ3Y3JEd3N2WnI0QUhTL0NZK2tQYTNK?=
 =?utf-8?B?MEpyZGZRY0c4K1YrejJTWTBzZTkwWFRLaXZra3crVU9zSnYreVRxcEYrSXdx?=
 =?utf-8?B?MDl2Mm5TRnRSNzUxdVpkblRHWGFKMS9wT0lxR0drNDRZb0tkd241dTNTdHFl?=
 =?utf-8?B?L1g4aGErQUhFK3NwT3FpU0tFL3JjUTFUY1orVm9JUUJSWUhxa2kzY1g1aEF1?=
 =?utf-8?B?MHVmWjVpODNmdHc3bGVOb3ZQTmxEemFzSkgwV0JnVm5kRHk2V2FxUXpuR0pw?=
 =?utf-8?B?aEt4Y3lTK1R4YzQwRGZTQ3YxODhQZHl6amJBZGVsMEpoZWM5Z2pqWU8yTUpa?=
 =?utf-8?B?WlNORk1yUkdFWTNDbVNZWDRuQTBkOWNmQlpWbE83RFBtMXN5bnh2UjBGVHox?=
 =?utf-8?B?K2h6TWlwN3ZWQVp1T1laS2h5Y2dmUnBHckg0ZDRlNnlxUEJ4b3JucFhMVXkx?=
 =?utf-8?B?MTQzMUZVUnE3L0xlZVo0VVY0T1pHa285TVo3V0hsMFVac0l2YXlpdFMxMjBj?=
 =?utf-8?B?Z2NDcmpmMTE2M05RQzduUjZyVUxBVzRhbGZhTXpUMlc3WmJLczdqa25Bd1Bq?=
 =?utf-8?B?QVFIZUZQQXZXVUlZcGZUblYrV2Y5ZHFlc1pGWEkrcUUzeTVrb0FkMkpaRkpu?=
 =?utf-8?B?MnRQUUtRcHNUdHAwanBOQkJxL0lmTXZJVGdzZGI5aVZKWVMxd29VUzVpN0NY?=
 =?utf-8?B?ekNxY0JCaVFSQmt5WmF0MjNmaW91MXNsM1lKc3FJd0hMZHBKUlIwblpEOTVJ?=
 =?utf-8?B?VTZLam5JL2tQd25XWkozV1RSSVFxUVByc2I3YVpMdVFVcnhhZWhxU2pPYmd6?=
 =?utf-8?B?bGFJUFhzclBWMFk2UGdnUjlzUk5PUktyUVRrMWFjaXJlcjM2YkZhYmVnc1RE?=
 =?utf-8?B?L3BMdVdjRDIvQis0NWptR05FdW5tZmpMdUVVT25xZHdmaDVwU0xXSlpFZENl?=
 =?utf-8?B?VmdQYnY4TmcvL3M4NVBzZjIvVW1MWjBHUW0yU1NoUGJQSTBmSjdPUmM0Um1B?=
 =?utf-8?B?YXdablNSWS92eXI2Y2tLSllxYUZETEVjUUFncXNsblJ3ZGI0V2NxUVZsNVVC?=
 =?utf-8?B?QkZPdnhOS3MvWTk3QXdnTWIxVzF0MVhxT3pJc29UUW9GeldHS0JocDlxQUxP?=
 =?utf-8?B?cjlmb3ZxdGVXOXZocDZZOTB0REtJS3ZIK2xGZkZoa3kxSUUvcmhFMjBXTVNE?=
 =?utf-8?B?a21ySkF2VEZ5Vkx0aFVqMEdIK1RsaXdoTUUvcnYzS3h0WDFoZTl3c0Z1MjBG?=
 =?utf-8?B?dFdveTlwWTJYLzg0aWkvb3J5U1IycFo4bjc2V1BXSzNKVzlFVDhpOWYxQXgy?=
 =?utf-8?B?aWlqLzZCbFlvaXpUdUMvSFBLeUlaVG0yZVBJaC9FNDlkSWRBcEM5YWxRc2Mr?=
 =?utf-8?B?MmRtSlpSNFMvS1dhZktScmtkTFF6RE9ldUl6WFJjdGNzMVIrZllBK3kzM2M2?=
 =?utf-8?B?N2l1NE1VMitRT3Q1OTNJdEZxTU1UY1M5NlR4NDF2SFRXWS9IMlBLb1FaY28z?=
 =?utf-8?B?dlFmcFA0eGdNbUhibXdnNnhuOGNsLytqSG5qUHpiN3NyKzlhNTN4ZUJONHhj?=
 =?utf-8?Q?0NoB3mcTzyqX+yKTf1duzAaOH?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e1dc620c-cbf2-4b76-bb65-08dc4eb6589f
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Mar 2024 23:33:45.9402
 (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: JzPTlLzFlEQ1ynSdzx1qR0/2qLiabuMIWiV+Jc39SziHOwS6MLiBIl4JTceQ7uDxHh7iCKnonu+tMz4BRO6E2A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7971
X-OriginatorOrg: intel.com

.. to continue the previous review ...

>   
> +static int __tdx_td_init(struct kvm *kvm)
> +{
> +	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> +	cpumask_var_t packages;
> +	unsigned long *tdcs_pa = NULL;
> +	unsigned long tdr_pa = 0;
> +	unsigned long va;
> +	int ret, i;
> +	u64 err;
> +
> +	ret = tdx_guest_keyid_alloc();
> +	if (ret < 0)
> +		return ret;
> +	kvm_tdx->hkid = ret;
> +
> +	va = __get_free_page(GFP_KERNEL_ACCOUNT);
> +	if (!va)
> +		goto free_hkid;
> +	tdr_pa = __pa(va);
> +
> +	tdcs_pa = kcalloc(tdx_info->nr_tdcs_pages, sizeof(*kvm_tdx->tdcs_pa),
> +			  GFP_KERNEL_ACCOUNT | __GFP_ZERO);
> +	if (!tdcs_pa)
> +		goto free_tdr;

Empty line.

> +	for (i = 0; i < tdx_info->nr_tdcs_pages; i++) {
> +		va = __get_free_page(GFP_KERNEL_ACCOUNT);
> +		if (!va)
> +			goto free_tdcs;
> +		tdcs_pa[i] = __pa(va);
> +	}
> +
> +	if (!zalloc_cpumask_var(&packages, GFP_KERNEL)) {
> +		ret = -ENOMEM;
> +		goto free_tdcs;
> +	}

Empty line.

> +	cpus_read_lock();
> +	/*
> +	 * Need at least one CPU of the package to be online in order to
> +	 * program all packages for host key id.  Check it.
> +	 */
> +	for_each_present_cpu(i)
> +		cpumask_set_cpu(topology_physical_package_id(i), packages);
> +	for_each_online_cpu(i)
> +		cpumask_clear_cpu(topology_physical_package_id(i), packages);
> +	if (!cpumask_empty(packages)) {
> +		ret = -EIO;
> +		/*
> +		 * Because it's hard for human operator to figure out the
> +		 * reason, warn it.
> +		 */
> +#define MSG_ALLPKG	"All packages need to have online CPU to create TD. Online CPU and retry.\n"
> +		pr_warn_ratelimited(MSG_ALLPKG);
> +		goto free_packages;
> +	}
> +
> +	/*
> +	 * Acquire global lock to avoid TDX_OPERAND_BUSY:
> +	 * TDH.MNG.CREATE and other APIs try to lock the global Key Owner
> +	 * Table (KOT) to track the assigned TDX private HKID.  It doesn't spin
> +	 * to acquire the lock, returns TDX_OPERAND_BUSY instead, and let the
> +	 * caller to handle the contention.  This is because of time limitation
> +	 * usable inside the TDX module and OS/VMM knows better about process
> +	 * scheduling.
> +	 *
> +	 * APIs to acquire the lock of KOT:
> +	 * TDH.MNG.CREATE, TDH.MNG.KEY.FREEID, TDH.MNG.VPFLUSHDONE, and
> +	 * TDH.PHYMEM.CACHE.WB. > +	 */

Don't need to mention all SEAMCALLs here, but put a comment where 
appliciable, i.e., where they are used.

	/*
	 * TDH.MNG.CREATE tries to grab the global TDX module and fails
	 * with TDX_OPERAND_BUSY when it fails to grab.  Take the global
	 * lock to prevent it from failure.
	 */
> +	mutex_lock(&tdx_lock);
> +	err = tdh_mng_create(tdr_pa, kvm_tdx->hkid);
> +	mutex_unlock(&tdx_lock);

Empty line.

> +	if (err == TDX_RND_NO_ENTROPY) {
> +		ret = -EAGAIN;
> +		goto free_packages;
> +	}
> +	if (WARN_ON_ONCE(err)) {
> +		pr_tdx_error(TDH_MNG_CREATE, err, NULL);
> +		ret = -EIO;
> +		goto free_packages;
> +	}

I would prefer more empty lines.

> +	kvm_tdx->tdr_pa = tdr_pa;
> +
> +	for_each_online_cpu(i) {
> +		int pkg = topology_physical_package_id(i);
> +
> +		if (cpumask_test_and_set_cpu(pkg, packages))
> +			continue;
> +
> +		/*
> +		 * Program the memory controller in the package with an
> +		 * encryption key associated to a TDX private host key id
> +		 * assigned to this TDR.  Concurrent operations on same memory
> +		 * controller results in TDX_OPERAND_BUSY.  Avoid this race by
> +		 * mutex.
> +		 */

IIUC the race can only happen when you are creating multiple TDX guests 
simulatenously?  Please clarify this in the comment.

And I even don't think you need all these TDX module details:

		/*
		 * Concurrent run of TDH.MNG.KEY.CONFIG on the same
		 * package resluts in TDX_OPERAND_BUSY.  When creating
		 * multiple TDX guests simultaneously this can run
		 * concurrently.  Take the per-package lock to
		 * serialize.
		 */
> +		mutex_lock(&tdx_mng_key_config_lock[pkg]);
> +		ret = smp_call_on_cpu(i, tdx_do_tdh_mng_key_config,
> +				      &kvm_tdx->tdr_pa, true);
> +		mutex_unlock(&tdx_mng_key_config_lock[pkg]);
> +		if (ret)
> +			break;
> +	}
> +	cpus_read_unlock();
> +	free_cpumask_var(packages);
> +	if (ret) {
> +		i = 0;
> +		goto teardown;
> +	}
> +
> +	kvm_tdx->tdcs_pa = tdcs_pa;
> +	for (i = 0; i < tdx_info->nr_tdcs_pages; i++) {
> +		err = tdh_mng_addcx(kvm_tdx->tdr_pa, tdcs_pa[i]);
> +		if (err == TDX_RND_NO_ENTROPY) {
> +			/* Here it's hard to allow userspace to retry. */
> +			ret = -EBUSY;
> +			goto teardown;
> +		}
> +		if (WARN_ON_ONCE(err)) {
> +			pr_tdx_error(TDH_MNG_ADDCX, err, NULL);
> +			ret = -EIO;
> +			goto teardown;
> +		}
> +	}
> +
> +	/*
> +	 * Note, TDH_MNG_INIT cannot be invoked here.  TDH_MNG_INIT requires a dedicated
> +	 * ioctl() to define the configure CPUID values for the TD.
> +	 */

Then, how about renaming this function to __tdx_td_create()?

> +	return 0;
> +
> +	/*
> +	 * The sequence for freeing resources from a partially initialized TD
> +	 * varies based on where in the initialization flow failure occurred.
> +	 * Simply use the full teardown and destroy, which naturally play nice
> +	 * with partial initialization.
> +	 */
> +teardown:
> +	for (; i < tdx_info->nr_tdcs_pages; i++) {
> +		if (tdcs_pa[i]) {
> +			free_page((unsigned long)__va(tdcs_pa[i]));
> +			tdcs_pa[i] = 0;
> +		}
> +	}
> +	if (!kvm_tdx->tdcs_pa)
> +		kfree(tdcs_pa);

The code to "free TDCS pages in a loop and free the array" is done below 
with duplicated code.  I am wondering whether we have way to eliminate one.

But I have lost track here, so perhaps we can review again after we 
split the patch to smaller pieces.

> +	tdx_mmu_release_hkid(kvm);
> +	tdx_vm_free(kvm);
> +	return ret;
> +
> +free_packages:
> +	cpus_read_unlock();
> +	free_cpumask_var(packages);
> +free_tdcs:
> +	for (i = 0; i < tdx_info->nr_tdcs_pages; i++) {
> +		if (tdcs_pa[i])
> +			free_page((unsigned long)__va(tdcs_pa[i]));
> +	}
> +	kfree(tdcs_pa);
> +	kvm_tdx->tdcs_pa = NULL;
> +
> +free_tdr:
> +	if (tdr_pa)
> +		free_page((unsigned long)__va(tdr_pa));
> +	kvm_tdx->tdr_pa = 0;
> +free_hkid:
> +	if (is_hkid_assigned(kvm_tdx))
> +		tdx_hkid_free(kvm_tdx);
> +	return ret;
> +}
> +
>   int tdx_vm_ioctl(struct kvm *kvm, void __user *argp)
>   {
>   	struct kvm_tdx_cmd tdx_cmd;
> @@ -215,12 +664,13 @@ static int tdx_md_read(struct tdx_md_map *maps, int nr_maps)
>   
>   static int __init tdx_module_setup(void)
>   {
> -	u16 num_cpuid_config;
> +	u16 num_cpuid_config, tdcs_base_size;
>   	int ret;
>   	u32 i;
>   
>   	struct tdx_md_map mds[] = {
>   		TDX_MD_MAP(NUM_CPUID_CONFIG, &num_cpuid_config),
> +		TDX_MD_MAP(TDCS_BASE_SIZE, &tdcs_base_size),
>   	};
>   
>   	struct tdx_metadata_field_mapping fields[] = {
> @@ -273,6 +723,8 @@ static int __init tdx_module_setup(void)
>   		c->edx = ecx_edx >> 32;
>   	}
>   
> +	tdx_info->nr_tdcs_pages = tdcs_base_size / PAGE_SIZE;
> +

Round up the 'tdcs_base_size' to make sure you have enough room, or put 
a WARN() here if not page aligned?

>   	return 0;
>   
>   error_out:
> @@ -319,13 +771,27 @@ int __init tdx_hardware_setup(struct kvm_x86_ops *x86_ops)
>   	struct tdx_enabled enable = {
>   		.err = ATOMIC_INIT(0),
>   	};
> +	int max_pkgs;
>   	int r = 0;
> +	int i;

Nit: you can put the 3 into one line.

>   
> +	if (!cpu_feature_enabled(X86_FEATURE_MOVDIR64B)) {
> +		pr_warn("MOVDIR64B is reqiured for TDX\n");

It's better to make it more clear:

"Disable TDX: MOVDIR64B is not supported or disabled by the kernel."

Or, to match below:

"Cannot enable TDX w/o MOVDIR64B".

> +		return -EOPNOTSUPP;
> +	}
>   	if (!enable_ept) {
>   		pr_warn("Cannot enable TDX with EPT disabled\n");
>   		return -EINVAL;
>   	}
>   
> +	max_pkgs = topology_max_packages();
> +	tdx_mng_key_config_lock = kcalloc(max_pkgs, sizeof(*tdx_mng_key_config_lock),
> +				   GFP_KERNEL);
> +	if (!tdx_mng_key_config_lock)
> +		return -ENOMEM;
> +	for (i = 0; i < max_pkgs; i++)
> +		mutex_init(&tdx_mng_key_config_lock[i]);
> +

Using a per-socket lock looks a little bit overkill to me.  I don't know 
whether we need to do in the initial version.  Will leave to others.

Please at least add a comment to explain this is for better performance 
when creating multiple TDX guests IIUC?

>   	if (!zalloc_cpumask_var(&enable.enabled, GFP_KERNEL)) {
>   		r = -ENOMEM;
>   		goto out;
> @@ -350,4 +816,5 @@ int __init tdx_hardware_setup(struct kvm_x86_ops *x86_ops)
>   void tdx_hardware_unsetup(void)
>   {
>   	kfree(tdx_info);
> +	kfree(tdx_mng_key_config_lock);

The kernel actually has a mutex_destroy().  It is empty when 
CONFIG_DEBUG_LOCK_ALLOC is off, but I think it should be standard 
proceedure to also mutex_destory()?

[...]