Received-SPF: pass (google.com: domain of linux-kernel+bounces-109460-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <46638b78-eb75-4ab4-af7e-b3cad0d00d7b@intel.com>
Date: Thu, 21 Mar 2024 11:37:58 +1300
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v19 029/130] KVM: TDX: Add C wrapper functions for
 SEAMCALLs to the TDX module
Content-Language: en-US
To: Isaku Yamahata <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>, <chen.bo@intel.com>, <hang.yuan@intel.com>,
	<tina.zhang@intel.com>, Sean Christopherson
	<sean.j.christopherson@intel.com>, Binbin Wu <binbin.wu@linux.intel.com>,
	Yuan Yao <yuan.yao@intel.com>, <isaku.yamahata@linux.intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <7cfd33d896fce7b49bcf4b7179d0ded22c06b8c2.1708933498.git.isaku.yamahata@intel.com>
 <579cb765-8a5e-4058-bc1d-9de7ac4b95d1@intel.com>
 <20240320213600.GI1994522@ls.amr.corp.intel.com>
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <20240320213600.GI1994522@ls.amr.corp.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?VEgrbGdMUG1CcXVOYjZWL0dMMjNuQ3d6bUZvU3FDRzBkMWoydm1XR0tqQ3Bk?=
 =?utf-8?B?U0dzUlJxelI0NW1BTkJhK0lzZTFudEZkQUY2eVBicDNkbFpocGMrY1BVc0sw?=
 =?utf-8?B?eElNQUlBNmtMUUp4ZEhpODdmRjlYbHR6c1E3eVpXR2xxNDFoS1JhMkp3bTMv?=
 =?utf-8?B?ZUVHYlo3cmFTUFdqNU9XWFBSM1Y5cmZyUVlFdWUwOS96VE5BYnpZUGtVdVFS?=
 =?utf-8?B?Q2JRQVRXWkR0cDQ1c3ppaW1MQVdpdkFtWFk0bzRZV0ZJREFOS1VIY1E4K0NM?=
 =?utf-8?B?V09tcW5lR1MrK2xacTF6RW52Z2RlUXlLdktEWk9tYWp2WWY4OFFUbVV0QmZr?=
 =?utf-8?B?RnlZYXVlaXI2T2dFdkNQd0I4WmRUS1RMQ3I4TmFGRmd3NkJUb00wTFYzTndp?=
 =?utf-8?B?aXJQbndvSEpPWFZpeEZoZVBqR3lFVTEyMWRJcVhiMWtWV0t1SisyV3E5T0tS?=
 =?utf-8?B?UFFxblZsaHpMRkZWQnJLOVJNTkhoZGdUWG5FTzFFYjRqV1pOTzFBYUtQc29k?=
 =?utf-8?B?WjMvWjdwU25RMG9pcFlBK21LcTVIUWVhYVNKaUJaLzZiKzdWdWx6ZTRGOWpD?=
 =?utf-8?B?NEl3STgrSXVVZFBleHJ0czQxT3p5WStBOGZrZXpKSS9kZnJOb0c3Wi96L3M2?=
 =?utf-8?B?c0dIWXQ5U2NIUDFONnBWcDJJWm5vY3hENlRLVTdaamlJL09iMGlkNGZRMzBB?=
 =?utf-8?B?SWxSL1FmWi9vdFlwbktKYWFndS9rRlpMRThXUWVQTkhBSGNmQjhKMjJWc1ZE?=
 =?utf-8?B?UCtSdTRuaG9LT2tScENRTjN3M0ljeTRnRWRuY2dkTWllZTBCRGxMOTFQclVk?=
 =?utf-8?B?YjFvUlhPR3puK0dCQmdtVjlaRFBTT3ViWi9UcVNrcExGQkh4VTRodml0MUly?=
 =?utf-8?B?alpaekk2dTFtaGZOeGI4L3Y5c0VhZmZsMDNHYllhZWR2ZkhLdWFsRHpSSkRX?=
 =?utf-8?B?UVRTTWRxYWZzZ1ZWZmkzS1pXaDhFeWptckxEa2tseWN1SDdoazFRNmEzeVI5?=
 =?utf-8?B?SFRrdWZ4dU9JengrQ09PUkJhR0N3d2pWWVYwZU1jZUhCMFpaVUlWRzdZVmpE?=
 =?utf-8?B?dnZtRXBOZFEwTThvUE1jeEdsenIyaVdJOEJ1T1MvTTJVZEFBb0Jra1RUWlk3?=
 =?utf-8?B?dnQ2L25yYi9VcHdoY3RNMFdDOGlPeFZMWEpKWEI1Ukw4cmUyRjZITEMrYzRF?=
 =?utf-8?B?NlVCUEJGTUY3bWtjd283dHFCT2J1L2NNNzg3VnBLbDNRYjM0bVdoUTlCZjl1?=
 =?utf-8?B?UkpGbGJ2R1VhVGFsOXdmUkZ0ODFwalBBNk1uUVJ5MGVtYXFDcUp6aURnejF1?=
 =?utf-8?B?NEV2WUI4N0xOVjZ1M2VvOXFjSVpJUHhQUDVGNUg2MWYwT3JyOWtreEF2WjZt?=
 =?utf-8?B?cExhL2NPWldKV2MzSjcxNk9YQkhJbS9JUU9UejE4elBVdkxCRk5rL0xKdUFE?=
 =?utf-8?B?bk16T1JEV3NGVUZRYVdlRjRKL3poRkFQM1FzSzhCRjFqVStxVGtzNitzLzdi?=
 =?utf-8?B?T1k1bTRJQUZpWEhvdGxWQXh2MkFPUEdMRzZrVjdnK0RwOTRjbmNyQ3pCOFdG?=
 =?utf-8?B?ckczVDJheUdDM2RHRUc3R2UwaFpTOWZFSEZVSERQYWdwb3l2R25ZU1V6T1RI?=
 =?utf-8?B?NDhMK3ZPbWRPRTBwNURhcnVtRmxXNXdzYVoraEVrTTBiY3prZU5BT1VzVmVs?=
 =?utf-8?B?L3E4dGtRdHlKeFh4QkozL1RsVGJjVUZJc0V1cHBhV2owRjYvdXk4b3BNOVlq?=
 =?utf-8?B?cTZVUjBKTTVxbUxJd2JTaG5NMWxSK1lMcnhkRjMvWWF0ZysrWnRrU2VkYmY3?=
 =?utf-8?B?NUpzbVpHYjA4a1BSNlovY3psSHYwVGFSbTk2VUFvMGltcUs5VUQ1M3hYQjdN?=
 =?utf-8?B?OXpSYWs4bTlzTSttdEJ1cFowOGpYM0tTNy9SZjlld1hmdUowVXFHYml6QnhP?=
 =?utf-8?B?OEFHWUxtN0JtdWtJL3pmaFlDWnc3b0lYSGNYcXlncmpiS2RlUFcvR2FiZ29u?=
 =?utf-8?B?clBxZ3VVK3FaM2x5MnVSdkZXUUwzZEN4dHlrWnhzRnI0WGhscjBLOTFaY0NL?=
 =?utf-8?B?WWpiQUFFNGxaS3pYYitnSXFQTnhYY25yaUtiaDQ0STZYVlA3T2s1TnJFRTRE?=
 =?utf-8?Q?jqePO0x/ptNolfmtwyeBGntuM?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2f174984-0ccd-4378-b75f-08dc492e6b10
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Mar 2024 22:38:09.4464
 (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: sk/9z3Ep/kcSZ+IO2LWgvQzgwYcCSDiTqKCAENwycEeDVZYo5iTyVn85Ot1nbHdV1Zl9B6+3Gj+apgtUyPzX7w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB6419
X-OriginatorOrg: intel.com


On 21/03/2024 10:36 am, Isaku Yamahata wrote:
> On Wed, Mar 20, 2024 at 01:03:21PM +1300,
> "Huang, Kai" <kai.huang@intel.com> wrote:
> 
>>> +static inline u64 tdx_seamcall(u64 op, struct tdx_module_args *in,
>>> +			       struct tdx_module_args *out)
>>> +{
>>> +	u64 ret;
>>> +
>>> +	if (out) {
>>> +		*out = *in;
>>> +		ret = seamcall_ret(op, out);
>>> +	} else
>>> +		ret = seamcall(op, in);
>>
>> I think it's silly to have the @out argument in this way.
>>
>> What is the main reason to still have it?
>>
>> Yeah we used to have the @out in __seamcall() assembly function.  The
>> assembly code checks the @out and skips copying registers to @out when it is
>> NULL.
>>
>> But it got removed when we tried to unify the assembly for TDCALL/TDVMCALL
>> and SEAMCALL to have a *SINGLE* assembly macro.
>>
>> https://lore.kernel.org/lkml/cover.1692096753.git.kai.huang@intel.com/
>>
>> To me that means we should just accept the fact we will always have a valid
>> @out.
>>
>> But there might be some case that you _obviously_ need the @out and I
>> missed?
> 
> As I replied at [1], those four wrappers need to return values.
> The first three on error, the last one on success.
> 
>    [1] https://lore.kernel.org/kvm/20240320202040.GH1994522@ls.amr.corp.intel.com/
> 
>    tdh_mem_sept_add(kvm_tdx, gpa, tdx_level, hpa, &entry, &level_state);
>    tdh_mem_page_aug(kvm_tdx, gpa, hpa, &entry, &level_state);
>    tdh_mem_page_remove(kvm_tdx, gpa, tdx_level, &entry, &level_state);
>    u64 tdh_vp_rd(struct vcpu_tdx *tdx, u64 field, u64 *value)
> 
> We can delete out from other wrappers.

Ah, OK.  I got you don't want to invent separate wrappers for each 
seamcall() variants like:

  - tdx_seamcall(u64 fn, struct tdx_module_args *args);
  - tdx_seamcall_ret(u64 fn, struct tdx_module_args *args);
  - tdx_seamcall_saved_ret(u64 fn, struct tdx_module_args *args);

To be honest I found they were kinda annoying myself during the "unify 
TDCALL/SEAMCALL and TDVMCALL assembly" patchset.

But life is hard...

And given (it seems) we are going to remove kvm_spurious_fault(), I 
think the tdx_seamcall() variants are just very simple wrapper of plain 
seamcall() variants.

So how about we have some macros:

static inline bool is_seamcall_err_kernel_defined(u64 err)
{
	return err & TDX_SW_ERROR;
}

#define TDX_KVM_SEAMCALL(_kvm, _seamcall_func, _fn, _args)	\
	({				\
		u64 _ret = _seamcall_func(_fn, _args);
		KVM_BUG_ON(_kvm, is_seamcall_err_kernel_defined(_ret));
		_ret;
	})

#define tdx_kvm_seamcall(_kvm, _fn, _args)	\
	TDX_KVM_SEAMCALL(_kvm, seamcall, _fn, _args)

#define tdx_kvm_seamcall_ret(_kvm, _fn, _args)	\
	TDX_KVM_SEAMCALL(_kvm, seamcall_ret, _fn, _args)

#define tdx_kvm_seamcall_saved_ret(_kvm, _fn, _args)	\
	TDX_KVM_SEAMCALL(_kvm, seamcall_saved_ret, _fn, _args)

This is consistent with what we have in TDX host code, and this handles 
NO_ENTROPY error internally.

Or, maybe we can just use the seamcall_ret() for ALL SEAMCALLs, except 
using seamcall_saved_ret() for TDH.VP.ENTER.

u64 tdx_kvm_seamcall(sruct kvm*kvm, u64 fn,
			struct tdx_module_args *args)
{
	u64 ret = seamcall_ret(fn, args);

	KVM_BUG_ON(kvm, is_seamcall_err_kernel_defined(ret);

	return ret;
}

IIUC this at least should give us a single tdx_kvm_seamcall() API for 
majority (99%) code sites?

And obviously I'd like other people to weigh in too.

> Because only TDH.MNG.CREATE() and TDH.MNG.ADDCX() can return TDX_RND_NO_ENTROPY, > we can use __seamcall().  The TDX spec doesn't guarantee such error code
> convention.  It's very unlikely, though.

I don't quite follow the "convention" part.  Can you elaborate?

NO_ENTROPY is already handled in seamcall() variants.  Can we just use 
them directly?

> 
> 
>>> +static inline u64 tdh_sys_lp_shutdown(void)
>>> +{
>>> +	struct tdx_module_args in = {
>>> +	};
>>> +
>>> +	return tdx_seamcall(TDH_SYS_LP_SHUTDOWN, &in, NULL);
>>> +}
>>
>> As Sean already pointed out, I am sure it's/should not used in this series.
>>
>> That being said, I found it's not easy to determine whether one wrapper will
>> be used by this series or not.  The other option is we introduce the
>> wrapper(s) when they get actally used, but I can see (especially at this
>> stage) it's also a apple vs orange question that people may have different
>> preference.
>>
>> Perhaps we can say something like below in changelog ...
>>
>> "
>> Note, not all VM-managing related SEAMCALLs have a wrapper here, but only
>> provide wrappers that are essential to the run the TDX guest with basic
>> feature set.
>> "
>>
>> ... so that people will at least to pay attention to this during the review?
> 
> Makes sense. We can split this patch into other patches that first use the
> wrappers.

Obviously I didn't want to make you do dramatic patchset reorganization, 
so it's up to you.