Received-SPF: pass (google.com: domain of linux-kernel+bounces-136051-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <80971062-9827-490e-a8eb-980a4f249e94@intel.com>
Date: Tue, 9 Apr 2024 12:37:14 +1200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v19 023/130] KVM: TDX: Initialize the TDX module when
 loading the KVM intel kernel module
To: "Yamahata, Isaku" <isaku.yamahata@intel.com>, "kvm@vger.kernel.org"
	<kvm@vger.kernel.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>
CC: "isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>, Paolo Bonzini
	<pbonzini@redhat.com>, "Aktas, Erdem" <erdemaktas@google.com>, "Sean
 Christopherson" <seanjc@google.com>, Sagi Shahar <sagis@google.com>, "Chen,
 Bo2" <chen.bo@intel.com>, "Yuan, Hang" <hang.yuan@intel.com>, "Zhang, Tina"
	<tina.zhang@intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <f028d43abeadaa3134297d28fb99f283445c0333.1708933498.git.isaku.yamahata@intel.com>
Content-Language: en-US
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <f028d43abeadaa3134297d28fb99f283445c0333.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?MFFHN1ZhaVFRRHJZMGUrRnA0RVJkblZ5eUJVNk5GdlJHMXpoOUdmV29XTnh6?=
 =?utf-8?B?S2hJd2kraGh2dXpnOEM2T2U4bUJWU2JUekhMOHZPMEhLOUlCV2d0dy9yUzBI?=
 =?utf-8?B?TFh1SUpBVVNXVTE2VDd1MDVqRnQ3UnZqZW9HbHl5OEk0UGxPY2hjaEdsdVVh?=
 =?utf-8?B?T3lBUDk4cUVQZ2tPVDNhUk54UEg2WFdPK1FWV2FDRXREZWIyYWloSVMvVlNP?=
 =?utf-8?B?U2hDWlhQUlkwcnBYTjdMMG9pL0k2OUx6TlRtL2JLT0xOaHNDUVhWV2RCSFVX?=
 =?utf-8?B?VVRKZFVPR2wvYTFtYzFnUTR6ZjhZS05MSzh0d1FDcGlhVXRVdkhncXlYYmd5?=
 =?utf-8?B?eDNJZUZ6M1ducHpiR1Brbm9QQjJTVCsrSm50VkpTWjZRb3dTUmxvMU42aEhQ?=
 =?utf-8?B?akd4ZlpnVmR1VmtyRkhSaXFHZkhJR0dGTFgxQWFUNEJoVC9DaVJCeis5cTNM?=
 =?utf-8?B?Q1p2S04yNU9SZTlJczNDUnRZdWxrWUE0RlF3NS9tKzliRUpQYndjWC9xc1pv?=
 =?utf-8?B?TFAvTTNiUzhqRWZPaDRNZEFRMHpnQ056ejM3N3FucjNLY3EzMDIxVEdWOHY1?=
 =?utf-8?B?OWY0enFlOGUrVUJISmhxNTR2eEplOHRDNDlvL1h3ZjhOM3pQM3ZpSGs2WGZT?=
 =?utf-8?B?NmlUbm1PbC9KUElWblVtS1R2S3hndytndlJVTnBEbm1mdkhNVlhrQkl3QXpz?=
 =?utf-8?B?Y2N6cFhweFExZ0YxWkR5RVlvVFByM2RyRnBRVTVGcElaYnlzUE9rUEU1T3A1?=
 =?utf-8?B?OGdlVXVnYmlsb1ptT2JuQWVYNmRxeTNhQXRPREVLY1Q0V01STG5sYllYMWkx?=
 =?utf-8?B?NWdJcTh3WEdOcUs3Tlk2eVNsZmZLVnpTYXRsYUpDQlhvaldSSlBhemlJM3dC?=
 =?utf-8?B?SEkrM1lwaWp4SmRESC8wUXd4YWlaVENZdkIrczBSVHkyRW14Z2NDZ2NCd3kw?=
 =?utf-8?B?c2Ntcll2MWRJMmpkaWxvTHpQMzVmeFliQ0t6bkpiT1RYZDFPZnp5YzJzUHNY?=
 =?utf-8?B?djA2RnBGUG1YcldpMlJkNTRFOFN6dkpiaENTeVExUjJnek5uQ2xFR25laEdv?=
 =?utf-8?B?cENHRDNncU5SMEFXejdqSERSN1VhbThuQ2QrZHpGNTJkajVRQTA5Uzg5U2VW?=
 =?utf-8?B?Q1FkL2wza0RiTEt1OXlaWERCaEJTRkNJSEVKRmwwNTllQWwrOEZQLzJKSXpr?=
 =?utf-8?B?cUMzMzZIWjZKWHV4UmpqYkpoNUtMNTVvVnp6REV2Rk5wSThISU5Nd2l6Q0F6?=
 =?utf-8?B?NFltRGlwcWw0VzNOalIrUksxaVlFcnZ2d29UR1BJZGJPVzZrMU5mbWV5UjJu?=
 =?utf-8?B?L2tsVXVDMExpWmZpbFdpVU95WDIrZklUZUZqSzQzZytTbmlHWmNGOW5Pejhn?=
 =?utf-8?B?aTNjTzZmaW5GNkJSM01YSkRQZERxSWNGYzhSNkRhR0dRVEZyVytSZklOSVJT?=
 =?utf-8?B?am1tK2RWN05wQ2tkdDVDa1ZEWkxuU2dJLzArR0djekVmaytDWTNCU0JLS01T?=
 =?utf-8?B?cVB4QjhDaEpwR1BCY2tVUnBuWGN5YXNTbEZtZ1J1azMrTzFOZllPK1pGNUx2?=
 =?utf-8?B?OWplcXNUVUlIOU5tZXFwbUg5VHlTTEY0K2kzU2xSRjJYakMxSmlHM1gwbFE1?=
 =?utf-8?B?S01GWmxJSkt6eStxSjJHdG82cldxeWs4VVoxY1hRU25yUDh4UjFud0tWQzVj?=
 =?utf-8?B?OERValFVUU01VklNenhXUmRoWitGdjY2Y25WYlpYc3RzZzJBZ1Jzcm5mdG1z?=
 =?utf-8?B?RUZSVGtYQTQvU1J2bnpmcStzTGNJL1hRVnducSt0S3ZVTlVQWm0xK3Vzek5W?=
 =?utf-8?B?Y1JvckxqQXA3bmRWM04vRE9US25XbEdobFVrNXdTSG10SHhVYzNjeGQwVDFr?=
 =?utf-8?B?dDNMeTFsM3BuaURiVlNlQWUxUGlxbDBxRXdEb1RsQUd4RXkxM1dyalR6MWlS?=
 =?utf-8?B?dm1HWi93L3J1TXNUNkx2SW9OV0wrbExhUWRhVXdIR0tzZWpoQllIVTIwUnov?=
 =?utf-8?B?UnlIdnhWUGZybnNwd0U0THFuTGVxVVJVclFTRDV2cTVEcExubWJrWDdmK1JB?=
 =?utf-8?B?cnZOSzhJUGIxRzlKQTJJNkZ2L3M2UmhDR3EvdjRPNCtZREhnWmttSnJ2S1dh?=
 =?utf-8?Q?wXbJFSmhBKjTd8ihil1SgvH8s?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2c2db193-1681-421b-f8f7-08dc582d393b
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Apr 2024 00:37:23.8941
 (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: P68GTJ8UDF7pGJ8am0YWv3zWSfCLNhes7ZcHtn6ZNn0wgXU3EgtT4FmBrK4x5rVZjIUj4AqHbGcihbfRV9phFw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB8051
X-OriginatorOrg: intel.com


On 26/02/2024 9:25 pm, Yamahata, Isaku wrote:
> +struct tdx_enabled {
> +	cpumask_var_t enabled;
> +	atomic_t err;
> +};
> +
> +static void __init tdx_on(void *_enable)
> +{
> +	struct tdx_enabled *enable = _enable;
> +	int r;
> +
> +	r = vmx_hardware_enable();
> +	if (!r) {
> +		cpumask_set_cpu(smp_processor_id(), enable->enabled);
> +		r = tdx_cpu_enable();
> +	}
> +	if (r)
> +		atomic_set(&enable->err, r);
> +}
> +
> +static void __init vmx_off(void *_enabled)
> +{
> +	cpumask_var_t *enabled = (cpumask_var_t *)_enabled;
> +
> +	if (cpumask_test_cpu(smp_processor_id(), *enabled))
> +		vmx_hardware_disable();
> +}
> +
> +int __init tdx_hardware_setup(struct kvm_x86_ops *x86_ops)
> +{
> +	struct tdx_enabled enable = {
> +		.err = ATOMIC_INIT(0),
> +	};
> +	int r = 0;
> +
> +	if (!enable_ept) {
> +		pr_warn("Cannot enable TDX with EPT disabled\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!zalloc_cpumask_var(&enable.enabled, GFP_KERNEL)) {
> +		r = -ENOMEM;
> +		goto out;
> +	}
> +
> +	/* tdx_enable() in tdx_module_setup() requires cpus lock. */
> +	cpus_read_lock();
> +	on_each_cpu(tdx_on, &enable, true); /* TDX requires vmxon. */
> +	r = atomic_read(&enable.err);
> +	if (!r)
> +		r = tdx_module_setup();
> +	else
> +		r = -EIO;

I was thinking why do we need to convert to -EIO.

Convert error code to -EIO unconditionally would cause the original 
error code being lost.  Although given tdx_on() is called on all online 
cpus in parallel, the @enable.err could be imprecise anyway, the 
explicit conversion seems not quite reasonable to be done _here_.

I think it would be more reasonable to explicitly set the error code to 
-EIO in tdx_on(), where we _exactly_ know what went wrong and can still 
possibly do something before losing the error code.

E.g., we can dump the error code to the user, but looks both 
vmx_hardware_enable() and tdx_cpu_enable() will do so already so we can 
safely lose the error code there.

We can perhaps add a comment to point this out before losing the error 
code if that's better:

	/*
	 * Both vmx_hardware_enable() and tdx_cpu_enable() print error
	 * message when they fail.  Just convert the error code to -EIO
	 * when multiple cpu fault the @err cannot be used to precisely
	 * record the error code for them anyway.
	 */

> +	on_each_cpu(vmx_off, &enable.enabled, true);
> +	cpus_read_unlock();
> +	free_cpumask_var(enable.enabled);
> +
> +out:
> +	return r;
> +}